-
Notifications
You must be signed in to change notification settings - Fork 185
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
scalafix-cli_3 & test dependencies (GSoC 2022) #1650
Conversation
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Show resolved
Hide resolved
scalafix-reflect/src/main/scala/scalafix/internal/reflect/RuleInstrumentation.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala/scalafix/internal/reflect/RuleInstrumentation.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala/scalafix/internal/reflect/RuleInstrumentation.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala/scalafix/internal/reflect/RuleInstrumentation.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see the first little bits of dotty in here!
However, compilation alone won't give many guarantees on this kind of code/PR, so I am wondering if we should
- tackle part of Break up
unit
the test monolith #1593 first to extract some tests relevant to -reflect to -reflect! - just accept that we'll merge something that might be completely broken (and we'll figure it out only in one or 2 weeks when cross-compiling/running
unit3
) - accept that this will be a larger PR than the others, including
scalafix-reflect_3
,scalafix-cli_3
and finallyunit3
where we will know if this code works.
I would tend to favor (3), as long as this PR maintains a good git hygiene. Any other opinion?
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
val defaultMsg = "Multiple compilation errors occurred for Scala 3, " + | ||
"please try to use a CLI version which compiles into Scala 2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be quite abstract/hard to understand for an end-user.
What about something like:
Error compiling rule(s) from source using Scala 3 compiler; to use the Scala 2.x compiler instead, use the corresponding scalafix-cli artifact or force scalafixScalaBinaryVersion to 2.x in your build tool.
It's not perfect as scalafixScalaBinaryVersion is a sbt-scalafix concept, but
- judging by the number of tickets/requests, sbt-scalafix is by far the most used client so far
- build tools will have something similar (although maybe not named exactly the same) if they allow picking up a specific scalafix scala version
- we already have a reference to it in this repo
scalafix/scalafix-rules/src/main/scala-2/scalafix/internal/rule/ExplicitResultTypes.scala
Line 74 in e11610f
s"To fix this problem, either remove ExplicitResultTypes from .scalafix.conf or make sure the scalafixScalaBinaryVersion setting key matches $inputBinaryScalaVersion."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on this tbh, I don't see any problem with requiring users to set a scalafixScalaBinaryVersion
as well. Could be useful to document this config somewhere tho.
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
I don't like option 2 either, but then wouldn't it be the same for 1 seems like creating a separate About 3 I don't mind the pr being bigger, but there's also ways to mitigate it via either branching off from this branch ( |
d18043e
to
9a7c571
Compare
scalafix-cli/src/main/scala-3/scalafix/internal/v1/TPrintImplicits.scala
Outdated
Show resolved
Hide resolved
scalafix-cli/src/main/scala-3/scalafix/internal/v1/TPrintImplicits.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
2f4e960
to
8dff964
Compare
scalafix-reflect/src/main/scala-3/scalafix/internal/reflect/RuleCompiler.scala
Outdated
Show resolved
Hide resolved
7c30911
to
c3456aa
Compare
… well, remove duplications This is the commit message scalacenter#2: build sbt refactor for core and rules, preliminary
nearing merging state: down to 4 open, actionable comments |
Just realized we had a major regression in ci-3 since that rebase. It looks like ad2f544 triggers a (false positive?) failure in ScalafixArgumentsSuite that breaks testing infrastructure for further tests.
Removing all |
scalameta/scalameta#2825 seems to be culprit (incompatible with scalatest_3 3.2.{13,14} for some unknown reason), by the following bisect https://oss.sonatype.org/content/repositories/snapshots/org/scalameta/scalameta_2.13/
|
I tried to force runtime scala libs to 2.13.8 in unit (to counter the transitive effect of bumping scalameta which now depends on them), without any success
I can't extract a repro and I can't get a proper stack trace, but I am wondering if it could be due to breaking changes brought by scala/bug#12650 and Lines 242 to 245 in 2708904
Next step would be to try a scalameta built against 2.13.10 to confirm/deny that hypothesis. |
Using a local build of https://github.com/scalameta/scalameta/commits/6d9b59da427df20f79fa0786fc680e6596da1586 (which uses 2.13.10 to build) does fix the problem, so it does look related to scala/bug#12650. That means that scalameta 4.6.1 should help, so we could just wait for that. I am still puzzled to know which part of the scalameta code is involved when using scalatest's |
No idea, it's super weird 🤔 |
Adding
My assumption is that the client and the server sides of scalatest (per I still don't know why/how (see below), but it seems the scala library on the server-side is stuck to 2.13.8, while the one on the client-side is driven by scalameta:
Is the eviction effectively different depending on the JVM process (same classpath, different classloader) ? With scalameta 4.6.0
|
That's indeed the problem. sbt uses a layered classloader by default, which loads scala libraries defined in
This library is different than the one brought transitively by scalameta-testkit 4.6.0 in the user classpath (2.13.9), which does get loaded in the forked process. Because of the breaking change in 2.13.9, serialization is impacted and the 2 processes can't communicate through classes serialized through a dynamically-generated This problem only happens with unit3 (unit2_13 is fine) as Options by order of preference (but maybe not speed to unblock this PR)
I'll add a commit with (3) for now to continue addressing other comments of this PR. |
fixes ToolClasspathSuite
Using scala.reflect.io in Scala 3 is a recipe for bugs as we should be using the equivalent dotty classes instead
In scala 3's AbstractFileClassLoader, `loadClass` is routing loadClass calls to findClass, causing "attempted duplicate class definition" when loading a rule sharing the same FQN as a previously compiled one. In scala 2, there is no failure but loaded rules could be the result of previous compilations, so returning a fresh classloader each time is probably what we want.
"Error compiling rule(s) from source using Scala 3 compiler; " + | ||
"to use the Scala 2.x compiler instead, use the corresponding " + | ||
"scalafix-cli artifact or force scalafixScalaBinaryVersion " + | ||
"to 2.x in your build tool" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't manage to get that error when trying to compile a rule with quasiquotes with scalafix-cli_3
cs launch -r ivy2local ch.epfl.scala:scalafix-cli_3.1.3:0.10.4+22-b8cd292e+20221018-1241-SNAPSHOT -M scalafix.cli.Cli -- --rules=https://raw.githubusercontent.com/scalacenter/scalafix-named-literal-arguments/fb9f65ee9f09f167651b3d9bfe94a9be76818759/scalafix/rules/src/main/scala/fix/Namedliteralarguments_v1.scala
error: 2 errors
[E0] Any is not a valid result type of an unapply method of an extractor.
[E1] Scala 2 macro cannot be used in Dotty. See https://dotty.epfl.ch/docs/reference/dropped-features/macros.html
To turn this error into a warning, pass -Xignore-scala2-macros to the compiler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added d15af9a
cs launch -r ivy2local ch.epfl.scala:scalafix-cli_3.1.3:0.10.4+23-d15af9a0+20221018-1320-SNAPSHOT -M scalafix.cli.Cli -- --rules=https://raw.githubusercontent.com/scalacenter/scalafix-named-literal-arguments/fb9f65ee9f09f167651b3d9bfe94a9be76818759/scalafix/rules/src/main/scala/fix/Namedliteralarguments_v1.scala
error: 3 errors
[E0] Any is not a valid result type of an unapply method of an extractor.
[E1] Scala 2 macro cannot be used in Dotty. See https://dotty.epfl.ch/docs/reference/dropped-features/macros.html
To turn this error into a warning, pass -Xignore-scala2-macros to the compiler
[E2] Error compiling rule(s) from source using Scala 3 compiler; to use the Scala 2.x compiler instead, use the corresponding scalafix-cli artifact or force scalafixScalaBinaryVersion to 2.x in your build tool
Follows #1629 & #1643
Towards #1680