From 5818c2c65d0e8c5924eef70abb9b98f33fffe65d Mon Sep 17 00:00:00 2001 From: Gautam Korlam Date: Sun, 9 Apr 2017 20:38:59 -0700 Subject: [PATCH 1/6] Add an option to exclude paths from being analyzed --- .../google/errorprone/ErrorProneAnalyzer.java | 23 +++++++++-- .../google/errorprone/ErrorProneOptions.java | 39 ++++++++++++++++++- 2 files changed, 57 insertions(+), 5 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 8835cc0f869..57384720510 100644 --- a/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java +++ b/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java @@ -38,8 +38,11 @@ import com.sun.tools.javac.util.Context; import com.sun.tools.javac.util.Log; import com.sun.tools.javac.util.PropagatedException; + +import java.net.URI; import java.util.HashSet; import java.util.Set; +import java.util.regex.Pattern; /** A {@link TaskListener} that runs Error Prone over attributed compilation units. */ @Trusted @@ -138,11 +141,15 @@ public void finished(TaskEvent taskEvent) { // We only get TaskEvents for compilation units if they contain no package declarations // (e.g. package-info.java files). In this case it's safe to analyze the // CompilationUnitTree immediately. - transformer.get().apply(path, subContext, descriptionListener); + if (!isExcludedPath(compilation.getSourceFile().toUri())) { + transformer.get().apply(path, subContext, descriptionListener); + } } else if (finishedCompilation(path.getCompilationUnit())) { // Otherwise this TaskEvent is for a ClassTree, and we can scan the whole // CompilationUnitTree once we've seen all the enclosed classes. - transformer.get().apply(new TreePath(compilation), subContext, descriptionListener); + if (!isExcludedPath(compilation.getSourceFile().toUri())) { + transformer.get().apply(new TreePath(compilation), subContext, descriptionListener); + } } } catch (ErrorProneError e) { e.logFatalError(log); @@ -160,7 +167,17 @@ public void finished(TaskEvent taskEvent) { } } - /** Returns true if all declarations inside the given compilation unit have been visited. */ + /** + * Returns true if all declarations inside the given compilation unit have been visited. + */ + private boolean isExcludedPath(URI uri) { + Pattern excludedPattern = errorProneOptions.getExcludedPattern(); + return (excludedPattern != null) && excludedPattern.matcher(uri.toString()).matches(); + } + + /** + * Returns true if all declarations inside the given compilation unit have been visited. + */ private boolean finishedCompilation(CompilationUnitTree tree) { OUTER: for (Tree decl : tree.getTypeDecls()) { 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 b21111025f3..49bb47ad726 100644 --- a/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java +++ b/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java @@ -17,6 +17,7 @@ package com.google.errorprone; import com.google.auto.value.AutoValue; +import com.google.common.base.Joiner; import com.google.common.base.Optional; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; @@ -25,6 +26,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.errorprone.apply.ImportOrganizer; + import java.io.IOException; import java.io.InputStream; import java.io.ObjectInputStream; @@ -33,8 +35,10 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.Map; import java.util.Set; +import java.util.regex.Pattern; /** * Processes command-line options specific to error-prone. @@ -56,6 +60,7 @@ public class ErrorProneOptions { private static final String DISABLE_WARNINGS_IN_GENERATED_CODE_FLAG = "-XepDisableWarningsInGeneratedCode"; private static final String COMPILING_TEST_ONLY_CODE = "-XepCompilingTestOnlyCode"; + private static final String EXCLUDED_PATHS_PREFIX = "-XepExcludedPaths:"; /** see {@link javax.tools.OptionChecker#isSupportedOption(String)} */ public static int isSupportedOption(String option) { @@ -64,6 +69,7 @@ public static int isSupportedOption(String option) { || option.startsWith(ErrorProneFlags.PREFIX) || option.startsWith(PATCH_OUTPUT_LOCATION) || option.startsWith(PATCH_CHECKS_PREFIX) + || option.startsWith(EXCLUDED_PATHS_PREFIX) || option.equals(IGNORE_UNKNOWN_CHECKS_FLAG) || option.equals(DISABLE_WARNINGS_IN_GENERATED_CODE_FLAG) || option.equals(ERRORS_AS_WARNINGS_FLAG) @@ -156,6 +162,7 @@ final PatchingOptions build() { private final boolean isTestOnlyTarget; private final ErrorProneFlags flags; private final PatchingOptions patchingOptions; + private final Pattern excludedPattern; private ErrorProneOptions( ImmutableMap severityMap, @@ -167,7 +174,8 @@ private ErrorProneOptions( boolean disableAllChecks, boolean isTestOnlyTarget, ErrorProneFlags flags, - PatchingOptions patchingOptions) { + PatchingOptions patchingOptions, + Pattern excludedPattern) { this.severityMap = severityMap; this.remainingArgs = remainingArgs; this.ignoreUnknownChecks = ignoreUnknownChecks; @@ -178,6 +186,7 @@ private ErrorProneOptions( this.isTestOnlyTarget = isTestOnlyTarget; this.flags = flags; this.patchingOptions = patchingOptions; + this.excludedPattern = excludedPattern; } public String[] getRemainingArgs() { @@ -212,6 +221,10 @@ public PatchingOptions patchingOptions() { return patchingOptions; } + public Pattern getExcludedPattern() { + return excludedPattern; + } + private static class Builder { private boolean ignoreUnknownChecks = false; private boolean disableWarningsInGeneratedCode = false; @@ -222,6 +235,7 @@ private static class Builder { private Map severityMap = new HashMap<>(); private final ErrorProneFlags.Builder flagsBuilder = ErrorProneFlags.builder(); private final PatchingOptions.Builder patchingOptionsBuilder = PatchingOptions.builder(); + private Pattern excludedPattern; private void parseSeverity(String arg) { // Strip prefix @@ -301,7 +315,12 @@ public ErrorProneOptions build(ImmutableList remainingArgs) { disableAllChecks, isTestOnlyTarget, flagsBuilder.build(), - patchingOptionsBuilder.build()); + patchingOptionsBuilder.build(), + excludedPattern); + } + + public void setExcludedPattern(Pattern excludedPattern) { + this.excludedPattern = excludedPattern; } } @@ -391,6 +410,10 @@ public static ErrorProneOptions processArgs(Iterable args) { String remaining = arg.substring(PATCH_IMPORT_ORDER_PREFIX.length()); ImportOrganizer importOrganizer = ImportOrderParser.getImportOrganizer(remaining); builder.patchingOptionsBuilder().importOrganizer(importOrganizer); + } else if (arg.startsWith(EXCLUDED_PATHS_PREFIX)) { + String remaining = arg.substring(EXCLUDED_PATHS_PREFIX.length()); + Iterable paths = Splitter.on(',').trimResults().split(remaining); + builder.setExcludedPattern(getExcludedPattern(paths)); } else { remainingArgs.add(arg); } @@ -412,4 +435,16 @@ public static ErrorProneOptions processArgs(String[] args) { Preconditions.checkNotNull(args); return processArgs(Arrays.asList(args)); } + + private static Pattern getExcludedPattern(Iterable excludeSubStrings) { + if (!excludeSubStrings.iterator().hasNext()) { + return null; + } + Set escapedExcluded = new LinkedHashSet<>(); + for (String str: excludeSubStrings) { + escapedExcluded.add(str.replace(".", "\\.")); + } + String choiceRegexp = Joiner.on("|").join(escapedExcluded); + return Pattern.compile("(?:.*)(?:" + choiceRegexp + ")(?:.*)"); + } } From 51c422be49077dd95740270991925ea6f65feb94 Mon Sep 17 00:00:00 2001 From: Gautam Korlam Date: Wed, 1 Nov 2017 16:29:50 -0700 Subject: [PATCH 2/6] Use ASTHelpers.getFileNameFromUri --- .../main/java/com/google/errorprone/ErrorProneAnalyzer.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 57384720510..046d2c49978 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.Throwables; import com.google.errorprone.scanner.ErrorProneScannerTransformer; import com.google.errorprone.scanner.ScannerSupplier; +import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.Tree; import com.sun.source.util.TaskEvent; @@ -172,7 +173,7 @@ public void finished(TaskEvent taskEvent) { */ private boolean isExcludedPath(URI uri) { Pattern excludedPattern = errorProneOptions.getExcludedPattern(); - return (excludedPattern != null) && excludedPattern.matcher(uri.toString()).matches(); + return (excludedPattern != null) && excludedPattern.matcher(ASTHelpers.getFileNameFromUri(uri)).matches(); } /** From e4a5dff0cbbe215e277053fa2faa2d5bbf01449b Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 2 Nov 2017 19:58:24 -0700 Subject: [PATCH 3/6] Don't substitute for '.' in paths This was leftover code that shouldn't have made it here --- .../main/java/com/google/errorprone/ErrorProneOptions.java | 6 +----- 1 file changed, 1 insertion(+), 5 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 49bb47ad726..c11e49b82da 100644 --- a/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java +++ b/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java @@ -440,11 +440,7 @@ private static Pattern getExcludedPattern(Iterable excludeSubStrings) { if (!excludeSubStrings.iterator().hasNext()) { return null; } - Set escapedExcluded = new LinkedHashSet<>(); - for (String str: excludeSubStrings) { - escapedExcluded.add(str.replace(".", "\\.")); - } - String choiceRegexp = Joiner.on("|").join(escapedExcluded); + String choiceRegexp = Joiner.on("|").join(excludeSubStrings); return Pattern.compile("(?:.*)(?:" + choiceRegexp + ")(?:.*)"); } } From 4019abfc260cfb9d2de69090917798755b8ce792 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 2 Nov 2017 19:59:26 -0700 Subject: [PATCH 4/6] add test --- .../google/errorprone/ErrorProneOptionsTest.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) 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 ef3630716f2..52460e9ded8 100644 --- a/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java +++ b/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java @@ -26,6 +26,8 @@ import java.util.Arrays; import java.util.List; import java.util.Map; +import java.util.regex.Pattern; + import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -163,6 +165,20 @@ public void recognizesCompilingTestOnlyCode() { assertThat(options.isTestOnlyTarget()).isTrue(); } + @Test + public void recognizesExcludedPaths() { + ErrorProneOptions options = + ErrorProneOptions.processArgs( + new String[] {"-XepExcludedPaths:build/generated,other_output"}); + Pattern excludedPattern = options.getExcludedPattern(); + assertThat(excludedPattern).isNotNull(); + assertThat(excludedPattern.matcher("fizz/build/generated/Gen.java").matches()).isTrue(); + assertThat(excludedPattern.matcher("fizz/bazz/generated/Gen.java").matches()).isFalse(); + assertThat(excludedPattern.matcher("other_output/Gen.java").matches()).isTrue(); + assertThat(excludedPattern.matcher("foo/other_output/subdir/Gen.java").matches()).isTrue(); + } + + @Test public void recognizesPatch() { ErrorProneOptions options = From 28d7edeec9b705a72d30c22be6ca7b508ea337cd Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 2 Nov 2017 20:54:25 -0700 Subject: [PATCH 5/6] address comments --- .../google/errorprone/ErrorProneAnalyzer.java | 18 ++++++------- .../google/errorprone/ErrorProneOptions.java | 13 ++-------- .../errorprone/ErrorProneOptionsTest.java | 4 ++- .../ErrorProneJavaCompilerTest.java | 26 +++++++++++++++++++ 4 files changed, 39 insertions(+), 22 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 046d2c49978..d433ed2b4ab 100644 --- a/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java +++ b/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java @@ -138,17 +138,15 @@ public void finished(TaskEvent taskEvent) { DescriptionListener descriptionListener = descriptionListenerFactory.getDescriptionListener(log, compilation); try { - if (path.getLeaf().getKind() == Tree.Kind.COMPILATION_UNIT) { - // We only get TaskEvents for compilation units if they contain no package declarations - // (e.g. package-info.java files). In this case it's safe to analyze the - // CompilationUnitTree immediately. - if (!isExcludedPath(compilation.getSourceFile().toUri())) { + if (!isExcludedPath(compilation.getSourceFile().toUri())) { + if (path.getLeaf().getKind() == Tree.Kind.COMPILATION_UNIT) { + // We only get TaskEvents for compilation units if they contain no package declarations + // (e.g. package-info.java files). In this case it's safe to analyze the + // CompilationUnitTree immediately. transformer.get().apply(path, subContext, descriptionListener); - } - } else if (finishedCompilation(path.getCompilationUnit())) { - // Otherwise this TaskEvent is for a ClassTree, and we can scan the whole - // CompilationUnitTree once we've seen all the enclosed classes. - if (!isExcludedPath(compilation.getSourceFile().toUri())) { + } else if (finishedCompilation(path.getCompilationUnit())) { + // Otherwise this TaskEvent is for a ClassTree, and we can scan the whole + // CompilationUnitTree once we've seen all the enclosed classes. transformer.get().apply(new TreePath(compilation), subContext, descriptionListener); } } 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 c11e49b82da..45d7978fe19 100644 --- a/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java +++ b/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java @@ -411,9 +411,8 @@ public static ErrorProneOptions processArgs(Iterable args) { ImportOrganizer importOrganizer = ImportOrderParser.getImportOrganizer(remaining); builder.patchingOptionsBuilder().importOrganizer(importOrganizer); } else if (arg.startsWith(EXCLUDED_PATHS_PREFIX)) { - String remaining = arg.substring(EXCLUDED_PATHS_PREFIX.length()); - Iterable paths = Splitter.on(',').trimResults().split(remaining); - builder.setExcludedPattern(getExcludedPattern(paths)); + String pathRegex = arg.substring(EXCLUDED_PATHS_PREFIX.length()); + builder.setExcludedPattern(Pattern.compile(pathRegex)); } else { remainingArgs.add(arg); } @@ -435,12 +434,4 @@ public static ErrorProneOptions processArgs(String[] args) { Preconditions.checkNotNull(args); return processArgs(Arrays.asList(args)); } - - private static Pattern getExcludedPattern(Iterable excludeSubStrings) { - if (!excludeSubStrings.iterator().hasNext()) { - return null; - } - String choiceRegexp = Joiner.on("|").join(excludeSubStrings); - return Pattern.compile("(?:.*)(?:" + choiceRegexp + ")(?:.*)"); - } } 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 52460e9ded8..077f32c324c 100644 --- a/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java +++ b/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java @@ -169,13 +169,15 @@ public void recognizesCompilingTestOnlyCode() { public void recognizesExcludedPaths() { ErrorProneOptions options = ErrorProneOptions.processArgs( - new String[] {"-XepExcludedPaths:build/generated,other_output"}); + new String[] {"-XepExcludedPaths:(.*/)?(build/generated|other_output)/.*\\.java"}); Pattern excludedPattern = options.getExcludedPattern(); assertThat(excludedPattern).isNotNull(); assertThat(excludedPattern.matcher("fizz/build/generated/Gen.java").matches()).isTrue(); assertThat(excludedPattern.matcher("fizz/bazz/generated/Gen.java").matches()).isFalse(); + assertThat(excludedPattern.matcher("fizz/abuild/generated/Gen.java").matches()).isFalse(); assertThat(excludedPattern.matcher("other_output/Gen.java").matches()).isTrue(); assertThat(excludedPattern.matcher("foo/other_output/subdir/Gen.java").matches()).isTrue(); + assertThat(excludedPattern.matcher("foo/other_output/subdir/Gen.cpp").matches()).isFalse(); } diff --git a/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java b/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java index 80466b3dcf0..921b5846b63 100644 --- a/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java +++ b/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java @@ -398,6 +398,32 @@ public void testFixGeneratedConstructor() throws Exception { .contains("AssertionError: Cannot edit synthetic AST nodes"); } + @Test + public void testWithExcludedPaths() throws Exception { + CompilationResult result = + doCompile( + Arrays.asList("bugpatterns/testdata/SelfAssignmentPositiveCases1.java"), + Collections.emptyList(), + Collections.>emptyList()); + assertThat(result.succeeded).isFalse(); + + result = + doCompile( + Arrays.asList("bugpatterns/testdata/SelfAssignmentPositiveCases1.java"), + Arrays.asList("-XepExcludedPaths:.*/bugpatterns/.*"), + Collections.>emptyList()); + assertThat(result.succeeded).isTrue(); + + // ensure regexp must match the full path + result = + doCompile( + Arrays.asList("bugpatterns/testdata/SelfAssignmentPositiveCases1.java"), + Arrays.asList("-XepExcludedPaths:bugpatterns"), + Collections.>emptyList()); + assertThat(result.succeeded).isFalse(); + } + + private static class CompilationResult { public final boolean succeeded; public final DiagnosticTestHelper diagnosticHelper; From 3d5cc3ccd4a8cd3e6512a710594775e029afdaa4 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 2 Nov 2017 20:57:10 -0700 Subject: [PATCH 6/6] remove extraneous blank lines in import blocks --- .../src/main/java/com/google/errorprone/ErrorProneAnalyzer.java | 1 - .../src/main/java/com/google/errorprone/ErrorProneOptions.java | 1 - .../test/java/com/google/errorprone/ErrorProneOptionsTest.java | 1 - 3 files changed, 3 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 d433ed2b4ab..c62c8032cc9 100644 --- a/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java +++ b/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java @@ -39,7 +39,6 @@ import com.sun.tools.javac.util.Context; import com.sun.tools.javac.util.Log; import com.sun.tools.javac.util.PropagatedException; - import java.net.URI; import java.util.HashSet; import java.util.Set; 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 45d7978fe19..8a8baa44f1a 100644 --- a/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java +++ b/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java @@ -26,7 +26,6 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.errorprone.apply.ImportOrganizer; - import java.io.IOException; import java.io.InputStream; import java.io.ObjectInputStream; 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 077f32c324c..5eed074273f 100644 --- a/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java +++ b/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java @@ -27,7 +27,6 @@ import java.util.List; import java.util.Map; import java.util.regex.Pattern; - import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4;