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 15, 2022 · e1efd1c · e1efd1c
1 parent 9bcce83
commit e1efd1c
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
@@ -137,21 +137,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();
     }
   }
 
@@ -390,6 +376,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) {
@@ -423,6 +411,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);
@@ -433,6 +422,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
@@ -450,7 +440,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)) {
@@ -470,6 +461,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
@@ -219,9 +219,6 @@ public void recognizesPatch() {
 
   @Test
   public void throwsExceptionWithBadPatchArgs() {
-    assertThrows(
-        InvalidCommandLineOptionException.class,
-        () -> ErrorProneOptions.processArgs(new String[] {"-XepPatchLocation:IN_PLACE"}));
     assertThrows(
         InvalidCommandLineOptionException.class,
         () ->
@@ -238,6 +235,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 =