scala/scala-asm

Retire this fork

Opened this issue · 17 comments

retronym commented

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 commented

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

retronym commented

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 commented

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 commented

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.

adriaanm commented

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 commented

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

mkurz commented

@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 commented

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

lrytz commented

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 commented

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 commented

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!

mkurz commented

Just to let you guys know, ASM 9.0, which brings support for Java 15 and even Java 16 already just got released:

Maybe scala-asm should upgrade to that version - or maybe this new release helps to finally retire this custom fork?

  • 👍1
lrytz commented

One open question is whether we can drop the re-packaging / shading. If scala-compiler.jar depends on a specific asm version, it might break users that want to use a different asm version.

However we do the same with JLine, so maybe it's fine.

harpocrates commented

What is the current status of this issue? Is this still desired and is now a good time for it? If so, I'd like to take a stab at this over the weekend.

AFAICT, this still needs to be addressed:

scala/scala#6733 ... The open piece of work is to make our PrdConsAnalyzer work without a6a43f7, see discussion

Also, I guess the Scala compiler would need to depend on several libraries: asm, asm-commons, asm-tree, asm-util.

lrytz commented

IMO the main issue is the risk for dependency conflicts, see my last comment. I understand it's only for projects depending on scala-compiler, but this might be more common than expected, and ASM is quite certainly more commonly used than JLine.

dwijnand commented

Given there's an FAQ on it, I assume it's best we keep repackaging the library: https://asm.ow2.io/faq.html#Q15

I would be supportive of that infrastructure existing in the scala/scala build rather than externally, unless there are good reasons for the status quo.

But I thought there were still some trailing changes in our fork that we can't upstream?