scalafix

scalafix copied to clipboard

WIP pr for having reflect3, cli3, testkit3 modules

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

1. tackle part of #1593 first to extract some tests relevant to -reflect to -reflect!
2. 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)
3. accept that this will be a larger PR than the others, including scalafix-reflect_3, scalafix-cli_3 and finally unit3 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?

I don't like option 2 either, but then wouldn't it be the same for core3, rules3 that we've already merged? I just want to understand if there's a difference and what. Between 1 and 3 my thought is what's gonna be more fun or where am I gonna learn more?

1 seems like creating a separate unit-reflect module and moving reflect tests from unit into this one. On Another note I'd like at some point to write some unit tests if possible (not the ones using input/output classes)

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 (reflect3) or by squashing commits and reviewing them one by one (eg. one commit for reflect3, one for cli3, etc)

rebased against master to verify that https://github.com/scalacenter/scalafix/pull/1665 has a positive impact

Rebased against 0.10.4

nearing merging state: down to 4 open, actionable comments

Rebased against 0.10.4

Just realized we had a major regression in ci-3 since that rebase.

It looks like https://github.com/scalacenter/scalafix/pull/1683/commits/ad2f54471f93d8362625ddfae1dcc8d3ed70f085 triggers a (false positive?) failure in ScalafixArgumentsSuite that breaks testing infrastructure for further tests.

$ sbt "unit3Target3/testOnly *ScalafixArgumentsSuite"
...
[info] ScalafixArgumentsSuite:
[info] - ScalafixArguments.evaluate with a semantic rule !!! CANCELED !!! (3 milliseconds)
[info]   org.scalatest.exceptions.TestCanceledException was thrown. (ScalafixArgumentsSuite.scala:83)
[info] - ScalafixArguments.evaluate getting StaleSemanticdb !!! CANCELED !!! (0 milliseconds)
[info]   org.scalatest.exceptions.TestCanceledException was thrown. (ScalafixArgumentsSuite.scala:165)
[info] - ScalafixArguments.evaluate doesn't take into account withMode and withMainCallback !!! CANCELED !!! (0 milliseconds)
[info]   org.scalatest.exceptions.TestCanceledException was thrown. (ScalafixArgumentsSuite.scala:200)
Warning: Unable to read from client, please check on client for futher details of the problem.
Reporter completed abruptly with an exception after receiving event: TestSucceeded(Ordinal(0, 71),ScalafixArgumentsSuite,scalafix.tests.interfaces.ScalafixArgumentsSuite,Some(scalafix.tests.interfaces.ScalafixArgumentsSuite),CommentFileNonAtomic retrieves 2 patches,CommentFileNonAtomic retrieves 2 patches,Vector(),Some(97),Some(IndentedText(- CommentFileNonAtomic retrieves 2 patches,CommentFileNonAtomic retrieves 2 patches,0)),Some(LineInFile(257,ScalafixArgumentsSuite.scala,Some(Please set the environment variable SCALACTIC_FILL_FILE_PATHNAMES to yes at compile time to enable this feature.))),Some(scalafix.tests.interfaces.ScalafixArgumentsSuite),None,pool-1-thread-1-ScalaTest-running-ScalafixArgumentsSuite,1665911264396).
java.net.SocketException: Broken pipe (Write failed)
	at java.net.SocketOutputStream.socketWrite0(Native Method)
	at java.net.SocketOutputStream.socketWrite(SocketOutputStream.java:111)
	at java.net.SocketOutputStream.write(SocketOutputStream.java:155)
	at java.io.ObjectOutputStream$BlockDataOutputStream.drain(ObjectOutputStream.java:1877)
	at java.io.ObjectOutputStream$BlockDataOutputStream.setBlockDataMode(ObjectOutputStream.java:1786)
	at java.io.ObjectOutputStream.writeNonProxyDesc(ObjectOutputStream.java:1286)
	at java.io.ObjectOutputStream.writeClassDesc(ObjectOutputStream.java:1231)
	at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1427)
	at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1178)
	at java.io.ObjectOutputStream.writeFatalException(ObjectOutputStream.java:1577)
	at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:351)
	at org.scalatest.tools.SocketReporter.liftedTree1$1(SocketReporter.scala:46)
	at org.scalatest.tools.SocketReporter.apply(SocketReporter.scala:55)
	at org.scalatest.DispatchReporter.org$scalatest$DispatchReporter$Propagator$$_$run$$anonfun$1(DispatchReporter.scala:249)
	at scala.runtime.function.JProcedure1.apply(JProcedure1.java:15)
	at scala.runtime.function.JProcedure1.apply(JProcedure1.java:10)
	at scala.collection.immutable.List.foreach(List.scala:333)
	at org.scalatest.DispatchReporter$Propagator.run(DispatchReporter.scala:249)
	at java.lang.Thread.run(Thread.java:748)

Removing all cancel() in the suite seems to help... Will investigate

https://github.com/scalameta/scalameta/pull/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/

4.5.13+71-aca5dd6e-SNAPSHOT/ 	Tue Sep 20 07:02:07 UTC 2022 	OK
4.5.13+73-4db513a9-SNAPSHOT/ 	Tue Sep 20 11:37:57 UTC 2022 	KO  

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

    dependencyOverrides += "org.scala-lang" % "scala-compiler" % "2.13.8",
    dependencyOverrides += "org.scala-lang" % "scala-reflect" % "2.13.8",
    dependencyOverrides += "org.scala-lang" % "scalap" % "2.13.8",

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 https://github.com/scala/bug/issues/12650 and https://github.com/scalacenter/scalafix/blob/27089042180078dc3a5f770eda5309e81738f2a5/build.sbt#L242-L245

Next step would be to try a scalameta built against 2.13.10 to confirm/deny that hypothesis.

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 https://github.com/scala/bug/issues/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 cancel()... Any clue @tgodzik ?

I am still puzzled to know which part of the scalameta code is involved when using scalatest's cancel()... Any clue @tgodzik ?

No idea, it's super weird :thinking:

Adding -Xlog:exceptions=info, we can see the deserialization issue (most likely on TestCanceledException since that's the exception thrown on the client-side)

$ sbt -J-Xlog:exceptions=info "unit3Target3/testOnly *ScalafixArgumentsSuite"
...
[70.108s][info][exceptions] Exception <a 'java/io/InvalidClassException'{0x00000000bf525de0}: local class incompatible: stream classdesc serialVersionUID = 1590627031578033034, local class serialVersionUID = 8944294921032069635>
 thrown in interpreter method <{method} {0x00007f3cc84a3ce0} 'readObject' '()Ljava/lang/Object;' in 'java/io/ObjectInputStream'>
 at bci 3 for thread 0x00007f3c2e92b000 (Thread-24)
[70.108s][info][exceptions] Exception <a 'java/io/InvalidClassException'{0x00000000bf525de0}: local class incompatible: stream classdesc serialVersionUID = 1590627031578033034, local class serialVersionUID = 8944294921032069635>
 thrown in interpreter method <{method} {0x00007f3b56599c20} 'react' '()V' in 'org/scalatest/tools/Framework$Skeleton$1$React'>
 at bci 15 for thread 0x00007f3c2e92b000 (Thread-24)

My assumption is that the client and the server sides of scalatest (per fork := true) run with a scala library that has significantly different bytecode for some core classes that TestCanceledException rely on, impacting the dynamic java serialVersionUID generation.

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:

• 2.13.8 in 4.5.13 OK
• 2.13.9 in 4.6.0 KO
• 2.13.10 in 4.6.0-SNAPSHOT OK

Is the eviction effectively different depending on the JVM process (same classpath, different classloader) ?

---

With scalameta 4.6.0

• https://github.com/sbt/sbt/issues/1918

  $ sbt unit3Target3/Test/console                                               
  ...
  Welcome to Scala 3.1.3 (11.0.16, Java OpenJDK 64-Bit Server VM).
  Type in expressions for evaluation. Or try :help.
  
  scala> println(util.Properties.versionString)
  version 2.13.8
  

• println(util.Properties.versionString) reports "2.13.9" in tests

  $ sbt unit3Target3/Test/externalDependencyClasspath
  ...
  [info] * Attributed(/home/brice/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.13.9/scala-library-2.13.9.jar)
  ...
  [info] * Attributed(/home/brice/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scalap/2.13.8/scalap-2.13.8.jar)
  [info] * Attributed(/home/brice/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-reflect/2.13.8/scala-reflect-2.13.8.jar)
  [info] * Attributed(/home/brice/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-compiler/2.13.8/scala-compiler-2.13.8.jar)
  

Is the eviction effectively different depending on the JVM process (same classpath, different classloader) ?

That's indeed the problem.

sbt uses a layered classloader by default, which loads scala libraries defined in scalaInstance ahead, independently of the user classpath(s).

$ sbt "show unit3Target3/scalaInstance"
[info] Scala instance { version label 3.1.3, actual version 3.1.3, library jars:
/home/brice/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala3-library_3/3.1.3/scala3-library_3-3.1.3.jar,
/home/brice/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.13.8/scala-library-2.13.8.jar,
...

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 serialVersionUID :facepalm:

This problem only happens with unit3 (unit2_13 is fine) as overrideScalaVersion does not seem to be honored with Scala 3.

Options by order of preference (but maybe not speed to unblock this PR)

1. wait for scalameta to be published with something else than scala-library:2.13.9
2. Implement manually overrideScalaVersion: force the Scala 2 library version from scalaInstance (as brought by the Scala 3 library version) into the user classpath
3. use ClassLoaderLayeringStrategy.Flat to have scala-library:2.13.9 on both scalatest client and server (perf regression expected)
4. disable forking (other problems expected)

I'll add a commit with (3) for now to continue addressing other comments of this PR.