Fix crash with fully-qualified names in module-info.java import (#534)

Fixes #533 

We had no tests before for a `module-info.java` file, so we missed this regression.

4 files changed, 66 insertions, 8 deletions

diff --git a/jdk17-unit-tests/build.gradle b/jdk17-unit-tests/build.gradle
index 9d4c1b2..6752d8a 100644
--- a/jdk17-unit-tests/build.gradle
+++ b/jdk17-unit-tests/build.gradle
@@ -21,6 +21,13 @@ plugins {
 // Use JDK 17 for this module, via a toolchain
 java.toolchain.languageVersion.set JavaLanguageVersion.of(17)
 
+configurations {
+    // We use this configuration to expose a module path that can be
+    // used to test analysis of module-info.java files.
+    // See com.uber.nullaway.jdk17.NullAwayModuleInfoTests
+    testModulePath
+}
+
 dependencies {
     testImplementation project(":nullaway")
     testImplementation deps.test.junit4
@@ -28,6 +35,7 @@ dependencies {
         exclude group: "junit", module: "junit"
     }
     testImplementation deps.build.jsr305Annotations
+    testModulePath deps.test.cfQual
 }
 
 test {
@@ -44,6 +52,9 @@ test {
             "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED",
             "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED",
             "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED",
+            // Expose a module path for tests as a JVM property.
+            // Used by com.uber.nullaway.jdk17.NullAwayModuleInfoTests
+            "-Dtest.module.path=${configurations.testModulePath.asPath}"
     ]
 }
 
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
new file mode 100644
index 0000000..0f3cffc
--- /dev/null
+++ b/jdk17-unit-tests/src/test/java/com/uber/nullaway/jdk17/NullAwayModuleInfoTests.java
@@ -0,0 +1,43 @@
+package com.uber.nullaway.jdk17;
+
+import com.google.errorprone.CompilationTestHelper;
+import com.uber.nullaway.NullAway;
+import java.util.Arrays;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class NullAwayModuleInfoTests {
+
+  @Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+  private CompilationTestHelper defaultCompilationHelper;
+
+  @Before
+  public void setup() {
+    defaultCompilationHelper =
+        CompilationTestHelper.newInstance(NullAway.class, getClass())
+            .setArgs(
+                Arrays.asList(
+                    "-d",
+                    temporaryFolder.getRoot().getAbsolutePath(),
+                    // The module path system property is set in the build.gradle file to just
+                    // include the jar for the Checker Framework qualifier annotations
+                    "--module-path",
+                    System.getProperty("test.module.path"),
+                    "-XepOpt:NullAway:AnnotatedPackages=com.uber"));
+  }
+
+  @Test
+  public void testModuleInfo() {
+    // just check that the tool doesn't crash
+    defaultCompilationHelper
+        .addSourceLines(
+            "module-info.java",
+            "module com.uber.mymodule {",
+            "  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 d491c22..e8acbfb 100644
--- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java
+++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
@@ -430,8 +430,10 @@ public class NullAway extends BugChecker
       return Description.NO_MATCH;
     }
     Symbol symbol = ASTHelpers.getSymbol(tree);
-    // some checks for cases where we know it is not
-    // a null dereference
+    // 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()) {
       return Description.NO_MATCH;
     }
@@ -1929,7 +1931,9 @@ public class NullAway extends BugChecker
         exprMayBeNull = false;
         break;
       case MEMBER_SELECT:
-        exprMayBeNull = mayBeNullFieldAccess(state, expr, exprSymbol);
+        // 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);
         break;
       case IDENTIFIER:
         if (exprSymbol != null && exprSymbol.getKind().equals(ElementKind.FIELD)) {
diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
index 8a19015..758b5c5 100644
--- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
+++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
@@ -300,15 +300,15 @@ public class NullabilityUtil {
   /**
    * Check if a field might be null, based on the type.
    *
-   * @param symbol symbol for field
+   * @param symbol symbol for field; must be non-null
    * @param config NullAway config
    * @return true if based on the type, package, and name of the field, the analysis should assume
    *     the field might be null; false otherwise
+   * @throws NullPointerException if {@code symbol} is null
    */
-  public static boolean mayBeNullFieldFromType(@Nullable Symbol symbol, Config config) {
-    if (symbol == null) {
-      return true;
-    }
+  public static boolean mayBeNullFieldFromType(Symbol symbol, Config config) {
+    Preconditions.checkNotNull(
+        symbol, "mayBeNullFieldFromType should never be called with a null symbol");
     return !(symbol.getSimpleName().toString().equals("class")
             || symbol.isEnum()
             || isUnannotated(symbol, config))