Skip to content

Commit

Permalink
Propagate check flags in patch mode
Browse files Browse the repository at this point in the history
Fixes #4699

COPYBARA_INTEGRATE_REVIEW=#4699 from PicnicSupermarket:sschroevers/unconditionally-apply-overrides 205c5a7
PiperOrigin-RevId: 711776790
  • Loading branch information
Stephan202 authored and Error Prone Team committed Jan 3, 2025
1 parent 6a7ff67 commit 85af056
Show file tree
Hide file tree
Showing 3 changed files with 171 additions and 48 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.common.base.Suppliers;
import com.google.common.collect.ImmutableSet;
import com.google.errorprone.BugPattern.SeverityLevel;
import com.google.errorprone.ErrorProneOptions.Severity;
import com.google.errorprone.RefactoringCollection.RefactoringResult;
import com.google.errorprone.scanner.ErrorProneScannerTransformer;
import com.google.errorprone.scanner.ScannerSupplier;
Expand Down Expand Up @@ -79,15 +80,18 @@ public static ErrorProneAnalyzer createAnalyzer(
.or(
Suppliers.memoize(
() -> {
ScannerSupplier toUse =
ErrorPronePlugins.loadPlugins(scannerSupplier, context);
ImmutableSet<String> namedCheckers =
epOptions.patchingOptions().namedCheckers();
if (!namedCheckers.isEmpty()) {
toUse = toUse.filter(bci -> namedCheckers.contains(bci.canonicalName()));
} else {
toUse = toUse.applyOverrides(epOptions);
}
ScannerSupplier toUse =
ErrorPronePlugins.loadPlugins(scannerSupplier, context)
.applyOverrides(epOptions)
.filter(
bci -> {
String name = bci.canonicalName();
return epOptions.getSeverityMap().get(name) != Severity.OFF
&& (namedCheckers.isEmpty()
|| namedCheckers.contains(name));
});
return ErrorProneScannerTransformer.create(toUse.get());
}));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.errorprone.bugpatterns.BadShiftAmount;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher;
import com.google.errorprone.bugpatterns.ChainingConstructorIgnoresParameter;
import com.google.errorprone.bugpatterns.Finally;
import com.google.errorprone.fixes.SuggestedFix;
Expand All @@ -42,21 +43,27 @@
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.VariableTree;
import com.sun.tools.javac.file.JavacFileManager;
import com.sun.tools.javac.util.Constants;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.OutputStreamWriter;
import java.io.PrintWriter;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import javax.inject.Inject;
import javax.lang.model.SourceVersion;
import javax.tools.DiagnosticListener;
import javax.tools.JavaCompiler;
import javax.tools.JavaFileObject;
import javax.tools.SimpleJavaFileObject;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
Expand Down Expand Up @@ -410,6 +417,150 @@ public void withExcludedPaths() {
assertThat(result.succeeded).isFalse();
}

@BugPattern(summary = "Test bug pattern to test custom patch functionality", severity = ERROR)
public static final class AssignmentUpdater extends BugChecker implements VariableTreeMatcher {
private final String newValue;

@Inject
AssignmentUpdater(ErrorProneFlags flags) {
newValue = flags.get("AssignmentUpdater:NewValue").orElse("flag-not-set");
}

@Override
public Description matchVariable(VariableTree tree, VisitorState state) {
return describeMatch(
tree, SuggestedFix.replace(tree.getInitializer(), Constants.format(newValue)));
}
}

@Test
public void patchAll() throws IOException {
JavaFileObject fileObject =
createOnDiskFileObject(
"StringConstantWrapper.java",
"""
class StringConstantWrapper {
String s = "old-value";
}
""");

CompilationResult result =
doCompile(
Collections.singleton(fileObject),
Arrays.asList("-XepPatchChecks:", "-XepPatchLocation:IN_PLACE"),
ImmutableList.of(AssignmentUpdater.class));
assertThat(result.succeeded).isTrue();
assertThat(Files.readString(Path.of(fileObject.toUri())))
.isEqualTo(
"""
class StringConstantWrapper {
String s = "flag-not-set";
}
""");
}

@Test
public void patchAllWithCheckDisabled() throws IOException {
JavaFileObject fileObject =
createOnDiskFileObject(
"StringConstantWrapper.java",
"""
class StringConstantWrapper {
String s = "old-value";
}
""");

CompilationResult result =
doCompile(
Collections.singleton(fileObject),
Arrays.asList(
"-XepPatchChecks:", "-XepPatchLocation:IN_PLACE", "-Xep:AssignmentUpdater:OFF"),
ImmutableList.of(AssignmentUpdater.class));
assertThat(result.succeeded).isTrue();
assertThat(Files.readString(Path.of(fileObject.toUri())))
.isEqualTo(
"""
class StringConstantWrapper {
String s = "old-value";
}
""");
}

@Test
public void patchSingleWithCheckDisabled() throws IOException {
JavaFileObject fileObject =
createOnDiskFileObject(
"StringConstantWrapper.java",
"""
class StringConstantWrapper {
String s = "old-value";
}
""");

CompilationResult result =
doCompile(
Collections.singleton(fileObject),
Arrays.asList(
"-XepPatchChecks:AssignmentUpdater",
"-XepPatchLocation:IN_PLACE",
"-Xep:AssignmentUpdater:OFF"),
ImmutableList.of(AssignmentUpdater.class));
assertThat(result.succeeded).isTrue();
assertThat(Files.readString(Path.of(fileObject.toUri())))
.isEqualTo(
"""
class StringConstantWrapper {
String s = "old-value";
}
""");
}

@Test
public void patchSingleWithBugPatternCustomization() throws IOException {
JavaFileObject fileObject =
createOnDiskFileObject(
"StringConstantWrapper.java",
"""
class StringConstantWrapper {
String s = "old-value";
}
""");

CompilationResult result =
doCompile(
Collections.singleton(fileObject),
Arrays.asList(
"-XepPatchChecks:AssignmentUpdater",
"-XepPatchLocation:IN_PLACE",
"-XepOpt:AssignmentUpdater:NewValue=new-value"),
ImmutableList.of(AssignmentUpdater.class));
assertThat(result.succeeded).isTrue();
assertThat(Files.readString(Path.of(fileObject.toUri())))
.isEqualTo(
"""
class StringConstantWrapper {
String s = "new-value";
}
""");
}

/**
* Creates a {@link JavaFileObject} with matching on-disk contents.
*
* <p>This method aids in testing patching functionality, as {@code IN_PLACE} patching requires
* that compiled code actually exists on disk.
*/
private JavaFileObject createOnDiskFileObject(String fileName, String source) throws IOException {
Path location = tempDir.getRoot().toPath().resolve(fileName);
Files.writeString(location, source);
return new SimpleJavaFileObject(location.toUri(), SimpleJavaFileObject.Kind.SOURCE) {
@Override
public String getCharContent(boolean ignoreEncodingErrors) {
return source;
}
};
}

private static class CompilationResult {
public final boolean succeeded;
public final String output;
Expand All @@ -427,6 +578,14 @@ private CompilationResult doCompile(
List<String> fileNames,
List<String> extraArgs,
List<Class<? extends BugChecker>> customCheckers) {
return doCompile(
forResources(getClass(), fileNames.toArray(new String[0])), extraArgs, customCheckers);
}

private CompilationResult doCompile(
Iterable<? extends JavaFileObject> files,
List<String> extraArgs,
List<Class<? extends BugChecker>> customCheckers) {
DiagnosticTestHelper diagnosticHelper = new DiagnosticTestHelper();
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
PrintWriter printWriter = new PrintWriter(new OutputStreamWriter(outputStream, UTF_8), true);
Expand All @@ -441,12 +600,7 @@ private CompilationResult doCompile(
: new ErrorProneJavaCompiler(ScannerSupplier.fromBugCheckerClasses(customCheckers));
JavaCompiler.CompilationTask task =
errorProneJavaCompiler.getTask(
printWriter,
fileManager,
diagnosticHelper.collector,
args,
null,
forResources(getClass(), fileNames.toArray(new String[0])));
printWriter, fileManager, diagnosticHelper.collector, args, null, files);

return new CompilationResult(
task.call(), new String(outputStream.toByteArray(), UTF_8), diagnosticHelper);
Expand Down
35 changes: 0 additions & 35 deletions core/src/test/java/com/google/errorprone/scanner/ScannerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -106,41 +106,6 @@ class Test {
.doTest();
}

@Test
public void dontRunPatchForDisabledChecks() {
compilationHelper
.addSourceLines(
"Test.java",
"""
import com.google.errorprone.scanner.ScannerTest.Foo;
class Test {
Foo foo;
}
""")
.setArgs("-XepPatchLocation:IN_PLACE", "-XepPatchChecks:", "-Xep:ShouldNotUseFoo:OFF")
.doTest();
}

@Test
public void dontRunPatchUnlessRequested() {
compilationHelper
.addSourceLines(
"Test.java",
"""
import com.google.errorprone.scanner.ScannerTest.Foo;
class Test {
Foo foo;
}
""")
.setArgs(
"-XepPatchLocation:IN_PLACE",
"-Xep:ShouldNotUseFoo:WARN",
"-XepPatchChecks:DeadException")
.doTest();
}

@OkToUseFoo // Foo can use itself. But this shouldn't suppress errors on *usages* of Foo.
public static final class Foo<T> {}

Expand Down

0 comments on commit 85af056

Please sign in to comment.