diff --git a/annotation/src/main/java/com/google/errorprone/BugPattern.java b/annotation/src/main/java/com/google/errorprone/BugPattern.java index aa5a23a028f..09259e8ae62 100644 --- a/annotation/src/main/java/com/google/errorprone/BugPattern.java +++ b/annotation/src/main/java/com/google/errorprone/BugPattern.java @@ -29,6 +29,65 @@ */ @Retention(RUNTIME) public @interface BugPattern { + + /** A collection of standardized tags that can be applied to BugPatterns. */ + final class StandardTags { + private StandardTags() {} + + /** + * This check, for reasons of backwards compatibility or difficulty in cleaning up, should be + * considered very likely to represent a real error in the vast majority ({@code >99.9%}) of + * cases, but couldn't otherwise be turned on as an ERROR. + * + *

Systems trying to determine the set of likely errors from a collection of BugPatterns + * should act as if any BugPattern with {@link #severity()} of {@link SeverityLevel#ERROR} also + * has this tag. + */ + public static final String LIKELY_ERROR = "LikelyError"; + + /** + * This check detects a coding pattern that is valid within the Java language and doesn't + * represent a runtime defect, but is otherwise discouraged for reasons of consistency within a + * project or ease of understanding by other programmers. + * + *

Checks using this tag should limit their replacements to those that don't change the + * behavior of the code (for example: adding clarifying parentheses, reordering modifiers in a + * single declaration, removing implicit modifiers like {@code public} for members in an {@code + * interface}). + */ + public static final String STYLE = "Style"; + + /** + * This check detects a potential performance issue, where an easily-identifiable replacement + * for the code being made will always result in a net positive performance improvement. + */ + public static final String PERFORMANCE = "Performance"; + + /** + * This check detects code that may technically be working within a limited domain, but is + * fragile, or violates generally-accepted assumptions of behavior. + * + *

Examples: DefaultCharset, where code implicitly uses the JVM default charset, will work in + * circumstances where data being fed to the system happens to be compatible with the Charset, + * but breaks down if fed data outside. + */ + public static final String FRAGILE_CODE = "FragileCode"; + + /** + * This check points out potential issues when operating in a concurrent context + * + *

The code may work fine when accessed by 1 thread at a time, but may have some unintended + * behavior when running in multiple threads. + */ + public static final String CONCURRENCY = "Concurrency"; + + /** + * This check points out a coding pattern that, while functional, has an easier-to-read or + * faster alternative. + */ + public static final String SIMPLIFICATION = "Simplification"; + } + /** * A unique identifier for this bug, used for @SuppressWarnings and in the compiler error message. */ @@ -52,8 +111,27 @@ public enum LinkType { NONE } - /** The class of bug this bug checker detects. */ - Category category(); + /** + * A list of Stringly-typed tags to apply to this check. These tags can be consumed by tools + * aggregating Error Prone checks (for example: a git pre-commit hook could clean up Java source + * by finding any checks tagged "Style", run an Error Prone compile over the code with those + * checks enabled, collect the fixes suggested and apply them). + * + *

To allow for sharing of tags across systems, a number of standard tags are available as + * static constants in {@link StandardTags}. It is strongly encouraged to extract any custom tags + * used in annotation property to constants that are shared by your codebase. + */ + String[] tags() default {}; + + /** + * The class of bug this bug checker detects. + * + * @deprecated This category field hasn't provided much value, as the 'problem domain' of each + * BugChecker is evident from the checker itself. We've introduced {@link #tags} as a means to + * apply general tags to checks. + */ + @Deprecated + Category category() default Category.ONE_OFF; public enum Category { /** General Java or JDK errors. */ diff --git a/annotation/src/main/java/com/google/errorprone/BugPatternValidator.java b/annotation/src/main/java/com/google/errorprone/BugPatternValidator.java index 1be384b64ae..eb071884353 100644 --- a/annotation/src/main/java/com/google/errorprone/BugPatternValidator.java +++ b/annotation/src/main/java/com/google/errorprone/BugPatternValidator.java @@ -16,7 +16,6 @@ package com.google.errorprone; -import com.google.common.base.Function; import com.google.common.base.Joiner; import com.google.common.collect.Collections2; import java.lang.annotation.Annotation; @@ -31,14 +30,6 @@ */ public class BugPatternValidator { - private static final Function, String> GET_CANONICAL_NAME = - new Function, String>() { - @Override - public String apply(Class input) { - return input.getCanonicalName(); - } - }; - private static final Joiner COMMA_JOINER = Joiner.on(", "); public static void validate(BugPattern pattern) throws ValidationException { @@ -81,7 +72,8 @@ public static void validate(BugPattern pattern) throws ValidationException { String.format( "Expected no custom suppression annotations but found these: %s", COMMA_JOINER.join( - Collections2.transform(customSuppressionAnnotations, GET_CANONICAL_NAME)))); + Collections2.transform( + customSuppressionAnnotations, Class::getCanonicalName)))); } break; } diff --git a/annotations/src/main/java/com/google/errorprone/annotations/RequiresNamedParameters.java b/annotations/src/main/java/com/google/errorprone/annotations/RequiresNamedParameters.java deleted file mode 100644 index 0b86e8e502e..00000000000 --- a/annotations/src/main/java/com/google/errorprone/annotations/RequiresNamedParameters.java +++ /dev/null @@ -1,38 +0,0 @@ -/* - * Copyright 2017 Google Inc. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.errorprone.annotations; - -import static java.lang.annotation.ElementType.METHOD; -import static java.lang.annotation.RetentionPolicy.CLASS; - -import java.lang.annotation.Documented; -import java.lang.annotation.Retention; -import java.lang.annotation.Target; - -/** - * Requires invocations of the method to record the corresponding formal parameter name for each - * argument in a comment, e.g. {@code foo(/*x=*/ 1, /*y=/ 2)}. - * - *

Labelling comments must be before the argument and must contain the parameter name followed by - * an equals character. All arguments must be labelled. - * - * @author andrewrice@google.com (Andrew Rice) - */ -@Documented -@Retention(CLASS) -@Target(METHOD) -public @interface RequiresNamedParameters {} diff --git a/check_api/src/main/java/com/google/errorprone/BugCheckerInfo.java b/check_api/src/main/java/com/google/errorprone/BugCheckerInfo.java index c503f68ebd6..103fb820c36 100644 --- a/check_api/src/main/java/com/google/errorprone/BugCheckerInfo.java +++ b/check_api/src/main/java/com/google/errorprone/BugCheckerInfo.java @@ -57,6 +57,9 @@ public class BugCheckerInfo implements Serializable { */ private final ImmutableSet allNames; + /** Set of String tags associated with the checker. */ + private final ImmutableSet tags; + /** * The error message to print in compiler diagnostics when this check triggers. Corresponds to the * {@code summary} attribute from its {@code BugPattern}. @@ -105,19 +108,18 @@ public static BugCheckerInfo create(Class checker) { } private BugCheckerInfo(Class checker, BugPattern pattern) { - this.checker = checker; - canonicalName = pattern.name(); - allNames = ImmutableSet.builder().add(canonicalName).add(pattern.altNames()).build(); - message = pattern.summary(); - defaultSeverity = pattern.severity(); - linkUrl = createLinkUrl(pattern); - suppressibility = pattern.suppressibility(); - if (suppressibility == Suppressibility.CUSTOM_ANNOTATION) { - customSuppressionAnnotations = - new HashSet<>(Arrays.asList(pattern.customSuppressionAnnotations())); - } else { - customSuppressionAnnotations = Collections.>emptySet(); - } + this( + checker, + pattern.name(), + ImmutableSet.builder().add(pattern.name()).add(pattern.altNames()).build(), + pattern.summary(), + pattern.severity(), + createLinkUrl(pattern), + pattern.suppressibility(), + pattern.suppressibility() == Suppressibility.CUSTOM_ANNOTATION + ? new HashSet<>(Arrays.asList(pattern.customSuppressionAnnotations())) + : Collections.emptySet(), + ImmutableSet.copyOf(pattern.tags())); } private BugCheckerInfo( @@ -128,7 +130,8 @@ private BugCheckerInfo( SeverityLevel defaultSeverity, String linkUrl, Suppressibility suppressibility, - Set> customSuppressionAnnotations) { + Set> customSuppressionAnnotations, + ImmutableSet tags) { this.checker = checker; this.canonicalName = canonicalName; this.allNames = allNames; @@ -137,6 +140,7 @@ private BugCheckerInfo( this.linkUrl = linkUrl; this.suppressibility = suppressibility; this.customSuppressionAnnotations = customSuppressionAnnotations; + this.tags = tags; } /** @@ -156,7 +160,8 @@ public BugCheckerInfo withCustomDefaultSeverity(SeverityLevel defaultSeverity) { defaultSeverity, linkUrl, suppressibility, - customSuppressionAnnotations); + customSuppressionAnnotations, + tags); } private static final String URL_FORMAT = "http://errorprone.info/bugpattern/%s"; @@ -229,6 +234,10 @@ public Set> customSuppressionAnnotations() { return customSuppressionAnnotations; } + public ImmutableSet getTags() { + return tags; + } + public Class checkerClass() { return checker; } diff --git a/check_api/src/main/java/com/google/errorprone/RefactoringCollection.java b/check_api/src/main/java/com/google/errorprone/RefactoringCollection.java index e009faac459..7b3b5440d2a 100644 --- a/check_api/src/main/java/com/google/errorprone/RefactoringCollection.java +++ b/check_api/src/main/java/com/google/errorprone/RefactoringCollection.java @@ -82,7 +82,7 @@ static RefactoringCollection refactor(PatchingOptions patchingOptions) { Function postProcess; if (patchingOptions.inPlace()) { - fileDestination = new FsFileDestination(); + fileDestination = new FsFileDestination(rootPath); postProcess = uri -> RefactoringResult.create( @@ -160,7 +160,7 @@ RefactoringResult applyChanges(URI uri) throws Exception { return RefactoringResult.create("", RefactoringResultType.NO_CHANGES); } - doApplyProcess(fileDestination, new FsFileSource(), listeners); + doApplyProcess(fileDestination, new FsFileSource(rootPath), listeners); return postProcess.apply(uri); } @@ -186,16 +186,19 @@ private void doApplyProcess( Collection listeners) { for (DelegatingDescriptionListener listener : listeners) { try { - SourceFile file = fileSource.readFile(listener.base.getPath()); + SourceFile file = fileSource.readFile(listener.base.getRelevantFileName()); listener.base.applyDifferences(file); fileDestination.writeFile(file); } catch (IOException e) { - logger.log(Level.WARNING, "Failed to apply diff to file " + listener.base.getPath(), e); + logger.log( + Level.WARNING, + "Failed to apply diff to file " + listener.base.getRelevantFileName(), + e); } } } - private static final class DelegatingDescriptionListener implements DescriptionListener { + private final class DelegatingDescriptionListener implements DescriptionListener { final DescriptionBasedDiff base; final DescriptionListener listener; diff --git a/check_api/src/main/java/com/google/errorprone/apply/DescriptionBasedDiff.java b/check_api/src/main/java/com/google/errorprone/apply/DescriptionBasedDiff.java index 6c6d3370a1d..d5d7b507c07 100644 --- a/check_api/src/main/java/com/google/errorprone/apply/DescriptionBasedDiff.java +++ b/check_api/src/main/java/com/google/errorprone/apply/DescriptionBasedDiff.java @@ -25,8 +25,6 @@ import com.google.errorprone.matchers.Description; import com.sun.tools.javac.tree.EndPosTable; import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; -import java.nio.file.Path; -import java.nio.file.Paths; import java.util.LinkedHashSet; import java.util.Set; @@ -40,6 +38,7 @@ */ public final class DescriptionBasedDiff implements DescriptionListener, Diff { + private final String sourcePath; private final boolean ignoreOverlappingFixes; private final JCCompilationUnit compilationUnit; private final Set importsToAdd; @@ -63,6 +62,7 @@ private DescriptionBasedDiff( boolean ignoreOverlappingFixes, ImportOrganizer importOrganizer) { this.compilationUnit = checkNotNull(compilationUnit); + this.sourcePath = compilationUnit.getSourceFile().toUri().getPath(); this.ignoreOverlappingFixes = ignoreOverlappingFixes; this.importsToAdd = new LinkedHashSet<>(); this.importsToRemove = new LinkedHashSet<>(); @@ -71,8 +71,8 @@ private DescriptionBasedDiff( } @Override - public Path getPath() { - return Paths.get(compilationUnit.getSourceFile().toUri()); + public String getRelevantFileName() { + return sourcePath; } public boolean isEmpty() { diff --git a/check_api/src/main/java/com/google/errorprone/apply/Diff.java b/check_api/src/main/java/com/google/errorprone/apply/Diff.java index bc79891f4a7..abadc23e382 100644 --- a/check_api/src/main/java/com/google/errorprone/apply/Diff.java +++ b/check_api/src/main/java/com/google/errorprone/apply/Diff.java @@ -16,16 +16,14 @@ package com.google.errorprone.apply; -import java.nio.file.Path; - /** * All the differences to be applied to a source file to be applied in a refactoring. * * @author sjnickerson@google.com (Simon Nickerson) */ public interface Diff { - /** Gets the path of the file this difference applies to */ - public Path getPath(); + /** Gets the name of the file this difference applies to */ + public String getRelevantFileName(); /** * Applies this difference to the supplied {@code sourceFile}. diff --git a/check_api/src/main/java/com/google/errorprone/apply/DiffApplier.java b/check_api/src/main/java/com/google/errorprone/apply/DiffApplier.java index f633c902fbf..467e0b81f47 100644 --- a/check_api/src/main/java/com/google/errorprone/apply/DiffApplier.java +++ b/check_api/src/main/java/com/google/errorprone/apply/DiffApplier.java @@ -20,7 +20,6 @@ import com.google.common.collect.Sets; import com.google.common.util.concurrent.AbstractService; import java.io.IOException; -import java.nio.file.Path; import java.util.Set; import java.util.concurrent.ArrayBlockingQueue; import java.util.concurrent.ConcurrentSkipListSet; @@ -41,8 +40,8 @@ public class DiffApplier extends AbstractService { private static final Logger logger = Logger.getLogger(DiffApplier.class.getName()); private final ExecutorService workerService; - private final Set refactoredPaths; - private final Set diffsFailedPaths; + private final Set refactoredPaths; + private final Set diffsFailedPaths; private final FileSource source; private final FileDestination destination; private final AtomicInteger completedFiles; @@ -68,7 +67,7 @@ public DiffApplier(int diffParallelism, FileSource source, FileDestination desti diffParallelism, 5, TimeUnit.SECONDS, - new ArrayBlockingQueue<>(50), + new ArrayBlockingQueue(50), new ThreadPoolExecutor.CallerRunsPolicy()); } @@ -115,7 +114,7 @@ private final class Task implements Runnable { @Override public void run() { try { - SourceFile file = source.readFile(diff.getPath()); + SourceFile file = source.readFile(diff.getRelevantFileName()); diff.applyDifferences(file); destination.writeFile(file); @@ -124,8 +123,8 @@ public void run() { logger.log(Level.INFO, String.format("Completed %d files in %s", completed, stopwatch)); } } catch (IOException | DiffNotApplicableException e) { - logger.log(Level.WARNING, "Failed to apply diff to file " + diff.getPath(), e); - diffsFailedPaths.add(diff.getPath()); + logger.log(Level.WARNING, "Failed to apply diff to file " + diff.getRelevantFileName(), e); + diffsFailedPaths.add(diff.getRelevantFileName()); } finally { decrementTasks(); } @@ -133,7 +132,7 @@ public void run() { } public Future put(Diff diff) { - if (refactoredPaths.add(diff.getPath())) { + if (refactoredPaths.add(diff.getRelevantFileName())) { runState.incrementAndGet(); return workerService.submit(new Task(diff)); } diff --git a/check_api/src/main/java/com/google/errorprone/apply/FileSource.java b/check_api/src/main/java/com/google/errorprone/apply/FileSource.java index c69d1af65f2..b1cd56a55ca 100644 --- a/check_api/src/main/java/com/google/errorprone/apply/FileSource.java +++ b/check_api/src/main/java/com/google/errorprone/apply/FileSource.java @@ -17,10 +17,9 @@ package com.google.errorprone.apply; import java.io.IOException; -import java.nio.file.Path; /** @author sjnickerson@google.com (Simon Nickerson) */ public interface FileSource { - SourceFile readFile(Path path) throws IOException; + SourceFile readFile(String path) throws IOException; } diff --git a/check_api/src/main/java/com/google/errorprone/apply/FsFileDestination.java b/check_api/src/main/java/com/google/errorprone/apply/FsFileDestination.java index 634bb9b56d8..cf5ccd439b0 100644 --- a/check_api/src/main/java/com/google/errorprone/apply/FsFileDestination.java +++ b/check_api/src/main/java/com/google/errorprone/apply/FsFileDestination.java @@ -19,13 +19,21 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; +import java.nio.file.Path; /** A {@link FileDestination} that writes content to a destination on the local filesystem. */ public final class FsFileDestination implements FileDestination { + private final Path rootPath; + + public FsFileDestination(Path rootPath) { + this.rootPath = rootPath; + } + @Override public void writeFile(SourceFile update) throws IOException { - Files.write(update.getPath(), update.getSourceText().getBytes(StandardCharsets.UTF_8)); + Path targetPath = rootPath.resolve(update.getPath()); + Files.write(targetPath, update.getSourceText().getBytes(StandardCharsets.UTF_8)); } @Override diff --git a/check_api/src/main/java/com/google/errorprone/apply/FsFileSource.java b/check_api/src/main/java/com/google/errorprone/apply/FsFileSource.java index c133ce473b5..cb747312286 100644 --- a/check_api/src/main/java/com/google/errorprone/apply/FsFileSource.java +++ b/check_api/src/main/java/com/google/errorprone/apply/FsFileSource.java @@ -24,8 +24,15 @@ /** A FileSource that reads source files from the local filesystem. */ public final class FsFileSource implements FileSource { + private final Path rootPath; + + public FsFileSource(Path rootPath) { + this.rootPath = rootPath; + } + @Override - public SourceFile readFile(Path path) throws IOException { - return new SourceFile(path, new String(Files.readAllBytes(path), StandardCharsets.UTF_8)); + public SourceFile readFile(String path) throws IOException { + return new SourceFile( + path, new String(Files.readAllBytes(rootPath.resolve(path)), StandardCharsets.UTF_8)); } } diff --git a/check_api/src/main/java/com/google/errorprone/apply/SourceFile.java b/check_api/src/main/java/com/google/errorprone/apply/SourceFile.java index 18766562d96..54d927ad9ba 100644 --- a/check_api/src/main/java/com/google/errorprone/apply/SourceFile.java +++ b/check_api/src/main/java/com/google/errorprone/apply/SourceFile.java @@ -23,8 +23,6 @@ import java.io.LineNumberReader; import java.io.StringReader; import java.nio.CharBuffer; -import java.nio.file.Path; -import java.nio.file.Paths; import java.util.ArrayList; import java.util.List; import javax.tools.JavaFileObject; @@ -39,26 +37,20 @@ */ public class SourceFile { - private final Path path; + private final String path; private final StringBuilder sourceBuilder; public static SourceFile create(JavaFileObject fileObject) throws IOException { - Path path; - try { - path = Paths.get(fileObject.toUri()); - } catch (IllegalArgumentException e) { - throw new IllegalArgumentException(fileObject.toUri().toString(), e); - } - return new SourceFile(path, fileObject.getCharContent(false)); + return new SourceFile(fileObject.toUri().getPath(), fileObject.getCharContent(false)); } - public SourceFile(Path path, CharSequence source) { + public SourceFile(String path, CharSequence source) { this.path = path; sourceBuilder = new StringBuilder(source); } /** Returns the path for this source file */ - public Path getPath() { + public String getPath() { return path; } diff --git a/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java b/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java index 3c6c9b1c3aa..3c29b0be5f7 100644 --- a/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java +++ b/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java @@ -80,7 +80,6 @@ import com.sun.tools.javac.util.Options; import com.sun.tools.javac.util.Position; import java.io.IOException; -import java.nio.file.Paths; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Arrays; @@ -643,7 +642,7 @@ public static boolean compilesWithFix(Fix fix, VisitorState state) { try { fixSource = new SourceFile( - Paths.get(modifiedFile.toUri()), + modifiedFile.getName(), modifiedFile.getCharContent(false /*ignoreEncodingErrors*/)); } catch (IOException e) { return false; diff --git a/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java b/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java index 881f6ca0c0c..6e9071b12e4 100644 --- a/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java +++ b/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java @@ -720,6 +720,22 @@ public boolean matches(MethodInvocationTree methodInvocationTree, VisitorState s * "javax.annotation.Nullable", or "some.package.OuterClassName$InnerClassName") */ public static Matcher hasAnnotation(final String annotationClass) { + return new Matcher() { + @Override + public boolean matches(T tree, VisitorState state) { + return ASTHelpers.hasAnnotation(ASTHelpers.getDeclaredSymbol(tree), annotationClass, state); + } + }; + } + + /** + * Determines whether an expression refers to a symbol that has an annotation of the given type. + * This includes annotations inherited from superclasses due to @Inherited. + * + * @param annotationClass the binary class name of the annotation (e.g. + * "javax.annotation.Nullable", or "some.package.OuterClassName$InnerClassName") + */ + public static Matcher symbolHasAnnotation(final String annotationClass) { return new Matcher() { @Override public boolean matches(T tree, VisitorState state) { @@ -736,6 +752,22 @@ public boolean matches(T tree, VisitorState state) { */ public static Matcher hasAnnotation( final Class inputClass) { + return new Matcher() { + @Override + public boolean matches(T tree, VisitorState state) { + return ASTHelpers.hasAnnotation(ASTHelpers.getDeclaredSymbol(tree), inputClass, state); + } + }; + } + + /** + * Determines whether an expression refers to a symbol that has an annotation of the given type. + * This includes annotations inherited from superclasses due to @Inherited. + * + * @param inputClass The class of the annotation to look for (e.g, Produces.class). + */ + public static Matcher symbolHasAnnotation( + final Class inputClass) { return new Matcher() { @Override public boolean matches(T tree, VisitorState state) { diff --git a/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java b/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java index 1bc89583ef5..5fb0f9d80e3 100644 --- a/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java +++ b/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java @@ -17,6 +17,7 @@ package com.google.errorprone.scanner; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.BugPattern.Suppressibility; @@ -147,6 +148,7 @@ public class ErrorProneScanner extends Scanner { private final Set> customSuppressionAnnotations = new HashSet<>(); private final Map severities; + private final ImmutableSet bugCheckers; /** * Create an error-prone scanner for a non-hardcoded set of checkers. @@ -182,8 +184,9 @@ public ErrorProneScanner(Iterable checkers) { * @param severities The default check severities. */ public ErrorProneScanner(Iterable checkers, Map severities) { + this.bugCheckers = ImmutableSet.copyOf(checkers); this.severities = severities; - for (BugChecker checker : checkers) { + for (BugChecker checker : this.bugCheckers) { registerNodeTypes(checker); } } @@ -1205,4 +1208,8 @@ protected void handleError(Suppressible s, Throwable t) { public Map severityMap() { return severities; } + + public ImmutableSet getBugCheckers() { + return this.bugCheckers; + } } diff --git a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java index ac919bb5760..8259b0f67d4 100644 --- a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java +++ b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java @@ -89,9 +89,11 @@ import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; +import java.util.stream.Stream; import javax.annotation.Nullable; import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.ElementKind; @@ -140,12 +142,23 @@ public static boolean sameVariable(ExpressionTree expr1, ExpressionTree expr2) { } /** - * Gets the symbol for a tree. Returns null if this tree does not have a symbol because it is of - * the wrong type, if {@code tree} is null, or if the symbol cannot be found due to a compilation - * error. + * Gets the symbol declared by a tree. Returns null if this tree does not declare a symbol, if + * {@code tree} is null. */ - // TODO(eaftan): refactor other code that accesses symbols to use this method - public static Symbol getSymbol(Tree tree) { + public static Symbol getDeclaredSymbol(Tree tree) { + if (tree instanceof AnnotationTree) { + return getSymbol(((AnnotationTree) tree).getAnnotationType()); + } + if (tree instanceof PackageTree) { + return getSymbol((PackageTree) tree); + } + if (tree instanceof ParameterizedTypeTree) { + return getSymbol(((ParameterizedTypeTree) tree).getType()); + } + if (tree instanceof TypeParameterTree) { + Type type = ((JCTypeParameter) tree).type; + return type == null ? null : type.tsym; + } if (tree instanceof ClassTree) { return getSymbol((ClassTree) tree); } @@ -155,6 +168,16 @@ public static Symbol getSymbol(Tree tree) { if (tree instanceof VariableTree) { return getSymbol((VariableTree) tree); } + return null; + } + + /** + * Gets the symbol for a tree. Returns null if this tree does not have a symbol because it is of + * the wrong type, if {@code tree} is null, or if the symbol cannot be found due to a compilation + * error. + */ + // TODO(eaftan): refactor other code that accesses symbols to use this method + public static Symbol getSymbol(Tree tree) { if (tree instanceof JCFieldAccess) { return ((JCFieldAccess) tree).sym; } @@ -167,24 +190,11 @@ public static Symbol getSymbol(Tree tree) { if (tree instanceof JCNewClass) { return ASTHelpers.getSymbol((NewClassTree) tree); } - if (tree instanceof AnnotationTree) { - return getSymbol(((AnnotationTree) tree).getAnnotationType()); - } - if (tree instanceof PackageTree) { - return getSymbol((PackageTree) tree); - } - if (tree instanceof ParameterizedTypeTree) { - return getSymbol(((ParameterizedTypeTree) tree).getType()); - } - if (tree instanceof TypeParameterTree) { - Type type = ((JCTypeParameter) tree).type; - return type == null ? null : type.tsym; - } if (tree instanceof MemberReferenceTree) { return ((JCMemberReference) tree).sym; } - return null; + return getDeclaredSymbol(tree); } /** Gets the symbol for a class. */ @@ -438,13 +448,27 @@ public static MethodSymbol findSuperMethodInType( } public static Set findSuperMethods(MethodSymbol methodSymbol, Types types) { + return findSuperMethods(methodSymbol, types, false) + .collect(Collectors.toCollection(LinkedHashSet::new)); + } + + /** + * Finds (if it exists) first (in the class hierarchy) non-interface super method of given {@code + * method}. + */ + public static Optional findSuperMethod(MethodSymbol methodSymbol, Types types) { + return findSuperMethods(methodSymbol, types, true).findFirst(); + } + + private static Stream findSuperMethods( + MethodSymbol methodSymbol, Types types, boolean skipInterfaces) { TypeSymbol owner = (TypeSymbol) methodSymbol.owner; return types .closure(owner.type) .stream() + .filter(closureTypes -> skipInterfaces ? !closureTypes.isInterface() : true) .map(type -> findSuperMethodInType(methodSymbol, type, types)) - .filter(x -> x != null) - .collect(Collectors.toCollection(LinkedHashSet::new)); + .filter(Objects::nonNull); } /** @@ -530,7 +554,7 @@ public static boolean hasAnnotation( */ public static boolean hasAnnotation( Tree tree, Class annotationClass, VisitorState state) { - Symbol sym = getSymbol(tree); + Symbol sym = getDeclaredSymbol(tree); return hasAnnotation(sym, annotationClass.getName(), state); } diff --git a/check_api/src/test/java/com/google/errorprone/apply/SourceFileTest.java b/check_api/src/test/java/com/google/errorprone/apply/SourceFileTest.java index 802674cd57a..dd24c6d612d 100644 --- a/check_api/src/test/java/com/google/errorprone/apply/SourceFileTest.java +++ b/check_api/src/test/java/com/google/errorprone/apply/SourceFileTest.java @@ -18,8 +18,6 @@ import static org.junit.Assert.assertEquals; -import java.nio.file.Path; -import java.nio.file.Paths; import java.util.Arrays; import java.util.List; import org.junit.Before; @@ -35,7 +33,7 @@ @RunWith(JUnit4.class) public class SourceFileTest { - private static final Path DUMMY_PATH = Paths.get("java/com/google/foo/bar/FooBar.java"); + private static final String DUMMY_PATH = "java/com/google/foo/bar/FooBar.java"; private static final String SOURCE_TEXT = "// Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do\n" + "// eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut\n" diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java index 05f688267c0..1ee420935da 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java @@ -17,8 +17,8 @@ package com.google.errorprone.bugpatterns; import static com.google.errorprone.matchers.Description.NO_MATCH; -import static com.google.errorprone.matchers.Matchers.hasAnnotation; import static com.google.errorprone.matchers.Matchers.instanceMethod; +import static com.google.errorprone.matchers.Matchers.symbolHasAnnotation; import static com.google.errorprone.matchers.Matchers.toType; import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; import static com.google.errorprone.util.ASTHelpers.getReceiver; @@ -52,7 +52,7 @@ public abstract class AbstractMustBeClosedChecker extends BugChecker { protected static final Matcher HAS_MUST_BE_CLOSED_ANNOTATION = - hasAnnotation(MustBeClosed.class.getCanonicalName()); + symbolHasAnnotation(MustBeClosed.class.getCanonicalName()); private static final Matcher CLOSE_METHOD = instanceMethod().onDescendantOf("java.lang.AutoCloseable").named("close"); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/BadAnnotationImplementation.java b/core/src/main/java/com/google/errorprone/bugpatterns/BadAnnotationImplementation.java index d44f58cdcba..48fcbbec170 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/BadAnnotationImplementation.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/BadAnnotationImplementation.java @@ -29,6 +29,7 @@ import com.google.common.base.Predicate; import com.google.common.base.Verify; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.matchers.Description; @@ -56,7 +57,8 @@ "Classes that implement Annotation must override equals and hashCode. Consider " + "using AutoAnnotation instead of implementing Annotation by hand.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.LIKELY_ERROR ) public class BadAnnotationImplementation extends BugChecker implements ClassTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/BadComparable.java b/core/src/main/java/com/google/errorprone/bugpatterns/BadComparable.java index 0f09cfa19ae..397d18e8b60 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/BadComparable.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/BadComparable.java @@ -28,6 +28,7 @@ import static com.google.errorprone.suppliers.Suppliers.INT_TYPE; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.TypeCastTreeMatcher; import com.google.errorprone.fixes.Fix; @@ -56,7 +57,8 @@ + " the target type (JLS 5.1.3). In a compare or compareTo method, this can cause" + " incorrect and unstable sort orders.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class BadComparable extends BugChecker implements TypeCastTreeMatcher { /** Matcher for the overriding method of 'int java.lang.Comparable.compareTo(T other)' */ diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/BoxedPrimitiveConstructor.java b/core/src/main/java/com/google/errorprone/bugpatterns/BoxedPrimitiveConstructor.java index bda62354767..33a7631a5c7 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/BoxedPrimitiveConstructor.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/BoxedPrimitiveConstructor.java @@ -27,6 +27,7 @@ import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.Category; import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; import com.google.errorprone.fixes.Fix; @@ -56,7 +57,8 @@ name = "BoxedPrimitiveConstructor", category = Category.JDK, summary = "valueOf or autoboxing provides better time and space performance", - severity = SeverityLevel.WARNING + severity = SeverityLevel.WARNING, + tags = StandardTags.PERFORMANCE ) public class BoxedPrimitiveConstructor extends BugChecker implements NewClassTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ClassCanBeStatic.java b/core/src/main/java/com/google/errorprone/bugpatterns/ClassCanBeStatic.java index 70b60ebbb53..42cedd60240 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ClassCanBeStatic.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ClassCanBeStatic.java @@ -21,6 +21,7 @@ import static com.google.errorprone.matchers.Description.NO_MATCH; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.fixes.SuggestedFixes; @@ -43,7 +44,8 @@ + " of its enclosing class. An inner class that is made non-static unnecessarily" + " uses more memory and does not make the intent of the class clear.", category = JDK, - severity = WARNING + severity = WARNING, + tags = {StandardTags.STYLE, StandardTags.PERFORMANCE} ) public class ClassCanBeStatic extends BugChecker implements ClassTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ClassNewInstance.java b/core/src/main/java/com/google/errorprone/bugpatterns/ClassNewInstance.java index dbd68befeee..21626fcebe8 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ClassNewInstance.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ClassNewInstance.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -62,7 +63,8 @@ summary = "Class.newInstance() bypasses exception checking; prefer" + " getDeclaredConstructor().newInstance()", - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class ClassNewInstance extends BugChecker implements MethodInvocationTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ConstantField.java b/core/src/main/java/com/google/errorprone/bugpatterns/ConstantField.java index 5195f2493e6..508b611c781 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ConstantField.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ConstantField.java @@ -21,6 +21,7 @@ import com.google.common.base.CaseFormat; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; import com.google.errorprone.fixes.SuggestedFixes; @@ -37,7 +38,8 @@ name = "ConstantField", category = JDK, summary = "Field name is CONSTANT_CASE, but field is not static and final", - severity = SUGGESTION + severity = SUGGESTION, + tags = StandardTags.STYLE ) public class ConstantField extends BugChecker implements VariableTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DefaultCharset.java b/core/src/main/java/com/google/errorprone/bugpatterns/DefaultCharset.java index 45de0a0db7f..423077ff575 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/DefaultCharset.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/DefaultCharset.java @@ -29,6 +29,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; @@ -74,7 +75,8 @@ summary = "Implicit use of the platform default charset, which can result in e.g. non-ASCII" + " characters being silently replaced with '?' in many environments", - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class DefaultCharset extends BugChecker implements MethodInvocationTreeMatcher, NewClassTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/EqualsHashCode.java b/core/src/main/java/com/google/errorprone/bugpatterns/EqualsHashCode.java index 8e256b5b859..697a21fab96 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/EqualsHashCode.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/EqualsHashCode.java @@ -31,6 +31,7 @@ import com.google.common.collect.ImmutableList; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.matchers.Description; @@ -53,7 +54,8 @@ name = "EqualsHashCode", summary = "Classes that override equals should also override hashCode.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class EqualsHashCode extends BugChecker implements ClassTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ExpectedExceptionChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/ExpectedExceptionChecker.java index 506372d740a..c4110aae9e4 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ExpectedExceptionChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ExpectedExceptionChecker.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.fixes.Fix; import com.google.errorprone.matchers.Description; @@ -39,7 +40,8 @@ name = "ExpectedExceptionChecker", category = JUNIT, summary = "Calls to ExpectedException#expect should always be followed by exactly one statement.", - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class ExpectedExceptionChecker extends AbstractExpectedExceptionChecker { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/Finally.java b/core/src/main/java/com/google/errorprone/bugpatterns/Finally.java index fd9998b2120..62996caefbf 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/Finally.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/Finally.java @@ -20,6 +20,7 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.BreakTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.ContinueTreeMatcher; @@ -62,7 +63,8 @@ + " try-catch block will be ignored. Consider using try-with-resources instead.", category = JDK, severity = WARNING, - generateExamplesFromTestCases = false + generateExamplesFromTestCases = false, + tags = StandardTags.FRAGILE_CODE ) public class Finally extends BugChecker implements ContinueTreeMatcher, ThrowTreeMatcher, BreakTreeMatcher, ReturnTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/FloatingPointLiteralPrecision.java b/core/src/main/java/com/google/errorprone/bugpatterns/FloatingPointLiteralPrecision.java index cbe89b68147..718c2d62b79 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/FloatingPointLiteralPrecision.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/FloatingPointLiteralPrecision.java @@ -23,6 +23,7 @@ import com.google.common.base.CharMatcher; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.LiteralTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -37,7 +38,8 @@ name = "FloatingPointLiteralPrecision", category = JDK, summary = "Floating point literal loses precision", - severity = WARNING + severity = WARNING, + tags = StandardTags.STYLE ) public class FloatingPointLiteralPrecision extends BugChecker implements LiteralTreeMatcher { @Override diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClash.java b/core/src/main/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClash.java index e642eb98603..5a8da54cea4 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClash.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClash.java @@ -16,6 +16,7 @@ package com.google.errorprone.bugpatterns; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.errorprone.BugPattern.Category.JDK; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.matchers.Description.NO_MATCH; @@ -24,6 +25,7 @@ import static java.util.stream.Collectors.joining; import com.google.common.collect.HashMultimap; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Multimap; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; @@ -41,10 +43,12 @@ import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Types; import com.sun.tools.javac.util.List; +import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.Deque; /** @author cushon@google.com (Liam Miller-Cushon) */ @BugPattern( @@ -88,9 +92,24 @@ public Description matchClass(ClassTree tree, VisitorState state) { } Collection clash = new ArrayList<>(methods.removeAll(functionalInterfaceSignature(state, msym))); + + // Ignore inherited methods that are overridden in the original class. Note that we have to + // handle transitive inheritance explicitly to handle cases where the visibility of an + // overridded method is expanded somewhere in the type hierarchy. + Deque worklist = new ArrayDeque<>(); + worklist.push(msym); clash.remove(msym); - // ignore inherited methods that are overridden in the original class - clash.removeIf(m -> msym.overrides(m, origin, types, false)); + while (!worklist.isEmpty()) { + MethodSymbol msym2 = worklist.removeFirst(); + ImmutableList overrides = + clash + .stream() + .filter(m -> msym2.overrides(m, origin, types, /*checkResult=*/ false)) + .collect(toImmutableList()); + worklist.addAll(overrides); + clash.removeAll(overrides); + } + if (!clash.isEmpty()) { String message = "When passing lambda arguments to this function, callers will need a cast to" diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/FunctionalInterfaceMethodChanged.java b/core/src/main/java/com/google/errorprone/bugpatterns/FunctionalInterfaceMethodChanged.java index 74fd2f69381..f9ac3705177 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/FunctionalInterfaceMethodChanged.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/FunctionalInterfaceMethodChanged.java @@ -58,7 +58,7 @@ public class FunctionalInterfaceMethodChanged extends BugChecker implements MethodTreeMatcher { private static final Matcher IS_FUNCTIONAL_INTERFACE = - Matchers.hasAnnotation(FunctionalInterface.class); + Matchers.symbolHasAnnotation(FunctionalInterface.class); @Override public Description matchMethod(MethodTree tree, VisitorState state) { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/FutureReturnValueIgnored.java b/core/src/main/java/com/google/errorprone/bugpatterns/FutureReturnValueIgnored.java index d17aeebaa6c..7458ee18b4f 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/FutureReturnValueIgnored.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/FutureReturnValueIgnored.java @@ -23,6 +23,7 @@ import static com.google.errorprone.util.ASTHelpers.hasAnnotation; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.fixes.SuggestedFix; @@ -58,7 +59,8 @@ + "If you don’t check the return value of these methods, you will never find out if they " + "threw an exception.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public final class FutureReturnValueIgnored extends AbstractReturnValueIgnored { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/GetClassOnEnum.java b/core/src/main/java/com/google/errorprone/bugpatterns/GetClassOnEnum.java index 16b2ea9b350..b7dc1424d90 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/GetClassOnEnum.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/GetClassOnEnum.java @@ -21,6 +21,7 @@ import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; @@ -34,7 +35,8 @@ name = "GetClassOnEnum", category = JDK, summary = "Calling getClass() on an enum may return a subclass of the enum type", - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class GetClassOnEnum extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/IncompatibleModifiersChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/IncompatibleModifiersChecker.java index e1751ac6c15..cb29e17aa80 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/IncompatibleModifiersChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/IncompatibleModifiersChecker.java @@ -25,6 +25,7 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Sets; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.annotations.IncompatibleModifiers; import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; @@ -51,8 +52,10 @@ + "annotations respect their @IncompatibleModifiers specifications.", linkType = NONE, category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.LIKELY_ERROR ) + // TODO(cushon): merge the implementation with RequiredModifiersChecker public class IncompatibleModifiersChecker extends BugChecker implements AnnotationTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteRead.java b/core/src/main/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteRead.java index bc16bcebe2a..5d8b2190eb1 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteRead.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteRead.java @@ -28,6 +28,7 @@ import com.google.common.collect.ImmutableList; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.matchers.Description; @@ -55,7 +56,8 @@ "Please also override int read(byte[], int, int), otherwise multi-byte reads from this " + "input stream are likely to be slow.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.PERFORMANCE ) public class InputStreamSlowMultibyteRead extends BugChecker implements ClassTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/IterableAndIterator.java b/core/src/main/java/com/google/errorprone/bugpatterns/IterableAndIterator.java index 81bcb00dfba..b3bbc038c95 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/IterableAndIterator.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/IterableAndIterator.java @@ -23,6 +23,7 @@ import com.google.common.collect.Lists; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.matchers.Description; @@ -43,7 +44,8 @@ + "while an `Iterable` is a representation of literally iterable elements. " + "An `Iterable` can generate multiple valid `Iterator`s, though.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class IterableAndIterator extends BugChecker implements ClassTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/JavaLangClash.java b/core/src/main/java/com/google/errorprone/bugpatterns/JavaLangClash.java index cc12977a946..e09d61ef37f 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/JavaLangClash.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/JavaLangClash.java @@ -23,6 +23,7 @@ import static javax.lang.model.element.Modifier.PUBLIC; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.TypeParameterTreeMatcher; @@ -44,7 +45,8 @@ name = "JavaLangClash", category = JDK, summary = "Never reuse class names from java.lang", - severity = WARNING + severity = WARNING, + tags = StandardTags.STYLE ) public class JavaLangClash extends BugChecker implements ClassTreeMatcher, TypeParameterTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/LogicalAssignment.java b/core/src/main/java/com/google/errorprone/bugpatterns/LogicalAssignment.java index f8f37ab02c0..e495c192150 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/LogicalAssignment.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/LogicalAssignment.java @@ -21,6 +21,7 @@ import static com.google.errorprone.matchers.Description.NO_MATCH; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.DoWhileLoopTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.ForLoopTreeMatcher; @@ -44,7 +45,8 @@ summary = "Assignment where a boolean expression was expected;" + " use == if this assignment wasn't expected or add parentheses for clarity.", - severity = WARNING + severity = WARNING, + tags = StandardTags.LIKELY_ERROR ) public class LogicalAssignment extends BugChecker implements IfTreeMatcher, WhileLoopTreeMatcher, DoWhileLoopTreeMatcher, ForLoopTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MissingDefault.java b/core/src/main/java/com/google/errorprone/bugpatterns/MissingDefault.java index f5929f043cd..4c93874b14b 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/MissingDefault.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MissingDefault.java @@ -23,6 +23,7 @@ import com.google.common.collect.Iterables; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.SwitchTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -45,7 +46,8 @@ "The Google Java Style Guide requires that each switch statement includes a default statement" + " group, even if it contains no code. (This requirement is lifted for any switch" + " statement that covers all values of an enum.)", - severity = WARNING + severity = WARNING, + tags = StandardTags.STYLE ) public class MissingDefault extends BugChecker implements SwitchTreeMatcher { @Override diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MissingOverride.java b/core/src/main/java/com/google/errorprone/bugpatterns/MissingOverride.java index 160454a1248..09c6c46ac07 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/MissingOverride.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MissingOverride.java @@ -20,6 +20,7 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -38,7 +39,8 @@ name = "MissingOverride", summary = "method overrides method in supertype; expected @Override", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.STYLE ) public class MissingOverride extends BugChecker implements MethodTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MixedArrayDimensions.java b/core/src/main/java/com/google/errorprone/bugpatterns/MixedArrayDimensions.java index 9ce5187e85b..c58b8eb7476 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/MixedArrayDimensions.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MixedArrayDimensions.java @@ -24,6 +24,7 @@ import com.google.common.base.CharMatcher; import com.google.common.collect.ImmutableList; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; @@ -44,6 +45,7 @@ summary = "C-style array declarations should not be used", severity = SUGGESTION, linkType = CUSTOM, + tags = StandardTags.STYLE, link = "https://google.github.io/styleguide/javaguide.html#s4.8.3.2-array-declarations" ) public class MixedArrayDimensions extends BugChecker diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MultiVariableDeclaration.java b/core/src/main/java/com/google/errorprone/bugpatterns/MultiVariableDeclaration.java index 2c3c2f4b439..5c251b12a1c 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/MultiVariableDeclaration.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MultiVariableDeclaration.java @@ -26,6 +26,7 @@ import com.google.common.collect.Iterators; import com.google.common.collect.PeekingIterator; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.BlockTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; @@ -47,6 +48,7 @@ category = JDK, severity = SUGGESTION, linkType = CUSTOM, + tags = StandardTags.STYLE, link = "https://google.github.io/styleguide/javaguide.html#s4.8.2.1-variables-per-declaration" ) public class MultiVariableDeclaration extends BugChecker diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MultipleTopLevelClasses.java b/core/src/main/java/com/google/errorprone/bugpatterns/MultipleTopLevelClasses.java index 9ef9f8de05a..1b7643b8fcd 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/MultipleTopLevelClasses.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MultipleTopLevelClasses.java @@ -22,6 +22,7 @@ import com.google.common.base.Joiner; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher; import com.google.errorprone.matchers.Description; @@ -40,6 +41,7 @@ severity = SUGGESTION, documentSuppression = false, linkType = CUSTOM, + tags = StandardTags.STYLE, link = "https://google.github.io/styleguide/javaguide.html#s3.4.1-one-top-level-class" ) public class MultipleTopLevelClasses extends BugChecker implements CompilationUnitTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NarrowingCompoundAssignment.java b/core/src/main/java/com/google/errorprone/bugpatterns/NarrowingCompoundAssignment.java index dffb146e2ce..342ef4ed5a6 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/NarrowingCompoundAssignment.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NarrowingCompoundAssignment.java @@ -23,6 +23,7 @@ import com.google.common.base.Optional; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.CompoundAssignmentTreeMatcher; import com.google.errorprone.fixes.Fix; @@ -41,7 +42,8 @@ name = "NarrowingCompoundAssignment", summary = "Compound assignments may hide dangerous casts", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class NarrowingCompoundAssignment extends BugChecker implements CompoundAssignmentTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NoAllocationChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/NoAllocationChecker.java index 0a03abae7e8..7894b6dbabe 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/NoAllocationChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NoAllocationChecker.java @@ -34,6 +34,7 @@ import static com.google.errorprone.matchers.Matchers.kindIs; import static com.google.errorprone.matchers.Matchers.methodReturnsNonPrimitiveType; import static com.google.errorprone.matchers.Matchers.not; +import static com.google.errorprone.matchers.Matchers.symbolHasAnnotation; import static com.google.errorprone.matchers.Matchers.typeCast; import static com.google.errorprone.matchers.Matchers.variableInitializer; import static com.google.errorprone.matchers.Matchers.variableType; @@ -144,7 +145,7 @@ public class NoAllocationChecker extends BugChecker hasAnnotation(NoAllocation.class.getName()); private static final Matcher noAllocationMethodInvocationMatcher = - hasAnnotation(NoAllocation.class.getName()); + symbolHasAnnotation(NoAllocation.class.getName()); private static final Matcher anyExpression = anything(); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NonAtomicVolatileUpdate.java b/core/src/main/java/com/google/errorprone/bugpatterns/NonAtomicVolatileUpdate.java index f66caf8ee88..fbf19c78055 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/NonAtomicVolatileUpdate.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NonAtomicVolatileUpdate.java @@ -29,6 +29,7 @@ import static com.google.errorprone.matchers.Matchers.toType; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.AssignmentTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.CompoundAssignmentTreeMatcher; @@ -63,7 +64,8 @@ + "on this variable in a synchronized block. If the variable is an integer, you could " + "use an AtomicInteger instead of a volatile int.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class NonAtomicVolatileUpdate extends BugChecker implements UnaryTreeMatcher, CompoundAssignmentTreeMatcher, AssignmentTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NonOverridingEquals.java b/core/src/main/java/com/google/errorprone/bugpatterns/NonOverridingEquals.java index c739bf59b98..e7de9076b3d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/NonOverridingEquals.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NonOverridingEquals.java @@ -36,6 +36,7 @@ import static com.sun.tools.javac.code.Flags.ENUM; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -57,7 +58,8 @@ name = "NonOverridingEquals", summary = "equals method doesn't override Object.equals", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class NonOverridingEquals extends BugChecker implements MethodTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NullableConstructor.java b/core/src/main/java/com/google/errorprone/bugpatterns/NullableConstructor.java index a1077fbb005..2351fdbeb40 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/NullableConstructor.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NullableConstructor.java @@ -21,6 +21,7 @@ import static com.google.errorprone.matchers.Description.NO_MATCH; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -36,7 +37,8 @@ summary = "Constructors should not be annotated with @Nullable since they cannot return null", explanation = "Constructors never return null.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.STYLE ) public class NullableConstructor extends BugChecker implements MethodTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NullablePrimitive.java b/core/src/main/java/com/google/errorprone/bugpatterns/NullablePrimitive.java index 6410d729665..e7368ae7c49 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/NullablePrimitive.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NullablePrimitive.java @@ -21,6 +21,7 @@ import static com.google.errorprone.matchers.Description.NO_MATCH; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.AnnotatedTypeTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; @@ -43,7 +44,8 @@ summary = "@Nullable should not be used for primitive types since they cannot be null", explanation = "Primitives can never be null.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.STYLE ) public class NullablePrimitive extends BugChecker implements AnnotatedTypeTreeMatcher, VariableTreeMatcher, MethodTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NullableVoid.java b/core/src/main/java/com/google/errorprone/bugpatterns/NullableVoid.java index c8c128d880a..818cbeb9afb 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/NullableVoid.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NullableVoid.java @@ -21,6 +21,7 @@ import static com.google.errorprone.matchers.Description.NO_MATCH; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -39,7 +40,8 @@ + " since they cannot return null", explanation = "void-returning methods cannot return null.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.STYLE ) public class NullableVoid extends BugChecker implements MethodTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/OperatorPrecedence.java b/core/src/main/java/com/google/errorprone/bugpatterns/OperatorPrecedence.java index 7065eea0888..813b50d4e59 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/OperatorPrecedence.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/OperatorPrecedence.java @@ -21,6 +21,7 @@ import static com.google.errorprone.matchers.Description.NO_MATCH; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -37,7 +38,8 @@ name = "OperatorPrecedence", category = JDK, summary = "Use grouping parenthesis to make the operator precedence explicit", - severity = WARNING + severity = WARNING, + tags = StandardTags.STYLE ) public class OperatorPrecedence extends BugChecker implements BinaryTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/PackageLocation.java b/core/src/main/java/com/google/errorprone/bugpatterns/PackageLocation.java index 6e4e7539bb7..d035e61fe6d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/PackageLocation.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/PackageLocation.java @@ -22,6 +22,7 @@ import com.google.common.base.CharMatcher; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.annotations.SuppressPackageLocation; import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher; @@ -37,7 +38,8 @@ severity = SUGGESTION, suppressibility = CUSTOM_ANNOTATION, documentSuppression = false, - customSuppressionAnnotations = SuppressPackageLocation.class + customSuppressionAnnotations = SuppressPackageLocation.class, + tags = StandardTags.STYLE ) public class PackageLocation extends BugChecker implements CompilationUnitTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/PreconditionsInvalidPlaceholder.java b/core/src/main/java/com/google/errorprone/bugpatterns/PreconditionsInvalidPlaceholder.java index 5678d053451..f6c0a7ffcfc 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/PreconditionsInvalidPlaceholder.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/PreconditionsInvalidPlaceholder.java @@ -21,6 +21,7 @@ import static com.google.errorprone.matchers.Matchers.staticMethod; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -44,7 +45,8 @@ + "message template string and the number of arguments does not match the number of " + "%s placeholders.", category = GUAVA, - severity = WARNING + severity = WARNING, + tags = StandardTags.LIKELY_ERROR ) public class PreconditionsInvalidPlaceholder extends BugChecker implements MethodInvocationTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ProtoFieldPreconditionsCheckNotNull.java b/core/src/main/java/com/google/errorprone/bugpatterns/ProtoFieldPreconditionsCheckNotNull.java index 526c06396bd..4acff220ce2 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ProtoFieldPreconditionsCheckNotNull.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ProtoFieldPreconditionsCheckNotNull.java @@ -25,6 +25,7 @@ import static com.google.errorprone.matchers.Matchers.staticMethod; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.fixes.Fix; @@ -51,7 +52,8 @@ + "If you meant to check whether an optional field has been set, you should use the " + "hasField() method instead.", category = GUAVA, - severity = WARNING + severity = WARNING, + tags = StandardTags.LIKELY_ERROR ) public class ProtoFieldPreconditionsCheckNotNull extends BugChecker implements MethodInvocationTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ReferenceEquality.java b/core/src/main/java/com/google/errorprone/bugpatterns/ReferenceEquality.java index 73ce97a55bb..7ee94cce68d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ReferenceEquality.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ReferenceEquality.java @@ -21,6 +21,7 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; @@ -37,7 +38,8 @@ name = "ReferenceEquality", summary = "Comparison using reference equality instead of value equality", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class ReferenceEquality extends AbstractReferenceEquality { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/RemoveUnusedImports.java b/core/src/main/java/com/google/errorprone/bugpatterns/RemoveUnusedImports.java index 2b4dd8f4ecf..017d0a15e71 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/RemoveUnusedImports.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/RemoveUnusedImports.java @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher; import com.google.errorprone.bugpatterns.StaticImports.StaticImportInfo; @@ -60,7 +61,8 @@ explanation = "This import is unused.", category = JDK, severity = SUGGESTION, - documentSuppression = false + documentSuppression = false, + tags = StandardTags.STYLE ) public final class RemoveUnusedImports extends BugChecker implements CompilationUnitTreeMatcher { @Override diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/RequiredModifiersChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/RequiredModifiersChecker.java index df028ab0fd6..e6ff67fe2d9 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/RequiredModifiersChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/RequiredModifiersChecker.java @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.annotations.RequiredModifiers; import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; @@ -46,7 +47,8 @@ + "use it on an element that is missing one or more required modifiers.", linkType = NONE, category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.LIKELY_ERROR ) public class RequiredModifiersChecker extends BugChecker implements AnnotationTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ShortCircuitBoolean.java b/core/src/main/java/com/google/errorprone/bugpatterns/ShortCircuitBoolean.java index 9ff37ac4bd9..b7f2523747a 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ShortCircuitBoolean.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ShortCircuitBoolean.java @@ -23,22 +23,32 @@ import static com.google.errorprone.util.ASTHelpers.isSameType; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.sun.source.tree.BinaryTree; import com.sun.source.tree.Tree; +import com.sun.source.tree.Tree.Kind; +import com.sun.source.util.TreeScanner; import com.sun.tools.javac.tree.JCTree; +import java.util.Iterator; -/** @author cushon@google.com (Liam Miller-Cushon) */ +/** + * @author cushon@google.com (Liam Miller-Cushon) + * @author sulku@google.com (Marsela Sulku) + * @author mariasam@google.com (Maria Sam) + */ @BugPattern( name = "ShortCircuitBoolean", category = JDK, summary = "Prefer the short-circuiting boolean operators && and || to & and |.", - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class ShortCircuitBoolean extends BugChecker implements BinaryTreeMatcher { + @Override public Description matchBinary(BinaryTree tree, VisitorState state) { switch (tree.getKind()) { @@ -51,11 +61,43 @@ public Description matchBinary(BinaryTree tree, VisitorState state) { if (!isSameType(getType(tree), state.getSymtab().booleanType, state)) { return NO_MATCH; } - return describeMatch( - tree, - SuggestedFix.replace( + + Iterator stateIterator = state.getPath().getParentPath().iterator(); + Tree parent = stateIterator.next(); + + if (parent instanceof BinaryTree + && (parent.getKind() == Kind.AND || parent.getKind() == Kind.OR)) { + return NO_MATCH; + } else { + SuggestedFix.Builder fix = SuggestedFix.builder(); + new TreeScannerBinary(state).scan(tree, fix); + return describeMatch(tree, fix.build()); + } + } + + /** Replaces the operators when visiting the binary nodes */ + public static class TreeScannerBinary extends TreeScanner { + /** saved state */ + public VisitorState state; + + /** constructor */ + public TreeScannerBinary(VisitorState currState) { + this.state = currState; + } + + @Override + public Void visitBinary(BinaryTree tree, SuggestedFix.Builder p) { + if (tree.getKind() == Kind.AND || tree.getKind() == Kind.OR) { + p.replace( state.getEndPosition(tree.getLeftOperand()), ((JCTree) tree.getRightOperand()).getStartPosition(), - tree.getKind() == Tree.Kind.AND ? " && " : " || ")); + tree.getKind() == Tree.Kind.AND ? " && " : " || "); + } + + return super.visitBinary(tree, p); + } } } + + + diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ShouldHaveEvenArgs.java b/core/src/main/java/com/google/errorprone/bugpatterns/ShouldHaveEvenArgs.java index c9c762446c4..3835c77cad7 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ShouldHaveEvenArgs.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ShouldHaveEvenArgs.java @@ -51,7 +51,9 @@ public class ShouldHaveEvenArgs extends BugChecker implements MethodInvocationTr instanceMethod() .onDescendantOfAny( "com.google.common.truth.MapSubject", - "com.google.common.truth.MapSubject.UsingCorrespondence") + "com.google.common.truth.MapSubject.UsingCorrespondence", + "com.google.common.truth.MultimapSubject", + "com.google.common.truth.MultimapSubject.UsingCorrespondence") .named("containsExactly"); @Override diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/SimpleDateFormatConstant.java b/core/src/main/java/com/google/errorprone/bugpatterns/SimpleDateFormatConstant.java index 6da2deccfa3..8007a9825c9 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/SimpleDateFormatConstant.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/SimpleDateFormatConstant.java @@ -25,6 +25,7 @@ import com.google.common.base.CaseFormat; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; import com.google.errorprone.fixes.Fix; @@ -45,7 +46,8 @@ name = "SimpleDateFormatConstant", category = JDK, summary = "SimpleDateFormat is not thread-safe, and should not be used as a constant field.", - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class SimpleDateFormatConstant extends BugChecker implements VariableTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StaticQualifiedUsingExpression.java b/core/src/main/java/com/google/errorprone/bugpatterns/StaticQualifiedUsingExpression.java index a8b06a29a15..37a5950e533 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/StaticQualifiedUsingExpression.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StaticQualifiedUsingExpression.java @@ -27,6 +27,7 @@ import static com.google.errorprone.matchers.Matchers.staticMethod; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MemberSelectTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -48,7 +49,8 @@ category = JDK, severity = WARNING, altNames = {"static", "static-access", "StaticAccessedFromInstance"}, - generateExamplesFromTestCases = false + generateExamplesFromTestCases = false, + tags = StandardTags.STYLE ) public class StaticQualifiedUsingExpression extends BugChecker implements MemberSelectTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TestExceptionChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/TestExceptionChecker.java index ca0546d67b0..9ec87a84324 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/TestExceptionChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/TestExceptionChecker.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; @@ -41,7 +42,8 @@ summary = "Using @Test(expected=...) is discouraged, since the test will pass if *any* statement in" + " the test method throws the expected exception", - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class TestExceptionChecker extends AbstractTestExceptionChecker { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TruthConstantAsserts.java b/core/src/main/java/com/google/errorprone/bugpatterns/TruthConstantAsserts.java index 96618eb18eb..2c1322dedf6 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/TruthConstantAsserts.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/TruthConstantAsserts.java @@ -23,6 +23,7 @@ import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -42,7 +43,8 @@ name = "TruthConstantAsserts", summary = "Truth Library assert is called on a constant.", category = TRUTH, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class TruthConstantAsserts extends BugChecker implements MethodInvocationTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterShadowing.java b/core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterShadowing.java index 73b3fbe2912..ec5dad51474 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterShadowing.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterShadowing.java @@ -24,6 +24,7 @@ import com.google.common.collect.MoreCollectors; import com.google.common.collect.Streams; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; @@ -50,7 +51,8 @@ name = "TypeParameterShadowing", summary = "Type parameter declaration overrides another type parameter already declared", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.STYLE ) public class TypeParameterShadowing extends BugChecker implements MethodTreeMatcher, ClassTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterUnusedInFormals.java b/core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterUnusedInFormals.java index ce47d9630e8..f137558fb3e 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterUnusedInFormals.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterUnusedInFormals.java @@ -20,6 +20,7 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.matchers.Description; @@ -41,7 +42,8 @@ + " generics: operations on the type parameter are unchecked, it hides unsafe casts at" + " invocations of the method, and it interacts badly with method overload resolution.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class TypeParameterUnusedInFormals extends BugChecker implements MethodTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/URLEqualsHashCode.java b/core/src/main/java/com/google/errorprone/bugpatterns/URLEqualsHashCode.java index 5a8aff8eb4b..09079b6c4f1 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/URLEqualsHashCode.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/URLEqualsHashCode.java @@ -21,6 +21,7 @@ import static com.google.errorprone.matchers.Matchers.anyOf; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; import com.google.errorprone.matchers.Description; @@ -46,7 +47,8 @@ "Creation of a Set/HashSet/HashMap of java.net.URL." + " equals() and hashCode() of java.net.URL class make blocking internet connections.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class URLEqualsHashCode extends BugChecker implements NewClassTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryStaticImport.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryStaticImport.java index 623bd075104..b6e06c51302 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryStaticImport.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryStaticImport.java @@ -20,6 +20,7 @@ import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.ImportTreeMatcher; import com.google.errorprone.bugpatterns.StaticImports.StaticImportInfo; @@ -36,7 +37,8 @@ + " replaced by equivalent non-static imports.", category = JDK, severity = SUGGESTION, - documentSuppression = false + documentSuppression = false, + tags = StandardTags.STYLE ) public class UnnecessaryStaticImport extends BugChecker implements ImportTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnsynchronizedOverridesSynchronized.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnsynchronizedOverridesSynchronized.java index 3ac5a19b0ef..b93b62501b8 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnsynchronizedOverridesSynchronized.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnsynchronizedOverridesSynchronized.java @@ -21,6 +21,7 @@ import static com.google.errorprone.util.ASTHelpers.isSameType; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.SuggestedFixes; @@ -35,7 +36,8 @@ name = "UnsynchronizedOverridesSynchronized", summary = "Unsynchronized method overrides a synchronized method.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class UnsynchronizedOverridesSynchronized extends BugChecker implements MethodTreeMatcher { @Override diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/WaitNotInLoop.java b/core/src/main/java/com/google/errorprone/bugpatterns/WaitNotInLoop.java index f38a0db0495..9341c9c8deb 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/WaitNotInLoop.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/WaitNotInLoop.java @@ -25,6 +25,7 @@ import static com.google.errorprone.matchers.WaitMatchers.waitMethodWithTimeout; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -43,7 +44,8 @@ "Because of spurious wakeups, Object.wait() and Condition.await() must always be " + "called in a loop", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class WaitNotInLoop extends BugChecker implements MethodInvocationTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/WildcardImport.java b/core/src/main/java/com/google/errorprone/bugpatterns/WildcardImport.java index 58e7454a158..98623d29af3 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/WildcardImport.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/WildcardImport.java @@ -24,6 +24,7 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher; import com.google.errorprone.fixes.Fix; @@ -58,6 +59,7 @@ severity = SUGGESTION, linkType = CUSTOM, documentSuppression = false, + tags = StandardTags.STYLE, link = "https://google.github.io/styleguide/javaguide.html#s3.3.1-wildcard-imports" ) public class WildcardImport extends BugChecker implements CompilationUnitTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/android/FragmentInjection.java b/core/src/main/java/com/google/errorprone/bugpatterns/android/FragmentInjection.java index 8994d19ccb0..54f8d8a12ae 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/android/FragmentInjection.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/android/FragmentInjection.java @@ -34,6 +34,7 @@ import com.google.common.collect.ImmutableList; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; @@ -58,7 +59,8 @@ "Classes extending PreferenceActivity must implement isValidFragment such that it does not" + " unconditionally return true to prevent vulnerability to fragment injection attacks.", category = ANDROID, - severity = WARNING + severity = WARNING, + tags = StandardTags.LIKELY_ERROR ) public class FragmentInjection extends BugChecker implements ClassTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/android/FragmentNotInstantiable.java b/core/src/main/java/com/google/errorprone/bugpatterns/android/FragmentNotInstantiable.java index 92d550928fd..7707371563f 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/android/FragmentNotInstantiable.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/android/FragmentNotInstantiable.java @@ -30,6 +30,7 @@ import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; @@ -50,7 +51,8 @@ "Subclasses of Fragment must be instantiable via Class#newInstance():" + " the class must be public, static and have a public nullary constructor", category = ANDROID, - severity = WARNING + severity = WARNING, + tags = StandardTags.LIKELY_ERROR ) public class FragmentNotInstantiable extends BugChecker implements ClassTreeMatcher { private static final String MESSAGE_BASE = "Fragment is not instantiable: "; diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristic.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristic.java index 05390913e6d..8c10254e1fe 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristic.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristic.java @@ -52,7 +52,8 @@ public boolean isAcceptableChange( .noneMatch( p -> { MatchType match = - NamedParameterComment.match(comments.get(p.formal().index()), p.formal().name()); + NamedParameterComment.match(comments.get(p.formal().index()), p.formal().name()) + .matchType(); return match == MatchType.EXACT_MATCH || match == MatchType.APPROXIMATE_MATCH; }); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterChecker.java index 1e50b6a7b93..34de4eb9950 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterChecker.java @@ -19,21 +19,18 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.errorprone.BugPattern.Category.JDK; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; -import static com.google.errorprone.matchers.Matchers.hasAnnotation; import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.common.collect.Streams; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; -import com.google.errorprone.annotations.RequiresNamedParameters; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; import com.google.errorprone.bugpatterns.argumentselectiondefects.NamedParameterComment.MatchType; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; -import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; import com.google.errorprone.util.Commented; import com.google.errorprone.util.Comments; @@ -44,7 +41,6 @@ import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.VarSymbol; import com.sun.tools.javac.parser.Tokens.Comment; -import com.sun.tools.javac.parser.Tokens.Comment.CommentStyle; import com.sun.tools.javac.util.List; import java.util.Optional; import java.util.stream.Collectors; @@ -58,13 +54,6 @@ @BugPattern( name = "NamedParameters", summary = "Parameter name in argument comment is missing or incorrect", - explanation = - "For clarity, and to avoid potentially incorrectly swapping arguments, arguments may be " - + "explicitly matched to their parameter by preceding them with a block comment " - + "containing the parameter name followed by a colon. Mismatches between the name in the " - + "comment and the actual name will then cause a compilation error. If the called method " - + "is annotated with RequiresNamedParameters then an error will occur if any names are " - + "omitted.", category = JDK, severity = WARNING ) @@ -74,132 +63,109 @@ public class NamedParameterChecker extends BugChecker @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { return matchNewClassOrMethodInvocation( - ASTHelpers.getSymbol(tree), Comments.findCommentsForArguments(tree, state), tree, state); + ASTHelpers.getSymbol(tree), Comments.findCommentsForArguments(tree, state), tree); } @Override public Description matchNewClass(NewClassTree tree, VisitorState state) { return matchNewClassOrMethodInvocation( - ASTHelpers.getSymbol(tree), Comments.findCommentsForArguments(tree, state), tree, state); + ASTHelpers.getSymbol(tree), Comments.findCommentsForArguments(tree, state), tree); } private Description matchNewClassOrMethodInvocation( - MethodSymbol symbol, - ImmutableList> arguments, - Tree tree, - VisitorState state) { + MethodSymbol symbol, ImmutableList> arguments, Tree tree) { if (symbol == null) { return Description.NO_MATCH; } - boolean commentsRequired = hasRequiresNamedParametersAnnotation().matches(tree, state); - - // if we don't have parameter names available then raise an error if comments required, else - // silently ignore + // if we don't have parameter names available then give up List parameters = symbol.getParameters(); if (containsSyntheticParameterName(parameters)) { - return commentsRequired - ? buildDescription(tree) - .setMessage( - "Method requires parameter name comments but parameter names are not available.") - .build() - : Description.NO_MATCH; + return Description.NO_MATCH; } ImmutableList labelledArguments = LabelledArgument.createFromParametersList(parameters, arguments); - ImmutableList incorrectlyLabelledArguments = - labelledArguments - .stream() - .filter(labelledArgument -> !labelledArgument.isCorrectlyAnnotated(commentsRequired)) - .collect(toImmutableList()); - - if (incorrectlyLabelledArguments.isEmpty()) { - return Description.NO_MATCH; - } - // Build fix // In general: if a comment is missing and it should be there then we suggest adding it // If a comment is wrong but matches the parameter name of a different argument then we suggest // swapping the arguments. If a comment is wrong and matches nothing then we suggest changing it SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); - for (LabelledArgument argumentWithBadLabel : incorrectlyLabelledArguments) { - switch (argumentWithBadLabel.match()) { + ImmutableList.Builder incorrectParameterDescriptions = ImmutableList.builder(); + for (LabelledArgument labelledArgument : labelledArguments) { + switch (labelledArgument.matchedComment().matchType()) { case NOT_ANNOTATED: - fixBuilder.prefixWith( - argumentWithBadLabel.actualParameter().tree(), - NamedParameterComment.toCommentText(argumentWithBadLabel.parameterName())); + case EXACT_MATCH: break; - case BAD_MATCH: case APPROXIMATE_MATCH: - // we know that this has a comment because it was a bad match - Comment badLabel = argumentWithBadLabel.label().get(); + removeComment(labelledArgument.matchedComment().comment(), fixBuilder); + addComment(labelledArgument, fixBuilder); + incorrectParameterDescriptions.add( + String.format( + "%s (comment does not conform to required style)", + labelledArgument.parameterName())); + break; + case BAD_MATCH: + Comment badLabel = labelledArgument.matchedComment().comment(); Optional maybeGoodTarget = - findGoodSwap(argumentWithBadLabel, labelledArguments); + findGoodSwap(labelledArgument, labelledArguments); if (maybeGoodTarget.isPresent()) { LabelledArgument argumentWithCorrectLabel = maybeGoodTarget.get(); fixBuilder.swap( - argumentWithBadLabel.actualParameter().tree(), + labelledArgument.actualParameter().tree(), argumentWithCorrectLabel.actualParameter().tree()); - Optional correctLabel = argumentWithCorrectLabel.label(); - if (correctLabel.isPresent()) { - replaceComment(badLabel, correctLabel.get().getText(), fixBuilder); + if (argumentWithCorrectLabel.matchedComment().matchType() == MatchType.NOT_ANNOTATED) { + removeComment(badLabel, fixBuilder); + addComment(argumentWithCorrectLabel, fixBuilder); } else { - replaceComment(badLabel, "", fixBuilder); - fixBuilder.prefixWith( - argumentWithCorrectLabel.actualParameter().tree(), - NamedParameterComment.toCommentText(argumentWithCorrectLabel.parameterName())); + replaceComment( + badLabel, + argumentWithCorrectLabel.matchedComment().comment().getText(), + fixBuilder); } } else { // there were no matches so maybe the comment is wrong - suggest a fix to change it replaceComment( badLabel, - NamedParameterComment.toCommentText(argumentWithBadLabel.parameterName()), + NamedParameterComment.toCommentText(labelledArgument.parameterName()), fixBuilder); } + incorrectParameterDescriptions.add( + String.format( + "%s (comment does not match formal parameter name)", + labelledArgument.parameterName())); break; - case EXACT_MATCH: - throw new IllegalArgumentException( - "There should be no good matches in the list of bad matches"); } } + if (fixBuilder.isEmpty()) { + return Description.NO_MATCH; + } + return buildDescription(tree) .setMessage( "Parameters with incorrectly labelled arguments: " - + incorrectlyLabelledArguments - .stream() - .map(NamedParameterChecker::describeIncorrectlyLabelledArgument) - .collect(Collectors.joining(", "))) + + incorrectParameterDescriptions.build().stream().collect(Collectors.joining(", "))) .addFix(fixBuilder.build()) .build(); } - private static String describeIncorrectlyLabelledArgument(LabelledArgument p) { - switch (p.match()) { - case NOT_ANNOTATED: - case APPROXIMATE_MATCH: - return String.format("%s (missing name label)", p.parameterName()); - case BAD_MATCH: - return String.format("%s (label doesn't match parameter name)", p.parameterName()); - case EXACT_MATCH: - // fall through - } - throw new IllegalArgumentException("Impossible match type in list of bad matches"); - } - - private static boolean containsSyntheticParameterName(List parameters) { - return parameters - .stream() - .map(p -> p.getSimpleName().toString()) - .anyMatch(p -> p.matches("arg[0-9]")); + private static void addComment( + LabelledArgument labelledArgument, SuggestedFix.Builder fixBuilder) { + fixBuilder.prefixWith( + labelledArgument.actualParameter().tree(), + NamedParameterComment.toCommentText(labelledArgument.parameterName())); } + /** + * Replace the given comment with the replacementText. The replacement text is used verbatim and + * so should begin and end with block comment delimiters + */ private static void replaceComment( Comment comment, String replacementText, SuggestedFix.Builder fixBuilder) { int commentStart = comment.getSourcePos(0); @@ -207,6 +173,17 @@ private static void replaceComment( fixBuilder.replace(commentStart, commentEnd, replacementText); } + private static void removeComment(Comment comment, SuggestedFix.Builder fixBuilder) { + replaceComment(comment, "", fixBuilder); + } + + private static boolean containsSyntheticParameterName(List parameters) { + return parameters + .stream() + .map(p -> p.getSimpleName().toString()) + .anyMatch(p -> p.matches("arg[0-9]")); + } + /** * Search all arguments for a target argument which would make a good swap with the source * argument. A good swap would result in the label for the source argument (exactly) matching the @@ -222,11 +199,11 @@ private static Optional findGoodSwap( } boolean sourceLabelMatchesTarget = - NamedParameterComment.match(source.actualParameter(), target.parameterName()) + NamedParameterComment.match(source.actualParameter(), target.parameterName()).matchType() == MatchType.EXACT_MATCH; MatchType targetCommentMatch = - NamedParameterComment.match(target.actualParameter(), source.parameterName()); + NamedParameterComment.match(target.actualParameter(), source.parameterName()).matchType(); boolean targetLabelMatchesSource = targetCommentMatch == MatchType.EXACT_MATCH @@ -239,10 +216,6 @@ private static Optional findGoodSwap( return Optional.empty(); } - private static Matcher hasRequiresNamedParametersAnnotation() { - return hasAnnotation(RequiresNamedParameters.class.getCanonicalName()); - } - /** Information about an argument, the name attached to it with a comment */ @AutoValue abstract static class LabelledArgument { @@ -251,28 +224,7 @@ abstract static class LabelledArgument { abstract Commented actualParameter(); - abstract MatchType match(); - - boolean isCorrectlyAnnotated(boolean commentRequired) { - switch (match()) { - case EXACT_MATCH: - return true; - case BAD_MATCH: - case APPROXIMATE_MATCH: - return false; - case NOT_ANNOTATED: - return !commentRequired; - } - return false; - } - - Optional label() { - return Streams.findLast( - actualParameter() - .beforeComments() - .stream() - .filter(c -> c.getStyle() == CommentStyle.BLOCK)); - } + abstract NamedParameterComment.MatchedComment matchedComment(); static ImmutableList createFromParametersList( List parameters, ImmutableList> actualParameters) { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterComment.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterComment.java index 7476c71e131..c204b6bab72 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterComment.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterComment.java @@ -16,6 +16,8 @@ package com.google.errorprone.bugpatterns.argumentselectiondefects; +import com.google.auto.value.AutoValue; +import com.google.common.base.CharMatcher; import com.google.common.collect.Streams; import com.google.errorprone.util.Commented; import com.google.errorprone.util.Comments; @@ -60,11 +62,71 @@ enum MatchType { NOT_ANNOTATED, } + @AutoValue + abstract static class MatchedComment { + + abstract Comment comment(); + + abstract MatchType matchType(); + + static MatchedComment create(Comment comment, MatchType matchType) { + return new AutoValue_NamedParameterComment_MatchedComment(comment, matchType); + } + + static MatchedComment notAnnotated() { + return new AutoValue_NamedParameterComment_MatchedComment( + new Comment() { + @Override + public String getText() { + throw new IllegalArgumentException( + "Attempt to call getText on comment when in NOT_ANNOTATED state"); + } + + @Override + public int getSourcePos(int i) { + throw new IllegalArgumentException( + "Attempt to call getText on comment when in NOT_ANNOTATED state"); + } + + @Override + public CommentStyle getStyle() { + throw new IllegalArgumentException( + "Attempt to call getText on comment when in NOT_ANNOTATED state"); + } + + @Override + public boolean isDeprecated() { + throw new IllegalArgumentException( + "Attempt to call getText on comment when in NOT_ANNOTATED state"); + } + }, + MatchType.NOT_ANNOTATED); + } + } + + private static boolean isApproximateMatchingComment(Comment comment, String formal) { + switch (comment.getStyle()) { + case BLOCK: + case LINE: + // sometimes people use comments around arguments for higher level structuring - such as + // dividing two separate blocks of arguments. In these cases we want to avoid concluding + // that its a match. Therefore we also check to make sure that the comment is not really + // long and that it doesn't contain acsii-art style markup. + String commentText = Comments.getTextFromComment(comment); + boolean textMatches = Arrays.asList(commentText.split("[^a-zA-Z0-9_]+")).contains(formal); + boolean tooLong = commentText.length() > formal.length() + 5 && commentText.length() > 50; + boolean tooMuchMarkup = CharMatcher.anyOf("-*!@<>").countIn(commentText) > 5; + return textMatches && !tooLong && !tooMuchMarkup; + default: + return false; + } + } + /** * Determine the kind of match we have between the comments on this argument and the formal * parameter name. */ - static MatchType match(Commented actual, String formal) { + static MatchedComment match(Commented actual, String formal) { Optional lastBlockComment = Streams.findLast( actual.beforeComments().stream().filter(c -> c.getStyle() == CommentStyle.BLOCK)); @@ -73,17 +135,22 @@ static MatchType match(Commented actual, String formal) { Matcher m = PARAMETER_COMMENT_PATTERN.matcher(Comments.getTextFromComment(lastBlockComment.get())); if (m.matches()) { - return m.group(1).equals(formal) ? MatchType.EXACT_MATCH : MatchType.BAD_MATCH; + return MatchedComment.create( + lastBlockComment.get(), + m.group(1).equals(formal) ? MatchType.EXACT_MATCH : MatchType.BAD_MATCH); } } - if (Stream.concat(actual.beforeComments().stream(), actual.afterComments().stream()) - .map(Comments::getTextFromComment) - .anyMatch(comment -> Arrays.asList(comment.split("[^a-zA-Z0-9_]+")).contains(formal))) { - return MatchType.APPROXIMATE_MATCH; + Optional approximateMatchComment = + Stream.concat(actual.beforeComments().stream(), actual.afterComments().stream()) + .filter(comment -> isApproximateMatchingComment(comment, formal)) + .findFirst(); + + if (approximateMatchComment.isPresent()) { + return MatchedComment.create(approximateMatchComment.get(), MatchType.APPROXIMATE_MATCH); } - return MatchType.NOT_ANNOTATED; + return MatchedComment.notAnnotated(); } /** @@ -91,7 +158,7 @@ static MatchType match(Commented actual, String formal) { * name. */ static String toCommentText(String formal) { - return String.format("/*%s%s*/", formal, PARAMETER_COMMENT_MARKER); + return String.format("/* %s%s */", formal, PARAMETER_COMMENT_MARKER); } private NamedParameterComment() {} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/inject/InjectedConstructorAnnotations.java b/core/src/main/java/com/google/errorprone/bugpatterns/inject/InjectedConstructorAnnotations.java index cfc2b6e1d99..289349f90a7 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/inject/InjectedConstructorAnnotations.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/inject/InjectedConstructorAnnotations.java @@ -26,6 +26,7 @@ import static com.google.errorprone.matchers.Matchers.hasArgumentWithValue; import static com.google.errorprone.matchers.Matchers.isType; import static com.google.errorprone.matchers.Matchers.methodIsConstructor; +import static com.google.errorprone.matchers.Matchers.symbolHasAnnotation; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; @@ -60,7 +61,7 @@ public class InjectedConstructorAnnotations extends BugChecker implements Method new Matcher() { @Override public boolean matches(AnnotationTree annotationTree, VisitorState state) { - return hasAnnotation(GUICE_BINDING_ANNOTATION) + return symbolHasAnnotation(GUICE_BINDING_ANNOTATION) .matches(annotationTree.getAnnotationType(), state); } }; diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/inject/QualifierWithTypeUse.java b/core/src/main/java/com/google/errorprone/bugpatterns/inject/QualifierWithTypeUse.java index dcd3dfd8a3e..f362b42f4c6 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/inject/QualifierWithTypeUse.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/inject/QualifierWithTypeUse.java @@ -29,6 +29,7 @@ import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; @@ -56,7 +57,8 @@ "Injection frameworks currently don't understand Qualifiers in TYPE_PARAMETER or" + " TYPE_USE contexts.", category = INJECT, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class QualifierWithTypeUse extends BugChecker implements ClassTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/overloading/InconsistentOverloads.java b/core/src/main/java/com/google/errorprone/bugpatterns/overloading/InconsistentOverloads.java new file mode 100644 index 00000000000..809608ba506 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/overloading/InconsistentOverloads.java @@ -0,0 +1,161 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.overloading; + +import static com.google.common.collect.ImmutableList.sortedCopyOf; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static java.util.Comparator.comparingInt; +import static java.util.stream.Collectors.groupingBy; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.Category; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.tree.JCTree; +import java.util.Collection; +import java.util.Comparator; +import java.util.List; +import java.util.Optional; + +/** + * A {@link BugChecker} that detects inconsistently overloaded methods in Java classes. + * + *

The bug checker works in several stages. First, it group the class methods by name. Then each + * group is processed separately (and violations are reported on per-group basis). + * + *

The group is processed by building a {@link ParameterTrie}. This trie is an archetype built + * from methods of lower arity used to determine ordering of methods of higher arity. After ordering + * of arguments of particular method has been determined (using the archetype) it is then added to + * the trie and serves as basis for methods of even higher arity. + * + *

If the determined ordering is different the original parameter ordering a {@link + * ParameterOrderingViolation} is reported. It is essentially a list of expected parameter and list + * of actual parameters then used to build a {@link SuggestedFix} object. + * + * @author hanuszczak@google.com (Łukasz Hanuszczak) + */ +@BugPattern( + name = "InconsistentOverloads", + summary = + "The ordering of parameters in overloaded methods should be as consistent as possible (when" + + " viewed from left to right)", + generateExamplesFromTestCases = false, + category = Category.JDK, + severity = SeverityLevel.WARNING +) +public final class InconsistentOverloads extends BugChecker implements ClassTreeMatcher { + + @Override + public Description matchClass(ClassTree classTree, VisitorState state) { + processClassMethods(getClassTreeMethods(classTree), state); + + /* + * We want to report inconsistencies per method group. There is no "method group" unit in the + * Java AST, so we match on the class, group its methods to method groups and process each group + * separately. + * + * Because of this we return no match for the class itself but we report policy violations for + * each group after it is processed. + */ + return Description.NO_MATCH; + } + + private void processClassMethods(List classMethodTrees, VisitorState state) { + for (List groupMethods : getMethodGroups(classMethodTrees)) { + processGroupMethods(groupMethods, state); + } + } + + private void processGroupMethods(List groupMethodTrees, VisitorState state) { + Preconditions.checkArgument(!groupMethodTrees.isEmpty()); + for (ParameterOrderingViolation violation : getViolations(groupMethodTrees)) { + Description.Builder description = buildDescription(violation.methodTree()); + description.setMessage(violation.getDescription()); + state.reportMatch(description.build()); + } + } + + private static ImmutableList getViolations( + List groupMethodTrees) { + ImmutableList.Builder result = ImmutableList.builder(); + + ParameterTrie trie = new ParameterTrie(); + for (MethodTree methodTree : sortedByArity(groupMethodTrees)) { + Optional violation = trie.extendAndComputeViolation(methodTree); + violation.ifPresent(result::add); + } + + return result.build(); + } + + private static ImmutableList sortedByArity(Iterable methodTrees) { + return sortedCopyOf(comparingArity().thenComparing(comparingPositions()), methodTrees); + } + + private static Comparator comparingPositions() { + return comparingInt(InconsistentOverloads::getStartPosition); + } + + private static Comparator comparingArity() { + return comparingInt(ParameterTrie::getMethodTreeArity); + } + + /** + * Returns a collection of method groups for given list of {@code classMethods}. + * + *

A method group is a list of methods with the same name. + * + *

It is assumed that given {@code classMethods} really do belong to the same class. The + * returned collection does not guarantee any particular group ordering. + */ + private static Collection> getMethodGroups(List classMethods) { + return classMethods.stream().collect(groupingBy(MethodTree::getName)).values(); + } + + /** + * Returns a list of {@link MethodTree} declared in the given {@code classTree}. + * + *

Only method trees that belong to the {@code classTree} are returned, so methods declared in + * nested classes are not going to be considered. + */ + private static ImmutableList getClassTreeMethods(ClassTree classTree) { + List members = classTree.getMembers(); + return members + .stream() + .filter((member) -> member instanceof MethodTree) + .map((member) -> (MethodTree) member) + .collect(toImmutableList()); + } + + /** + * Returns a start position of given {@code tree}. + * + *

The only purpose of this method is to avoid doing a hacky casting to {@link JCTree}. + */ + private static int getStartPosition(Tree tree) { + return ((JCTree) tree).getStartPosition(); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/overloading/ParameterOrderingViolation.java b/core/src/main/java/com/google/errorprone/bugpatterns/overloading/ParameterOrderingViolation.java new file mode 100644 index 00000000000..69dd88d3f86 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/overloading/ParameterOrderingViolation.java @@ -0,0 +1,92 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.overloading; + +import static java.util.stream.Collectors.joining; + +import com.google.auto.value.AutoValue; +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; +import com.google.errorprone.fixes.SuggestedFix; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.VariableTree; + +/** + * A class used to represent ordering violations within a {@link MethodTree}. + * + *

It is a triplet of three things: an original {@link MethodTree}, List<{@link VariableTree}> of + * expected parameters, and a List<{@link VariableTree}> of actual parameters. Violations are + * reported by using {@link ParameterTrie#extendAndComputeViolation(MethodTree)}. + * + * @author hanuszczak@google.com (Łukasz Hanuszczak) + */ +@AutoValue +abstract class ParameterOrderingViolation { + + public abstract MethodTree methodTree(); + + public abstract ImmutableList actual(); + + public abstract ImmutableList expected(); + + @AutoValue.Builder + abstract static class Builder { + + abstract Builder setMethodTree(MethodTree methodTree); + + abstract Builder setActual(ImmutableList actual); + + abstract Builder setExpected(ImmutableList expected); + + abstract ParameterOrderingViolation autoBuild(); + + public ParameterOrderingViolation build() { + ParameterOrderingViolation orderingViolation = autoBuild(); + + int actualParametersCount = orderingViolation.actual().size(); + int expectedParameterCount = orderingViolation.expected().size(); + Preconditions.checkState(actualParametersCount == expectedParameterCount); + + return orderingViolation; + } + } + + public static ParameterOrderingViolation.Builder builder() { + return new AutoValue_ParameterOrderingViolation.Builder(); + } + + /** + * Provides a violation description with suggested parameter ordering. + * + *

An automated {@link SuggestedFix} is not returned with the description because it is not + * safe: reordering the parameters can break the build (if we are lucky) or - in case of + * reordering parameters with the same types - break runtime behaviour of the program. + */ + public String getDescription() { + return "The parameters of this method are inconsistent with other overloaded versions." + + " A consistent order would be: " + + getSuggestedSignature(); + } + + private String getSuggestedSignature() { + return String.format("%s(%s)", methodTree().getName(), getSuggestedParameters()); + } + + private String getSuggestedParameters() { + return expected().stream().map(ParameterTree::toString).collect(joining(", ")); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/overloading/ParameterTree.java b/core/src/main/java/com/google/errorprone/bugpatterns/overloading/ParameterTree.java new file mode 100644 index 00000000000..47e3ce07652 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/overloading/ParameterTree.java @@ -0,0 +1,90 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.overloading; + +import com.google.auto.value.AutoValue; +import com.google.common.base.Preconditions; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Flags; +import java.util.Set; +import javax.lang.model.element.Modifier; +import javax.lang.model.element.Name; + +/** + * A simpler version of standard {@link VariableTree} used to distinguish between real variable + * declarations and method parameters. + * + *

{@link VariableTree} is in a sense "too rich" to represent method parameters. For example, it + * is allowed to have an initializer or arbitrary modifiers. This class is a much simpler version of + * variable tree used to distinguish actual real {@link VariableTree} (variable declarations) from + * simple method parameters. During construction it is validated that the underlying {@link + * VariableTree} can really be used as method parameter. + * + *

It also provides slightly more sensible (for our purposes) {@link Object#toString()} + * implementation. + * + * @author hanuszczak@google.com (Łukasz Hanuszczak) + */ +@AutoValue +abstract class ParameterTree { + + public abstract Name getName(); + + public abstract Tree getType(); + + public abstract boolean isVarArgs(); + + public static ParameterTree create(VariableTree variableTree) { + Preconditions.checkArgument(isValidParameterTree(variableTree)); + + Name name = variableTree.getName(); + Tree type = variableTree.getType(); + boolean isVarargs = isVariableTreeVarArgs(variableTree); + return new AutoValue_ParameterTree(name, type, isVarargs); + } + + private static boolean isValidParameterTree(VariableTree variableTree) { + // A valid parameter has no initializer. + if (variableTree.getInitializer() != null) { + return false; + } + + // A valid parameter either has no modifiers or has only `final` keyword. + Set flags = variableTree.getModifiers().getFlags(); + return flags.isEmpty() || (flags.size() == 1 && flags.contains(Modifier.FINAL)); + } + + @Override + public String toString() { + String type = getType().toString(); + String name = getName().toString(); + + // Hacky fix to improve pretty-printing of varargs (otherwise they are printed as Type[]). + if (isVarArgs()) { + type = type.substring(0, type.length() - 2) + "..."; + } + + return type + " " + name; + } + + // TODO(hanuszczak): This should probably be moved to ASTHelpers. + private static boolean isVariableTreeVarArgs(VariableTree variableTree) { + return (ASTHelpers.getSymbol(variableTree).flags() & Flags.VARARGS) != 0; + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/overloading/ParameterTrie.java b/core/src/main/java/com/google/errorprone/bugpatterns/overloading/ParameterTrie.java new file mode 100644 index 00000000000..2f3e95b024e --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/overloading/ParameterTrie.java @@ -0,0 +1,250 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.overloading; + +import static java.util.Comparator.comparingInt; + +import com.google.auto.value.AutoValue; +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; +import com.sun.source.tree.MethodTree; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.SortedSet; +import java.util.TreeSet; +import javax.lang.model.element.Name; + +/** + * A simple trie implementation. + * + *

Edges between trie nodes represent method parameters and each path represents some method + * signature. This trie is an archetype that next method signatures must follow - we extend the trie + * with new signatures reporting all ordering violations along the way. + * + * @author hanuszczak@google.com (Łukasz Hanuszczak) + */ +class ParameterTrie { + + private final Map children; + + public ParameterTrie() { + this.children = new HashMap<>(); + } + + /** + * Extends the trie with parameters of {@code methodTree}. + * + *

If any ordering {@link ParameterOrderingViolation} is discovered during extension procedure, + * they are reported in the result. + * + *

The method signature is considered to violate the consistency if there exists a path in the + * archetype (the trie) that could be used but would require parameter reordering. The extension + * algorithm simply walks down the trie as long as possible until the signature has parameters + * matching existing edges in the trie. Once this is no longer possible, new nodes are the trie is + * extended with remaining parameters. + */ + public Optional extendAndComputeViolation(MethodTree methodTree) { + return new ParameterTrieExtender(methodTree).execute(this); + } + + /** + * A convenience class used by {@link ParameterTrie#extendAndComputeViolation(MethodTree)} method. + * + *

The extension algorithm is inherently stateful. Because threading three state arguments in + * every recursive function invocation is inconvenient, we use this class to keep track of the + * state in class members. + * + *

The violation detection works like this: first a method signature (i.e. all the method + * parameters) is added to the trie. As long as it is possible the algorithm tries to find a + * parameter that can be followed using existing edges in the trie. When no such parameter is + * found, the trie extension procedure begins where remaining parameters are added to the trie in + * order in which they appear in the initial list. This whole procedure (a path that was followed + * during the process) determines a "correct", "consistent" ordering of the parameters. If the + * original input list of parameters has a different order that the one determined by the + * algorithm a violation is reported. + */ + private static class ParameterTrieExtender { + + private final MethodTree methodTree; + + private final SortedSet inputParameters; + private final List outputParameters; + + public ParameterTrieExtender(MethodTree methodTree) { + this.methodTree = methodTree; + + this.inputParameters = new TreeSet<>(comparingInt(Parameter::position)); + this.outputParameters = new ArrayList<>(); + } + + /** + * Extends given {@code trie} with initial {@link MethodTree}. + * + *

If any {@link ParameterOrderingViolation} is found during the extension procedure, it is + * reported in the result. + */ + public Optional execute(ParameterTrie trie) { + Preconditions.checkArgument(trie != null); + + initialize(); + walk(trie); + return validate(); + } + + /** + * Initializes the input and output parameter collections. + * + *

After initialization, the input parameters should be equal to parameters of the given + * {@link MethodTree} and output parameters should be empty. After processing this should be + * reversed: input parameters list should be empty and output parameters should be an ordered + * version of parameters of the given {@link MethodTree}. + */ + private void initialize() { + for (int i = 0; i < getMethodTreeArity(methodTree); i++) { + inputParameters.add(Parameter.create(methodTree, i)); + } + outputParameters.clear(); + } + + /** + * Processes input parameters by walking along the trie as long as there is a path. + * + *

Once the walk is no longer possible (there is no trie child that matches one of the input + * parameters) the extender begins expansion procedure (see {@link + * ParameterTrieExtender#execute(ParameterTrie)}). + */ + private void walk(ParameterTrie trie) { + Preconditions.checkArgument(trie != null); + + for (Parameter parameter : inputParameters) { + if (parameter.tree().isVarArgs() || !trie.children.containsKey(parameter.name())) { + continue; + } + + inputParameters.remove(parameter); + outputParameters.add(parameter); + walk(trie.children.get(parameter.name())); + return; + } + + // Walking not possible anymore, start expansion. + expand(trie); + } + + /** + * Expands the trie with leftover input parameters. + * + *

It is assumed that given {@code trie} does not have a key corresponding to any input + * parameter which should happen only if {@link ParameterTrieExtender#walk(ParameterTrie)} has + * nowhere else to go. + * + *

Only non-varargs parameters are added to the trie. Any varargs parameters should always be + * placed at the end of the method signature so it makes no sense to include them in the + * archetype. + */ + private void expand(ParameterTrie trie) { + Preconditions.checkArgument(trie != null); + + if (inputParameters.isEmpty()) { + return; + } + + Parameter parameter = inputParameters.first(); + inputParameters.remove(parameter); + outputParameters.add(parameter); + + ParameterTrie allocatedTrie = new ParameterTrie(); + if (!parameter.tree().isVarArgs()) { + trie.children.put(parameter.name(), allocatedTrie); + } + expand(allocatedTrie); + } + + /** + * Reports {@link ParameterOrderingViolation} if output parameters are not ordered as in input + * file. + * + *

This method simply goes through the list of output parameters (input parameters ordered by + * trie traversal algorithm). If a parameter at particular position differs from its original + * position a violation is reported. + * + *

If the ordering is consistent then there is no violation and an empty optional is + * returned. + */ + private Optional validate() { + ImmutableList.Builder actual = ImmutableList.builder(); + ImmutableList.Builder expected = ImmutableList.builder(); + + boolean valid = true; + for (int i = 0; i < outputParameters.size(); i++) { + Parameter parameter = outputParameters.get(i); + if (parameter.position() != i) { + valid = false; + } + + actual.add(parameter.tree()); + expected.add(getParameterTree(parameter.position())); + } + + if (valid) { + return Optional.empty(); + } else { + ParameterOrderingViolation violation = + ParameterOrderingViolation.builder() + .setMethodTree(methodTree) + .setActual(actual.build()) + .setExpected(expected.build()) + .build(); + return Optional.of(violation); + } + } + + /** Returns a {@link ParameterTree} at {@code position} in the associated method. */ + private ParameterTree getParameterTree(int position) { + return ParameterTree.create(methodTree.getParameters().get(position)); + } + } + + /** + * A class used to represent a {@link ParameterTree} (which is essentially just a {@link + * com.sun.source.tree.VariableTree} and its original position within the {@link MethodTree}. + */ + @AutoValue + abstract static class Parameter { + + public abstract ParameterTree tree(); + + public abstract int position(); + + public Name name() { + return tree().getName(); + } + + public static Parameter create(MethodTree methodTree, int position) { + ParameterTree parameterTree = ParameterTree.create(methodTree.getParameters().get(position)); + return new AutoValue_ParameterTrie_Parameter(parameterTree, position); + } + } + + /** Returns arity (number of parameters) of given {@code methodTree}. */ + static int getMethodTreeArity(MethodTree methodTree) { + return methodTree.getParameters().size(); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/AbstractLockMethodChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/AbstractLockMethodChecker.java index cedbdb50fd4..7191d6fa9ff 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/AbstractLockMethodChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/AbstractLockMethodChecker.java @@ -18,7 +18,6 @@ import com.google.common.base.Functions; import com.google.common.base.Joiner; -import com.google.common.base.Optional; import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -31,6 +30,7 @@ import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; import java.util.List; +import java.util.Optional; import java.util.Set; /** @@ -105,7 +105,7 @@ private static Optional> parseLockExpressions( Optional guard = GuardedByBinder.bindString(lockExpression, GuardedBySymbolResolver.from(tree, state)); if (!guard.isPresent()) { - return Optional.absent(); + return Optional.empty(); } builder.add(guard.get()); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/DoubleCheckedLocking.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/DoubleCheckedLocking.java index 65e41e81f8c..2b5c4a10485 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/DoubleCheckedLocking.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/DoubleCheckedLocking.java @@ -22,6 +22,7 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.IfTreeMatcher; @@ -60,7 +61,8 @@ name = "DoubleCheckedLocking", summary = "Double-checked locking on non-volatile fields is unsafe", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class DoubleCheckedLocking extends BugChecker implements IfTreeMatcher { @Override diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinder.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinder.java index 569553d63bb..0291ef07116 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinder.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinder.java @@ -18,7 +18,6 @@ import static com.google.errorprone.bugpatterns.threadsafety.IllegalGuardedBy.checkGuardedBy; -import com.google.common.base.Optional; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.threadsafety.GuardedByExpression.Kind; import com.google.errorprone.util.ASTHelpers; @@ -34,6 +33,7 @@ import com.sun.tools.javac.code.Types; import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.util.Names; +import java.util.Optional; import javax.lang.model.element.Name; /** @@ -45,7 +45,7 @@ public class GuardedByBinder { /** * Creates a {@link GuardedByExpression} from a bound AST node, or returns {@code - * Optional.absent()} if the AST node doesn't correspond to a 'simple' lock expression. + * Optional.empty()} if the AST node doesn't correspond to a 'simple' lock expression. */ public static Optional bindExpression( JCTree.JCExpression exp, VisitorState visitorState) { @@ -59,7 +59,7 @@ public static Optional bindExpression( visitorState.getTypes(), Names.instance(visitorState.context)))); } catch (IllegalGuardedBy expected) { - return Optional.absent(); + return Optional.empty(); } } @@ -75,7 +75,7 @@ static Optional bindString(String string, GuardedBySymbolRe Types.instance(resolver.context()), Names.instance(resolver.context())))); } catch (IllegalGuardedBy expected) { - return Optional.absent(); + return Optional.empty(); } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByUtils.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByUtils.java index 632d3011a37..1e8ec934fac 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByUtils.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByUtils.java @@ -16,37 +16,46 @@ package com.google.errorprone.bugpatterns.threadsafety; +import static com.google.errorprone.util.ASTHelpers.getSymbol; + import com.google.auto.value.AutoValue; -import com.google.common.base.Optional; import com.google.errorprone.VisitorState; -import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Attribute; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.parser.JavacParser; import com.sun.tools.javac.parser.ParserFactory; import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.util.Context; +import java.util.Optional; +import javax.lang.model.util.SimpleAnnotationValueVisitor8; /** @author cushon@google.com (Liam Miller-Cushon) */ public class GuardedByUtils { - static String getGuardValue(Tree tree) { - { - net.jcip.annotations.GuardedBy guardedBy = - ASTHelpers.getAnnotation(tree, net.jcip.annotations.GuardedBy.class); - if (guardedBy != null) { - return guardedBy.value(); - } - } - - { - javax.annotation.concurrent.GuardedBy guardedBy = - ASTHelpers.getAnnotation(tree, javax.annotation.concurrent.GuardedBy.class); - if (guardedBy != null) { - return guardedBy.value(); - } + static String getGuardValue(Tree tree, VisitorState state) { + Symbol sym = getSymbol(tree); + if (sym == null) { + return null; } + return sym.getRawAttributes() + .stream() + .filter(a -> a.getAnnotationType().asElement().getSimpleName().contentEquals("GuardedBy")) + .findFirst() + .flatMap( + a -> Optional.ofNullable(a.member(state.getName("value"))).flatMap(v -> asString(v))) + .orElse(null); + } - return null; + private static Optional asString(Attribute v) { + return Optional.ofNullable( + v.accept( + new SimpleAnnotationValueVisitor8() { + @Override + public String visitString(String s, Void aVoid) { + return s; + } + }, + null)); } public static JCTree.JCExpression parseString(String guardedByString, Context context) { @@ -81,7 +90,7 @@ static GuardedByValidationResult ok() { } public static GuardedByValidationResult isGuardedByValid(Tree tree, VisitorState state) { - String guard = GuardedByUtils.getGuardValue(tree); + String guard = GuardedByUtils.getGuardValue(tree, state); if (guard == null) { return GuardedByValidationResult.ok(); } @@ -92,7 +101,7 @@ public static GuardedByValidationResult isGuardedByValid(Tree tree, VisitorState return GuardedByValidationResult.invalid("could not resolve guard"); } - Symbol treeSym = ASTHelpers.getSymbol(tree); + Symbol treeSym = getSymbol(tree); if (treeSym == null) { // this shouldn't happen unless the compilation had already failed. return GuardedByValidationResult.ok(); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzer.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzer.java index df204522e7d..875c8f18773 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzer.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/HeldLockAnalyzer.java @@ -20,7 +20,6 @@ import com.google.auto.value.AutoValue; import com.google.common.base.Function; -import com.google.common.base.Optional; import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; @@ -53,6 +52,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Optional; import java.util.Set; import javax.lang.model.element.Modifier; @@ -142,7 +142,7 @@ public Void visitMethod(MethodTree tree, HeldLockSet locks) { // @GuardedBy annotations on methods are trusted for declarations, and checked // for invocations. - String guard = GuardedByUtils.getGuardValue(tree); + String guard = GuardedByUtils.getGuardValue(tree, visitorState); if (guard != null) { Optional bound = GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, visitorState)); @@ -221,7 +221,7 @@ public Void visitVariable(VariableTree node, HeldLockSet locks) { } private void checkMatch(ExpressionTree tree, HeldLockSet locks) { - String guardString = GuardedByUtils.getGuardValue(tree); + String guardString = GuardedByUtils.getGuardValue(tree, visitorState); if (guardString == null) { return; } @@ -456,7 +456,7 @@ static Optional from( GuardedByBinder.bindExpression(guardedMemberExpression, state); if (!guardedMember.isPresent()) { - return Optional.absent(); + return Optional.empty(); } GuardedByExpression memberBase = ((GuardedByExpression.Select) guardedMember.get()).base(); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableAnnotationChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableAnnotationChecker.java index e3c1f53af1c..9c7ba0b831e 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableAnnotationChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableAnnotationChecker.java @@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; import com.google.errorprone.annotations.Immutable; @@ -44,7 +45,8 @@ altNames = "Immutable", category = JDK, summary = "Annotations should always be immutable", - severity = WARNING + severity = WARNING, + tags = StandardTags.LIKELY_ERROR ) public class ImmutableAnnotationChecker extends BugChecker implements ClassTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/StaticGuardedByInstance.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/StaticGuardedByInstance.java index e74bab066a8..236ca04c563 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/StaticGuardedByInstance.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/StaticGuardedByInstance.java @@ -21,6 +21,7 @@ import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.Category; import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.SynchronizedTreeMatcher; @@ -45,11 +46,12 @@ name = "StaticGuardedByInstance", category = Category.JDK, summary = "Writes to static fields should not be guarded by instance locks", - severity = SeverityLevel.WARNING + severity = SeverityLevel.WARNING, + tags = StandardTags.FRAGILE_CODE ) public class StaticGuardedByInstance extends BugChecker implements SynchronizedTreeMatcher { - public static final String MESSAGE = + private static final String MESSAGE = "Write to static variable should not be guarded by instance lock '%s'"; @Override diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/SynchronizeOnNonFinalField.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/SynchronizeOnNonFinalField.java index a0be05dd4a0..7262154ff3b 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/SynchronizeOnNonFinalField.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/SynchronizeOnNonFinalField.java @@ -20,6 +20,7 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.matchers.Description; @@ -43,7 +44,8 @@ + "* If the field needs to be mutable, create a separate lock by adding a private" + " final field and synchronizing on it to guard all accesses.", category = JDK, - severity = WARNING + severity = WARNING, + tags = StandardTags.FRAGILE_CODE ) public class SynchronizeOnNonFinalField extends BugChecker implements BugChecker.SynchronizedTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownMutability.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownMutability.java index 75cafb83cd9..e936a18a9e6 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownMutability.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownMutability.java @@ -164,10 +164,14 @@ private static ImmutableMap buildImmutableClass .add(java.math.BigInteger.class) .add(java.net.InetAddress.class) .add(java.net.URI.class) + .add(java.util.UUID.class) .add(java.util.Locale.class) .add(java.util.regex.Pattern.class) .add("android.net.Uri") .add("java.util.Optional", "T") + .add("java.util.OptionalDouble") + .add("java.util.OptionalInt") + .add("java.util.OptionalLong") .add("java.time.Duration") .add("java.time.Instant") .add("java.time.LocalDate") diff --git a/core/src/main/java/com/google/errorprone/refaster/BlockTemplate.java b/core/src/main/java/com/google/errorprone/refaster/BlockTemplate.java index fa6cbb40d14..440edb64c47 100644 --- a/core/src/main/java/com/google/errorprone/refaster/BlockTemplate.java +++ b/core/src/main/java/com/google/errorprone/refaster/BlockTemplate.java @@ -139,6 +139,13 @@ public Choice> apply(UnifierWithUnconsumedStatements st BlockTemplateMatch match = new BlockTemplateMatch( block, checkedUnifier.get(), offset, offset + consumedStatements); + boolean verified = + ExpressionTemplate.trueOrNull( + ExpressionTemplate.PLACEHOLDER_VERIFIER.scan( + templateStatements(), checkedUnifier.get())); + if (!verified) { + return Choice.none(); + } return matchesStartingAnywhere( block, offset + consumedStatements, diff --git a/core/src/main/java/com/google/errorprone/refaster/ExpressionTemplate.java b/core/src/main/java/com/google/errorprone/refaster/ExpressionTemplate.java index d02bd255778..be5a912cadc 100644 --- a/core/src/main/java/com/google/errorprone/refaster/ExpressionTemplate.java +++ b/core/src/main/java/com/google/errorprone/refaster/ExpressionTemplate.java @@ -30,7 +30,9 @@ import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.refaster.annotation.AlsoNegation; import com.google.errorprone.refaster.annotation.UseImportPolicy; +import com.sun.source.tree.Tree; import com.sun.source.tree.Tree.Kind; +import com.sun.source.util.TreeScanner; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.TypeTag; import com.sun.tools.javac.code.Types; @@ -55,6 +57,7 @@ import java.lang.annotation.Annotation; import java.util.Map; import java.util.logging.Logger; +import javax.annotation.Nullable; /** * Implementation of a template to match and replace an expression anywhere in an AST. @@ -130,10 +133,39 @@ public Iterable match(JCTree target, Context context) { return ImmutableList.of(); } + static boolean trueOrNull(@Nullable Boolean b) { + return b == null || b; + } + + /** + * Placeholders' verification step only checks that they use variables that haven't *yet* been + * matched to another local variable. This scanner reruns the verification step for the whole + * tree, returning false if a violation was found, and true or null otherwise. + */ + static final TreeScanner PLACEHOLDER_VERIFIER = + new TreeScanner() { + @Override + public Boolean reduce(Boolean a, Boolean b) { + return trueOrNull(a) && trueOrNull(b); + } + + @Override + public Boolean visitOther(Tree t, Unifier u) { + if (t instanceof UPlaceholderExpression) { + return ((UPlaceholderExpression) t).reverify(u); + } else if (t instanceof UPlaceholderStatement) { + return ((UPlaceholderStatement) t).reverify(u); + } else { + return super.visitOther(t, u); + } + } + }; + @Override public Choice unify(final JCExpression target, Unifier unifier) { return expression() .unify(target, unifier) + .condition(u -> trueOrNull(PLACEHOLDER_VERIFIER.scan(expression(), u))) .thenOption( new Function>() { diff --git a/core/src/main/java/com/google/errorprone/refaster/PlaceholderUnificationVisitor.java b/core/src/main/java/com/google/errorprone/refaster/PlaceholderUnificationVisitor.java index 6eee7ee2bb1..255de227ad3 100644 --- a/core/src/main/java/com/google/errorprone/refaster/PlaceholderUnificationVisitor.java +++ b/core/src/main/java/com/google/errorprone/refaster/PlaceholderUnificationVisitor.java @@ -239,13 +239,10 @@ public State> apply(State> state) { } static boolean equivalentExprs(Unifier unifier, JCExpression expr1, JCExpression expr2) { - try { - return Types.instance(unifier.getContext()).isSameType(expr2.type, expr1.type) - && expr2.toString().equals(expr1.toString()); - } catch (NullPointerException e) { - throw new RuntimeException( - "Types are: " + expr2 + " " + expr2.type + " and " + expr1 + " " + expr1.type, e); - } + return expr1.type != null + && expr2.type != null + && Types.instance(unifier.getContext()).isSameType(expr2.type, expr1.type) + && expr2.toString().equals(expr1.toString()); } /** diff --git a/core/src/main/java/com/google/errorprone/refaster/ULocalVarIdent.java b/core/src/main/java/com/google/errorprone/refaster/ULocalVarIdent.java index a6f177b54da..bfba876d571 100644 --- a/core/src/main/java/com/google/errorprone/refaster/ULocalVarIdent.java +++ b/core/src/main/java/com/google/errorprone/refaster/ULocalVarIdent.java @@ -21,6 +21,7 @@ import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.IdentifierTree; import com.sun.tools.javac.tree.JCTree.JCIdent; +import java.util.Objects; /** * Identifier corresponding to a template local variable. @@ -51,7 +52,8 @@ private Key key() { public Choice visitIdentifier(IdentifierTree ident, Unifier unifier) { LocalVarBinding binding = unifier.getBinding(key()); return Choice.condition( - binding != null && ASTHelpers.getSymbol(ident).equals(binding.getSymbol()), unifier); + binding != null && Objects.equals(ASTHelpers.getSymbol(ident), binding.getSymbol()), + unifier); } @Override diff --git a/core/src/main/java/com/google/errorprone/refaster/UMethodDecl.java b/core/src/main/java/com/google/errorprone/refaster/UMethodDecl.java index de957a6fadf..982da7b09bc 100644 --- a/core/src/main/java/com/google/errorprone/refaster/UMethodDecl.java +++ b/core/src/main/java/com/google/errorprone/refaster/UMethodDecl.java @@ -120,7 +120,6 @@ public ImmutableList getTypeParameters() { @Override public VariableTree getReceiverParameter() { - // TODO(cushon): java 8. - throw new IllegalStateException(); + return null; } } diff --git a/core/src/main/java/com/google/errorprone/refaster/UNewArray.java b/core/src/main/java/com/google/errorprone/refaster/UNewArray.java index 05ca8e33979..62a1692a095 100644 --- a/core/src/main/java/com/google/errorprone/refaster/UNewArray.java +++ b/core/src/main/java/com/google/errorprone/refaster/UNewArray.java @@ -92,13 +92,11 @@ public JCNewArray inline(Inliner inliner) throws CouldNotResolveImportException @Override public List getAnnotations() { - // TODO(cushon): java 8. - throw new IllegalStateException(); + return ImmutableList.of(); } @Override public List> getDimAnnotations() { - // TODO(cushon): java 8. - throw new IllegalStateException(); + return ImmutableList.of(); } } diff --git a/core/src/main/java/com/google/errorprone/refaster/UPlaceholderExpression.java b/core/src/main/java/com/google/errorprone/refaster/UPlaceholderExpression.java index 3a0cea50564..9ec5fffd525 100644 --- a/core/src/main/java/com/google/errorprone/refaster/UPlaceholderExpression.java +++ b/core/src/main/java/com/google/errorprone/refaster/UPlaceholderExpression.java @@ -19,6 +19,7 @@ import com.google.auto.value.AutoValue; import com.google.common.base.Function; import com.google.common.base.Functions; +import com.google.common.base.MoreObjects; import com.google.common.base.Optional; import com.google.common.collect.Collections2; import com.google.common.collect.ImmutableList; @@ -135,6 +136,16 @@ public R accept(TreeVisitor visitor, D data) { return visitor.visitOther(this, data); } + public boolean reverify(Unifier unifier) { + return MoreObjects.firstNonNull( + new PlaceholderVerificationVisitor( + Collections2.transform( + placeholder().requiredParameters(), Functions.forMap(arguments())), + arguments().values()) + .scan(unifier.getBinding(placeholder().exprKey()), unifier), + true); + } + @Override protected Choice defaultAction(Tree node, Unifier unifier) { // for now we only match JCExpressions diff --git a/core/src/main/java/com/google/errorprone/refaster/UPlaceholderStatement.java b/core/src/main/java/com/google/errorprone/refaster/UPlaceholderStatement.java index 52ec9cdfdcc..0a806e5fbd0 100644 --- a/core/src/main/java/com/google/errorprone/refaster/UPlaceholderStatement.java +++ b/core/src/main/java/com/google/errorprone/refaster/UPlaceholderStatement.java @@ -19,6 +19,7 @@ import com.google.auto.value.AutoValue; import com.google.common.base.Function; import com.google.common.base.Functions; +import com.google.common.base.MoreObjects; import com.google.common.base.Optional; import com.google.common.collect.Collections2; import com.google.common.collect.ImmutableList; @@ -85,6 +86,16 @@ ConsumptionState consume(JCStatement impl) { } } + public boolean reverify(Unifier unifier) { + return MoreObjects.firstNonNull( + new PlaceholderVerificationVisitor( + Collections2.transform( + placeholder().requiredParameters(), Functions.forMap(arguments())), + arguments().values()) + .scan(unifier.getBinding(placeholder().blockKey()), unifier), + true); + } + @Override public Choice apply( final UnifierWithUnconsumedStatements initState) { diff --git a/core/src/main/java/com/google/errorprone/refaster/UTemplater.java b/core/src/main/java/com/google/errorprone/refaster/UTemplater.java index 225a09c3842..e3912c9cba2 100644 --- a/core/src/main/java/com/google/errorprone/refaster/UTemplater.java +++ b/core/src/main/java/com/google/errorprone/refaster/UTemplater.java @@ -458,7 +458,9 @@ public UExpression visitMethodInvocation(MethodInvocationTree tree, Void v) { template(strArg)); } else if (anyMatch(AS_VARARGS, tree.getMethodSelect(), new Unifier(context))) { ExpressionTree arg = Iterables.getOnlyElement(tree.getArguments()); - checkArgument(ASTHelpers.hasAnnotation(arg, Repeated.class, new VisitorState(context))); + checkArgument( + ASTHelpers.hasAnnotation( + ASTHelpers.getSymbol(arg), Repeated.class, new VisitorState(context))); return template(arg); } Map placeholderMethods = diff --git a/core/src/main/java/com/google/errorprone/refaster/UVariableDecl.java b/core/src/main/java/com/google/errorprone/refaster/UVariableDecl.java index 7745a2a2c5c..ca4c5227d9e 100644 --- a/core/src/main/java/com/google/errorprone/refaster/UVariableDecl.java +++ b/core/src/main/java/com/google/errorprone/refaster/UVariableDecl.java @@ -128,12 +128,11 @@ public R accept(TreeVisitor visitor, D data) { @Override public ModifiersTree getModifiers() { - throw new UnsupportedOperationException(); + return null; } @Override public ExpressionTree getNameExpression() { - // TODO(cushon): java 8. - throw new IllegalStateException(); + return null; } } diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 3b15636aba6..5f00a5a0502 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -226,6 +226,7 @@ import com.google.errorprone.bugpatterns.inject.guice.OverridesGuiceInjectableMethod; import com.google.errorprone.bugpatterns.inject.guice.OverridesJavaxInjectableMethod; import com.google.errorprone.bugpatterns.inject.guice.ProvidesMethodOutsideOfModule; +import com.google.errorprone.bugpatterns.overloading.InconsistentOverloads; import com.google.errorprone.bugpatterns.threadsafety.DoubleCheckedLocking; import com.google.errorprone.bugpatterns.threadsafety.GuardedByChecker; import com.google.errorprone.bugpatterns.threadsafety.ImmutableAnnotationChecker; @@ -462,6 +463,7 @@ public static ScannerSupplier errorChecks() { EmptyTopLevelDeclaration.class, ExpectedExceptionChecker.class, HardCodedSdCardPath.class, + InconsistentOverloads.class, InjectedConstructorAnnotations.class, InsecureCipherMode.class, InvalidTargetingOnScopingAnnotation.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClashTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClashTest.java index b5e3a08c650..ae564a14cf7 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClashTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClashTest.java @@ -220,4 +220,46 @@ public void suppressWarningsOnMethod() { "}") .doTest(); } + + @Test + public void shouldIgnore_transitiveInheritanceWithExpandedVisibility() { + testHelper + .addSourceLines( + "pkg1/FunctionalInterface.java", + "package pkg1;", + "public interface FunctionalInterface {", + " String apply(String s);", + "}") + .addSourceLines( + "pkg2/BaseClass.java", + "package pkg2;", + "import pkg1.FunctionalInterface;", + "public abstract class BaseClass {", + " abstract String doIt(FunctionalInterface fi);", + "}") + .addSourceLines( + "pkg2/DerivedClass.java", + "package pkg2;", + "import pkg1.FunctionalInterface;", + "public class DerivedClass extends BaseClass {", + " @Override public String doIt(FunctionalInterface fi) {", + " return null;", + " }", + "}") + .addSourceLines( + "pkg3/Test.java", + "package pkg3;", + "import pkg1.FunctionalInterface;", + "import pkg2.DerivedClass;", + "public class Test {", + " DerivedClass getDerivedClass() {", + " return new DerivedClass() {", + " @Override public String doIt(FunctionalInterface fi) {", + " return null;", + " }", + " };", + " }", + "}") + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ShortCircuitBooleanTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ShortCircuitBooleanTest.java index 8e38781a359..076cc90b227 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ShortCircuitBooleanTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ShortCircuitBooleanTest.java @@ -31,13 +31,28 @@ public void positive() { .addSourceLines( "Test.java", "class Test {", - " void f(boolean b1, boolean b2, int i1, int i2) {", + " void f(boolean b1, boolean b2, boolean b3, int i1, int i2) {", " // BUG: Diagnostic contains: b1 && b2", " boolean r = b1 & b2;", " // BUG: Diagnostic contains: b1 || b2", " r = b1 | b2;", + " // BUG: Diagnostic contains: b1 || !b2 || b3 && b1 && b2", + " r = b1 | !b2 | b3 & b1 & b2;", + " // BUG: Diagnostic contains: b1 || (b2 && b3) && (b1 || b2)", + " r = b1 | (b2 & b3) & (b1 | b2);", + " // BUG: Diagnostic contains: b1 || (b2 && b3)", + " r = b1 | (b2 & b3);", + " // BUG: Diagnostic contains: (b1 || b2) && b3", + " r = (b1 | b2) & b3;", + " // BUG: Diagnostic contains: b1 || b3 && b3", + " r = b1 | b3 && b3;", + " // BUG: Diagnostic contains: b1 || b2 == b3", + " r = b1 | b2 == b3;", + " // BUG: Diagnostic contains: (b1 || b2) != b3", + " r = (b1 | b2) != b3;", " int i = i1 | i2;", " i = i1 & i2;", + " i = i1 & i2 | i1;", " }", "}") .doTest(); diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ShouldHaveEvenArgsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ShouldHaveEvenArgsTest.java index 5ac9adfa113..f59fcd52d4d 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ShouldHaveEvenArgsTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ShouldHaveEvenArgsTest.java @@ -18,6 +18,7 @@ import com.google.errorprone.CompilationTestHelper; import org.junit.Before; +import org.junit.Ignore; // NOLINT import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -45,4 +46,16 @@ public void testPositiveCase() throws Exception { public void testNegativeCase() throws Exception { compilationHelper.addSourceFile("ShouldHaveEvenArgsNegativeCases.java").doTest(); } + + @Ignore("Public truth doesn't contain this method") + @Test + public void testPositiveCase_multimap() throws Exception { + compilationHelper.addSourceFile("ShouldHaveEvenArgsMultimapPositiveCases.java").doTest(); + } + + @Ignore("Public truth doesn't contain this method") + @Test + public void testNegativeCase_multimap() throws Exception { + compilationHelper.addSourceFile("ShouldHaveEvenArgsMultimapNegativeCases.java").doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectCheckerTest.java index 3e74028c257..13cdb709750 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectCheckerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectCheckerTest.java @@ -67,7 +67,7 @@ public void argumentSelectionDefectChecker_findsSwap_withSwappedMatchingPair() { " abstract void target(Object first, Object second);", " void test(Object first, Object second) {", " // BUG: Diagnostic contains: target(first, second)", - " // target(/*first=*/second, /*second=*/first)", + " // target(/* first= */second, /* second= */first)", " target(second, first);", " }", "}") @@ -84,7 +84,7 @@ public void argumentSelectionDefectChecker_findsSwap_withSwappedMatchingPairWith " abstract Object getSecond();", " void test(Object first) {", " // BUG: Diagnostic contains: target(first, getSecond())", - " // target(/*first=*/getSecond(), /*second=*/first)", + " // target(/* first= */getSecond(), /* second= */first)", " target(getSecond(), first);", " }", "}") @@ -100,7 +100,7 @@ public void argumentSelectionDefectChecker_findsSwap_withOneNullArgument() { " abstract void target(Object first, Object second);", " void test(Object second) {", " // BUG: Diagnostic contains: target(null, second)", - " // target(/*first=*/second, /*second=*/null)", + " // target(/* first= */second, /* second= */null)", " target(second, null);", " }", "}") @@ -130,7 +130,7 @@ public void argumentSelectionDefectChecker_commentsOnlyOnSwappedPair_withThreeAr " abstract void target(Object first, Object second, Object third);", " void test(Object first, Object second, Object third) {", " // BUG: Diagnostic contains: target(first, second, third)", - " // target(/*first=*/second, /*second=*/first, third)", + " // target(/* first= */second, /* second= */first, third)", " target(second, first, third);", " }", "}") @@ -235,7 +235,7 @@ public void argumentSelectionDefectCheckerWithPenalty_findsSwap_withSwappedMatch " abstract void target(Object first, Object second);", " void test(Object first, Object second) {", " // BUG: Diagnostic contains: target(first, second)", - " // target(/*first=*/second, /*second=*/first)", + " // target(/* first= */second, /* second= */first)", " target(second, first);", " }", "}") @@ -288,7 +288,7 @@ ArgumentSelectionDefectWithNameInCommentsHeuristic.class, getClass()) "abstract class Test {", " abstract void target(Object first, Object second);", " void test(Object first, Object second) {", - " target(/*first=*/second, /*second=*/first);", + " target(/* first= */second, /* second= */first);", " }", "}") .doTest(); diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AssertEqualsArgumentOrderCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AssertEqualsArgumentOrderCheckerTest.java index d04de7fc78a..c22059c67e3 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AssertEqualsArgumentOrderCheckerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AssertEqualsArgumentOrderCheckerTest.java @@ -72,7 +72,7 @@ public void assertEqualsCheck_swapsArguments_withOrderActualExpected() throws Ex " static void assertEquals(Object expected, Object actual) {};", " void test(Object expected, Object actual) {", " // BUG: Diagnostic contains: assertEquals(expected, actual)", - " // assertEquals(/*expected=*/actual, /*actual=*/expected)", + " // assertEquals(/* expected= */actual, /* actual= */expected)", " assertEquals(actual, expected);", " }", "}") @@ -242,7 +242,7 @@ public void assertEqualsCheck_makesNoChange_withCommentedNames() throws Exceptio "abstract class ErrorProneTest {", " static void assertEquals(Object expected, Object actual) {};", " void test(Object expected, Object actual) {", - " assertEquals(/*expected=*/actual, /*actual=*/expected);", + " assertEquals(/* expected= */actual, /* actual= */expected);", " }", "}") .doTest(); diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristicTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristicTest.java index 45ec06ebfe4..d043df26f45 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristicTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristicTest.java @@ -99,7 +99,7 @@ public void nameInCommentHeuristic_returnsTrue_whereCommentMatchesFormalParamete " abstract void target(Object first);", " void test(Object first) {", " // BUG: Diagnostic contains: true", - " target(/*first=*/ first);", + " target(/*first= */ first);", " }", "}") .doTest(); diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterCheckerTest.java index bd4b4a51d4e..8163916e13e 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterCheckerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterCheckerTest.java @@ -55,8 +55,8 @@ public void namedParametersChecker_findsError_withOneBadComment() { "abstract class Test {", " abstract void target(Object param1, Object param2);", " void test(Object arg1, Object arg2) {", - " // BUG: Diagnostic contains: target(arg2, /*param2=*/arg1)", - " target(/*param2=*/arg1, arg2);", + " // BUG: Diagnostic contains: target(arg2, /* param2= */arg1)", + " target(/* param2= */arg1, arg2);", " }", "}") .doTest(); @@ -70,8 +70,8 @@ public void namedParametersChecker_suggestsSwap_withSwappedArgs() { "abstract class Test {", " abstract void target(Object param1, Object param2);", " void test(Object arg1, Object arg2) {", - " // BUG: Diagnostic contains: target(/*param1=*/arg1, /*param2=*/arg2)", - " target(/*param2=*/arg2, /*param1=*/arg1);", + " // BUG: Diagnostic contains: target(/* param1= */arg1, /* param2= */arg2)", + " target(/* param2= */arg2, /* param1= */arg1);", " }", "}") .doTest(); @@ -85,73 +85,112 @@ public void namedParametersChecker_suggestsSwap_withOneCommentedSwappedArgs() { "abstract class Test {", " abstract void target(Object param1, Object param2);", " void test(Object arg1, Object arg2) {", - " // BUG: Diagnostic contains: target(arg1, /*param2=*/arg2)", - " target(/*param2=*/arg2, arg1);", + " // BUG: Diagnostic contains: target(arg1, /* param2= */arg2)", + " target(/* param2= */arg2, arg1);", " }", "}") .doTest(); } @Test - public void namedParametersChecker_suggestsComments_onRequiredNamesMethod() { + public void namedParametersChecker_reformatsComment_onRequiredNamesMethod() { compilationHelper .addSourceLines( "Test.java", - "import com.google.errorprone.annotations.RequiresNamedParameters;", "abstract class Test {", - " @RequiresNamedParameters", - " abstract void target(Object param1, Object param2);", - " void test(Object arg1, Object arg2) {", - " // BUG: Diagnostic contains: target(/*param1=*/arg1, /*param2=*/arg2)", - " target(arg1, arg2);", + " abstract void target(Object param);", + " void test(Object arg) {", + " // BUG: Diagnostic contains: target(/* param= */arg)", + " target(/*note param = */arg);", " }", "}") .doTest(); } @Test - public void namedParametersChecker_ignoresCall_onRequiredNamesMethodWithComments() { + public void namedParametersChecker_reformatsComment_withNoEquals() { compilationHelper .addSourceLines( "Test.java", - "import com.google.errorprone.annotations.RequiresNamedParameters;", "abstract class Test {", - " @RequiresNamedParameters", - " abstract void target(Object param1, Object param2);", - " void test(Object arg1, Object arg2) {", - " target(/*param1=*/arg1, /*param2=*/arg2);", + " abstract void target(Object param);", + " void test(Object arg) {", + " // BUG: Diagnostic contains: target(/* param= */arg)", + " target(/*param*/arg);", " }", "}") .doTest(); } @Test - public void namedParametersChecker_suggestsCommentsAndSwaps_onRequiredNamesMethod() { + public void namedParametersChecker_reformatsComment_blockAfter() { compilationHelper .addSourceLines( "Test.java", - "import com.google.errorprone.annotations.RequiresNamedParameters;", "abstract class Test {", - " @RequiresNamedParameters", - " abstract void target(Object p1, Object p2, Object p3);", - " void test(Object arg1, Object arg2, Object arg3) {", - " // BUG: Diagnostic contains: target(/*p1=*/arg1, /*p2=*/arg2, /*p3=*/arg3)", - " target(/*p3=*/arg3, arg2, /*p1=*/arg1);", + " abstract void target(Object param);", + " void test(Object arg) {", + " // BUG: Diagnostic contains: target(/* param= */arg)", + " target(arg/*param*/);", " }", "}") .doTest(); } @Test - public void namedParametersChecker_doesNotMatchComment_withSpacesAndExtraText() { + public void namedParametersChecker_reformatsMatchingComment_lineAfter() { compilationHelper .addSourceLines( "Test.java", "abstract class Test {", " abstract void target(Object param);", " void test(Object arg) {", - " // BUG: Diagnostic contains: target(/*param=*/arg)", - " target(/*note param = */arg);", + " // BUG: Diagnostic contains: target(/* param= */arg)", + " target(arg); //param", + " }", + "}") + .doTest(); + } + + @Test + public void namedParametersChecker_ignoresComment_nonMatchinglineAfter() { + compilationHelper + .addSourceLines( + "Test.java", + "abstract class Test {", + " abstract void target(Object param);", + " void test(Object arg) {", + " target(arg); // some_other_comment", + " }", + "}") + .doTest(); + } + + @Test + public void namedParametersChecker_ignoresComment_markedUpDelimiter() { + compilationHelper + .addSourceLines( + "Test.java", + "abstract class Test {", + " abstract void target(Object param1, Object param2);", + " void test(Object arg1, Object arg2) {", + " target(arg1,", + " /* ---- param1 <-> param2 ---- */", + " arg2);", + " }", + "}") + .doTest(); + } + + @Test + public void namedParametersChecker_ignoresComment_wrongNameWithNoEquals() { + compilationHelper + .addSourceLines( + "Test.java", + "abstract class Test {", + " abstract void target(Object param);", + " void test(Object arg) {", + " target(/* some_other_comment */arg);", " }", "}") .doTest(); @@ -166,7 +205,7 @@ public void namedParametersChecker_matchesComment_withChainedMethod() { " abstract Test getTest(Object param);", " abstract void target(Object param2);", " void test(Object arg, Object arg2) {", - " getTest(/*param=*/arg).target(arg2);", + " getTest(/* param= */arg).target(arg2);", " }", "}") .doTest(); @@ -180,8 +219,8 @@ public void namedParametersChecker_suggestsChangeComment_whenNoMatchingNames() { "abstract class Test {", " abstract void target(Object param1, Object param2);", " void test(Object arg1, Object arg2) {", - " // BUG: Diagnostic contains: target(/*param1=*/arg1, arg2)", - " target(/*parm1=*/arg1, arg2);", + " // BUG: Diagnostic contains: target(/* param1= */arg1, arg2)", + " target(/* notMatching= */arg1, arg2);", " }", "}") .doTest(); diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/overloading/InconsistentOverloadsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/InconsistentOverloadsTest.java new file mode 100644 index 00000000000..651570fd375 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/InconsistentOverloadsTest.java @@ -0,0 +1,76 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.overloading; + +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.bugpatterns.BugChecker; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** + * Test cases for {@link InconsistentOverloads}. + * + * @author hanuszczak@google.com (Łukasz Hanuszczak) + */ +@RunWith(JUnit4.class) +public final class InconsistentOverloadsTest { + + private CompilationTestHelper compilationHelper; + + @Before + public void createCompilationHelper() { + Class bugCheckerClass = InconsistentOverloads.class; + compilationHelper = CompilationTestHelper.newInstance(bugCheckerClass, getClass()); + } + + @Test + public void inconsistentOverloadsNegativeCases() { + compilationHelper.addSourceFile("InconsistentOverloadsNegativeCases.java").doTest(); + } + + @Test + public void inconsistentOverloadsPositiveCasesAnnotations() { + compilationHelper.addSourceFile("InconsistentOverloadsPositiveCasesAnnotations.java").doTest(); + } + + @Test + public void inconsistentOverloadsPositiveCasesGeneral() { + compilationHelper.addSourceFile("InconsistentOverloadsPositiveCasesGeneral.java").doTest(); + } + + @Test + public void inconsistentOverloadsPositiveCasesGenerics() { + compilationHelper.addSourceFile("InconsistentOverloadsPositiveCasesGenerics.java").doTest(); + } + + @Test + public void inconsistentOverloadsPositiveCasesInterleaved() { + compilationHelper.addSourceFile("InconsistentOverloadsPositiveCasesInterleaved.java").doTest(); + } + + @Test + public void inconsistentOverloadsPositiveCasesSimple() { + compilationHelper.addSourceFile("InconsistentOverloadsPositiveCasesSimple.java").doTest(); + } + + @Test + public void inconsistentOverloadsPositiveCasesVarargs() { + compilationHelper.addSourceFile("InconsistentOverloadsPositiveCasesVarargs.java").doTest(); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsNegativeCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsNegativeCases.java new file mode 100644 index 00000000000..aef43d8e079 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsNegativeCases.java @@ -0,0 +1,46 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.overloading.testdata; + +public final class InconsistentOverloadsNegativeCases { + + public void foo(Object object) {} + + public void foo(Object object, int x, int y) {} + + public void foo(Object object, int x, int y, String string) {} + + public void bar(int x, int y, int z) {} + + public void bar(int x) {} + + public void bar(int x, int y) {} + + public void baz(String string) {} + + public void baz(int x, int y, String otherString) {} + + public void baz(int x, int y, String otherString, Object object) {} + + public void quux(int x, int y, int z) {} + + public void quux(int x, int y, String string) {} + + public void norf(int x, int y) {} + + public void norf(Object object, String string) {} +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesAnnotations.java b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesAnnotations.java new file mode 100644 index 00000000000..1b85c2ecf76 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesAnnotations.java @@ -0,0 +1,51 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.overloading.testdata; + +import javax.annotation.Nullable; + +public abstract class InconsistentOverloadsPositiveCasesAnnotations { + + @interface Bar {} + + @interface Baz {} + + // BUG: Diagnostic contains: foo(String x, String y, Object z) + abstract void foo(@Nullable Object z, String y, @Nullable String x); + + abstract void foo(@Nullable String x); + + // BUG: Diagnostic contains: foo(String x, String y) + abstract void foo(String y, @Nullable String x); + + // BUG: Diagnostic contains: quux(Object object, String string) + int quux(String string, @Bar @Baz Object object) { + return string.hashCode() + quux(object); + } + + int quux(@Bar @Baz Object object) { + return object.hashCode(); + } + + // BUG: Diagnostic contains: quux(Object object, String string, int x, int y) + abstract int quux(String string, int x, int y, @Bar @Baz Object object); + + abstract int norf(@Bar @Baz String string); + + // BUG: Diagnostic contains: norf(String string, Object object) + abstract int norf(Object object, @Baz @Bar String string); +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesGeneral.java b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesGeneral.java new file mode 100644 index 00000000000..ff31653005a --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesGeneral.java @@ -0,0 +1,48 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.overloading.testdata; + +public final class InconsistentOverloadsPositiveCasesGeneral { + + public void foo(Object object) {} + + // BUG: Diagnostic contains: foo(Object object, int i) + public void foo(int i, Object object) {} + + // BUG: Diagnostic contains: foo(Object object, int i, String string) + public void foo(String string, Object object, int i) {} + + // BUG: Diagnostic contains: bar(int i, int j, String x, String y, Object object) + public void bar(Object object, String x, String y, int i, int j) {} + + public void bar(int i, int j) {} + + // BUG: Diagnostic contains: bar(int i, int j, String x, String y) + public void bar(String x, String y, int i, int j) {} + + public void baz(int i, int j) {} + + public void baz(Object object) {} + + // BUG: Diagnostic contains: baz(int i, int j, String x, Object object) + public void baz(String x, int i, int j, Object object) {} + + public void quux(int x, int y, String string) {} + + // BUG: Diagnostic contains: quux(int x, int y, Object object) + public void quux(Object object, int y, int x) {} +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesGenerics.java b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesGenerics.java new file mode 100644 index 00000000000..52d588c7b3b --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesGenerics.java @@ -0,0 +1,35 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.overloading.testdata; + +import java.util.List; + +public final class InconsistentOverloadsPositiveCasesGenerics { + + // BUG: Diagnostic contains: foo(List numbers, List> nestedNumbers) + public void foo(List> nestedNumbers, List numbers) {} + + public void foo(List numbers) {} + + // BUG: Diagnostic contains: foo(Iterable numbers, String description) + public void foo(String description, Iterable numbers) {} + + public void bar(int x) {} + + // BUG: Diagnostic contains: bar(int x, List> strings) + public void bar(List> strings, int x) {} +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesInterleaved.java b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesInterleaved.java new file mode 100644 index 00000000000..a30551fc7df --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesInterleaved.java @@ -0,0 +1,41 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.overloading.testdata; + +public final class InconsistentOverloadsPositiveCasesInterleaved { + + // BUG: Diagnostic contains: baz(int x, String string, int y) + public void baz(int y, int x, String string) {} + + // BUG: Diagnostic contains: foo(int x, int y, int z, String string) + public void foo(int x, int z, int y, String string) {} + + public void foo(int x, int y) {} + + public void bar(String string, Object object) {} + + // BUG: Diagnostic contains: baz(int x, String string) + public void baz(String string, int x) {} + + // BUG: Diagnostic contains: foo(int x, int y, int z) + public void foo(int z, int x, int y) {} + + // BUG: Diagnostic contains: bar(String string, Object object, int x, int y) + public void bar(int x, int y, String string, Object object) {} + + public void baz(int x) {} +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesSimple.java b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesSimple.java new file mode 100644 index 00000000000..95c8997f5c7 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesSimple.java @@ -0,0 +1,28 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.overloading.testdata; + +public final class InconsistentOverloadsPositiveCasesSimple { + + public void foo(Object object) {} + + // BUG: Diagnostic contains: foo(Object object, int x, int y) + public void foo(int x, int y, Object object) {} + + // BUG: Diagnostic contains: foo(Object object, int x, int y, String string) + public void foo(String string, int y, Object object, int x) {} +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesVarargs.java b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesVarargs.java new file mode 100644 index 00000000000..d038d76d5b9 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesVarargs.java @@ -0,0 +1,37 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.overloading.testdata; + +public abstract class InconsistentOverloadsPositiveCasesVarargs { + + public void foo(String... rest) {} + + public void foo(int x, String... rest) {} + + // BUG: Diagnostic contains: foo(int x, int y, String... rest) + public void foo(int y, int x, String... rest) {} + + abstract void bar(float x, float y); + + // BUG: Diagnostic contains: bar(float x, float y, float z, Object... rest) + abstract void bar(float z, float y, float x, Object... rest); + + // BUG: Diagnostic contains: bar(float x, float y, float z, String string) + abstract void bar(float y, String string, float x, float z); + + abstract void bar(Object... rest); +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/IterableAndIteratorNegativeCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/IterableAndIteratorNegativeCases.java index 2e5033e1f35..5a692fe6739 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/IterableAndIteratorNegativeCases.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/IterableAndIteratorNegativeCases.java @@ -27,58 +27,11 @@ public static class MyNode { MyNode next; } - /** Test List that implements both Iterator and Iterable */ - // BUG: Diagnostic contains: both - public static class MyBadList implements Iterator, Iterable { - private MyNode head; - - public MyBadList() { - head = null; - } - - @Override - public boolean hasNext() { - return head != null; - } - - @Override - public MyNode next() { - if (hasNext()) { - MyNode ret = head; - head = head.next; - return ret; - } - return null; - } - - @Override - public void remove() { - throw new UnsupportedOperationException("remove is not supported"); - } - - public void add(MyNode node) { - if (!hasNext()) { - head.next = node; - } - head = node; - } - - @Override - public Iterator iterator() { - return this; - } - } - - /** Test List that extends the above bad implementation Diagnostic should bypass this */ - public static class MyBadListInherited extends MyBadList { - public MyBadListInherited() {} - } - /** Test List that implements only Iterator */ - public static class MyGoodList implements Iterator { + public static class MyList1 implements Iterator { private MyNode head; - public MyGoodList() { + public MyList1() { head = null; } @@ -110,15 +63,14 @@ public void add(MyNode node) { } } - /** Test List that implicitly implements both interfaces */ - // BUG: Diagnostic contains: both - public static class MyImplicitlyBadList extends MyGoodList implements Iterable { - - public MyImplicitlyBadList() {} + /** Test List that implements only Iterable */ + public static class MyList2 implements Iterable { @Override public Iterator iterator() { - return this; + MyList1 l = new MyList1(); + // code to populate the list goes here + return l; } } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/IterableAndIteratorPositiveCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/IterableAndIteratorPositiveCases.java index 7ab488e724f..638d6bece15 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/IterableAndIteratorPositiveCases.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/IterableAndIteratorPositiveCases.java @@ -27,11 +27,58 @@ public static class MyNode { MyNode next; } + /** Test List that implements both Iterator and Iterable */ + // BUG: Diagnostic contains: both + public static class MyBadList implements Iterator, Iterable { + private MyNode head; + + public MyBadList() { + head = null; + } + + @Override + public boolean hasNext() { + return head != null; + } + + @Override + public MyNode next() { + if (hasNext()) { + MyNode ret = head; + head = head.next; + return ret; + } + return null; + } + + @Override + public void remove() { + throw new UnsupportedOperationException("remove is not supported"); + } + + public void add(MyNode node) { + if (!hasNext()) { + head.next = node; + } + head = node; + } + + @Override + public Iterator iterator() { + return this; + } + } + + /** Test List that extends the above bad implementation Diagnostic should bypass this */ + public static class MyBadListInherited extends MyBadList { + public MyBadListInherited() {} + } + /** Test List that implements only Iterator */ - public static class MyList1 implements Iterator { + public static class MyGoodList implements Iterator { private MyNode head; - public MyList1() { + public MyGoodList() { head = null; } @@ -63,14 +110,15 @@ public void add(MyNode node) { } } - /** Test List that implements only Iterable */ - public static class MyList2 implements Iterable { + /** Test List that implicitly implements both interfaces */ + // BUG: Diagnostic contains: both + public static class MyImplicitlyBadList extends MyGoodList implements Iterable { + + public MyImplicitlyBadList() {} @Override public Iterator iterator() { - MyList1 l = new MyList1(); - // code to populate the list goes here - return l; + return this; } } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/ShouldHaveEvenArgsMultimapNegativeCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/ShouldHaveEvenArgsMultimapNegativeCases.java new file mode 100644 index 00000000000..21316e0b147 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/ShouldHaveEvenArgsMultimapNegativeCases.java @@ -0,0 +1,58 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.testdata; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableMultimap; +import com.google.common.collect.Multimap; + +/** + * Negative test cases for {@link ShouldHaveEvenArgs} check. + * + * @author monnoroch@google.com (Max Strakhov) + */ +public class ShouldHaveEvenArgsMultimapNegativeCases { + + private static final Multimap multimap = ImmutableMultimap.of(); + + public void testWithMinimalArgs() { + assertThat(multimap).containsExactly("hello", "there"); + } + + public void testWithEvenArgs() { + assertThat(multimap).containsExactly("hello", "there", "hello", "there"); + } + + public void testWithVarargs(Object... args) { + assertThat(multimap).containsExactly("hello", args); + assertThat(multimap).containsExactly("hello", "world", args); + } + + public void testWithArray() { + String[] arg = {"hello", "there"}; + assertThat(multimap).containsExactly("yolo", arg); + + String key = "hello"; + Object[] value = new Object[] {}; + Object[][] args = new Object[][] {}; + + assertThat(multimap).containsExactly(key, value); + assertThat(multimap).containsExactly(key, value, (Object[]) args); + assertThat(multimap).containsExactly(key, value, key, value, key, value); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/ShouldHaveEvenArgsMultimapPositiveCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/ShouldHaveEvenArgsMultimapPositiveCases.java new file mode 100644 index 00000000000..7b789316872 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/ShouldHaveEvenArgsMultimapPositiveCases.java @@ -0,0 +1,78 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.testdata; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableMultimap; +import com.google.common.collect.Multimap; +import com.google.common.truth.Correspondence; + +/** + * Positive test cases for {@link ShouldHaveEvenArgs} check. + * + * @author monnoroch@google.com (Max Strakhov) + */ +public class ShouldHaveEvenArgsMultimapPositiveCases { + + private static final Multimap multimap = ImmutableMultimap.of(); + + public void testWithOddArgs() { + // BUG: Diagnostic contains: even number of arguments + assertThat(multimap).containsExactly("hello", "there", "rest"); + + // BUG: Diagnostic contains: even number of arguments + assertThat(multimap).containsExactly("hello", "there", "hello", "there", "rest"); + + // BUG: Diagnostic contains: even number of arguments + assertThat(multimap).containsExactly(null, null, null, null, new Object[] {}); + } + + public void testWithArrayArgs() { + String key = "hello"; + Object[] value = new Object[] {}; + Object[][] args = new Object[][] {}; + + // BUG: Diagnostic contains: even number of arguments + assertThat(multimap).containsExactly(key, value, (Object) args); + } + + public void testWithOddArgsWithCorrespondence() { + assertThat(multimap) + .comparingValuesUsing(new TestCorrespondence()) + // BUG: Diagnostic contains: even number of arguments + .containsExactly("hello", "there", "rest"); + + assertThat(multimap) + .comparingValuesUsing(new TestCorrespondence()) + // BUG: Diagnostic contains: even number of arguments + .containsExactly("hello", "there", "hello", "there", "rest"); + } + + private static class TestCorrespondence extends Correspondence { + + @Override + public boolean compare(String str1, String str2) { + return true; + } + + @Override + public String toString() { + return "test_correspondence"; + } + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/proto/ProtoTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/proto/ProtoTest.java index 6e7931d6ed5..6c19dcfee75 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/proto/ProtoTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/proto/ProtoTest.java @@ -363,16 +363,9 @@ public Builder mergeFrom( onChanged(); return this; default: - { - if (!parseUnknownField( - input, unknownFields, - extensionRegistry, tag)) { - this.setUnknownFields(unknownFields.build()); - onChanged(); - return this; - } - break; - } + this.setUnknownFields(unknownFields.build()); + onChanged(); + return this; case 10: { com.google.errorprone.bugpatterns.proto.ProtoTest.TestFieldProtoMessage.Builder @@ -953,16 +946,9 @@ public Builder mergeFrom( onChanged(); return this; default: - { - if (!parseUnknownField( - input, unknownFields, - extensionRegistry, tag)) { - this.setUnknownFields(unknownFields.build()); - onChanged(); - return this; - } - break; - } + this.setUnknownFields(unknownFields.build()); + onChanged(); + return this; case 10: { com.google.errorprone.bugpatterns.proto.ProtoTest.TestFieldProtoMessage.Builder diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinderTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinderTest.java index 3cc9b87dd50..160a93e449d 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinderTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinderTest.java @@ -16,10 +16,10 @@ package com.google.errorprone.bugpatterns.threadsafety; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; -import com.google.common.base.Optional; import com.google.errorprone.ErrorProneInMemoryFileManager; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.CompilationUnitTree; @@ -27,11 +27,14 @@ import com.sun.tools.javac.api.JavacTool; import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.TreeScanner; +import java.io.BufferedWriter; +import java.io.OutputStreamWriter; import java.io.PrintWriter; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Optional; import javax.tools.JavaCompiler; import javax.tools.JavaFileObject; import org.junit.Ignore; @@ -493,7 +496,8 @@ private String bind(String className, String exprString, JavaFileObject fileObje JavacTaskImpl task = (JavacTaskImpl) javaCompiler.getTask( - new PrintWriter(System.err, true), + new PrintWriter( + new BufferedWriter(new OutputStreamWriter(System.err, UTF_8)), true), fileManager, null, Collections.emptyList(), diff --git a/core/src/test/java/com/google/errorprone/matchers/MatchersTest.java b/core/src/test/java/com/google/errorprone/matchers/MatchersTest.java index 5caa6aa2595..f051bf8bc55 100644 --- a/core/src/test/java/com/google/errorprone/matchers/MatchersTest.java +++ b/core/src/test/java/com/google/errorprone/matchers/MatchersTest.java @@ -31,9 +31,11 @@ import com.google.errorprone.MatcherChecker; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.scanner.ErrorProneScanner; import com.google.errorprone.scanner.ScannerSupplier; +import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; import org.junit.Before; import org.junit.Test; @@ -319,6 +321,41 @@ public void methodReturnsNonPrimitiveType() { .doTest(); } + @Test + public void methodInvocationDoesntMatchAnnotation() { + CompilationTestHelper.newInstance(NoAnnotatedCallsChecker.class, getClass()) + .addSourceLines( + "test/MethodInvocationDoesntMatchAnnotation.java", + "package test;", + "public class MethodInvocationDoesntMatchAnnotation {", + " @Deprecated", + " public void matches() {", + " }", + " public void doesntMatch() {", + " matches();", + " }", + "}") + .doTest(); + } + + @Test + public void methodInvocationHasDeclarationAnnotation() { + CompilationTestHelper.newInstance(NoAnnotatedDeclarationCallsChecker.class, getClass()) + .addSourceLines( + "test/MethodInvocationMatchesDeclAnnotation.java", + "package test;", + "public class MethodInvocationMatchesDeclAnnotation {", + " @Deprecated", + " public void matches() {", + " }", + " public void callsMatch() {", + " // BUG: Diagnostic contains:", + " matches();", + " }", + "}") + .doTest(); + } + @BugPattern( name = "InLoopChecker", summary = "Checker that flags the given expression statement if the given matcher matches", @@ -353,4 +390,40 @@ public Description matchMethod(MethodTree tree, VisitorState state) { return matcher.matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH; } } + + /** Simple checker to make sure hasAnnotation doesn't match on MethodInvocationTree. */ + @BugPattern( + name = "MethodInvocationTreeChecker", + summary = "Checker that flags the given method invocation if the given matcher matches", + category = ONE_OFF, + severity = ERROR + ) + public static class NoAnnotatedCallsChecker extends BugChecker + implements MethodInvocationTreeMatcher { + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (Matchers.hasAnnotation("java.lang.Deprecated").matches(tree, state)) { + return describeMatch(tree); + } + return Description.NO_MATCH; + } + } + + /** Checker that makes sure symbolHasAnnotation matches on MethodInvocationTree. */ + @BugPattern( + name = "NoAnnotatedDeclarationCallsChecker", + summary = "Checker that flags the given method invocation if the given matcher matches", + category = ONE_OFF, + severity = ERROR + ) + public static class NoAnnotatedDeclarationCallsChecker extends BugChecker + implements MethodInvocationTreeMatcher { + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (Matchers.symbolHasAnnotation("java.lang.Deprecated").matches(tree, state)) { + return describeMatch(tree); + } + return Description.NO_MATCH; + } + } } diff --git a/core/src/test/java/com/google/errorprone/refaster/CompilerBasedTest.java b/core/src/test/java/com/google/errorprone/refaster/CompilerBasedTest.java index 7c39e79488e..f4b105e4541 100644 --- a/core/src/test/java/com/google/errorprone/refaster/CompilerBasedTest.java +++ b/core/src/test/java/com/google/errorprone/refaster/CompilerBasedTest.java @@ -21,32 +21,23 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.io.CharStreams; -import com.google.common.io.Resources; -import com.google.common.jimfs.Jimfs; import com.google.errorprone.apply.SourceFile; import com.google.testing.compile.JavaFileObjects; import com.sun.source.tree.CompilationUnitTree; import com.sun.tools.javac.api.JavacTaskImpl; import com.sun.tools.javac.api.JavacTool; -import com.sun.tools.javac.file.JavacFileManager; import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; import com.sun.tools.javac.tree.JCTree.JCMethodDecl; import com.sun.tools.javac.tree.TreeInfo; import com.sun.tools.javac.tree.TreeScanner; import com.sun.tools.javac.util.Context; -import java.io.IOError; import java.io.IOException; -import java.nio.file.FileSystem; -import java.nio.file.Files; -import java.nio.file.Path; -import java.util.Arrays; import java.util.Locale; import java.util.Map; import javax.tools.DiagnosticCollector; import javax.tools.JavaCompiler; import javax.tools.JavaFileObject; import javax.tools.StandardJavaFileManager; -import org.junit.After; /** * Abstract skeleton for tests that run the compiler on the fly. @@ -58,8 +49,6 @@ public class CompilerBasedTest { protected SourceFile sourceFile; protected Iterable compilationUnits; private Map methods; - private final JavacFileManager fileManager = new JavacFileManager(new Context(), false, UTF_8); - private final FileSystem fileSystem = Jimfs.newFileSystem(); protected void compile(TreeScanner scanner, JavaFileObject fileObject) { JavaCompiler compiler = JavacTool.create(); @@ -118,37 +107,10 @@ public void visitTopLevel(JCCompilationUnit tree) { } protected void compile(String... lines) { - try { - Path dir = fileSystem.getRootDirectories().iterator().next().resolve("tmp"); - Files.createDirectories(dir); - Path path = Files.createTempFile(dir, "CompilerBasedTestInput", ".java"); - Files.write(path, Arrays.asList(lines), UTF_8); - compile(fileManager.getJavaFileObject(path)); - } catch (IOException e) { - throw new IOError(e); - } - } - - JavaFileObject forResource(String name) { - try { - Path dir = fileSystem.getRootDirectories().iterator().next().resolve("tmp"); - Files.createDirectories(dir); - Path path = Files.createTempDirectory(dir, "tmp").resolve(name); - Files.createDirectories(path.getParent()); - Files.write(path, Resources.toByteArray(Resources.getResource(name))); - return fileManager.getJavaFileObject(path); - } catch (IOException e) { - throw new IOError(e); - } + compile(JavaFileObjects.forSourceLines("CompilerBasedTestInput", lines)); } protected JCMethodDecl getMethodDeclaration(String name) { return methods.get(name); } - - @After - public void close() throws IOException { - fileSystem.close(); - fileManager.close(); - } } diff --git a/core/src/test/java/com/google/errorprone/refaster/DescriptionBasedDiffTest.java b/core/src/test/java/com/google/errorprone/refaster/DescriptionBasedDiffTest.java index b81fa61dec7..43e3390c577 100644 --- a/core/src/test/java/com/google/errorprone/refaster/DescriptionBasedDiffTest.java +++ b/core/src/test/java/com/google/errorprone/refaster/DescriptionBasedDiffTest.java @@ -69,11 +69,9 @@ public void noDiffs() { @Test public void oneDiff() { DescriptionBasedDiff diff = createDescriptionBasedDiff(); - int idx = - sourceFile.getSourceText().indexOf("foo", sourceFile.getSourceText().indexOf("class")); diff.onDescribed( new Description( - null, "message", SuggestedFix.replace(idx, idx + 3, "bar"), SeverityLevel.SUGGESTION)); + null, "message", SuggestedFix.replace(117, 120, "bar"), SeverityLevel.SUGGESTION)); diff.applyDifferences(sourceFile); assertThat(sourceFile.getLines()) .containsExactly( @@ -91,11 +89,9 @@ public void oneDiff() { @Test public void prefixDiff() { DescriptionBasedDiff diff = createDescriptionBasedDiff(); - int idx = - sourceFile.getSourceText().indexOf("foo", sourceFile.getSourceText().indexOf("class")) + 3; diff.onDescribed( new Description( - null, "message", SuggestedFix.replace(idx, idx, "bar"), SeverityLevel.SUGGESTION)); + null, "message", SuggestedFix.replace(120, 120, "bar"), SeverityLevel.SUGGESTION)); diff.applyDifferences(sourceFile); assertThat(sourceFile.getLines()) .containsExactly( @@ -113,18 +109,11 @@ public void prefixDiff() { @Test public void twoDiffs() { DescriptionBasedDiff diff = createDescriptionBasedDiff(); - int idx1 = - sourceFile.getSourceText().indexOf("out", sourceFile.getSourceText().indexOf("class")); - int idx2 = - sourceFile.getSourceText().indexOf("foo", sourceFile.getSourceText().indexOf("class")); diff.onDescribed( new Description( null, "message", - SuggestedFix.builder() - .replace(idx1, idx1 + 3, "longer") - .replace(idx2, idx2 + 3, "bar") - .build(), + SuggestedFix.builder().replace(104, 107, "longer").replace(117, 120, "bar").build(), SeverityLevel.SUGGESTION)); diff.applyDifferences(sourceFile); assertThat(sourceFile.getLines()) @@ -143,8 +132,6 @@ public void twoDiffs() { @Test public void overlappingDiffs_throws() { DescriptionBasedDiff diff = createDescriptionBasedDiff(); - int idx = - sourceFile.getSourceText().indexOf("foo", sourceFile.getSourceText().indexOf("class")); assertThrows( IllegalArgumentException.class, () -> @@ -153,8 +140,8 @@ public void overlappingDiffs_throws() { null, "message", SuggestedFix.builder() - .replace(idx, idx + 3, "baz") - .replace(idx, idx + 3, "bar") + .replace(117, 120, "baz") + .replace(117, 120, "bar") .build(), SeverityLevel.SUGGESTION))); @@ -163,7 +150,7 @@ public void overlappingDiffs_throws() { new Description( null, "bah", - SuggestedFix.builder().replace(idx, idx + 3, "baz").build(), + SuggestedFix.builder().replace(117, 120, "baz").build(), SeverityLevel.SUGGESTION)); assertThrows( @@ -173,7 +160,7 @@ public void overlappingDiffs_throws() { new Description( null, "message", - SuggestedFix.builder().replace(idx, idx + 3, "bar").build(), + SuggestedFix.builder().replace(117, 120, "bar").build(), SeverityLevel.SUGGESTION))); DescriptionBasedDiff diff3 = @@ -183,14 +170,14 @@ public void overlappingDiffs_throws() { new Description( null, "bah", - SuggestedFix.builder().replace(idx, idx + 3, "baz").build(), + SuggestedFix.builder().replace(117, 120, "baz").build(), SeverityLevel.SUGGESTION)); // No throw, since it's lenient. Refactors to the first "baz" replacement and ignores this. diff3.onDescribed( new Description( null, "message", - SuggestedFix.builder().replace(idx, idx + 3, "bar").build(), + SuggestedFix.builder().replace(117, 120, "bar").build(), SeverityLevel.SUGGESTION)); diff3.applyDifferences(sourceFile); @@ -257,17 +244,13 @@ public void removeImport() { @Test public void twoDiffsWithImport() { DescriptionBasedDiff diff = createDescriptionBasedDiff(); - int idx1 = - sourceFile.getSourceText().indexOf("out", sourceFile.getSourceText().indexOf("class")); - int idx2 = - sourceFile.getSourceText().indexOf("foo", sourceFile.getSourceText().indexOf("class")); diff.onDescribed( new Description( null, "message", SuggestedFix.builder() - .replace(idx1, idx1 + 3, "longer") - .replace(idx2, idx2 + 3, "bar") + .replace(104, 107, "longer") + .replace(117, 120, "bar") .addImport("com.google.foo.Bar") .build(), SeverityLevel.SUGGESTION)); diff --git a/core/src/test/java/com/google/errorprone/refaster/TemplateIntegrationTest.java b/core/src/test/java/com/google/errorprone/refaster/TemplateIntegrationTest.java index e3446f56881..d82f21fd8b4 100644 --- a/core/src/test/java/com/google/errorprone/refaster/TemplateIntegrationTest.java +++ b/core/src/test/java/com/google/errorprone/refaster/TemplateIntegrationTest.java @@ -25,6 +25,7 @@ import com.google.common.collect.FluentIterable; import com.google.common.collect.Iterables; import com.google.errorprone.CodeTransformer; +import com.google.testing.compile.JavaFileObjects; import com.sun.source.tree.ClassTree; import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.Tree; @@ -77,10 +78,13 @@ private void expectTransforms( private void runTest(String testName) throws IOException { CodeTransformer transformer = - extractRefasterRule(forResource(String.format("%s/%s.java", TEMPLATE_DIR, testName))); + extractRefasterRule( + JavaFileObjects.forResource(String.format("%s/%s.java", TEMPLATE_DIR, testName))); - JavaFileObject input = forResource(String.format("%s/%sExample.java", INPUT_DIR, testName)); - JavaFileObject output = forResource(String.format("%s/%sExample.java", OUTPUT_DIR, testName)); + JavaFileObject input = + JavaFileObjects.forResource(String.format("%s/%sExample.java", INPUT_DIR, testName)); + JavaFileObject output = + JavaFileObjects.forResource(String.format("%s/%sExample.java", OUTPUT_DIR, testName)); expectTransforms(transformer, input, output); } @@ -313,5 +317,9 @@ public void inferLambdaBodyType() throws IOException { public void asVarargs() throws IOException { runTest("AsVarargsTemplate"); } -} + @Test + public void placeholderAllowedVars() throws IOException { + runTest("PlaceholderAllowedVarsTemplate"); + } +} diff --git a/core/src/test/java/com/google/errorprone/refaster/testdata/input/PlaceholderAllowedVarsTemplateExample.java b/core/src/test/java/com/google/errorprone/refaster/testdata/input/PlaceholderAllowedVarsTemplateExample.java new file mode 100644 index 00000000000..78993a78ef0 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/refaster/testdata/input/PlaceholderAllowedVarsTemplateExample.java @@ -0,0 +1,35 @@ +/* + * Copyright 2017 Google Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package com.google.errorprone.refaster.testdata; + +public class PlaceholderAllowedVarsTemplateExample { + public void shouldMatch() { + String accum = "foo"; + if (!"foo".equals("bar")) { + System.out.println("in if"); + accum += "bar"; + } + System.out.println("foo"); + } + + public void shouldNotMatch() { + String accum = "foo"; + if (!"foo".equals("bar")) { + System.out.println(accum); + accum += "bar"; + } + System.out.println("foo"); + } +} diff --git a/core/src/test/java/com/google/errorprone/refaster/testdata/output/PlaceholderAllowedVarsTemplateExample.java b/core/src/test/java/com/google/errorprone/refaster/testdata/output/PlaceholderAllowedVarsTemplateExample.java new file mode 100644 index 00000000000..e4bf7d4b268 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/refaster/testdata/output/PlaceholderAllowedVarsTemplateExample.java @@ -0,0 +1,32 @@ +/* + * Copyright 2017 Google Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package com.google.errorprone.refaster.testdata; + +public class PlaceholderAllowedVarsTemplateExample { + public void shouldMatch() { + String accum = "foo"; + System.out.println("found match: " + accum); + System.out.println("foo"); + } + + public void shouldNotMatch() { + String accum = "foo"; + if (!"foo".equals("bar")) { + System.out.println(accum); + accum += "bar"; + } + System.out.println("foo"); + } +} diff --git a/core/src/test/java/com/google/errorprone/refaster/testdata/template/PlaceholderAllowedVarsTemplate.java b/core/src/test/java/com/google/errorprone/refaster/testdata/template/PlaceholderAllowedVarsTemplate.java new file mode 100644 index 00000000000..8d8afabe4fa --- /dev/null +++ b/core/src/test/java/com/google/errorprone/refaster/testdata/template/PlaceholderAllowedVarsTemplate.java @@ -0,0 +1,43 @@ +/* + * Copyright 2017 Google Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package com.google.errorprone.refaster.testdata.template; + +import com.google.errorprone.refaster.annotation.AfterTemplate; +import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.google.errorprone.refaster.annotation.Placeholder; + +public abstract class PlaceholderAllowedVarsTemplate { + @Placeholder + abstract boolean condition(); + + @Placeholder + abstract void beforeBody(); + + @Placeholder + abstract String concatBody(); + + @BeforeTemplate + void before(String str) { + if (condition()) { + beforeBody(); + str += concatBody(); + } + } + + @AfterTemplate + void after(String str) { + System.out.println("found match: " + str); + } +} diff --git a/core/src/test/java/com/google/errorprone/util/ASTHelpersFindSuperMethodsTest.java b/core/src/test/java/com/google/errorprone/util/ASTHelpersFindSuperMethodsTest.java new file mode 100644 index 00000000000..16ca6d8ad44 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/util/ASTHelpersFindSuperMethodsTest.java @@ -0,0 +1,213 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.util; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.HashBasedTable; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Table; +import com.google.errorprone.VisitorState; +import com.google.errorprone.matchers.CompilerBasedAbstractTest; +import com.google.errorprone.scanner.Scanner; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Types; +import java.util.Optional; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** + * Test cases for {@link ASTHelpers#findSuperMethod(MethodSymbol, Types)} and {@link + * ASTHelpers#findSuperMethod(MethodSymbol, Types)}. + * + * @author Łukasz Hanuszczak (hanuszczak@google.com) + */ +@RunWith(JUnit4.class) +public final class ASTHelpersFindSuperMethodsTest extends CompilerBasedAbstractTest { + + private FindSuperMethodsTestScanner scanner; + + @Before + public void prepareScanClassHierarchy() { + writeFile("Foo.java", "abstract class Foo {", " public abstract void foo();", "}"); + writeFile( + "Bar.java", + "class Bar extends Foo {", + " @Override", + " public void foo() {", + " System.out.println(\"bar\");", + " }", + "}"); + writeFile( + "Baz.java", + "class Baz extends Bar {", + " @Override", + " public void foo() {", + " System.out.println(\"baz\");", + " }", + "}"); + writeFile( + "Quux.java", + "class Quux extends Baz {", + " public void foo(String string) {", + " System.out.println(\"I am not an override! \" + string);", + " }", + " public int bar(int x, int y) {", + " return x * y;", + " }", + "}"); + writeFile( + "Norf.java", + "class Norf extends Quux {", + " @Override", + " public void foo() {", + " System.out.println(\"norf\");", + " }", + " @Override", + " public int bar(int x, int y) {", + " return super.bar(x, y) + 42;", + " }", + "}"); + + scanner = new FindSuperMethodsTestScanner(); + assertCompiles(scanner); + } + + @Test + public void findSuperMethods_findsSingleMethod() { + MethodSymbol barOfNorf = scanner.getMethod("Norf", "bar"); + MethodSymbol barOfQuux = scanner.getMethod("Quux", "bar"); + assertThat(findSuperMethods(barOfNorf)).isEqualTo(ImmutableList.of(barOfQuux)); + } + + @Test + public void findSuperMethods_findsAllMethodsInTheHierarchy() { + MethodSymbol fooOfNorf = scanner.getMethod("Norf", "foo"); + MethodSymbol fooOfBaz = scanner.getMethod("Baz", "foo"); + MethodSymbol fooOfBar = scanner.getMethod("Bar", "foo"); + MethodSymbol fooOfQuux = scanner.getMethod("Foo", "foo"); + assertThat(findSuperMethods(fooOfNorf)) + .isEqualTo(ImmutableList.of(fooOfBaz, fooOfBar, fooOfQuux)); + } + + @Test + public void findSuperMethod_findsNothingForAbstractMethod() { + MethodSymbol fooOfFoo = scanner.getMethod("Foo", "foo"); + assertThat(findSuperMethod(fooOfFoo)).isEqualTo(Optional.empty()); + } + + @Test + public void findSuperMethod_findsNothingForNewNonAbstractMethod() { + MethodSymbol barOfQuux = scanner.getMethod("Quux", "bar"); + assertThat(findSuperMethod(barOfQuux)).isEqualTo(Optional.empty()); + } + + @Test + public void findSuperMethod_findsAbstractSuperMethod() { + MethodSymbol fooOfFoo = scanner.getMethod("Foo", "foo"); + MethodSymbol fooOfBar = scanner.getMethod("Bar", "foo"); + assertThat(findSuperMethod(fooOfBar)).isEqualTo(Optional.of(fooOfFoo)); + } + + @Test + public void findSuperMethod_findsNormalSuperMethodForDirectSuperclass() { + MethodSymbol fooOfBar = scanner.getMethod("Bar", "foo"); + MethodSymbol fooOfBaz = scanner.getMethod("Baz", "foo"); + assertThat(findSuperMethod(fooOfBaz)).isEqualTo(Optional.of(fooOfBar)); + + MethodSymbol barOfQuux = scanner.getMethod("Quux", "bar"); + MethodSymbol barOfNorf = scanner.getMethod("Norf", "bar"); + assertThat(findSuperMethod(barOfNorf)).isEqualTo(Optional.of(barOfQuux)); + } + + @Test + public void findSuperMethod_findsNormalSuperMethodForNonDirectSuperclass() { + MethodSymbol fooOfBaz = scanner.getMethod("Baz", "foo"); + MethodSymbol fooOfNorf = scanner.getMethod("Norf", "foo"); + assertThat(findSuperMethod(fooOfNorf)).isEqualTo(Optional.of(fooOfBaz)); + } + + private ImmutableList findSuperMethods(MethodSymbol method) { + return ASTHelpers.findSuperMethods(method, getTypes()).stream().collect(toImmutableList()); + } + + private Optional findSuperMethod(MethodSymbol method) { + return ASTHelpers.findSuperMethod(method, getTypes()); + } + + private Types getTypes() { + return scanner.getState().getTypes(); + } + + /** + * A quite hacky class used to assert things in {@link ASTHelpersFindSuperMethodsTest}. + * + *

This does two things: it builds a mapping from class names and method names to method + * symbols (for easy assertions) and keeps track of the last visited {@link VisitorState}. + * + *

We cannot do assertions in the {@link Scanner#scan(Tree, VisitorState)} like all the other + * test cases do because we need data from all processed classes (and {@link Scanner#scan(Tree, + * VisitorState)} is triggered for every single class). Therefore we need to make all assertions + * in the test method itself but {@link ASTHelpers#findSuperMethods(MethodSymbol, Types)} requires + * a {@link VisitorState} to be used. So we simply remember last state passed to the {@link + * Scanner#scan(Tree, VisitorState)}. + */ + private static class FindSuperMethodsTestScanner extends Scanner { + + // A `class name -> method name -> method symbol` structure mapping of given files. + private final Table methods; + + // Last state passed to the `Scanner#scan` method. + private VisitorState state; + + public FindSuperMethodsTestScanner() { + this.methods = HashBasedTable.create(); + } + + public MethodSymbol getMethod(String className, String methodName) { + return methods.get(className, methodName); + } + + public VisitorState getState() { + return state; + } + + @Override + public Void scan(Tree tree, VisitorState state) { + this.state = state; + return super.scan(tree, state); + } + + @Override + public Void visitMethod(MethodTree methodTree, VisitorState state) { + String classContext = ASTHelpers.getSymbol(methodTree).owner.getSimpleName().toString(); + String methodContext = methodTree.getName().toString(); + + MethodSymbol method = ASTHelpers.getSymbol(methodTree); + if (!methods.contains(classContext, methodContext)) { + methods.put(classContext, methodContext, method); + } + + return super.visitMethod(methodTree, state); + } + } +} diff --git a/core/src/test/java/com/google/errorprone/util/ASTHelpersTest.java b/core/src/test/java/com/google/errorprone/util/ASTHelpersTest.java index 59d93d4085d..2630a2a7265 100644 --- a/core/src/test/java/com/google/errorprone/util/ASTHelpersTest.java +++ b/core/src/test/java/com/google/errorprone/util/ASTHelpersTest.java @@ -569,6 +569,29 @@ public Void visitMethod(MethodTree tree, VisitorState state) { assertCompiles(scanner); } + @Test + public void testHasAnnotationOnMethodInvocation() { + writeFile( + "A.java", // + "public class A {", + " @Deprecated public void doIt() {}", + " void caller() { doIt(); }", + "}"); + TestScanner scanner = + new TestScanner() { + @Override + public Void visitMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (ASTHelpers.getSymbol(tree).toString().equals("doIt()")) { + setAssertionsComplete(); + assertThat(ASTHelpers.hasAnnotation(tree, Deprecated.class, state)).isFalse(); + } + return super.visitMethodInvocation(tree, state); + } + }; + tests.add(scanner); + assertCompiles(scanner); + } + @BugPattern( name = "HasDirectAnnotationWithSimpleNameChecker", category = Category.ONE_OFF, diff --git a/docs/bugpattern/PackageLocation.md b/docs/bugpattern/PackageLocation.md index b9562df606c..f9e41685f18 100644 --- a/docs/bugpattern/PackageLocation.md +++ b/docs/bugpattern/PackageLocation.md @@ -1,9 +1,10 @@ Java files should be located in a directory that -matches the fully qualified name of the package. +ends with the fully qualified name of the package. For example, classes in the package `edu.oswego.cs.dl.util.concurrent` should be located in: `.../edu/oswego/cs/dl/util/concurrent`. + ## Suppression If necessary, the check may be suppressed by annotating the enclosing package diff --git a/docs/bugpattern/argumentselectiondefects/NamedParameters.md b/docs/bugpattern/argumentselectiondefects/NamedParameters.md new file mode 100644 index 00000000000..b58255442df --- /dev/null +++ b/docs/bugpattern/argumentselectiondefects/NamedParameters.md @@ -0,0 +1,69 @@ +For clarity, and to avoid potentially incorrectly swapping arguments, arguments +may be explicitly matched to their parameter by preceding them with a block +comment containing the parameter name followed by an equals sign. Mismatches +between the name in the comment and the actual name will then cause a +compilation error. + +## Parameter-name comment style + +The style accepted for comments that name parameters is to use a block comment, +before the argument, including an equals sign. + +```java +test(/* param1= */arg1, + /* param2= */arg2); +``` + +The use of spaces (or not) around the parameter name and the equals sign is +optional. + +### Rationale + +There are a variety of styles in use for commenting arguments in method calls. +Adopting a consistent style should be valuable for readability. There are also a +variety of shortcomings with alternative techniques. + +*Shortcomings with the use of line comments* (instead of block comments). +Firstly, they force a vertical layout of the method call even if it would fit on +one line. Secondly, associating line comments with arguments is hard to get +right. For example: + +```java +test(arg1, // param1 + arg2); // param2 +``` + +In this example `param1` is in the same parse tree node as `arg2` (because its +after the comma) and `param2` is not even in the same parse tree node as the +method invocation (its after the semi-colon). + +*Shortcomings with the use of block comments after the argument* arise because +its not always natural to put them in the right place. For example: + +```java +// not as intended +test(arg1, /* param1 */ + arg2); /* param2 */ + +// intended +test(arg1 /* param1 */, + arg2 /* param2 */); +``` + +*The benefit of including the equals-sign* is that it helps associate the +comment with the correct argument. For example: + +```java +test(arg1, /* param2= */ + arg2); +``` + +In this example the formatter has moved the comment that should be before `arg2` +onto the previous line. However, the inclusion of the equals sign means that +there is at least a visual cue that this has happened. + +*Commonality with other languages*. Python support for named parameters uses an +equals sign, and the clang-tidy +[misc-comment-argument](https://clang.llvm.org/extra/clang-tidy/checks/misc-argument-comment.html) +check uses block-comments with equals signs. + diff --git a/docs/bugpattern/overloading/InconsistentOverloads.md b/docs/bugpattern/overloading/InconsistentOverloads.md new file mode 100644 index 00000000000..618f1712e8b --- /dev/null +++ b/docs/bugpattern/overloading/InconsistentOverloads.md @@ -0,0 +1,29 @@ +Inconsistently ordered parameters in method overloads can be very confusing to +the users. For example, the following overloads are considered to be consistent: + +```java +public void foo(Bar bar, Baz baz) { ... } +public void foo(Bar bar, Baz baz, int x) { ... } +``` + +However, these are not consistent: + +```java +public void foo(Bar bar, Baz baz) { ... } +public void foo(Bar bar, int x, Baz baz) { ... } +``` + +Having inconsistent parameters not only makes the code more confusing and puts +additional burden on the user, but can also lead to very serious bugs. Consider +the following overloaded methods: + +```java +public void foo(Bar bar, String suffix) { ... } +public void foo(Bar bar, String prefix, String suffix) { ... } +``` + +If the caller has a code like `foo(bar, "quux")` and wants to add custom prefix +support he will most likely do it like `foo(bar, "quux", "norf")`. The compiler +will accept this because the types match perfectly. However, this is clearly a +bug caused by unintuitive API and method overloading. +