Keep annotation order #15063
Keep annotation order #15063
Conversation
raboof
force-pushed
the
keep-annotation-order-fixing-14743
branch
from
e5a22d2 to
d359e08
Compare
I think it's worth adding a test even if you can't get it to fail for reference here, can you reproduce the problem by manually compiling that file multiple times? |
raboof
force-pushed
the
keep-annotation-order-fixing-14743
branch
from
d359e08 to
4d772b4
Compare
| @Enabled | ||
| @Deprecated |
There was a problem hiding this comment.
Shouldn't the test involve using the same annotation multiple times to trigger use of RepeatableAnnotations, like in scala/scala@954c5d3#diff-8bd66281732306fbd83125f4c1bba7a81797b9ed31a0857afb0b805dd6e1fe6cR258-R262 ?
There was a problem hiding this comment.
I guess that would also be useful to test, though it wasn't the scenario I was aiming to fix: because the 'annotations.groupBy(_.symbol)` doesn't have a predictable order of keys, even regular 'unrepeated' annotations would not necessarily end up in the bytecode in the same order.
There was a problem hiding this comment.
Ah, I didn't realize that this code wasn't simply bypassed when no repeated annotations exist. But yeah, if you could copy the tests that are in the scala 2 PRs that would be nice: to compile multiple files at once (the .java annotation definition and the scala usage) you can create a subdirectory in tests/pos.
Ok, added!
No, unfortunately not... |
compiler/src/dotty/tools/dotc/transform/RepeatableAnnotations.scala
Outdated
Show resolved
Hide resolved
raboof
force-pushed
the
keep-annotation-order-fixing-14743
branch
from
4d772b4 to
793304e
Compare
tests/annotations1/a.scala
Outdated
| @@ -0,0 +1,2 @@ | |||
| class Annot1(s: String) extends scala.annotation.StaticAnnotation | |||
There was a problem hiding this comment.
Are these tests meant to be under tests/pos/ rather than directly under tests/?
tests/annotations1/a.scala
Outdated
| @@ -0,0 +1,2 @@ | |||
| class Annot1(s: String) extends scala.annotation.StaticAnnotation | |||
This change makes sure non-repeated annotations are kept in the order they were found in the source code. The motivation is not necessarily to have them in the original order, but to have them in an order that is deterministic across rebuilds (potentially even across different machines), for reasons discussed further in scala#7661 and the corresponding scala/scala-dev#405 Some integration tests were added in `tests/pos` to be picked up by `IdempotencyCheck.scala`, but unfortunately I haven't successfully reproduced the nondeterminism that way. I didn't see an obvious place for a 'unit test' of this code, I'd be happy to add one when someone can recommend a good place to put it. This is basically the dotty equivalent of scala/scala@954c5d3 Fixes scala#14743
raboof
force-pushed
the
keep-annotation-order-fixing-14743
branch
from
793304e to
414091f
Compare
This change makes sure non-repeated annotations are kept in the order
they were found in the source code.
The motivation is not necessarily to have them in the original order,
but to have them in an order that is deterministic across rebuilds
(potentially even across different machines), for reasons discussed
further in #7661 and the corresponding scala/scala-dev#405
I tried adding an 'integration test' in
tests/posto be picked up byIdempotencyCheck.scala, but unfortunately couldn't reproduce the nondeterminism that way, so didn't end up including it in this commit.I didn't see an obvious place for a 'unit test' of this code, I'd be
happy to add one when someone can recommend a good place to put it.
This is basically the dotty equivalent of
scala/scala@954c5d3
Fixes #14743