NullAway icon indicating copy to clipboard operation
NullAway copied to clipboard

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

Open gavlyukovskiy opened this issue 1 year ago • 13 comments

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:

  • org.jspecify:jspecify:1.0.0
  • com.google.errorprone:error_prone_core:2.29.2
  • com.uber.nullaway:nullaway:0.11.1
  • net.ltgt.errorprone:4.0.1

gavlyukovskiy avatar Aug 03 '24 13:08 gavlyukovskiy

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?

msridhar avatar Aug 03 '24 14:08 msridhar

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

gavlyukovskiy avatar Aug 03 '24 15:08 gavlyukovskiy

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<[email protected] 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 avatar Aug 03 '24 20:08 msridhar

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]

msridhar avatar Aug 03 '24 21:08 msridhar

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.

gavlyukovskiy avatar Aug 04 '24 14:08 gavlyukovskiy

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

msridhar avatar Aug 04 '24 14:08 msridhar

Thank you @msridhar

gavlyukovskiy avatar Aug 04 '24 14:08 gavlyukovskiy

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 avatar Aug 04 '24 15:08 msridhar

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 avatar Aug 05 '24 14:08 msridhar

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).

msridhar avatar Aug 17 '24 14:08 msridhar

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?

gavlyukovskiy avatar Aug 21 '24 15:08 gavlyukovskiy

@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

msridhar avatar Aug 21 '24 15:08 msridhar

I have confirmed that just-released JDK 23.0.2 contains the javac fix relevant to this issue. Leaving this issue open as should only close when it's fixed on all of NullAway's supported JDK versions. For now, if you build with JDK 23.0.2 or higher this issue should not be present.

msridhar avatar Jan 22 '25 23:01 msridhar

Given that the javac fix for exposing type annotations was backported to JDK 21.0.8 (see JDK-8225377), should this problem still appear when using the -XDaddTypeAnnotationsToSymbol=true compiler arg and NullAway 0.12.9?

bannmann avatar Aug 22 '25 11:08 bannmann

@bannmann this bug should be fixed in JDK 21.0.8 with the -XDaddTypeAnnotationsToSymbol=true compiler arg passed. I updated the JSpecify support wiki page accordingly. I'll go ahead and close this issue; thanks!

msridhar avatar Aug 23 '25 17:08 msridhar