Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stabilize order of annotations in the class file #7429

Merged
merged 1 commit into from Nov 19, 2018

Conversation

retronym
Copy link
Member

@retronym retronym commented Nov 14, 2018

Regressed in #6846, which added support for encoding repeated
annotations.

Test failure before replacing groupBy with LinkedHashMap:

$ sbt junit/testOnly scala.tools.nsc.DeterminismTest
...
java.lang.AssertionError: assertion failed: Difference detected between recompiling List(b.scala, Annot1.java) Run:
jardiff -r /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/reference814657788418452571 /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/recompileOutput4882243280168823330

$ jardiff -r /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/reference814657788418452571 /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/recompileOutput4882243280168823330
diff --git a/Test.class.asm b/Test.class.asm
index 98bfd80..a056f9a 100644
--- a/Test.class.asm
+++ b/Test.class.asm
@@ -4,10 +4,10 @@

   // compiled from: b.scala

-  @LAnnot2;(value=java.lang.Object.class)
-
   @LAnnot1;(value="foo")

+  @LAnnot2;(value=java.lang.Object.class)
+
   @Lscala/reflect/ScalaSignature;(bytes="\u0006\u0001u1AAA\u0002\u0001\r!)Q\u0002\u0001C\u0001\u001d\u0009!A+Z:u\u0015\u0005!\u0011a\u0002\u001ff[B$\u0018PP\u0002\u0001'\u0009\u0001q\u0001\u0005\u0002\u0009\u00175\u0009\u0011BC\u0001\u000b\u0003\u0015\u00198-\u00197b\u0013\u0009a\u0011B\u0001\u0004B]f\u0014VMZ\u0001\u0007y%t\u0017\u000e\u001e \u0015\u0003=\u0001\"\u0001\u0005\u0001\u000e\u0003\rAC\u0001\u0001\n\u0016-A\u0011\u0001cE\u0005\u0003)\r\u0011a!\u00118o_R\u0014\u0014!\u0002<bYV,7%A\u0004)\u0009\u0001ARc\u0007\u0009\u0003!eI!AG\u0002\u0003\r\u0005sgn\u001c;2C\u0005a\u0012a\u00014p_\u0002")

@scala-jenkins scala-jenkins added this to the 2.12.8 milestone Nov 14, 2018
@retronym retronym requested a review from hrhino November 14, 2018 07:49
@@ -1436,11 +1436,14 @@ abstract class RefChecks extends Transform {
checkTypeRefBounds(ann.tpe, tree)
}

annots
.map(_.transformArgs(transformTrees))
.groupBy(_.symbol)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR groupBy was the culprit here as it doesn't preserve the original iteration order.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, So call groupBy on a List Will not gives me a ListMap?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it gives back a HashMap currently:

/** Partitions this $coll into a map of ${coll}s according to some discriminator function.
*
* $willForceEvaluation
*
* @param f the discriminator function.
* @tparam K the type of keys returned by the discriminator function.
* @return A map from keys to ${coll}s such that the following invariant holds:
* {{{
* (xs groupBy f)(k) = xs filter (x => f(x) == k)
* }}}
* That is, every key `k` is bound to a $coll of those elements `x`
* for which `f(x)` equals `k`.
*
*/
def groupBy[K](f: A => K): immutable.Map[K, C] = {
val m = mutable.Map.empty[K, Builder[A, C]]
val it = iterator
while (it.hasNext) {
val elem = it.next()
val key = f(elem)
val bldr = m.getOrElseUpdate(key, newSpecificBuilder)
bldr += elem
}
var result = immutable.HashMap.empty[K, C]
val mapIt = m.iterator
while (mapIt.hasNext) {
val (k, v) = mapIt.next()
result = result.updated(k, v.result())
}
result
}

Copy link
Contributor

@He-Pin He-Pin Nov 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just looked into the code too, I think that should change. The groupBy method on LinearSeq should return a ListMap, but which is slow.

Regressed in scala#6846, which added support for encoding repeated
annotations.

Test failure before replacing `groupBy` with `LinkedHashMap`:

```
$ sbt junit/testOnly scala.tools.nsc.DeterminismTest
...
java.lang.AssertionError: assertion failed: Difference detected between recompiling List(b.scala, Annot1.java) Run:
jardiff -r /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/reference814657788418452571 /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/recompileOutput4882243280168823330

$ jardiff -r /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/reference814657788418452571 /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/recompileOutput4882243280168823330
diff --git a/Test.class.asm b/Test.class.asm
index 98bfd80..a056f9a 100644
--- a/Test.class.asm
+++ b/Test.class.asm
@@ -4,10 +4,10 @@

   // compiled from: b.scala

-  @LAnnot2;(value=java.lang.Object.class)
-
   @LAnnot1;(value="foo")

+  @LAnnot2;(value=java.lang.Object.class)
+
   @Lscala/reflect/ScalaSignature;(bytes="\u0006\u0001u1AAA\u0002\u0001\r!)Q\u0002\u0001C\u0001\u001d\u0009!A+Z:u\u0015\u0005!\u0011a\u0002\u001ff[B$\u0018PP\u0002\u0001'\u0009\u0001q\u0001\u0005\u0002\u0009\u00175\u0009\u0011BC\u0001\u000b\u0003\u0015\u00198-\u00197b\u0013\u0009a\u0011B\u0001\u0004B]f\u0014VMZ\u0001\u0007y%t\u0017\u000e\u001e \u0015\u0003=\u0001\"\u0001\u0005\u0001\u000e\u0003\rAC\u0001\u0001\n\u0016-A\u0011\u0001cE\u0005\u0003)\r\u0011a!\u00118o_R\u0014\u0014!\u0002<bYV,7%A\u0004)\u0009\u0001ARc\u0007\u0009\u0003!eI!AG\u0002\u0003\r\u0005sgn\u001c;2C\u0005a\u0012a\u00014p_\u0002")
```

WIP stabilize annotation ordering
@retronym retronym merged commit 25c7215 into scala:2.12.x Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants