Nullable annotation on type parameter used in method vararg argument not recognized

[mihalyr]

In latest Guava 33.4.8 Sets.newHashSet the parameter elements are of @Nullable type, but NullAway fails to recognize it.

https://github.com/google/guava/blob/f06690fa3e874f65515e8fd338a74d636e2c792f/guava/src/com/google/common/collect/Sets.java#L200-L205

  @SuppressWarnings("NonApiType") // acts as a direct substitute for a constructor call
  public static <E extends @Nullable Object> HashSet<E> newHashSet(E... elements) {
    HashSet<E> set = newHashSetWithExpectedSize(elements.length);
    Collections.addAll(set, elements);
    return set;
  }

Error:

/var/home/user/workspace/nullaway-test/WebsiteProvider.java:50: error: [NullAway] passing @Nullable parameter 'website' where @NonNull is required
      final Set<String> params = Sets.newHashSet(website, company);
                                                 ^
    (see http://t.uber.com/nullaway )

This got flagged only after recently bumping NullAway and Guava versions. It used to work before.

[msridhar]

Thanks for the report. Which version of NullAway were you using before and after? And, just to confirm, you are not passing the flag to run NullAway in JSpecify mode, right?

[msridhar]

I dug a bit on this. The new error is due to google/guava@8ebb375 which switches Guava to using @NullMarked annotations, meaning NullAway now treats Guava packages as annotated for nullness checking and hence does additional checking on calls to Guava methods. I don't think the issue is related to the NullAway version, unless you are updating from a very old version.

Right now I'm seeing a bug where even in JSpecify mode I can't get this code to type check. I think it's due to the use of the type variable in the varargs position. I'll have to dig a bit deeper to find a fix. Even then, I'm not sure there is a way to get this code to type check outside JSpecify mode.

If you need an immediate workaround to unblock the Guava upgrade (not recommended in the long term), you could try adding com.google.common as an unannotated sub-package in the NullAway settings, which will disable nullness checking for calls to Guava code (at the cost of missing potential NPEs).

[mihalyr]

Thank you @msridhar for looking into it. I suspected that the Guava changes will be the culprit - https://github.com/google/guava/releases/tag/v33.4.1 "Today, we are releasing Guava 33.4.1 and a few other 33.4.x releases. All the releases maintain binary compatibility, but they include changes to nullness and the module system that may be disruptive to some users."

It's not a big problem on our side, as this construct with nullable inputs is only used in a few places so we are already unblocked by a couple of @SuppressWarnings("NullAway") annotations, just thought I'll let you know in case there is a fix possible.

Which version of NullAway were you using before and after? And, just to confirm, you are not passing the flag to run NullAway in JSpecify mode, right?

0.11.3 -> 0.12.6, no jspecify mode (I didn't even know this existed, I haven't seen jspecify used in the wild yet), and Guava 33.4.0 -> 33.4.8.

[cpovirk]

To pass a nullable element, I would try declaring the Set you're creating as having a nullable type argument:

final Set<@Nullable String> params = Sets.newHashSet(website, company);

It's entirely possible that that still won't work right :) But it's the next thing that I'd try, followed by:

final Set<@Nullable String> params = Sets.<@Nullable String>newHashSet(website, company);

Depending on which of those (if any) fixes the problem, the NullAway folks might be able to figure out what needs to be done (potentially more than just #1200).

[msridhar]

I've confirmed in #1200 that the following works in JSpecify mode (or it will work once #1200 lands):

final Set<@Nullable String> params = Sets.newHashSet(website, company);

Caveat: it will only work for now when compiling with a sufficiently recent javac version (from JDK 23 or 24), due to the need to read type-use annotations from bytecode.

Outside of JSpecify mode, NullAway simply assumes that type arguments are always @NonNull so it won't be possible to get this code to type check.

@mihalyr once #1200 makes it into a release, if you have time you could try running NullAway in JSpecify mode in your code base (assuming you can compile using a recent javac; see also here. It seems likely that once JDK 21.0.8 is released it will also have the necessary fixes). It could allow you to remove a couple of suppressions and it might catch some other issues. But, it's possible it will lead to additional false positives (for now) and it may require more annotations in your code.

[jeffrey-easyesi]

I think #1200 may introduce a false positive when the varargs array itself is nullable:

@NullMarked
class Test {
    <T extends Object> void varargsTest(T @Nullable... args) {}
    void f() { this.<String>varargsTest((String[])null); } // causes a warning in issue1999 branch
}

[msridhar]

Thanks @jeffrey-easyesi ! I'll look into it and hold off on landing until your test case is resolved.

[jeffrey-easyesi]

A thought: javac has already determined whether every call is varargs or not. Could be simpler/more efficient to reuse the result of that work, which can be found in
((JCTree.JCMethodInvocation) tree).varargsElement
or
((JCTree.JCNewClass) tree).varargsElement
If null, then the actual parameter is the array instead of individual args. (If non-null, it's the (annotated) individual arg type)

[msridhar]

@jeffrey-easyesi great tip, thanks!! I used it to simplify the code quite a bit in ef41fe9