Handle duplicates when merging ScannerSuppliers

Loading different implementations of a check is still disallowed, but we can now tolerate e.g. loading the same check both as a built-in and as a plugin. RELNOTES: N/A ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=185219339
google · Feb 12, 2018 · 0bcd7ee · 0bcd7ee
1 parent be8fd8e
commit 0bcd7ee
Show file tree

Hide file tree

Showing 3 changed files with 172 additions and 19 deletions.
diff --git a/check_api/src/main/java/com/google/errorprone/scanner/ScannerSupplier.java b/check_api/src/main/java/com/google/errorprone/scanner/ScannerSupplier.java
@@ -19,6 +19,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Predicate;
 import com.google.common.base.Supplier;
+import com.google.common.collect.HashBiMap;
 import com.google.common.collect.ImmutableBiMap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -32,6 +33,7 @@
 import com.google.errorprone.InvalidCommandLineOptionException;
 import com.google.errorprone.bugpatterns.BugChecker;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.Map;
@@ -229,19 +231,42 @@ public ScannerSupplier applyOverrides(ErrorProneOptions errorProneOptions)
    */
   @CheckReturnValue
   public ScannerSupplier plus(ScannerSupplier other) {
-    ImmutableBiMap<String, BugCheckerInfo> combinedAllChecks =
+    HashBiMap<String, BugCheckerInfo> combinedAllChecks = HashBiMap.create(this.getAllChecks());
-        ImmutableBiMap.<String, BugCheckerInfo>builder()
+    other
-            .putAll(this.getAllChecks())
+        .getAllChecks()
-            .putAll(other.getAllChecks())
+        .forEach(
-            .build();
+            (k, v) -> {
-    ImmutableMap<String, SeverityLevel> combinedSeverities =
+              BugCheckerInfo existing = combinedAllChecks.putIfAbsent(k, v);
-        ImmutableMap.<String, SeverityLevel>builder()
+              if (existing != null
-            .putAll(severities())
+                  && !existing.checkerClass().getName().contentEquals(v.checkerClass().getName())) {
-            .putAll(other.severities())
+                throw new IllegalArgumentException(
-            .build();
+                    String.format(
+                        "Cannot combine scanner suppliers with different implementations of"
+                            + " '%s': %s, %s",
+                        k, v.checkerClass().getName(), existing.checkerClass().getName()));
+              }
+            });
+    HashMap<String, SeverityLevel> combinedSeverities = new LinkedHashMap<>(this.severities());
+    other
+        .severities()
+        .forEach(
+            (k, v) -> {
+              SeverityLevel existing = combinedSeverities.putIfAbsent(k, v);
+              if (existing != null && !existing.equals(v)) {
+                throw new IllegalArgumentException(
+                    String.format(
+                        "Cannot combine scanner suppliers with different severities for"
+                            + " '%s': %s, %s",
+                        k, v, existing));
+              }
+            });
     ImmutableSet<String> disabled = ImmutableSet.copyOf(Sets.union(disabled(), other.disabled()));
     ErrorProneFlags combinedFlags = this.getFlags().plus(other.getFlags());
-    return new ScannerSupplierImpl(combinedAllChecks, combinedSeverities, disabled, combinedFlags);
+    return new ScannerSupplierImpl(
+        ImmutableBiMap.copyOf(combinedAllChecks),
+        ImmutableMap.copyOf(combinedSeverities),
+        disabled,
+        combinedFlags);
   }
 
   /**

diff --git a/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java b/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java
@@ -21,11 +21,15 @@
 import static com.google.errorprone.BugPattern.Category.JDK;
 import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
 import static com.google.errorprone.scanner.BuiltInCheckerSuppliers.getSuppliers;
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.Assert.expectThrows;
 
+import com.google.common.base.Joiner;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
+import com.google.common.jimfs.Jimfs;
 import com.google.common.truth.FailureMetadata;
 import com.google.common.truth.Subject;
 import com.google.errorprone.BugCheckerInfo;
@@ -48,9 +52,23 @@
 import com.google.errorprone.bugpatterns.RestrictedApiChecker;
 import com.google.errorprone.bugpatterns.StaticQualifiedUsingExpression;
 import com.google.errorprone.bugpatterns.StringEquality;
+import com.sun.source.util.JavacTask;
+import com.sun.tools.javac.api.JavacTool;
+import com.sun.tools.javac.file.JavacFileManager;
+import java.io.IOException;
+import java.net.URI;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.nio.file.FileSystem;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.util.Collections;
+import java.util.Locale;
 import java.util.Map;
 import java.util.function.Supplier;
+import javax.tools.JavaFileObject.Kind;
+import javax.tools.SimpleJavaFileObject;
+import javax.tools.StandardLocation;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -96,16 +114,126 @@ public void plusWorks() {
             PreconditionsCheckNotNull.class);
   }
 
+  // Allow different instances of classes to be merged, provided they have the same name.
+  // This allows e.g. seeing the same check built in to Error Prone and loaded on the
+  // processorpath.
   @Test
-  public void plusDoesntAllowDuplicateChecks() {
+  public void plusAllowsDuplicateClassLoading() throws Exception {
-    ScannerSupplier ss1 =
+    FileSystem fileSystem = Jimfs.newFileSystem();
-        ScannerSupplier.fromBugCheckerClasses(
+
-            ArrayEquals.class, StaticQualifiedUsingExpression.class);
+    Class<? extends BugChecker> class1 =
-    ScannerSupplier ss2 = ScannerSupplier.fromBugCheckerClasses(ArrayEquals.class);
+        compileAndLoadChecker(
+            fileSystem,
+            "com.google.errorprone.bugpatterns.TestChecker",
+            "package com.google.errorprone.bugpatterns;",
+            "import com.google.errorprone.BugPattern;",
+            "@BugPattern(name = \"TestChecker\", summary = \"\","
+                + " severity = BugPattern.SeverityLevel.WARNING)",
+            "public class TestChecker extends BugChecker {}");
+
+    Class<? extends BugChecker> class2 =
+        compileAndLoadChecker(
+            fileSystem,
+            "com.google.errorprone.bugpatterns.TestChecker",
+            "package com.google.errorprone.bugpatterns;",
+            "import com.google.errorprone.BugPattern;",
+            "@BugPattern(name = \"TestChecker\", summary = \"\","
+                + " severity = BugPattern.SeverityLevel.WARNING)",
+            "public class TestChecker extends BugChecker {}");
+
+    ScannerSupplier ss1 = ScannerSupplier.fromBugCheckerClasses(class1);
+    ScannerSupplier ss2 = ScannerSupplier.fromBugCheckerClasses(class2);
+
+    assertThat(class1).isNotEqualTo(class2);
+    ScannerSupplier ss = ss1.plus(ss2);
+    assertThat(ss.getAllChecks()).hasSize(1);
+    assertThat(Iterables.getOnlyElement(ss.getAllChecks().values()).checkerClass())
+        .isEqualTo(class1);
+  }
+
+  /** Another check with the canonical name ArrayEquals, for testing. */
+  @BugPattern(name = "ArrayEquals", summary = "", severity = ERROR)
+  public static class OtherArrayEquals extends BugChecker {}
+
+  @Test
+  public void plusDisallowsDuplicates() throws Exception {
+    ScannerSupplier ss1 = ScannerSupplier.fromBugCheckerClasses(ArrayEquals.class);
+    ScannerSupplier ss2 = ScannerSupplier.fromBugCheckerClasses(OtherArrayEquals.class);
 
-    IllegalArgumentException expected =
+    IllegalArgumentException thrown =
         expectThrows(IllegalArgumentException.class, () -> ss1.plus(ss2));
-    assertThat(expected.getMessage()).contains("ArrayEquals");
+    assertThat(thrown)
+        .hasMessageThat()
+        .contains(
+            "different implementations of 'ArrayEquals':"
+                + " com.google.errorprone.scanner.ScannerSupplierTest$OtherArrayEquals,"
+                + " com.google.errorprone.bugpatterns.ArrayEquals");
+  }
+
+  @Test
+  public void plusDisallowsDifferentSeverities() throws Exception {
+    FileSystem fileSystem = Jimfs.newFileSystem();
+
+    Class<? extends BugChecker> class1 =
+        compileAndLoadChecker(
+            fileSystem,
+            "com.google.errorprone.bugpatterns.TestChecker",
+            "package com.google.errorprone.bugpatterns;",
+            "import com.google.errorprone.BugPattern;",
+            "@BugPattern(name = \"TestChecker\", summary = \"\","
+                + " severity = BugPattern.SeverityLevel.ERROR)",
+            "public class TestChecker extends BugChecker {}");
+
+    Class<? extends BugChecker> class2 =
+        compileAndLoadChecker(
+            fileSystem,
+            "com.google.errorprone.bugpatterns.TestChecker",
+            "package com.google.errorprone.bugpatterns;",
+            "import com.google.errorprone.BugPattern;",
+            "@BugPattern(name = \"TestChecker\", summary = \"\","
+                + " severity = BugPattern.SeverityLevel.WARNING)",
+            "public class TestChecker extends BugChecker {}");
+
+    ScannerSupplier ss1 = ScannerSupplier.fromBugCheckerClasses(class1);
+    ScannerSupplier ss2 = ScannerSupplier.fromBugCheckerClasses(class2);
+
+    IllegalArgumentException thrown =
+        expectThrows(IllegalArgumentException.class, () -> ss1.plus(ss2));
+
+    assertThat(class1).isNotEqualTo(class2);
+    assertThat(thrown)
+        .hasMessageThat()
+        .contains("different severities for 'TestChecker': WARNING, ERROR");
+  }
+
+  static Class<? extends BugChecker> compileAndLoadChecker(
+      FileSystem fileSystem, String name, String... lines)
+      throws IOException, ClassNotFoundException {
+    JavacTool javacTool = JavacTool.create();
+    JavacFileManager fileManager =
+        javacTool.getStandardFileManager(null, Locale.getDefault(), UTF_8);
+    Path tmp = fileSystem.getPath("tmp");
+    Files.createDirectories(tmp);
+    Path output = Files.createTempDirectory(tmp, "output");
+    fileManager.setLocationFromPaths(StandardLocation.CLASS_OUTPUT, ImmutableList.of(output));
+    JavacTask task =
+        javacTool.getTask(
+            null,
+            fileManager,
+            null,
+            Collections.emptyList(),
+            null,
+            Collections.singletonList(
+                new SimpleJavaFileObject(
+                    URI.create(name.replace('.', '/') + ".java"), Kind.SOURCE) {
+                  @Override
+                  public CharSequence getCharContent(boolean b) throws IOException {
+                    return Joiner.on('\n').join(lines);
+                  }
+                }));
+    assertThat(task.call()).isTrue();
+    return Class.forName(name, true, new URLClassLoader(new URL[] {output.toUri().toURL()}))
+        .asSubclass(BugChecker.class);
   }
 
   @Test

diff --git a/test_helpers/pom.xml b/test_helpers/pom.xml
@@ -139,7 +139,7 @@
       <!-- Apache 2.0 -->
       <groupId>com.google.jimfs</groupId>
       <artifactId>jimfs</artifactId>
-      <version>1.0</version>
+      <version>1.1</version>
     </dependency>
     <dependency>
       <!-- Apache 2.0 -->