From 35e51f3824ac35139b65f8130006957d5c4bbd32 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Wed, 13 Sep 2023 08:56:44 -0700 Subject: [PATCH] Support running all available patch checks The `BaseErrorProneJavaCompiler` [supports](https://github.com/google/error-prone/blob/master/check_api/src/main/java/com/google/errorprone/BaseErrorProneJavaCompiler.java#L225) running _all_ patch checks; it just requires that one doesn't provide any explicitly named checkers. In practice this doesn't work, however, because: - `PatchingOptions` [requires](https://github.com/google/error-prone/blob/master/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java#L142) a named checker or a custom refactorer. - Specifying `-XepPatchChecks:` without any explicit check will [cause](https://github.com/google/error-prone/blob/master/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java#L404) the empty string to be stored as a named checker. This PR proposes to resolve the above issues by: 1. Requiring `-XepPatchLocation` when `-XepPatchChecks` is specified, but not vice versa. 2. Omitting empty strings from `-XepPatchChecks:`. Fixes #947 COPYBARA_INTEGRATE_REVIEW=https://github.com/google/error-prone/pull/947 from PicnicSupermarket:bugfix/support-empty-PatchChecks c5d9125dc2cd3a6db0217f25c958904e20591bb2 PiperOrigin-RevId: 565064731 --- .../google/errorprone/ErrorProneOptions.java | 28 ++++++++----------- .../errorprone/ErrorProneOptionsTest.java | 21 ++++++++++++-- 2 files changed, 30 insertions(+), 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 index a0c63f0f68b..eb0f58f0cbf 100644 --- a/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java +++ b/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java @@ -140,21 +140,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(); } } @@ -419,6 +405,8 @@ public static ErrorProneOptions processArgs(Iterable 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) { @@ -458,6 +446,7 @@ public static ErrorProneOptions processArgs(Iterable 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); @@ -468,6 +457,7 @@ public static ErrorProneOptions processArgs(Iterable 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 @@ -485,7 +475,8 @@ public static ErrorProneOptions processArgs(Iterable args) { } }); } else { - Iterable checks = Splitter.on(',').trimResults().split(remaining); + Iterable checks = + Splitter.on(',').trimResults().omitEmptyStrings().split(remaining); builder.patchingOptionsBuilder().namedCheckers(ImmutableSet.copyOf(checks)); } } else if (arg.startsWith(PATCH_IMPORT_ORDER_PREFIX)) { @@ -505,6 +496,11 @@ public static ErrorProneOptions processArgs(Iterable 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 index 1bd1ed419c7..6465bd2d57b 100644 --- a/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java +++ b/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java @@ -238,9 +238,6 @@ public void recognizesPatch() { @Test public void throwsExceptionWithBadPatchArgs() { - assertThrows( - InvalidCommandLineOptionException.class, - () -> ErrorProneOptions.processArgs(new String[] {"-XepPatchLocation:IN_PLACE"})); assertThrows( InvalidCommandLineOptionException.class, () -> @@ -257,6 +254,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 =