Skip to content

Commit

Permalink
Support running all available patch checks
Browse files Browse the repository at this point in the history
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=#947 from PicnicSupermarket:bugfix/support-empty-PatchChecks c5d9125
PiperOrigin-RevId: 565064731
  • Loading branch information
Stephan202 authored and Error Prone Team committed Sep 13, 2023
1 parent aa9fd7e commit 35e51f3
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

Expand Down Expand Up @@ -419,6 +405,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) {
Expand Down Expand Up @@ -458,6 +446,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);
Expand All @@ -468,6 +457,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
Expand All @@ -485,7 +475,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)) {
Expand All @@ -505,6 +496,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());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,9 +238,6 @@ public void recognizesPatch() {

@Test
public void throwsExceptionWithBadPatchArgs() {
assertThrows(
InvalidCommandLineOptionException.class,
() -> ErrorProneOptions.processArgs(new String[] {"-XepPatchLocation:IN_PLACE"}));
assertThrows(
InvalidCommandLineOptionException.class,
() ->
Expand All @@ -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 =
Expand Down

0 comments on commit 35e51f3

Please sign in to comment.