search

sbt / zinc

Scala incremental compiler library, used by sbt and other build tools
Apache License 2.0
335 stars 121 forks source link

Adding @Retention(RetentionPolicy.RUNTIME) to a java annotation does not correctly recompile downstream classes #630

Closed lihaoyi closed 2 years ago

lihaoyi commented 5 years ago 
sbt.version=1.2.6
package test;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;

//@Retention(RetentionPolicy.RUNTIME)
public @interface Test {
}
package test;
@Test
public class Foo{
  public static void main(String[] args){
    System.out.println(Foo.class.getAnnotations().length);
  }
}
$ sbt run
[info] Compiling 2 Java sources to /Users/lihaoyi/test/target/scala-2.12/classes ...
0
$ vim Test.java
$ cat Test.java
package test;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;

@Retention(RetentionPolicy.RUNTIME)
public @interface Test {
}
$ sbt run
[info] Compiling 1 Java source to /Users/lihaoyi/test/target/scala-2.12/classes ...
0
$ sbt clean
$ sbt run
[info] Compiling 2 Java sources to /Users/lihaoyi/test/target/scala-2.12/classes ...
1
retronym commented 2 years ago

The same root cause also causes undercompilation when an annotation is removed, or when it's @Target is changed, or it is otherwise edited to change its attributes.

This should be fixed by extending sbt.internal.inc.classfile.Parser to parse annotation attributes and look for class references within the descriptors.

==> ./src/main/java/A1.java <==
import java.lang.annotation.*;

@Retention(RetentionPolicy.CLASS)
@Target(ElementType.METHOD)
@interface A1 {

}

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.METHOD)
@interface A2 {

}

==> ./src/main/java/J1.java <==
class J1 {
  @A1 @A2 void foo() {}
}
 javap -v -cp ./target/scala-2.13/classes J1
Classfile /Users/jz/code/zinc-undercompilation-java-annotations/target/scala-2.13/classes/J1.class
  Last modified 14/01/2022; size 319 bytes
  MD5 checksum b84ad0994b3be62eb5f2fed987450004
  Compiled from "J1.java"
class J1
  minor version: 0
  major version: 52
  flags: ACC_SUPER
Constant pool:
   #1 = Methodref          #3.#15         // java/lang/Object."<init>":()V
   #2 = Class              #16            // J1
   #3 = Class              #17            // java/lang/Object
   #4 = Utf8               <init>
   #5 = Utf8               ()V
   #6 = Utf8               Code
   #7 = Utf8               LineNumberTable
   #8 = Utf8               foo
   #9 = Utf8               RuntimeVisibleAnnotations
  #10 = Utf8               LA2;
  #11 = Utf8               RuntimeInvisibleAnnotations
  #12 = Utf8               LA1;
  #13 = Utf8               SourceFile
  #14 = Utf8               J1.java
  #15 = NameAndType        #4:#5          // "<init>":()V
  #16 = Utf8               J1
  #17 = Utf8               java/lang/Object
{
  J1();
    descriptor: ()V
    flags:
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokespecial #1                  // Method java/lang/Object."<init>":()V
         4: return
      LineNumberTable:
        line 1: 0

  void foo();
    descriptor: ()V
    flags:
    Code:
      stack=0, locals=1, args_size=1
         0: return
      LineNumberTable:
        line 2: 0
    RuntimeVisibleAnnotations:
      0: #10()
    RuntimeInvisibleAnnotations:
      0: #12()
}
SourceFile: "J1.java"
SethTisue commented 2 years ago

our internal/inc/classfile/Parser.scala is a port of https://github.com/clarkware/jdepend/blob/master/src/jdepend/framework/ClassFileParser.java?ts=4

We should be able to port this upstream change: https://github.com/clarkware/jdepend/commit/e8271a2b053c6e1857ba54131d36e2316a108f89?ts=4

(but see also the next two commits in the file's history, which fix up the added u2 method)

note that the sources have mixed tabs & spaces 🙀, hence I added the ?ts=4 to the above URLst to make GitHub render them properly

lrytz commented 2 years ago

our internal/inc/classfile/Parser.scala is a port of https://github.com/clarkware/jdepend/commits/master/src/jdepend/framework/ClassFileParser.java

https://github.com/sbt/zinc/blob/707d949a63948c26656b30f56d10a28e579faada/internal/zinc-classfile/src/main/scala/sbt/internal/inc/classfile/Parser.scala#L27-L28

jdepend switched to the MIT License

SethTisue commented 2 years ago

some notes:

so far I wrote a passing test (that tests something involving a dependency between two Java source files that's already handled correctly), then turned it into a failing test that actually tests the right thing, so now I'm ready to embark on an actual fix

SethTisue commented 2 years ago

Porting the jdepend changes isn't entirely straightforward, because our version of ClassfileParser is a re-imagining rather than a method-by-method port, so I'm needing to understand both versions well in order to "decompile" the added imperative code into the functional style used in our version.

Before trying to get the end-to-end scripted test to pass, I should add one or more unit tests to AnalyzeSpecification.scala (which I can run with e.g. zincClassfile/testOnly *AnalyzeSpecification)

DependencyContext is an enum which is either DependencyByMemberRef, DependencyByInheritance, or LocalDependencyByInheritance. It isn't super clear to me if I need to add a new type, or whether I can just use DependencyByMemberRef. I guess I should start there and then see what Dale or Jason thinks.

SethTisue commented 2 years ago

The changes to jdepend only handle RuntimeVisibleAnnotations, but Jason's example above also shows a RuntimeInvisibleAnnotations descriptor (corresponding to java.lang.annotation.RetentionPolicy.CLASS).

@retronym I would have thought that RuntimeInvisibleAnnotations were equally necessary to parse, so I don't understand why jdepend doesn't handle them. Do you have any insight into this? Do you think it was an oversight on their part, or is it just that their requirements are different than ours? It seems to me that for example, someone might change the retention policy of the upstream annotation, requiring recompilation downstream (as you alluded to with "otherwise edited to change its attributes").

retronym commented 2 years ago

We should consider them. I’m not familiar with the goals of j depend, so not sure if it’s a conscious choice or an oversight.

SethTisue commented 2 years ago

Another weird thing about the upstream JDepend code is that most of the file uses DataInputStream for parsing, but then that code produces a byte[] for each annotation, but then the contents of that byte array are parsed using code that access the byte array directly and the error-prone-ness of that are exactly why the second and third commits had to be added later. Not sure why, maybe premature optimization? I'm using DataInputStream throughout (you can make a DataInputStream from a byte array using ByteArrayInputStream).

Dale helped me understand aspects of the zinc scripted tests that were puzzling to me; my eventual PR will improve the documentation on that.

SethTisue commented 2 years ago

I have the classfile parser reading annotations, and that is sufficient to cause the Foo -> Test dependency to be picked up, which is sufficient to fix some under-compilation scenarios (e.g. annotations-in-java-sources-a in my wip branch).

But although it's necessary to fix Haoyi's scenario (annotations-in-java-sources-b), it isn't sufficient.

We also need to change zinc to consider the change to Test to be an API change. Not all upstream changes cause recompilation downstream, only API changes.

SethTisue commented 2 years ago

What determines what an API change is?

ExtractAPI.scala does what its name says. (Not sure if APIUtil.minimize, called by the api(...) methods in Incremental.scala, is also relevant.)

https://github.com/sbt/zinc/pull/53 seems to be the last time this stuff was hacked on, primarily 0f77be96713d4b03d4fb1ff8066b64b4a3ddc6f6 and c9e365b810e1f0e55d9641e1164b2454d3d3be5a

As for sameness of APIs: SameAPI has sameAnnotations, HashAPI has hashAnnotations, so perhaps I'll need to modify them, or perhaps it will be sufficient to modify ExtractAPI, since code for comparing annotations already exists. SameAPI and HashAPI take Definitions as input, and Definition (in xsbt.api) has an annotations: Array[Annotation] member, and Annotated and TypeParameter both have such a member as well.

Static annotations

The phrase "static annotation" used in these files and commits was new to me, or at least I hadn't seen it in a long time. This isn't Java terminology, but Scala terminology: scala.annotation.StaticAnnotation. The ScalaDoc for that trait says:

Annotation classes defined in Scala are not stored in classfiles in a Java-compatible manner and therefore not visible in Java reflection. In order to achieve this, the annotation has to be written in Java.

so I think that's out of bounds for me at the moment; I'm focusing on Java-defined annotations. But the fact that ExtractAPI only considers annotations that pass isStatic may be exactly where the problem lies...!

The following comment in ExtractAPI.scala seems directly relevant:

  // The compiler only pickles static annotations, so only include these in the API.
  // This way, the API is not sensitive to whether we compiled from source or loaded from classfile.
  // (When looking at the sources we see all annotations, but when loading from classes we only see the pickled (static) ones.)

This was added by Martin Duhemm in 3792b80e5a from #630, but he was porting https://github.com/sbt/sbt/pull/2343/commits/d32a0eaaa06780e089683b9c9be2def32930796b and https://github.com/sbt/sbt/pull/2343/commits/698902ba44daa2f223b687ce3ce5cd44db72db34 (by Krzysztof and Adriaan) — I feel like those commits may be overzealously ignoring annotations that don't extend scala.annotation.ScalaAnnotation, but I'll have to dig deeper to be sure.

SethTisue commented 2 years ago

I think I was barking up the wrong tree looking at ExtractAPI, since there are only Java sources involved in this particular bug. Java sources are always processed by running javac and then analyzing the resulting classfiles; whereas ExtractAPI interacts with the Scala compiler.

My next idea for a fix plan is as follows.

First, let's back up a bit and think about what we're trying to do here. We have an annotationTest, and its "annotation type declaration" (to use the JLS terminology) is itself annotated (or not), in this case with @Retention (or not).

In normal user code the presence or absence of an annotation on a definition is not an API change and doesn't require recompilation downstream.

The most targeted fix possible would say: "addition, removal, or change of the @Retention annotation on an annotation type declaration causes recompilation downstream".

But being quite that targeted is probably overkill.

We could loosen it one step and say, "any changes to the annotations on an annotation type declaration causes recompilation downstream". But why not go further and say, "if the classfile parser notices any change at all to an annotation type declaration, recompile downstream". Technically this could cause overcompilation in some cases, but changes to a Java-declared annotation is an unusual case, and overcompilation isn't so bad (it's wasteful but doesn't affect correctness), so I think we could favor simplicity of implementation here.

An example where a similar decision was made is APIChangeDueToMacroDefinition, which is commented:

 * If we recompile a source file that contains a macro definition then we always assume that it's
 * api has changed. The reason is that there's no way to determine if changes to macros implementation
 * are affecting its users or not. Therefore we err on the side of caution.

I'm proposing making a similar decision here — err on the side of caution, assume the API changed.

How to implement? I haven't done much digging on that yet, but my best guess at the moment is to take APIChangeDueToMacroDefinition as a model, add APIChangeDueToAnnotationDeclaration, and then modify MemberRefInvalidator to use InvalidateUnconditionally when it sees APIChangeDueToAnnotationDeclaration.

SethTisue commented 2 years ago

My PR, #1079, is exactly along the lines of my last remarks above.