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

"why not promoted" logic needs to be updated for field promotion #53102

Open
stereotype441 opened this issue Aug 2, 2023 · 4 comments
Open
Assignees
Labels
area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. fe-analyzer-shared-field-promotion fe-analyzer-shared-flow-analysis

Comments

@stereotype441
Copy link
Member

Consider the following code (assuming Dart language version 3.2):

class C {
  final int? _i;
  C(this._i);
}

class D {
  int? _i;
}

f(C c) {
  if (c._i == null) return;
  print(c._i + 1);
}

main() {
  f(C(0));
}

The CFE issues the following error message when trying to compile this:

../../tmp/proj/test.dart:12:14: Error: Operator '+' cannot be called on 'int?' because it is potentially null.
  print(c._i + 1);
             ^
../../tmp/proj/test.dart:2:14: Context: '_i' refers to a property so it couldn't be promoted.
See http://dart.dev/go/non-promo-property
  final int? _i;
             ^

And the analyzer says:

  error - test.dart:12:14 - The operator '+' can't be unconditionally invoked because the receiver can be 'null'. Try adding a null check to the target ('!'). - unchecked_use_of_nullable_value
           - '_i' refers to a property so it couldn't be promoted.  See http://dart.dev/go/non-promo-property at test.dart:2:14.

It's correct to have errors with this example program, but the messages explaining why are incorrect. The reason _i can't be promoted is not because it's a property; it's because there's another non-final field with the same name in the same file.

The logic that creates these "why not promoted" messages needs to be updated to account for field promotion. Ideally it would be able to distinguish the following situations:

  • The property can't be promoted because it's public.
  • The property can't be promoted because it's a getter.
  • The property can't be promoted because it's a non-final field.
  • The property can't be promoted because there's a getter with the same name elsewhere in the library.
  • The property can't be promoted because there's a non-final field with the same name elsewhere in the library.
  • The property can't be promoted because there's a noSuchMethod forwarder with the same name elsewhere in the library.
@parlough
Copy link
Member

parlough commented Aug 31, 2023

\cc @MaryaBelanger

As part of dart-lang/site-www#4254, since these diagnostic improvements likely won't be included until Dart 3.3 (as I understand it), we'll need to make sure https://dart.dev/tools/non-promotion-reasons is accurate/updated for the promotion changes.

Note that since https://dart.dev/tools/non-promotion-reasons is linked to from the SDK, it will need to reflect the behavior in 3.2 and before 3.2, with some way to distinguish between differences as necessary.

Edit: Crossed out misunderstood timelines.

@stereotype441
Copy link
Member Author

@parlough
Just to avoid confusion here, I want to make it clear that I am planning to do the diagnostic improvements before 3.2, and Marya and I exchanged emails yesterday to coordinate our efforts. (In fact I'm starting the work today!)

I can understand why you might have thought these diagnostic improvements wouldn't be included until Dart 3.3, because yesterday was the cutoff deadline for the Beta-2 branch, and our policy is not to turn on new language features after a Beta-2 release. But that policy doesn't apply to improvements that are outside the scope of the language specification. These diagnostic updates are outside the scope of the language specification because they won't change what programs are accepted by the compiler, and they won't the semantics of any accepted programs; they will just help the user to understand an error better when a program is rejected.

@parlough
Copy link
Member

parlough commented Aug 31, 2023

@stereotype441 I definitely misunderstood the estimated timeline then, thanks so much for clarifying! It's great to hear that these diagnostic improvements can still land in 3.2.

I edited my comment to avoid confusion by others :)

copybara-service bot pushed a commit that referenced this issue Sep 18, 2023
…messages.

This makes it easier to see at a glance which messages need to be
supported by the website.

I've included the new links that I intend to support as part of the
new "field promotion" feature.

Bug: #53102
Change-Id: I67ad47c5a00db9807a6c726677a06427cdbe02c2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/325803
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
@MaryaBelanger
Copy link
Contributor

The logic that creates these "why not promoted" messages needs to be updated to account for field promotion. Ideally it would be able to distinguish the following situations:

The property can't be promoted because it's public.
The property can't be promoted because it's a getter.
The property can't be promoted because it's a non-final field.
The property can't be promoted because there's a getter with the same name elsewhere in the library.
The property can't be promoted because there's a non-final field with the same name elsewhere in the library.
The property can't be promoted because there's a noSuchMethod forwarder with the same name elsewhere in the library.

I think these are all addressed in dart-lang/site-www#5246, I'll request your review there to confirm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. fe-analyzer-shared-field-promotion fe-analyzer-shared-flow-analysis
Projects
None yet
Development

No branches or pull requests

3 participants