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

[CP][beta channel] Improve messaging on invalid use of >>> #47037

Closed
johnniwinther opened this issue Aug 29, 2021 · 2 comments
Closed

[CP][beta channel] Improve messaging on invalid use of >>> #47037

johnniwinther opened this issue Aug 29, 2021 · 2 comments
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. cherry-pick-review Issue that need cherry pick triage to approve merge-to-beta

Comments

@johnniwinther
Copy link
Member

commit(s) to merge: 72de10d

merge instructions: https://dart-review.googlesource.com/c/sdk/+/211700 (no merge conflicts, but unittests needed infra changes)

What is the issue: Using the >>> operator in a <2.14 library results in confusing parser errors because >>> is parsed as >> followed by a >, instead of mentioning that >>> needs the triple-shift language feature. Running dart on this program

// @dart=2.13
class Class {
  operator >>>(o) => null;
}
method(int i, int j) {
  i >>> j;
  i >>>= j;  
}

results in these confusing messages

test.dart:3:12: Error: A method declaration needs an explicit list of parameters.
Try adding a parameter list to the method declaration.
  operator >>>(o) => null;
           ^^
test.dart:3:14: Error: Expected '{' before this.
  operator >>>(o) => null;
             ^
test.dart:3:12: Error: Operator '>>' should have exactly one parameter.
  operator >>>(o) => null;
           ^^
test.dart:3:14: Error: Operator declarations must be preceded by the keyword 'operator'.
Try adding the keyword 'operator'.
  operator >>>(o) => null;
             ^
test.dart:6:7: Error: Expected an identifier, but got '>'.
Try inserting an identifier before '>'.
  i >>> j;
      ^
test.dart:7:7: Error: Expected an identifier, but got '>='.
Try inserting an identifier before '>='.
  i >>>= j;
      ^^

What is the fix: The fix is to special case >> immediately followed by > in the parser. This is done just after parsing operator > and just before parsing >> followed by > as a binary expression. At these points the normal message for an unavailable language feature is reported, instead the parser error it would normally have led to.

This means that after the fix, the output for the program above is:

test.dart:4:12: Error: This requires the 'triple-shift' language feature to be enabled.
Try updating your pubspec.yaml to set the minimum SDK constraint to 2.14 or higher, and running 'pub get'.
  operator >>>(o) => null;
           ^^^
test.dart:8:5: Error: This requires the 'triple-shift' language feature to be enabled.
Try updating your pubspec.yaml to set the minimum SDK constraint to 2.14 or higher, and running 'pub get'.
  i >>> j;
    ^^^
test.dart:9:5: Error: This requires the 'triple-shift' language feature to be enabled.
Try updating your pubspec.yaml to set the minimum SDK constraint to 2.14 or higher, and running 'pub get'.
  i >>>= j;
    ^^^^

Why cherrypick: Since the triple-shift feature is launched with the upcoming stable release, the messaging should clearly recognize failed attempts to use the features, and not indicate that the feature is unknown.

Risk: Low-medium risk. The parser change is added at points where parser errors are inevitable, but the error recovery might still result in unwanted cascading messages.

Link to original issue(s): #46886

/cc @kevmoo @mit-mit @whesse @athomas @vsmenon @devoncarew @lrhn @jensjoha

@johnniwinther johnniwinther added area-front-end Use area-front-end for front end / CFE / kernel format related issues. merge-to-beta cherry-pick-review Issue that need cherry pick triage to approve labels Aug 29, 2021
@johnniwinther johnniwinther changed the title [CP][beta channel] Please add descriptive title for the pick here [CP][beta channel] Improve messaging on invalid use of >>> Aug 30, 2021
@devoncarew
Copy link
Member

@johnniwinther - I think the resolution here is that:

  • we don't want to take the cherry pick for stable
  • this should likely be combined with a follow on fix that was found

GIven that we should likely close this issue, and potentially come up with a new cherry pick (after seeing if this is an issue that people do hit?).

@devoncarew
Copy link
Member

Closing this cherry-pick; we can re-formulate as a cp to stable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. cherry-pick-review Issue that need cherry pick triage to approve merge-to-beta
Projects
None yet
Development

No branches or pull requests

2 participants