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

Add an option to exclude paths from being analyzed #599

Closed
wants to merge 6 commits into from
Closed

Add an option to exclude paths from being analyzed #599

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5818c2c
Add an option to exclude paths from being analyzed
Apr 10, 2017
51c422b
Use ASTHelpers.getFileNameFromUri
Nov 1, 2017
e4a5dff
Don't substitute for '.' in paths
msridhar Nov 3, 2017
4019abf
add test
msridhar Nov 3, 2017
28d7ede
address comments
msridhar Nov 3, 2017
3d5cc3c
remove extraneous blank lines in import blocks
msridhar Nov 3, 2017
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
24 changes: 21 additions & 3 deletions check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -38,8 +39,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
Expand Down Expand Up @@ -138,11 +142,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())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to de-duplicate this logic.

transformer.get().apply(new TreePath(compilation), subContext, descriptionListener);
}
}
} catch (ErrorProneError e) {
e.logFatalError(log);
Expand All @@ -160,7 +168,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(ASTHelpers.getFileNameFromUri(uri)).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()) {
Expand Down
35 changes: 33 additions & 2 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 @@ -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;
Expand All @@ -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;
Expand All @@ -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.
Expand All @@ -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) {
Expand All @@ -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)
Expand Down Expand Up @@ -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<String, Severity> severityMap,
Expand All @@ -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;
Expand All @@ -178,6 +186,7 @@ private ErrorProneOptions(
this.isTestOnlyTarget = isTestOnlyTarget;
this.flags = flags;
this.patchingOptions = patchingOptions;
this.excludedPattern = excludedPattern;
}

public String[] getRemainingArgs() {
Expand Down Expand Up @@ -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;
Expand All @@ -222,6 +235,7 @@ private static class Builder {
private Map<String, Severity> 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
Expand Down Expand Up @@ -301,7 +315,12 @@ public ErrorProneOptions build(ImmutableList<String> remainingArgs) {
disableAllChecks,
isTestOnlyTarget,
flagsBuilder.build(),
patchingOptionsBuilder.build());
patchingOptionsBuilder.build(),
excludedPattern);
}

public void setExcludedPattern(Pattern excludedPattern) {
this.excludedPattern = excludedPattern;
}
}

Expand Down Expand Up @@ -391,6 +410,10 @@ public static ErrorProneOptions processArgs(Iterable<String> 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<String> paths = Splitter.on(',').trimResults().split(remaining);
builder.setExcludedPattern(getExcludedPattern(paths));
} else {
remainingArgs.add(arg);
}
Expand All @@ -412,4 +435,12 @@ public static ErrorProneOptions processArgs(String[] args) {
Preconditions.checkNotNull(args);
return processArgs(Arrays.asList(args));
}

private static Pattern getExcludedPattern(Iterable<String> excludeSubStrings) {
Copy link
Collaborator

@cushon cushon Nov 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

WDYT about making the flag take a regex instead of a comma-separated list that gets processed into a regex? Making it an arbitrary regex avoids the need to define what the syntax is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense; will change

if (!excludeSubStrings.iterator().hasNext()) {
return null;
}
String choiceRegexp = Joiner.on("|").join(excludeSubStrings);
return Pattern.compile("(?:.*)(?:" + choiceRegexp + ")(?:.*)");
}
}
16 changes: 16 additions & 0 deletions check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -163,6 +165,20 @@ public void recognizesCompilingTestOnlyCode() {
assertThat(options.isTestOnlyTarget()).isTrue();
}

@Test
public void recognizesExcludedPaths() {
Copy link
Collaborator

@cushon cushon Nov 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Can you add an integration test that verifies the option is used to filter the compilation units that get analyzed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will look into it; is there an existing test whose structure I can copy?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are test cases in ErrorProneJavaCompilerTest that use flags to enable/disable checks and change severities. Maybe something like that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, thanks! I added a test case there.

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 =
Expand Down