false-positive trigger on java17record.equals(null)

[dmivankov]

• If you think you found a bug, please include a code sample that reproduces the problem. A test case that reproduces the issue is preferred. A stack trace alone is ok but may not contain enough context for us to address the issue.

class A {
    void test() {
        record Foo() {};
        new Foo().equals(null);
    }
}

trips with

error: [NullAway] passing @nullable parameter 'null' where @nonnull is required
new Foo().equals(null);

But equals to null on java17 records does work without producing NPE.

• Please include the library version number, including the minor and patch version, in the issue text.
  0.9.8

[lazaroclapp]

Right. Can definitely repro this, but a fix is slightly involved.

Presumably, a Foo::equals method symbol is generated for every record automatically and the java compiler doesn't add a @Nullable annotation to that generated method. At the same time, this will be inside the same package as Foo, which means it will be inside annotated code, and thus just adding a "library model" for it, or a handler won't be easy (we have Handler. onUnannotatedInvocationGetExplicitlyNullablePositions, but nothing equivalent for annotated code).

@msridhar : Do you think it's worth special-casing record methods inside NullAway core, then? (Seems like a candidate for core anyways, since records count as "a Java language feature" rather than a library) Can we easily do this without jeopardizing JDK8/JDK11 compatibility or performance?

A separate question is: are there other methods of record, that we will need special handling for?

[msridhar]

Question, do we get a warning on code like the following, which doesn't have records?

class X { 
  boolean equals(Object o) { return false; }
  static boolean foo() { return (new X()).equals(null); }
}

If we don't get a warning, how does that work? If we do get a warning, then I guess the issue here is that there is no explicit equals() method to annotate for a record?

[dmivankov]

empty record gets compiled to final class extending Record (via javap -c A\$1Foo.class )

Compiled from "A.java"
final class A$1Foo extends java.lang.Record {
  A$1Foo();
    Code:
       0: aload_0
       1: invokespecial #1                  // Method java/lang/Record."<init>":()V
       4: return

  public final java.lang.String toString();
    Code:
       0: aload_0
       1: invokedynamic #7,  0              // InvokeDynamic #0:toString:(LA$1Foo;)Ljava/lang/String;
       6: areturn

  public final int hashCode();
    Code:
       0: aload_0
       1: invokedynamic #11,  0             // InvokeDynamic #0:hashCode:(LA$1Foo;)I
       6: ireturn

  public final boolean equals(java.lang.Object);
    Code:
       0: aload_0
       1: aload_1
       2: invokedynamic #15,  0             // InvokeDynamic #0:equals:(LA$1Foo;Ljava/lang/Object;)Z
       7: ireturn
}

and it's also impossible to extend java.lang.Record directly (via javac at least), and not possible to override equals for records.
So maybe following would work

• treat java.lang.Record.equals as unannotated as well as equals of any (final) class extending Record

[lazaroclapp]

Right. We can handle this easily enough for a class, since the equals method must be explicitly declared or else is inherited from a "third-party" class for which we can have optimistic assumptions, library models, etc. But Record.equals(...) is more like generated code, which just happens to be part of the JDK17 semantics.

In a perfect world where JSpecify goes through the JSR process or something, we would have records generate an already @Nullable annotated equals(...) method. But we are unlikely to get that any time soon, so I think we might want to special case the method in core NullAway.

At least, the decompiled code above shows that this would be the only method of record for which we need to do this kind of override... something like if (isRecordType(classSymbol) && methodSymbol.getSimpleName().equals(EQUALS_NAME) && ...) { ... }, possibly at the level of out annotation retrieval layer so both dataflow and AST traversal see the "injected" argument annotation?

[msridhar]

I agree we can do a special case for records for now (though if I need to implement it, it will be a few weeks).

More broadly, I wonder if we should do a proper override check on Java equals() methods and force their parameter to be @Nullable. This could help expose some bugs as really any equals() method should handle a null parameter. But this could lead to a bunch of new error messages on existing code, so we'd need to think it over more.

[Stephan202]

But this could lead to a bunch of new error messages on existing code, so we'd need to think it over more.

Running EqualsMissingNullable over the Uber code base could help assess the impact.

[msridhar]

Running EqualsMissingNullable over the Uber code base could help assess the impact.

Thanks! Presumably this check could be used to auto fix existing issues as well. Might be worth making this change in NullAway and documenting the migration strategy.