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

Restrict syntax of typed patterns #16150

Merged
merged 9 commits into from
Nov 17, 2022
Merged

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Oct 7, 2022

liufengyun and others added 7 commits October 7, 2022 09:47
In Scala 2, a typed pattern `p: T` restricts that `p` can only be a
pattern variable.

In Dotty, scala#6919 allows `p` to be any pattern, in order to support
 pattern matching on generic number literals.

This PR aligns the syntax with Scala 2 by stipulating that in a typed
pattern `p: T`, either

- `p` is a pattern variable, or
- `p` is a number literal
The test case `tests/explicit-nulls/neg/strip.scala` specify that null
unions inside intersection types should work.  After changing the
scrutinee type of the extractor `OrNull` it is no longer the case.

Changing the scrutinee type is still justified because it agrees with
the name as well as the usage in `Types.scala`.

In contrast, in `Typer.scala`, the logic is more clear without using `OrNull`.
@dwijnand dwijnand marked this pull request as ready for review October 7, 2022 14:43
@dwijnand dwijnand requested a review from odersky October 7, 2022 14:43
*/
def pattern1(location: Location = Location.InPattern): Tree =
val p = pattern2()
if in.isColon then
if (isVarPattern(p) || p.isInstanceOf[Number]) && in.isColon then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code does not match the syntax: simple literals can also be strings, characters, or booleans. Also there should be tests that all simple literals can be given a type ascription.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The numeric literal tests are in your genericNumLits, BigFloat, GenericNumLits tests.

@odersky odersky assigned dwijnand and unassigned odersky Oct 15, 2022
@dwijnand dwijnand assigned bishabosha and unassigned dwijnand Nov 5, 2022
Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approve for change to syntax documents

@dwijnand dwijnand assigned odersky and unassigned bishabosha Nov 7, 2022
@dwijnand dwijnand requested a review from odersky November 7, 2022 09:29
@dwijnand dwijnand added this to the 3.3.0-RC1 milestone Nov 14, 2022
@dwijnand
Copy link
Member Author

@odersky happy to go with this, allowing only numeric literals (in addition to pattern variables)? Wouldn't want this to slip to 3.4.0.

@bishabosha bishabosha added the needs-minor-release This PR cannot be merged until the next minor release label Nov 14, 2022
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. But I am not sure what happens with quotes and splices. Don't we need something like

$x : T

? Is that still allowed?

Reassigning to @nicolasstucki to get his input.

@odersky odersky assigned nicolasstucki and unassigned odersky Nov 17, 2022
@dwijnand
Copy link
Member Author

It is, like

x match
  case '{ $x: T } => // match using outer `T`

in tests/pos-macros/i10050.scala.

@dwijnand dwijnand merged commit c5181b9 into scala:main Nov 17, 2022
@dwijnand dwijnand deleted the disable-typed-unapply branch November 17, 2022 12:23
@odersky
Copy link
Contributor

odersky commented Nov 17, 2022

OK, then it's good to go.

jchyb added a commit to jchyb/dotty that referenced this pull request Nov 17, 2022
PR scala#16257 introduced a test for bindings in type annotations.
Simultanously, scala#16150 disallowed the syntax that allowed the test to
parse and compile.
The test should be no longer needed.
@jchyb jchyb mentioned this pull request Nov 17, 2022
@smarter
Copy link
Member

smarter commented Nov 17, 2022

This broke the CI, but @dwijnand is currently looking into it: #16361

nicolasstucki added a commit that referenced this pull request Nov 18, 2022
Should fix CI.
PR #16257 introduced a test for handling bindings in type annotations
inside match cases. Simultaneously, #16150 disallowed the same syntax.
Because of that the test should no longer be necessary, as it only
pertains to the removed feature.
nicolasstucki added a commit to dotty-staging/dotty that referenced this pull request Nov 18, 2022
This kind of patterns were disabled in scala#16150.

Fixes scala#16367
nicolasstucki added a commit to dotty-staging/dotty that referenced this pull request Nov 18, 2022
This kind of patterns were disabled in scala#16150.

Fixes scala#16367
nicolasstucki added a commit that referenced this pull request Nov 18, 2022
This kind of patterns were disabled in #16150.

Fixes #16367
@WojciechMazur
Copy link
Contributor

The following snippet fails in nightly. Bisect points to this PR. Was that behaviour expected?:

@main def Test = 
    val x: Any = 1
    x match
      case x @ (works: Long) => x
      case x @ fails: Int => x 

errror

[error] bisect/test.scala:5:21: '=>' expected, but ':' found
[error]       case x @ fails: Int => x 
[error]         

The snippet is based on a code found in tasty-query by Open CB

@odersky
Copy link
Contributor

odersky commented Dec 8, 2022

I think the error is expected.

@dwijnand
Copy link
Member Author

little-inferno pushed a commit to little-inferno/dotty that referenced this pull request Jan 25, 2023
PR scala#16257 introduced a test for bindings in type annotations.
Simultanously, scala#16150 disallowed the syntax that allowed the test to
parse and compile.
The test should be no longer needed.
little-inferno pushed a commit to little-inferno/dotty that referenced this pull request Jan 25, 2023
This kind of patterns were disabled in scala#16150.

Fixes scala#16367
prolativ added a commit to dotty-staging/dotty that referenced this pull request May 10, 2023
…attern other than a variable or a number literal.

This partially reverts the changes from scala#16150.
This change is motivated by not breaking source compatibility for a number of projects in the Open Community Build.
prolativ added a commit to dotty-staging/dotty that referenced this pull request May 10, 2023
…n other than a variable or a number literal.

This partially reverts the changes from scala#16150.
This change is motivated by not breaking source compatibility for a number of projects in the Open Community Build.
prolativ added a commit to dotty-staging/dotty that referenced this pull request May 10, 2023
…n other than a variable or a number literal.

This partially reverts the changes from scala#16150.
This change is motivated by not breaking source compatibility for a number of projects in the Open Community Build.
Kordyjan added a commit that referenced this pull request May 12, 2023
Raise a warning instead of an error for a type ascription on a pattern
other than a variable or a number literal.

This partially reverts the changes from
#16150.
This change is motivated by not breaking source compatibility for a
number of projects in the Open Community Build.
Kordyjan pushed a commit that referenced this pull request May 12, 2023
…n other than a variable or a number literal.

This partially reverts the changes from #16150.
This change is motivated by not breaking source compatibility for a number of projects in the Open Community Build.
@Kordyjan Kordyjan modified the milestones: 3.3.0-RC1, 3.3.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
8 participants