From 85af05658ff6d0e4d413bf7971301d191c30897a Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 3 Jan 2025 09:37:16 -0800 Subject: [PATCH] Propagate check flags in patch mode Fixes #4699 COPYBARA_INTEGRATE_REVIEW=https://github.com/google/error-prone/pull/4699 from PicnicSupermarket:sschroevers/unconditionally-apply-overrides 205c5a701b2c26834041399e9695cfbdcf56a820 PiperOrigin-RevId: 711776790 --- .../google/errorprone/ErrorProneAnalyzer.java | 18 +- .../ErrorProneJavaCompilerTest.java | 166 +++++++++++++++++- .../errorprone/scanner/ScannerTest.java | 35 ---- 3 files changed, 171 insertions(+), 48 deletions(-) diff --git a/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java b/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java index 1c04f3d1191..5a643d7fd8b 100644 --- a/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java +++ b/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java @@ -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; @@ -79,15 +80,18 @@ public static ErrorProneAnalyzer createAnalyzer( .or( Suppliers.memoize( () -> { - ScannerSupplier toUse = - ErrorPronePlugins.loadPlugins(scannerSupplier, context); ImmutableSet 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()); })); diff --git a/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java b/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java index fb73c80a010..72cc7fa348d 100644 --- a/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java +++ b/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java @@ -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; @@ -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; @@ -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. + * + *

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; @@ -427,6 +578,14 @@ private CompilationResult doCompile( List fileNames, List extraArgs, List> customCheckers) { + return doCompile( + forResources(getClass(), fileNames.toArray(new String[0])), extraArgs, customCheckers); + } + + private CompilationResult doCompile( + Iterable files, + List extraArgs, + List> customCheckers) { DiagnosticTestHelper diagnosticHelper = new DiagnosticTestHelper(); ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); PrintWriter printWriter = new PrintWriter(new OutputStreamWriter(outputStream, UTF_8), true); @@ -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); diff --git a/core/src/test/java/com/google/errorprone/scanner/ScannerTest.java b/core/src/test/java/com/google/errorprone/scanner/ScannerTest.java index 4ee69a50baf..73c494d21af 100644 --- a/core/src/test/java/com/google/errorprone/scanner/ScannerTest.java +++ b/core/src/test/java/com/google/errorprone/scanner/ScannerTest.java @@ -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 {}