Disable JarInfer handler by default (#261)

This is mostly so that we can have a somewhat-functional NullAway on JDK 11 sooner (see #259). But also, removing a handler may give a small performance boost. With this change, you need to pass `-XepOpt:NullAway:JarInferEnabled=true` to enable JarInfer. Also, we separate out the JarInfer tests into their own module, so that there is no direct dependence from the `nullaway` module to the `jarinfer` libraries. This will enable us to get the `:nullaway:test` target passing on JDK 11 sooner.
uber · Dec 3, 2018 · cf8b704 · cf8b704
1 parent c3ab5d2
commit cf8b704
Show file tree

Hide file tree

Showing 10 changed files with 142 additions and 51 deletions.
diff --git a/jar-infer/nullaway-integration-test/build.gradle b/jar-infer/nullaway-integration-test/build.gradle
@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) 2017. Uber Technologies
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+plugins {
+  id "java"
+  // For code coverage:
+  id 'jacoco'
+  id 'com.github.kt3k.coveralls'
+}
+
+sourceCompatibility = "1.8"
+targetCompatibility = "1.8"
+
+dependencies {
+    testCompile deps.test.junit4
+    testCompile(deps.build.errorProneTestHelpers) {
+        exclude group: "junit", module: "junit"
+    }
+    testCompile project(":nullaway")
+    testCompile project(":jar-infer:test-java-lib-jarinfer")
+
+}
+
+
+test {
+  maxHeapSize = "1024m"
+  jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}"
+}
+
+// From https://github.com/kt3k/coveralls-gradle-plugin
+jacocoTestReport {
+    reports {
+        xml.enabled = true // coveralls plugin depends on xml format report
+        html.enabled = true
+    }
+}
diff --git a/...ay-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java b/...ay-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java
@@ -0,0 +1,70 @@
+package com.uber.nullaway.jarinfer;
+
+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 JarInferIntegrationTest {
+
+  @Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+  private CompilationTestHelper compilationHelper;
+
+  @Before
+  public void setup() {
+    compilationHelper = CompilationTestHelper.newInstance(NullAway.class, getClass());
+  }
+
+  @Test
+  public void jarinferLoadStubsTest() {
+    compilationHelper
+        .setArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:JarInferEnabled=true",
+                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.nullaway.[a-zA-Z0-9.]+.unannotated"))
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "import com.uber.nullaway.jarinfer.toys.unannotated.Toys;",
+            "class Test {",
+            "  void test1(@Nullable String s) {",
+            "    // BUG: Diagnostic contains: passing @Nullable parameter 's'",
+            "    Toys.test1(s, \"let's\", \"try\");",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void jarinferNullableReturnsTest() {
+    compilationHelper
+        .setArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.nullaway.[a-zA-Z0-9.]+.unannotated",
+                "-XepOpt:NullAway:JarInferEnabled=true",
+                "-XepOpt:NullAway:JarInferUseReturnAnnotations=true"))
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "import com.uber.nullaway.jarinfer.toys.unannotated.Toys;",
+            "class Test {",
+            "  void test1(@Nullable String s) {",
+            "    // BUG: Diagnostic contains: passing @Nullable parameter 'Toys.getString(false, s)'",
+            "    Toys.test1(Toys.getString(false, s), \"let's\", \"try\");",
+            "  }",
+            "}")
+        .doTest();
+  }
+}
diff --git a/nullaway/build.gradle b/nullaway/build.gradle
@@ -51,7 +51,6 @@ dependencies {
     testCompile deps.test.cfQual
     testCompile deps.test.cfCompatQual
     testCompile project(":test-java-lib")
-    testCompile project(":jar-infer:test-java-lib-jarinfer")
     testCompile deps.test.inferAnnotations
     testCompile deps.apt.javaxInject
     testCompile deps.test.rxjava2

diff --git a/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java b/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java
@@ -89,6 +89,8 @@ public abstract class AbstractConfig implements Config {
   protected String autofixSuppressionComment;
 
   /** --- JarInfer configs --- */
+  protected boolean jarInferEnabled;
+
   protected boolean jarInferUseReturnAnnotations;
 
   protected String jarInferRegexStripModelJarName;
@@ -229,6 +231,11 @@ static MethodClassAndName create(String enclosingClass, String methodName) {
   }
 
   /** --- JarInfer configs --- */
+  @Override
+  public boolean isJarInferEnabled() {
+    return jarInferEnabled;
+  }
+
   @Override
   public boolean isJarInferUseReturnAnnotations() {
     return jarInferUseReturnAnnotations;

diff --git a/nullaway/src/main/java/com/uber/nullaway/Config.java b/nullaway/src/main/java/com/uber/nullaway/Config.java
@@ -121,9 +121,12 @@ public interface Config {
    */
   String getAutofixSuppressionComment();
 
+  // --- JarInfer configs ---
+
+  /** @return true if JarInfer should be enabled */
+  boolean isJarInferEnabled();
+
   /**
-   * --- JarInfer configs ---
-   *
    * @return true if NullAway should use the @Nullable return value annotations inferred by
    *     JarInfer.
    */

diff --git a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java
@@ -123,6 +123,11 @@ public String getAutofixSuppressionComment() {
     throw new IllegalStateException(error_msg);
   }
 
+  @Override
+  public boolean isJarInferEnabled() {
+    throw new IllegalStateException(error_msg);
+  }
+
   /** --- JarInfer configs --- */
   @Override
   public boolean isJarInferUseReturnAnnotations() {

diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java
@@ -57,6 +57,8 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
       EP_FL_NAMESPACE + ":AcknowledgeRestrictiveAnnotations";
   static final String FL_SUPPRESS_COMMENT = EP_FL_NAMESPACE + ":AutoFixSuppressionComment";
   /** --- JarInfer configs --- */
+  static final String FL_JI_ENABLED = EP_FL_NAMESPACE + ":JarInferEnabled";
+
   static final String FL_JI_USE_RETURN = EP_FL_NAMESPACE + ":JarInferUseReturnAnnotations";
 
   static final String FL_JI_REGEX_MODEL_PATH = EP_FL_NAMESPACE + ":JarInferRegexStripModelJar";
@@ -122,6 +124,7 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
           "Invalid -XepOpt" + FL_SUPPRESS_COMMENT + " value. Comment must be single line.");
     }
     /** --- JarInfer configs --- */
+    jarInferEnabled = flags.getBoolean(FL_JI_ENABLED).orElse(false);
     jarInferUseReturnAnnotations = flags.getBoolean(FL_JI_USE_RETURN).orElse(false);
     // The defaults of these two options translate to: remove .aar/.jar from the file name, and also
     // implicitly mean that NullAway will search for jarinfer models in the same jar which contains

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java
@@ -43,7 +43,9 @@ public static Handler buildDefault(Config config) {
       // bytecode annotations
       handlerListBuilder.add(new RestrictiveAnnotationHandler(config));
     }
-    handlerListBuilder.add(new InferredJARModelsHandler(config));
+    if (config.isJarInferEnabled()) {
+      handlerListBuilder.add(new InferredJARModelsHandler(config));
+    }
     handlerListBuilder.add(new LibraryModelsHandler());
     handlerListBuilder.add(new RxNullabilityPropagator());
     handlerListBuilder.add(new ContractHandler());

diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java
@@ -1207,53 +1207,6 @@ public void testReadStaticInConstructor() {
         .doTest();
   }
 
-  @Test
-  public void jarinferLoadStubsTest() {
-    compilationHelper
-        .setArgs(
-            Arrays.asList(
-                "-d",
-                temporaryFolder.getRoot().getAbsolutePath(),
-                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
-                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.nullaway.[a-zA-Z0-9.]+.unannotated"))
-        .addSourceLines(
-            "Test.java",
-            "package com.uber;",
-            "import javax.annotation.Nullable;",
-            "import com.uber.nullaway.jarinfer.toys.unannotated.Toys;",
-            "class Test {",
-            "  void test1(@Nullable String s) {",
-            "    // BUG: Diagnostic contains: passing @Nullable parameter 's'",
-            "    Toys.test1(s, \"let's\", \"try\");",
-            "  }",
-            "}")
-        .doTest();
-  }
-
-  @Test
-  public void jarinferNullableReturnsTest() {
-    compilationHelper
-        .setArgs(
-            Arrays.asList(
-                "-d",
-                temporaryFolder.getRoot().getAbsolutePath(),
-                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
-                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.nullaway.[a-zA-Z0-9.]+.unannotated",
-                "-XepOpt:NullAway:JarInferUseReturnAnnotations=true"))
-        .addSourceLines(
-            "Test.java",
-            "package com.uber;",
-            "import javax.annotation.Nullable;",
-            "import com.uber.nullaway.jarinfer.toys.unannotated.Toys;",
-            "class Test {",
-            "  void test1(@Nullable String s) {",
-            "    // BUG: Diagnostic contains: passing @Nullable parameter 'Toys.getString(false, s)'",
-            "    Toys.test1(Toys.getString(false, s), \"let's\", \"try\");",
-            "  }",
-            "}")
-        .doTest();
-  }
-
   @Test
   public void customErrorURL() {
     compilationHelper

diff --git a/settings.gradle b/settings.gradle
@@ -23,3 +23,4 @@ include ':jar-infer:android-jarinfer-models-sdk28'
 include ':jar-infer:jar-infer-lib'
 include ':jar-infer:jar-infer-cli'
 include ':jar-infer:test-java-lib-jarinfer'
+include ':jar-infer:nullaway-integration-test'