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

Prohibit variable and identifier patterns from being named when or as, to simplify parsing. #3033

Merged
merged 1 commit into from
May 3, 2023

Conversation

stereotype441
Copy link
Member

This spec change reflects the discussion at dart-lang/sdk#52199 (comment).

Test cases are here: https://dart-review.googlesource.com/c/sdk/+/300320

@stereotype441
Copy link
Member Author

@munificent I decided not to change the behaviour of when and as in constant patterns that are qualified names, or the behaviour of when in object patterns. Does that seem reasonable to you?

@munificent
Copy link
Member

I would be very tempted to go farther and treat a pattern like an async function body where it establishes a region of code where when and as are treated as reserved words. That seems conceptually simpler to me than having them be contextual keywords only in certain specific contexts with a pattern but not in other contexts. I can't see much good coming from allowing you to use when or as as, say, the name of a type argument or the constant in a relational pattern.

There's also a process question here. Now that the proposal has essentially shipped, does it make sense to update the proposal itself, or should we treat this as a language change with a separate little proposal document for it? I suspect the latter will be easier for the implementers and people writing tests, but I could be wrong.

@stereotype441
Copy link
Member Author

stereotype441 commented May 3, 2023

I would be very tempted to go farther and treat a pattern like an async function body where it establishes a region of code where when and as are treated as reserved words. That seems conceptually simpler to me than having them be contextual keywords only in certain specific contexts with a pattern but not in other contexts. I can't see much good coming from allowing you to use when or as as, say, the name of a type argument or the constant in a relational pattern.

Hmm, I definitely see some value in that from a standpoint of language evolution, since it helps move us toward a world where when and as really are reserved words. (I'm not sure if that's a goal of yours or not). Implementation-wise, though, it's a lot heavier of a change, because tokens are classified by the scanner, not the parser, and the scanner has no way of knowing when it's inside a pattern. So we would have to examine every part of the parser that's reachable from inside a pattern, and update its identifier-handling logic so that it rejects when and as in the appropriate circumstances. In practice this means we have to think about all pattern and expression parsing logic. And if we ever decide to relax the restriction that the expressions inside relational patterns must be constant, we would technically have to think about statement parsing logic too. That's a lot of parser rework to contemplate.

Implementation difficulty aside, I'm also not convinced it would be doing a service to users. If I'm unlucky enough to be the consumer of an API that contains a getter named when, I might have a legitimate reason to write an object pattern like Foo(when: var x?). Or if I'm switching on an enum and one of the enum values is named when, I might still want to be able to write case Foo.when:. And I might not really be in a good position to rename the code I depend on, so my only choice at that point is, don't use patterns. It doesn't seem like it would benefit anyone.

I think it's fine to stop people from naming things when or as at the declaration site, because that's where they're in a good position to choose another name. I could even get behind making a lint that stops people from naming things when or as even in code that doesn't specifically use patterns. But since when and as are still valid names for things that a pattern might want to refer to, I don't think we should stop people from referring to those things unless it's necessary to avoid parser ambiguities (and fortunately, it's not 😃)

There's also a process question here. Now that the proposal has essentially shipped, does it make sense to update the proposal itself, or should we treat this as a language change with a separate little proposal document for it? I suspect the latter will be easier for the implementers and people writing tests, but I could be wrong.

That's a really good question! I don't know how best to handle this.

@munificent
Copy link
Member

Implementation difficulty aside, I'm also not convinced it would be doing a service to users. If I'm unlucky enough to be the consumer of an API that contains a getter named when, I might have a legitimate reason to write an object pattern like Foo(when: var x?). Or if I'm switching on an enum and one of the enum values is named when, I might still want to be able to write case Foo.when:. And I might not really be in a good position to rename the code I depend on, so my only choice at that point is, don't use patterns. It doesn't seem like it would benefit anyone.

Good point! OK, I'm sold on the approach in this PR.

That's a really good question! I don't know how best to handle this.

cc @dart-lang/dart-team

Maybe the larger team has thoughts.

@leafpetersen
Copy link
Member

Maybe the larger team has thoughts.

For the scale of change we seem to be settling on, I'd just suggest landing it as a change to the existing spec (i.e. this PR).

@stereotype441
Copy link
Member Author

Maybe the larger team has thoughts.

For the scale of change we seem to be settling on, I'd just suggest landing it as a change to the existing spec (i.e. this PR).

Works for me! I'll go ahead and land as is.

On a related note:

  • Do you think this change should be language versioned? (My preference is no, since I think breakages will be minimal)
  • Do you think we should go through the breaking change process for this? (Again my preference is no, for the same reasons)
  • If I can get this change done quickly, do you think it would be worth trying to cherry-pick it into the stable release?

@stereotype441 stereotype441 merged commit a620967 into dart-lang:main May 3, 2023
@stereotype441 stereotype441 deleted the patterns-242 branch May 3, 2023 21:31
@leafpetersen
Copy link
Member

On a related note:

  • Do you think this change should be language versioned? (My preference is no, since I think breakages will be minimal)

No.

  • Do you think we should go through the breaking change process for this? (Again my preference is no, for the same reasons)

No.

  • If I can get this change done quickly, do you think it would be worth trying to cherry-pick it into the stable release?

Yes.

@dnfield
Copy link

dnfield commented May 3, 2023

How does this change affect https://pub.dev/documentation/mockito/latest/mockito/when.html?

@stereotype441
Copy link
Member Author

How does this change affect https://pub.dev/documentation/mockito/latest/mockito/when.html?

With the narrower scope we agreed on in #3033 (comment), there's no effect on https://pub.dev/documentation/mockito/latest/mockito/when.html. The only things being prohibited here are:
The ability to declare a local variable named when through the use of a pattern, e.g.:

switch (...) {
  case var when:
    ...
}

The ability to assign to a local variable named when through the use of a pattern, e.g.:

String who, what, where, when, why, how; // This is ok
(who, what, where, when, why, how) = getQuestions(); // This isn't

And the ability to refer to lone (unprefixed) constants named when in a pattern or a switch, e.g.:

const when = 1;
switch (...) {
  case when:
    ...
}

None of which should interfere with the use of when in mockito.

@dnfield
Copy link

dnfield commented May 3, 2023

Thanks for the explanation!

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request May 8, 2023
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 #52260.

Bug: #52260
Change-Id: I229f627aa639659c30b83c74895759207da279f7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/301482
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Jens Johansen <jensj@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request May 12, 2023
… 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants