Track access paths of the form `Foo.this.bar` (#937)

Fixes #936 

We add limited tracking of such access paths, to handle null checks on
fields from an enclosing class of a nested class.

2 files changed, 69 insertions, 0 deletions

diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java
index d4fa677..784d0d0 100644
--- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java
+++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java
@@ -46,6 +46,7 @@ import javax.lang.model.element.Element;
 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;
@@ -477,6 +478,19 @@ public final class AccessPath implements MapKey {
       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 {
diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayAccessPathsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayAccessPathsTests.java
index f3f0c07..7282e4e 100644
--- a/nullaway/src/test/java/com/uber/nullaway/NullAwayAccessPathsTests.java
+++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayAccessPathsTests.java
@@ -379,4 +379,59 @@ public class NullAwayAccessPathsTests extends NullAwayTestsBase {
             "}")
         .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();
+  }
 }