From 35b4442ace8f2bd30ee2e6d512d32e111247f82f Mon Sep 17 00:00:00 2001 From: eaftan Date: Wed, 24 May 2017 15:28:18 -0700 Subject: [PATCH 01/17] Fix false positives from FunctionalInterfaceClash. If you have an inheritance chain that expands the visibility of an overridden method, the FunctionalInterfaceClash checker may consider consider those to be separate methods rather than overrides of the same method, causing a false positive. RELNOTES: None ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157045905 --- .../bugpatterns/FunctionalInterfaceClash.java | 23 +++++++++- .../FunctionalInterfaceClashTest.java | 42 +++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) 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/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(); + } } From 971f4deafb7ddcdaaf585472a0682ad92630e2d0 Mon Sep 17 00:00:00 2001 From: cushon Date: Wed, 24 May 2017 23:10:19 -0700 Subject: [PATCH 02/17] Automated rollback of 8a87d5714da2668a0044983ae3a86912ec160582. *** Original change description *** Do less path string manipulation to avoid windows-specific issues with patch application. RELNOTES: N/A *** ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157080648 --- .../errorprone/RefactoringCollection.java | 13 +++--- .../apply/DescriptionBasedDiff.java | 8 ++-- .../com/google/errorprone/apply/Diff.java | 6 +-- .../google/errorprone/apply/DiffApplier.java | 15 ++++--- .../google/errorprone/apply/FileSource.java | 3 +- .../errorprone/apply/FsFileDestination.java | 10 ++++- .../google/errorprone/apply/FsFileSource.java | 11 ++++- .../google/errorprone/apply/SourceFile.java | 16 ++------ .../errorprone/fixes/SuggestedFixes.java | 3 +- .../errorprone/apply/SourceFileTest.java | 4 +- .../refaster/CompilerBasedTest.java | 40 +------------------ .../refaster/DescriptionBasedDiffTest.java | 39 +++++------------- .../refaster/TemplateIntegrationTest.java | 11 +++-- 13 files changed, 65 insertions(+), 114 deletions(-) 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/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/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..9b52be18f38 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); } @@ -314,4 +318,3 @@ public void asVarargs() throws IOException { runTest("AsVarargsTemplate"); } } - From 0c3140277c5ef1ff13f654064ad61501aa3e9910 Mon Sep 17 00:00:00 2001 From: cushon Date: Thu, 25 May 2017 12:45:16 -0700 Subject: [PATCH 03/17] Support all annotations named 'GuardedBy' including com.android.internal.annotations.GuardedBy. RELNOTES: Support all annotations named 'GuardedBy' ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157140444 --- .../AbstractLockMethodChecker.java | 4 +- .../threadsafety/GuardedByBinder.java | 8 +-- .../threadsafety/GuardedByUtils.java | 49 +++++++++++-------- .../threadsafety/HeldLockAnalyzer.java | 8 +-- .../threadsafety/GuardedByBinderTest.java | 8 ++- 5 files changed, 45 insertions(+), 32 deletions(-) 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/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/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(), From 88e39d04b2b3b394cd6170e39fbd09f5599af43c Mon Sep 17 00:00:00 2001 From: hanuszczak Date: Thu, 25 May 2017 16:21:23 -0700 Subject: [PATCH 04/17] Refactor duplicated code for finding super-method symbol to the Error Prone core. RELNOTES: n/a ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157167781 --- .../google/errorprone/util/ASTHelpers.java | 20 +- .../util/ASTHelpersFindSuperMethodsTest.java | 213 ++++++++++++++++++ 2 files changed, 231 insertions(+), 2 deletions(-) create mode 100644 core/src/test/java/com/google/errorprone/util/ASTHelpersFindSuperMethodsTest.java 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..0c5b6e52ec8 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; @@ -438,13 +440,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); } /** 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); + } + } +} From c44ae4ca7dbec465f0f3cbf9be085ea0def94b70 Mon Sep 17 00:00:00 2001 From: plumpy Date: Thu, 25 May 2017 17:02:18 -0700 Subject: [PATCH 05/17] Fix docs for NamedParameters ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157171839 --- .../argumentselectiondefects/NamedParameterChecker.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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..9db13e66778 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 @@ -61,10 +61,10 @@ 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.", + + "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. If " + + "the called method is annotated with RequiresNamedParameters then an error will occur " + + "if any names are omitted.", category = JDK, severity = WARNING ) From 75f7a2bef45ff9dc654788bdb40ff6ceb5845328 Mon Sep 17 00:00:00 2001 From: glorioso Date: Fri, 26 May 2017 10:14:42 -0700 Subject: [PATCH 06/17] Add java.util.Optional{Double,Int,Long} and java.util.UUID to the list of well known immutable classes. RELNOTES: N/A ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157233479 --- .../bugpatterns/threadsafety/WellKnownMutability.java | 4 ++++ 1 file changed, 4 insertions(+) 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") From 6d4472d83c839aa1bc522dea9ed1b4f9a54fc08c Mon Sep 17 00:00:00 2001 From: hanuszczak Date: Fri, 26 May 2017 12:09:26 -0700 Subject: [PATCH 07/17] Add a simplified checker to warn about inconsistent overloads. RELNOTES: New checker to warn about inconsistent overloads. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157247805 --- .../overloading/InconsistentOverloads.java | 161 +++++++++++ .../ParameterOrderingViolation.java | 92 +++++++ .../overloading/ParameterTree.java | 90 +++++++ .../overloading/ParameterTrie.java | 250 ++++++++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../InconsistentOverloadsTest.java | 76 ++++++ .../InconsistentOverloadsNegativeCases.java | 46 ++++ ...tentOverloadsPositiveCasesAnnotations.java | 51 ++++ ...nsistentOverloadsPositiveCasesGeneral.java | 48 ++++ ...sistentOverloadsPositiveCasesGenerics.java | 35 +++ ...tentOverloadsPositiveCasesInterleaved.java | 41 +++ ...onsistentOverloadsPositiveCasesSimple.java | 28 ++ ...nsistentOverloadsPositiveCasesVarargs.java | 37 +++ .../overloading/InconsistentOverloads.md | 29 ++ 14 files changed, 986 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/overloading/InconsistentOverloads.java create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/overloading/ParameterOrderingViolation.java create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/overloading/ParameterTree.java create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/overloading/ParameterTrie.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/overloading/InconsistentOverloadsTest.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsNegativeCases.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesAnnotations.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesGeneral.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesGenerics.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesInterleaved.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesSimple.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/overloading/testdata/InconsistentOverloadsPositiveCasesVarargs.java create mode 100644 docs/bugpattern/overloading/InconsistentOverloads.md 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/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/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/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. + From a60e149ffe355763dc7adb36da72d04bc358d408 Mon Sep 17 00:00:00 2001 From: bangert Date: Tue, 30 May 2017 12:08:09 -0700 Subject: [PATCH 08/17] Fix ASTHelpers.hasAnnotation and Matchers.hasAnnotation to not consider declarations on references symbols. "method(foo, bar)" should not match hasAnnotation("java.lang.Deprecated") even if the declaration of method is marked "@Deprecated". RELNOTES: hasAnnotation now only considers annotations on declaration trees. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157494127 --- .../google/errorprone/matchers/Matchers.java | 32 ++++++++ .../google/errorprone/util/ASTHelpers.java | 48 +++++++----- .../AbstractMustBeClosedChecker.java | 4 +- .../FunctionalInterfaceMethodChanged.java | 2 +- .../bugpatterns/NoAllocationChecker.java | 3 +- .../NamedParameterChecker.java | 4 +- .../InjectedConstructorAnnotations.java | 3 +- .../errorprone/refaster/UTemplater.java | 4 +- .../errorprone/matchers/MatchersTest.java | 73 +++++++++++++++++++ .../errorprone/util/ASTHelpersTest.java | 23 ++++++ 10 files changed, 168 insertions(+), 28 deletions(-) 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/util/ASTHelpers.java b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java index 0c5b6e52ec8..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 @@ -142,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); } @@ -157,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; } @@ -169,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. */ @@ -546,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/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/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/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/argumentselectiondefects/NamedParameterChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterChecker.java index 9db13e66778..b26c54eef48 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,7 +19,7 @@ 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 static com.google.errorprone.matchers.Matchers.symbolHasAnnotation; import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; @@ -240,7 +240,7 @@ private static Optional findGoodSwap( } private static Matcher hasRequiresNamedParametersAnnotation() { - return hasAnnotation(RequiresNamedParameters.class.getCanonicalName()); + return symbolHasAnnotation(RequiresNamedParameters.class.getCanonicalName()); } /** Information about an argument, the name attached to it with a comment */ 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/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/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/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, From 3a79fd6702bed297a490b0a9159824082436e21a Mon Sep 17 00:00:00 2001 From: monnoroch Date: Wed, 31 May 2017 12:32:04 -0700 Subject: [PATCH 09/17] Add MultimapSubject.containsExactly() and MultimapSubject.comparingValuesUsing().containsExactly() to the should have even args check. RELNOTES: ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157619475 --- .../bugpatterns/ShouldHaveEvenArgs.java | 4 +- .../bugpatterns/ShouldHaveEvenArgsTest.java | 13 ++++ ...ouldHaveEvenArgsMultimapNegativeCases.java | 58 ++++++++++++++ ...ouldHaveEvenArgsMultimapPositiveCases.java | 78 +++++++++++++++++++ 4 files changed, 152 insertions(+), 1 deletion(-) create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/testdata/ShouldHaveEvenArgsMultimapNegativeCases.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/testdata/ShouldHaveEvenArgsMultimapPositiveCases.java 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/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/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"; + } + } +} From 3215c1c1a5f816439b92700198241e37c630dbe4 Mon Sep 17 00:00:00 2001 From: epmjohnston Date: Fri, 2 Jun 2017 10:25:17 -0700 Subject: [PATCH 10/17] Bug Fix: BugCheckers should *only* be instantiated by ScannerSupplier, which determines whether the default constructor exists, or whether an ErrorProneFlags constructor should be invoked instead. Tricorder's ErrorProneFindingsJavacAnalyzer was directly calling BugChecker constructors rather than allowing a ScannerSupplier to construct them. This broke when ErrorProneFlags constructors were added to the Immutable checkers, and the no-args constructors removed. This cl adds a getter to ErrorProneScanner for a scanner's instantiated BugCheckers, so no classes outside of Error Prone have to instantiate BugCheckers themselves. RELNOTES: N/A ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157845791 --- .../com/google/errorprone/scanner/ErrorProneScanner.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) 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; + } } From f2aaa2c60ab66147c20e8da72065faab71f13356 Mon Sep 17 00:00:00 2001 From: lowasser Date: Fri, 2 Jun 2017 12:37:29 -0700 Subject: [PATCH 11/17] Add a second reverification pass to placeholder methods. Placeholders are not generally allowed to reference variables that are bound to variables Refaster knows about but aren't passed into the placeholder method. But this verification hadn't found when there were variables Refaster hadn't *yet* matched but would match after the placeholder. So now, we just rerun that verification pass at the end of the match. Bug discovered by sulku@ and mariasam@ :) RELNOTES: fixes a subtle Refaster placeholder bug ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157862410 --- .../errorprone/refaster/BlockTemplate.java | 7 +++ .../refaster/ExpressionTemplate.java | 32 ++++++++++++++ .../PlaceholderUnificationVisitor.java | 11 ++--- .../errorprone/refaster/ULocalVarIdent.java | 4 +- .../errorprone/refaster/UMethodDecl.java | 3 +- .../google/errorprone/refaster/UNewArray.java | 6 +-- .../refaster/UPlaceholderExpression.java | 11 +++++ .../refaster/UPlaceholderStatement.java | 11 +++++ .../errorprone/refaster/UVariableDecl.java | 5 +-- .../refaster/TemplateIntegrationTest.java | 5 +++ ...PlaceholderAllowedVarsTemplateExample.java | 35 +++++++++++++++ ...PlaceholderAllowedVarsTemplateExample.java | 32 ++++++++++++++ .../PlaceholderAllowedVarsTemplate.java | 43 +++++++++++++++++++ 13 files changed, 188 insertions(+), 17 deletions(-) create mode 100644 core/src/test/java/com/google/errorprone/refaster/testdata/input/PlaceholderAllowedVarsTemplateExample.java create mode 100644 core/src/test/java/com/google/errorprone/refaster/testdata/output/PlaceholderAllowedVarsTemplateExample.java create mode 100644 core/src/test/java/com/google/errorprone/refaster/testdata/template/PlaceholderAllowedVarsTemplate.java 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/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/test/java/com/google/errorprone/refaster/TemplateIntegrationTest.java b/core/src/test/java/com/google/errorprone/refaster/TemplateIntegrationTest.java index 9b52be18f38..d82f21fd8b4 100644 --- a/core/src/test/java/com/google/errorprone/refaster/TemplateIntegrationTest.java +++ b/core/src/test/java/com/google/errorprone/refaster/TemplateIntegrationTest.java @@ -317,4 +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); + } +} From a9cdfd7df6581fe32625f55a42ade081ee50a1fc Mon Sep 17 00:00:00 2001 From: glorioso Date: Fri, 2 Jun 2017 12:53:14 -0700 Subject: [PATCH 12/17] Update documentation for PackageLocation to clarify that the package path needs to _end with_ the package. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157864018 --- docs/bugpattern/PackageLocation.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 From 5f9a2ec2abee5d5320eb24218dbcaa8f5f5a73fe Mon Sep 17 00:00:00 2001 From: glorioso Date: Sat, 7 Jan 2017 14:31:02 -0500 Subject: [PATCH 13/17] Swap IterableAndIterator{Positive,Negative}Cases, as they represent the opposite than in other cases. RELNOTES: n/a ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=158062600 --- .../IterableAndIteratorNegativeCases.java | 62 +++---------------- .../IterableAndIteratorPositiveCases.java | 62 ++++++++++++++++--- 2 files changed, 62 insertions(+), 62 deletions(-) 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; } } } From 8e7e8b4e89659d3fed7a9bd385c0125dd45f5f7b Mon Sep 17 00:00:00 2001 From: eaftan Date: Wed, 7 Jun 2017 11:26:53 -0700 Subject: [PATCH 14/17] Fix test breakage due to removal of GeneratedMessage.Builder#parseUnknownField method. This is just test code that represents generated proto code. The actual content is not meaningful for our tests. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=158293382 --- .../bugpatterns/testdata/proto/ProtoTest.java | 26 +++++-------------- 1 file changed, 6 insertions(+), 20 deletions(-) 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 From a9c4a64004da4a23c815074b0586a6d7c83f77b8 Mon Sep 17 00:00:00 2001 From: andrewrice Date: Fri, 7 Apr 2017 15:25:35 -0400 Subject: [PATCH 15/17] Updated NamedParameters to improve error message when comment doesn't conform to style and also to detect bad after-block comments. Change the suggestion to include spaces around the equals sign (the check is agnostic to spaces). This change also removes @RequiresNamedParameters. We probably shouldn't be providing a new language feature which legitimizes an API design which should instead be refactored to improve usability. RELNOTES: Change suggestion for NamedParameters to include spaces around the equals sign, improved errormessage and removed @RequiresNamedParameters ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=158570586 --- .../annotations/RequiresNamedParameters.java | 38 ---- .../NameInCommentHeuristic.java | 3 +- .../NamedParameterChecker.java | 168 +++++++----------- .../NamedParameterComment.java | 83 ++++++++- .../ArgumentSelectionDefectCheckerTest.java | 12 +- .../AssertEqualsArgumentOrderCheckerTest.java | 4 +- .../NameInCommentHeuristicTest.java | 2 +- .../NamedParameterCheckerTest.java | 103 +++++++---- .../NamedParameters.md | 69 +++++++ 9 files changed, 286 insertions(+), 196 deletions(-) delete mode 100644 annotations/src/main/java/com/google/errorprone/annotations/RequiresNamedParameters.java create mode 100644 docs/bugpattern/argumentselectiondefects/NamedParameters.md 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/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 b26c54eef48..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.symbolHasAnnotation; 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 an equals sign (\"=\"). 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 symbolHasAnnotation(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/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/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. + From 92e517941694ef23d745d3ef985a15c3cabe9174 Mon Sep 17 00:00:00 2001 From: glorioso Date: Mon, 12 Jun 2017 10:51:40 -0700 Subject: [PATCH 16/17] Deprecate BugPattern.category, replacing it with a set of String tags. These tags can be used by tooling (including our Documentation generation) to group BugCheckers into different buckets, and tag most of the existing warning-level BugChecker instances. RELNOTES: Deprecate BugPattern.category in favor of a collection of String tags ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=158733828 --- .../com/google/errorprone/BugPattern.java | 82 ++++++++++++++++++- .../errorprone/BugPatternValidator.java | 12 +-- .../com/google/errorprone/BugCheckerInfo.java | 39 +++++---- .../BadAnnotationImplementation.java | 4 +- .../errorprone/bugpatterns/BadComparable.java | 4 +- .../BoxedPrimitiveConstructor.java | 4 +- .../bugpatterns/ClassCanBeStatic.java | 4 +- .../bugpatterns/ClassNewInstance.java | 4 +- .../errorprone/bugpatterns/ConstantField.java | 4 +- .../bugpatterns/DefaultCharset.java | 4 +- .../bugpatterns/EqualsHashCode.java | 4 +- .../bugpatterns/ExpectedExceptionChecker.java | 4 +- .../errorprone/bugpatterns/Finally.java | 4 +- .../FloatingPointLiteralPrecision.java | 4 +- .../bugpatterns/FutureReturnValueIgnored.java | 4 +- .../bugpatterns/GetClassOnEnum.java | 4 +- .../IncompatibleModifiersChecker.java | 5 +- .../InputStreamSlowMultibyteRead.java | 4 +- .../bugpatterns/IterableAndIterator.java | 4 +- .../errorprone/bugpatterns/JavaLangClash.java | 4 +- .../bugpatterns/LogicalAssignment.java | 4 +- .../bugpatterns/MissingDefault.java | 4 +- .../bugpatterns/MissingOverride.java | 4 +- .../bugpatterns/MixedArrayDimensions.java | 2 + .../bugpatterns/MultiVariableDeclaration.java | 2 + .../bugpatterns/MultipleTopLevelClasses.java | 2 + .../NarrowingCompoundAssignment.java | 4 +- .../bugpatterns/NonAtomicVolatileUpdate.java | 4 +- .../bugpatterns/NonOverridingEquals.java | 4 +- .../bugpatterns/NullableConstructor.java | 4 +- .../bugpatterns/NullablePrimitive.java | 4 +- .../errorprone/bugpatterns/NullableVoid.java | 4 +- .../bugpatterns/OperatorPrecedence.java | 4 +- .../bugpatterns/PackageLocation.java | 4 +- .../PreconditionsInvalidPlaceholder.java | 4 +- .../ProtoFieldPreconditionsCheckNotNull.java | 4 +- .../bugpatterns/ReferenceEquality.java | 4 +- .../bugpatterns/RemoveUnusedImports.java | 4 +- .../bugpatterns/RequiredModifiersChecker.java | 4 +- .../bugpatterns/ShortCircuitBoolean.java | 4 +- .../bugpatterns/SimpleDateFormatConstant.java | 4 +- .../StaticQualifiedUsingExpression.java | 4 +- .../bugpatterns/TestExceptionChecker.java | 4 +- .../bugpatterns/TruthConstantAsserts.java | 4 +- .../bugpatterns/TypeParameterShadowing.java | 4 +- .../TypeParameterUnusedInFormals.java | 4 +- .../bugpatterns/URLEqualsHashCode.java | 4 +- .../bugpatterns/UnnecessaryStaticImport.java | 4 +- .../UnsynchronizedOverridesSynchronized.java | 4 +- .../errorprone/bugpatterns/WaitNotInLoop.java | 4 +- .../bugpatterns/WildcardImport.java | 2 + .../android/FragmentInjection.java | 4 +- .../android/FragmentNotInstantiable.java | 4 +- .../inject/QualifierWithTypeUse.java | 4 +- .../threadsafety/DoubleCheckedLocking.java | 4 +- .../ImmutableAnnotationChecker.java | 4 +- .../threadsafety/StaticGuardedByInstance.java | 6 +- .../SynchronizeOnNonFinalField.java | 4 +- 58 files changed, 269 insertions(+), 79 deletions(-) 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/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/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/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/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..cb9fa3b13b5 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ShortCircuitBoolean.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ShortCircuitBoolean.java @@ -23,6 +23,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.BinaryTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -36,7 +37,8 @@ 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 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/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/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/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 { From 215ee79a69e26ed323ac9718bb4e7fa52da494e9 Mon Sep 17 00:00:00 2001 From: sulku Date: Tue, 13 Jun 2017 12:23:11 -0700 Subject: [PATCH 17/17] Edited ShortCircuitBoolean ErrorProne fix to only show one suggested fix per line ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=158880824 --- .../bugpatterns/ShortCircuitBoolean.java | 50 +++++++++++++++++-- .../bugpatterns/ShortCircuitBooleanTest.java | 17 ++++++- 2 files changed, 61 insertions(+), 6 deletions(-) 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 cb9fa3b13b5..b7f2523747a 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ShortCircuitBoolean.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ShortCircuitBoolean.java @@ -30,9 +30,16 @@ 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, @@ -41,6 +48,7 @@ tags = StandardTags.FRAGILE_CODE ) public class ShortCircuitBoolean extends BugChecker implements BinaryTreeMatcher { + @Override public Description matchBinary(BinaryTree tree, VisitorState state) { switch (tree.getKind()) { @@ -53,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/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();