Regression in sbt 1.9.5, likely zinc regression

[mdedetrich]

Reproduction steps

Scala version: N/A (see PR)

Updating an existing project to SBT 1.9.5, see apache/pekko#648

Problem

[error] /home/runner/work/incubator-pekko/incubator-pekko/stream/src/main/scala/org/apache/pekko/stream/impl/io/compression/GzipDecompressor.scala:29:52: Generated class org.apache.pekko.stream.impl.io.compression.createLogic$$anon differs only in case from org.apache.pekko.stream.impl.io.compression.createLogic$$anon (defined in DeflateDecompressor.scala).
[error]   Such classes will overwrite one another on case-insensitive filesystems.
[error]   override def createLogic(attr: Attributes) = new DecompressorParsingLogic {
[error]                                                    ^
[error] /home/runner/work/incubator-pekko/incubator-pekko/stream/src/main/scala/org/apache/pekko/stream/impl/io/compression/GzipDecompressor.scala:40:26: Generated class org.apache.pekko.stream.impl.io.compression.createLogic$$anon$inflating$ differs only in case from org.apache.pekko.stream.impl.io.compression.createLogic$$anon$inflating$ (defined in DeflateDecompressor.scala).
[error]   Such classes will overwrite one another on case-insensitive filesystems.
[error]     override case object inflating extends Inflate(false) with Step
[error]                          ^
[error] two errors found

Original notes

Still need to figure out what the root cause is, basically a project (Pekko) was updated to 1.9.5 and then MiMa started complaining about bincompat issues (see https://github.com/apache/incubator-pekko/actions/runs/6182223720/job/16781910666?pr=648). My initial suspicion/hunch is that it may be due to the new compiler bridge that was added in zinc which was brought in by the new sbt version (see scala/scala#10472)

[SethTisue]

I'm testing sbt 1.9.5 in the Scala 2 community build now — perhaps that will generate some insight.

fyi @lrytz

[mdedetrich]

So I doubt that this is due to the new compiler bridge because that was added for Scala 2.13 and MiMa is complaining about Scala 2.12 artifacts.

I am looking through the various changes put into Zinc recently (which is what new sbt 1.9.5 brings in) and the only one that comes to mind is sbt/zinc#1244 (it seems to be dealing with filenames which is what MiMa is complaining about).

The inliner was also enabled in Zinc for Scala 2.13, but I seriously doubt thats the issue (this would be some serious bug in the inliner and furthermore would only be applicable if Zinc on Scala 2.13 is used to compile Scala 2.12 artifacts which I don't think is the case)

[pjfanning]

The issue occurs with sbt +compile when you compile with latest https://github.com/apache/incubator-pekko (main branch) code but with project/build.properties edited to use sbt 1.9.5. You don't need to do a Mima check to reproduce it.

I checked and it only seems to happen with sbt ++2.12 compile - it does not happen with sbt ++2.13 compile

[SethTisue]

the only one that comes to mind is sbt/zinc#1244

@dwijnand wdyt, plausible culprit?

[He-Pin]

I tested with sbt 1.9.5 and 2.13.11, fails too.

[lrytz]

MiMa started complaining

Just to clarify - is it really MiMa complaining? The error message "... differs only in case from ..." is from the compiler (https://github.com/scala/scala/blob/v2.12.18/src/compiler/scala/tools/nsc/backend/jvm/PostProcessor.scala#L104-L105)

[mdedetrich]

Just to clarify - is it really MiMa complaining? The error message "... differs only in case from ..." is from the compiler (https://github.com/scala/scala/blob/v2.12.18/src/compiler/scala/tools/nsc/backend/jvm/PostProcessor.scala#L104-L105)

Your right, it happened in the MiMa step but it does seem to be the actual Scala compiler

[pjfanning]

@lrytz see #12868 (comment)

[lrytz]

Created scala-steward-org/scala-steward#3167 for now -- I don't know if this would work?

[lrytz]

I can confirm it's caused by sbt/zinc#1244. I'll continue digging, but probably won't have anything done during ScalaDays.

[mdedetrich]

I can confirm it's caused by sbt/zinc#1244. I'll continue digging, but probably won't have anything done during ScalaDays.

Thanks for diagnosing, enjoy the conference!

[SethTisue]

cc @eed3si9n

[eed3si9n]

So at this point it may result in spurious/false warnings, and it might fail builds with fatal warnings?
Let me know how wide the impact (does it affect most builds etc), and if we need to follow up with new Zinc and sbt soon.

[lrytz]

A lot of guessing here from my side as I haven't had time to look in detail, but the interaction is something like this:

We have package p { class C { def m { new anon { def sol } } } }.

The phase travel exitingFlatten added in sbt/zinc#1244 causes the (cached) flatname of ClassSymbol for the anonymous class to be computed after the flatten phase. The owner chain of the symbol is different at this point. So instead of something like p.C$m$anon we end up with only p.m$anon.

The error in pekko is due to two anonymous classes now having the same name accidentally. However, I believe that this has a broad effect, many anonymous classes will get a different name when compiling with the new zinc.

While it's possibly OK in terms of binary compatibiliy (anonymous classes cannot be referenced externally), it's still an unintended wide-reaching change.

It probably should be considered a bug in the compiler, but we cannot change existing compiler releases. The new zinc needs to continue working with existing compiler releases. Therefore I think we need to revert the change in zinc. Since poeple might start using the new sbt and publish libraries with it, we should do that rather quickly and get another sbt release out.

[mdedetrich]

I agree with @lrytz here in that it seems the best course of action is to revert that PR and re-release zinc+sbt, then there is more breathing room to solve that PR properly without causing regressions.

[SethTisue]

community build results:

the 2.13 builds are green

on 2.12, the akka-stream build consistently has a test failure

example failure: https://scala-ci.typesafe.com/job/scala-2.12.x-jdk8-integrate-community-build/8240/artifact/logs/akka-stream-build.log: [akka-stream] [ERROR] [SECURITY][09/14/2023 23:51:01.548] [GzipSpec-akka.test.stream-dispatcher-7] [akka.actor.ActorSystemImpl(GzipSpec)] Uncaught error from thread [GzipSpec-akka.test.stream-dispatcher-7]: Receiver class akka/stream/impl/io/compression/createLogic$$anon$inflating$ must be the current class or a subtype of interface akka/stream/impl/io/compression/createLogic$$anon$Step, shutting down JVM since 'akka.jvm-exit-on-fatal-error' is enabled for ActorSystem[GzipSpec]

note that while the 2.13 build has current Akka, the 2.12 build is on a much older version

@eed3si9n I'm always happy to run new sbt versions through the community build on request, including milestones and RCs

[mdedetrich]

note that while the 2.13 build has current Akka, the 2.12 build is on a much older version

FYI Pekko is also failing on 2.12 but with the latest Scala 2.12 scala version (i.e. 2.12.18) so I don't think the specific Scala 2.12 version makes a difference here.

[eed3si9n]

Since poeple might start using the new sbt and publish libraries with it, we should do that rather quickly and get another sbt release out.

On it.

[eed3si9n]

sbt 1.9.6 is released - https://eed3si9n.com/sbt-1.9.6

I merged sbt/zinc#1244 with a cursory review (it looked ok to me), so I take responsibility on this regression. IT’S ME HI.

[eed3si9n]

Closing this as resolved.

[lrytz]

Thanks Eugene!

I merged sbt/zinc#1244 with a cursory review (it looked ok to me), so I take responsibility on this regression

I don't think anyone would have spotted that during review...

[SethTisue]

@eed3si9n i agree, this was unlikely to have been caught in advance by a human reviewer. but the community build did catch it, so ( later, after we catch our breath) maybe we could work out a way for me to help test sbt changes and/or zinc changes there, hopefully without much extra effort from your side being needed

I’m interested both in a test-all-of-sbt option, and perhaps in a test-new-zinc-with-existing-sbt option as well?

[eed3si9n]

Yea, in general future Zinc changes should probably go through some testing, even when they look somewhat innocuous.

[mkurz]

So... if I just published a bunch of libraries including Play to maven central using sbt 1.9.5 it's better to re-publish them again with 1.9.6? 😢

[eed3si9n]

Yea, I think it's a judgement call since the definition and call site of a lambda is closed within your own library, but it's definitely an odd duck name mangling. Given the wide usage of Play, it might be safe to republish?

[mkurz]

Given the wide usage of Play, it might be safe to republish?

I think so. Anyway, thank you for the excellent and continuous work on sbt 👍

[mdedetrich]

Just wanted to say that SBT 1.9.6 solves the problem with Pekko, so I can confirm that sbt/zinc#1244 was indeed the culprit

[mkurz]

sbt/sbt#7382

sbt 1.9.6 still seems to have the Zinc issue

?

[eed3si9n]

@mkurz That seems to be an unrelated issue from this one.

[mkurz]

Told scala-steward to hold back the already published artifacts, like you did with sbt 1.9.5:

• Hold back artifacts published with sbt 1.9.5 scala-steward-org/scala-steward#3169