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.

Sign up for GitHub

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

Jump to bottom

[CP] Patterns parser: prohibit variable/identifier patterns named when/as. #52311

Closed
stereotype441 opened this issue May 8, 2023 · 4 comments
Closed

[CP] Patterns parser: prohibit variable/identifier patterns named when/as. #52311

stereotype441 opened this issue May 8, 2023 · 4 comments
Assignees
@vsmenon @whesse @itsjustkevin @mit-mit @athomas
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable

Comments

@stereotype441
Copy link
Member

Commit(s) to merge

dc639c1

Target

Stable

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/302100

Issue Description

Before the release of Dart 3.0, it was discovered that if the variable name in a variable pattern, or the identifier in an identifier pattern, was when or as, that would lead to a parsing ambiguity which could not be resolved with the current amount of lookahead the parser possesses. This caused parse errors for patterns like:

case Enum.value when !flag:

(The parser would erroneously interpret Enum.value as a type and when as the name of a pattern variable).

To reduce risk to the Dart 3.0 release, we fixed this in a minimal way, by prohibiting the variable name in a variable pattern from being called when or as, but only in those circumstances where the ambiguity arose.

At the same time, we made a change on the main development branch that prohibits when or as from being used as the variable name in any variable pattern, and simultaneously prohibits when or as from being used as the identifier in an identifier pattern. (We also updated the spec in dart-lang/language#3033). This cherry-pick brings over that change to the stable branch, bringing the implementation in line with the spec.

What is the fix

Three new error codes are introduced, to cover the three circumstances in which when or as might be used illegally: in a declared variable pattern, in an assigned variable pattern, or in an identifier pattern. The parser is modified so that it generates these errors in appropriate circumstances.

Why cherry-pick

This change makes the implementation more consistent (both with itself and with the spec), and helps users avoid future trouble by preventing people from writing new code with patterns that refer ambiguously to identifiers named when or as.

If we don't do this cherry-pick, we'll have to accept in perpetuity that Dart 3.0 allows these ambiguous identifier names, and any efforts to prohibit these names in the future will require a language-versioned change (which is significantly more maintenance effort for the Dart team).

Risk

low

Issue link(s)

#52260

Extra Info

No response
@stereotype441 stereotype441 added the cherry-pick-review Issue that need cherry pick triage to approve label May 8, 2023
@kevmoo kevmoo added the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label May 10, 2023
@itsjustkevin
Copy link
Contributor

@vsmenon @leafpetersen @munificent and @mit-mit could you take another look at this cherry-pick

@munificent
Copy link
Member

we'll have to accept in perpetuity that Dart 3.0 allows these ambiguous identifier names, and any efforts to prohibit these names in the future will require a language-versioned change (which is significantly more maintenance effort for the Dart team).

I mean, we could ship it as a non-versioned breaking change.

But, yes, I think getting this fix in sooner and closing the barn door before the horse gets out seems like a good idea.

@mit-mit
Copy link
Member

mit-mit commented May 12, 2023

Picking SGTM

@itsjustkevin itsjustkevin added merge-to-stable cherry-pick-approved Label for approved cherrypick request labels May 12, 2023
@leafpetersen
Copy link
Member

LGTM

copybara-service bot pushed a commit that referenced this issue May 12, 2023
@stereotype441
[stable] Patterns parser: prohibit variable/identifier patterns named…
2f11564 
… when/as.

In https://dart-review.googlesource.com/c/sdk/+/299400, the parser was
adjusted so that it no longer accepts `when` and `as` as the names for
variable patterns in cases where there is a possible ambiguity
(e.g. `int when` is not accepted as a pattern because `int` is a
legitimate pattern, therefore `when` could introduce a guard
clause). This change further prohibits `when` and `as` from being the
names of variable patterns or identifier patterns even in the case
where there is no ambiguity. This is in line with the discussion at
#52199 (comment),
and the spec change at
dart-lang/language#3033.

Three new error codes are introduced, to cover the three circumstances
in which `when` or `as` might be used illegally: in a declared
variable pattern, in an assigned variable pattern, or in an identifier
pattern. I've also added analyzer tests to ensure that the parser
recovers from these errors nicely. Unfortunately, nice error recovery
is only feasible in the non-ambiguous cases.

I've also updated the language test expectations in
`tests/language/patterns/version_2_32_changes_error_test.dart` to
reflect the new error messages, and added a few more examples of uses
of `when` and `as` that are still permitted.

Fixes: #52311

Bug: #52260
Change-Id: I30cb3a1ca627c6db3d281740fb59de74c4118c2b
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/301482
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/302100
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Jens Johansen <jensj@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
@itsjustkevin itsjustkevin added the cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. label May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vsmenon vsmenon

@whesse whesse

@itsjustkevin itsjustkevin

@mit-mit mit-mit

@athomas athomas

Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@kevmoo @munificent @stereotype441 @vsmenon @whesse @leafpetersen @itsjustkevin @mit-mit @athomas