uber / NullAway

A tool to help eliminate NullPointerExceptions (NPEs) in your Java code with low build-time overhead
MIT License
3.62k stars 290 forks source link

NullAway doesn't recognize @Nullable annotation in a different compilation unit #1011

Open gavlyukovskiy opened 1 month ago

gavlyukovskiy commented 1 month ago

I've been migrating on of our projects to jspecify and noticed that NullAway reports nullability problems for passing null values to the @Nullable generic parameters in another compilation unit (main source set).

src/main/java:

public class Main {
    public static void main(String[] args) {
        // ok
        withFunction(x -> null);
    }

    public static void withFunction(Function<String, @Nullable String> f) {}
}

src/test/java:

class MainTest {
    public static void main(String[] args) {
        // error: [NullAway] returning @Nullable expression from method with @NonNull return type
        Main.withFunction(x -> null);
    }
}

Compiling the test class gives the following error:

nullaway-reproducer/src/test/java/com/github/gavlyukovskiy/MainTest.java:6: error: [NullAway] returning @Nullable expression from method with @NonNull return type
        Main.withFunction(x -> null);
                          ^
    (see http://t.uber.com/nullaway )
1 error

I've also tried extracting the expression into Function<String, @Nullable String> variable, but passing that gives slightly different error:

nullaway-reproducer/src/test/java/com/github/gavlyukovskiy/MainTest.java:11: error: [NullAway] Cannot pass parameter of type Function<String, @Nullable String>, as formal parameter has type Function<String, String>, which has mismatched type parameter nullability
        Main.withFunction(f);
                          ^
    (see http://t.uber.com/nullaway )
1 error

I've tried the same test but without generic parameters:

public static void with(@Nullable String v) {}

and both main and test source sets react to it correctly (both with and without @Nullable annotation), so the problems appears to be with generics.

I've tried reproducing the situation inside com.uber.nullaway.jspecify.GenericsTests, but as they're the same compilation unit it wasn't possible.

Reproducer: https://github.com/gavlyukovskiy/nullaway-reproducer (./gradlew build)

Versions:

msridhar commented 1 month ago

Hi @gavlyukovskiy, one caveat is that our JSpecify mode checks are still limited. That said I think your specific test case should work. You may be running into https://github.com/uber/NullAway/issues/1005. Any chance you could try compiling using JDK 22 and see if that fixes the problem?

gavlyukovskiy commented 1 month ago

Hi @msridhar, thank you for looking into that. I've just tried temurin-22.0.2, but get the same error

msridhar commented 1 month ago

This is very interesting, thanks for reporting. I can reproduce the problem, even on JDK 22, and I've pushed a test case here. This to me looks like a bug in javac. NullAway gets the type for the lambda being passed as a parameter here:

https://github.com/uber/NullAway/blob/7c5edf7f35aa8d0a9a1b150fdbac27a49f7cc4f7/nullaway/src/main/java/com/uber/nullaway/NullAway.java#L927-L927

On JDK 21, this type is Function<String,String> with the type-use annotation missing (as expected). On JDK 22, we see the type Function<@Nullable String,String>, which is also wrong; the @Nullable should be on the second type argument. At the invocation of withFunction I see the type of the function as (java.util.function.Function<java.lang.@org.jspecify.annotations.Nullable String,java.lang.String>)void which also has the type-use annotation in the wrong place.

@cushon @cpovirk do you agree this is potentially a bug in javac's reading of type-use annotations from bytecode? Is this a known issue?

msridhar commented 1 month ago

FWIW, here is what I see for the withFunction method when I run javap -v on the class file:

  public static void withFunction(java.util.function.Function<java.lang.String, java.lang.String>);
    descriptor: (Ljava/util/function/Function;)V
    flags: (0x0009) ACC_PUBLIC, ACC_STATIC
    Code:
      stack=0, locals=1, args_size=1
         0: return
      LineNumberTable:
        line 8: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       1     0     f   Ljava/util/function/Function;
      LocalVariableTypeTable:
        Start  Length  Slot  Name   Signature
            0       1     0     f   Ljava/util/function/Function<Ljava/lang/String;Ljava/lang/String;>;
    Signature: #21                          // (Ljava/util/function/Function<Ljava/lang/String;Ljava/lang/String;>;)V
    RuntimeVisibleTypeAnnotations:
      0: #23(): METHOD_FORMAL_PARAMETER, param_index=0, location=[TYPE_ARGUMENT(1)]
        org.jspecify.annotations.Nullable

I'm pretty sure TYPE_ARGUMENT(1) means the second type argument. Also, within NullAway if I call getRawTypeAttributes() on the MethodSymbol for withFunction I see the position of the @Nullable annotation as:

[METHOD_FORMAL_PARAMETER, param_index = 0, location = (TYPE_ARGUMENT(1)), pos = -1]
gavlyukovskiy commented 1 month ago

That's an interesting find. It seems like it already puts an annotation on the first argument as well for BiFunction or custom functional interface.

msridhar commented 1 month ago

@gavlyukovskiy I've confirmed this is an issue in javac. I'll put updates here as there is progress towards a fix.

gavlyukovskiy commented 1 month ago

Thank you @msridhar

msridhar commented 1 month ago

I should say a bit more here. This test case exposes a new issue on JDK 22+. On JDK 21 and earlier the issue with not reading type annotations from bytecode was already known and is covered by #1005

msridhar commented 1 month ago

There is now a PR https://github.com/openjdk/jdk/pull/20460 up to fix this issue. Once a fix lands, it will have to get into a released JDK before NullAway users can benefit from it. Thank you again for reporting this! I will leave this issue open until a fix reaches a JDK release.

msridhar commented 3 weeks ago

Small update here. The above PR has landed and will ship with JDK 24. There is a backport for JDK 23: https://github.com/openjdk/jdk23u/pull/67 Assuming the backport goes through, I think this would be fixed in the first patch release of JDK 23 (it's too late to make the GA release).

gavlyukovskiy commented 3 weeks ago

Thank you for the update @msridhar! It might be a wrong place to ask, but is there any chance such a fix will be backported to jdk21u?

msridhar commented 3 weeks ago

@gavlyukovskiy there is an effort to get related fixes for reading of type annotations from bytecodes backported to JDK 21 and possibly earlier. See https://github.com/jspecify/jspecify/issues/365, https://bugs.openjdk.org/browse/JDK-8323093, and https://mail.openjdk.org/pipermail/compiler-dev/2024-July/027344.html. At this point, I imagine that if that backport is approved, the fixes for this issue would be included. /cc @cushon