Switching from javax.annotation.Nullable to org.checkerframework.checker.nullness.qual.Nullable does not always work

[andrey-vasilyev]

trafficstars

It seems like everybody is slowly switching from jsr305 to something else. I am switching to checker framework. But there is an issue. If I mark some methods in my interface as org.checkerframework.checker.nullness.qual.Nullable (instead of javax.annotation.Nullable) immutables do not consider them as optional any more i.e. building an immutable fails with an error that not every required attribute is set. The tricky part was to figure out that this only happens if my interface with Nullable annotation is in a separate jar file. When both interface and it's implementing abstract @Value.Immutable class are in the same module things seem to work normal.

I was able to trace it to org.immutables.value.processor.meta.ValueAttribute.initSpecialAnnotations. When it calls element.getAnnotationMirrors() it doesn't return checkers Nullable annotation but jsr305 Nullable annotation is returned as expected. I understand this is a JDK platform code and assume JSR305 annotations are treated in a special way. Basically just wanted to know if my assumption is correct or not. Thanks.

[elucash]

The first thing I can think of is if org.checkerframework.checker.nullness.qual.Nullable is source only annotation (RetentionPolicy.SOURCE), so if it's already compiled (in a separate jar) it's completely erased/untraceable (the use sites/signatures where the annotation is applied, obviously annotation type itself is still where it is in Nullable.class etc). Other annotations are at least (RetentionPolicy.CLASS and stored in class files, but only RetentionPolicy.RUNTIME are stored and then propagated to runtime reflection, but since annotation processing is compile-time it can read CLASS-ones and RUNTIMEs too). Is there a mechanism in checkers framework to declare a "nickname" annotation with the RetentionPolicy.CLASS and make it apply the same constraints as..qual.Nullable? It was possible in jsr305 (not certain in which implementation though) with TypeQualifierNickname IIRC

[andrey-vasilyev]

The retention policy was my first guess also, but it is not the case in checkers:

@Documented
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE_USE, ElementType.TYPE_PARAMETER})
@SubtypeOf({})
@QualifierForLiterals(LiteralKind.NULL)
@DefaultFor(types = Void.class)
public @interface Nullable {}

I even tried to decompile the class file with javap -v -p and the annotation was there.

[elucash]

Ok, second guess, is qual.Nullable available on the compilation classpath during annotation processing? Probably yes, but if not then it might be why getAnnotationMirrors would not load it.

[andrey-vasilyev]

I wish you were right but it is available.

[andrey-vasilyev]

I decided to go deeper and I guess I've found something. So here is the important part the same interface decompiled with javap

  #10 = Utf8               getValue
  #11 = Utf8               ()Ljava/lang/Integer;
  #12 = Utf8               RuntimeVisibleTypeAnnotations
  #13 = Utf8               Lorg/checkerframework/checker/nullness/qual/Nullable;

  #10 = Utf8               getValue
  #11 = Utf8               ()Ljava/lang/Integer;
  #12 = Utf8               RuntimeVisibleAnnotations
  #13 = Utf8               Ljavax/annotation/Nullable;

So with qual.Nullable we get RuntimeVisibleTypeAnnotations and with javax Nullable we get RuntimeVisibleAnnotations and this is what actually makes the difference. May be this is because qual.Nullable has ElementType.TYPE_USE target. I have to admit this is the first time I get so deep into annotation processors ;) So may be you can take it from here. I mean may be immutables should account for such case. What do you think?

[elucash]

Yes, that is definitely the case, if getAnnotationMirrors is called on the Element (ie. ExecutableElement for a method) it will definitely not have annotations coming from say element.getReturnType().getAnnotationMirrors() (TypeMirror.getAnnotationMirrors). I would say that it's easy to fix, but because we've already been bitten by type/vs element annotations (in the context of ), this might have some ripple effects (like when we try to insert annotations on nullability annotations on fields, parameters/local variables in some places and then a conflicting or the same duplicate annotation might appear just from formatting the type itself). So I'll try it, but not sure how to detect or deal with potential problems

[elucash]

well, I just discovered that maybe we do handle type annotations for nullability (see initTypeName())

    if (provider.nullableTypeAnnotation) {
      this.nullability = NullabilityAnnotationInfo.forTypeUse();
    }

where provider.nullableTypeAnnotation is being discovered in convoluted TypeStringProvider.process() method. This might explain why it's actually works (for situations where it's coming from a separate jar). If that is working correctly, then my next guess would be a bug in compiler/annotation processing model.

[elucash]

private StringBuilder typeAnnotationsToBuffer(List<? extends AnnotationMirror> annotations, boolean nestedTypeUse) {
    StringBuilder annotationBuffer = new StringBuilder(100);
    for (AnnotationMirror annotationMirror : annotations) {  // <-- if we ever getting this annotation mirror here
      boolean canBeAppliedToMethodAsWell = !nestedTypeUse // just to short circuit computation early
          && Annotations.annotationMatchesTarget(annotationMirror.getAnnotationType().asElement(), ElementType.METHOD);
      if (canBeAppliedToMethodAsWell) {  // <-- I wonder if we're getting this case
        // skip this type annotation on top type
        continue;
      }
      CharSequence sequence = AnnotationMirrors.toCharSequence(annotationMirror, importsResolver);
      if (!nullableTypeAnnotation && sequence.toString().endsWith(EPHEMERAL_ANNOTATION_NULLABLE)) {
        this.nullableTypeAnnotation = true;  // <-- here we set it
      }
      annotationBuffer
          .append(sequence)
          .append(' ');
    }
    return annotationBuffer;
  }

see <-- comments above

[andrey-vasilyev]

It seems like typeAnnotationsToBuffer is never called. I did a fast check, maybe I've missed something

[elucash]

    List<? extends AnnotationMirror> annotations = AnnotationMirrors.from(type);
    if (!annotations.isEmpty()) {

so it appears to be empty

[andrey-vasilyev]

yes, with both Nullable annotations if I remember correctly

[elucash]

the javax.annotation.Nullable applies just on methods/fields/params, so it will not be present on TypeMirror, and will be handled in initSpecialAnnotations() by Element.getAnnotationMirrors(). The code in initSpecialAnnotations, in TypeStringProvider etc is so ugly because it's full of ugly hacks born in pain of working around of bugs in Javac and ECJ (especially in scenarios with not-yet-generated-types), some of those were fixed over time in compiler but it's really scary to go there and revise or refactor anything. What I usually did, was to download ECJ compiler jar in addition to javac we have in JDK and do some command line compilation with both, immutables and jars explicitly added to classpath. And, of course, with a dozen of ugly debug printing inserted in immutables (don't remember about syserr, but ProcessingEnv.getMessager().printMessage(MANDATORY_WARNING worked well). I know it's some work to setup (and I cannot find my older shell scripts with examples), but we can return to it soon. So far, my hypotheses are: 1. A compiler bug or 2. Something we don't know about annotation processing

[jacques-n]

My casual observation while working on Apache Calcite

Bug: Static inner class used as the property type => immutables treated the property as required

import Foo;
import org.immutables.value.Value;
import org.checkerframework.checker.nullness.qual.Nullable

@Value.Immutable
interface Emu {
  Foo.@Nullable Bar getFoo();
}

Correct Behavior: Static import of the inner class => property was correctly treated as nullable

import static Foo.Bar;
import org.immutables.value.Value;
import org.checkerframework.checker.nullness.qual.Nullable

@Value.Immutable
interface Emu {
  @Nullable Bar getFoo();
}

This was observed on Immutables 2.8.8 and jdk8u302.

[elucash]

I've made some revisions/bugfixes about how type annotations are parsed/analysed. I can try to include a test for the example above to see how if works now/fix if it is something simple enough (we're on 2.9.0-rc1 now, will release one or more rc)

[jacques-n]

I've also seen a variation of this problem which is more similar to the one referenced above. In this situation (where module 1 and module 2 are different compilation units) and the following classes:

// Module 1 Files
interface CheckerModule1 {
  @org.checkerframework.checker.nullness.qual.Nullable int getProp()
}

interface JavaxModule1 {
  @javax.annotation.Nullable int getProp()
}

// Module 2 Files
@Value.Immutable
interface CheckerModule2 implements CheckerModule1 {}

@Value.Immutable
interface JavaxModule2 implements JavaxModule1 {}

Outcome:

• CheckerModule2.prop is considered required. (bug)
• JavaxModule2.prop is considered nullable. (correct)

[elucash]

made some debugging recently, so far it appears that if coming from another module, Javac would not give us the type annotations (regardless of the retention). Actually, help wanted to confirm/deny this.