Support running all available patch checks

The `BaseErrorProneJavaCompiler` already supported this, but `ErrorProneOptions` prevented the feature from being used. Concrete changes: 1. Require `-XepPatchLocation` when `-XepPatchChecks` is specified, but not vice versa. 2. Omit empty strings from `-XepPatchChecks:`. See google#947
PicnicSupermarket · Apr 25, 2018 · d55d05f · d55d05f
1 parent 201e0fe
commit d55d05f
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 19 deletions.
diff --git a/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java b/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java
@@ -132,21 +132,7 @@ abstract static class Builder {
 
       abstract Builder importOrganizer(ImportOrganizer importOrganizer);
 
-      abstract PatchingOptions autoBuild();
-
-      final PatchingOptions build() {
-
-        PatchingOptions patchingOptions = autoBuild();
-
-        // If anything is specified, then (checkers or refaster) and output must be set.
-        if ((!patchingOptions.namedCheckers().isEmpty()
-                || patchingOptions.customRefactorer().isPresent())
-            ^ patchingOptions.doRefactor()) {
-          throw new InvalidCommandLineOptionException(
-              "-XepPatchChecks and -XepPatchLocation must be specified together");
-        }
-        return patchingOptions;
-      }
+      abstract PatchingOptions build();
     }
   }
 
@@ -347,6 +333,8 @@ public static ErrorProneOptions processArgs(Iterable<String> args) {
      * You can pass the IGNORE_UNKNOWN_CHECKS_FLAG to opt-out of that checking.  This allows you to
      * use command lines from different versions of error-prone interchangeably.
      */
+    boolean patchLocationSet = false;
+    boolean patchCheckSet = false;
     Builder builder = new Builder();
     for (String arg : args) {
       switch (arg) {
@@ -374,6 +362,7 @@ public static ErrorProneOptions processArgs(Iterable<String> args) {
           } else if (arg.startsWith(ErrorProneFlags.PREFIX)) {
             builder.parseFlag(arg);
           } else if (arg.startsWith(PATCH_OUTPUT_LOCATION)) {
+            patchLocationSet = true;
             String remaining = arg.substring(PATCH_OUTPUT_LOCATION.length());
             if (remaining.equals("IN_PLACE")) {
               builder.patchingOptionsBuilder().inPlace(true);
@@ -384,6 +373,7 @@ public static ErrorProneOptions processArgs(Iterable<String> args) {
               builder.patchingOptionsBuilder().baseDirectory(remaining);
             }
           } else if (arg.startsWith(PATCH_CHECKS_PREFIX)) {
+            patchCheckSet = true;
             String remaining = arg.substring(PATCH_CHECKS_PREFIX.length());
             if (remaining.startsWith("refaster:")) {
               // Refaster rule, load from InputStream at file
@@ -401,7 +391,8 @@ public static ErrorProneOptions processArgs(Iterable<String> args) {
                         }
                       });
             } else {
-              Iterable<String> checks = Splitter.on(',').trimResults().split(remaining);
+              Iterable<String> checks =
+                  Splitter.on(',').trimResults().omitEmptyStrings().split(remaining);
               builder.patchingOptionsBuilder().namedCheckers(ImmutableSet.copyOf(checks));
             }
           } else if (arg.startsWith(PATCH_IMPORT_ORDER_PREFIX)) {
@@ -417,6 +408,11 @@ public static ErrorProneOptions processArgs(Iterable<String> args) {
       }
     }
 
+    if (patchCheckSet && !patchLocationSet) {
+      throw new InvalidCommandLineOptionException(
+          "-XepPatchLocation must be specified when -XepPatchChecks is");
+    }
+
     return builder.build(remainingArgs.build());
   }
 

diff --git a/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java b/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java
@@ -207,9 +207,6 @@ public void recognizesPatch() {
 
   @Test
   public void throwsExceptionWithBadPatchArgs() {
-    assertThrows(
-        InvalidCommandLineOptionException.class,
-        () -> ErrorProneOptions.processArgs(new String[] {"-XepPatchLocation:IN_PLACE"}));
     assertThrows(
         InvalidCommandLineOptionException.class,
         () ->
@@ -226,6 +223,24 @@ public void recognizesRefaster() {
     assertThat(options.patchingOptions().customRefactorer()).isPresent();
   }
 
+  @Test
+  public void understandsEmptySetOfNamedCheckers() {
+    ErrorProneOptions options =
+        ErrorProneOptions.processArgs(new String[] {"-XepPatchLocation:IN_PLACE"});
+    assertThat(options.patchingOptions().doRefactor()).isTrue();
+    assertThat(options.patchingOptions().inPlace()).isTrue();
+    assertThat(options.patchingOptions().namedCheckers()).isEmpty();
+    assertThat(options.patchingOptions().customRefactorer()).isAbsent();
+
+    options =
+        ErrorProneOptions.processArgs(
+            new String[] {"-XepPatchLocation:IN_PLACE", "-XepPatchChecks:"});
+    assertThat(options.patchingOptions().doRefactor()).isTrue();
+    assertThat(options.patchingOptions().inPlace()).isTrue();
+    assertThat(options.patchingOptions().namedCheckers()).isEmpty();
+    assertThat(options.patchingOptions().customRefactorer()).isAbsent();
+  }
+
   @Test
   public void importOrder_staticFirst() {
     ErrorProneOptions options =