Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
gavlyukovskiy opened this issue Aug 3, 2024 · 12 comments

Comments

@gavlyukovskiy
Copy link

gavlyukovskiy commented Aug 3, 2024

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
@msridhar
Copy link
Collaborator

msridhar commented Aug 3, 2024

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 #1005. Any chance you could try compiling using JDK 22 and see if that fixes the problem?

@gavlyukovskiy
Copy link
Author

gavlyukovskiy commented Aug 3, 2024

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

@msridhar
Copy link
Collaborator

msridhar commented Aug 3, 2024

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:

methodSymbol, ASTHelpers.getType(lambdaTree), state, config)

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
Copy link
Collaborator

msridhar commented Aug 3, 2024

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
Copy link
Author

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
Copy link
Collaborator

msridhar commented Aug 4, 2024

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

@gavlyukovskiy
Copy link
Author

Thank you @msridhar

@msridhar
Copy link
Collaborator

msridhar commented Aug 4, 2024

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
Copy link
Collaborator

msridhar commented Aug 5, 2024

There is now a PR openjdk/jdk#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 added a commit that referenced this issue Aug 11, 2024
We can enable the test on appropriate JDK versions once
openjdk/jdk#20460 lands. #1011
@msridhar
Copy link
Collaborator

Small update here. The above PR has landed and will ship with JDK 24. There is a backport for JDK 23: openjdk/jdk23u#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
Copy link
Author

gavlyukovskiy commented Aug 21, 2024

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
Copy link
Collaborator

@gavlyukovskiy there is an effort to get related fixes for reading of type annotations from bytecodes backported to JDK 21 and possibly earlier. See jspecify/jspecify#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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants