[backport] Upgrade to scala-asm 6.2

[ElfoLiNk]

Backport #6733 to 2.12.x in order to fix scala/bug#10717

[lrytz]

@retronym how do you estimate the risk for regressions?

[retronym]

I'd be happy to get this change into 2.12.x.

As one test, we do a jardiff analysis to see how this change affects our bootstrapped distribution. I can't find a record of that analysis for the change on the 2.13.x branch, although I recall doing it.

ASM seems to be pretty well tested itself these days.

[adriaanm]

@lrytz could you do this diff and merge if it looks good?

[retronym]

I've published a bootstrapped distribution as 2.12.7-bin-1ee376b-SNAPSHOT

$ git co  79b7f2a
$ git commit --allow-empty -m "restarr"
$ sbt -Dstarr.version=2.12.7-bin-79b7f2a-SNAPSHOT 'setupPublishCore https://scala-ci.typesafe.com/artifactory/scala-pr-validation-snapshots/' clean publish
...
[info]      published scala-library to https://scala-ci.typesafe.com/artifactory/scala-pr-validation-snapshots;build.timestamp=1536716050731/org/scala-lang/scala-library/2.12.7-bin-1ee376b-SNAPSHOT/scala-library-2.12.7-bin-1ee376b-SNAPSHOT.jar

And produced a jardiff:

$ jardiff $(scala-classpath 2.12.7-bin-79b7f2a-SNAPSHOT) $(scala-classpath 2.12.7-bin-1ee376b-SNAPSHOT) | gist --filename pr-7089.diff
WARN: unable to invoke scalap on: /scala/concurrent/duration/package.class: Unexpected failure
WARN: unable to invoke scalap on: /scala/concurrent/duration/package.class: Unexpected failure

https://gist.github.com/25b1cf35e039d8a6fb3ab838e98dfa6f

The diff has only one class of change:

   // access flags 0x1
   public <init>(Lscala/util/matching/Regex$MatchIterator;)V
-    // parameter final  $outer
+    // parameter final synthetic  $outer
     ALOAD 1
     IFNONNULL L0
     ACONST_NULL

What's the cause of that?

[retronym]

The same change occured when the original PR was merged to 2.13.x. I don't think we noticed.

Javac marks its outer parameters with ACC_MANDATED. The outer field itself is ACC_SYNTHETIC.

class Test {
    class Inner {};
}

 $ javac -d /tmp -parameters /tmp/Test.java && javap -v -cp /tmp 'Test$Inner'
Classfile /tmp/Test$Inner.class
  Last modified 12/09/2018; size 320 bytes
  MD5 checksum bd152caf1f1244b3f249316a2662a89f
  Compiled from "Test.java"
class Test$Inner
  minor version: 0
  major version: 52
  flags: ACC_SUPER
Constant pool:
   #1 = Fieldref           #3.#14         // Test$Inner.this$0:LTest;
   #2 = Methodref          #4.#15         // java/lang/Object."<init>":()V
   #3 = Class              #17            // Test$Inner
   #4 = Class              #20            // java/lang/Object
   #5 = Utf8               this$0
   #6 = Utf8               LTest;
   #7 = Utf8               <init>
   #8 = Utf8               (LTest;)V
   #9 = Utf8               Code
  #10 = Utf8               LineNumberTable
  #11 = Utf8               MethodParameters
  #12 = Utf8               SourceFile
  #13 = Utf8               Test.java
  #14 = NameAndType        #5:#6          // this$0:LTest;
  #15 = NameAndType        #7:#21         // "<init>":()V
  #16 = Class              #22            // Test
  #17 = Utf8               Test$Inner
  #18 = Utf8               Inner
  #19 = Utf8               InnerClasses
  #20 = Utf8               java/lang/Object
  #21 = Utf8               ()V
  #22 = Utf8               Test
{
  final Test this$0;
    descriptor: LTest;
    flags: ACC_FINAL, ACC_SYNTHETIC

  Test$Inner(Test);
    descriptor: (LTest;)V
    flags:
    Code:
      stack=2, locals=2, args_size=2
         0: aload_0
         1: aload_1
         2: putfield      #1                  // Field this$0:LTest;
         5: aload_0
         6: invokespecial #2                  // Method java/lang/Object."<init>":()V
         9: return
      LineNumberTable:
        line 2: 0
    MethodParameters:
      Name                           Flags
      this$0                         final mandated
}

@lrytz thoughts?

[retronym]

Related? scala/bug#10880

[retronym]

Oh, my mistake. The baseline for comparison should be a self-bootstrapped build of the base commit to include #6672, which adds the ACC_SYNTHETIC flag to the outer parameter.

[lrytz]

Thanks @retronym - and thanks @ElfoLiNk!

[SethTisue]

this made a bunch of projects become green again in the Scala 2.12-on-JDK-10 community build 🎉

[mkurz]

Scala 2.12.7 is due today. However given that Java 11 will be released next week and a couple of days later ASM 7.0 will be released as well would it make sense to postpone the Scala 2.12.7 release for like 10 days to also pull in ASM 7.0?
Otherwise maybe there will be incompatibilities with Java 11 and Scala 2.12.7... And Scala 2.12.8 is due in February 2019, long time to wait...

[mkurz]

/cc @SethTisue @retronym @lrytz @adriaanm

[adriaanm]

I find it too risky to pull in a major version bump of ASM like that on 2.12. (Let alone that I hope to merge the last PR for 2.12.7 this morning ;-))

We'll definitely look into ASM 7 for 2.13.x, and possibly back port to 2.12.x later. Note that we only officially support Java 8 both on 2.12 and 2.13. We will try to improve JDK 11 support for the 2.12 series over time, but I think Feb is a reasonable time frame given that 2.12.0 will soon have been out for two years.

PS: If you need faster support, we do have commercial offerings for that...

[mkurz]

@adriaanm Thanks, I understand.

[adriaanm]

Ok, thanks for raising this in any case! I don't follow the ASM list myself, so it's a good reminder we should bump soon at least on 2.13.x :-)

[SethTisue]

(scala/community-build#742 can't catch every possible issue, but it does assure that 2.12.7 can't have a JDK compatibility bug as bad as the one that forced the withdrawal of 2.12.5. so there's that.)

[mkurz]

@adriaanm @SethTisue ASM 7.0 beta released! The final release should happen "Problably in a few weeks.". Let's use it in Scala 2.13?

[mkeskells]

Asm 7 provides support for java 11. Java 11 is the next lts version after java 8 so a big deal for corporates
It could be fortuitous if the port is not too big for 2.12 8 as well

[mkurz]

@mkeskells See above comment.

[mkeskells]

@mkurz I saw it - that's why I made the comment

[SethTisue]

is there at least one specific known issue that upgrading to ASM 7 for 2.12.8 would fix...? that would change the benefit vs. risk calculation.

perhaps we could do the upgrade for 2.13.x and then decide later whether known issues justify a backport for 2.12.8

[mkeskells]

there isn't a specific issue that we are looking for, and see what issues occur on 2.13 seems like a good plan, and then its an down to seeing which is harder - maintaining the differences and backport/forward-port issues or upgrading

[mkurz]

ASM 7 released!
https://mail.ow2.org/wws/arc/asm/2018-10/msg00008.html
http://central.maven.org/maven2/org/ow2/asm/asm/7.0/

Would it be nice to have that in 2.13.0-RC1 😉?

[mkurz]

Any chance you upgrade to ASM 7 until 2.13.0-RC1? Is that on your roadmap?

[lrytz]

#7384 :-)

[mkurz]

@lrytz Nice! Thanks!

[xuwei-k]

is there at least one specific known issue that upgrading to ASM 7 for 2.12.8 would fix...?

scala/bug#11372