Skip to content
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

Migration: consider handling compound assignments where the read type is nullable #38676

Closed
stereotype441 opened this issue Oct 1, 2019 · 2 comments
Assignees
Labels
area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool). NNBD Issues related to NNBD Release
Milestone

Comments

@stereotype441
Copy link
Member

stereotype441 commented Oct 1, 2019

It's not clear what the migration tool should do if it encounters a compound assignment where the read type is nullable, e.g.:

class C {
  C operator+(C other) { ... }
}
void f(C/*?*/ x, C y) {
  x += y;
}

The only way to properly migrate the expression x += y is to first desugar it to x = x + y and then add the appropriate null check to produce x = x! + y. But this can’t be done in the general case where the LHS assignment might be a property access or an index expression, because duplicating the LHS would change semantics.

In https://dart-review.googlesource.com/c/sdk/+/119525 we simply report the situation as a problem for the user to fix manually. But if this comes up frequently enough we may wish to do something different, for example:

  • Go ahead and desugar the expression in cases where it is semantics preserving, or
  • Go ahead and desugar the expression in all cases, and warn the user if semantics might have changed
@stereotype441 stereotype441 added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-nnbd-migration NNBD Issues related to NNBD Release labels Oct 1, 2019
@stereotype441 stereotype441 added this to the Future milestone Oct 1, 2019
@stereotype441
Copy link
Member Author

As of d8fcc65 we're not even reporting this as a problem for the user to fix manually. The user isn't alerted to the problem until they try to run (or analyze) the migrated code.

@stereotype441 stereotype441 self-assigned this Apr 20, 2020
@stereotype441
Copy link
Member Author

There is actually a more general problem here, even for code that doesn't involve nulls. For example, consider:

abstract class _C extends _D {
  _D/*!*/ operator+(int/*!*/ value);
}
abstract class _D {}
_f(_C/*!*/ x, int/*!*/ y) => x += y;

This can't be migrated without desugaring the assignment, because what is needed is to change x += y to x = (x + y) as _C.

dart-bot pushed a commit that referenced this issue May 1, 2020
We produce a diagnostic under the following conditions:

- For compound assignments, if the type read from the LHS is nullable
  (this is illegal after NNBD, and requires user intervention to fix).

- For compound assignments, if the type returned from the combiner is
  not assignable to the LHS (this was required prior to NNBD, but it
  is a stricter condition after migration, both because the LHS might
  have a non-nullable type, and because implicit downcasts are not
  allowed).

- For null-aware assignments, if the type read from the LHS is
  non-nullable (this indicates that once strong mode is enabled, the
  assignment will be dead code).

Bug: #38676
Change-Id: Icb242ba36437e38364ada069880831eb05e3a513
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/145664
Reviewed-by: Mike Fairhurst <mfairhurst@google.com>
dart-bot pushed a commit that referenced this issue May 1, 2020
… nullability

Bug: #38676
Change-Id: I344a9c9ea1e79af864c239dbcc924f78e798a8aa
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/145900
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
dart-bot pushed a commit that referenced this issue May 4, 2020
Bug: #38676
Change-Id: Iab5097359d8de7aa9411f51ee0ec6212c3adb678
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/145920
Reviewed-by: Samuel Rawlins <srawlins@google.com>
@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 area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Nov 16, 2020
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). NNBD Issues related to NNBD Release
Projects
None yet
Development

No branches or pull requests

1 participant