Retire this fork

[retronym]

Revisit each of our customizations and either find way to make this in the scala/scala repo (as a client of ASM 6.2) or push changes upstream.

Move customization to scala/scala

Changes to scala/scala that enable these reversions in scala-asm

Upstream

That leaves a smaller diff that make sense to upstream.

"Allow setting stack values in analysis frames"

eb6050e / retronym/asm#2

We should upstream this change. It would help if we could motivate it with a standalone ASM analysis that it enables

"Multiple methods for initializing analysis values"

a2791a0 / retronym/asm#3

This change appears to backwards compatible, visitor implementations that only override newValue will be called as before. So hopefully it could be considered upstream.

Once again, a motivating example will help us sell the change. Could we consider submitting AliasingFrame, NullnessFrame NullnessAnalyzer NullnessInterpreter and copyPropagation to asm-commons?

"Call interpreter.copyInstruction consistently"

a6a43f7 / retronym/asm#4

This appears to be a bugfix. Internally in asm, the copyOperation hook is only used in BasicVerifier, which doesn't deal with the DUP* instructions for which we've added extra calls.

Assorted error message improvements

retronym/asm#5
retronym/asm#6

Include the class/method name in "code too large" or "string literal too large" exceptions. Hopefully uncontroversial to upstream.

[lrytz]

Thanks for moving this forward! What would we do about the package name, use jarjar?

[retronym]

It comes down the old rock and hard place: if we shade and embed asm.jar in scala-compiler.jar to avoid the possibility of causing a classpath clash for user code that programatically uses scala-compiler.jar but also uses another library that depends on an incompatible version of ASM, but we prevent them from upgrading to ASM independently of scala-compiler.jar to get a bug fix or feature.

[lrytz]

Being at the root of the ecosystem, I think there is some risk here.

Do we still shade jline? I saw https://github.com/scala/scala/blob/2.13.x/project/JarJar.scala, but couldn't figure out how we actually use it in our build.

[retronym]

For JLine we support both shaded and unshaded use. It's pretty gnarly. To use this trick in the backend we'd need to fence of some sort of BackendInterface containing all references to ASM and ship both a shaded and unshaded version of its implementation.

scala-compiler.jar isn't quite the root of the ecosystem IMO. But nevertheless I share your concerns.

[retronym]

I've submitted the merge requests upstream.

https://gitlab.ow2.org/asm/asm/merge_requests/177
https://gitlab.ow2.org/asm/asm/merge_requests/178
https://gitlab.ow2.org/asm/asm/merge_requests/179
https://gitlab.ow2.org/asm/asm/merge_requests/180
https://gitlab.ow2.org/asm/asm/merge_requests/181
https://gitlab.ow2.org/asm/asm/merge_requests/182

[adriaanm]

For 2.13, we dropped the shaded jline since the repl is split out in core + UI (we need to talk to the spark folks about this too, because the original reason to shade it was that they had an ancient conflicting jline on the classpath)

[mkurz]

@retronym Just curious - since most of your upstream merge requests got merged, is this scala-asm fork still needed?

[mkurz]

@retronym Just curious - since most of your upstream merge requests got merged, is this scala-asm fork still needed?

Answering myself: Comment scala/scala#6733 (comment) contains the answer: vanilla ASM can be used starting with ASM 6.3. Great!

[SethTisue]

(but scala/bug#10717 seems to indicate that a 2.12.x backport is still pending)

[lrytz]

scala/scala#6733 (and current 2.13.x) still uses this fork. The open piece of work is to make our PrdConsAnalyzer work without a6a43f7, see discussion

[lrytz]

There is only 1 patch left (https://github.com/scala/scala-asm/commits/s-7.3.1) in the current scala-asm fork, but it was not accepted upstream, so we need to work around it (https://gitlab.ow2.org/asm/asm/merge_requests/180).

However, with the latest upgrade, they added a class-name based whitelist to allow TraceClassVisitor (and a few others) to use JDK 14 bytecodes even when not running in ASM8_EXPERIMENTAL mode. It's not crucial for core functionality, but we use it in tools and tests. We had to adjust the whitelist in our fork since we use a different package name.

However, changing the whitelist would not work (?) if we shade the classfiles. Related discussion: https://gitlab.ow2.org/asm/asm/issues/317894#note_51814. Maybe we can just start using ASM as a regular dependency of scala-compiler.jar? That would also allow people to upgrade asm without waiting for us. See also google/guice#1198.

[mkurz]

ASM 8.0.1 available. Any change there that helps to finally retire this fork here?

Maybe we can just start using ASM as a regular dependency of scala-compiler.jar? That would also allow people to upgrade asm without waiting for us.

Sounds like a good idea!