Skip to content

Migration tool should assume assertions enabled. #44292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
stereotype441 opened this issue Nov 23, 2020 · 1 comment
Closed

Migration tool should assume assertions enabled. #44292

stereotype441 opened this issue Nov 23, 2020 · 1 comment
Labels
area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool). P2 A bug or feature request we're likely to work on

Comments

@stereotype441
Copy link
Member

I found this issue while investigating #44280.

Repro steps:

Observed result:

  • lib/get_it_impl.dart lines 329-332 (at the end of the method _GetItImplementation.get) are migrated like this:
    assert(instance is T,
        'Object with name $instanceName has a different type (${instanceFactory.registrationType.toString()}) than the one that is inferred (${T.toString()}) where you call it');

    return instance as T?;
  • As a result, the return type of _GetItImplementation.get is marked as nullable, and consequently, the return type of GetIt.get is marked as nullable.

Expected result:

  • The line assert(instance is T, ...); should be sufficient to indicate to the migration tool that instance is expected to have type T, therefore no ? should be added to the return statement.

This is yet another facet of #40566; since we're currently using flow analysis in the migration tool's edge builder, and flow analysis conservatively assumes the user might be running with assertions disabled, assertions like this aren't taken into account when building the nullability graph. But really, they should be taken into account, because the purpose of the edge builder is to build a nullability graph that reflects the user's intention about what should be nullable and what shouldn't be; and assertions are a valuable source of information about intention regardless of whether they get enabled at runtime or not.

@stereotype441 stereotype441 added P2 A bug or feature request we're likely to work on area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool). labels Nov 23, 2020
@stereotype441
Copy link
Member Author

As of 1c7fe71, the null safety migration tool has been removed from active development and retired. No further work on the tool is planned.

If you still need help, or you believe this issue has been closed in error, please feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool). P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

1 participant