[annotator] support scala 3.4 irrefutable patterns in for-comprehension #SCL-22274 fixed #660

disordered · 2024-04-29T15:55:27Z

This fixes false missing withFilters error for irrefutable patterns in for comprehensions introduced with Scala 3.4 and Scala 3.0 + -source:future.

Related #SCL-22468

disordered · 2024-04-29T15:58:30Z

scala/scala-impl/src/org/jetbrains/plugins/scala/lang/psi/impl/expr/ScForImpl.scala

-    def needsPatternMatchFilter(pattern: ScPattern): Boolean =
-      !pattern.isIrrefutableFor(if (forDisplay) pattern.expectedType else None)
+    def needsPatternMatchFilter(pattern: ScPattern): Boolean = {
+      val hasSc3Case = pattern.isScala3OrSource3Enabled &&


It currently works without this check, however, with Scala 3, when pattern starts with case, compiler will require the right hand to have withFilter.
I believe this check should stay for this reason.

It seems like the check is required, but currently not covered with tests.
It will be more clear once comment is addressed

disordered · 2024-04-29T16:08:21Z

It doesn't address the case where type narrowing occurs (couldn't figure out how to detect type narrowing):

for y :: ys <- List(List(1), List(2)) do println(y)

IntelliJ will not produce an error for Scala 3.4, but compiler will produce this:

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`).
This patch can be rewritten automatically under -rewrite -source 3.2-migration.
    for y :: ys <- List(List(1), List(2)) do println(y)

and Scala 3.0+ will produce the same but as a warning.

unkarjedy · 2024-04-30T11:06:21Z

Please see this part of our guidelines

Every commit message should reference the YouTrack issue number in format #SCL-XXXXX
Place primary issue number in the end of the first message line
If you have multiple equally-important related issues place them in the end of the first message line
If you have some non-primary, but related issues you can reference them in the commit message body.
In the last commit of a series which fixes an issue, append #SCL-XXXXX fixed to the end of the first message line issue which is fixed. This will close the YouTrack issues automatically when the change is merged into main branch.

Right now the commit message doesn't mention YT issues.
I see the mention in the PR, we can just copy it in the commit itself

unkarjedy · 2024-04-30T11:53:38Z

...scala-impl/test/org/jetbrains/plugins/scala/annotator/ForComprehensionHighlightingTest.scala

+  import Message.Error
+
+  override protected def supportedIn(version: ScalaVersion): Boolean = version >= LatestScalaVersions.Scala_3_0


Let's use the exact version Scala_3_3

Do you mean only to bump to 3.3 or change it to version == LatestScalaVersions.Scala_3_3?
If the latter, why? These tests should pass for all Scala 3 versions (as of right now at least).

EDIT: I just realised there are some incorrect assumptions in the test cases. I'll restructure the tests and then let's take another round on the changes.

unkarjedy · 2024-04-30T11:54:05Z

...scala-impl/test/org/jetbrains/plugins/scala/annotator/ForComprehensionHighlightingTest.scala

+}
+
+class ForComprehensionIrrefutableTest_3_future extends ForComprehensionHighlightingTestBase {


Here as well, let's use the exact version Scala_3_3

Also, to ensure all the cases are covered, maybe let's inherit from ForComprehensionRefutableTest_3 and override needed tests

(same for Scala 3.4 tests)

unkarjedy · 2024-04-30T12:00:20Z

scala/scala-impl/src/org/jetbrains/plugins/scala/lang/psi/impl/expr/ScForImpl.scala

-      !pattern.isIrrefutableFor(if (forDisplay) pattern.expectedType else None)
+    def needsPatternMatchFilter(pattern: ScPattern): Boolean = {
+      val hasSc3Case = pattern.isScala3OrSource3Enabled &&
+        pattern.prevSiblingNotWhitespace.exists(_.getText == ScalaModifier.CASE)


As IntelliJ suggests, please replace _.getText == ScalaModifier.CASE with _.textMatches(ScalaModifier.CASE)

Changed, though, I don't get that suggestion. Possibly, because I don't have access to plugin com.jetbrains.intellij.api.watcher.

Ah, right the inspection should work in internal mode.
https://github.com/JetBrains/intellij-scala/blob/7a050b30da86b846ca88fa92f51f875b7b805c1d/README.md#prerequisites

(optional but recommended)
Enable internal mode in IDEA to get access to helpful internal actions and debug information

disordered · 2024-04-30T12:30:33Z

Please see this part of our guidelines

Every commit message should reference the YouTrack issue number in format #SCL-XXXXX
Place primary issue number in the end of the first message line
If you have multiple equally-important related issues place them in the end of the first message line
If you have some non-primary, but related issues you can reference them in the commit message body.
In the last commit of a series which fixes an issue, append #SCL-XXXXX fixed to the end of the first message line issue which is fixed. This will close the YouTrack issues automatically when the change is merged into main branch.

Right now the commit message doesn't mention YT issues. I see the mention in the PR, we can just copy it in the commit itself

Want me to force update first commit or just touch up the PR title and description?
Also, should I mention the 22468, if it will auto close it? I think that narrowing should be addressed as well, to make intellij highlight the error, but maybe in a separate PR.

unkarjedy · 2024-04-30T14:50:55Z

scala/scala-impl/src/org/jetbrains/plugins/scala/lang/psi/impl/expr/ScForImpl.scala

-    def needsPatternMatchFilter(pattern: ScPattern): Boolean =
-      !pattern.isIrrefutableFor(if (forDisplay) pattern.expectedType else None)
+    def needsPatternMatchFilter(pattern: ScPattern): Boolean = {
+      val hasSc3Case = pattern.isScala3OrSource3Enabled &&


Is OrSource3Enabled part needed in this check?
AFAIU the logic should be only about Scala 3 and not about Scala 2.
pattern.isInScala3File should do.

Or even extract val features = pattern.features and use features.isScala3, features.hasSourceFutureFlag

unkarjedy · 2024-04-30T14:58:07Z

Want me to force update first commit or just touch up the PR title and description?

Force push to your branch should be ok

unkarjedy · 2024-04-30T14:59:54Z

Also, should I mention the 22468, if it will auto close it?

You can omit the "fixed" part and just mention the ticket.
This is still used by our automation

I think that narrowing should be addressed as well, to make intellij highlight the error, but maybe in a separate PR.

Sounds like a good plan.
This issue is less critical compared to irrefutability

…on #SCL-22274 fixed This fixes false missing withFilters error for irrefutable patterns in for comprehensions introduced with Scala 3.4 and Scala 3.0 + -source:future. Related #SCL-22468

disordered · 2024-04-30T15:56:22Z

Alright, all comments should be addressed now (including first commit messaging).
I restructured test inheritance a bit. Also, changed the check logic for withFilter requirement as I realized that bulk of tests are run under Scala 2.10 and was not behaving correctly under 3.3 and 3.4.

Should I do something about it?
One option could be to add @RunWithScalaVersions(Array(...)) on ForComprehensionHighlightingTest. If so, I guess with versions 2.13, 3.3 and 3.4?

unkarjedy · 2024-04-30T16:56:10Z

Thanks for the contribution!
I will manually cherry-pick the changes and run through our internal CI.
We might even include it in 2024.1 EAP/Release

Should I do something about it?
One option could be to add @RunWithScalaVersions(Array(...)) on ForComprehensionHighlightingTest. If so, I guess with versions 2.13, 3.3 and 3.4?

No, thanks. I will deal with it.

disordered commented Apr 29, 2024

View reviewed changes

disordered changed the title ~~[annotator] support scala 3.4 irrefutable patterns in for-comprehension #SCL-22274~~ [annotator] support scala 3.4 irrefutable patterns in for-comprehension #SCL-22274 & #SCL-22468 Apr 29, 2024

unkarjedy requested review from unkarjedy and sugakandrey and removed request for sugakandrey April 30, 2024 11:04

unkarjedy reviewed Apr 30, 2024

View reviewed changes

disordered added 5 commits April 30, 2024 17:44

[annotator] support scala 3.4 irrefutable patterns in for-comprehensi…

f1310bb

…on #SCL-22274 fixed This fixes false missing withFilters error for irrefutable patterns in for comprehensions introduced with Scala 3.4 and Scala 3.0 + -source:future. Related #SCL-22468

Change exists check to use textMatches

01a7dc6

Disable better-monadic-for tests for scala 3+ versions

4d5b85a

Restructure tests, correct scala 3 refutability check

2e8b41c

Don't check for Xsource:3

6e7564d

disordered force-pushed the scl-22468 branch from af33a7b to 6e7564d Compare April 30, 2024 15:48

disordered changed the title ~~[annotator] support scala 3.4 irrefutable patterns in for-comprehension #SCL-22274 & #SCL-22468~~ [annotator] support scala 3.4 irrefutable patterns in for-comprehension #SCL-22274 fixed Apr 30, 2024

unkarjedy closed this May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[annotator] support scala 3.4 irrefutable patterns in for-comprehension #SCL-22274 fixed #660

[annotator] support scala 3.4 irrefutable patterns in for-comprehension #SCL-22274 fixed #660

disordered commented Apr 29, 2024 •

edited

disordered Apr 29, 2024

unkarjedy Apr 30, 2024

disordered commented Apr 29, 2024

unkarjedy commented Apr 30, 2024 •

edited

unkarjedy Apr 30, 2024

disordered Apr 30, 2024 •

edited

unkarjedy Apr 30, 2024

unkarjedy Apr 30, 2024 •

edited

unkarjedy Apr 30, 2024

disordered Apr 30, 2024

unkarjedy Apr 30, 2024

disordered commented Apr 30, 2024 •

edited

unkarjedy Apr 30, 2024 •

edited

unkarjedy commented Apr 30, 2024

unkarjedy commented Apr 30, 2024 •

edited

disordered commented Apr 30, 2024

unkarjedy commented Apr 30, 2024

		import Message.Error

		override protected def supportedIn(version: ScalaVersion): Boolean = version >= LatestScalaVersions.Scala_3_0

		}

		class ForComprehensionIrrefutableTest_3_future extends ForComprehensionHighlightingTestBase {

[annotator] support scala 3.4 irrefutable patterns in for-comprehension #SCL-22274 fixed #660

[annotator] support scala 3.4 irrefutable patterns in for-comprehension #SCL-22274 fixed #660

Conversation

disordered commented Apr 29, 2024 • edited

disordered Apr 29, 2024

Choose a reason for hiding this comment

unkarjedy Apr 30, 2024

Choose a reason for hiding this comment

disordered commented Apr 29, 2024

unkarjedy commented Apr 30, 2024 • edited

unkarjedy Apr 30, 2024

Choose a reason for hiding this comment

disordered Apr 30, 2024 • edited

Choose a reason for hiding this comment

unkarjedy Apr 30, 2024

Choose a reason for hiding this comment

unkarjedy Apr 30, 2024 • edited

Choose a reason for hiding this comment

unkarjedy Apr 30, 2024

Choose a reason for hiding this comment

disordered Apr 30, 2024

Choose a reason for hiding this comment

unkarjedy Apr 30, 2024

Choose a reason for hiding this comment

disordered commented Apr 30, 2024 • edited

unkarjedy Apr 30, 2024 • edited

Choose a reason for hiding this comment

unkarjedy commented Apr 30, 2024

unkarjedy commented Apr 30, 2024 • edited

disordered commented Apr 30, 2024

unkarjedy commented Apr 30, 2024

disordered commented Apr 29, 2024 •

edited

unkarjedy commented Apr 30, 2024 •

edited

disordered Apr 30, 2024 •

edited

unkarjedy Apr 30, 2024 •

edited

disordered commented Apr 30, 2024 •

edited

unkarjedy Apr 30, 2024 •

edited

unkarjedy commented Apr 30, 2024 •

edited