-
Notifications
You must be signed in to change notification settings - Fork 291
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
false-positive trigger on java17record.equals(null) #619
Comments
Right. Can definitely repro this, but a fix is slightly involved. Presumably, a @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 |
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 |
empty record gets compiled to 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
|
Right. We can handle this easily enough for a class, since the In a perfect world where JSpecify goes through the JSR process or something, we would have records generate an already 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 |
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 |
Running |
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. |
trips with
But equals to null on java17 records does work without producing NPE.
0.9.8
The text was updated successfully, but these errors were encountered: