Better fix for crash on member selects inside module-info.java (#544)

Now, in `matchMemberSelectTree()`, we check if the `MemberSelectTree` represents a reference to a module.  If it does, we do not check if its base expression could be `null`.  This change gives us a stronger invariant in `mayBeNullExpr()`: any `IdentifierTree` or `MemberSelectTree` passed to `mayBeNullExpr()` should have a non-null symbol.  We explicitly check this to help us flag any future weird cases more easily.

Unfortunately, to maintain JDK 8 compatibility, the check for whether a `MemberSelectTree` represents a module requires reflection.  Testing did not reveal any significant performance impact.  If / when NullAway requires JDK 11, we can and should get rid of the reflection.

Code coverage is reduced due to the new runtime checks for (hopefully) impossible cases in `mayBeNullExpr()`.

Fixes #533 

2 files changed, 46 insertions, 8 deletions

diff --git a/jdk17-unit-tests/src/test/java/com/uber/nullaway/jdk17/NullAwayModuleInfoTests.java b/jdk17-unit-tests/src/test/java/com/uber/nullaway/jdk17/NullAwayModuleInfoTests.java
index 0f3cffc..64f0966 100644
--- a/jdk17-unit-tests/src/test/java/com/uber/nullaway/jdk17/NullAwayModuleInfoTests.java
+++ b/jdk17-unit-tests/src/test/java/com/uber/nullaway/jdk17/NullAwayModuleInfoTests.java
@@ -36,6 +36,9 @@ public class NullAwayModuleInfoTests {
         .addSourceLines(
             "module-info.java",
             "module com.uber.mymodule {",
+            "  // Important: two-level deep module tests matching of identifier `java` as base expression;",
+            "  // see further discussion at https://github.com/uber/NullAway/pull/544#discussion_r780829467",
+            "  requires java.base;",
             "  requires static org.checkerframework.checker.qual;",
             "}")
         .doTest();
diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
index e8acbfb..7b2fd19 100644
--- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java
+++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
@@ -223,6 +223,14 @@ public class NullAway extends BugChecker
   private final Map<ExpressionTree, Nullness> computedNullnessMap = new LinkedHashMap<>();
 
   /**
+   * Used to check if a symbol represents a module in {@link #matchMemberSelect(MemberSelectTree,
+   * VisitorState)}. We need to use reflection to preserve compatibility with Java 8.
+   *
+   * <p>TODO remove this once NullAway requires JDK 11
+   */
+  @Nullable private final Class<?> moduleElementClass;
+
+  /**
    * Error Prone requires us to have an empty constructor for each Plugin, in addition to the
    * constructor taking an ErrorProneFlags object. This constructor should not be used anywhere
    * else. Checker objects constructed with this constructor will fail with IllegalStateException if
@@ -233,6 +241,7 @@ public class NullAway extends BugChecker
     handler = Handlers.buildEmpty();
     nonAnnotatedMethod = this::isMethodUnannotated;
     errorBuilder = new ErrorBuilder(config, "", ImmutableSet.of());
+    moduleElementClass = null;
   }
 
   public NullAway(ErrorProneFlags flags) {
@@ -240,6 +249,14 @@ public class NullAway extends BugChecker
     handler = Handlers.buildDefault(config);
     nonAnnotatedMethod = this::isMethodUnannotated;
     errorBuilder = new ErrorBuilder(config, canonicalName(), allNames());
+    Class<?> moduleElementClass = null;
+    try {
+      moduleElementClass =
+          getClass().getClassLoader().loadClass("javax.lang.model.element.ModuleElement");
+    } catch (ClassNotFoundException e) {
+      // can occur pre JDK 11
+    }
+    this.moduleElementClass = moduleElementClass;
   }
 
   private boolean isMethodUnannotated(MethodInvocationNode invocationNode) {
@@ -431,10 +448,13 @@ public class NullAway extends BugChecker
     }
     Symbol symbol = ASTHelpers.getSymbol(tree);
     // Some checks for cases where we know this cannot be a null dereference.  The tree's symbol may
-    // be null in cases where the tree represents a package name, e.g., in the package declaration
-    // in a class, or in a requires clause in a module-info.java file; it should never be null for a
-    // real field dereference or method call
-    if (symbol == null || symbol.getSimpleName().toString().equals("class") || symbol.isEnum()) {
+    // be null in cases where the tree represents part of a package name, e.g., in the package
+    // declaration in a class, or in a requires clause in a module-info.java file; it should never
+    // be null for a real field dereference or method call
+    if (symbol == null
+        || symbol.getSimpleName().toString().equals("class")
+        || symbol.isEnum()
+        || isModuleSymbol(symbol)) {
       return Description.NO_MATCH;
     }
 
@@ -450,6 +470,15 @@ public class NullAway extends BugChecker
     return Description.NO_MATCH;
   }
 
+  /**
+   * Checks if {@code symbol} represents a JDK 9+ module using reflection.
+   *
+   * <p>TODO just check using instanceof once NullAway requires JDK 11
+   */
+  private boolean isModuleSymbol(Symbol symbol) {
+    return moduleElementClass != null && moduleElementClass.isAssignableFrom(symbol.getClass());
+  }
+
   @Override
   public Description matchMethod(MethodTree tree, VisitorState state) {
     if (!matchWithinTopLevelClass) {
@@ -1931,12 +1960,18 @@ public class NullAway extends BugChecker
         exprMayBeNull = false;
         break;
       case MEMBER_SELECT:
-        // A MemberSelectTree for a field dereference or method call cannot have a null symbol; see
-        // comments in matchMemberSelect()
-        exprMayBeNull = exprSymbol == null ? false : mayBeNullFieldAccess(state, expr, exprSymbol);
+        if (exprSymbol == null) {
+          throw new IllegalStateException(
+              "unexpected null symbol for dereference expression " + state.getSourceForNode(expr));
+        }
+        exprMayBeNull = mayBeNullFieldAccess(state, expr, exprSymbol);
         break;
       case IDENTIFIER:
-        if (exprSymbol != null && exprSymbol.getKind().equals(ElementKind.FIELD)) {
+        if (exprSymbol == null) {
+          throw new IllegalStateException(
+              "unexpected null symbol for identifier " + state.getSourceForNode(expr));
+        }
+        if (exprSymbol.getKind().equals(ElementKind.FIELD)) {
           // Special case: mayBeNullFieldAccess runs handler.onOverrideMayBeNullExpr before
           // dataflow.
           return mayBeNullFieldAccess(state, expr, exprSymbol);