Parser gives bad error messages for >>> in non->>>-enabled code

[lrhn]

For a program like

void main(List<String> arguments) {
  var x = 10 >>> 2;
  print('x: $x');
}

compiling or analyzing with a 2.12 language version gives:

$ dart compile exe bin/testlang.dart
Info: Compiling with sound null safety
bin/testlang.dart:2:16: Error: Expected an identifier, but got '>'.
Try inserting an identifier before '>'.
  var x = 10 >>> 2;
               ^
Error: AOT compilation failed

$ dart analyze
Analyzing testlang...                  1.9s

  error • bin/testlang.dart:2:16 • Expected an identifier. • missing_identifier

1 issue found.

If we compare that to the error message for lacking extension methods:

  error • bin/testlang.dart:1:1 • This requires the 'extension-methods' language feature to be enabled.
          Try updating your pubspec.yaml to set the minimum SDK constraint to 2.6 or higher, and running 'pub
          get'. • experiment_not_enabled

would it be possible to give a similar useful error message for uses of >>> in language versions before it's enabled.

[lrhn]

I'm aware that it's non-trivial because #>>>(2) was technically a valid expression pre 2.13, and introducing >>> as a token changes the meaning of that expression. The program:

// Add or remove to change behavior:
// @dart=2.12
extension on Symbol {
  String operator >(_) => "Greater Than'd";
  String call(_) => "Called";
}
void main() {
  print(#>>>(2)); 
}

changes what it does depending on whether >>> is a single token or >> followed by >.

If that's hard to get around, I think we'll be willing to accept breaking that code. (I can't come up with any other syntax than a > extension invocation on a Symbol where the difference matters, which is also not a compile-time error pre-2.13 anyway).

[vsmenon]

I'm tentatively marking this as August stable - if we do have a fix, we should consider it for cherry-picking.

@johnniwinther @jensjoha - do you have a sense yet on how hard this will be to fix?

[johnniwinther]

The change to implement this is non-trivial and the needed CLs will not be safe to cherry-pick, so I recommend that we punt this for a later release, if we want to do it at all.

The underlying problem is that the choice to interpret >>> as a single token or a sequence of >> and > is performed in the scanner. Changing the scanner to always interpret it as a single token has the consequence of creating the breaking change mentioned by @lrhn. What is even more problematic, for this to be an easy, cherry-pickable fix, is that the CFE and the analyzer uses the fact that >>> never occurs as the way to disallow use of the triple shift operator. If the scanner allows it, we need to add error reporting in all cases where >>> could occur but is not supported by the current language version. A change like this requires changes in many parts of the front-ends and is generally error-prone.

[lrhn]

So, not as easy as it seemed. ☹️

An alternative might be (guessing here) to make the parser recognize operator >> > and similarly easily-detectable mistakes, and error-recover/report it specially?

[jensjoha]

Currently the parser doesn't know about nnbd settings, triple-shift-operator settings etc. So for one thing, making it know about it wouldn't be trivial, for another thing making it know about it seems like a step in the wrong direction.

[lrhn]

Does the parser need to know?
If it sees operator >>> with a single operator token, it's happy.
If it sees operator >> > it's going to be unhappy. If it can then see that there is no space between the >> and > token (and possibly even if it can't see that) it can report then that "the >>> operator is only available in Dart 2.xx or above". That's true whether the current language version is above that or not. If someone triggers it by actually writing operator >> > with spaces ... it's probably still no worse than the current error message shown above.

(Why is the original error message asking for an identifier when any expression would work?)

[mit-mit]

Alternatively, we can focus on making sure that the min SDK constraint is high enough for new apps that these new language features are available "out of the box".

I already made this change for the Dart SDK:
cfc3a65

I could prepare a similar change for the Flutter SDK templates.

[lrhn]

Encouraging people to use higher SDK-constraints is fine, but won't help those who don't.

If you have a 2.12 language version, a 2.13 SDK, and you write operator >>>, you need to get some kind of error.

That's going to happen for a while. Since 2.12 was the last breaking change, there is no urgent need to increment your package's language version to 2.13 (or 2.14) until you actually want to use a feature from 2.13 (which means >>>) or 2.14 (some other feature). Upping the SDK constraint of your package might negatively affect your clients, by forcing them to upgrade. (Do we want Pub to penalize packages that do not require the most recent SDK? If not, which is very likely, then we can't expect people to require the most recent SDK unless they actually need it.)

So, we can't avoid the error.

This issue exists because the current error that you currently get is not that helpful.
It's a generic error coming from failing to parse operator >> > because the 2.12 language version doesn't consider >>> a token.

In order to give a better error message we need to recognize that someone is trying to write the >>> operator in code that is claimed to work prior to that operator being introduced. Which means recognizing the >>> operator before it exists.

Adding >>> as a token when parsing earlier language versions has a cost: It changes the meaning of some (highly speculative) code, which is probably acceptable, but it also, as Johnni pointed out, means that we need to make the feature detection further down the code chain, instead of that code just never seeing >>> unless it validly exists in the language.

So, either we do that, or we somehow special case error recovery for >> followed by > to give error messages talking about >>>.
(I remember our pre-CFE parsers as actually recognizing >>> and saying that it wasn't in the language, then splitting it into the >> or >s it needed to be in, like we do for a >> that turns out to be type-argument end markers.)

[jensjoha]

Parser changed landed.

[johnniwinther]

@vsmenon @mit-mit @lrhn Should we cherry-pick the landed parser changes into the upcoming stable release?

[vsmenon]

@devoncarew to track

@johnniwinther does @jensjoha 's change fix this issue or is there more to follow?

[johnniwinther]

The change improves the message in most use cases. See for instance the errors reported here.

There is no additional work planned.

[devoncarew]

OK, caught up on the thread. FYI, the cherry pick window for stable closes this monday 8/30, and we're very close to the release, so we need to raise our bar for things we'd cherry-pick.

I'd suggest filing a cherry-pick request for this. We'd want to know that it can merge cleanly (or prepare a CL for the merge). We can then discuss on the cherry-pick request the pros and cons of cherry picking.

@jensjoha / @johnniwinther - do you want to file the cherry-pick?

[jensjoha]

I think this can be closed.