Properly check generic method overriding in explicitly-typed anonymous classes #808
Conversation
msridhar
added
the
jspecify
| " public @Nullable String apply(String s) { return null; }", | ||
| " };", | ||
| " FnClass<String, @Nullable String> fn4 = new FnClass<String, @Nullable String>() {", | ||
| " public @Nullable String apply(String s) { return null; }", |
There was a problem hiding this comment.
Why are both true positive cases on the parameter and both true negatives on the return value? Seems like it might test more combinations to do one each?
| if (newClassTree == null) { | ||
| throw new RuntimeException( | ||
| "method should be inside a NewClassTree " + state.getSourceForNode(path.getLeaf())); | ||
| } |
There was a problem hiding this comment.
Do we want a check that the NewClassTree is for the same type as symbol['s supertype]? Or can that be safely assumed to never break?
There was a problem hiding this comment.
Can we add a test case for when the class name is given as something other than a simple name? new Foo.Builder<@Nullable String>() { ... } or even new Foo<@Nullable String>.Builder() {...}?
There was a problem hiding this comment.
Do we want a check that the
NewClassTreeis for the same type assymbol['s supertype]? Or can that be safely assumed to never break?
I added a check in af6ca1f; can't hurt :-)
Can we add a test case for when the class name is given as something other than a simple name?
new Foo.Builder<@Nullable String>() { ... }or evennew Foo<@Nullable String>.Builder() {...}?
I added some tests in 16eb9c2 but one of them is failing (see overrideAnonymousNestedClass()) 😐 I propose I fix that issue in a follow-up; does that sound ok?
…m/msridhar/NullAway into override-anonymous-class-checking
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #808 +/- ##
============================================
- Coverage 86.79% 86.75% -0.05%
- Complexity 1870 1876 +6
============================================
Files 74 74
Lines 6178 6197 +19
Branches 1202 1206 +4
============================================
+ Hits 5362 5376 +14
- Misses 405 408 +3
- Partials 411 413 +2
☔ View full report in Codecov by Sentry. |
|
@lazaroclapp I finally got around to addressing your feedback here; sorry for the long delay. |
| .doTest(); | ||
| } | ||
|
|
||
| @Ignore("Need to add support for this case") |
There was a problem hiding this comment.
Let's create and link an issue!
This completes a task lingering from #755, which did not handle overriding in anonymous classes. This case is slightly tricky as javac does not preserve annotations on type arguments in the types it computes for anonymous classes. To fix, we pass a
Symbolwhere we were passing aTypein a couple places, as then we can callSymbol.isAnonymous()to check for an anonymous type. In such a case, we try to get the type from the enclosingNewClassTreeof the overriding method. Note that this PR still does not handle anonymous classes that use the diamond operator; we add a test to show the gaps.