Fix #2797: Don't lift Java annotation arguments out of call #2819

OlivierBlanvillain · 2017-06-28T12:46:07Z

Here is a minimized version, the reproduction requires loading the annotation from a class file:

2797.java:

public @interface Fork {
  int value() default -1;
  String[] jvmArgs() default { "nope" };
}

2797.scala:

@Fork(jvmArgs = Array("I'm", "hot"))
class SexyScalacBenchmark

rm *.class; javac -d . 2797.java; dotc 2797.scala

olafurpg · 2017-06-28T13:06:46Z

Thank you for minimizing @OlivierBlanvillain !

The scala/compiler-benchmark project compiles with this workaround and the unit tests pass. The annotations still seem to be emitted weirdly since I hit a new error when running the jmh benchmarks

Annotation @Param is not applicable to methods.
   [org.openjdk.jmh.generators.reflection.RFMethodInfo@20affcd9]

Annotation @Param is not applicable to methods.
   [org.openjdk.jmh.generators.reflection.RFMethodInfo@133b86b1]

I don't know if that error is related to this issue or not, the @Param annotations is defined on vars here https://github.com/scala/compiler-benchmark/blob/d0f038e0a9f367e26dbe9825de9d00e9e699f592/compilation/src/main/scala/scala/tools/nsc/ScalacBenchmark.scala#L18 It seems the annotation is attached to the wrong symbol

OlivierBlanvillain · 2017-06-29T07:37:45Z

@olafurpg Last commit should moves the annotation from getter/setter methods to the actual field, let me know if that's enough to make JMH happy!

olafurpg · 2017-06-29T07:41:48Z

I can confirm that the JMH benchmark runs from the latest commit! Thank you @OlivierBlanvillain

olafurpg · 2017-06-30T11:02:51Z

Any chance to get this merged? It would help unblock scala/compiler-benchmark#31

OlivierBlanvillain · 2017-06-30T11:13:00Z

It needs a review, @odersky could you have a look?

DarkDimius · 2017-07-10T07:39:50Z

@odersky could you please review it today so we can get it into next milestone?
It would be nice to have JMH working with the released jar.

odersky · 2017-07-10T14:32:37Z

@smarter I am not sure how Scala annotations with default arguments are handled in the backend. Maybe @DarkDimius knows more?

odersky · 2017-07-10T14:38:50Z

I tried a translation to Scala; seems to work.

smarter · 2017-07-10T14:42:59Z

tests/pending/pos/i2797/Fork_1.java

@@ -0,0 +1,7 @@
+// Test is pending because we have no good way to test it.


I don't understand why this test is pending, since Fork is suffixed with 1 and Test with 2, they should be compiled as you want

They are not. I get: "Not found: Fork" when I compile Test.

I've fixed the test. The issue was that the annotation needed to be called Fork_1 since the name of the file is Fork_1.java. The javac error messages are eaten by our test suite, there's a WIP PR to fix that at #2811

Ah, good to know.

This is nessary for interactiong with java libraries such as JMH. Matches scalac's behavior.

Lifting such arguments breaks the contract with the backend, which expects to see default getters inline, not lifted out.

…83aca6)

- Java-defined annotation constructors need to have JavaDefined set. - Change the points where we suppress lifting. One case slipped through before. - Also: Add test.

Test is pending because we have no good way to test it. We need to: Compile Fork.java, and then compile Test.scala with Fork.class on the classpath.

Fork_1.java failed to compile before because the class name did not match the filename.

smarter · 2017-07-10T17:46:32Z

It might not crash in the backend, but the Scala test case still generates some weird code:

@{
    <accessor> val value$1: Int = Fork.$lessinit$greater$default$1
    <accessor> val jvmArgs$1: Array{scala$Array$$T = String} = 
      Array.apply[String](["I\'m","hot" : String])(
        scala.reflect.ClassTag.apply[String](classOf[class String])
      )
    new Fork(Test.this.value$1, Test.this.jvmArgs$1)

It looks like scalac does something similar but with a warning:

% scalac tests/pos/i2797a/Fork.scala tests/pos/i2797a/Test.scala
tests/pos/i2797a/Test.scala:4: warning: Usage of named or default arguments transformed this annotation
constructor call into a block. The corresponding AnnotationInfo
will contain references to local values and default getters instead
of the actual argument trees
@Fork(jvmArgs = Array("I'm", "hot"))
 ^

smarter

LGTM since this is an improvement on the current situation

While the test now succeeds under separate compilation, it fails join compilation (which happens to be tested by the legacy tests currently), you can reproduce that by running: dotc tests/pending/pos/i2797/Fork_1.java tests/pending/pos/i2797/Test_2.scala This fails with: -- Error: tests/pending/pos/i2797/Test_2.scala:4:0 ------------------------------------- 4 |@Fork_1(jvmArgs = Array("I'm", "hot")) |^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |missing argument for parameter value of constructor Fork_1: (value: |Int, jvmArgs: Array[String]): Fork_1 Because the JavaParsers output a constructor for Fork_1 without default values, to fix this we need JavaParsers to do something similar to ClassfileParser#addAnnotationConstructor

smarter · 2017-07-10T18:35:40Z

Had to disable i2797 again because of joint compilation failure, see last commit, I'll open an issue for that.

odersky · 2017-07-10T19:32:13Z

In fact, we could put i2797 in pos-special and have legacyTests not try it.

smarter · 2017-07-10T21:25:43Z

Looks like separate compilation is not used with compileDir unfortunately.

OlivierBlanvillain requested a review from odersky June 29, 2017 07:45

DarkDimius assigned odersky Jul 4, 2017

odersky force-pushed the workaround-2797 branch from 178bfa8 to a4e6cb7 Compare July 10, 2017 13:52

odersky requested review from DarkDimius and removed request for odersky July 10, 2017 14:04

smarter reviewed Jul 10, 2017

View reviewed changes

odersky changed the title ~~Workaround #2797 by removing an assertion~~ Fix #2797: Don't lift Java annotation arguments out of call Jul 10, 2017

odersky added this to the 0.2 Tech Preview milestone Jul 10, 2017

OlivierBlanvillain and others added 9 commits July 10, 2017 19:26

Workaround scala#2797 by removing an assertion

c5f98c0

Minor spacing & formating

b6618f6

Move getter/setter annotation to the underlying field

4b6e15c

This is nessary for interactiong with java libraries such as JMH. Matches scalac's behavior.

Don't lift arguments out of Java annotation constructors

579dd1d

Lifting such arguments breaks the contract with the backend, which expects to see default getters inline, not lifted out.

Workaround scala#2797 by removing an assertion (reverted from commit e…

a1b6c3e

…83aca6)

Fix handling of defaults in Java annotations

546c59f

- Java-defined annotation constructors need to have JavaDefined set. - Change the points where we suppress lifting. One case slipped through before. - Also: Add test.

Move test to pending

a02b10b

Test is pending because we have no good way to test it. We need to: Compile Fork.java, and then compile Test.scala with Fork.class on the classpath.

Add Scala test case

c15ff7b

Fix typo in docs

7a35bf8

odersky force-pushed the workaround-2797 branch from 5d7b140 to 7a35bf8 Compare July 10, 2017 17:27

Fix test i2797

a707a74

Fork_1.java failed to compile before because the class name did not match the filename.

smarter approved these changes Jul 10, 2017

View reviewed changes

odersky force-pushed the workaround-2797 branch from 6071740 to bc2b6c3 Compare July 11, 2017 09:45

smarter merged commit f14e289 into scala:master Jul 11, 2017

biboudis mentioned this pull request Jul 11, 2017

Compile benchmark with Dotty, fixes #29. scala/compiler-benchmark#31

Merged

olafurpg mentioned this pull request Jul 12, 2017

Match error in backend using Java defined annotation with default parameters #2704

Closed

allanrenucci deleted the workaround-2797 branch December 14, 2017 19:25

		@@ -0,0 +1,7 @@
		// Test is pending because we have no good way to test it.

Fix #2797: Don't lift Java annotation arguments out of call #2819

Fix #2797: Don't lift Java annotation arguments out of call #2819

Uh oh!

Conversation

OlivierBlanvillain commented Jun 28, 2017 • edited by odersky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olafurpg commented Jun 28, 2017

Uh oh!

OlivierBlanvillain commented Jun 29, 2017

Uh oh!

olafurpg commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olafurpg commented Jun 30, 2017

Uh oh!

OlivierBlanvillain commented Jun 30, 2017

Uh oh!

DarkDimius commented Jul 10, 2017

Uh oh!

odersky commented Jul 10, 2017

Uh oh!

odersky commented Jul 10, 2017

Uh oh!

smarter Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

odersky Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

smarter Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

odersky Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

smarter commented Jul 10, 2017

Uh oh!

smarter left a comment

Choose a reason for hiding this comment

Uh oh!

smarter commented Jul 10, 2017

Uh oh!

odersky commented Jul 10, 2017

Uh oh!

smarter commented Jul 10, 2017

Uh oh!

Uh oh!

OlivierBlanvillain commented Jun 28, 2017 •

edited by odersky

Loading

olafurpg commented Jun 29, 2017 •

edited

Loading