Skip to content

Commit

Permalink
Track access paths of the form Foo.this.bar (#937)
Browse files Browse the repository at this point in the history
Fixes #936 

We add limited tracking of such access paths, to handle null checks on
fields from an enclosing class of a nested class.
  • Loading branch information
msridhar authored Mar 22, 2024
1 parent 3bde26c commit 5bedf0a
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 0 deletions.
14 changes: 14 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import javax.lang.model.element.ElementKind;
import javax.lang.model.element.Modifier;
import javax.lang.model.element.VariableElement;
import org.checkerframework.nullaway.dataflow.cfg.node.ClassNameNode;
import org.checkerframework.nullaway.dataflow.cfg.node.FieldAccessNode;
import org.checkerframework.nullaway.dataflow.cfg.node.IntegerLiteralNode;
import org.checkerframework.nullaway.dataflow.cfg.node.LocalVariableNode;
Expand Down Expand Up @@ -477,6 +478,19 @@ && isBoxingMethod(ASTHelpers.getSymbol(methodInvocationTree))) {
result =
new AccessPath(
((LocalVariableNode) node).getElement(), ImmutableList.copyOf(elements), mapKey);
} else if (node instanceof ClassNameNode) {
// It is useful to make an access path if elements.size() > 1 and elements.getFirst() is
// "this". In this case, we may have an access of a field of an enclosing class from a nested
// class. Tracking an access path for "Foo.this" alone is not useful, since that can never be
// null. Also, we do not attempt to canonicalize cases where "Foo.this" is used to refer to
// the receiver of the current method instead of "this".
if (elements.size() > 1
&& elements.getFirst().getJavaElement().getSimpleName().contentEquals("this")) {
Element rootElement = elements.pop().getJavaElement();
result = new AccessPath(rootElement, ImmutableList.copyOf(elements), mapKey);
} else {
result = null;
}
} else if (node instanceof ThisNode || node instanceof SuperNode) {
result = new AccessPath(null, ImmutableList.copyOf(elements), mapKey);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -379,4 +379,59 @@ public void testFinalFieldNullabilityPropagatesToInnerContexts() {
"}")
.doTest();
}

@Test
public void testAccessUsingExplicitThis() {
defaultCompilationHelper
.addSourceLines(
"Foo.java",
"package com.uber;",
"import javax.annotation.Nullable;",
"public class Foo {",
" @Nullable public Object bar;",
" public class Nested {",
" @Nullable public Object bar;",
" public void testNegative1() {",
" if (Foo.this.bar != null) {",
" Foo.this.bar.toString();",
" }",
" }",
" public void testNegative2() {",
" // Foo.this can never be null",
" Foo.this.toString();",
" }",
" public void testPositive() {",
" if (Foo.this.bar != null) {",
" // BUG: Diagnostic contains: dereferenced expression this.bar is @Nullable",
" this.bar.toString();",
" }",
" if (this.bar != null) {",
" // BUG: Diagnostic contains: dereferenced expression Foo.this.bar is @Nullable",
" Foo.this.bar.toString();",
" }",
" }",
" }",
" public void testUnhandled1() {",
" if (bar != null) {",
" // This is safe but we don't currently handle it",
" // BUG: Diagnostic contains: dereferenced expression Foo.this.bar is @Nullable",
" Foo.this.bar.toString();",
" }",
" }",
" public void testUnhandled2() {",
" if (Foo.this.bar != null) {",
" // This is safe but we don't currently handle it",
" // BUG: Diagnostic contains: dereferenced expression bar is @Nullable",
" bar.toString();",
" }",
" }",
" public void testNegative1() {",
" if (bar != null) {",
" // This is safe and handled",
" this.bar.toString();",
" }",
" }",
"}")
.doTest();
}
}

0 comments on commit 5bedf0a

Please sign in to comment.