Skip to content

DisableSyntax: Add special case for asInstanceOf[Matchable] in Scala 3 #2245

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

DisableSyntax: Add special case for asInstanceOf[Matchable] in Scala 3 #2245

wants to merge 11 commits into from

Conversation

subhramit
Copy link
Contributor

Fixes #1763

Note that the fix is based on the comment which mentions that it's acceptable to have false positives. We look at the next tokens after asInstanceOf to check for [Matchable].

I am new to Scala and Scalafix, so please let me know if anything is off.

@subhramit
Copy link
Contributor Author

While trying to execute the test via sbt shell, I keep getting the following:

[IJ]expect3_3_6Target3_3_6 / testOnly -- -z DisableSyntax
[error] stack trace is suppressed; run 'last integration3_3_6 / update' for the full output
[error] (integration3_3_6 / update) lmcoursier.internal.shaded.coursier.error.FetchError$DownloadingArtifacts: Error fetching artifacts:
[error] file:/C:/Users/G15/.m2/repository/com/github/luben/zstd-jni/1.5.6-3/zstd-jni-1.5.6-3.jar: not found: C:\Users\G15\.m2\repository\com\github\luben\zstd-jni\1.5.6-3\zstd-jni-1.5.6-3.jar
[error] Total time: 1 s, completed 15 May 2025, 5:09:59 am

Is there a quick fix for this?

bjaglin
bjaglin reviewed May 15, 2025
View reviewed changes
Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, code & test LGTM functionally. I kicked off CI and added corresponding comments.

While trying to execute the test via sbt shell, I keep getting the following:

[IJ]expect3_3_6Target3_3_6 / testOnly -- -z DisableSyntax
[error] stack trace is suppressed; run 'last integration3_3_6 / update' for the full output
[error] (integration3_3_6 / update) lmcoursier.internal.shaded.coursier.error.FetchError$DownloadingArtifacts: Error fetching artifacts:
[error] file:/C:/Users/G15/.m2/repository/com/github/luben/zstd-jni/1.5.6-3/zstd-jni-1.5.6-3.jar: not found: C:\Users\G15\.m2\repository\com\github\luben\zstd-jni\1.5.6-3\zstd-jni-1.5.6-3.jar
[error] Total time: 1 s, completed 15 May 2025, 5:09:59 am

Is there a quick fix for this?

I am not familiar with this error. That dependency comes from coursier. Did you try to flush the coursier cache? You could try to reproduce the problem with the coursier cli via cs fetch com.github.luben:zstd-jni:1.5.6-3 maybe.

bjaglin
bjaglin reviewed May 15, 2025
View reviewed changes
scalafix-tests/input/src/main/scala-3/test/disableSyntax/MatchableDisableSyntax.scala Outdated
override def equals(obj: Any): Boolean =
obj.asInstanceOf [ Matchable ] match { /* assert: DisableSyntax.asInstanceOf
^^^^^^^^^^^^^
asInstanceOf casts are disabled, use pattern matching instead
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to have the proper message here as well - for that you need to make your token matching logic resilient to whitespaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially thought of taking a reasonable estimate of 10 tokens (thinking it'd suffice for practical purposes) and filtering whitespaces through them, but it seemed like really bad/"magic" code, so tried to implement a more general solution in c8ef86a. Let me know if it looks good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. I will see if I can think of a more idiomatic way to ignore white spaces, but probably not before next week.

Copy link
Contributor Author

@subhramit subhramit May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In case you do come up with something later and lack time, you can comment the sketch and I'd be happy to file a follow-up.

@subhramit
Copy link
Contributor Author

subhramit commented May 15, 2025 •
edited
Loading

Hi @bjaglin, thanks for taking time to look through, the comments were really helpful!

I am not familiar with this error. That dependency comes from coursier. Did you try to flush the coursier cache? You could try to reproduce the problem with the coursier cli via cs fetch com.github.luben:zstd-jni:1.5.6-3 maybe.

As a hack, I manually downloaded the plugin from https://repo1.maven.org/maven2/com/github/luben/zstd-jni/1.5.6-3/ and placed it at the location it was looking for. That fixed it. However, I was getting too many exceptions on Windows, so shifted to GitHub codespaces for the time being (also to run scalafmt).

@subhramit
End with newline
c73dd90 
Signed-off-by: subhramit <[email protected]>
@subhramit subhramit requested a review from bjaglin May 15, 2025 16:36
@subhramit

This comment was marked as resolved.

Sign in to view
@subhramit
Copy link
Contributor Author

subhramit commented May 15, 2025 •
edited
Loading

CI seems to complain for JDK 21 at scalafix.tests.cli.CliSemanticSuite

[info] CliSemanticSuite:
[info] - --classpath ok (1 second, 338 milliseconds)
[info] - --auto-classpath ok *** FAILED *** (6 seconds, 440 milliseconds)
[info]   CommandLineError=4 did not equal Ok=0 error: Unable to load symbol table: D:\a\scalafix\scalafix\scalafix-tests\expect\target\jvm-3.6.4-target3.6.4\test-classes.bak (BaseCliSuite.scala:193)

Any idea what could be causing this? Seems unrelated at first sight.

@bjaglin
Copy link
Collaborator

bjaglin commented May 15, 2025

CI seems to complain for JDK 21 at scalafix.tests.cli.CliSemanticSuite

[info] CliSemanticSuite:
[info] - --classpath ok (1 second, 338 milliseconds)
[info] - --auto-classpath ok *** FAILED *** (6 seconds, 440 milliseconds)
[info]   CommandLineError=4 did not equal Ok=0 error: Unable to load symbol table: D:\a\scalafix\scalafix\scalafix-tests\expect\target\jvm-3.6.4-target3.6.4\test-classes.bak (BaseCliSuite.scala:193)

Any idea what could be causing this? Seems unrelated at first sight.a

Most likely a race condition creating flakiness (the .bak indicates a concurrent compilation during classpath introspection). The windows tests are more flaky on CI for some reasons... I triggered a re-run.

@subhramit
Copy link
Contributor Author

@bjaglin the checks are all green. Is there anything else to be done in the context of this PR?

@bjaglin
Copy link
Collaborator

bjaglin commented May 16, 2025

As I mentioned above, I need to review the implementation, which won't happen in the coming days because I don't have the bandwidth.

@subhramit
Copy link
Contributor Author

As I mentioned above, I need to review the implementation, which won't happen in the coming days because I don't have the bandwidth.

Apologies, I misinterpreted the comment to be in the context of a later enhancement of code quality. Sure, as convenient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjaglin bjaglin Awaiting requested review from bjaglin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow .asInstanceOf[Matchable] as this is erased to Object
2 participants
@subhramit @bjaglin