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

Properly check generic method overriding in explicitly-typed anonymous classes #808

Merged
merged 94 commits into from
Sep 29, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
94 commits
Select commit Hold shift + click to select a range
362ee2f
all changes for handling method overrides
NikitaAware Mar 31, 2023
4a690c2
Merge branch 'master' into try-method-match
msridhar Apr 5, 2023
ac1e7f8
code for pretty printing generic types
msridhar Apr 26, 2023
241793d
better printing of types in errors related to generic types
msridhar Apr 27, 2023
7c86f30
fix compile error on JDK 8
msridhar Apr 27, 2023
899f615
Merge branch 'generic-type-pretty' into try-method-match
msridhar Apr 27, 2023
90b0c69
fix
msridhar Apr 27, 2023
74837a1
cleanup
msridhar Apr 27, 2023
6e3f2d0
better error message
msridhar Apr 27, 2023
56cddbe
WIP
msridhar Apr 27, 2023
b9d698c
Merge branch 'master' into check-toolchains
msridhar Apr 27, 2023
56fb488
test
msridhar Apr 27, 2023
304e567
reorder
msridhar Apr 27, 2023
7381c6e
just windows
msridhar Apr 27, 2023
2689a62
Merge branch 'master' into generic-type-pretty
msridhar Apr 27, 2023
5d7e5af
Merge branch 'check-toolchains' into generic-type-pretty
msridhar Apr 27, 2023
570a03a
Merge branch 'generic-type-pretty' into try-method-match
msridhar Apr 27, 2023
7cdb1b8
Merge branch 'master' into generic-type-pretty
msridhar Apr 27, 2023
a2dbade
Merge branch 'generic-type-pretty' into try-method-match
msridhar Apr 27, 2023
608f3db
tweaks
msridhar Apr 28, 2023
39c6e7d
cleanup
msridhar Apr 28, 2023
ac24db5
more cleanup
msridhar Apr 28, 2023
eb1ecae
cleanup
msridhar Apr 28, 2023
3bbd1d6
docs
msridhar Apr 30, 2023
ee73b04
more
msridhar May 6, 2023
e56cb12
more
msridhar May 6, 2023
05782eb
more
msridhar May 6, 2023
90a4530
more
msridhar May 6, 2023
a769e30
cleanup tests
msridhar May 6, 2023
6630bbf
another rename
msridhar May 6, 2023
b33ff48
WIP
msridhar May 6, 2023
8500957
more tests
msridhar May 7, 2023
250fb68
more
msridhar May 7, 2023
0593341
Make all methods in GenericsChecks static
msridhar May 7, 2023
68adca8
Merge branch 'make-generics-checks-methods-static' into override-anon…
msridhar May 7, 2023
1298081
more, still need tests for conditional expressions as receivers
msridhar May 7, 2023
fb3f418
Merge branch 'master' into try-method-match
msridhar May 8, 2023
e2bd583
Merge branch 'try-method-match' into make-generics-checks-methods-static
msridhar May 8, 2023
40affd0
Merge branch 'make-generics-checks-methods-static' into override-anon…
msridhar May 8, 2023
564c8dc
Merge branch 'master' into try-method-match
msridhar May 14, 2023
662606a
fix nullaway error
msridhar May 14, 2023
3de870c
Merge branch 'master' into try-method-match
msridhar Jun 14, 2023
f7d8dc9
Merge branch 'master' into try-method-match
msridhar Jun 14, 2023
08596f0
fix class ordering in test
msridhar Jun 14, 2023
97ec62a
bug fix: did not handle direct dereference of method call expression …
msridhar Jun 15, 2023
8bca16e
test interactions with @Contract
msridhar Jun 15, 2023
242b2e0
simplify using new API
msridhar Jun 15, 2023
631c417
better variable names
msridhar Jun 15, 2023
9024a67
rename variables and add a TODO
msridhar Jun 15, 2023
be06ff1
add a comment
msridhar Jun 15, 2023
87d194a
Merge branch 'try-method-match' into make-generics-checks-methods-static
msridhar Jun 20, 2023
b364573
Merge branch 'make-generics-checks-methods-static' into override-anon…
msridhar Jun 20, 2023
bb848fa
Merge branch 'master' into try-method-match
msridhar Jun 21, 2023
55ac129
Merge branch 'try-method-match' into make-generics-checks-methods-static
msridhar Jun 21, 2023
58dcd9f
Merge branch 'make-generics-checks-methods-static' into override-anon…
msridhar Jun 21, 2023
1ad0952
bug fix
msridhar Jun 21, 2023
7333529
delete unused method
msridhar Jun 21, 2023
09a2679
failing test
msridhar Jun 21, 2023
d0aa308
fixes
msridhar Jun 22, 2023
12e78ea
big simplification
msridhar Jun 22, 2023
2e36bde
comments
msridhar Jun 22, 2023
83b82fb
Merge branch 'master' into try-method-match
msridhar Jun 23, 2023
bc3b181
Merge branch 'master' into try-method-match
msridhar Jul 8, 2023
254c99a
Merge branch 'try-method-match' into make-generics-checks-methods-static
msridhar Jul 8, 2023
fee899b
Merge branch 'make-generics-checks-methods-static' into override-anon…
msridhar Jul 8, 2023
df25816
Merge branch 'master' into try-method-match
msridhar Jul 15, 2023
9582cff
Merge branch 'master' into try-method-match
msridhar Jul 18, 2023
4afd3d9
Merge branch 'master' into try-method-match
msridhar Aug 11, 2023
2e4a0e7
Merge branch 'try-method-match' into make-generics-checks-methods-static
msridhar Aug 11, 2023
bb94132
Merge branch 'make-generics-checks-methods-static' into override-anon…
msridhar Aug 11, 2023
aa2bc8c
Merge branch 'master' into try-method-match
msridhar Aug 11, 2023
dc1c5fa
Merge branch 'try-method-match' into make-generics-checks-methods-static
msridhar Aug 11, 2023
4f6c05b
Merge branch 'try-method-match' into override-anonymous-class-checking
msridhar Aug 11, 2023
2521dac
Merge branch 'make-generics-checks-methods-static' into override-anon…
msridhar Aug 11, 2023
eefe6e2
Merge branch 'master' into make-generics-checks-methods-static
msridhar Aug 12, 2023
549146c
Merge branch 'make-generics-checks-methods-static' into override-anon…
msridhar Aug 12, 2023
033b081
add test case
msridhar Aug 13, 2023
f7e0a87
Merge branch 'master' into override-anonymous-class-checking
msridhar Aug 18, 2023
f266006
fix comment
msridhar Aug 18, 2023
9a981dc
exclude test
msridhar Aug 18, 2023
3633f7b
Merge branch 'master' into override-anonymous-class-checking
msridhar Aug 21, 2023
d1a50bf
Merge branch 'master' into override-anonymous-class-checking
msridhar Aug 24, 2023
cfdda5f
Merge branch 'master' into override-anonymous-class-checking
msridhar Aug 25, 2023
ef854c4
Merge branch 'master' into override-anonymous-class-checking
msridhar Aug 30, 2023
ed5a7bc
Merge branch 'master' into override-anonymous-class-checking
msridhar Aug 30, 2023
b6c7081
Merge branch 'override-anonymous-class-checking' of https://github.co…
msridhar Aug 30, 2023
df4d0f7
Merge branch 'master' into override-anonymous-class-checking
msridhar Sep 1, 2023
bcc16c4
Merge branch 'master' into override-anonymous-class-checking
msridhar Sep 1, 2023
2721787
Merge branch 'master' into override-anonymous-class-checking
msridhar Sep 7, 2023
73255af
Merge branch 'master' into override-anonymous-class-checking
msridhar Sep 28, 2023
778447e
test more cases
msridhar Sep 28, 2023
af6ca1f
check that type is in fact a supertype
msridhar Sep 28, 2023
16eb9c2
add tests
msridhar Sep 29, 2023
26085c4
link issue and add static import
msridhar Sep 29, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions nullaway/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ def jdk8Test = tasks.register("testJdk8", Test) {
filter {
// JDK 8 does not support diamonds on anonymous classes
excludeTestsMatching "com.uber.nullaway.NullAwayJSpecifyGenericsTests.overrideDiamondAnonymousClass"
// tests cannot run on JDK 8 since Mockito version no longer supports it
excludeTestsMatching "com.uber.nullaway.NullAwaySerializationTest.initializationError"
excludeTestsMatching "com.uber.nullaway.handlers.contract.ContractUtilsTest.getEmptyAntecedent"
}
}

Expand Down
8 changes: 7 additions & 1 deletion nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static java.util.stream.Collectors.joining;

import com.google.common.base.Preconditions;
import com.google.common.base.Verify;
import com.google.errorprone.VisitorState;
import com.google.errorprone.suppliers.Supplier;
import com.google.errorprone.suppliers.Suppliers;
Expand All @@ -21,6 +22,7 @@
import com.sun.source.tree.ParameterizedTypeTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.VariableTree;
import com.sun.source.util.SimpleTreeVisitor;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Attribute;
import com.sun.tools.javac.code.BoundKind;
Expand Down Expand Up @@ -719,10 +721,14 @@
TreePath path = state.getPath();
NewClassTree newClassTree = ASTHelpers.findEnclosingNode(path, NewClassTree.class);
if (newClassTree == null) {
throw new RuntimeException(
"method should be inside a NewClassTree " + state.getSourceForNode(path.getLeaf()));

Check warning on line 725 in nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

View check run for this annotation

Codecov / codecov/patch

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java#L724-L725

Added lines #L724 - L725 were not covered by tests
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {...}?

Copy link
Collaborator Author

@msridhar msridhar Sep 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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 even new 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?

return getTreeType(newClassTree, state);
Type typeFromTree = getTreeType(newClassTree, state);
if (typeFromTree != null) {
Verify.verify(state.getTypes().isAssignable(symbol.type, typeFromTree));
}
return typeFromTree;
} else {
return symbol.type;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.google.errorprone.CompilationTestHelper;
import java.util.Arrays;
import org.junit.Ignore;
import org.junit.Test;

public class NullAwayJSpecifyGenericsTests extends NullAwayTestsBase {
Expand Down Expand Up @@ -993,16 +994,56 @@ public void overrideExplicitlyTypedAnonymousClass() {
" // BUG: Diagnostic contains: parameter s is @NonNull, but parameter in superclass method",
" public String apply(String s) { return s; }",
" };",
" FnClass<@Nullable String, String> fn2 = new FnClass<@Nullable String, String>() {",
" FnClass<String, String> fn2 = new FnClass<String, String>() {",
" // BUG: Diagnostic contains: method returns @Nullable, but superclass method",
" public @Nullable String apply(String s) { return null; }",
" };",
" Fn<String, @Nullable String> fn3 = new Fn<String, @Nullable String>() {",
" public @Nullable String apply(String s) { return null; }",
" };",
" FnClass<@Nullable String, String> fn4 = new FnClass<@Nullable String, String>() {",
" public String apply(@Nullable String s) { return \"hello\"; }",
" };",
" }",
" static void anonymousClassesFullName() {",
" Test.Fn<@Nullable String, String> fn1 = new Test.Fn<@Nullable String, String>() {",
" // BUG: Diagnostic contains: parameter s is @NonNull, but parameter in superclass method",
" public String apply(String s) { return s; }",
" };",
" Fn<String, @Nullable String> fn3 = new Fn<String, @Nullable String>() {",
" Test.FnClass<String, String> fn2 = new Test.FnClass<String, String>() {",
" // BUG: Diagnostic contains: method returns @Nullable, but superclass method",
" public @Nullable String apply(String s) { return null; }",
" };",
" FnClass<String, @Nullable String> fn4 = new FnClass<String, @Nullable String>() {",
" Test.Fn<String, @Nullable String> fn3 = new Test.Fn<String, @Nullable String>() {",
" public @Nullable String apply(String s) { return null; }",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

" };",
" Test.FnClass<@Nullable String, String> fn4 = new Test.FnClass<@Nullable String, String>() {",
" public String apply(@Nullable String s) { return \"hello\"; }",
" };",
" }",
"}")
.doTest();
}

@Ignore("Need to add support for this case")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create and link an issue!

@Test
public void overrideAnonymousNestedClass() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"class Test {",
" class Wrapper<P extends @Nullable Object> {",
" abstract class Fn<R extends @Nullable Object> {",
" abstract R apply(P p);",
" }",
" }",
" void anonymousNestedClasses() {",
" Wrapper<@Nullable String>.Fn<String> fn1 = (this.new Wrapper<@Nullable String>()).new Fn<String>() {",
" // BUG: Diagnostic contains: parameter s is @NonNull, but parameter in superclass method",
" public String apply(String s) { return s; }",
" };",
" }",
"}")
.doTest();
Expand Down