Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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`.
Co-authored-by: Jamie Thompson <bishbashboshjt@gmail.com>
@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
@dwijnand dwijnand requested a review from bishabosha November 5, 2022 10:04
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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
8 participants