Change in handling of nullable arrays with eisop 3.42.0

[cushon]

The following example is similar to an issue seen in the wild where kotlinc emits code where org.jetbrains.annotations.Nullable appears as a declaration annotation (and not as a type annotation). Similar behaviour can be seen with javac when targeting language levels that do not support type use annotations.

(This repro is based on b/318857000.)

---

Repro

import org.jetbrains.annotations.Nullable;

public interface Lib {
  @Nullable String[] get();
}

class User {
  void go(Lib lib) {
    String[] b = lib.get();
  }
}

$ wget https://repo1.maven.org/maven2/org/jetbrains/annotations/24.1.0/annotations-24.1.0.jar
$ javac --release 7 -cp annotations-24.1.0.jar Lib.java

$ ./checker-framework-3.41.0-eisop1/checker/bin/javac -processor nullness -cp .:annotations-24.1.0.jar User.java
OK

$ ./checker-framework-3.42.0-eisop1/checker/bin/javac -processor nullness -cp .:annotations-24.1.0.jar User.java
User.java:3: error: [assignment.type.incompatible] incompatible types in assignment.
    String[] b = lib.get();
                        ^
  found   : @Nullable String @NonNull []
  required: @NonNull String @Nullable []
1 error

[cpovirk]

Thanks, Liam!

Note also that the JetBrains annotations were formerly not usable as type-use annotations, only as declaration annotations. In fact, kotlinc downloads still come bundled with the annotations-13.0.jar from back then. Building against that jar, then, is another way to get the problematic bytecode from @Nullable String[] get();—without even needing to use --release 7. I haven't investigated in detail, but I wouldn't be surprised if javac sometimes ran automatically against that jar, perhaps as part of Kotlin annotation processing.

[wmdietl]

Thanks for tracing this down and for the minimal test case, @cushon!

In #658 I thought that this change was just a semantic-preserving change to make the code more uniform. I was wrong. The JetBrains annotations are both type use and declaration annotations, so the code needs to be more careful what it checks. A misleadingly-named method didn't help.

I hopefully fix this again in #674. I added a test case to try to avoid this backsliding in the future and cleaned up the code a bit. Hopefully...

One thing to note: when compiling Lib.java with a Java 8+ compiler, javac puts two annotations into the bytecode:

    RuntimeInvisibleAnnotations:
      0: #8()
        org.jetbrains.annotations.Nullable
    RuntimeInvisibleTypeAnnotations:
      0: #8(): METHOD_RETURN, location=[ARRAY]
        org.jetbrains.annotations.Nullable

This is why the return type of that method is treated as @Nullable String @Nullable [], as evidenced by this error. If this isn't as expected, let's discuss this in a separate issue.

Feedback, here or on #674, is always welcome!

[cushon]

Thank you!

I think the Java 8+ output for that example is expected. I don't have a strong opinion about what the best thing for the analysis to do in that situation is.

[cpovirk]

+1, thanks.

Ouch on that method name!

For the Java 8+ output, I lean more toward what I'd describe as "Do what the bytecode says," which I'd consider to be "nullable array of nullable strings." In practice, I hope to mostly not care: We'll eventually try to get Google code off of hybrid type-use/declaration annotations like JetBrains's, at which point we should see them only in the output of kotlinc, which matches the --release 7 output. That makes the Java 8+ output mostly practically irrelevant.

---

I was going to then add: "For all we know, the source code might have been '@Nullable String @Nullable [] get();,' which is fairly clearly an intentional attempt to annotate both the array and its elements." But of course that source code would have produced three annotations in the bytecode (2 type-use and 1 declaration), so it doesn't necessarily have much bearing on the 2-annotation bytecode above.

Still, I feel good about the principle of treating RuntimeInvisibleAnnotations as applying to the array as a whole, even if we suspect that they might have been meant for its elements. I wouldn't be surprised if there were some edge case with inner types (which might appear in source either qualified or unqualified), in which we can't necessarily determine what the source code looked like.

Additionally, cushon@ recently dug up JDK-8223936 (which was also in the list of type-use-annotation bugs that you'd provided), so I would really not want to work too hard to reverse-engineer user intent from the current javac behavior, which doesn't even appear to be correct in all cases.

You know, that's all really more an argument for keeping whatever your current behavior is (after this fix), regardless of whether it matches my preferences :)