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

In 3.4 make refutable patterns in a for comprehension an error #18842

Merged

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented Nov 3, 2023

supersedes #16665

Only make refutable patterns in a for comprehension an error, here we have a clear set in stone solution: put case before the pattern.

It is still in the air the ideal solution for pattern val definitions, see https://contributors.scala-lang.org/t/pre-sip-replace-non-sensical-unchecked-annotations/6342/85, so keep those as a warning for now

Release notes

In 3.4 refutable patterns (i.e. patterns that might not match) in a for-comprehension generator must now be preceded by case, or else an error is reported.

e.g.

val xss = List(List(1, 2), List(3))

for y :: ys <- xss do println(y > 1)
//  ^^^^^^^
// error: pattern's type ::[Int] is more specialized than the right hand
// side expression's type List[Int]
// 
// If the narrowing is intentional, this can be communicated by adding
// the `case` keyword before the full pattern, which will result in a filtering
// for expression (using `withFilter`).

.withFilter is also no longer inserted for a pattern in a generator unless prefixed by case, meaning improved ergonomics for many types that do not implement .withFilter

@carlosedp
Copy link

Do this PR supports the direct tuple assignment in a for comprehension?

@nicolasstucki nicolasstucki linked an issue Nov 6, 2023 that may be closed by this pull request
@bishabosha
Copy link
Member Author

Do this PR supports the direct tuple assignment in a for comprehension?

not sure quite what you mean, but e.g.

for (a, b) <- ((1 to 10) zip ('a' to 'z'))
yield s"$a$b"

will desugar to

((1 to 10) zip ('a' to 'z'))
  .map((a, b) => s"$a$b")

instead of

((1 to 10) zip ('a' to 'z'))
  .withFilter({ case (a, b) => true; case _ => false })
  .map((a, b) => s"$a$b")

@bishabosha bishabosha requested a review from sjrd November 6, 2023 09:11
@@ -38,6 +22,22 @@
| If the narrowing is intentional, this can be communicated by adding the `case` keyword before the full pattern,
| which will result in a filtering for expression (using `withFilter`).
| This patch can be rewritten automatically under -rewrite -source 3.2-migration.
-- Error: tests/neg/refutable-pattern-binding-messages.scala:5:14 ------------------------------------------------------
5 | val Positive(p) = 5 // error: refutable extractor
Copy link
Member

Choose a reason for hiding this comment

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

Why did these error messages move? It seems non-optimal, as the messages are not in source order anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

all that was changed was the warning became an error, so perhaps under -Werror true errors print last?

tests/pos/irrefutable.scala Outdated Show resolved Hide resolved
@carlosedp
Copy link

I mean, with this we could write:

  def z = for
      (a, b) <- ZIO.fromOption(Some(1, 2))
    yield (a, b)

Like the behavior with "-source:future" instead of assigning the result first to a variable and then unpack the tuple:

  def y = for
      x <- ZIO.fromOption(Some(1, 2))
      (a, b) = x
    yield (a, b)

@bishabosha bishabosha force-pushed the refutable-pattern-in-for-is-error branch from d10a4b1 to 0bfd343 Compare November 6, 2023 13:51
@bishabosha
Copy link
Member Author

bishabosha commented Nov 6, 2023

I mean, with this we could write:

  def z = for
      (a, b) <- ZIO.fromOption(Some(1, 2))
    yield (a, b)

Like the behavior with "-source:future" instead of assigning the result first to a variable and then unpack the tuple:

  def y = for
      x <- ZIO.fromOption(Some(1, 2))
      (a, b) = x
    yield (a, b)

yes this PR makes the pattern checking for comprehensions behave like -source:future, but now its with -source:3.4 (or newer)

@bishabosha bishabosha requested a review from sjrd November 6, 2023 14:00
@bishabosha bishabosha added the needs-minor-release This PR cannot be merged until the next minor release label Nov 8, 2023
@bishabosha bishabosha merged commit 849ee9c into scala:main Nov 8, 2023
18 checks passed
@bishabosha bishabosha deleted the refutable-pattern-in-for-is-error branch November 8, 2023 16:04
@bishabosha bishabosha added the release-notes Should be mentioned in the release notes label Nov 8, 2023
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 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 release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate filtering for for pat <- xs ; must be irrefutable.
5 participants