Parser confused by prefix "!" in pattern guard

[munificent]

Consider:

case Enum.value when !flag:

The intent is that this is matching the constant Enum.value when flag is false. However, the compiler reports:

temp.dart:8:10: Error: 'Enum.value' can't be used as a type because 'Enum' doesn't refer to an import prefix.
    case Enum.value when !flag: print('ok');
         ^^^^^^^^^^
temp.dart:8:27: Error: Expected ':' before this.
    case Enum.value when !flag: print('ok');
                          ^^^^

What it thinks it is parsing is:

case (Enum.value when)! flag:

That is, a null-assert pattern whose inner pattern is Enum.value when. The latter is then interpreted as a variable pattern whose name is when and whose type is the prefixed name Enum.value.

After that, it sees an unexpected identifier flag and doesn't know what to do. This is a parser bug because there is an unambiguous way to parse this, which is to treat when as a contextual keyword.

But, more broadly, we should probably specify explicitly that when can't be used as an identifier in a pattern at all to avoid subtle or complex parsing.

[munificent]

Note that the type name doesn't have to be qualified. A simpler repro is:

case foo when !flag:

[stereotype441]

Ok, I see the problem. Taking case foo when !flag: as an example, the core issue is that since the parser is event-based, it needs to decide at the point where it sees foo whether it's looking at a variable pattern with a type or an identifier pattern. It can't just try one interpretation and then backtrack if it's wrong, because there's no way to "un-fire" the parser events once it's fired them. But! Thanks to some heroics in the implementation, it has the ability to look ahead and figure out if something looks like a type; and if it does, look at the token(s) after the type.

So what it currently does is:

• If the token sequence starting at foo looks like a type, and the token that follows is an identifier, it assumes it's looking at a variable pattern with a type; otherwise it assumes it's looking at an identifier pattern.
• EXCEPT that if the token that follows the supposed type is when or as (both of which are valid identifiers), then it probes further:
• If the token that follows when or as is a token that could legitimately follow a pattern, then it assumes that it's looking at a variable pattern with a type. (The tokens that could legitimately follow a pattern are ,, :, ||, &&, ), }, ], as, when, ?, !).
• Otherwise it assumes that it's looking at an identifier pattern.

The place where this breaks down is in the third bullet: if the token that follows the supposed type is when, and the token that follows is as, when, or !, then we still haven't resolved the ambiguity, because those tokens could all legitimately start a guard expression. Similarly, if the token that follows the supposed type is as, and the token that follows is when, then we still haven't resolved the ambiguity, because those tokens could all legitimately start a type. So in point of fact we have problems parsing all of the following:

• case foo when as: (where as is a local boolean variable)
• case foo when when: (where when is a local boolean variable)
• case foo when !flag: (the subject of this issue)
• case foo as when: (where when is the name of a type)

I thought I was being so clever when I came up with the disambiguation technique described in the bullets above. Now I wish I had done something like @munificent's suggestion:

But, more broadly, we should probably specify explicitly that when can't be used as an identifier in a pattern at all to avoid subtle or complex parsing.

However, given the late hour, I'm inclined to suggest a more targeted fix, where we simply change the disambiguation rule to:

• If the token sequence starting at foo looks like a type, and the token that follows is an identifier, assume the parser is looking at a variable pattern with a type; otherwise assume it's looking at an identifier pattern.
• EXCEPT that if the token that follows the supposed type is when or as, then assume the parser is looking at an identifier pattern.

(The italicized part is the change).

I'll work on a CL that does this. I'm happy to discuss further if folks feel like this is the wrong approach.

[munificent]

The targeted fix sounds fine to me. By "assume the parser is looking at an identifier pattern" does that mean that if the assumption turns out to be false then it will still treat when as an identifier in a variable pattern?

So given:

case foo when:

Will it parse that as a variable pattern of type foo with name when? I'm OK with the answer being "yes" or "no".

For 3.1, I'm definitely interested in shipping a (probably language versioned) change that says that when simply can't be used as an identifier in a pattern or guard.

[munificent]

I just checked a big corpus of pub packages, open source Flutter apps, open source Flutter widgets, the Dart repo, and the Flutter repo, and I found zero switch cases containing the identifier when. So I think this targeted workaround or even a broader one that always treats when as a keyword is likely to be not problematic for users.

[stereotype441]

The targeted fix sounds fine to me. By "assume the parser is looking at an identifier pattern" does that mean that if the assumption turns out to be false then it will still treat when as an identifier in a variable pattern?

Nope! That's the disadvantage of our event-based parser architecture: once you decide on an interpretation and start consuming tokens, you can't go back and change your mind. So:

case foo when:

will become a parse error.

For 3.1, I'm definitely interested in shipping a (probably language versioned) change that says that when simply can't be used as an identifier in a pattern or guard.

SGTM! I'll get to work on the fix now.

[munificent]

So:

case foo when:

will become a parse error.

That's 100% fine with me. No existing pre-Dart 3.0 code should be broken by this (since no non-pattern switch case constant expressions syntactically look like variable patterns) and I'm fine with saying that you can't use when as a variable name in a switch case pattern.

[stereotype441]

So:

case foo when:

will become a parse error.

That's 100% fine with me. No existing pre-Dart 3.0 code should be broken by this (since no non-pattern switch case constant expressions syntactically look like variable patterns) and I'm fine with saying that you can't use when as a variable name in a switch case pattern.

Just to be clear: the change I'm contemplating also prohibits when and as as typed variable patterns in an irrefutable context, e.g.:

var (int when, int as) = foo(); // ERROR: your variable names are bad and you should feel bad!

I could make the fix only apply in refutable contexts, but it would require some additional plumbing work (and thus be more risky).

[munificent]

Just to be clear: the change I'm contemplating also prohibits when and as as typed variable patterns in an irrefutable context, e.g.:

var (int when, int as) = foo(); // ERROR: your variable names are bad and you should feel bad!

I could make the fix only apply in refutable contexts, but it would require some additional plumbing work (and thus be more risky).

That's also fine. There is no pre-Dart 3.0 code that contains irrefutable patterns, so it's non-breaking. And I don't see any positive value in allowing when or as as variable names in patterns in new code. Hell, I'd make them reserved words if I could (but not right now, obviously).

[munificent]

I went ahead and checked the head commits of the Dart SDK and Flutter repos and I don't see any uses of when as an identifier in a pattern, so I don't think this fix should break anything. (At least, that I could find, in those two repos.)

I did run into many uses of when as a function name from Mockito, but those seem to be unaffected. I also ran into a couple of uses of when as a variable, ironically, in the parser:

// pkg/_fe_analyzer_shared/lib/src/parser/parser_impl.dart:8813
            when = token = next;

// pkg/_fe_analyzer_shared/lib/src/parser/parser_impl.dart:8814
            listener.beginSwitchCaseWhenClause(when);

// pkg/_fe_analyzer_shared/lib/src/parser/parser_impl.dart:8821
          listener.endCaseExpression(caseKeyword, when, token);

// pkg/_fe_analyzer_shared/lib/src/parser/parser_impl.dart:10424
          when = token = next;

As long as the parser doesn't parse all assignments as pattern assignments, this should be OK.

I also don't see any uses of as in patterns that would be problematic.

[stereotype441]

Fix is out for review: https://dart-review.googlesource.com/c/sdk/+/299400