Skip to content

2.13.17 regression (?): Scalafix failing in community build #902

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

Open
SethTisue opened this issue Apr 17, 2025 · 3 comments · May be fixed by scala/scala#11048
Open

2.13.17 regression (?): Scalafix failing in community build #902

SethTisue opened this issue Apr 17, 2025 · 3 comments · May be fixed by scala/scala#11048
Milestone

Comments

@SethTisue
Copy link
Member

reproducible locally using dbuild (also probably reproducible outside dbuild if you locally publish scalameta?)

https://scala-ci.typesafe.com/job/scala-2.13.x-jdk17-integrate-community-build/2616/artifact/logs/scalafix-build.log

[info] - scala/test/removeUnused/RemoveUnusedPatternVars.scala *** FAILED *** (98 milliseconds)
--- obtained
+++ expected
@@ -385,3 +385,3 @@
   def f(v: (Int, (Boolean, String))): Int = v match {
-    case (i, v @ (_, _)) => i
+    case (i, (_, _)) => i
   }

I haven't bisected it but it's in this range:

-nightly=2.13.17-bin-483b4ee
+nightly=2.13.17-bin-c392ee5

eyeballing that range, scala/scala#11032 looks like a possible culprit (@som-snytt)

@SethTisue SethTisue added this to the 2.13.17 milestone Apr 17, 2025
@som-snytt
Copy link

som-snytt commented Apr 17, 2025

v is no longer reported because that is the selector expression.

Compare

x match {
  case Some(x) =>
}
// generalizes
x match {
  case x: Some[_] =>
}

Remark at scala/scala#11012 (comment)

I comment that it doesn't do tuples (i, j) match { case (i, j) => } but a possible narrowing of the behavior would be to allow it only if there is exactly one pattern var. Then v would warn because i is present (albeit used).

Dotty is not (yet) so liberal, but considers only a top-level Bind in the pattern. Probably I intended to import the behavior (or the improved behavior). (The dotty check has regressions or other tickets with priority.)

@som-snytt
Copy link

Although underreporting might be better, I don't mind reverting that one LOC to restore the warning. That is, revert to current dotty.

I'll prepare a quick PR for your consideration.

@som-snytt
Copy link

The OP should work as expected, but the PR still allows case Some(x) and similar. In particular, case email(s, _) => for arbitrary extraction of one patvar.

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 a pull request may close this issue.

2 participants