Fix bug with implicit equals() methods in interfaces #898
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #898 +/- ##
============================================
+ Coverage 86.95% 86.96% +0.01%
- Complexity 1953 1956 +3
============================================
Files 77 77
Lines 6315 6320 +5
Branches 1222 1224 +2
============================================
+ Hits 5491 5496 +5
+ Misses 420 419 -1
- Partials 404 405 +1 ☔ View full report in Codecov by Sentry. |
| @@ -985,4 +985,20 @@ public void cfgConstructionSymbolCompletionFailure() { | |||
| "}") | |||
| .doTest(); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testDefaultEqualsInInterfaceTakesNullable() { | |||
There was a problem hiding this comment.
Before this PR, this test would fail in the :nullaway:testJdk21 task
| if (methodSymbol.owner.isInterface()) { | ||
| Symbol.MethodSymbol baseSymbol = (Symbol.MethodSymbol) methodSymbol.baseSymbol(); | ||
| if (baseSymbol != methodSymbol && baseSymbol.owner == state.getSymtab().objectType.tsym) { | ||
| methodSymbol = baseSymbol; | ||
| } | ||
| } | ||
| return methodSymbol; | ||
| } |
There was a problem hiding this comment.
Correct me if I'm understanding this wrong, but previously we would get a symbol from the interface, which would eventually be considered annotated in the caller (matchMethodInvocation and later handleInvocation).
Now that we have this special handling to resolve to the base symbol (i.e., from java.lang.Object), which would be considered unannotated (everything from the JDK is unannotated), and then NullAway won't think that equals must have a nonnull param. Right?
If so, this change looks good to me! (I also agree that this is the interim solution and the end goal is for NullAway to enforce nullable on equals, but that's for another PR)
There was a problem hiding this comment.
@yuxincs yes, this is exactly correct.
Fixes #897
This fixes a regression in our handling of implicit
equals()methods in interfaces when building on JDK 21. I see this as an interim fix, until we can fix NullAway to properly always assume / enforce that the parameter ofequals()is@Nullable. But, I think we should fix the regression in the short term.