Not-null varargs argument with Nullable elements - considered as Nullable

[krisso-rtb]

@Documented
@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.TYPE_USE, ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER,
        ElementType.LOCAL_VARIABLE })
public @interface Nullable {
}
--------------------------------------------------------------------------
@Documented
@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.TYPE_USE, ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER,
        ElementType.LOCAL_VARIABLE })
public @interface NonNull {
}
--------------------------------------------------------------------------
protected static String composeName(@Nullable String @NonNull... names) {
    StringJoiner stringJoiner = new StringJoiner(":");
    for (String name : names) {
        if (name != null && !name.isEmpty()) {
            stringJoiner.add(name);
        }
    }
    return stringJoiner.toString();
}

result:

error: [NullAway] enhanced-for expression names is @Nullable

which points to line:

for (String name : names) {

Key facts:

• names is vararg array, it's expected to be not-null and contain:
• Nullable String elements
• For some reason these two annotations are mixed.

Nice examples: https://help.eclipse.org/latest/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Ftasks%2Ftask-using_null_type_annotations.htm&cp=1_3_9_1_3&anchor=compatibility

There is also one answer I found helpful: https://stackoverflow.com/questions/32327134/where-does-a-nullable-annotation-refer-to-in-case-of-a-varargs-parameter

[msridhar]

Yes, NullAway doesn't properly parse annotations on arrays (or varargs); it interprets the @Nullable annotation in your example as pertaining to the reference to the array itself, not to the references within the array. Unfortunately fixing this will introduce compatibility issues, so we need to be careful. We plan to do this as part of our efforts to support JSpecify. Thanks for the report!

[cpovirk]

This is probably not a surprise, but I notice the same thing with annotations on type arguments: They're interpreted as if they're on the "top-level" type. For example:

package p;

import java.util.List;
import org.checkerframework.checker.nullness.qual.Nullable;

class TypeArgumentAnnotation {
  void use(List<@Nullable String> list) {
    list.hashCode();
  }
}

TypeArgumentAnnotation.java:8: error: [NullAway] dereferenced expression list is @Nullable
    list.hashCode();
        ^

[msridhar]

Wow @cpovirk that was not expected. I am going to pull that out as a separate issue.

[msridhar]

@cpovirk the issue you observed was fixed in #702

[cpovirk]

The main fix here does still need to be in NullAway, but I did notice one other thing when I was looking back at this issue:

Because the @Nullable annotation in this example includes PARAMETER (as well as TYPE_USE) in its @Target, it does get applied to the entire parameter, too. We can see in the .class file that the parameter has @Nullable on it. That's in addition to the @NonNull annotation on the parameter type and to the @Nullable annotation on the parameter's element type. That means that the array is annotated as both nullable and non-null. I don't know how NullAway normally handles such conflicts. But you could avoid that entirely by removing PARAMETER from the @Target.

(If you still have tools that require PARAMETER in the @Target, you might be able to use a separate @Nullable annotation for them. Unfortunately, whenever you have annotations that include both TYPE_USE and other target locations, you may encounter cases in which they're applied to multiple locations.)

[msridhar]

Thanks a lot @cpovirk! We will keep an eye out for that issue. I would have been very confused without your comment. I hope to get back to this issue at some point soon.

[msridhar]

Similar to https://github.com/uber/NullAway/pull/1010#issuecomment-2267595218 here is a table of expected behaviors with varargs annotations:

Annotation Type	Annotation Position	Mode	@Nullable varargs array	@Nullable array elements	@Nullable args at calls
declaration	-	standard	❌	❌	✅
declaration	-	legacy	✅	❌	✅
declaration	-	JSpecify	❌	❌	✅
type use	before ...	standard	✅	❌	❌
type use	before ...	legacy	✅	❌	✅
type use	before ...	JSpecify	✅	❌	❌
type use	elements	standard	❌	❌	✅
type use	elements	legacy	✅	❌	✅
type use	elements	JSpecify	❌	✅	✅
type use	both	standard	✅	❌	✅
type use	both	legacy	✅	❌	✅
type use	both	JSpecify	✅	✅	✅
both	before ...	standard	✅	❌	❌
both	before ...	legacy	✅	❌	✅
both	before ...	JSpecify	✅	❌	❌
both	elements	standard	❌	❌	✅
both	elements	legacy	✅	❌	✅
both	elements	JSpecify	❌	✅	✅
both	both	standard	✅	❌	✅
both	both	legacy	✅	❌	✅
both	both	JSpecify	✅	✅	✅

A subtlety here is that a declaration annotation should allow @Nullable arguments to be passed at call sites outside of JSpecify mode, even though we cannot check within the method that they are used correctly. This is because in almost all cases, one wants to write @Nullable on the varargs formal parameter to indicate that individual varargs actual parameters may be @Nullable.