Skip to content

Migration tool could understand manually lazily initialized fields #44109

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
srawlins opened this issue Nov 8, 2020 · 2 comments
Closed

Migration tool could understand manually lazily initialized fields #44109

srawlins opened this issue Nov 8, 2020 · 2 comments
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 type-enhancement A request for a change that isn't a bug

Comments

@srawlins
Copy link
Member

srawlins commented Nov 8, 2020

It would be great if the tool recognized a manually lazily initialized field:

class C {
  int _i;
  int get i {
    _i ??= ...;
    return _i;
  }
}

In this case , the tool makes _i nullable, and often makes get i nullable as well (depending on how the body is structured).

The big problems come when get i is considered nullable, when it should be non-nullable. I can generally force non-nullability by hinting /*late final*/ int _i;, but I think this would make illegal code (or just warnings) w.r.t. _i ??= ...;. The fix that I would do as a human are, perhaps strangely, to flip the field _i and the getter i, to use lazy initialization:

class C {
  late final int i = _i;
  int get _i {
    var i_ = ...;
    return i_;
  }
}

But I'm not sure that this is what we want the tool to do... sometimes the field and the getter are not adjacent class elements.

This all holds for top-level variables too.

@srawlins srawlins added legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug analyzer-nnbd-migration labels Nov 8, 2020
@stereotype441
Copy link
Member

Personally I think the first step in addressing this would be to have flow analysis's edge builder treat fields as promotable for purposes of understanding user intent (see #40566). If we did this, then it would not try to mark the getter as nullable (because it would see that _i is intended to be non-null after the ??=), so it would produce this:

class C {
  int? _i;
  int get i {
    _i ??= ...;
    return _i!;
  }
}

(Note that the fix builder would still use proper flow analysis, which doesn't promote fields, so the tool would see that a ! is needed on the return statement).

I realize this is not the migration you're asking for, but it's still a lot better than what we do today. In particular, since it doesn't mark the getter as nullable, it won't propagate nullability virally into other parts of the user's code.

Once we've done that I we could circle back and consider more sophisticated logic that would detect this pattern and replace it with a late variable.

@stereotype441 stereotype441 added area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool). and removed analyzer-nnbd-migration legacy-area-analyzer Use area-devexp instead. labels Nov 16, 2020
@stereotype441
Copy link
Member

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 type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

2 participants