Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Generalize -XepPatchChecks command line parsing #787

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Generalize -XepPatchChecks command line parsing #787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 26 additions & 18 deletions check_api/src/main/java/com/google/errorprone/BaseErrorProneJavaCompiler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

import static com.google.common.base.StandardSystemProperty.JAVA_SPECIFICATION_VERSION;

import com.google.common.base.Optional;
import com.google.common.base.Supplier;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.errorprone.RefactoringCollection.RefactoringResult;
Expand Down Expand Up @@ -220,26 +222,32 @@ static ErrorProneAnalyzer createAnalyzer(
}
refactoringCollection[0] = RefactoringCollection.refactor(epOptions.patchingOptions(), context);

// Refaster refactorer or using builtin checks
CodeTransformer codeTransformer =
epOptions
.patchingOptions()
.customRefactorer()
.or(
() -> {
ScannerSupplier toUse =
ErrorPronePlugins.loadPlugins(scannerSupplier, context)
.applyOverrides(epOptions);
ImmutableSet<String> namedCheckers = epOptions.patchingOptions().namedCheckers();
if (!namedCheckers.isEmpty()) {
toUse = toUse.filter(bci -> namedCheckers.contains(bci.canonicalName()));
}
return ErrorProneScannerTransformer.create(toUse.get());
})
.get();
// Use Refaster refactorer if configured
Optional<Supplier<CodeTransformer>> customRefactorer =
epOptions.patchingOptions().customRefactorer();
CodeTransformer builtinTransformer =
createBuiltinRefactorer(scannerSupplier, epOptions, context, customRefactorer.isPresent());
CodeTransformer compoundTransformer =
customRefactorer
.transform(s -> CompositeCodeTransformer.compose(s.get(), builtinTransformer))
.or(builtinTransformer);

return ErrorProneAnalyzer.createWithCustomDescriptionListener(
codeTransformer, epOptions, context, refactoringCollection[0]);
compoundTransformer, epOptions, context, refactoringCollection[0]);
}

private static CodeTransformer createBuiltinRefactorer(
ScannerSupplier scannerSupplier,
ErrorProneOptions epOptions,
Context context,
boolean forceFilter) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This forceFilter flag makes sure that the default patch mode of applying all built-in checks is disabled when a Refaster rule is supplied. I added this for backwards compatibility, but it does mean there's (still) no way to apply all built-in checks and any additional Refaster rules. That's another way in which Refaster is "special" (contrast this with service-loaded custom plugins: those are applied by default).

This is another thing I'd like to improve; input welcome!

ScannerSupplier toUse =
ErrorPronePlugins.loadPlugins(scannerSupplier, context).applyOverrides(epOptions);
Set<String> namedCheckers = epOptions.patchingOptions().namedCheckers();
if (forceFilter || !namedCheckers.isEmpty()) {
toUse = toUse.filter(bci -> namedCheckers.contains(bci.canonicalName()));
}
return ErrorProneScannerTransformer.create(toUse.get());
}

static class RefactoringTask implements TaskListener {
Expand Down
54 changes: 35 additions & 19 deletions check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.errorprone.apply.ImportOrganizer;
import java.io.IOException;
import java.io.InputStream;
import java.io.ObjectInputStream;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -434,25 +436,7 @@ public static ErrorProneOptions processArgs(Iterable<String> args) {
}
} else if (arg.startsWith(PATCH_CHECKS_PREFIX)) {
String remaining = arg.substring(PATCH_CHECKS_PREFIX.length());
if (remaining.startsWith("refaster:")) {
// Refaster rule, load from InputStream at file
builder
.patchingOptionsBuilder()
.customRefactorer(
() -> {
String path = remaining.substring("refaster:".length());
try (InputStream in =
Files.newInputStream(FileSystems.getDefault().getPath(path));
ObjectInputStream ois = new ObjectInputStream(in)) {
return (CodeTransformer) ois.readObject();
} catch (IOException | ClassNotFoundException e) {
throw new RuntimeException("Can't load Refaster rule from " + path, e);
}
});
} else {
Iterable<String> checks = Splitter.on(',').trimResults().split(remaining);
builder.patchingOptionsBuilder().namedCheckers(ImmutableSet.copyOf(checks));
}
configurePatchChecks(builder, remaining);
} else if (arg.startsWith(PATCH_IMPORT_ORDER_PREFIX)) {
String remaining = arg.substring(PATCH_IMPORT_ORDER_PREFIX.length());
ImportOrganizer importOrganizer = ImportOrderParser.getImportOrganizer(remaining);
Expand All @@ -473,6 +457,38 @@ public static ErrorProneOptions processArgs(Iterable<String> args) {
return builder.build(remainingArgs.build());
}

private static void configurePatchChecks(Builder builder, String arg) {
List<Supplier<CodeTransformer>> customCodeTransformers = new ArrayList<>();
ImmutableSet.Builder<String> namedCheckers = ImmutableSet.builder();
for (String check : Splitter.on(',').trimResults().split(arg)) {
if (check.startsWith("refaster:")) {
customCodeTransformers.add(
createRefasterCodeTransformerLoader(check.substring("refaster:".length())));
} else {
namedCheckers.add(check);
}
}
builder.patchingOptionsBuilder().namedCheckers(namedCheckers.build());
if (!customCodeTransformers.isEmpty())
builder
.patchingOptionsBuilder()
.customRefactorer(
() ->
CompositeCodeTransformer.compose(
Iterables.transform(customCodeTransformers, Supplier::get)));
}

private static Supplier<CodeTransformer> createRefasterCodeTransformerLoader(String path) {
return () -> {
try (InputStream in = Files.newInputStream(FileSystems.getDefault().getPath(path));
ObjectInputStream ois = new ObjectInputStream(in)) {
return (CodeTransformer) ois.readObject();
} catch (IOException | ClassNotFoundException e) {
throw new RuntimeException("Can't load Refaster rule from " + path, e);
}
};
}

/**
* Given a list of command-line arguments, produce the corresponding {@link ErrorProneOptions}
* instance.
Expand Down