{@code
* public class C {
- * private static final S = "Hello";
+ * private static final String S = "Hello";
* void m(@CompileTimeConstant final String s) { }
* void n(@CompileTimeConstant final String t) {
* m(S + " World!");
diff --git a/check_api/pom.xml b/check_api/pom.xml
index 4a023c78089..a20e3661e93 100644
--- a/check_api/pom.xml
+++ b/check_api/pom.xml
@@ -21,7 +21,7 @@
com.google.errorprone
error_prone_parent
- HEAD-SNAPSHOT
+ 2.15.0
error-prone check api
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 62d4a76208a..ced2a69e834 100644
--- a/check_api/src/main/java/com/google/errorprone/RefactoringCollection.java
+++ b/check_api/src/main/java/com/google/errorprone/RefactoringCollection.java
@@ -160,12 +160,11 @@ public DescriptionListener getDescriptionListener(Log log, JCCompilationUnit com
RefactoringResult applyChanges(URI uri) throws Exception {
Collection listeners = foundSources.removeAll(uri);
- if (listeners.isEmpty()) {
- return RefactoringResult.create("", RefactoringResultType.NO_CHANGES);
+ if (doApplyProcess(fileDestination, new FsFileSource(rootPath), listeners)) {
+ return postProcess.apply(uri);
}
- doApplyProcess(fileDestination, new FsFileSource(rootPath), listeners);
- return postProcess.apply(uri);
+ return RefactoringResult.create("", RefactoringResultType.NO_CHANGES);
}
private static void writePatchFile(
@@ -185,15 +184,21 @@ private static void writePatchFile(
}
}
- private static void doApplyProcess(
+ private static boolean doApplyProcess(
FileDestination fileDestination,
FileSource fileSource,
Collection listeners) {
+ boolean appliedDiff = false;
for (DelegatingDescriptionListener listener : listeners) {
+ if (listener.base.isEmpty()) {
+ continue;
+ }
+
try {
SourceFile file = fileSource.readFile(listener.base.getRelevantFileName());
listener.base.applyDifferences(file);
fileDestination.writeFile(file);
+ appliedDiff = true;
} catch (IOException e) {
logger.log(
Level.WARNING,
@@ -201,6 +206,8 @@ private static void doApplyProcess(
e);
}
}
+
+ return appliedDiff;
}
private static final class DelegatingDescriptionListener implements DescriptionListener {
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 0e704ba6040..3df86f4abdd 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
@@ -123,9 +123,6 @@ public void applyDifferences(SourceFile sourceFile) throws DiffNotApplicableExce
Replacements.CoalescePolicy.REPLACEMENT_FIRST);
}
}
- for (Replacement replacement : replacements.descending()) {
- sourceFile.replaceChars(
- replacement.startPosition(), replacement.endPosition(), replacement.replaceWith());
- }
+ sourceFile.makeReplacements(replacements);
}
}
diff --git a/check_api/src/main/java/com/google/errorprone/apply/ImportOrganizer.java b/check_api/src/main/java/com/google/errorprone/apply/ImportOrganizer.java
index 509398d16ac..f63e1436a7c 100644
--- a/check_api/src/main/java/com/google/errorprone/apply/ImportOrganizer.java
+++ b/check_api/src/main/java/com/google/errorprone/apply/ImportOrganizer.java
@@ -16,6 +16,7 @@
package com.google.errorprone.apply;
import com.google.auto.value.AutoValue;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.Collection;
import java.util.List;
import java.util.Map;
@@ -182,6 +183,7 @@ public String asImportBlock() {
* @param keys the keys to add, in order, if a key is not in the groups then it is ignored.
* @return this for chaining.
*/
+ @CanIgnoreReturnValue
public OrganizedImports addGroups(
Map> groups, Iterable keys) {
for (K key : keys) {
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 5f76a929c83..b5a6d69dd0e 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
@@ -16,9 +16,15 @@
package com.google.errorprone.apply;
+import static com.google.common.base.Preconditions.checkArgument;
+
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
import com.google.common.io.CharSource;
+import com.google.errorprone.fixes.Replacement;
+import com.google.errorprone.fixes.Replacements;
import java.io.IOException;
import java.io.LineNumberReader;
import java.io.StringReader;
@@ -158,4 +164,51 @@ public void replaceChars(int startPosition, int endPosition, String replacement)
path, sourceBuilder.length(), startPosition, endPosition, replacement));
}
}
+
+ void makeReplacements(Replacements changes) {
+ ImmutableSet replacements = changes.ascending();
+ switch (replacements.size()) {
+ case 0:
+ return;
+ case 1:
+ {
+ Replacement onlyReplacement = Iterables.getOnlyElement(replacements);
+ replaceChars(
+ onlyReplacement.startPosition(),
+ onlyReplacement.endPosition(),
+ onlyReplacement.replaceWith());
+ return;
+ }
+ default:
+ break;
+ }
+
+ // Since we have many replacements to make all at once, it's better to start off with a clean
+ // slate, rather than make multiple separate replacements which each require shifting around
+ // the tail of our sourceBuilder. If we do them all at once, we can work forward from the
+ // beginning of the tile, so that each new replacement does not affect any previous
+ // replacements.
+ StringBuilder newContent = new StringBuilder();
+ int positionInOriginal = 0;
+ for (Replacement repl : replacements) {
+ checkArgument(
+ repl.endPosition() <= sourceBuilder.length(),
+ "End [%s] should not exceed source length [%s]",
+ repl.endPosition(),
+ sourceBuilder.length());
+
+ // Write the unmodified content leading up to this change
+ newContent.append(sourceBuilder, positionInOriginal, repl.startPosition());
+ // And the modified content for this change
+ newContent.append(repl.replaceWith());
+ // Then skip everything from source between start and end
+ positionInOriginal = repl.endPosition();
+ }
+ // Flush out any remaining content after the final change
+ newContent.append(sourceBuilder, positionInOriginal, sourceBuilder.length());
+ // Overwrite the contents of our old buffer. Note we mutate the existing StringBuilder rather
+ // than replacing it, because other clients may have a view of the content via getAsSequence,
+ // and we want that view to reflect the new content.
+ setSourceText(newContent);
+ }
}
diff --git a/check_api/src/main/java/com/google/errorprone/dataflow/AccessPathStore.java b/check_api/src/main/java/com/google/errorprone/dataflow/AccessPathStore.java
index 8d9d14f4194..a17548bc8fa 100644
--- a/check_api/src/main/java/com/google/errorprone/dataflow/AccessPathStore.java
+++ b/check_api/src/main/java/com/google/errorprone/dataflow/AccessPathStore.java
@@ -21,6 +21,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableMap;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.LinkedHashMap;
import java.util.Map;
import javax.annotation.Nullable;
@@ -114,6 +115,7 @@ public static final class Builder> {
this.heap = new LinkedHashMap<>(prototype.heap());
}
+ @CanIgnoreReturnValue
public Builder setInformation(AccessPath aPath, V value) {
heap.put(checkNotNull(aPath), checkNotNull(value));
return this;
diff --git a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessPropagationTransfer.java b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessPropagationTransfer.java
index 46509a70f54..3f3f0c6c861 100644
--- a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessPropagationTransfer.java
+++ b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessPropagationTransfer.java
@@ -38,6 +38,7 @@
import com.google.common.io.Files;
import com.google.common.primitives.UnsignedInteger;
import com.google.common.primitives.UnsignedLong;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.dataflow.AccessPath;
import com.google.errorprone.dataflow.AccessPathStore;
import com.google.errorprone.dataflow.AccessPathValues;
@@ -327,6 +328,7 @@ protected NullnessPropagationTransfer(
* Stores the given Javac context to find and analyze field initializers. Set before analyzing a
* method and reset after.
*/
+ @CanIgnoreReturnValue
NullnessPropagationTransfer setContext(@Nullable Context context) {
// This is a best-effort check (similar to ArrayList iterators, for instance), no guarantee
Preconditions.checkArgument(
@@ -345,6 +347,7 @@ NullnessPropagationTransfer setContext(@Nullable Context context) {
* unit. Analyzing initializers from other compilation units tends to fail because type
* information is sometimes missing on nodes returned from {@link Trees}.
*/
+ @CanIgnoreReturnValue
NullnessPropagationTransfer setCompilationUnit(@Nullable CompilationUnitTree compilationUnit) {
this.compilationUnit = compilationUnit;
return this;
diff --git a/check_api/src/main/java/com/google/errorprone/fixes/BranchedSuggestedFixes.java b/check_api/src/main/java/com/google/errorprone/fixes/BranchedSuggestedFixes.java
index b6c26e454d5..850bb2d1de5 100644
--- a/check_api/src/main/java/com/google/errorprone/fixes/BranchedSuggestedFixes.java
+++ b/check_api/src/main/java/com/google/errorprone/fixes/BranchedSuggestedFixes.java
@@ -16,6 +16,7 @@
package com.google.errorprone.fixes;
import com.google.common.collect.ImmutableList;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
/**
* Helper class for accumulating a branching tree of alternative fixes designed to help build as set
@@ -64,12 +65,14 @@ public static class Builder {
private ImmutableList.Builder builder = ImmutableList.builder();
private ImmutableList savedList = ImmutableList.of();
+ @CanIgnoreReturnValue
public Builder startWith(SuggestedFix fix) {
savedList = ImmutableList.of();
builder = ImmutableList.builder().add(fix);
return this;
}
+ @CanIgnoreReturnValue
public Builder addOption(SuggestedFix fix) {
if (!savedList.isEmpty()) {
for (SuggestedFix s : savedList) {
@@ -79,6 +82,7 @@ public Builder addOption(SuggestedFix fix) {
return this;
}
+ @CanIgnoreReturnValue
public Builder then() {
savedList = builder.build();
builder = ImmutableList.builder();
diff --git a/check_api/src/main/java/com/google/errorprone/fixes/Replacements.java b/check_api/src/main/java/com/google/errorprone/fixes/Replacements.java
index b3ff921353f..615495c1bce 100644
--- a/check_api/src/main/java/com/google/errorprone/fixes/Replacements.java
+++ b/check_api/src/main/java/com/google/errorprone/fixes/Replacements.java
@@ -25,6 +25,7 @@
import com.google.common.collect.Range;
import com.google.common.collect.RangeMap;
import com.google.common.collect.TreeRangeMap;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.Collection;
import java.util.Comparator;
import java.util.LinkedHashSet;
@@ -89,10 +90,12 @@ public String coalesce(String replacement, String existing) {
public abstract String coalesce(String replacement, String existing);
}
+ @CanIgnoreReturnValue
public Replacements add(Replacement replacement) {
return add(replacement, CoalescePolicy.REJECT);
}
+ @CanIgnoreReturnValue
public Replacements add(Replacement replacement, CoalescePolicy coalescePolicy) {
if (replacements.containsKey(replacement.range())) {
Replacement existing = replacements.get(replacement.range());
@@ -143,7 +146,12 @@ private void checkOverlaps(Replacement replacement) {
}
}
- /** Non-overlapping replacements, sorted in descending order by position. */
+ /**
+ * Non-overlapping replacements, sorted in descending order by position. Prefer using {@link
+ * #ascending} when applying changes, because applying changes in reverse tends to result in
+ * quadratic-time copying of the underlying string.
+ */
+ @Deprecated
public Set descending() {
// TODO(cushon): refactor SuggestedFix#getReplacements and just return a Collection,
return new LinkedHashSet<>(replacements.values());
@@ -151,8 +159,6 @@ public Set descending() {
/** Non-overlapping replacements, sorted in ascending order by position. */
public ImmutableSet ascending() {
- // TODO(amalloy): Encourage using this instead of descending()
- // Applying replacements in forward order is substantially more efficient, and only a bit harder
return ImmutableSet.copyOf(replacements.descendingMap().values());
}
diff --git a/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFix.java b/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFix.java
index 9b280f364c5..9dec16437aa 100644
--- a/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFix.java
+++ b/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFix.java
@@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.Tree;
@@ -94,7 +95,7 @@ public Set getReplacements(EndPosTable endPositions) {
replacements.add(
fix.getReplacement(endPositions), Replacements.CoalescePolicy.EXISTING_FIRST);
}
- return replacements.descending();
+ return replacements.ascending();
}
/** {@link Builder#replace(Tree, String)} */
@@ -182,6 +183,7 @@ public SuggestedFix build() {
return new SuggestedFix(this);
}
+ @CanIgnoreReturnValue
private Builder with(FixOperation fix) {
fixes.add(fix);
return this;
@@ -193,11 +195,13 @@ private Builder with(FixOperation fix) {
*
* Should be limited to one sentence.
*/
+ @CanIgnoreReturnValue
public Builder setShortDescription(String shortDescription) {
this.shortDescription = shortDescription;
return this;
}
+ @CanIgnoreReturnValue
public Builder replace(Tree node, String replaceWith) {
checkNotSyntheticConstructor(node);
return with(new ReplacementFix((DiagnosticPosition) node, replaceWith));
@@ -211,6 +215,7 @@ public Builder replace(Tree node, String replaceWith) {
* @param endPos The position at which to end replacing, exclusive
* @param replaceWith The string to replace with
*/
+ @CanIgnoreReturnValue
public Builder replace(int startPos, int endPos, String replaceWith) {
DiagnosticPosition pos = new IndexedPosition(startPos, endPos);
return with(new ReplacementFix(pos, replaceWith));
@@ -230,6 +235,7 @@ public Builder replace(int startPos, int endPos, String replaceWith) {
* @param startPosAdjustment The adjustment to add to the start position (negative is OK)
* @param endPosAdjustment The adjustment to add to the end position (negative is OK)
*/
+ @CanIgnoreReturnValue
public Builder replace(
Tree node, String replaceWith, int startPosAdjustment, int endPosAdjustment) {
checkNotSyntheticConstructor(node);
@@ -239,21 +245,25 @@ public Builder replace(
replaceWith));
}
+ @CanIgnoreReturnValue
public Builder prefixWith(Tree node, String prefix) {
checkNotSyntheticConstructor(node);
return with(new PrefixInsertion((DiagnosticPosition) node, prefix));
}
+ @CanIgnoreReturnValue
public Builder postfixWith(Tree node, String postfix) {
checkNotSyntheticConstructor(node);
return with(new PostfixInsertion((DiagnosticPosition) node, postfix));
}
+ @CanIgnoreReturnValue
public Builder delete(Tree node) {
checkNotSyntheticConstructor(node);
return replace(node, "");
}
+ @CanIgnoreReturnValue
public Builder swap(Tree node1, Tree node2) {
checkNotSyntheticConstructor(node1);
checkNotSyntheticConstructor(node2);
@@ -268,6 +278,7 @@ public Builder swap(Tree node1, Tree node2) {
* Add an import statement as part of this SuggestedFix. Import string should be of the form
* "foo.bar.baz".
*/
+ @CanIgnoreReturnValue
public Builder addImport(String importString) {
importsToAdd.add("import " + importString);
return this;
@@ -277,6 +288,7 @@ public Builder addImport(String importString) {
* Add a static import statement as part of this SuggestedFix. Import string should be of the
* form "foo.bar.baz".
*/
+ @CanIgnoreReturnValue
public Builder addStaticImport(String importString) {
importsToAdd.add("import static " + importString);
return this;
@@ -286,6 +298,7 @@ public Builder addStaticImport(String importString) {
* Remove an import statement as part of this SuggestedFix. Import string should be of the form
* "foo.bar.baz".
*/
+ @CanIgnoreReturnValue
public Builder removeImport(String importString) {
importsToRemove.add("import " + importString);
return this;
@@ -295,6 +308,7 @@ public Builder removeImport(String importString) {
* Remove a static import statement as part of this SuggestedFix. Import string should be of the
* form "foo.bar.baz".
*/
+ @CanIgnoreReturnValue
public Builder removeStaticImport(String importString) {
importsToRemove.add("import static " + importString);
return this;
@@ -303,6 +317,7 @@ public Builder removeStaticImport(String importString) {
/**
* Merges all edits from {@code other} into {@code this}. If {@code other} is null, do nothing.
*/
+ @CanIgnoreReturnValue
public Builder merge(@Nullable Builder other) {
if (other == null) {
return this;
@@ -319,6 +334,7 @@ public Builder merge(@Nullable Builder other) {
/**
* Merges all edits from {@code other} into {@code this}. If {@code other} is null, do nothing.
*/
+ @CanIgnoreReturnValue
public Builder merge(@Nullable SuggestedFix other) {
if (other == null) {
return this;
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 d2cd507ce26..e5655402cda 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
@@ -567,7 +567,7 @@ public static void qualifyDocReference(
* parentheses if no elements are left.
*/
public static SuggestedFix removeElement(
- ExpressionTree tree, List trees, VisitorState state) {
+ Tree tree, List trees, VisitorState state) {
int indexOf = trees.indexOf(tree);
checkArgument(indexOf != -1, "trees must contain tree");
if (trees.size() == 1) {
@@ -926,7 +926,6 @@ private static int getThrowsPosition(MethodTree tree, VisitorState state) {
*
* @see #addSuppressWarnings(VisitorState, String, String)
*/
- @Nullable
public static SuggestedFix addSuppressWarnings(VisitorState state, String warningToSuppress) {
return addSuppressWarnings(state, warningToSuppress, null);
}
@@ -942,14 +941,16 @@ public static SuggestedFix addSuppressWarnings(VisitorState state, String warnin
*
In the event that a suppressible element couldn't be found (e.g.: the state is pointing at a
* CompilationUnit, or some other internal inconsistency has occurred), or the enclosing
* suppressible element already has a {@code @SuppressWarnings} annotation with {@code
- * warningToSuppress}, this method will return null.
+ * warningToSuppress}, this method will throw an {@link IllegalArgumentException}.
*/
- @Nullable
public static SuggestedFix addSuppressWarnings(
VisitorState state, String warningToSuppress, @Nullable String lineComment) {
SuggestedFix.Builder fixBuilder = SuggestedFix.builder();
addSuppressWarnings(fixBuilder, state, warningToSuppress, lineComment);
- return fixBuilder.isEmpty() ? null : fixBuilder.build();
+ if (fixBuilder.isEmpty()) {
+ throw new IllegalArgumentException("Couldn't find a node to attach @SuppressWarnings.");
+ }
+ return fixBuilder.build();
}
/**
diff --git a/check_api/src/main/java/com/google/errorprone/matchers/Description.java b/check_api/src/main/java/com/google/errorprone/matchers/Description.java
index 8ca94524dd3..0df00249505 100644
--- a/check_api/src/main/java/com/google/errorprone/matchers/Description.java
+++ b/check_api/src/main/java/com/google/errorprone/matchers/Description.java
@@ -24,6 +24,7 @@
import com.google.common.collect.ImmutableList;
import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.SeverityLevel;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.annotations.CheckReturnValue;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.SuggestedFix;
@@ -174,6 +175,7 @@ private Builder(
* @param fix a suggested fix for this problem
* @throws NullPointerException if {@code fix} is {@code null}
*/
+ @CanIgnoreReturnValue
public Builder addFix(Fix fix) {
checkNotNull(fix, "fix must not be null");
if (!fix.isEmpty()) {
@@ -190,6 +192,7 @@ public Builder addFix(Fix fix) {
* @throws NullPointerException if {@code fix} is {@code null}
* @deprecated prefer referring to empty fixes using {@link SuggestedFix#emptyFix()}.
*/
+ @CanIgnoreReturnValue
@Deprecated
public Builder addFix(Optional fix) {
checkNotNull(fix, "fix must not be null");
@@ -203,6 +206,7 @@ public Builder addFix(Optional fix) {
* @param fixes a list of suggested fixes for this problem
* @throws NullPointerException if {@code fixes} or any of its elements are {@code null}
*/
+ @CanIgnoreReturnValue
public Builder addAllFixes(List fixes) {
checkNotNull(fixes, "fixes must not be null");
for (Fix fix : fixes) {
@@ -217,6 +221,7 @@ public Builder addAllFixes(List fixes) {
*
* @param message A custom error message without the check name ("[checkname]") or link
*/
+ @CanIgnoreReturnValue
public Builder setMessage(String message) {
checkNotNull(message, "message must not be null");
this.rawMessage = message;
@@ -227,6 +232,7 @@ public Builder setMessage(String message) {
* Set a custom link URL. The custom URL will be used instead of the default one which forms
* part of the {@code @}BugPattern.
*/
+ @CanIgnoreReturnValue
public Builder setLinkUrl(String linkUrl) {
checkNotNull(linkUrl, "linkUrl must not be null");
this.linkUrl = linkUrl;
diff --git a/check_api/src/main/java/com/google/errorprone/matchers/JUnitMatchers.java b/check_api/src/main/java/com/google/errorprone/matchers/JUnitMatchers.java
index 80fc2a7b3e0..85b98bb1bac 100644
--- a/check_api/src/main/java/com/google/errorprone/matchers/JUnitMatchers.java
+++ b/check_api/src/main/java/com/google/errorprone/matchers/JUnitMatchers.java
@@ -37,8 +37,8 @@
import static com.google.errorprone.matchers.Matchers.nestingKind;
import static com.google.errorprone.matchers.Matchers.not;
import static com.google.errorprone.suppliers.Suppliers.VOID_TYPE;
-import static com.google.errorprone.util.ASTHelpers.findSuperMethods;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.streamSuperMethods;
import static javax.lang.model.element.NestingKind.TOP_LEVEL;
import com.google.common.collect.ImmutableList;
@@ -86,8 +86,7 @@ public static boolean hasJUnitAnnotation(MethodTree tree, VisitorState state) {
if (hasJUnitAttr(methodSym)) {
return true;
}
- return findSuperMethods(methodSym, state.getTypes()).stream()
- .anyMatch(JUnitMatchers::hasJUnitAttr);
+ return streamSuperMethods(methodSym, state.getTypes()).anyMatch(JUnitMatchers::hasJUnitAttr);
}
/** Checks if a method symbol has any attribute from the org.junit package. */
diff --git a/check_api/src/main/java/com/google/errorprone/matchers/TestNgMatchers.java b/check_api/src/main/java/com/google/errorprone/matchers/TestNgMatchers.java
index e820e00e2b3..b5b0f9ca678 100644
--- a/check_api/src/main/java/com/google/errorprone/matchers/TestNgMatchers.java
+++ b/check_api/src/main/java/com/google/errorprone/matchers/TestNgMatchers.java
@@ -15,8 +15,8 @@
*/
package com.google.errorprone.matchers;
-import static com.google.errorprone.util.ASTHelpers.findSuperMethods;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.streamSuperMethods;
import com.google.errorprone.VisitorState;
import com.sun.source.tree.ClassTree;
@@ -44,8 +44,7 @@ public static boolean hasTestNgAnnotation(MethodTree tree, VisitorState state) {
if (hasTestNgAttr(methodSym)) {
return true;
}
- return findSuperMethods(methodSym, state.getTypes()).stream()
- .anyMatch(TestNgMatchers::hasTestNgAttr);
+ return streamSuperMethods(methodSym, state.getTypes()).anyMatch(TestNgMatchers::hasTestNgAttr);
}
/** Checks if a class is annotated with any annotation from the org.testng package. */
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 7ff84c44ac9..cb14b34bc62 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
@@ -703,11 +703,20 @@ public static MethodSymbol findSuperMethodInType(
return null;
}
+ /**
+ * Finds supermethods of {@code methodSymbol}, not including {@code methodSymbol} itself, and
+ * including interfaces.
+ */
public static Set findSuperMethods(MethodSymbol methodSymbol, Types types) {
return findSuperMethods(methodSymbol, types, /* skipInterfaces= */ false)
.collect(toCollection(LinkedHashSet::new));
}
+ /** See {@link #findSuperMethods(MethodSymbol, Types)}. */
+ public static Stream streamSuperMethods(MethodSymbol methodSymbol, Types types) {
+ return findSuperMethods(methodSymbol, types, /* skipInterfaces= */ false);
+ }
+
private static Stream findSuperMethods(
MethodSymbol methodSymbol, Types types, boolean skipInterfaces) {
TypeSymbol owner = (TypeSymbol) methodSymbol.owner;
@@ -1752,6 +1761,7 @@ public Type visitAnnotation(AnnotationTree tree, Void unused) {
return null;
}
+ @Nullable
@Override
public Type visitCase(CaseTree tree, Void unused) {
Tree t = parent.getParentPath().getLeaf();
@@ -1856,6 +1866,7 @@ public Type visitReturn(ReturnTree tree, Void unused) {
throw new AssertionError("return not enclosed by method or lambda");
}
+ @Nullable
@Override
public Type visitSynchronized(SynchronizedTree node, Void unused) {
// The null occurs if you've asked for the type of the parentheses around the expression.
@@ -2146,7 +2157,6 @@ public static ImmutableSet getThrownExceptions(Tree tree, VisitorState sta
/** Scanner for determining what types are thrown by a tree. */
public static final class ScanThrownTypes extends TreeScanner {
- boolean inResources = false;
ArrayDeque> thrownTypes = new ArrayDeque<>();
SetMultimap thrownTypesByVariable = HashMultimap.create();
@@ -2204,9 +2214,15 @@ public Void visitTry(TryTree tree, Void unused) {
}
public void scanResources(TryTree tree) {
- inResources = true;
+ for (Tree resource : tree.getResources()) {
+ Symbol symbol = getType(resource).tsym;
+
+ if (symbol instanceof ClassSymbol) {
+ getCloseMethod((ClassSymbol) symbol, state)
+ .ifPresent(methodSymbol -> getThrownTypes().addAll(methodSymbol.getThrownTypes()));
+ }
+ }
scan(tree.getResources(), null);
- inResources = false;
}
@Override
@@ -2224,22 +2240,12 @@ public Void visitThrow(ThrowTree tree, Void unused) {
@Override
public Void visitNewClass(NewClassTree tree, Void unused) {
- MethodSymbol symbol = getSymbol(tree);
- if (symbol != null) {
- getThrownTypes().addAll(symbol.getThrownTypes());
- }
+ getThrownTypes().addAll(getSymbol(tree).getThrownTypes());
return super.visitNewClass(tree, null);
}
@Override
public Void visitVariable(VariableTree tree, Void unused) {
- if (inResources) {
- Symbol symbol = getSymbol(tree.getType());
- if (symbol instanceof ClassSymbol) {
- getCloseMethod((ClassSymbol) symbol, state)
- .ifPresent(methodSymbol -> getThrownTypes().addAll(methodSymbol.getThrownTypes()));
- }
- }
return super.visitVariable(tree, null);
}
diff --git a/check_api/src/main/java/com/google/errorprone/util/Commented.java b/check_api/src/main/java/com/google/errorprone/util/Commented.java
index 529f5a97c80..3b1282142cd 100644
--- a/check_api/src/main/java/com/google/errorprone/util/Commented.java
+++ b/check_api/src/main/java/com/google/errorprone/util/Commented.java
@@ -18,6 +18,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.parser.Tokens.Comment;
@@ -51,6 +52,7 @@ abstract static class Builder {
protected abstract ImmutableList.Builder afterCommentsBuilder();
+ @CanIgnoreReturnValue
Builder addComment(
Comment comment, int nodePosition, int tokenizingOffset, Position position) {
OffsetComment offsetComment = new OffsetComment(comment, tokenizingOffset);
@@ -67,6 +69,7 @@ Builder addComment(
return this;
}
+ @CanIgnoreReturnValue
Builder addAllComment(
Iterable comments,
int nodePosition,
diff --git a/check_api/src/main/java/com/google/errorprone/util/ErrorProneScope.java b/check_api/src/main/java/com/google/errorprone/util/ErrorProneScope.java
index 6d2c845c6ae..0a9bc79197e 100644
--- a/check_api/src/main/java/com/google/errorprone/util/ErrorProneScope.java
+++ b/check_api/src/main/java/com/google/errorprone/util/ErrorProneScope.java
@@ -27,6 +27,7 @@
import java.lang.reflect.Proxy;
import java.util.Arrays;
import java.util.function.Predicate;
+import org.checkerframework.checker.nullness.qual.Nullable;
/** A compatibility wrapper around {@code com.sun.tools.javac.util.Filter} */
public final class ErrorProneScope {
@@ -59,7 +60,7 @@ public boolean anyMatch(Predicate predicate) {
private static final Class FILTER_CLASS = getFilterClass();
- private static Class getFilterClass() {
+ private static @Nullable Class getFilterClass() {
if (RuntimeVersion.isAtLeast17()) {
return null;
}
diff --git a/check_api/src/main/java/com/google/errorprone/util/FindIdentifiers.java b/check_api/src/main/java/com/google/errorprone/util/FindIdentifiers.java
index d644a5fbbb8..76dfca39cc5 100644
--- a/check_api/src/main/java/com/google/errorprone/util/FindIdentifiers.java
+++ b/check_api/src/main/java/com/google/errorprone/util/FindIdentifiers.java
@@ -132,7 +132,7 @@ private static ClassTree getEnclosingClass(TreePath treePath) {
return (ClassTree) treePath.getLeaf();
}
- while (treePath != null) {
+ while (true) {
TreePath parent = treePath.getParentPath();
if (parent == null) {
return null;
@@ -144,7 +144,6 @@ private static ClassTree getEnclosingClass(TreePath treePath) {
}
treePath = parent;
}
- return null;
}
/**
@@ -486,12 +485,6 @@ private static boolean inStaticContext(TreePath path) {
break;
case METHOD_INVOCATION: // JLS 8.8.7.1 explicit constructor invocation
MethodSymbol methodSym = ASTHelpers.getSymbol((MethodInvocationTree) tree);
- if (methodSym == null) {
- // sometimes javac can't resolve the symbol. In this case just assume that we are
- // in a static context - this is a safe approximation in our context (checking
- // visibility)
- return true;
- }
if (methodSym.isConstructor()
&& (Objects.equals(methodSym.owner, enclosingClass)
|| Objects.equals(methodSym.owner, directSuperClass))) {
diff --git a/check_api/src/main/java/com/google/errorprone/util/Reachability.java b/check_api/src/main/java/com/google/errorprone/util/Reachability.java
index 1d34f65f658..67d31bf7d9c 100644
--- a/check_api/src/main/java/com/google/errorprone/util/Reachability.java
+++ b/check_api/src/main/java/com/google/errorprone/util/Reachability.java
@@ -167,9 +167,6 @@ private static boolean isSystemExit(ExpressionTree expression) {
return false;
}
MethodSymbol sym = getSymbol((MethodInvocationTree) expression);
- if (sym == null) {
- return false;
- }
return sym.owner.getQualifiedName().contentEquals("java.lang.System")
&& sym.getSimpleName().contentEquals("exit");
}
diff --git a/check_api/src/test/java/com/google/errorprone/apply/ImportStatementsTest.java b/check_api/src/test/java/com/google/errorprone/apply/ImportStatementsTest.java
index 559a874e3be..b1a28b9a528 100644
--- a/check_api/src/test/java/com/google/errorprone/apply/ImportStatementsTest.java
+++ b/check_api/src/test/java/com/google/errorprone/apply/ImportStatementsTest.java
@@ -20,6 +20,7 @@
import static org.junit.Assert.assertThrows;
import com.google.common.collect.ImmutableList;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.sun.source.tree.TreeVisitor;
import com.sun.tools.javac.tree.EndPosTable;
import com.sun.tools.javac.tree.JCTree;
@@ -97,6 +98,7 @@ private static class StubImportBuilder {
* @param typeName the fully-qualified name of the type being imported
* @return a new JCImport stub
*/
+ @CanIgnoreReturnValue
StubImportBuilder addImport(String typeName) {
return addImport(typeName, /* isStatic= */ false);
}
@@ -107,6 +109,7 @@ StubImportBuilder addImport(String typeName) {
* @param typeName the fully-qualified name of the type being imported
* @return a new JCImport stub
*/
+ @CanIgnoreReturnValue
StubImportBuilder addStaticImport(String typeName) {
return addImport(typeName, /* isStatic= */ true);
}
@@ -118,6 +121,7 @@ StubImportBuilder addStaticImport(String typeName) {
* @param isStatic whether the import is static
* @return a new JCImport stub
*/
+ @CanIgnoreReturnValue
private StubImportBuilder addImport(String typeName, boolean isStatic) {
// craft import string
StringBuilder returnSB = new StringBuilder("import ");
diff --git a/check_api/src/test/java/com/google/errorprone/fixes/ReplacementsTest.java b/check_api/src/test/java/com/google/errorprone/fixes/ReplacementsTest.java
index 3117d66533d..8ef3a0901a6 100644
--- a/check_api/src/test/java/com/google/errorprone/fixes/ReplacementsTest.java
+++ b/check_api/src/test/java/com/google/errorprone/fixes/ReplacementsTest.java
@@ -69,24 +69,24 @@ public Range apply(Replacement replacement) {
};
@Test
- public void descending() {
+ public void ascending() {
assertThat(
Iterables.transform(
new Replacements()
.add(Replacement.create(0, 0, "hello"))
.add(Replacement.create(0, 1, "hello"))
- .descending(),
+ .ascending(),
AS_RANGES))
- .containsExactly(Range.closedOpen(0, 1), Range.closedOpen(0, 0))
+ .containsExactly(Range.closedOpen(0, 0), Range.closedOpen(0, 1))
.inOrder();
assertThat(
Iterables.transform(
new Replacements()
.add(Replacement.create(0, 1, "hello"))
.add(Replacement.create(0, 0, "hello"))
- .descending(),
+ .ascending(),
AS_RANGES))
- .containsExactly(Range.closedOpen(0, 1), Range.closedOpen(0, 0))
+ .containsExactly(Range.closedOpen(0, 0), Range.closedOpen(0, 1))
.inOrder();
}
diff --git a/core/pom.xml b/core/pom.xml
index a968768423a..49a2d07114a 100644
--- a/core/pom.xml
+++ b/core/pom.xml
@@ -21,7 +21,7 @@
com.google.errorprone
error_prone_parent
- HEAD-SNAPSHOT
+ 2.15.0
error-prone library
@@ -128,7 +128,7 @@
- com.google.gwt
+ org.gwtproject
gwt-user
${gwt.version}
test
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractJUnit4InitMethodNotRun.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractJUnit4InitMethodNotRun.java
index cec111f45c8..7d32a5c7628 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractJUnit4InitMethodNotRun.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractJUnit4InitMethodNotRun.java
@@ -37,6 +37,7 @@
import java.io.Serializable;
import java.util.List;
import javax.lang.model.element.Modifier;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* Base class for JUnit4SetUp/TearDown not run. This will take care of the nitty-gritty about
@@ -139,7 +140,7 @@ private static void makeProtectedPublic(
}
}
- private Description tryToReplaceAnnotation(
+ private @Nullable Description tryToReplaceAnnotation(
MethodTree methodTree, VisitorState state, String badAnnotation, String goodAnnotation) {
String finalName = getUnqualifiedClassName(goodAnnotation);
if (hasAnnotation(badAnnotation).matches(methodTree, 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 1c706eecc9f..78df7fc2be3 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java
@@ -45,6 +45,7 @@
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.UnusedReturnValueMatcher;
import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.AssignmentTree;
import com.sun.source.tree.BlockTree;
import com.sun.source.tree.ConditionalExpressionTree;
import com.sun.source.tree.ExpressionStatementTree;
@@ -245,7 +246,7 @@ private Description emptyFix(Tree tree) {
private static boolean variableInitializationCountsAsClosing(VarSymbol var) {
// static final fields don't need to be closed, because they never leave scope
- return var.isStatic() && var.getModifiers().contains(Modifier.FINAL);
+ return (var.isStatic() || var.owner.isEnum()) && var.getModifiers().contains(Modifier.FINAL);
}
// We allow calling @MBC methods anywhere inside of a static initializer. This is a compromise:
@@ -258,8 +259,12 @@ private static boolean isInStaticInitializer(VisitorState state) {
return Streams.stream(state.getPath())
.anyMatch(
tree ->
- tree instanceof VariableTree
- && variableInitializationCountsAsClosing((VarSymbol) getSymbol(tree)));
+ (tree instanceof VariableTree
+ && variableInitializationCountsAsClosing((VarSymbol) getSymbol(tree)))
+ || (tree instanceof AssignmentTree
+ && getSymbol(((AssignmentTree) tree).getVariable()) instanceof VarSymbol
+ && variableInitializationCountsAsClosing(
+ (VarSymbol) getSymbol(((AssignmentTree) tree).getVariable()))));
}
/**
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java
index f8d31064911..0d391209e0d 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java
@@ -201,6 +201,13 @@ protected boolean allowInExceptionThrowers() {
*/
protected Description describeReturnValueIgnored(
MethodInvocationTree methodInvocationTree, VisitorState state) {
+ return buildDescription(methodInvocationTree)
+ .addFix(makeFix(methodInvocationTree, state))
+ .setMessage(getMessage(getSymbol(methodInvocationTree).getSimpleName()))
+ .build();
+ }
+
+ final Fix makeFix(MethodInvocationTree methodInvocationTree, VisitorState state) {
// Find the root of the field access chain, i.e. a.intern().trim() ==> a.
ExpressionTree identifierExpr = ASTHelpers.getRootAssignable(methodInvocationTree);
Type identifierType = null;
@@ -236,10 +243,7 @@ protected Description describeReturnValueIgnored(
fix = SuggestedFix.delete(parent);
}
}
- return buildDescription(methodInvocationTree)
- .addFix(fix)
- .setMessage(getMessage(getSymbol(methodInvocationTree).getSimpleName()))
- .build();
+ return fix;
}
/**
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AlwaysThrows.java b/core/src/main/java/com/google/errorprone/bugpatterns/AlwaysThrows.java
index f1c30c268c6..28665ea5b38 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/AlwaysThrows.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/AlwaysThrows.java
@@ -48,6 +48,7 @@
import java.lang.reflect.InvocationTargetException;
import java.util.UUID;
import java.util.function.Consumer;
+import org.checkerframework.checker.nullness.qual.Nullable;
/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@BugPattern(summary = "Detects calls that will fail at runtime", severity = ERROR)
@@ -219,7 +220,7 @@ private Description checkImmutableMapOf(
return checkForRepeatedKeys(tree, keys);
}
- private Object getConstantKey(ExpressionTree key, VisitorState state) {
+ private @Nullable Object getConstantKey(ExpressionTree key, VisitorState state) {
return constantExpressions.constantExpression(key, state).orElse(null);
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/CannotMockFinalMethod.java b/core/src/main/java/com/google/errorprone/bugpatterns/CannotMockFinalMethod.java
new file mode 100644
index 00000000000..68c1a74bb10
--- /dev/null
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/CannotMockFinalMethod.java
@@ -0,0 +1,64 @@
+/*
+ * Copyright 2022 The Error Prone Authors.
+ *
+ * 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;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.matchers.Matchers.staticMethod;
+import static com.google.errorprone.util.ASTHelpers.getReceiver;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.tools.javac.code.Flags;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
+
+/** A BugPattern; see the summary */
+@BugPattern(
+ summary = "Mockito cannot mock final methods, and can't detect this at runtime",
+ severity = WARNING)
+public final class CannotMockFinalMethod extends BugChecker implements MethodInvocationTreeMatcher {
+
+ private static final Matcher WHEN =
+ staticMethod().onClass("org.mockito.Mockito").named("when");
+
+ private static final Matcher VERIFY =
+ staticMethod().onClass("org.mockito.Mockito").named("verify");
+
+ @Override
+ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+ if (WHEN.matches(tree, state)) {
+ ExpressionTree firstArgument = tree.getArguments().get(0);
+ if (!(firstArgument instanceof MethodInvocationTree)) {
+ return NO_MATCH;
+ }
+ return describe(tree, getSymbol((MethodInvocationTree) firstArgument));
+ }
+ var receiver = getReceiver(tree);
+ if (receiver != null && VERIFY.matches(receiver, state)) {
+ return describe(tree, getSymbol(tree));
+ }
+ return NO_MATCH;
+ }
+
+ private Description describe(MethodInvocationTree tree, MethodSymbol methodSymbol) {
+ return (methodSymbol.flags() & Flags.FINAL) == 0 ? NO_MATCH : describeMatch(tree);
+ }
+}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java b/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java
index 1d12464638f..051f98534b1 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java
@@ -17,17 +17,22 @@
package com.google.errorprone.bugpatterns;
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+import static com.google.errorprone.bugpatterns.CheckReturnValue.MessageTrailerStyle.NONE;
import static com.google.errorprone.bugpatterns.checkreturnvalue.AutoValueRules.autoBuilders;
import static com.google.errorprone.bugpatterns.checkreturnvalue.AutoValueRules.autoValueBuilders;
import static com.google.errorprone.bugpatterns.checkreturnvalue.AutoValueRules.autoValues;
import static com.google.errorprone.bugpatterns.checkreturnvalue.ExternalCanIgnoreReturnValue.externalIgnoreList;
+import static com.google.errorprone.bugpatterns.checkreturnvalue.ExternalCanIgnoreReturnValue.methodNameAndParams;
+import static com.google.errorprone.bugpatterns.checkreturnvalue.ExternalCanIgnoreReturnValue.surroundingClass;
import static com.google.errorprone.bugpatterns.checkreturnvalue.ProtoRules.mutableProtos;
import static com.google.errorprone.bugpatterns.checkreturnvalue.ProtoRules.protoBuilders;
import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicy.EXPECTED;
import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicy.OPTIONAL;
import static com.google.errorprone.bugpatterns.checkreturnvalue.Rules.globalDefault;
import static com.google.errorprone.bugpatterns.checkreturnvalue.Rules.mapAnnotationSimpleName;
+import static com.google.errorprone.fixes.SuggestedFix.emptyFix;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.getType;
import static com.google.errorprone.util.ASTHelpers.hasDirectAnnotationWithSimpleName;
import com.google.common.collect.ImmutableMap;
@@ -37,29 +42,35 @@
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
+import com.google.errorprone.bugpatterns.checkreturnvalue.PackagesRule;
import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicy;
import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicyEvaluator;
+import com.google.errorprone.fixes.Fix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.ExpressionStatementTree;
import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MemberReferenceTree;
+import com.sun.source.tree.MemberReferenceTree.ReferenceMode;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.NewClassTree;
+import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import com.sun.tools.javac.code.Type;
+import com.sun.tools.javac.code.Type.MethodType;
import java.util.Optional;
import javax.lang.model.element.ElementKind;
-import javax.lang.model.element.Name;
/**
* @author eaftan@google.com (Eddie Aftandilian)
*/
@BugPattern(
altNames = {"ResultOfMethodCallIgnored", "ReturnValueIgnored"},
- summary = "Ignored return value of method that is annotated with @CheckReturnValue",
+ summary = "The result of this call must be used",
severity = ERROR)
public class CheckReturnValue extends AbstractReturnValueIgnored
implements MethodTreeMatcher, ClassTreeMatcher {
@@ -70,26 +81,45 @@ public class CheckReturnValue extends AbstractReturnValueIgnored
static final String CHECK_ALL_CONSTRUCTORS = "CheckReturnValue:CheckAllConstructors";
static final String CHECK_ALL_METHODS = "CheckReturnValue:CheckAllMethods";
+ static final String CRV_PACKAGES = "CheckReturnValue:Packages";
+
+ private final MessageTrailerStyle messageTrailerStyle;
private final Optional constructorPolicy;
private final Optional methodPolicy;
private final ResultUsePolicyEvaluator evaluator;
public CheckReturnValue(ErrorProneFlags flags) {
super(flags);
+ this.messageTrailerStyle =
+ flags
+ .getEnum("CheckReturnValue:MessageTrailerStyle", MessageTrailerStyle.class)
+ .orElse(NONE);
this.constructorPolicy = defaultPolicy(flags, CHECK_ALL_CONSTRUCTORS);
this.methodPolicy = defaultPolicy(flags, CHECK_ALL_METHODS);
- this.evaluator =
- ResultUsePolicyEvaluator.create(
- mapAnnotationSimpleName(CHECK_RETURN_VALUE, EXPECTED),
- mapAnnotationSimpleName(CAN_IGNORE_RETURN_VALUE, OPTIONAL),
- protoBuilders(),
- mutableProtos(),
- autoValues(),
- autoValueBuilders(),
- autoBuilders(),
- externalIgnoreList(),
- globalDefault(methodPolicy, constructorPolicy));
+ ResultUsePolicyEvaluator.Builder builder =
+ ResultUsePolicyEvaluator.builder()
+ .addRules(
+ // The order of these rules matters somewhat because when checking a method, we'll
+ // evaluate them in the order they're listed here and stop as soon as one of them
+ // returns a result. The order shouldn't matter because most of these, with the
+ // exception of perhaps the external ignore list, are equivalent in importance and
+ // we should be checking declarations to ensure they aren't producing differing
+ // results (i.e. ensuring an @AutoValue.Builder setter method isn't annotated @CRV).
+ mapAnnotationSimpleName(CHECK_RETURN_VALUE, EXPECTED),
+ mapAnnotationSimpleName(CAN_IGNORE_RETURN_VALUE, OPTIONAL),
+ protoBuilders(),
+ mutableProtos(),
+ autoValues(),
+ autoValueBuilders(),
+ autoBuilders(),
+
+ // This is conceptually lower precedence than the above rules.
+ externalIgnoreList());
+ flags
+ .getList(CRV_PACKAGES)
+ .ifPresent(packagePatterns -> builder.addRule(PackagesRule.fromPatterns(packagePatterns)));
+ this.evaluator = builder.addRule(globalDefault(methodPolicy, constructorPolicy)).build();
}
private static Optional defaultPolicy(ErrorProneFlags flags, String flag) {
@@ -222,25 +252,115 @@ && hasDirectAnnotationWithSimpleName(ASTHelpers.getSymbol(tree), CAN_IGNORE_RETU
return Description.NO_MATCH;
}
+ private Description describeInvocationResultIgnored(
+ Tree tree,
+ String shortCall,
+ String shortCallWithoutNew,
+ MethodSymbol symbol,
+ Fix fix,
+ VisitorState state) {
+ String message =
+ String.format(
+ "The result of `%s` must be used\n"
+ + "If you really don't want to use the result, then assign it to a variable:"
+ + " `var unused = ...`.\n"
+ + "\n"
+ + "If callers of `%s` shouldn't be required to use its result,"
+ + " then annotate it with `@CanIgnoreReturnValue`.\n"
+ + "%s",
+ shortCall, shortCallWithoutNew, apiTrailer(symbol, state));
+ return buildDescription(tree).addFix(fix).setMessage(message).build();
+ }
+
@Override
- protected String getMessage(Name name) {
- return String.format(
- methodPolicy.orElse(OPTIONAL).equals(EXPECTED)
- ? "Ignored return value of '%s', which wasn't annotated with @CanIgnoreReturnValue"
- : "Ignored return value of '%s', which is annotated with @CheckReturnValue",
- name);
+ protected Description describeReturnValueIgnored(MethodInvocationTree tree, VisitorState state) {
+ MethodSymbol symbol = getSymbol(tree);
+ String shortCall = symbol.name + (tree.getArguments().isEmpty() ? "()" : "(...)");
+ String shortCallWithoutNew = shortCall;
+ return describeInvocationResultIgnored(
+ tree, shortCall, shortCallWithoutNew, symbol, makeFix(tree, state), state);
}
@Override
- protected Description describeReturnValueIgnored(NewClassTree newClassTree, VisitorState state) {
- return constructorPolicy.orElse(OPTIONAL).equals(EXPECTED)
- ? buildDescription(newClassTree)
- .setMessage(
- String.format(
- "Ignored return value of '%s', which wasn't annotated with"
- + " @CanIgnoreReturnValue",
- state.getSourceForNode(newClassTree.getIdentifier())))
- .build()
- : super.describeReturnValueIgnored(newClassTree, state);
+ protected Description describeReturnValueIgnored(NewClassTree tree, VisitorState state) {
+ MethodSymbol symbol = getSymbol(tree);
+ String shortCallWithoutNew =
+ state.getSourceForNode(tree.getIdentifier())
+ + (tree.getArguments().isEmpty() ? "()" : "(...)");
+ String shortCall = "new " + shortCallWithoutNew;
+ return describeInvocationResultIgnored(
+ tree, shortCall, shortCallWithoutNew, symbol, emptyFix(), state);
+ }
+
+ @Override
+ protected Description describeReturnValueIgnored(MemberReferenceTree tree, VisitorState state) {
+ MethodSymbol symbol = getSymbol(tree);
+ Type type = state.getTypes().memberType(getType(tree.getQualifierExpression()), symbol);
+ // TODO(cgdecker): There are probably other types than MethodType that we could resolve here
+ String parensAndMaybeEllipsis =
+ type instanceof MethodType && ((MethodType) type).getParameterTypes().isEmpty()
+ ? "()"
+ : "(...)";
+
+ String shortCallWithoutNew;
+ String shortCall;
+ if (tree.getMode() == ReferenceMode.NEW) {
+ shortCallWithoutNew =
+ state.getSourceForNode(tree.getQualifierExpression()) + parensAndMaybeEllipsis;
+ shortCall = "new " + shortCallWithoutNew;
+ } else {
+ shortCallWithoutNew = tree.getName() + parensAndMaybeEllipsis;
+ shortCall = shortCallWithoutNew;
+ }
+
+ String implementedMethod =
+ getType(tree).asElement().getSimpleName()
+ + "."
+ + state.getTypes().findDescriptorSymbol(getType(tree).asElement()).getSimpleName();
+ String methodReference = state.getSourceForNode(tree);
+ String message =
+ String.format(
+ "The result of `%s` must be used\n"
+ + "`%s` acts as an implementation of `%s`.\n"
+ + "— which is a `void` method, so it doesn't use the result of `%s`.\n"
+ + "\n"
+ + "To use the result, you may need to restructure your code.\n"
+ + "\n"
+ + "If you really don't want to use the result, then switch to a lambda that assigns"
+ + " it to a variable: `%s -> { var unused = ...; }`.\n"
+ + "\n"
+ + "If callers of `%s` shouldn't be required to use its result,"
+ + " then annotate it with `@CanIgnoreReturnValue`.\n"
+ + "%s",
+ shortCall,
+ methodReference,
+ implementedMethod,
+ shortCall,
+ parensAndMaybeEllipsis,
+ shortCallWithoutNew,
+ apiTrailer(symbol, state));
+ return buildDescription(tree).setMessage(message).build();
+ }
+
+ private String apiTrailer(MethodSymbol symbol, VisitorState state) {
+ if (symbol.enclClass().isAnonymous()) {
+ // I don't think we have a defined format for members of anonymous classes.
+ return "";
+ }
+ switch (messageTrailerStyle) {
+ case NONE:
+ return "";
+ case API_ERASED_SIGNATURE:
+ return "\n\nFull API: "
+ + surroundingClass(symbol)
+ + "#"
+ + methodNameAndParams(symbol, state.getTypes());
+ }
+ throw new AssertionError();
+ }
+
+ enum MessageTrailerStyle {
+ NONE,
+ API_ERASED_SIGNATURE,
}
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ComplexBooleanConstant.java b/core/src/main/java/com/google/errorprone/bugpatterns/ComplexBooleanConstant.java
index 1863163862a..25110d00998 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/ComplexBooleanConstant.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/ComplexBooleanConstant.java
@@ -29,6 +29,7 @@
import com.sun.source.util.SimpleTreeVisitor;
import com.sun.tools.javac.tree.JCTree.JCLiteral;
import java.util.Objects;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* @author Sumit Bhagwani (bhagwani@google.com)
@@ -53,7 +54,7 @@ public Description matchBinary(BinaryTree tree, VisitorState state) {
.build();
}
- Boolean booleanValue(BinaryTree tree) {
+ @Nullable Boolean booleanValue(BinaryTree tree) {
if (tree.getLeftOperand() instanceof JCLiteral && tree.getRightOperand() instanceof JCLiteral) {
return ASTHelpers.constValue(tree, Boolean.class);
}
@@ -62,7 +63,7 @@ Boolean booleanValue(BinaryTree tree) {
SimpleTreeVisitor boolValue =
new SimpleTreeVisitor() {
@Override
- public Boolean visitLiteral(LiteralTree node, Void unused) {
+ public @Nullable Boolean visitLiteral(LiteralTree node, Void unused) {
if (node.getValue() instanceof Boolean) {
return (Boolean) node.getValue();
}
@@ -70,7 +71,7 @@ public Boolean visitLiteral(LiteralTree node, Void unused) {
}
@Override
- public Boolean visitUnary(UnaryTree node, Void unused) {
+ public @Nullable Boolean visitUnary(UnaryTree node, Void unused) {
Boolean r = node.getExpression().accept(this, null);
if (r == null) {
return null;
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ConstantOverflow.java b/core/src/main/java/com/google/errorprone/bugpatterns/ConstantOverflow.java
index 64a714d3975..62aaef2d276 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/ConstantOverflow.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/ConstantOverflow.java
@@ -110,6 +110,7 @@ private static Fix longFix(ExpressionTree expr, VisitorState state) {
private static final SimpleTreeVisitor CONSTANT_VISITOR =
new SimpleTreeVisitor() {
+ @Nullable
@Override
public Number visitConditionalExpression(ConditionalExpressionTree node, Void p) {
Number ifTrue = node.getTrueExpression().accept(this, null);
@@ -126,6 +127,7 @@ public Number visitParenthesized(ParenthesizedTree node, Void p) {
return node.getExpression().accept(this, null);
}
+ @Nullable
@Override
public Number visitUnary(UnaryTree node, Void p) {
Number value = node.getExpression().accept(this, null);
@@ -139,6 +141,7 @@ public Number visitUnary(UnaryTree node, Void p) {
}
}
+ @Nullable
@Override
public Number visitBinary(BinaryTree node, Void p) {
Number lhs = node.getLeftOperand().accept(this, null);
@@ -170,6 +173,7 @@ public Number visitBinary(BinaryTree node, Void p) {
}
}
+ @Nullable
@Override
public Number visitTypeCast(TypeCastTree node, Void p) {
Number value = node.getExpression().accept(this, null);
@@ -199,6 +203,7 @@ public Number visitLiteral(LiteralTree node, Void unused) {
}
};
+ @Nullable
private static Long unop(Kind kind, long value) {
switch (kind) {
case UNARY_PLUS:
@@ -212,6 +217,7 @@ private static Long unop(Kind kind, long value) {
}
}
+ @Nullable
private static Integer unop(Kind kind, int value) {
switch (kind) {
case UNARY_PLUS:
@@ -225,6 +231,7 @@ private static Integer unop(Kind kind, int value) {
}
}
+ @Nullable
static Long binop(Kind kind, long lhs, long rhs) {
switch (kind) {
case MULTIPLY:
@@ -254,6 +261,7 @@ static Long binop(Kind kind, long lhs, long rhs) {
}
}
+ @Nullable
static Integer binop(Kind kind, int lhs, int rhs) {
switch (kind) {
case MULTIPLY:
@@ -283,6 +291,7 @@ static Integer binop(Kind kind, int lhs, int rhs) {
}
}
+ @Nullable
private static Number cast(TypeKind kind, Number value) {
switch (kind) {
case SHORT:
@@ -300,6 +309,7 @@ private static Number cast(TypeKind kind, Number value) {
}
}
+ @Nullable
private static Number getIntegralConstant(Tree node) {
Number number = ASTHelpers.constValue(node, Number.class);
if (number instanceof Integer || number instanceof Long) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ConstantPatternCompile.java b/core/src/main/java/com/google/errorprone/bugpatterns/ConstantPatternCompile.java
index 4b3bed540cb..9ad76070276 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/ConstantPatternCompile.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/ConstantPatternCompile.java
@@ -53,6 +53,7 @@
import javax.lang.model.element.ElementKind;
import javax.lang.model.element.Modifier;
import javax.lang.model.element.NestingKind;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* Flags variables initialized with {@link java.util.regex.Pattern#compile(String)} calls that could
@@ -206,7 +207,7 @@ public Void visitMemberSelect(MemberSelectTree tree, Void unused) {
}
/** Infer a name when upgrading the {@code Pattern} local to a constant. */
- private static String inferName(VariableTree tree, VisitorState state) {
+ private static @Nullable String inferName(VariableTree tree, VisitorState state) {
String name;
if ((name = fromName(tree)) != null) {
return name;
@@ -221,7 +222,7 @@ private static String inferName(VariableTree tree, VisitorState state) {
}
/** Use the existing local variable's name, unless it's terrible. */
- private static String fromName(VariableTree tree) {
+ private static @Nullable String fromName(VariableTree tree) {
String name = LOWER_CAMEL.to(UPPER_UNDERSCORE, tree.getName().toString());
if (name.length() > 1 && !name.equals("PATTERN")) {
return name;
@@ -235,7 +236,7 @@ private static String fromName(VariableTree tree) {
* e.g. use {@code FOO_PATTERN} for {@code Pattern.compile(FOO)} and {@code
* Pattern.compile(FOO_REGEX)}.
*/
- private static String fromInitializer(VariableTree tree) {
+ private static @Nullable String fromInitializer(VariableTree tree) {
ExpressionTree regex = ((MethodInvocationTree) tree.getInitializer()).getArguments().get(0);
if (!(regex instanceof IdentifierTree)) {
return null;
@@ -256,7 +257,7 @@ private static String fromInitializer(VariableTree tree) {
* If the pattern is only used once in a call to {@code matcher}, and the argument is a local, use
* that local's name. For example, infer {@code FOO_PATTERN} from {@code pattern.matcher(foo)}.
*/
- private static String fromUse(VariableTree tree, VisitorState state) {
+ private static @Nullable String fromUse(VariableTree tree, VisitorState state) {
VarSymbol sym = getSymbol(tree);
ImmutableList.Builder usesBuilder = ImmutableList.builder();
new TreePathScanner() {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DirectInvocationOnMock.java b/core/src/main/java/com/google/errorprone/bugpatterns/DirectInvocationOnMock.java
new file mode 100644
index 00000000000..b8c51c41206
--- /dev/null
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/DirectInvocationOnMock.java
@@ -0,0 +1,184 @@
+/*
+ * Copyright 2022 The Error Prone Authors.
+ *
+ * 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;
+
+import static com.google.common.collect.Streams.stream;
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.fixes.SuggestedFixes.qualifyStaticImport;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.matchers.Matchers.allOf;
+import static com.google.errorprone.matchers.Matchers.anyMethod;
+import static com.google.errorprone.matchers.Matchers.instanceMethod;
+import static com.google.errorprone.matchers.Matchers.receiverOfInvocation;
+import static com.google.errorprone.matchers.Matchers.staticMethod;
+import static com.google.errorprone.util.ASTHelpers.getReceiver;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.MoreAnnotations.getAnnotationValue;
+import static java.lang.String.format;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.sun.source.tree.AssignmentTree;
+import com.sun.source.tree.CompilationUnitTree;
+import com.sun.source.tree.ExpressionStatementTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.TypeCastTree;
+import com.sun.source.tree.VariableTree;
+import com.sun.source.util.TreeScanner;
+import com.sun.tools.javac.code.Flags;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import com.sun.tools.javac.code.Symbol.VarSymbol;
+import java.util.HashSet;
+import java.util.Set;
+
+/** A bugpattern; see the description. */
+@BugPattern(
+ summary =
+ "Methods should not be directly invoked on mocks. Should this be part of a verify(..)"
+ + " call?",
+ severity = WARNING)
+public final class DirectInvocationOnMock extends BugChecker implements CompilationUnitTreeMatcher {
+ @Override
+ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
+ ImmutableSet mocks = findMocks(state);
+ Set methodsCallingRealImplementations = new HashSet<>();
+
+ new SuppressibleTreePathScanner(state) {
+ @Override
+ public Void visitMethodInvocation(MethodInvocationTree tree, Void unused) {
+ if (THEN_CALL_REAL_METHOD.matches(tree, state)) {
+ var receiver = getReceiver(tree);
+ if (receiver != null && WHEN.matches(receiver, state)) {
+ ExpressionTree firstArgument = ((MethodInvocationTree) receiver).getArguments().get(0);
+ var firstArgumentSymbol = getSymbol(firstArgument);
+ if (firstArgumentSymbol instanceof MethodSymbol) {
+ methodsCallingRealImplementations.add((MethodSymbol) firstArgumentSymbol);
+ }
+ }
+ return super.visitMethodInvocation(tree, null);
+ }
+ if (DO_CALL_REAL_METHOD.matches(tree, state)) {
+ var methodSymbol = getSymbol(getCurrentPath().getParentPath().getParentPath().getLeaf());
+ if (methodSymbol instanceof MethodSymbol) {
+ methodsCallingRealImplementations.add((MethodSymbol) methodSymbol);
+ }
+ return super.visitMethodInvocation(tree, null);
+ }
+ if (methodsCallingRealImplementations.contains(getSymbol(tree))) {
+ return super.visitMethodInvocation(tree, null);
+ }
+ if ((getSymbol(tree).flags() & Flags.FINAL) != 0) {
+ return null;
+ }
+ Tree parent =
+ stream(getCurrentPath())
+ .skip(1)
+ .filter(t -> !(t instanceof TypeCastTree))
+ .findFirst()
+ .get();
+ var receiver = getReceiver(tree);
+ if (isMock(receiver)
+ && !(parent instanceof ExpressionTree
+ && WHEN.matches((ExpressionTree) parent, state))) {
+ var description =
+ buildDescription(tree)
+ .setMessage(
+ format(
+ "Methods should not be directly invoked on the mock `%s`. Should this be"
+ + " part of a verify(..) call?",
+ getSymbol(receiver).getSimpleName()));
+ if (getCurrentPath().getParentPath().getLeaf() instanceof ExpressionStatementTree) {
+ var fix = SuggestedFix.builder();
+ String verify = qualifyStaticImport("org.mockito.Mockito.verify", fix, state);
+ description.addFix(
+ fix.replace(receiver, format("%s(%s)", verify, state.getSourceForNode(receiver)))
+ .setShortDescription("turn into verify() call")
+ .build());
+ description.addFix(
+ SuggestedFix.builder()
+ .delete(tree)
+ .setShortDescription("delete redundant invocation")
+ .build());
+ }
+ state.reportMatch(description.build());
+ }
+ return super.visitMethodInvocation(tree, null);
+ }
+
+ private boolean isMock(ExpressionTree tree) {
+ var symbol = getSymbol(tree);
+ return symbol != null
+ && (mocks.contains(symbol)
+ || symbol.getAnnotationMirrors().stream()
+ .filter(am -> am.type.tsym.getQualifiedName().contentEquals("org.mockito.Mock"))
+ .findFirst()
+ .filter(am -> getAnnotationValue(am, "answer").isEmpty())
+ .isPresent());
+ }
+ }.scan(state.getPath(), null);
+
+ return NO_MATCH;
+ }
+
+ private ImmutableSet findMocks(VisitorState state) {
+ ImmutableSet.Builder mocks = ImmutableSet.builder();
+ new TreeScanner() {
+ @Override
+ public Void visitVariable(VariableTree tree, Void unused) {
+ if (tree.getInitializer() != null && MOCK.matches(tree.getInitializer(), state)) {
+ mocks.add(getSymbol(tree));
+ }
+ return super.visitVariable(tree, null);
+ }
+
+ @Override
+ public Void visitAssignment(AssignmentTree tree, Void unused) {
+ if (MOCK.matches(tree.getExpression(), state)) {
+ var symbol = getSymbol(tree.getVariable());
+ if (symbol instanceof VarSymbol) {
+ mocks.add((VarSymbol) symbol);
+ }
+ }
+ return super.visitAssignment(tree, null);
+ }
+ }.scan(state.getPath().getCompilationUnit(), null);
+ return mocks.build();
+ }
+
+ private static final Matcher MOCK =
+ staticMethod().onClass("org.mockito.Mockito").named("mock").withParameters("java.lang.Class");
+
+ private static final Matcher DO_CALL_REAL_METHOD =
+ allOf(
+ instanceMethod().onDescendantOf("org.mockito.stubbing.Stubber").named("when"),
+ receiverOfInvocation(
+ staticMethod().onClass("org.mockito.Mockito").named("doCallRealMethod")));
+
+ private static final Matcher WHEN = anyMethod().anyClass().named("when");
+
+ private static final Matcher THEN_CALL_REAL_METHOD =
+ instanceMethod()
+ .onDescendantOf("org.mockito.stubbing.OngoingStubbing")
+ .named("thenCallRealMethod");
+}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DistinctVarargsChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/DistinctVarargsChecker.java
index 13826a20823..16019469a4c 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/DistinctVarargsChecker.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/DistinctVarargsChecker.java
@@ -20,17 +20,19 @@
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
+import com.google.common.collect.ImmutableListMultimap;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
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.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import java.util.ArrayList;
+import java.util.Collection;
import java.util.List;
+import java.util.Map;
/**
* ErrorProne checker to generate warning when method expecting distinct varargs is invoked with
@@ -72,14 +74,14 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
return checkDistinctArgumentsWithFix(tree, state);
}
if (ALL_DISTINCT_ARG_MATCHER.matches(tree, state)) {
- return checkDistinctArguments(tree, tree.getArguments());
+ return checkDistinctArguments(state, tree.getArguments());
}
if (EVEN_PARITY_DISTINCT_ARG_MATCHER.matches(tree, state)) {
List arguments = new ArrayList<>();
for (int index = 0; index < tree.getArguments().size(); index += 2) {
arguments.add(tree.getArguments().get(index));
}
- return checkDistinctArguments(tree, arguments);
+ return checkDistinctArguments(state, arguments);
}
if (EVEN_AND_ODD_PARITY_DISTINCT_ARG_MATCHER.matches(tree, state)) {
List evenParityArguments = new ArrayList<>();
@@ -91,29 +93,34 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
oddParityArguments.add(tree.getArguments().get(index));
}
}
- return checkDistinctArguments(tree, evenParityArguments, oddParityArguments);
+ return checkDistinctArguments(state, evenParityArguments, oddParityArguments);
}
return Description.NO_MATCH;
}
+ private static ImmutableListMultimap argumentsByString(
+ VisitorState state, List arguments) {
+ ImmutableListMultimap.Builder result = ImmutableListMultimap.builder();
+ for (int i = 0; i < arguments.size(); i++) {
+ result.put(state.getSourceForNode(arguments.get(i)), i);
+ }
+ return result.build();
+ }
+
private Description checkDistinctArgumentsWithFix(MethodInvocationTree tree, VisitorState state) {
SuggestedFix.Builder suggestedFix = SuggestedFix.builder();
- for (int index = 1; index < tree.getArguments().size(); index++) {
- boolean isDistinctArgument = true;
- for (int prevElementIndex = 0; prevElementIndex < index; prevElementIndex++) {
- if (ASTHelpers.sameVariable(
- tree.getArguments().get(index), tree.getArguments().get(prevElementIndex))) {
- isDistinctArgument = false;
- break;
- }
- }
- if (!isDistinctArgument) {
- suggestedFix.merge(
- SuggestedFix.replace(
- state.getEndPosition(tree.getArguments().get(index - 1)),
- state.getEndPosition(tree.getArguments().get(index)),
- ""));
- }
+ List arguments = tree.getArguments();
+ ImmutableListMultimap argumentsByString = argumentsByString(state, arguments);
+ for (Map.Entry> entry : argumentsByString.asMap().entrySet()) {
+ entry.getValue().stream()
+ .skip(1)
+ .forEachOrdered(
+ index ->
+ suggestedFix.merge(
+ SuggestedFix.replace(
+ state.getEndPosition(arguments.get(index - 1)),
+ state.getEndPosition(arguments.get(index)),
+ "")));
}
if (suggestedFix.isEmpty()) {
return Description.NO_MATCH;
@@ -122,19 +129,14 @@ private Description checkDistinctArgumentsWithFix(MethodInvocationTree tree, Vis
}
private Description checkDistinctArguments(
- MethodInvocationTree tree, List... argumentsList) {
+ VisitorState state, List... argumentsList) {
for (List arguments : argumentsList) {
- for (int firstArgumentIndex = 0;
- firstArgumentIndex < arguments.size();
- firstArgumentIndex++) {
- for (int secondArgumentIndex = firstArgumentIndex + 1;
- secondArgumentIndex < arguments.size();
- secondArgumentIndex++) {
- if (ASTHelpers.sameVariable(
- arguments.get(firstArgumentIndex), arguments.get(secondArgumentIndex))) {
- return describeMatch(tree);
- }
- }
+ ImmutableListMultimap argumentsByString =
+ argumentsByString(state, arguments);
+ for (Map.Entry> entry : argumentsByString.asMap().entrySet()) {
+ entry.getValue().stream()
+ .skip(1)
+ .forEachOrdered(index -> state.reportMatch(describeMatch(arguments.get(index))));
}
}
return Description.NO_MATCH;
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DoNotCallChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/DoNotCallChecker.java
index a92c06de47b..cbacb2e14dc 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/DoNotCallChecker.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/DoNotCallChecker.java
@@ -19,6 +19,8 @@
import static com.google.common.collect.Streams.stream;
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.matchers.Matchers.allOf;
+import static com.google.errorprone.matchers.Matchers.not;
import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
import static com.google.errorprone.util.ASTHelpers.findSuperMethods;
@@ -28,11 +30,15 @@
import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
import static com.google.errorprone.util.ASTHelpers.isConsideredFinal;
import static com.google.errorprone.util.ASTHelpers.isSameType;
+import static com.sun.source.tree.Tree.Kind.IDENTIFIER;
+import static com.sun.source.tree.Tree.Kind.MEMBER_SELECT;
+import static com.sun.source.tree.Tree.Kind.METHOD_INVOCATION;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableMap;
import com.google.errorprone.BugPattern;
+import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
@@ -45,7 +51,9 @@
import com.sun.source.tree.AssignmentTree;
import com.sun.source.tree.CompilationUnitTree;
import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.IdentifierTree;
import com.sun.source.tree.MemberReferenceTree;
+import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.VariableTree;
@@ -66,6 +74,39 @@
@BugPattern(name = "DoNotCall", summary = "This method should not be called.", severity = ERROR)
public class DoNotCallChecker extends BugChecker
implements MethodTreeMatcher, CompilationUnitTreeMatcher {
+ private final boolean checkNewGetClassMethods;
+ private final boolean checkThreadRun;
+
+ public DoNotCallChecker(ErrorProneFlags flags) {
+ checkNewGetClassMethods =
+ flags.getBoolean("DoNotCallChecker:CheckNewGetClassMethods").orElse(true);
+ checkThreadRun = flags.getBoolean("DoNotCallChecker:CheckThreadRun").orElse(true);
+ }
+
+ private static final Matcher STACK_TRACE_ELEMENT_GET_CLASS =
+ instanceMethod().onExactClass("java.lang.StackTraceElement").named("getClass");
+
+ private static final Matcher ANY_GET_CLASS =
+ instanceMethod().anyClass().named("getClass");
+
+ private static final Matcher THREAD_RUN =
+ instanceMethod().onDescendantOf("java.lang.Thread").named("run").withNoParameters();
+
+ private static final Matcher CALL_ON_SUPER =
+ (invocation, state) -> {
+ if (invocation.getKind() != METHOD_INVOCATION) {
+ return false;
+ }
+ ExpressionTree select = ((MethodInvocationTree) invocation).getMethodSelect();
+ if (select.getKind() != MEMBER_SELECT) {
+ return false;
+ }
+ ExpressionTree receiver = ((MemberSelectTree) select).getExpression();
+ if (receiver.getKind() != IDENTIFIER) {
+ return false;
+ }
+ return ((IdentifierTree) receiver).getName().contentEquals("super");
+ };
// If your method cannot be annotated with @DoNotCall (e.g., it's a JDK or thirdparty method),
// then add it to this Map with an explanation.
@@ -133,6 +174,90 @@ public class DoNotCallChecker extends BugChecker
"Calling getClass on StackTraceElement returns the Class object for"
+ " StackTraceElement, you probably meant to retrieve the class containing the"
+ " execution point represented by this stack trace element using getClassName")
+ .put(
+ instanceMethod().onExactClass("java.lang.StackWalker").named("getClass"),
+ "Calling getClass on StackWalker returns the Class object for StackWalker, you"
+ + " probably meant to retrieve the class containing the execution point"
+ + " represented by this StackWalker using getCallerClass")
+ .put(
+ instanceMethod().onExactClass("java.lang.StackWalker$StackFrame").named("getClass"),
+ "Calling getClass on StackFrame returns the Class object for StackFrame, you probably"
+ + " meant to retrieve the class containing the execution point represented by"
+ + " this StackFrame using getClassName")
+ .put(
+ instanceMethod().onExactClass("java.lang.reflect.Constructor").named("getClass"),
+ "Calling getClass on Constructor returns the Class object for Constructor, you"
+ + " probably meant to retrieve the class containing the constructor represented"
+ + " by this Constructor using getDeclaringClass")
+ .put(
+ instanceMethod().onExactClass("java.lang.reflect.Field").named("getClass"),
+ "Calling getClass on Field returns the Class object for Field, you probably meant to"
+ + " retrieve the class containing the field represented by this Field using"
+ + " getDeclaringClass")
+ .put(
+ instanceMethod().onExactClass("java.lang.reflect.Method").named("getClass"),
+ "Calling getClass on Method returns the Class object for Method, you probably meant"
+ + " to retrieve the class containing the method represented by this Method using"
+ + " getDeclaringClass")
+ .put(
+ instanceMethod()
+ .onExactClass("java.lang.reflect.ParameterizedType")
+ .named("getClass"),
+ "Calling getClass on ParameterizedType returns the Class object for"
+ + " ParameterizedType, you probably meant to retrieve the class containing the"
+ + " method represented by this ParameterizedType using getRawType")
+ .put(
+ instanceMethod().onExactClass("java.beans.BeanDescriptor").named("getClass"),
+ "Calling getClass on BeanDescriptor returns the Class object for BeanDescriptor, you"
+ + " probably meant to retrieve the class described by this BeanDescriptor using"
+ + " getBeanClass")
+ .put(
+ /*
+ * LockInfo has a publicly visible subclass, MonitorInfo. It seems unlikely that
+ * anyone is using getClass() in an attempt to distinguish the two. (If anyone is,
+ * then it would make more sense to use instanceof, anyway.)
+ */
+ instanceMethod().onDescendantOf("java.lang.management.LockInfo").named("getClass"),
+ "Calling getClass on LockInfo returns the Class object for LockInfo, you probably"
+ + " meant to retrieve the class of the object that is being locked using"
+ + " getClassName")
+ /*
+ * These methods are part of Guava, but we have to list them in this "thirdparty" section:
+ * We can't annotate them with @DoNotCall because we can't override getClass.
+ */
+ .put(
+ instanceMethod()
+ .onExactClass("com.google.common.reflect.ClassPath$ClassInfo")
+ .named("getClass"),
+ "Calling getClass on ClassInfo returns the Class object for ClassInfo, you probably"
+ + " meant to retrieve the class described by this ClassInfo using getName or"
+ + " load")
+ /*
+ * Users of TypeToken have to create a subclass. The static type of their instance is
+ * probably often still "TypeToken," but that may change as we see more usage of `var`. So
+ * let's check subclasses, too. If anyone defines an overload of getClass on such a
+ * subclass, this check will give that person a bad time in one additional way.
+ */
+ .put(
+ instanceMethod()
+ .onDescendantOf("com.google.common.reflect.TypeToken")
+ .named("getClass"),
+ "Calling getClass on TypeToken returns the Class object for TypeToken, you probably"
+ + " meant to retrieve the class described by this TypeToken using getRawType")
+ .put(
+ /*
+ * A call to super.run() from a direct subclass of Thread is a no-op. That could be
+ * worth telling the user about, but it's not as big a deal as "You meant to call
+ * start()," so we ignore it here.
+ *
+ * (And if someone defines a MyThread class with a run() method that does something,
+ * then a call to super.run() from a subclass of *MyThread* would *not* be a no-op,
+ * and we wouldn't want to flag it. Still, *usually* it's likely to be useful to
+ * report a warning even on subclasses of Thread, such as anonymous classes.)
+ */
+ allOf(THREAD_RUN, not(CALL_ON_SUPER)),
+ "Calling run on Thread runs work on this thread, rather than the given thread, you"
+ + " probably meant to call start")
.buildOrThrow();
static final String DO_NOT_CALL = "com.google.errorprone.annotations.DoNotCall";
@@ -201,6 +326,14 @@ public Void visitMemberReference(MemberReferenceTree tree, Void unused) {
private void handleTree(ExpressionTree tree, MethodSymbol symbol) {
for (Map.Entry, String> matcher : THIRD_PARTY_METHODS.entrySet()) {
if (matcher.getKey().matches(tree, state)) {
+ if (!checkNewGetClassMethods
+ && ANY_GET_CLASS.matches(tree, state)
+ && !STACK_TRACE_ELEMENT_GET_CLASS.matches(tree, state)) {
+ return;
+ }
+ if (!checkThreadRun && THREAD_RUN.matches(tree, state)) {
+ return;
+ }
state.reportMatch(buildDescription(tree).setMessage(matcher.getValue()).build());
return;
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ErroneousThreadPoolConstructorChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/ErroneousThreadPoolConstructorChecker.java
index 4e3877392fd..e32128738da 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/ErroneousThreadPoolConstructorChecker.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/ErroneousThreadPoolConstructorChecker.java
@@ -64,6 +64,9 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) {
return Description.NO_MATCH;
}
List arguments = tree.getArguments();
+ if (arguments.size() < 2) {
+ return Description.NO_MATCH;
+ }
Integer corePoolSize = ASTHelpers.constValue(arguments.get(0), Integer.class);
Integer maximumPoolSize = ASTHelpers.constValue(arguments.get(1), Integer.class);
if (corePoolSize == null || maximumPoolSize == null || corePoolSize.equals(maximumPoolSize)) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ForOverrideChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/ForOverrideChecker.java
index 241c47fd322..e25438fcfaf 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/ForOverrideChecker.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/ForOverrideChecker.java
@@ -20,8 +20,8 @@
import static com.google.common.collect.Streams.findLast;
import static com.google.common.collect.Streams.stream;
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
-import static com.google.errorprone.util.ASTHelpers.findSuperMethods;
import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
+import static com.google.errorprone.util.ASTHelpers.streamSuperMethods;
import static java.util.stream.Stream.concat;
import com.google.common.collect.ImmutableList;
@@ -199,12 +199,13 @@ private static ImmutableList getOverriddenMethods(
"getOverriddenMethods may not be called on a static method");
}
- return concat(Stream.of(method), findSuperMethods(method, state.getTypes()).stream())
+ return concat(Stream.of(method), streamSuperMethods(method, state.getTypes()))
.filter(member -> hasAnnotation(member, FOR_OVERRIDE, state))
.collect(toImmutableList());
}
/** Get the outermost class/interface/enum of an element, or null if none. */
+ @Nullable
private static Type getOutermostClass(VisitorState state) {
return findLast(
stream(state.getPath())
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/InfiniteRecursion.java b/core/src/main/java/com/google/errorprone/bugpatterns/InfiniteRecursion.java
index 5056f001cb1..1fb495ef8ca 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/InfiniteRecursion.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/InfiniteRecursion.java
@@ -37,6 +37,7 @@
import com.sun.source.tree.TypeCastTree;
import com.sun.source.util.SimpleTreeVisitor;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import org.checkerframework.checker.nullness.qual.Nullable;
/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@BugPattern(
@@ -74,7 +75,7 @@ public MethodInvocationTree visitTypeCast(TypeCastTree tree, Void unused) {
}
@Override
- protected MethodInvocationTree defaultAction(Tree tree, Void unused) {
+ protected @Nullable MethodInvocationTree defaultAction(Tree tree, Void unused) {
return tree instanceof MethodInvocationTree ? (MethodInvocationTree) tree : null;
}
},
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/IsInstanceIncompatibleType.java b/core/src/main/java/com/google/errorprone/bugpatterns/IsInstanceIncompatibleType.java
index cebd9e8b9ef..8c2d9a92caa 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/IsInstanceIncompatibleType.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/IsInstanceIncompatibleType.java
@@ -38,6 +38,7 @@
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Type;
import java.util.List;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* @author cushon@google.com (Liam Miller-Cushon)
@@ -84,7 +85,7 @@ public Description matchMemberReference(MemberReferenceTree tree, VisitorState s
: buildMessage(argumentType, receiverType, tree, state);
}
- private static Type classTypeArgument(ExpressionTree tree) {
+ private static @Nullable Type classTypeArgument(ExpressionTree tree) {
ExpressionTree receiver = getReceiver(tree);
if (receiver == null) {
return null;
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/JdkObsolete.java b/core/src/main/java/com/google/errorprone/bugpatterns/JdkObsolete.java
index 23176749e12..5806a744ebd 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/JdkObsolete.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/JdkObsolete.java
@@ -354,6 +354,7 @@ public Void visitIdentifier(IdentifierTree tree, Void unused) {
return Optional.of(fix.build());
}
+ @Nullable
private static TreePath findEnclosingMethod(VisitorState state) {
TreePath path = state.getPath();
while (path != null) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/LockNotBeforeTry.java b/core/src/main/java/com/google/errorprone/bugpatterns/LockNotBeforeTry.java
index 03ccbcc379a..d82f87ae2d5 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/LockNotBeforeTry.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/LockNotBeforeTry.java
@@ -39,6 +39,7 @@
import com.sun.source.tree.TryTree;
import com.sun.source.util.TreePath;
import com.sun.source.util.TreeScanner;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* Suggests that calls to {@code Lock.lock} must be immediately followed by a {@code try-finally}
@@ -171,7 +172,7 @@ private static boolean releases(TryTree tryTree, ExpressionTree lockee, VisitorS
Boolean released =
new TreeScanner() {
@Override
- public Boolean reduce(Boolean r1, Boolean r2) {
+ public @Nullable Boolean reduce(Boolean r1, Boolean r2) {
return r1 == null ? r2 : (r2 == null ? null : r1 && r2);
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/LongDoubleConversion.java b/core/src/main/java/com/google/errorprone/bugpatterns/LongDoubleConversion.java
index 24ec06586c6..4bd71ae93fe 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/LongDoubleConversion.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/LongDoubleConversion.java
@@ -18,6 +18,7 @@
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.util.ASTHelpers.constValue;
import static com.google.errorprone.util.ASTHelpers.getType;
import static com.google.errorprone.util.ASTHelpers.targetType;
@@ -39,7 +40,7 @@
"Conversion from long to double may lose precision; use an explicit cast to double if this"
+ " was intentional",
severity = WARNING)
-public class LongDoubleConversion extends BugChecker implements MethodInvocationTreeMatcher {
+public final class LongDoubleConversion extends BugChecker implements MethodInvocationTreeMatcher {
@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
@@ -53,6 +54,10 @@ private void checkArgument(ExpressionTree argument, VisitorState state) {
if (!getType(argument).getKind().equals(TypeKind.LONG)) {
return;
}
+ Object constant = constValue(argument);
+ if (constant instanceof Long && constant.equals((long) ((Long) constant).doubleValue())) {
+ return;
+ }
ASTHelpers.TargetType targetType =
targetType(state.withPath(new TreePath(state.getPath(), argument)));
if (targetType != null && targetType.type().getKind().equals(TypeKind.DOUBLE)) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MethodCanBeStatic.java b/core/src/main/java/com/google/errorprone/bugpatterns/MethodCanBeStatic.java
index db42998f2da..36236efbfc7 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/MethodCanBeStatic.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/MethodCanBeStatic.java
@@ -24,6 +24,7 @@
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
import static java.util.Collections.disjoint;
import static javax.lang.model.element.Modifier.ABSTRACT;
+import static javax.lang.model.element.Modifier.DEFAULT;
import static javax.lang.model.element.Modifier.NATIVE;
import static javax.lang.model.element.Modifier.SYNCHRONIZED;
@@ -250,7 +251,7 @@ private static boolean isExcluded(MethodTree tree, VisitorState state) {
}
private static final ImmutableSet EXCLUDED_MODIFIERS =
- immutableEnumSet(NATIVE, SYNCHRONIZED, ABSTRACT);
+ immutableEnumSet(NATIVE, SYNCHRONIZED, ABSTRACT, DEFAULT);
/** Information about a {@link MethodSymbol} and whether it can be made static. */
private static final class MethodDetails {
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 92964a28a44..f9baf7b1923 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/MissingOverride.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/MissingOverride.java
@@ -18,8 +18,8 @@
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.matchers.Description.NO_MATCH;
-import static com.google.errorprone.util.ASTHelpers.findSuperMethods;
import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
+import static com.google.errorprone.util.ASTHelpers.streamSuperMethods;
import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.StandardTags;
@@ -59,7 +59,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
if (ignoreInterfaceOverrides && sym.enclClass().isInterface()) {
return NO_MATCH;
}
- return findSuperMethods(sym, state.getTypes()).stream()
+ return streamSuperMethods(sym, state.getTypes())
.findFirst()
.filter(unused -> ASTHelpers.getGeneratedBy(state).isEmpty())
// to allow deprecated methods to be removed non-atomically, we permit overrides of
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MockNotUsedInProduction.java b/core/src/main/java/com/google/errorprone/bugpatterns/MockNotUsedInProduction.java
new file mode 100644
index 00000000000..e7a32649353
--- /dev/null
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/MockNotUsedInProduction.java
@@ -0,0 +1,199 @@
+/*
+ * Copyright 2022 The Error Prone Authors.
+ *
+ * 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;
+
+import static com.google.common.collect.Streams.stream;
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.matchers.Matchers.anyMethod;
+import static com.google.errorprone.matchers.Matchers.anyOf;
+import static com.google.errorprone.matchers.Matchers.hasAnnotation;
+import static com.google.errorprone.matchers.Matchers.staticMethod;
+import static com.google.errorprone.util.ASTHelpers.canBeRemoved;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static java.util.stream.Stream.concat;
+import static javax.lang.model.element.ElementKind.FIELD;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.sun.source.tree.AssignmentTree;
+import com.sun.source.tree.CompilationUnitTree;
+import com.sun.source.tree.ExpressionStatementTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.IdentifierTree;
+import com.sun.source.tree.MemberSelectTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.VariableTree;
+import com.sun.source.util.TreePathScanner;
+import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Symbol.VarSymbol;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Stream;
+
+/** A BugPattern; see the summary. */
+@BugPattern(
+ severity = WARNING,
+ summary =
+ "This mock is instantiated and configured, but is never passed to production code. It"
+ + " should be either removed or used.")
+public final class MockNotUsedInProduction extends BugChecker
+ implements CompilationUnitTreeMatcher {
+ @Override
+ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
+ ImmutableMap mocks = findMocks(state);
+ if (mocks.isEmpty()) {
+ return NO_MATCH;
+ }
+ Set usedMocks = new HashSet<>();
+ new TreePathScanner() {
+ @Override
+ public Void visitMethodInvocation(MethodInvocationTree invocation, Void unused) {
+ // Don't count references to mocks within the arguments of a when(...) call to be a usage.
+ // We still need to scan the receiver for the case of
+ // `doReturn(someMockWhichIsAUsage).when(aMockWhichIsNotAUsage);`
+ if (WHEN_OR_VERIFY.matches(invocation, state)) {
+ scan(invocation.getMethodSelect(), null);
+ return null;
+ }
+ return super.visitMethodInvocation(invocation, null);
+ }
+
+ @Override
+ public Void visitMemberSelect(MemberSelectTree memberSelect, Void unused) {
+ handle(memberSelect);
+ return super.visitMemberSelect(memberSelect, null);
+ }
+
+ @Override
+ public Void visitIdentifier(IdentifierTree identifier, Void unused) {
+ handle(identifier);
+ return super.visitIdentifier(identifier, null);
+ }
+
+ private void handle(Tree tree) {
+ var symbol = getSymbol(tree);
+ if (symbol instanceof VarSymbol) {
+ usedMocks.add((VarSymbol) symbol);
+ }
+ }
+ }.scan(state.getPath(), null);
+ mocks.forEach(
+ (sym, mockTree) -> {
+ if (usedMocks.contains(sym)) {
+ return;
+ }
+ state.reportMatch(describeMatch(mockTree, generateFix(sym, state)));
+ });
+ return NO_MATCH;
+ }
+
+ /**
+ * Very crudely deletes every variable or expression statement which contains a reference to
+ * {@code sym}. This is inefficient insofar as we scan the entire file again, but only when
+ * generating a fix.
+ */
+ private static SuggestedFix generateFix(VarSymbol sym, VisitorState state) {
+ SuggestedFix.Builder fix = SuggestedFix.builder();
+ new TreePathScanner() {
+
+ @Override
+ public Void scan(Tree tree, Void unused) {
+ if (Objects.equals(getSymbol(tree), sym)) {
+ // Yes, at this point, the current path hasn't been updated to include `tree`...
+ concat(Stream.of(tree), stream(getCurrentPath()))
+ .filter(t -> t instanceof ExpressionStatementTree || t instanceof VariableTree)
+ .findFirst()
+ .ifPresent(fix::delete);
+ }
+ return super.scan(tree, null);
+ }
+ }.scan(state.getPath().getCompilationUnit(), null);
+ return fix.build();
+ }
+
+ private ImmutableMap findMocks(VisitorState state) {
+ Map mocks = new HashMap<>();
+ AtomicBoolean injectMocks = new AtomicBoolean(false);
+ new SuppressibleTreePathScanner(state) {
+ @Override
+ public Void visitVariable(VariableTree tree, Void unused) {
+ VarSymbol symbol = getSymbol(tree);
+ if (INJECT_MOCKS_ANNOTATED.matches(tree, state)) {
+ injectMocks.set(true);
+ }
+ if (isEligible(symbol)
+ && (MOCK_OR_SPY_ANNOTATED.matches(tree, state)
+ || (tree.getInitializer() != null && MOCK.matches(tree.getInitializer(), state)))) {
+ mocks.put(symbol, tree);
+ }
+ return super.visitVariable(tree, null);
+ }
+
+ @Override
+ public Void visitAssignment(AssignmentTree tree, Void unused) {
+ if (MOCK.matches(tree.getExpression(), state)) {
+ var symbol = getSymbol(tree.getVariable());
+ if (isEligible(symbol)) {
+ mocks.put((VarSymbol) symbol, tree);
+ }
+ }
+ return super.visitAssignment(tree, null);
+ }
+
+ private boolean isEligible(Symbol symbol) {
+ return symbol instanceof VarSymbol
+ && (!symbol.getKind().equals(FIELD) || canBeRemoved((VarSymbol) symbol))
+ && annotatedAtMostMock(symbol);
+ }
+
+ private boolean annotatedAtMostMock(Symbol symbol) {
+ return symbol.getAnnotationMirrors().stream()
+ .allMatch(a -> a.getAnnotationType().asElement().getSimpleName().contentEquals("Mock"));
+ }
+ }.scan(state.getPath().getCompilationUnit(), null);
+ // A bit hacky: but if we saw InjectMocks, just claim there are no potentially unused mocks.
+ return injectMocks.get() ? ImmutableMap.of() : ImmutableMap.copyOf(mocks);
+ }
+
+ private static final Matcher MOCK =
+ anyOf(
+ staticMethod()
+ .onClass("org.mockito.Mockito")
+ .namedAnyOf("mock")
+ .withParameters("java.lang.Class"),
+ staticMethod().onClass("org.mockito.Mockito").namedAnyOf("spy"));
+
+ private static final Matcher MOCK_OR_SPY_ANNOTATED =
+ anyOf(hasAnnotation("org.mockito.Mock"), hasAnnotation("org.mockito.Spy"));
+
+ private static final Matcher INJECT_MOCKS_ANNOTATED =
+ hasAnnotation("org.mockito.InjectMocks");
+
+ private static final Matcher WHEN_OR_VERIFY =
+ anyMethod().anyClass().namedAnyOf("when", "verify");
+}
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 1b260511a78..611fead87fa 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/NoAllocationChecker.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/NoAllocationChecker.java
@@ -38,7 +38,7 @@
import static com.google.errorprone.matchers.Matchers.typeCast;
import static com.google.errorprone.matchers.Matchers.variableInitializer;
import static com.google.errorprone.matchers.Matchers.variableType;
-import static com.google.errorprone.util.ASTHelpers.findSuperMethods;
+import static com.google.errorprone.util.ASTHelpers.streamSuperMethods;
import static com.sun.source.tree.Tree.Kind.AND_ASSIGNMENT;
import static com.sun.source.tree.Tree.Kind.DIVIDE_ASSIGNMENT;
import static com.sun.source.tree.Tree.Kind.LEFT_SHIFT_ASSIGNMENT;
@@ -393,7 +393,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
return NO_MATCH;
}
MethodSymbol symbol = ASTHelpers.getSymbol(tree);
- return findSuperMethods(symbol, state.getTypes()).stream()
+ return streamSuperMethods(symbol, state.getTypes())
.filter(s -> ASTHelpers.hasAnnotation(s, NoAllocation.class.getName(), state))
.findAny()
.map(
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ParameterName.java b/core/src/main/java/com/google/errorprone/bugpatterns/ParameterName.java
index 9cc7890541f..80e12e376b3 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/ParameterName.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/ParameterName.java
@@ -18,8 +18,6 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.common.collect.Iterables.getLast;
-import static com.google.common.collect.Streams.forEachPair;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
@@ -48,11 +46,15 @@
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.Position;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Deque;
+import java.util.Iterator;
import java.util.List;
+import java.util.Optional;
+import java.util.function.Consumer;
import java.util.regex.Matcher;
/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@@ -80,18 +82,21 @@ public ParameterName(ErrorProneFlags errorProneFlags) {
@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
- checkArguments(tree, tree.getArguments(), state);
+ checkArguments(tree, tree.getArguments(), state.getEndPosition(tree.getMethodSelect()), state);
return NO_MATCH;
}
@Override
public Description matchNewClass(NewClassTree tree, VisitorState state) {
- checkArguments(tree, tree.getArguments(), state);
+ checkArguments(tree, tree.getArguments(), state.getEndPosition(tree.getIdentifier()), state);
return NO_MATCH;
}
private void checkArguments(
- Tree tree, List arguments, VisitorState state) {
+ Tree tree,
+ List arguments,
+ int argListStartPosition,
+ VisitorState state) {
if (arguments.isEmpty()) {
return;
}
@@ -99,55 +104,77 @@ private void checkArguments(
if (NamedParameterComment.containsSyntheticParameterName(sym)) {
return;
}
- int start = getStartPosition(tree);
- int end = state.getEndPosition(getLast(arguments));
- if (start == Position.NOPOS || end == Position.NOPOS) {
+ int start = argListStartPosition;
+ if (start == Position.NOPOS) {
// best effort work-around for https://github.com/google/error-prone/issues/780
return;
}
- String source = state.getSourceCode().subSequence(start, end).toString();
- if (!source.contains("/*")) {
- // fast path if the arguments don't contain anything that looks like a comment
- return;
- }
String enclosingClass = ASTHelpers.enclosingClass(sym).toString();
if (exemptPackages.stream().anyMatch(enclosingClass::startsWith)) {
return;
}
- Deque tokens =
- new ArrayDeque<>(ErrorProneTokens.getTokens(source, start, state.context));
- forEachPair(
- sym.getParameters().stream(),
- arguments.stream(),
- (p, a) -> {
- if (advanceTokens(tokens, a, state)) {
- checkArgument(p, a, tokens.removeFirst(), state);
- }
- });
+ Iterator argumentIterator = arguments.iterator();
+ // For each parameter/argument pair, we tokenize the characters between the end of the
+ // previous argument (or the start of the argument list, in the case of the first argument)
+ // and the start of the current argument. The `start` variable is advanced each time, stepping
+ // over each argument when we finish processing it.
+ for (VarSymbol param : sym.getParameters()) {
+ if (!argumentIterator.hasNext()) {
+ return; // A vararg parameter has zero corresponding arguments passed
+ }
+ ExpressionTree argument = argumentIterator.next();
+ Optional> positions = positions(argument, state);
+ if (positions.isEmpty()) {
+ return;
+ }
+ start =
+ processArgument(
+ positions.get(), start, state, tok -> checkArgument(param, argument, tok, state));
+ }
// handle any varargs arguments after the first
- int numParams = sym.getParameters().size();
- int numArgs = arguments.size();
- if (numParams < numArgs) {
- for (ExpressionTree arg : arguments.subList(numParams, numArgs)) {
- if (advanceTokens(tokens, arg, state)) {
- checkComment(arg, tokens.removeFirst(), state);
- }
+ while (argumentIterator.hasNext()) {
+ ExpressionTree argument = argumentIterator.next();
+ Optional> positions = positions(argument, state);
+ if (positions.isEmpty()) {
+ return;
}
+ start =
+ processArgument(positions.get(), start, state, tok -> checkComment(argument, tok, state));
}
}
- private static boolean advanceTokens(
- Deque tokens, ExpressionTree actual, VisitorState state) {
- while (!tokens.isEmpty() && tokens.getFirst().pos() < getStartPosition(actual)) {
+ /** Returns the source span for a tree, or empty if the position information is not available. */
+ Optional> positions(Tree tree, VisitorState state) {
+ int endPosition = state.getEndPosition(tree);
+ if (endPosition == Position.NOPOS) {
+ return Optional.empty();
+ }
+ return Optional.of(Range.closedOpen(getStartPosition(tree), endPosition));
+ }
+
+ private static int processArgument(
+ Range positions,
+ int offset,
+ VisitorState state,
+ Consumer consumer) {
+ String source = state.getSourceCode().subSequence(offset, positions.upperEndpoint()).toString();
+ Deque tokens =
+ new ArrayDeque<>(ErrorProneTokens.getTokens(source, offset, state.context));
+ if (advanceTokens(tokens, positions)) {
+ consumer.accept(tokens.removeFirst());
+ }
+ return positions.upperEndpoint();
+ }
+
+ private static boolean advanceTokens(Deque tokens, Range actual) {
+ while (!tokens.isEmpty() && tokens.getFirst().pos() < actual.lowerEndpoint()) {
tokens.removeFirst();
}
if (tokens.isEmpty()) {
return false;
}
- Range argRange =
- Range.closedOpen(getStartPosition(actual), state.getEndPosition(actual));
- if (!argRange.contains(tokens.getFirst().pos())) {
+ if (!actual.contains(tokens.getFirst().pos())) {
return false;
}
return true;
@@ -173,6 +200,11 @@ private void checkArgument(
VarSymbol formal, ExpressionTree actual, ErrorProneToken token, VisitorState state) {
List matches = new ArrayList<>();
for (Comment comment : token.comments()) {
+ if (comment.getStyle().equals(CommentStyle.LINE)) {
+ // These are usually not intended as a parameter comment, and we don't want to flag if they
+ // happen to match the parameter comment format.
+ continue;
+ }
Matcher m =
NamedParameterComment.PARAMETER_COMMENT_PATTERN.matcher(
Comments.getTextFromComment(comment));
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/PreferredInterfaceType.java b/core/src/main/java/com/google/errorprone/bugpatterns/PreferredInterfaceType.java
index ba99856149a..0307c41db99 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/PreferredInterfaceType.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/PreferredInterfaceType.java
@@ -110,7 +110,8 @@ public final class PreferredInterfaceType extends BugChecker implements Compilat
"com.google.common.collect.ImmutableSetMultimap",
"com.google.common.collect.ImmutableMultimap",
"com.google.common.collect.ListMultimap",
- "com.google.common.collect.SetMultimap"));
+ "com.google.common.collect.SetMultimap"),
+ BetterTypes.of(isDescendantOf("java.lang.CharSequence"), "java.lang.String"));
private static final Matcher INTERESTING_TYPE =
anyOf(
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ProtoFieldNullComparison.java b/core/src/main/java/com/google/errorprone/bugpatterns/ProtoFieldNullComparison.java
index 2e6c07d122e..599bcf7ef8b 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/ProtoFieldNullComparison.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/ProtoFieldNullComparison.java
@@ -140,7 +140,7 @@ private static boolean isNull(ExpressionTree tree) {
public ProtoFieldNullComparison(ErrorProneFlags flags) {
this.matchTestAssertions =
- flags.getBoolean("ProtoFieldNullComparison:MatchTestAssertions").orElse(false);
+ flags.getBoolean("ProtoFieldNullComparison:MatchTestAssertions").orElse(true);
}
@Override
@@ -181,6 +181,7 @@ public Void visitVariable(VariableTree variable, Void unused) {
private Optional getInitializer(ExpressionTree tree) {
return Optional.ofNullable(
new SimpleTreeVisitor() {
+ @Nullable
@Override
public ExpressionTree visitMethodInvocation(MethodInvocationTree node, Void unused) {
return PROTO_RECEIVER.matches(node, state) ? node : null;
@@ -290,6 +291,7 @@ private interface Fixer {
private enum GetterTypes {
/** {@code proto.getFoo()} */
SCALAR {
+ @Nullable
@Override
Fixer match(ExpressionTree tree, VisitorState state) {
if (tree.getKind() != Kind.METHOD_INVOCATION) {
@@ -335,6 +337,7 @@ private String replaceLast(String text, String pattern, String replacement) {
},
/** {@code proto.getRepeatedFoo(index)} */
VECTOR_INDEXED {
+ @Nullable
@Override
Fixer match(ExpressionTree tree, VisitorState state) {
if (tree.getKind() != Kind.METHOD_INVOCATION) {
@@ -365,6 +368,7 @@ private String generateFix(
},
/** {@code proto.getRepeatedFooList()} */
VECTOR {
+ @Nullable
@Override
Fixer match(ExpressionTree tree, VisitorState state) {
if (tree.getKind() != Kind.METHOD_INVOCATION) {
@@ -391,6 +395,7 @@ private String generateFix(
},
/** {@code proto.getField(f)} or {@code proto.getExtension(outer, extension)}; */
EXTENSION_METHOD {
+ @Nullable
@Override
Fixer match(ExpressionTree tree, VisitorState state) {
if (EXTENSION_METHODS_WITH_NO_FIX.matches(tree, state)) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ProtoRedundantSet.java b/core/src/main/java/com/google/errorprone/bugpatterns/ProtoRedundantSet.java
index 43dda75068e..b295425a6c6 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/ProtoRedundantSet.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/ProtoRedundantSet.java
@@ -41,6 +41,7 @@
import java.util.Collection;
import java.util.Map;
import java.util.regex.Pattern;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* Checks that protocol buffers built with chained builders don't set the same field twice.
@@ -167,7 +168,7 @@ interface ProtoField {}
enum FieldType {
SINGLE {
@Override
- FieldWithValue match(String name, MethodInvocationTree tree) {
+ @Nullable FieldWithValue match(String name, MethodInvocationTree tree) {
if (name.startsWith("set") && tree.getArguments().size() == 1) {
return FieldWithValue.of(SingleField.of(name), tree, tree.getArguments().get(0));
}
@@ -176,7 +177,7 @@ FieldWithValue match(String name, MethodInvocationTree tree) {
},
REPEATED {
@Override
- FieldWithValue match(String name, MethodInvocationTree tree) {
+ @Nullable FieldWithValue match(String name, MethodInvocationTree tree) {
if (name.startsWith("set") && tree.getArguments().size() == 2) {
Integer index = ASTHelpers.constValue(tree.getArguments().get(0), Integer.class);
if (index != null) {
@@ -189,7 +190,7 @@ FieldWithValue match(String name, MethodInvocationTree tree) {
},
MAP {
@Override
- FieldWithValue match(String name, MethodInvocationTree tree) {
+ @Nullable FieldWithValue match(String name, MethodInvocationTree tree) {
if (name.startsWith("put") && tree.getArguments().size() == 2) {
Object key = ASTHelpers.constValue(tree.getArguments().get(0), Object.class);
if (key != null) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/RestrictedApiChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/RestrictedApiChecker.java
index 7762bf3f69f..4a6484e731a 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/RestrictedApiChecker.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/RestrictedApiChecker.java
@@ -21,6 +21,7 @@
import static com.google.common.collect.Iterables.getOnlyElement;
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.streamSuperMethods;
import com.google.common.collect.ImmutableSet;
import com.google.errorprone.BugPattern;
@@ -189,15 +190,14 @@ private Description checkMethodUse(
}
// Try each super method for @RestrictedApi
- Optional superWithRestrictedApi =
- ASTHelpers.findSuperMethods(method, state.getTypes()).stream()
- .filter((t) -> ASTHelpers.hasAnnotation(t, RestrictedApi.class, state))
- .findFirst();
- if (!superWithRestrictedApi.isPresent()) {
- return NO_MATCH;
- }
- return checkRestriction(
- getRestrictedApiAnnotation(superWithRestrictedApi.get(), state), where, state);
+ return streamSuperMethods(method, state.getTypes())
+ .filter((t) -> ASTHelpers.hasAnnotation(t, RestrictedApi.class, state))
+ .findFirst()
+ .map(
+ superWithRestrictedApi ->
+ checkRestriction(
+ getRestrictedApiAnnotation(superWithRestrictedApi, state), where, state))
+ .orElse(NO_MATCH);
}
@Nullable
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ReturnsNullCollection.java b/core/src/main/java/com/google/errorprone/bugpatterns/ReturnsNullCollection.java
index c93831d74db..6326b8a668e 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/ReturnsNullCollection.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/ReturnsNullCollection.java
@@ -53,7 +53,8 @@ private static boolean methodWithoutNullable(MethodTree tree, VisitorState state
allOf(
anyOf(
methodReturns(isSubtypeOf("java.util.Collection")),
- methodReturns(isSubtypeOf("java.util.Map"))),
+ methodReturns(isSubtypeOf("java.util.Map")),
+ methodReturns(isSubtypeOf("com.google.common.collect.Multimap"))),
ReturnsNullCollection::methodWithoutNullable);
public ReturnsNullCollection() {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/RxReturnValueIgnored.java b/core/src/main/java/com/google/errorprone/bugpatterns/RxReturnValueIgnored.java
index 1fcf18ebead..e60cf045f17 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/RxReturnValueIgnored.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/RxReturnValueIgnored.java
@@ -22,6 +22,7 @@
import static com.google.errorprone.matchers.Matchers.not;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
+import static com.google.errorprone.util.ASTHelpers.streamSuperMethods;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
@@ -66,7 +67,7 @@ private static boolean hasCirvAnnotation(ExpressionTree tree, VisitorState state
// if the super-type returned the exact same type. This lets us catch issues where a
// superclass was annotated with @CanIgnoreReturnValue but the parent did not intend to
// return an Rx type
- return ASTHelpers.findSuperMethods(sym, state.getTypes()).stream()
+ return streamSuperMethods(sym, state.getTypes())
.anyMatch(
superSym ->
hasAnnotation(superSym, CanIgnoreReturnValue.class, state)
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/SameNameButDifferent.java b/core/src/main/java/com/google/errorprone/bugpatterns/SameNameButDifferent.java
index 5ccba6f30ec..5163b07d59e 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/SameNameButDifferent.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/SameNameButDifferent.java
@@ -41,6 +41,7 @@
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.ClassSymbol;
import com.sun.tools.javac.code.Symbol.TypeSymbol;
+import com.sun.tools.javac.util.Position;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Deque;
@@ -48,6 +49,8 @@
import java.util.Map;
import java.util.Optional;
import javax.lang.model.element.ElementKind;
+import javax.lang.model.element.Name;
+import org.checkerframework.checker.nullness.qual.Nullable;
/** Looks for types being shadowed by other types in a way that may be confusing. */
@BugPattern(
@@ -92,21 +95,38 @@ private boolean shouldIgnore() {
&& getSymbol(parentTree) instanceof ClassSymbol;
}
+ private @Nullable String qualifiedName(Tree tree) {
+ if (state.getEndPosition(tree) == Position.NOPOS) {
+ return null;
+ }
+ ArrayDeque parts = new ArrayDeque<>();
+ while (tree instanceof MemberSelectTree) {
+ MemberSelectTree select = (MemberSelectTree) tree;
+ parts.addFirst(select.getIdentifier());
+ tree = select.getExpression();
+ }
+ if (!(tree instanceof IdentifierTree)) {
+ return null;
+ }
+ parts.addFirst(((IdentifierTree) tree).getName());
+ return Joiner.on('.').join(parts);
+ }
+
private void handle(Tree tree) {
if (tree instanceof IdentifierTree
&& ((IdentifierTree) tree).getName().contentEquals("Builder")) {
return;
}
- String treeSource = state.getSourceForNode(tree);
- if (treeSource == null) {
+ String qualifiedName = qualifiedName(tree);
+ if (qualifiedName == null) {
return;
}
Symbol symbol = getSymbol(tree);
if (symbol instanceof ClassSymbol) {
- List treePaths = table.get(treeSource, symbol.type.tsym);
+ List treePaths = table.get(qualifiedName, symbol.type.tsym);
if (treePaths == null) {
treePaths = new ArrayList<>();
- table.put(treeSource, symbol.type.tsym, treePaths);
+ table.put(qualifiedName, symbol.type.tsym, treePaths);
}
treePaths.add(getCurrentPath());
}
@@ -183,6 +203,9 @@ private static Optional getBetterImport(TypeSymbol classSymbol, String s
Symbol owner = classSymbol;
long dots = simpleName.chars().filter(c -> c == '.').count();
for (long i = 0; i < dots + 1; ++i) {
+ if (owner == null) {
+ return Optional.empty();
+ }
owner = owner.owner;
}
if (owner instanceof ClassSymbol) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/SelfAlwaysReturnsThis.java b/core/src/main/java/com/google/errorprone/bugpatterns/SelfAlwaysReturnsThis.java
new file mode 100644
index 00000000000..4da47b30279
--- /dev/null
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/SelfAlwaysReturnsThis.java
@@ -0,0 +1,161 @@
+/*
+ * Copyright 2022 The Error Prone Authors.
+ *
+ * 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;
+
+import static com.google.common.base.MoreObjects.firstNonNull;
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.util.ASTHelpers.enclosingClass;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.getType;
+import static com.google.errorprone.util.ASTHelpers.isConsideredFinal;
+import static com.google.errorprone.util.ASTHelpers.isSameType;
+import static com.google.errorprone.util.ASTHelpers.isVoidType;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.matchers.Description;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.IdentifierTree;
+import com.sun.source.tree.LambdaExpressionTree;
+import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.NewClassTree;
+import com.sun.source.tree.ReturnTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.TypeCastTree;
+import com.sun.source.tree.VariableTree;
+import com.sun.source.util.SimpleTreeVisitor;
+import com.sun.source.util.TreePathScanner;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import com.sun.tools.javac.code.Symbol.VarSymbol;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/**
+ * Non-abstract instance methods named {@code self()} that return the enclosing class must always
+ * {@code return this}.
+ */
+@BugPattern(
+ summary =
+ "Non-abstract instance methods named 'self()' that return the enclosing class must always"
+ + " 'return this'",
+ severity = WARNING)
+public final class SelfAlwaysReturnsThis extends BugChecker implements MethodTreeMatcher {
+ @Override
+ public Description matchMethod(MethodTree methodTree, VisitorState state) {
+ MethodSymbol methodSymbol = getSymbol(methodTree);
+
+ // The method must:
+ // * not be a constructor
+ // * be named `self`
+ // * have no params
+ // * be an instance method (not static)
+ // * have a body (not abstract)
+ if (methodSymbol.isConstructor()
+ || !methodSymbol.getSimpleName().contentEquals("self")
+ || !methodSymbol.getParameters().isEmpty()
+ || methodSymbol.isStatic()
+ || methodTree.getBody() == null) {
+ return NO_MATCH;
+ }
+
+ // * not have a void (or Void) return type
+ Tree returnType = methodTree.getReturnType();
+ if (isVoidType(getType(returnType), state)) {
+ return NO_MATCH;
+ }
+
+ // * have the same return type as the enclosing type
+ if (!isSameType(getType(returnType), enclosingClass(methodSymbol).type, state)) {
+ return NO_MATCH;
+ }
+
+ // TODO(kak): we should probably re-used the TreePathScanner from CanIgnoreReturnValueSuggester
+
+ // This TreePathScanner is mostly copied from CanIgnoreReturnValueSuggester
+ AtomicBoolean allReturnThis = new AtomicBoolean(true);
+ AtomicBoolean atLeastOneReturn = new AtomicBoolean(false);
+
+ new TreePathScanner() {
+ private final Set thises = new HashSet<>();
+
+ @Override
+ public Void visitVariable(VariableTree variableTree, Void unused) {
+ VarSymbol symbol = getSymbol(variableTree);
+ if (isConsideredFinal(symbol) && maybeCastThis(variableTree.getInitializer())) {
+ thises.add(symbol);
+ }
+ return super.visitVariable(variableTree, null);
+ }
+
+ @Override
+ public Void visitReturn(ReturnTree returnTree, Void unused) {
+ atLeastOneReturn.set(true);
+ if (!isThis(returnTree.getExpression())) {
+ allReturnThis.set(false);
+ // once we've set allReturnThis to false, no need to descend further
+ return null;
+ }
+ return super.visitReturn(returnTree, null);
+ }
+
+ /** Returns whether the given {@link ExpressionTree} is {@code this}. */
+ private boolean isThis(ExpressionTree returnExpression) {
+ return maybeCastThis(returnExpression) || thises.contains(getSymbol(returnExpression));
+ }
+
+ @Override
+ public Void visitLambdaExpression(LambdaExpressionTree node, Void unused) {
+ // don't descend into lambdas
+ return null;
+ }
+
+ @Override
+ public Void visitNewClass(NewClassTree node, Void unused) {
+ // don't descend into declarations of anonymous classes
+ return null;
+ }
+ }.scan(state.getPath(), null);
+
+ if (atLeastOneReturn.get() && allReturnThis.get()) {
+ return NO_MATCH;
+ }
+
+ return describeMatch(
+ methodTree, SuggestedFix.replace(methodTree.getBody(), "{ return this; }"));
+ }
+
+ private static boolean maybeCastThis(Tree tree) {
+ return firstNonNull(
+ new SimpleTreeVisitor() {
+
+ @Override
+ public Boolean visitTypeCast(TypeCastTree tree, Void unused) {
+ return visit(tree.getExpression(), null);
+ }
+
+ @Override
+ public Boolean visitIdentifier(IdentifierTree tree, Void unused) {
+ return tree.getName().contentEquals("this");
+ }
+ }.visit(tree, null),
+ false);
+ }
+}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/SelfAssignment.java b/core/src/main/java/com/google/errorprone/bugpatterns/SelfAssignment.java
index 019c24214cf..8d0175874b6 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/SelfAssignment.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/SelfAssignment.java
@@ -49,6 +49,7 @@
import com.sun.tools.javac.code.Flags;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Type;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* TODO(eaftan): Consider cases where the parent is not a statement or there is no parent?
@@ -122,7 +123,7 @@ public ExpressionTree visitTypeCast(TypeCastTree node, Void unused) {
}
@Override
- protected ExpressionTree defaultAction(Tree node, Void unused) {
+ protected @Nullable ExpressionTree defaultAction(Tree node, Void unused) {
return node instanceof ExpressionTree ? (ExpressionTree) node : null;
}
}.visit(expression, null);
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StronglyType.java b/core/src/main/java/com/google/errorprone/bugpatterns/StronglyType.java
index ec9e6063a27..03d38150575 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/StronglyType.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/StronglyType.java
@@ -29,6 +29,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.SetMultimap;
import com.google.errorprone.VisitorState;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.Description;
@@ -94,6 +95,7 @@ public abstract static class Builder {
abstract ImmutableSet.Builder primitiveTypesToReplaceBuilder();
/** Add a type that can be replaced with a stronger type. */
+ @CanIgnoreReturnValue
public final Builder addType(Type type) {
primitiveTypesToReplaceBuilder().add(type);
return this;
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TruthAssertExpected.java b/core/src/main/java/com/google/errorprone/bugpatterns/TruthAssertExpected.java
index 4753a66e46a..a974b30ad47 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/TruthAssertExpected.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/TruthAssertExpected.java
@@ -132,6 +132,7 @@ public List visitNewClass(NewClassTree node, Void unus
return node.getArguments();
}
+ @Nullable
@Override
public List visitMethodInvocation(
MethodInvocationTree node, Void unused) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnicodeDirectionalityCharacters.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnicodeDirectionalityCharacters.java
index 8d1b682e8a4..f0cdf0818fd 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/UnicodeDirectionalityCharacters.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnicodeDirectionalityCharacters.java
@@ -40,30 +40,28 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
for (int i = 0; i < source.length(); ++i) {
char c = source.charAt(i);
- if (isDangerous(c)) {
- state.reportMatch(
- describeMatch(
- new FixedPosition(tree, i),
- SuggestedFix.replace(i, i + 1, String.format("\\u%04x", (int) c))));
+ // Do not extract this switch to a method. It's ugly as-is, but profiling suggests this
+ // checker is expensive for large files, and also that the method-call overhead would
+ // double the time spent in this loop.
+ switch (c) {
+ case 0x202A: // Left-to-Right Embedding
+ case 0x202B: // Right-to-Left Embedding
+ case 0x202C: // Pop Directional Formatting
+ case 0x202D: // Left-to-Right Override
+ case 0x202E: // Right-to-Left Override
+ case 0x2066: // Left-to-Right Isolate
+ case 0x2067: // Right-to-Left Isolate
+ case 0x2068: // First Strong Isolate
+ case 0x2069: // Pop Directional Isolate
+ state.reportMatch(
+ describeMatch(
+ new FixedPosition(tree, i),
+ SuggestedFix.replace(i, i + 1, String.format("\\u%04x", (int) c))));
+ break;
+ default:
+ break;
}
}
return NO_MATCH;
}
-
- private static boolean isDangerous(char c) {
- switch (c) {
- case 0x202A: // Left-to-Right Embedding
- case 0x202B: // Right-to-Left Embedding
- case 0x202C: // Pop Directional Formatting
- case 0x202D: // Left-to-Right Override
- case 0x202E: // Right-to-Left Override
- case 0x2066: // Left-to-Right Isolate
- case 0x2067: // Right-to-Left Isolate
- case 0x2068: // First Strong Isolate
- case 0x2069: // Pop Directional Isolate
- return true;
- default:
- return false;
- }
- }
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnicodeInCode.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnicodeInCode.java
index 2a2c0fa77df..e5c72add1d4 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/UnicodeInCode.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnicodeInCode.java
@@ -21,22 +21,22 @@
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.util.ErrorProneTokens.getTokens;
+import static com.google.errorprone.util.SourceCodeEscapers.javaCharEscaper;
import static java.lang.String.format;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableRangeSet;
import com.google.common.collect.Range;
+import com.google.common.collect.RangeSet;
+import com.google.common.collect.TreeRangeSet;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
import com.google.errorprone.fixes.FixedPosition;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ErrorProneToken;
-import com.google.errorprone.util.SourceCodeEscapers;
import com.sun.source.tree.CompilationUnitTree;
import com.sun.tools.javac.parser.Tokens.TokenKind;
-import java.util.LinkedHashMap;
-import java.util.Map;
/** Bans using non-ASCII Unicode characters outside string literals and comments. */
@BugPattern(
@@ -47,17 +47,14 @@
public final class UnicodeInCode extends BugChecker implements CompilationUnitTreeMatcher {
@Override
public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
- ImmutableRangeSet commentsAndLiterals = commentsAndLiterals(state);
-
- Map violations = new LinkedHashMap<>();
-
- CharSequence sourceCode = state.getSourceCode();
+ RangeSet violations = TreeRangeSet.create();
+ String sourceCode = state.getSourceCode().toString();
for (int i = 0; i < sourceCode.length(); ++i) {
char c = sourceCode.charAt(i);
- if (!isAcceptableAscii(c) && !commentsAndLiterals.contains(i)) {
- violations.put(i, c);
+ if (!isAcceptableAscii(c)) {
+ violations.add(Range.closedOpen(i, i + 1));
}
}
@@ -65,19 +62,21 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
return NO_MATCH;
}
- ImmutableRangeSet suppressedRegions = suppressedRegions(state);
+ ImmutableRangeSet permissibleUnicodeRegions =
+ suppressedRegions(state).union(commentsAndLiterals(state, sourceCode));
- for (var e : violations.entrySet()) {
- int violatingLocation = e.getKey();
- char c = e.getValue();
- if (!suppressedRegions.contains(violatingLocation)) {
+ for (var range : violations.asDescendingSetOfRanges()) {
+ if (!permissibleUnicodeRegions.encloses(range)) {
state.reportMatch(
- buildDescription(new FixedPosition(tree, violatingLocation))
+ buildDescription(new FixedPosition(tree, range.lowerEndpoint()))
.setMessage(
format(
"Avoid using non-ASCII Unicode character (%s) outside of comments and"
+ " literals, as they can be confusing.",
- SourceCodeEscapers.javaCharEscaper().escape(Character.toString(c))))
+ javaCharEscaper()
+ .escape(
+ sourceCode.substring(
+ range.lowerEndpoint(), range.upperEndpoint()))))
.build());
}
}
@@ -88,9 +87,8 @@ private static boolean isAcceptableAscii(char c) {
return (c >= 0x20 && c <= 0x7E) || c == '\n' || c == '\r' || c == '\t';
}
- private static ImmutableRangeSet commentsAndLiterals(VisitorState state) {
- ImmutableList tokens =
- getTokens(state.getSourceCode().toString(), state.context);
+ private static ImmutableRangeSet commentsAndLiterals(VisitorState state, String source) {
+ ImmutableList tokens = getTokens(source, state.context);
return ImmutableRangeSet.unionOf(
concat(
tokens.stream()
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAssignment.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAssignment.java
index 9e34317586a..7ea5f1d9afe 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAssignment.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAssignment.java
@@ -121,13 +121,23 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) {
@Override
public Description matchVariable(VariableTree tree, VisitorState state) {
+ boolean hasMockAnnotation = HAS_MOCK_ANNOTATION.matches(tree, state);
+ boolean hasInjectyAnnotation = HAS_NON_MOCK_FRAMEWORK_ANNOTATION.matches(tree, state);
+ if (hasMockAnnotation && hasInjectyAnnotation) {
+ return buildDescription(tree)
+ .setMessage(
+ "Fields shouldn't be annotated with both @Mock and another @Inject-like annotation,"
+ + " because both Mockito and the injector will assign to the field, and one of"
+ + " the values will overwrite the other")
+ .build();
+ }
if (tree.getInitializer() == null) {
return NO_MATCH;
}
- if (HAS_MOCK_ANNOTATION.matches(tree, state)) {
+ if (hasMockAnnotation) {
return describeMatch(tree, createMockFix(tree, state));
}
- if (HAS_NON_MOCK_FRAMEWORK_ANNOTATION.matches(tree, state)) {
+ if (hasInjectyAnnotation) {
Description.Builder description = buildDescription(tree);
if (!tree.getModifiers().getFlags().contains(Modifier.FINAL)) {
String source =
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryLambda.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryLambda.java
index bcb19bb6db1..abacf67f7d1 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryLambda.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryLambda.java
@@ -66,6 +66,7 @@
import java.util.function.Predicate;
import javax.lang.model.element.ElementKind;
import javax.lang.model.element.Modifier;
+import org.checkerframework.checker.nullness.qual.Nullable;
/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@BugPattern(
@@ -303,7 +304,7 @@ public LambdaExpressionTree visitLambdaExpression(LambdaExpressionTree node, Voi
}
@Override
- public LambdaExpressionTree visitBlock(BlockTree node, Void unused) {
+ public @Nullable LambdaExpressionTree visitBlock(BlockTree node, Void unused) {
// when processing a method body, only consider methods with a single `return` statement
// that returns a method
return node.getStatements().size() == 1
@@ -312,7 +313,7 @@ public LambdaExpressionTree visitBlock(BlockTree node, Void unused) {
}
@Override
- public LambdaExpressionTree visitReturn(ReturnTree node, Void unused) {
+ public @Nullable LambdaExpressionTree visitReturn(ReturnTree node, Void unused) {
return node.getExpression() != null ? node.getExpression().accept(this, null) : null;
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryParentheses.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryParentheses.java
index ff618ae44bc..474d1b4ffa9 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryParentheses.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryParentheses.java
@@ -17,7 +17,6 @@
package com.google.errorprone.bugpatterns;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
-import static com.google.errorprone.BugPattern.StandardTags.STYLE;
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
@@ -36,8 +35,7 @@
summary =
"These grouping parentheses are unnecessary; it is unlikely the code will"
+ " be misinterpreted without them",
- severity = WARNING,
- tags = STYLE)
+ severity = WARNING)
public class UnnecessaryParentheses extends BugChecker implements ParenthesizedTreeMatcher {
@Override
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedTypeParameter.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedTypeParameter.java
new file mode 100644
index 00000000000..32221134d03
--- /dev/null
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedTypeParameter.java
@@ -0,0 +1,132 @@
+/*
+ * Copyright 2022 The Error Prone Authors.
+ *
+ * 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;
+
+import static com.google.common.collect.Iterables.getLast;
+import static com.google.errorprone.fixes.SuggestedFixes.removeElement;
+import static com.google.errorprone.util.ASTHelpers.findSuperMethods;
+import static com.google.errorprone.util.ASTHelpers.getStartPosition;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.methodCanBeOverridden;
+
+import com.google.common.collect.ImmutableMultiset;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.BugPattern.SeverityLevel;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.util.ErrorProneTokens;
+import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.CompilationUnitTree;
+import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.TypeParameterTree;
+import com.sun.source.util.TreeScanner;
+import com.sun.tools.javac.code.Flags;
+import com.sun.tools.javac.code.Symbol.TypeVariableSymbol;
+import com.sun.tools.javac.parser.Tokens.TokenKind;
+import java.util.List;
+
+/** A BugPattern; see the summary. */
+@BugPattern(
+ severity = SeverityLevel.WARNING,
+ summary = "This type parameter is unused and can be removed.")
+public final class UnusedTypeParameter extends BugChecker implements CompilationUnitTreeMatcher {
+ @Override
+ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
+ var usedIdentifiers = findUsedIdentifiers(tree);
+ new SuppressibleTreePathScanner(state) {
+ @Override
+ public Void visitClass(ClassTree node, Void unused) {
+ if ((getSymbol(node).flags() & Flags.FINAL) != 0) {
+ handle(node, node.getTypeParameters());
+ }
+ return super.visitClass(node, null);
+ }
+
+ @Override
+ public Void visitMethod(MethodTree node, Void unused) {
+ var symbol = getSymbol(node);
+ if (methodCanBeOverridden(symbol)
+ || !findSuperMethods(symbol, state.getTypes()).isEmpty()) {
+ return null;
+ }
+ handle(node, node.getTypeParameters());
+ return super.visitMethod(node, null);
+ }
+
+ private void handle(Tree tree, List typeParameters) {
+ for (TypeParameterTree typeParameter : typeParameters) {
+ if (usedIdentifiers.count(getSymbol(typeParameter)) == 1) {
+ state.reportMatch(
+ describeMatch(
+ typeParameter,
+ removeTypeParameter(tree, typeParameter, typeParameters, state)));
+ }
+ }
+ }
+ }.scan(state.getPath(), null);
+ return Description.NO_MATCH;
+ }
+
+ private static ImmutableMultiset findUsedIdentifiers(
+ CompilationUnitTree tree) {
+ ImmutableMultiset.Builder identifiers = ImmutableMultiset.builder();
+ new TreeScanner() {
+ @Override
+ public Void scan(Tree tree, Void unused) {
+ var symbol = getSymbol(tree);
+ if (symbol instanceof TypeVariableSymbol) {
+ identifiers.add((TypeVariableSymbol) symbol);
+ }
+ return super.scan(tree, unused);
+ }
+ }.scan(tree, null);
+ return identifiers.build();
+ }
+
+ private static SuggestedFix removeTypeParameter(
+ Tree tree,
+ TypeParameterTree typeParameter,
+ List typeParameters,
+ VisitorState state) {
+ if (typeParameters.size() > 1) {
+ return removeElement(typeParameter, typeParameters, state);
+ }
+ var tokens =
+ ErrorProneTokens.getTokens(
+ state.getSourceForNode(tree), getStartPosition(tree), state.context);
+ int startPos =
+ tokens.reverse().stream()
+ .filter(
+ t -> t.pos() <= getStartPosition(typeParameter) && t.kind().equals(TokenKind.LT))
+ .findFirst()
+ .get()
+ .pos();
+ int endPos =
+ tokens.stream()
+ .filter(
+ t ->
+ t.endPos() >= state.getEndPosition(getLast(typeParameters))
+ && (t.kind().equals(TokenKind.GT) || t.kind().equals(TokenKind.GTGT)))
+ .findFirst()
+ .get()
+ .endPos();
+ return SuggestedFix.replace(startPos, endPos, "");
+ }
+}
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 763b20c0219..73d550b5127 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/WildcardImport.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/WildcardImport.java
@@ -32,6 +32,7 @@
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.CaseTree;
import com.sun.source.tree.CompilationUnitTree;
+import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree;
import com.sun.source.tree.ImportTree;
import com.sun.source.tree.MemberSelectTree;
@@ -43,10 +44,15 @@
import com.sun.tools.javac.tree.JCTree.JCCompilationUnit;
import com.sun.tools.javac.tree.JCTree.JCIdent;
import com.sun.tools.javac.tree.TreeScanner;
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.MethodType;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.logging.Level;
+import java.util.logging.Logger;
import java.util.stream.Collectors;
import javax.lang.model.element.ElementKind;
@@ -59,6 +65,7 @@
tags = StandardTags.STYLE,
link = "https://google.github.io/styleguide/javaguide.html?cl=head#s3.3.1-wildcard-imports")
public class WildcardImport extends BugChecker implements CompilationUnitTreeMatcher {
+ private static final Logger logger = Logger.getLogger(WildcardImport.class.getName());
/** Maximum number of members to import before switching to qualified names. */
public static final int MAX_MEMBER_IMPORTS = 20;
@@ -213,6 +220,24 @@ static Fix createFix(
return fix.build();
}
+ private static final MethodHandle CONSTANT_CASE_LABEL_TREE_GET_EXPRESSION;
+
+ static {
+ MethodHandle h;
+ try {
+ Class constantCaseLabelTree = Class.forName("com.sun.source.tree.ConstantCaseLabelTree");
+ h =
+ MethodHandles.lookup()
+ .findVirtual(
+ constantCaseLabelTree,
+ "getConstantExpression",
+ MethodType.methodType(ExpressionTree.class));
+ } catch (ReflectiveOperationException e) {
+ h = null;
+ }
+ CONSTANT_CASE_LABEL_TREE_GET_EXPRESSION = h;
+ }
+
/**
* Add an import for {@code owner}, and qualify all on demand imported references to members of
* owner by owner's simple name.
@@ -228,11 +253,27 @@ public Void visitIdentifier(IdentifierTree tree, Void unused) {
return null;
}
Tree parent = getCurrentPath().getParentPath().getLeaf();
- if (parent.getKind() == Tree.Kind.CASE
- && ((CaseTree) parent).getExpression().equals(tree)
- && sym.owner.getKind() == ElementKind.ENUM) {
- // switch cases can refer to enum constants by simple name without importing them
- return null;
+ if (sym.owner.getKind() == ElementKind.ENUM) {
+ if (parent.getKind() == Tree.Kind.CASE
+ && ((CaseTree) parent).getExpression().equals(tree)) {
+ // switch cases can refer to enum constants by simple name without importing them
+ return null;
+ }
+ // In JDK 19, the tree representation of enum case-labels changes. We can't reference the
+ // relevant API directly because then this code wouldn't compile on earlier JDK versions.
+ // So instead we use method handles. The straightforward code would be:
+ // if (parent.getKind() == Tree.Kind.CONSTANT_CASE_LABEL
+ // && tree.equals(((ConstantCaseLabelTree) parent).getConstantExpression())) {...}
+ if (parent.getKind().name().equals("CONSTANT_CASE_LABEL")) {
+ try {
+ if (tree.equals(CONSTANT_CASE_LABEL_TREE_GET_EXPRESSION.invoke(parent))) {
+ return null;
+ }
+ } catch (Throwable e) {
+ // MethodHandle.invoke obliges us to catch Throwable here.
+ logger.log(Level.SEVERE, "Could not compare trees", e);
+ }
+ }
}
if (sym.owner.equals(owner) && unit.starImportScope.includes(sym)) {
fix.prefixWith(tree, owner.getSimpleName() + ".");
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/android/IsLoggableTagLength.java b/core/src/main/java/com/google/errorprone/bugpatterns/android/IsLoggableTagLength.java
index 20740965663..2602353ea37 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/android/IsLoggableTagLength.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/android/IsLoggableTagLength.java
@@ -112,6 +112,7 @@ private static VariableTree findEnclosingIdentifier(
.findEnclosing(ClassTree.class)
.accept(
new TreeScanner() {
+ @Nullable
@Override
public VariableTree visitVariable(VariableTree node, Void p) {
return getSymbol(node).equals(identifierSymbol) ? node : null;
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/apidiff/ApiDiffChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/apidiff/ApiDiffChecker.java
index aaee0fb2b55..a2d1afcafe9 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/apidiff/ApiDiffChecker.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/apidiff/ApiDiffChecker.java
@@ -38,6 +38,7 @@
import com.sun.tools.javac.code.Types;
import java.lang.annotation.Annotation;
import java.util.Optional;
+import org.checkerframework.checker.nullness.qual.Nullable;
/** A base Error Prone check implementation to enforce compliance with a given API diff. */
public abstract class ApiDiffChecker extends BugChecker
@@ -125,7 +126,7 @@ private boolean hasAnnotationForbiddingUse(Symbol sym, VisitorState state) {
* Finds the class of the expression's receiver: the declaring class of a static member access, or
* the type that an instance member is accessed on.
*/
- private static ClassSymbol getReceiver(ExpressionTree tree, Symbol sym) {
+ private static @Nullable ClassSymbol getReceiver(ExpressionTree tree, Symbol sym) {
if (sym.isStatic() || sym instanceof ClassSymbol) {
return sym.enclClass();
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentChangeFinder.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentChangeFinder.java
index 4a38cb00f26..8b6bf999aa0 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentChangeFinder.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentChangeFinder.java
@@ -18,6 +18,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.function.Function;
/**
@@ -58,6 +59,7 @@ abstract static class Builder {
* eliminating spurious findings. Heuristics are applied in order so add more expensive checks
* last.
*/
+ @CanIgnoreReturnValue
Builder addHeuristic(Heuristic heuristic) {
heuristicsBuilder().add(heuristic);
return this;
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/EnclosedByReverseHeuristic.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/EnclosedByReverseHeuristic.java
index 2ade769023c..54935fa59b6 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/EnclosedByReverseHeuristic.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/EnclosedByReverseHeuristic.java
@@ -24,6 +24,7 @@
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import java.util.Optional;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* Detect whether the method invocation we are examining is enclosed by either a method or a class
@@ -74,7 +75,7 @@ public boolean isAcceptableChange(
return findReverseWordsMatchInParentNodes(state) == null;
}
- protected String findReverseWordsMatchInParentNodes(VisitorState state) {
+ protected @Nullable String findReverseWordsMatchInParentNodes(VisitorState state) {
for (Tree tree : state.getPath()) {
Optional name = getName(tree);
if (name.isPresent()) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/LowInformationNameHeuristic.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/LowInformationNameHeuristic.java
index 2e0a6df3601..1aa2adaedef 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/LowInformationNameHeuristic.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/LowInformationNameHeuristic.java
@@ -20,6 +20,7 @@
import com.google.errorprone.VisitorState;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* A heuristic for checking if a formal parameter matches a predefined set of words which have been
@@ -58,7 +59,7 @@ public boolean isAcceptableChange(
* Return the first regular expression from the list of overloaded words which matches the
* parameter name.
*/
- protected String findMatch(Parameter parameter) {
+ protected @Nullable String findMatch(Parameter parameter) {
for (String regex : overloadedNamesRegexs) {
if (parameter.name().matches(regex)) {
return regex;
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/BuilderReturnThis.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/BuilderReturnThis.java
new file mode 100644
index 00000000000..36d177867b5
--- /dev/null
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/BuilderReturnThis.java
@@ -0,0 +1,150 @@
+/*
+ * Copyright 2022 The Error Prone Authors.
+ *
+ * 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.checkreturnvalue;
+
+import static com.google.common.base.MoreObjects.firstNonNull;
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.fixes.SuggestedFixes.qualifyType;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.isSameType;
+import static com.google.errorprone.util.ASTHelpers.isSubtype;
+import static java.lang.Boolean.TRUE;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.matchers.Description;
+import com.sun.source.tree.ConditionalExpressionTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.IdentifierTree;
+import com.sun.source.tree.LambdaExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.ParenthesizedTree;
+import com.sun.source.tree.ReturnTree;
+import com.sun.source.tree.TypeCastTree;
+import com.sun.source.util.TreeScanner;
+import com.sun.tools.javac.code.Symbol.ClassSymbol;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import com.sun.tools.javac.code.Type;
+
+/** Discourages builder instance methods that do not return 'this'. */
+@BugPattern(summary = "Builder instance method does not return 'this'", severity = WARNING)
+public class BuilderReturnThis extends BugChecker implements MethodTreeMatcher {
+
+ private static final String CRV = "com.google.errorprone.annotations.CheckReturnValue";
+
+ @Override
+ public Description matchMethod(MethodTree tree, VisitorState state) {
+ MethodSymbol sym = getSymbol(tree);
+ if (tree.getBody() == null) {
+ return NO_MATCH;
+ }
+ if (!instanceReturnsBuilder(sym, state)) {
+ return NO_MATCH;
+ }
+ if (!nonThisReturns(tree, state)) {
+ return NO_MATCH;
+ }
+ SuggestedFix.Builder fix = SuggestedFix.builder();
+ String crvName = qualifyType(state, fix, CRV);
+ fix.prefixWith(tree, "@" + crvName + "\n");
+ return describeMatch(tree, fix.build());
+ }
+
+ private static boolean instanceReturnsBuilder(MethodSymbol sym, VisitorState state) {
+ // instance methods
+ if (sym.isStatic()) {
+ return false;
+ }
+ // declared in a class with the simple name that contains Builder
+ ClassSymbol enclosingClass = sym.owner.enclClass();
+ if (!enclosingClass.getSimpleName().toString().endsWith("Builder")) {
+ return false;
+ }
+ // whose return type is the exact type of this
+ // or perhaps "a non-Object supertype of the this-type", for interfaces
+ Type returnType = sym.getReturnType();
+ if (!isSubtype(enclosingClass.asType(), returnType, state)
+ || isSameType(returnType, state.getSymtab().objectType, state)) {
+ return false;
+ }
+ return true;
+ }
+
+ // TODO(b/236055787): consolidate heuristics for 'return this;'
+ boolean nonThisReturns(MethodTree tree, VisitorState state) {
+
+ boolean[] result = {false};
+ new TreeScanner() {
+ @Override
+ public Void visitLambdaExpression(LambdaExpressionTree tree, Void unused) {
+ return null;
+ }
+
+ @Override
+ public Void visitMethod(MethodTree tree, Void unused) {
+ return null;
+ }
+
+ @Override
+ public Void visitReturn(ReturnTree tree, Void unused) {
+ if (!returnsThis(tree.getExpression())) {
+ result[0] = true;
+ }
+ return super.visitReturn(tree, null);
+ }
+
+ private boolean returnsThis(ExpressionTree tree) {
+ return firstNonNull(
+ new TreeScanner() {
+ @Override
+ public Boolean visitIdentifier(IdentifierTree tree, Void unused) {
+ return tree.getName().contentEquals("this");
+ }
+
+ @Override
+ public Boolean visitMethodInvocation(MethodInvocationTree tree, Void unused) {
+ return instanceReturnsBuilder(getSymbol(tree), state);
+ }
+
+ @Override
+ public Boolean visitConditionalExpression(
+ ConditionalExpressionTree tree, Void unused) {
+ return TRUE.equals(tree.getFalseExpression().accept(this, null))
+ && TRUE.equals(tree.getTrueExpression().accept(this, null));
+ }
+
+ @Override
+ public Boolean visitParenthesized(ParenthesizedTree tree, Void unused) {
+ return tree.getExpression().accept(this, null);
+ }
+
+ @Override
+ public Boolean visitTypeCast(TypeCastTree tree, Void unused) {
+ return tree.getExpression().accept(this, null);
+ }
+ }.scan(tree, null),
+ false);
+ }
+ }.scan(tree.getBody(), null);
+ return result[0];
+ }
+}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java
new file mode 100644
index 00000000000..87f861c3594
--- /dev/null
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java
@@ -0,0 +1,222 @@
+/*
+ * Copyright 2022 The Error Prone Authors.
+ *
+ * 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.checkreturnvalue;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.fixes.SuggestedFixes.qualifyType;
+import static com.google.errorprone.util.ASTHelpers.getReceiver;
+import static com.google.errorprone.util.ASTHelpers.getReturnType;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.getType;
+import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
+import static com.google.errorprone.util.ASTHelpers.isSubtype;
+import static com.google.errorprone.util.ASTHelpers.isVoidType;
+import static com.google.errorprone.util.ASTHelpers.stripParentheses;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.suppliers.Supplier;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.IdentifierTree;
+import com.sun.source.tree.LambdaExpressionTree;
+import com.sun.source.tree.MemberSelectTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.NewClassTree;
+import com.sun.source.tree.ReturnTree;
+import com.sun.source.tree.StatementTree;
+import com.sun.source.util.TreeScanner;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import com.sun.tools.javac.code.Type;
+
+/**
+ * Checker that recommends annotating a method with {@code @CanIgnoreReturnValue} if the method
+ * returns {@code this} (or other methods that are likely to also just return {@code this}).
+ */
+@BugPattern(
+ summary = "Methods that always 'return this' should be annotated with @CanIgnoreReturnValue",
+ severity = WARNING)
+public final class CanIgnoreReturnValueSuggester extends BugChecker implements MethodTreeMatcher {
+ private static final String CRV = "com.google.errorprone.annotations.CheckReturnValue";
+ private static final String CIRV = "com.google.errorprone.annotations.CanIgnoreReturnValue";
+
+ private static final Supplier PROTO_BUILDER =
+ VisitorState.memoize(s -> s.getTypeFromString("com.google.protobuf.MessageLite.Builder"));
+
+ // TODO(kak): catch places where an input parameter is always returned
+
+ @Override
+ public Description matchMethod(MethodTree methodTree, VisitorState state) {
+ MethodSymbol methodSymbol = getSymbol(methodTree);
+
+ // We have a number of preconditions we can check early to ensure that this method could
+ // possibly be @CIRV-suggestible, before attempting a deeper scan of the method.
+ if (methodSymbol.isStatic()
+ || methodTree.getBody() == null
+ // These methods should probably be @CheckReturnValue!
+ || isDefinitionOfZeroArgSelf(methodSymbol)
+ // Constructors can't "return", and generally shouldn't be @CIRV
+ || methodTree.getReturnType() == null
+ // methods whose return type is void or Void can't have @CIRV
+ || isVoidType(getType(methodTree.getReturnType()), state)
+ // b/236423646 - These methods that do nothing *but* `return this;` are likely to be
+ // overridden in other contexts, and we've decided that these methods shouldn't be annotated
+ // automatically.
+ || isSimpleReturnThisMethod(methodTree)
+ // TODO(kak): This appears to be a performance optimization for refactoring passes?
+ || isSubtype(methodSymbol.owner.type, PROTO_BUILDER.get(state), state)
+ || hasAnnotation(methodSymbol, CIRV, state)) {
+ return Description.NO_MATCH;
+ }
+
+ // if the enclosing type is already annotated with CIRV, we could theoretically _not_ directly
+ // annotate the method but we're likely to discourage annotating types with CIRV: b/229776283
+ // if the method is already directly annotated w/ @CRV, bail out
+ if (hasAnnotation(methodTree, CRV, state)) {
+ // TODO(kak): we might want to actually _remove_ @CRV and add @CIRV in this case!
+ return Description.NO_MATCH;
+ }
+
+ // OK, now the real implementation: For each possible return branch, does the expression
+ // returned look like "this" or instance methods that are also @CanIgnoreReturnValue.
+ if (!methodReturnsIgnorableValues(methodTree, state)) {
+ return Description.NO_MATCH;
+ }
+
+ SuggestedFix.Builder fix = SuggestedFix.builder();
+ String cirvName = qualifyType(state, fix, CIRV);
+ // we could add a trailing comment (e.g., @CanIgnoreReturnValue // returns `this`), but all
+ // developers will become familiar with these annotations sooner or later
+ fix.prefixWith(methodTree, "@" + cirvName + "\n");
+
+ return describeMatch(methodTree, fix.build());
+ }
+
+ private static boolean isSimpleReturnThisMethod(MethodTree methodTree) {
+ if (methodTree.getBody().getStatements().size() == 1) {
+ StatementTree onlyStatement = methodTree.getBody().getStatements().get(0);
+ if (onlyStatement instanceof ReturnTree) {
+ return returnsThisOrSelf((ReturnTree) onlyStatement);
+ }
+ }
+ return false;
+ }
+
+ private static boolean isIdentifier(ExpressionTree expr, String identifierName) {
+ expr = stripParentheses(expr);
+ if (expr instanceof IdentifierTree) {
+ return ((IdentifierTree) expr).getName().contentEquals(identifierName);
+ }
+ return false;
+ }
+
+ /** Returns whether or not the given {@link ReturnTree} returns exactly {@code this}. */
+ private static boolean returnsThisOrSelf(ReturnTree returnTree) {
+ return isIdentifier(returnTree.getExpression(), "this")
+ || (returnTree.getExpression() instanceof MethodInvocationTree
+ && isCallToZeroArgSelf((MethodInvocationTree) returnTree.getExpression()));
+ }
+
+ // this.self() or self()
+ private static boolean isCallToZeroArgSelf(MethodInvocationTree mit) {
+ if (!mit.getArguments().isEmpty()) {
+ return false;
+ }
+ if (isIdentifier(mit.getMethodSelect(), "self")) {
+ return true;
+ }
+ if (mit.getMethodSelect() instanceof MemberSelectTree) {
+ MemberSelectTree methodSelect = (MemberSelectTree) mit.getMethodSelect();
+ return isIdentifier(methodSelect.getExpression(), "this")
+ && methodSelect.getIdentifier().contentEquals("self");
+ }
+ return false;
+ }
+
+ private static boolean isDefinitionOfZeroArgSelf(MethodSymbol methodSymbol) {
+ return methodSymbol.getSimpleName().contentEquals("self")
+ && methodSymbol.getParameters().isEmpty();
+ }
+
+ private static boolean methodReturnsIgnorableValues(MethodTree tree, VisitorState state) {
+ class ReturnValuesFromMethodAreIgnorable extends TreeScanner {
+ private final VisitorState state;
+ private final Type enclosingClassType;
+ private final Type methodReturnType;
+ private boolean atLeastOneReturn = false;
+ private boolean allReturnsIgnorable = true;
+
+ private ReturnValuesFromMethodAreIgnorable(VisitorState state, MethodSymbol methSymbol) {
+ this.state = state;
+ this.methodReturnType = methSymbol.getReturnType();
+ this.enclosingClassType = methSymbol.enclClass().type;
+ }
+
+ @Override
+ public Void visitReturn(ReturnTree returnTree, Void unused) {
+ atLeastOneReturn = true;
+ if (!returnsThisOrSelf(returnTree)
+ && !isIgnorableMethodCallOnSameInstance(returnTree, state)) {
+ allReturnsIgnorable = false;
+ }
+ // Don't descend deeper into returns, since we already checked the body of this return.
+ return null;
+ }
+
+ private boolean isIgnorableMethodCallOnSameInstance(
+ ReturnTree returnTree, VisitorState state) {
+ if (returnTree.getExpression() instanceof MethodInvocationTree) {
+ MethodInvocationTree mit = (MethodInvocationTree) returnTree.getExpression();
+ ExpressionTree receiver = getReceiver(mit);
+ MethodSymbol calledMethod = getSymbol(mit);
+ if ((receiver == null && !calledMethod.isStatic())
+ || isIdentifier(receiver, "this")
+ || isIdentifier(receiver, "super")) {
+ // If the method we're calling is @CIRV and the enclosing class could be represented by
+ // the object being returned by the other method, then it's probable that the other
+ // method is likely to
+ // be an ignorable result.
+ return hasAnnotation(calledMethod, CIRV, state)
+ && isSubtype(enclosingClassType, methodReturnType, state)
+ && isSubtype(enclosingClassType, getReturnType(mit), state);
+ }
+ }
+ return false;
+ }
+
+ @Override
+ public Void visitLambdaExpression(LambdaExpressionTree node, Void unused) {
+ // don't descend into lambdas
+ return null;
+ }
+
+ @Override
+ public Void visitNewClass(NewClassTree node, Void unused) {
+ // don't descend into declarations of anonymous classes
+ return null;
+ }
+ }
+
+ var scanner = new ReturnValuesFromMethodAreIgnorable(state, getSymbol(tree));
+ scanner.scan(tree, null);
+ return scanner.atLeastOneReturn && scanner.allReturnsIgnorable;
+ }
+}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ExternalCanIgnoreReturnValue.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ExternalCanIgnoreReturnValue.java
index b64fc90de99..520d7d42f5b 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ExternalCanIgnoreReturnValue.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ExternalCanIgnoreReturnValue.java
@@ -61,6 +61,7 @@ private ExternalCanIgnoreReturnValue() {}
.errorProneOptions()
.getFlags()
.get(EXTERNAL_API_EXCLUSION_LIST)
+ .filter(s -> !s.isEmpty())
.map(
filename ->
loadConfigListFromFile(filename, state.errorProneOptions().getFlags()))
@@ -80,7 +81,7 @@ public Optional evaluateMethod(MethodSymbol method, VisitorStat
/** Encapsulates asking "does this API match the list of APIs I care about"? */
@FunctionalInterface
- private interface MethodPredicate {
+ interface MethodPredicate {
boolean methodMatches(MethodSymbol methodSymbol, VisitorState state);
}
@@ -90,26 +91,25 @@ private interface MethodPredicate {
enum ConfigParser {
AS_STRINGS {
@Override
- MethodPredicate load(CharSource file) throws IOException {
- return configByInterpretingMethodsAsStrings(file);
+ MethodPredicate load(String file, ErrorProneFlags flags) throws IOException {
+ return configByInterpretingMethodsAsStrings(MoreFiles.asCharSource(Paths.get(file), UTF_8));
}
},
PARSE_TOKENS {
@Override
- MethodPredicate load(CharSource file) throws IOException {
- return configByParsingApiObjects(file);
+ MethodPredicate load(String file, ErrorProneFlags flags) throws IOException {
+ return configByParsingApiObjects(MoreFiles.asCharSource(Paths.get(file), UTF_8));
}
};
- abstract MethodPredicate load(CharSource file) throws IOException;
+ abstract MethodPredicate load(String file, ErrorProneFlags flags) throws IOException;
}
private static MethodPredicate loadConfigListFromFile(String filename, ErrorProneFlags flags) {
ConfigParser configParser =
flags.getEnum(EXCLUSION_LIST_PARSER, ConfigParser.class).orElse(ConfigParser.AS_STRINGS);
try {
- CharSource file = MoreFiles.asCharSource(Paths.get(filename), UTF_8);
- return configParser.load(file);
+ return configParser.load(filename, flags);
} catch (IOException e) {
throw new UncheckedIOException(
"Could not load external resource for CanIgnoreReturnValue", e);
@@ -133,20 +133,7 @@ public boolean methodMatches(MethodSymbol methodSymbol, VisitorState state) {
private String apiSignature(MethodSymbol methodSymbol, Types types) {
return methodSymbol.owner.getQualifiedName()
+ "#"
- + methodSymbol.name
- + "("
- + paramsString(methodSymbol, types)
- + ")";
- }
-
- private String paramsString(MethodSymbol symbol, Types types) {
- if (symbol.params().isEmpty()) {
- return "";
- }
- return String.join(
- ",",
- Iterables.transform(
- symbol.params(), p -> fullyErasedAndUnannotatedType(p.type, types)));
+ + methodNameAndParams(methodSymbol, types);
}
};
}
@@ -160,7 +147,7 @@ private static MethodPredicate configByParsingApiObjects(CharSource file) throws
.collect(toImmutableSetMultimap(Api::className, api -> api));
}
return (methodSymbol, state) ->
- apis.get(methodSymbol.enclClass().getQualifiedName().toString()).stream()
+ apis.get(surroundingClass(methodSymbol)).stream()
.anyMatch(
api ->
methodSymbol.getSimpleName().contentEquals(api.methodName())
@@ -168,10 +155,26 @@ && methodParametersMatch(
api.parameterTypes(), methodSymbol.params(), state.getTypes()));
}
+ public static String surroundingClass(MethodSymbol methodSymbol) {
+ return methodSymbol.enclClass().getQualifiedName().toString();
+ }
+
+ public static String methodNameAndParams(MethodSymbol methodSymbol, Types types) {
+ return methodSymbol.name + "(" + paramsString(types, methodSymbol.params()) + ")";
+ }
+
private static boolean methodParametersMatch(
ImmutableList parameters, List methodParams, Types types) {
return Iterables.elementsEqual(
parameters,
Iterables.transform(methodParams, p -> fullyErasedAndUnannotatedType(p.type, types)));
}
+
+ private static String paramsString(Types types, List params) {
+ if (params.isEmpty()) {
+ return "";
+ }
+ return String.join(
+ ",", Iterables.transform(params, p -> fullyErasedAndUnannotatedType(p.type, types)));
+ }
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/NoCanIgnoreReturnValueOnClasses.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/NoCanIgnoreReturnValueOnClasses.java
new file mode 100644
index 00000000000..cfbd907270d
--- /dev/null
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/NoCanIgnoreReturnValueOnClasses.java
@@ -0,0 +1,247 @@
+/*
+ * Copyright 2022 The Error Prone Authors.
+ *
+ * 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.checkreturnvalue;
+
+import static com.google.common.base.MoreObjects.firstNonNull;
+import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+import static com.google.errorprone.fixes.SuggestedFixes.qualifyType;
+import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE;
+import static com.google.errorprone.matchers.Matchers.annotations;
+import static com.google.errorprone.matchers.Matchers.isType;
+import static com.google.errorprone.util.ASTHelpers.enclosingClass;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.getType;
+import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
+import static com.google.errorprone.util.ASTHelpers.isConsideredFinal;
+import static com.google.errorprone.util.ASTHelpers.isGeneratedConstructor;
+import static com.google.errorprone.util.ASTHelpers.isVoidType;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.errorprone.BugPattern;
+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.google.errorprone.matchers.MultiMatcher;
+import com.google.errorprone.matchers.MultiMatcher.MultiMatchResult;
+import com.sun.source.tree.AnnotationTree;
+import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.IdentifierTree;
+import com.sun.source.tree.LambdaExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.NewClassTree;
+import com.sun.source.tree.ReturnTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.TypeCastTree;
+import com.sun.source.tree.VariableTree;
+import com.sun.source.util.SimpleTreeVisitor;
+import com.sun.source.util.TreePathScanner;
+import com.sun.tools.javac.code.Symbol.ClassSymbol;
+import com.sun.tools.javac.code.Symbol.VarSymbol;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/**
+ * Checker that "pushes" the {@code @CanIgnoreReturnValue} annotation down from classes to methods.
+ */
+@BugPattern(
+ summary =
+ "@CanIgnoreReturnValue should not be applied to classes as it almost always overmatches (as"
+ + " it applies to constructors and all methods), and the CIRVness isn't conferred to"
+ + " its subclasses.",
+ severity = ERROR)
+public final class NoCanIgnoreReturnValueOnClasses extends BugChecker implements ClassTreeMatcher {
+ private static final String CRV = "com.google.errorprone.annotations.CheckReturnValue";
+ private static final String CIRV = "com.google.errorprone.annotations.CanIgnoreReturnValue";
+
+ private static final String EXTRA_SUFFIX = "";
+
+ @VisibleForTesting
+ static final String METHOD_COMMENT = " // pushed down from class to method;" + EXTRA_SUFFIX;
+
+ @VisibleForTesting
+ static final String CTOR_COMMENT = " // pushed down from class to constructor;" + EXTRA_SUFFIX;
+
+ private static final MultiMatcher HAS_CIRV_ANNOTATION =
+ annotations(AT_LEAST_ONE, isType(CIRV));
+
+ @Override
+ public Description matchClass(ClassTree tree, VisitorState state) {
+ MultiMatchResult cirvAnnotation =
+ HAS_CIRV_ANNOTATION.multiMatchResult(tree, state);
+ // if the class isn't directly annotated w/ @CIRV, bail out
+ if (!cirvAnnotation.matches()) {
+ return Description.NO_MATCH;
+ }
+
+ SuggestedFix.Builder fix = SuggestedFix.builder();
+ String cirvName = qualifyType(state, fix, CIRV);
+
+ // remove @CIRV from the class
+ fix.delete(cirvAnnotation.onlyMatchingNode());
+
+ // theoretically, we could also add @CRV to the class, since all APIs will have CIRV pushed down
+ // onto them, but it's very likely that a larger enclosing scope will already be @CRV (otherwise
+ // why did the user annotate this class as @CIRV?)
+
+ // scan the tree and add @CIRV to all non-void method declarations that aren't already annotated
+ // with @CIRV or @CRV
+ new TreePathScanner() {
+ @Override
+ public Void visitClass(ClassTree classTree, Void unused) {
+ // stop descending when we reach a class that's marked @CRV
+ return hasAnnotation(classTree, CRV, state) ? null : super.visitClass(classTree, unused);
+ }
+
+ @Override
+ public Void visitMethod(MethodTree methodTree, Void unused) {
+ if (shouldAddCirv(methodTree, state)) {
+ String trailingComment = null;
+
+ if (methodTree.getReturnType() == null) { // constructor
+ trailingComment = CTOR_COMMENT;
+ } else if (alwaysReturnsThis()) {
+ trailingComment = "";
+ } else {
+ trailingComment = METHOD_COMMENT;
+ }
+
+ fix.prefixWith(methodTree, "@" + cirvName + trailingComment + "\n");
+ }
+ // TODO(kak): we could also consider removing CRV from individual methods (since the
+ // enclosing class is now annotated as CRV.
+ return null;
+ }
+
+ private boolean alwaysReturnsThis() {
+ // TODO(b/236055787): share this TreePathScanner
+ AtomicBoolean allReturnThis = new AtomicBoolean(true);
+ AtomicBoolean atLeastOneReturn = new AtomicBoolean(false);
+
+ new TreePathScanner() {
+ private final Set thises = new HashSet<>();
+
+ @Override
+ public Void visitVariable(VariableTree variableTree, Void unused) {
+ VarSymbol symbol = getSymbol(variableTree);
+ if (isConsideredFinal(symbol) && maybeCastThis(variableTree.getInitializer())) {
+ thises.add(symbol);
+ }
+ return super.visitVariable(variableTree, null);
+ }
+
+ @Override
+ public Void visitReturn(ReturnTree returnTree, Void unused) {
+ atLeastOneReturn.set(true);
+ if (!isThis(returnTree.getExpression())) {
+ allReturnThis.set(false);
+ // once we've set allReturnThis to false, no need to descend further
+ return null;
+ }
+ return super.visitReturn(returnTree, null);
+ }
+
+ /** Returns whether the given {@link ExpressionTree} is {@code this}. */
+ private boolean isThis(ExpressionTree returnExpression) {
+ return maybeCastThis(returnExpression) || thises.contains(getSymbol(returnExpression));
+ }
+
+ @Override
+ public Void visitLambdaExpression(LambdaExpressionTree node, Void unused) {
+ // don't descend into lambdas
+ return null;
+ }
+
+ @Override
+ public Void visitNewClass(NewClassTree node, Void unused) {
+ // don't descend into declarations of anonymous classes
+ return null;
+ }
+
+ private boolean maybeCastThis(Tree tree) {
+ return firstNonNull(
+ new SimpleTreeVisitor() {
+
+ @Override
+ public Boolean visitTypeCast(TypeCastTree tree, Void unused) {
+ return visit(tree.getExpression(), null);
+ }
+
+ @Override
+ public Boolean visitIdentifier(IdentifierTree tree, Void unused) {
+ return tree.getName().contentEquals("this");
+ }
+
+ @Override
+ public Boolean visitMethodInvocation(MethodInvocationTree tree, Void unused) {
+ return getSymbol(tree).getSimpleName().contentEquals("self");
+ }
+ }.visit(tree, null),
+ false);
+ }
+ }.scan(getCurrentPath(), null);
+
+ return allReturnThis.get() && atLeastOneReturn.get();
+ }
+
+ private boolean shouldAddCirv(MethodTree methodTree, VisitorState state) {
+ if (isVoidType(getType(methodTree.getReturnType()), state)) { // void return types
+ return false;
+ }
+ if (hasAnnotation(methodTree, CIRV, state)) {
+ return false;
+ }
+ if (hasAnnotation(methodTree, CRV, state)) {
+ return false;
+ }
+ // if the constructor is implicit, don't add CIRV (we can't annotate a synthetic node!)
+ if (isGeneratedConstructor(methodTree)) {
+ return false;
+ }
+ // if the method is inside an AV or AV.Builder and is abstract (no body), don't add CIRV
+ ClassSymbol enclosingClass = enclosingClass(getSymbol(methodTree));
+ if (hasAnnotation(enclosingClass, "com.google.auto.value.AutoValue", state)
+ || hasAnnotation(enclosingClass, "com.google.auto.value.AutoValue.Builder", state)) {
+ if (methodTree.getBody() == null) {
+ return false;
+ }
+ }
+ // TODO(kak): should we also return false for private methods? I'm betting most of them are
+ // "accidentally" CIRV'ed by the enclosing class; any compile errors would be caught by
+ // building the enclosing class anyways.
+ return true;
+ }
+
+ @Override
+ public Void visitLambdaExpression(LambdaExpressionTree node, Void unused) {
+ // don't descend into lambdas
+ return null;
+ }
+
+ @Override
+ public Void visitNewClass(NewClassTree node, Void unused) {
+ // don't descend into declarations of anonymous classes
+ return null;
+ }
+ }.scan(state.getPath(), null);
+ return describeMatch(tree, fix.build());
+ }
+}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/PackagesRule.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/PackagesRule.java
new file mode 100644
index 00000000000..4402771e897
--- /dev/null
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/PackagesRule.java
@@ -0,0 +1,82 @@
+/*
+ * Copyright 2022 The Error Prone Authors.
+ *
+ * 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.checkreturnvalue;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.SymbolRule;
+import com.google.errorprone.suppliers.Supplier;
+import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Symbol.PackageSymbol;
+import com.sun.tools.javac.util.Name;
+import java.util.Optional;
+
+/**
+ * A rule that enables checking for methods belonging to a set of packages or any of their
+ * subpackages.
+ */
+public final class PackagesRule extends SymbolRule {
+
+ /**
+ * Returns a new rule using the given package {@code patterns}. Each pattern string must either be
+ * the fully qualified name of a package (to enable checking for methods in that package and its
+ * subpackages) or a {@code -} character followed by the fully qualified name of a package (to
+ * disable checking for methods in that package and its subpackages).
+ */
+ public static PackagesRule fromPatterns(Iterable patterns) {
+ return new PackagesRule(ImmutableList.copyOf(patterns));
+ }
+
+ private final Supplier> packagesSupplier;
+
+ private PackagesRule(ImmutableList patterns) {
+ this.packagesSupplier =
+ VisitorState.memoize(
+ state -> {
+ ImmutableMap.Builder builder = ImmutableMap.builder();
+ for (String pattern : patterns) {
+ if (pattern.charAt(0) == '-') {
+ builder.put(state.getName(pattern.substring(1)), false);
+ } else {
+ builder.put(state.getName(pattern), true);
+ }
+ }
+ return builder.buildOrThrow();
+ });
+ }
+
+ @Override
+ public final String id() {
+ return "Packages";
+ }
+
+ @Override
+ public Optional evaluate(Symbol symbol, VisitorState state) {
+ while (symbol instanceof PackageSymbol) {
+ Boolean value = packagesSupplier.get(state).get(((PackageSymbol) symbol).fullname);
+ if (value != null) {
+ return value
+ ? Optional.of(ResultUsePolicy.EXPECTED)
+ // stop evaluating if the package matched a negative pattern
+ : Optional.empty();
+ }
+ symbol = symbol.owner;
+ }
+ return Optional.empty();
+ }
+}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ProtoRules.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ProtoRules.java
index ae3c334d82b..8350f68c5a1 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ProtoRules.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ProtoRules.java
@@ -53,8 +53,10 @@ public static ResultUseRule mutableProtos() {
/** Rules for methods on protos. */
private static final class ProtoRule extends MethodRule {
- private static final Pattern RETURNS_THIS =
- Pattern.compile("(add|clear|merge|remove|set|put).*");
+ // Methods that start this way produce a modification to the proto, and either return this
+ // or return the parameter given, for chaining purposes.
+ private static final Pattern NAMED_MUTATOR_METHOD =
+ Pattern.compile("(add|clear|insert|merge|remove|set|put).*");
private final Supplier parentType;
private final String id;
@@ -73,11 +75,10 @@ public String id() {
public Optional evaluateMethod(MethodSymbol method, VisitorState state) {
if (isProtoSubtype(method.owner.type, state)) {
String methodName = method.name.toString();
- if (RETURNS_THIS.matcher(methodName).matches()) {
+ if (NAMED_MUTATOR_METHOD.matcher(methodName).matches()) {
return Optional.of(ResultUsePolicy.OPTIONAL);
}
- if (isGetterOfSubmessageBuilder(methodName)
- && isProtoSubtype(method.getReturnType(), state)) {
+ if (isMutatingAccessorMethod(methodName) && isProtoSubtype(method.getReturnType(), state)) {
return Optional.of(ResultUsePolicy.OPTIONAL);
}
}
@@ -88,11 +89,16 @@ private boolean isProtoSubtype(Type ownerType, VisitorState state) {
return isSubtype(ownerType, parentType.get(state), state);
}
- // fooBuilder.getBarBuilder() mutates the builder such that foo.hasBar() is now true.
- private static boolean isGetterOfSubmessageBuilder(String name) {
+ private static boolean isMutatingAccessorMethod(String name) {
// TODO(glorioso): Any other naming conventions to check?
// TODO(glorioso): Maybe worth making this a regex instead? But think about performance
- return name.startsWith("get") && name.endsWith("Builder") && !name.endsWith("OrBuilder");
+ if (name.startsWith("get")) {
+ // fooBuilder.getBarBuilder() mutates the builder such that foo.hasBar() is now true.
+ return (name.endsWith("Builder") && !name.endsWith("OrBuilder"))
+ // mutableFoo.getMutableBar() mutates Foo so that mutableFoo.hasBar() is now true
+ || name.startsWith("getMutable");
+ }
+ return false;
}
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/AbstractCollectionIncompatibleTypeMatcher.java b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/AbstractCollectionIncompatibleTypeMatcher.java
index 995e9a42cdf..59184c9956f 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/AbstractCollectionIncompatibleTypeMatcher.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/AbstractCollectionIncompatibleTypeMatcher.java
@@ -160,6 +160,7 @@ public MatchResult visitMemberReference(
}.visit(tree, null);
}
+ @Nullable
private MatchResult getMatchResult(
@Nullable ExpressionTree sourceTree, @Nullable Type sourceType, @Nullable Type targetType) {
if (sourceTree == null || sourceType == null || targetType == null) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CompatibleWithMisuse.java b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CompatibleWithMisuse.java
index d087ac9644b..e81e5eedd47 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CompatibleWithMisuse.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CompatibleWithMisuse.java
@@ -44,6 +44,7 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* @author glorioso@google.com (Nick Glorioso)
@@ -127,7 +128,7 @@ public Description matchAnnotation(AnnotationTree annoTree, VisitorState state)
// => X
// This function assumes the annotation tree will only have one argument, of type String, that
// is required.
- private static String valueArgumentFromCompatibleWithAnnotation(AnnotationTree tree) {
+ private static @Nullable String valueArgumentFromCompatibleWithAnnotation(AnnotationTree tree) {
ExpressionTree argumentValue = Iterables.getOnlyElement(tree.getArguments());
if (argumentValue.getKind() != Kind.ASSIGNMENT) {
// :-| Annotation symbol broken. Punt?
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/IncompatibleArgumentType.java b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/IncompatibleArgumentType.java
index b2e5c72032a..d1e864b5263 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/IncompatibleArgumentType.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/IncompatibleArgumentType.java
@@ -225,6 +225,7 @@ private static RequiredType resolveRequiredTypeForThisCall(
return requiredType;
}
+ @Nullable
private static RequiredType resolveTypeFromGenericMethod(
Type calledMethodType, MethodSymbol declaredMethod, String typeArgName) {
int tyargIndex = findTypeArgInList(declaredMethod, typeArgName);
@@ -236,6 +237,7 @@ private static RequiredType resolveTypeFromGenericMethod(
// Plumb through a type which is supposed to be a Types.Subst, then find the replacement
// type that the compiler resolved.
+ @Nullable
private static Type getTypeFromTypeMapping(
Type m, MethodSymbol declaredMethod, String namedTypeArg) {
ImmutableListMultimap substitutions =
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerArgumentToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerArgumentToString.java
index b7a9b762c2c..08e0a325c0e 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerArgumentToString.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerArgumentToString.java
@@ -355,6 +355,7 @@ Description unwrapArguments(
return describeMatch(tree, fix.build());
}
+ @Nullable
private static Parameter unwrap(ExpressionTree argument, char placeholder, VisitorState state) {
for (Unwrapper unwrapper : Unwrapper.values()) {
if (unwrapper.matcher.matches(argument, state)) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerRedundantIsEnabled.java b/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerRedundantIsEnabled.java
index e41106dc8bb..6d42b3f0fbe 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerRedundantIsEnabled.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerRedundantIsEnabled.java
@@ -47,6 +47,7 @@
import com.sun.tools.javac.code.Symbol;
import java.util.List;
import java.util.Optional;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* @author mariasam@google.com (Maria Sam)
@@ -136,7 +137,7 @@ private static ExpressionTree unwrap(ExpressionTree expr) {
new SimpleTreeVisitor() {
@Override
- protected ExpressionTree defaultAction(Tree tree, Void unused) {
+ protected @Nullable ExpressionTree defaultAction(Tree tree, Void unused) {
return tree instanceof ExpressionTree ? (ExpressionTree) tree : null;
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/formatstring/StrictFormatStringValidation.java b/core/src/main/java/com/google/errorprone/bugpatterns/formatstring/StrictFormatStringValidation.java
index 2d5ec6ab1df..43c047cb73e 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/formatstring/StrictFormatStringValidation.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/formatstring/StrictFormatStringValidation.java
@@ -100,6 +100,7 @@ public static ValidationResult validate(
}
/** Helps {@code validate()} validate a format string that is declared as a method parameter. */
+ @Nullable
private static ValidationResult validateFormatStringParameter(
ExpressionTree formatStringTree,
Symbol formatStringSymbol,
@@ -219,6 +220,7 @@ public ValidationResult visitVariable(VariableTree node, Void unused) {
return super.visitVariable(node, unused);
}
+ @Nullable
@Override
public ValidationResult reduce(ValidationResult r1, ValidationResult r2) {
if (r1 == null && r2 == null) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/inject/dagger/AndroidInjectionBeforeSuper.java b/core/src/main/java/com/google/errorprone/bugpatterns/inject/dagger/AndroidInjectionBeforeSuper.java
index f7168ff2738..a378f57b23c 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/inject/dagger/AndroidInjectionBeforeSuper.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/inject/dagger/AndroidInjectionBeforeSuper.java
@@ -42,6 +42,7 @@
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.VariableTree;
import com.sun.source.util.SimpleTreeVisitor;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* @author Ron Shapiro
@@ -127,7 +128,7 @@ private final class LifecycleMethodVisitor extends SimpleTreeVisitor No match
+ if (!ASTHelpers.hasAnnotation(enclosingMethodSym, "dagger.Provides", state)) {
return Description.NO_MATCH;
}
+ // Method is annotated as Nullable -> No match
+ if (ASTHelpers.hasDirectAnnotationWithSimpleName(enclosingMethodSym, "Nullable")) {
+ return Description.NO_MATCH;
+ }
+ // Type-use annotations do *NOT* work with Dagger. See b/117251022
+ // You must use *any* non-type-use Nullable annotation.
Fix addNullableFix =
SuggestedFix.builder()
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java b/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java
index 10885ba8f3b..feb5b8ef9e4 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java
@@ -240,7 +240,7 @@ && stringContainsComments(state.getSourceForNode(tree), state.context)) {
boolean terminalVarargsReplacement = varargsWithEmptyArguments && i == varNames.size() - 1;
String capturePrefixForVarargs = terminalVarargsReplacement ? "(?:,\\s*)?" : "";
// We want to avoid replacing a method invocation with the same name as the method.
- Pattern extractArgAndNextToken =
+ var extractArgAndNextToken =
Pattern.compile(
"\\b" + capturePrefixForVarargs + Pattern.quote(varNames.get(i)) + "\\b([^(])");
String replacementResult =
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidBlockTag.java b/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidBlockTag.java
index 4c9dc0dc9fa..d3f7e055e2f 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidBlockTag.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidBlockTag.java
@@ -192,7 +192,7 @@ public Void visitUnknownBlockTag(UnknownBlockTagTree unknownBlockTagTree, Void u
String message =
String.format(
"@%1$s is not a valid tag, but is a parameter name. "
- + "Use {@code %1%s} to refer to parameter names inline.",
+ + "Use {@code %1$s} to refer to parameter names inline.",
tagName);
state.reportMatch(
buildDescription(diagnosticPosition(getCurrentPath(), state))
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidInlineTag.java b/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidInlineTag.java
index 2523d727a3c..95eed6453f9 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidInlineTag.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidInlineTag.java
@@ -74,26 +74,42 @@ public final class InvalidInlineTag extends BugChecker
private static final Splitter DOT_SPLITTER = Splitter.on('.');
+ private void scanTags(
+ VisitorState state, Context context, ImmutableSet parameters, DocTreePath path) {
+ new InvalidTagChecker(state, context, parameters).scan(path, null);
+ }
+
+ private enum Context {
+ CLASS(JavadocTag.VALID_CLASS_TAGS),
+ METHOD(JavadocTag.VALID_METHOD_TAGS),
+ VARIABLE(JavadocTag.VALID_VARIABLE_TAGS);
+
+ final ImmutableSet validTags;
+ final Pattern misplacedCurly;
+ final Pattern parensRatherThanCurly;
+
+ Context(ImmutableSet validTags) {
+ this.validTags = validTags;
+ String validInlineTags =
+ validTags.stream()
+ .filter(tag -> tag.type() == TagType.INLINE)
+ .map(JavadocTag::name)
+ .collect(joining("|"));
+ this.misplacedCurly = Pattern.compile(String.format("@(%s)\\{", validInlineTags));
+ this.parensRatherThanCurly = Pattern.compile(String.format("\\(@(%s)", validInlineTags));
+ }
+ }
+
@Override
public Description matchClass(ClassTree classTree, VisitorState state) {
DocTreePath path = Utils.getDocTreePath(state);
if (path != null) {
ImmutableSet parameters = ImmutableSet.of();
- scanTags(state, JavadocTag.VALID_CLASS_TAGS, parameters, path);
+ scanTags(state, Context.CLASS, parameters, path);
}
return Description.NO_MATCH;
}
- private void scanTags(
- VisitorState state,
- ImmutableSet tags,
- ImmutableSet parameters,
- DocTreePath path) {
- try (InvalidTagChecker checker = new InvalidTagChecker(state, tags, parameters)) {
- checker.scan(path, null);
- }
- }
-
@Override
public Description matchMethod(MethodTree methodTree, VisitorState state) {
DocTreePath path = Utils.getDocTreePath(state);
@@ -102,7 +118,7 @@ public Description matchMethod(MethodTree methodTree, VisitorState state) {
methodTree.getParameters().stream()
.map(v -> v.getName().toString())
.collect(toImmutableSet());
- scanTags(state, JavadocTag.VALID_METHOD_TAGS, parameters, path);
+ scanTags(state, Context.METHOD, parameters, path);
}
return Description.NO_MATCH;
}
@@ -111,7 +127,7 @@ public Description matchMethod(MethodTree methodTree, VisitorState state) {
public Description matchVariable(VariableTree variableTree, VisitorState state) {
DocTreePath path = Utils.getDocTreePath(state);
if (path != null) {
- scanTags(state, JavadocTag.VALID_VARIABLE_TAGS, /* parameters= */ ImmutableSet.of(), path);
+ scanTags(state, Context.VARIABLE, /* parameters= */ ImmutableSet.of(), path);
}
return Description.NO_MATCH;
}
@@ -123,29 +139,19 @@ static String getMessageForInvalidTag(String paramName) {
paramName);
}
- final class InvalidTagChecker extends DocTreePathScanner implements AutoCloseable {
+ final class InvalidTagChecker extends DocTreePathScanner {
private final VisitorState state;
- private final ImmutableSet validTags;
private final ImmutableSet parameters;
-
- private final Pattern misplacedCurly;
- private final Pattern parensRatherThanCurly;
+ private final Context context;
private final Set fixedTags = new HashSet<>();
private InvalidTagChecker(
- VisitorState state, ImmutableSet validTags, ImmutableSet parameters) {
+ VisitorState state, Context context, ImmutableSet parameters) {
this.state = state;
- this.validTags = validTags;
+ this.context = context;
this.parameters = parameters;
- String validInlineTags =
- validTags.stream()
- .filter(tag -> tag.type() == TagType.INLINE)
- .map(JavadocTag::name)
- .collect(joining("|"));
- this.misplacedCurly = Pattern.compile(String.format("@(%s)\\{", validInlineTags));
- this.parensRatherThanCurly = Pattern.compile(String.format("\\(@(%s)", validInlineTags));
}
@Override
@@ -180,7 +186,7 @@ public Void visitText(TextTree node, Void unused) {
private void handleMalformedTags(TextTree node) {
String body = node.getBody();
- Matcher matcher = misplacedCurly.matcher(body);
+ Matcher matcher = context.misplacedCurly.matcher(body);
Comment comment = ((DCDocComment) getCurrentPath().getDocComment()).comment;
while (matcher.find()) {
int beforeAt = comment.getSourcePos(((DCText) node).pos + matcher.start());
@@ -198,7 +204,7 @@ private void handleMalformedTags(TextTree node) {
private void handleIncorrectParens(TextTree node) {
String body = node.getBody();
- Matcher matcher = parensRatherThanCurly.matcher(body);
+ Matcher matcher = context.parensRatherThanCurly.matcher(body);
Comment comment = ((DCDocComment) getCurrentPath().getDocComment()).comment;
while (matcher.find()) {
int beforeAt = comment.getSourcePos(((DCText) node).pos + matcher.start());
@@ -336,7 +342,7 @@ private void reportUnknownTag(DocTree docTree, JavadocTag tag) {
Utils.getBestMatch(
tag.name(),
/* maxEditDistance= */ 2,
- validTags.stream()
+ context.validTags.stream()
.filter(t -> t.type().equals(tag.type()))
.map(JavadocTag::name)
.collect(toImmutableSet()));
@@ -371,7 +377,7 @@ public Void scan(DocTree docTree, Void unused) {
return null;
}
JavadocTag tag = inlineTag(((DCInlineTag) docTree).getTagName());
- if (validTags.contains(tag) || JavadocTag.KNOWN_OTHER_TAGS.contains(tag)) {
+ if (context.validTags.contains(tag) || JavadocTag.KNOWN_OTHER_TAGS.contains(tag)) {
return null;
}
String message =
@@ -383,8 +389,5 @@ public Void scan(DocTree docTree, Void unused) {
.build());
return null;
}
-
- @Override
- public void close() {}
}
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/JavadocTag.java b/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/JavadocTag.java
index ae69ff70a4f..d0f91b89904 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/JavadocTag.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/JavadocTag.java
@@ -20,9 +20,11 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableSet;
+import com.google.errorprone.annotations.Immutable;
import java.util.stream.Stream;
/** Describes Javadoc tags, and contains lists of valid tags. */
+@Immutable
@AutoValue
abstract class JavadocTag {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ExtendsObject.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ExtendsObject.java
new file mode 100644
index 00000000000..62041841aca
--- /dev/null
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ExtendsObject.java
@@ -0,0 +1,60 @@
+/*
+ * Copyright 2022 The Error Prone Authors.
+ *
+ * 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.nullness;
+
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.util.ASTHelpers.getType;
+import static java.lang.String.format;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.BugPattern.SeverityLevel;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.TypeParameterTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.fixes.SuggestedFixes;
+import com.google.errorprone.matchers.Description;
+import com.sun.source.tree.AnnotatedTypeTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.TypeParameterTree;
+
+/** A bugpattern: see the summary. */
+@BugPattern(
+ summary =
+ "`T extends Object` is redundant in normal Java, and does not work to describe `T` as"
+ + " non-null across compilation boundaries when the Checker Framework unless you"
+ + " compile users against bytecode generated by the Checker Framework javac. (If you"
+ + " are building this code with the Checker Framework javac, then disable this check.)",
+ severity = SeverityLevel.WARNING)
+public final class ExtendsObject extends BugChecker implements TypeParameterTreeMatcher {
+ private static final String NON_NULL = "org.checkerframework.checker.nullness.qual.NonNull";
+
+ @Override
+ public Description matchTypeParameter(TypeParameterTree tree, VisitorState state) {
+ for (Tree bound : tree.getBounds()) {
+ if (!state.getTypes().isSameType(getType(bound), state.getSymtab().objectType)) {
+ continue;
+ }
+ if (!(bound instanceof AnnotatedTypeTree)) {
+ SuggestedFix.Builder fix = SuggestedFix.builder();
+ String nonNull = SuggestedFixes.qualifyType(state, fix, NON_NULL);
+ return describeMatch(bound, fix.prefixWith(bound, format(" @%s ", nonNull)).build());
+ }
+ }
+ return NO_MATCH;
+ }
+}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullArgumentForNonNullParameter.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullArgumentForNonNullParameter.java
new file mode 100644
index 00000000000..e1a42cfac25
--- /dev/null
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullArgumentForNonNullParameter.java
@@ -0,0 +1,216 @@
+/*
+ * Copyright 2022 The Error Prone Authors.
+ *
+ * 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.nullness;
+
+import static com.google.common.collect.Streams.forEachPair;
+import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+import static com.google.errorprone.VisitorState.memoize;
+import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.hasDefinitelyNullBranch;
+import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.hasExtraParameterForEnclosingInstance;
+import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.nullnessChecksShouldBeConservative;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.suppliers.Suppliers.typeFromString;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
+import static javax.lang.model.type.TypeKind.TYPEVAR;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.ErrorProneFlags;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher;
+import com.google.errorprone.dataflow.nullnesspropagation.Nullness;
+import com.google.errorprone.dataflow.nullnesspropagation.NullnessAnnotations;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.suppliers.Supplier;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.NewClassTree;
+import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import com.sun.tools.javac.code.Symbol.VarSymbol;
+import com.sun.tools.javac.code.Type;
+import com.sun.tools.javac.util.Name;
+import java.util.List;
+
+/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
+@BugPattern(summary = "Null is not permitted for this parameter.", severity = ERROR)
+public final class NullArgumentForNonNullParameter extends BugChecker
+ implements MethodInvocationTreeMatcher, NewClassTreeMatcher {
+ private static final Supplier JAVA_OPTIONAL_TYPE = typeFromString("java.util.Optional");
+ private static final Supplier GUAVA_OPTIONAL_TYPE =
+ typeFromString("com.google.common.base.Optional");
+ private static final Supplier OF_NAME = memoize(state -> state.getName("of"));
+ private static final Supplier COM_GOOGLE_COMMON_PREFIX_NAME =
+ memoize(state -> state.getName("com.google.common."));
+
+ private final boolean beingConservative;
+
+ public NullArgumentForNonNullParameter(ErrorProneFlags flags) {
+ this.beingConservative = nullnessChecksShouldBeConservative(flags);
+ }
+
+ @Override
+ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+ return match(getSymbol(tree), tree.getArguments(), state);
+ }
+
+ @Override
+ public Description matchNewClass(NewClassTree tree, VisitorState state) {
+ return match(getSymbol(tree), tree.getArguments(), state);
+ }
+
+ private Description match(
+ MethodSymbol methodSymbol, List args, VisitorState state) {
+ if (state.errorProneOptions().isTestOnlyTarget()) {
+ return NO_MATCH; // The tests of `foo` often invoke `foo(null)` to verify that it NPEs.
+ }
+
+ if (hasExtraParameterForEnclosingInstance(methodSymbol)) {
+ // TODO(b/232103314): Figure out the right way to handle the implicit outer `this` parameter.
+ return NO_MATCH;
+ }
+
+ if (methodSymbol.isVarArgs()) {
+ /*
+ * TODO(b/232103314): Figure out the right way to handle this, or at least handle all
+ * parameters but the last.
+ */
+ return NO_MATCH;
+ }
+
+ forEachPair(
+ args.stream(),
+ methodSymbol.getParameters().stream(),
+ (argTree, paramSymbol) -> {
+ if (!hasDefinitelyNullBranch(
+ argTree,
+ /*
+ * TODO(cpovirk): Precompute sets of definitelyNullVars and varsProvenNullByParentIf
+ * instead of passing empty sets.
+ */
+ ImmutableSet.of(),
+ ImmutableSet.of(),
+ state)) {
+ return;
+ }
+
+ if (!argumentMustBeNonNull(paramSymbol, state)) {
+ return;
+ }
+
+ state.reportMatch(describeMatch(argTree));
+ });
+
+ return NO_MATCH; // Any matches were reported through state.reportMatch.
+ }
+
+ private boolean argumentMustBeNonNull(VarSymbol sym, VisitorState state) {
+ if (sym.asType().isPrimitive()) {
+ return true;
+ }
+
+ /*
+ * Though we get most of our nullness information from annotations, there are technical
+ * obstacles to relying purely on them, including around type variables (see comments below)—not
+ * to mention that there are no annotations on JDK classes.
+ *
+ * As a workaround, we can hardcode specific APIs that feel worth the effort. For now, the only
+ * ones we hardcode are the two Optional.of methods. Those just happen to be the ones that I
+ * thought of and found hits for in our codebase.
+ */
+ if (sym.owner.name.equals(OF_NAME.get(state))
+ && (isParameterOfMethodOnType(sym, JAVA_OPTIONAL_TYPE, state)
+ || isParameterOfMethodOnType(sym, GUAVA_OPTIONAL_TYPE, state))) {
+ return true;
+ }
+
+ /*
+ * TODO(b/203207989): In theory, we should program this check to exclude inner classes until we
+ * fix the bug in MoreAnnotations.getDeclarationAndTypeAttributes, which is used by
+ * fromAnnotationsOn. In practice, annotations on both inner classes and outer classes are rare
+ * (especially when NullableOnContainingClass is enabled!), so this code currently still looks
+ * at parameters that are inner types, even though we might misinterpret them.
+ */
+ Nullness nullness = NullnessAnnotations.fromAnnotationsOn(sym).orElse(null);
+
+ if (nullness == Nullness.NONNULL && !beingConservative) {
+ /*
+ * Much code in the wild has @NonNull annotations on parameters that are apparently
+ * legitimately passed null arguments. Thus, we don't trust such annotations when running in
+ * conservative mode.
+ *
+ * TODO(cpovirk): Instead of ignoring @NonNull annotations entirely, add special cases for
+ * specific badly annotated APIs. Or better yet, get the annotations fixed.
+ */
+ return true;
+ }
+ if (nullness == Nullness.NULLABLE) {
+ return false;
+ }
+
+ if (sym.asType().getKind() == TYPEVAR) {
+ /*
+ * TODO(cpovirk): We could sometimes return true if we looked for @NullMarked and for any
+ * annotations on the type-parameter bounds. But looking at type-parameter bounds is hard
+ * because of JDK-8225377.
+ */
+ return false;
+ }
+
+ if (enclosingAnnotationDefaultsNonTypeVariablesToNonNull(sym, state)) {
+ return true;
+ }
+
+ return false;
+ }
+
+ private static boolean isParameterOfMethodOnType(
+ VarSymbol sym, Supplier typeSupplier, VisitorState state) {
+ Type target = typeSupplier.get(state);
+ return target != null && state.getTypes().isSameType(sym.enclClass().type, target);
+ }
+
+ private boolean enclosingAnnotationDefaultsNonTypeVariablesToNonNull(
+ Symbol sym, VisitorState state) {
+ for (; sym != null; sym = sym.getEnclosingElement()) {
+ if (hasAnnotation(sym, "com.google.protobuf.Internal$ProtoNonnullApi", state)) {
+ return true;
+ }
+ /*
+ * Similar to @NonNull (discussed above), the "default to non-null" annotation @NullMarked is
+ * sometimes used on code that hasn't had @Nullable annotations added to it where necessary.
+ * To avoid false positives, our conservative mode trusts @NullMarked only when it appears in
+ * com.google.common.
+ *
+ * TODO(cpovirk): Expand the list of packages that our conservative mode trusts @NullMarked
+ * on. We might be able to identify some packages that would be safe to trust today. For
+ * others, we could use ParameterMissingNullable, which adds missing annotations in situations
+ * similar to the ones identified by this check. (But note that ParameterMissingNullable
+ * doesn't help with calls that cross file boundaries.)
+ */
+ if (hasAnnotation(sym, "org.jspecify.nullness.NullMarked", state)
+ && (!beingConservative
+ || sym.packge().fullname.startsWith(COM_GOOGLE_COMMON_PREFIX_NAME.get(state)))) {
+ return true;
+ }
+ }
+ return false;
+ }
+}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullnessUtils.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullnessUtils.java
index 6dff9193333..c81767bcc4c 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullnessUtils.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullnessUtils.java
@@ -24,6 +24,7 @@
import static com.google.errorprone.fixes.SuggestedFix.emptyFix;
import static com.google.errorprone.matchers.Matchers.instanceMethod;
import static com.google.errorprone.suppliers.Suppliers.JAVA_LANG_VOID_TYPE;
+import static com.google.errorprone.util.ASTHelpers.enclosingClass;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
import static com.google.errorprone.util.ASTHelpers.getType;
import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
@@ -33,7 +34,9 @@
import static com.sun.source.tree.Tree.Kind.NULL_LITERAL;
import static com.sun.source.tree.Tree.Kind.PARAMETERIZED_TYPE;
import static com.sun.tools.javac.parser.Tokens.TokenKind.DOT;
+import static java.lang.Boolean.TRUE;
+import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableSet;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
@@ -49,6 +52,7 @@
import com.sun.source.tree.AssignmentTree;
import com.sun.source.tree.BinaryTree;
import com.sun.source.tree.BlockTree;
+import com.sun.source.tree.CaseTree;
import com.sun.source.tree.ConditionalExpressionTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree;
@@ -67,9 +71,13 @@
import com.sun.tools.javac.code.Kinds.KindSelector;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.ClassSymbol;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.processing.JavacProcessingEnvironment;
+import java.lang.reflect.Method;
+import java.util.List;
+import java.util.Objects;
import java.util.Set;
import javax.annotation.Nullable;
import javax.lang.model.element.Name;
@@ -231,7 +239,16 @@ static boolean isAlreadyAnnotatedNullable(Symbol symbol) {
return NullnessAnnotations.fromAnnotationsOn(symbol).orElse(null) == Nullness.NULLABLE;
}
- @com.google.auto.value.AutoValue // fully qualified to work around JDK-7177813(?) in JDK8 build
+ static boolean hasExtraParameterForEnclosingInstance(MethodSymbol symbol) {
+ // TODO(b/232103314): Figure out which cases the implicit outer `this` parameter exists in.
+ if (!symbol.isConstructor()) {
+ return false;
+ }
+ ClassSymbol constructedClass = enclosingClass(symbol);
+ return enclosingClass(constructedClass) != null && !constructedClass.isStatic();
+ }
+
+ @AutoValue
abstract static class NullableAnnotationToUse {
static NullableAnnotationToUse annotationToBeImported(String qualifiedName, boolean isTypeUse) {
return new AutoValue_NullnessUtils_NullableAnnotationToUse(
@@ -323,7 +340,7 @@ private static NullableAnnotationToUse pickNullableAnnotation(VisitorState state
.orElse(
state.isAndroidCompatible()
? "androidx.annotation.Nullable"
- : "javax.annotation.Nullable");
+ : "org.jspecify.nullness.Nullable");
if (sym != null) {
ClassSymbol classSym = (ClassSymbol) sym;
if (classSym.isAnnotationType()) {
@@ -331,7 +348,7 @@ private static NullableAnnotationToUse pickNullableAnnotation(VisitorState state
return annotationWithoutImporting(
"Nullable", isTypeUse(classSym.className()), /*isAlreadyInScope=*/ true);
} else {
- // It's not an annotation type. We have to fully-qualify the import.
+ // The imported `Nullable` is not an annotation type. Fully qualify the annotation.
return annotationWithoutImporting(
defaultType, isTypeUse(defaultType), /*isAlreadyInScope=*/ false);
}
@@ -493,7 +510,7 @@ public Boolean visitParenthesized(ParenthesizedTree tree, Void unused) {
return visit(tree.getExpression(), unused);
}
- // TODO(cpovirk): visitSwitchExpression
+ // For visitSwitchExpression logic, see defaultAction.
@Override
public Boolean visitTypeCast(TypeCastTree tree, Void unused) {
@@ -509,7 +526,13 @@ protected Boolean defaultAction(Tree tree, Void unused) {
* null)`.)
*/
return isVoid(getType(tree), stateForCompilationUnit)
- || definitelyNullVars.contains(getSymbol(tree));
+ || definitelyNullVars.contains(getSymbol(tree))
+ /*
+ * TODO(cpovirk): It would be nicer to report the finding on the null-returning `case`
+ * rather than on the `switch` as a whole. To do so, maybe we could change our visitor
+ * to accept `Boolean isCaseOfReturnedExpressionSwitch` instead of `Void unused`?
+ */
+ || isSwitchExpressionWithDefinitelyNullBranch(tree);
}
boolean isOptionalOrNull(MethodInvocationTree tree) {
@@ -521,9 +544,42 @@ boolean isOptionalOrNull(MethodInvocationTree tree) {
* But consider whether that would interfere with the TODO at the top of that method.
*/
}
+
+ boolean isSwitchExpressionWithDefinitelyNullBranch(Tree tree) {
+ return tree.getKind().name().equals("SWITCH_EXPRESSION")
+ && getCases(tree).stream()
+ .map(NullnessUtils::getBody)
+ .anyMatch(t -> Objects.equals(visit(t, null), TRUE));
+ }
}.visit(tree, null);
}
+ private static List getCases(Tree switchExpressionTree) {
+ try {
+ if (getCasesMethod == null) {
+ getCasesMethod =
+ Class.forName("com.sun.source.tree.SwitchExpressionTree").getMethod("getCases");
+ }
+ return (List) getCasesMethod.invoke(switchExpressionTree);
+ } catch (ReflectiveOperationException e) {
+ throw new LinkageError(e.getMessage(), e);
+ }
+ }
+
+ private static Tree getBody(Object caseTree) {
+ try {
+ if (getBodyMethod == null) {
+ getBodyMethod = CaseTree.class.getMethod("getBody");
+ }
+ return (Tree) getBodyMethod.invoke(caseTree);
+ } catch (ReflectiveOperationException e) {
+ throw new LinkageError(e.getMessage(), e);
+ }
+ }
+
+ private static Method getCasesMethod;
+ private static Method getBodyMethod;
+
/** Returns true if this is {@code x == null ? x : ...} or similar. */
private static boolean isTernaryXIfXIsNull(ConditionalExpressionTree tree) {
NullCheck nullCheck = getNullCheck(tree.getCondition());
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ParameterMissingNullable.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ParameterMissingNullable.java
index ced1b26104b..33c5fb95f97 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ParameterMissingNullable.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ParameterMissingNullable.java
@@ -22,10 +22,10 @@
import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.fixByAddingNullableAnnotationToType;
import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.getNullCheck;
import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.hasDefinitelyNullBranch;
+import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.hasExtraParameterForEnclosingInstance;
import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.isAlreadyAnnotatedNullable;
import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.nullnessChecksShouldBeConservative;
import static com.google.errorprone.matchers.Description.NO_MATCH;
-import static com.google.errorprone.util.ASTHelpers.enclosingClass;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
import static com.google.errorprone.util.ASTHelpers.getType;
import static com.google.errorprone.util.ASTHelpers.hasNoExplicitType;
@@ -55,7 +55,6 @@
import com.sun.source.util.TreePath;
import com.sun.source.util.TreeScanner;
import com.sun.tools.javac.code.Symbol;
-import com.sun.tools.javac.code.Symbol.ClassSymbol;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import java.util.List;
@@ -241,25 +240,17 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) {
return matchCall(getSymbol(tree), tree.getArguments(), state);
}
- private static boolean hasExtraParameterForEnclosingInstance(MethodSymbol symbol) {
- if (!symbol.isConstructor()) {
- return false;
- }
- ClassSymbol constructedClass = enclosingClass(symbol);
- return enclosingClass(constructedClass) != null && !constructedClass.isStatic();
- }
-
private Description matchCall(
MethodSymbol methodSymbol, List arguments, VisitorState state) {
if (hasExtraParameterForEnclosingInstance(methodSymbol)) {
- // TODO(cpovirk): Figure out the right way to handle the implicit outer `this` parameter.
+ // TODO(b/232103314): Figure out the right way to handle the implicit outer `this` parameter.
return NO_MATCH;
}
if (methodSymbol.isVarArgs()) {
/*
- * TODO(cpovirk): Figure out the right way to handle this, or at least handle all parameters
- * but the last.
+ * TODO(b/232103314): Figure out the right way to handle this, or at least handle all
+ * parameters but the last.
*/
return NO_MATCH;
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java
index 861fb26e260..feff9fa1bd4 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java
@@ -35,9 +35,11 @@
import static com.google.errorprone.util.ASTHelpers.constValue;
import static com.google.errorprone.util.ASTHelpers.findEnclosingMethod;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
import static com.google.errorprone.util.ASTHelpers.isConsideredFinal;
import static com.google.errorprone.util.ASTHelpers.methodCanBeOverridden;
import static com.sun.source.tree.Tree.Kind.NULL_LITERAL;
+import static com.sun.source.tree.Tree.Kind.THROW;
import static java.lang.Boolean.FALSE;
import static java.util.regex.Pattern.compile;
import static javax.lang.model.type.TypeKind.TYPEVAR;
@@ -74,6 +76,14 @@
/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@BugPattern(
summary = "Method returns a definitely null value but is not annotated @Nullable",
+ explanation =
+ "Annotating a method @Nullable communicates to tools that the method can return null. That"
+ + " means they can check that callers handle a returned null correctly.\n\n"
+ + "Adding @Nullable may require updating callers so that they deal with the"
+ + " possibly-null value. This can happen for example with Kotlin callers, or with Java"
+ + " callers that are checked for null-safety by static-analysis tools. Alternatively,"
+ + " depending on the tool, it may be possible to annotate Java callers temporarily with"
+ + " @SuppressWarnings(\"nullness\").",
severity = SUGGESTION)
public class ReturnMissingNullable extends BugChecker implements CompilationUnitTreeMatcher {
private static final Matcher METHODS_THAT_NEVER_RETURN =
@@ -272,6 +282,17 @@ void doVisitMethod(MethodTree tree) {
return;
}
+ if (tree.getBody() != null
+ && tree.getBody().getStatements().size() == 1
+ && getOnlyElement(tree.getBody().getStatements()).getKind() == THROW) {
+ return;
+ }
+
+ if (hasAnnotation(
+ tree, "com.google.errorprone.annotations.DoNotCall", stateForCompilationUnit)) {
+ return;
+ }
+
for (MethodSymbol methodKnownToReturnNull :
METHODS_KNOWN_TO_RETURN_NULL.get(stateForCompilationUnit)) {
if (stateForCompilationUnit
@@ -281,7 +302,13 @@ void doVisitMethod(MethodTree tree) {
fixByAddingNullableAnnotationToReturnType(
stateForCompilationUnit.withPath(getCurrentPath()), tree);
if (!fix.isEmpty()) {
- stateForCompilationUnit.reportMatch(describeMatch(tree, fix));
+ stateForCompilationUnit.reportMatch(
+ buildDescription(tree)
+ .setMessage(
+ "Nearly all implementations of this method must return null, but it is"
+ + " not annotated @Nullable")
+ .addFix(fix)
+ .build());
}
}
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/UnsafeWildcard.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/UnsafeWildcard.java
new file mode 100644
index 00000000000..08f9e93a65c
--- /dev/null
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/UnsafeWildcard.java
@@ -0,0 +1,364 @@
+/*
+ * Copyright 2019 The Error Prone Authors.
+ *
+ * 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.nullness;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+import static java.lang.Math.min;
+
+import com.google.common.collect.ImmutableListMultimap;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.AssignmentTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.ConditionalExpressionTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.LambdaExpressionTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.ParenthesizedTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.TypeCastTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.AssignmentTree;
+import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.ConditionalExpressionTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.LambdaExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.NewClassTree;
+import com.sun.source.tree.ParenthesizedTree;
+import com.sun.source.tree.ReturnTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.TypeCastTree;
+import com.sun.source.tree.VariableTree;
+import com.sun.tools.javac.code.Flags;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import com.sun.tools.javac.code.Symbol.TypeVariableSymbol;
+import com.sun.tools.javac.code.Symbol.VarSymbol;
+import com.sun.tools.javac.code.Type;
+import com.sun.tools.javac.code.Type.ArrayType;
+import com.sun.tools.javac.code.Type.IntersectionClassType;
+import com.sun.tools.javac.code.Type.MethodType;
+import com.sun.tools.javac.code.Type.UnionClassType;
+import com.sun.tools.javac.code.Type.WildcardType;
+import com.sun.tools.javac.tree.JCTree.JCClassDecl;
+import com.sun.tools.javac.tree.JCTree.JCExpression;
+import com.sun.tools.javac.tree.JCTree.JCLambda;
+import com.sun.tools.javac.tree.JCTree.JCMethodDecl;
+import com.sun.tools.javac.tree.JCTree.JCMethodInvocation;
+import com.sun.tools.javac.tree.JCTree.JCNewClass;
+import com.sun.tools.javac.tree.JCTree.JCParens;
+import com.sun.tools.javac.tree.JCTree.JCTypeCast;
+import com.sun.tools.javac.tree.JCTree.JCVariableDecl;
+import java.util.HashSet;
+import java.util.Map;
+import javax.lang.model.element.ElementKind;
+import javax.lang.model.type.TypeKind;
+
+/** Check to detect unsafe upcasts of {@code null} values to wildcard types. */
+@BugPattern(summary = "Certain wildcard types can confuse the compiler.", severity = ERROR)
+public class UnsafeWildcard extends BugChecker
+ implements AssignmentTreeMatcher,
+ ClassTreeMatcher,
+ ConditionalExpressionTreeMatcher,
+ LambdaExpressionTreeMatcher,
+ MethodInvocationTreeMatcher,
+ NewClassTreeMatcher,
+ ParenthesizedTreeMatcher,
+ ReturnTreeMatcher,
+ TypeCastTreeMatcher,
+ VariableTreeMatcher {
+
+ @Override
+ public Description matchAssignment(AssignmentTree tree, VisitorState state) {
+ return checkForUnsafeNullAssignment(
+ ((JCExpression) tree.getVariable()).type, tree.getExpression(), state);
+ }
+
+ @Override
+ public Description matchClass(ClassTree tree, VisitorState state) {
+ JCClassDecl classDecl = (JCClassDecl) tree;
+ // Check "extends" and "implements" for unsafe wildcards
+ for (JCExpression implemented : classDecl.getImplementsClause()) {
+ state.reportMatch(
+ checkForUnsafeWildcards(implemented, "Unsafe wildcard type: ", implemented.type, state));
+ }
+ if (classDecl.getExtendsClause() != null) {
+ return checkForUnsafeWildcards(
+ classDecl.getExtendsClause(),
+ "Unsafe wildcard type: ",
+ classDecl.getExtendsClause().type,
+ state);
+ }
+ return Description.NO_MATCH;
+ }
+
+ @Override
+ public Description matchConditionalExpression(
+ ConditionalExpressionTree tree, VisitorState state) {
+ // Ternary branches are implicitly upcast, so check in case they're null
+ Type ternaryType = ((JCExpression) tree).type;
+ state.reportMatch(checkForUnsafeNullAssignment(ternaryType, tree.getTrueExpression(), state));
+ return checkForUnsafeNullAssignment(ternaryType, tree.getFalseExpression(), state);
+ }
+
+ @Override
+ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) {
+ if (tree.getBody() instanceof ExpressionTree) {
+ Type targetType = ((JCLambda) tree).getDescriptorType(state.getTypes()).getReturnType();
+ return checkForUnsafeNullAssignment(targetType, (ExpressionTree) tree.getBody(), state);
+ } // else covered by matchReturn
+ return Description.NO_MATCH;
+ }
+
+ @Override
+ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+ // MethodType of this invocation gives us the args' target types with any type parameters
+ // substituted (unlike the method's symbol, which doesn't give us effective target types).
+ MethodType mtype = (MethodType) ((JCMethodInvocation) tree).meth.type;
+ MethodSymbol callee = ASTHelpers.getSymbol(tree);
+ if (!((JCMethodInvocation) tree).getTypeArguments().isEmpty()) {
+ // Check type arguments for problematic wildcards given in source.
+ for (JCExpression typearg : ((JCMethodInvocation) tree).getTypeArguments()) {
+ state.reportMatch(
+ checkForUnsafeWildcards(typearg, "Unsafe wildcard type: ", typearg.type, state));
+ }
+ } else if (!callee.type.getTypeArguments().isEmpty()) {
+ // Otherwise, check any inferred type arguments
+ ImmutableListMultimap mapping =
+ ASTHelpers.getTypeSubstitution(mtype, callee);
+ HashSet seen = new HashSet<>();
+ for (Map.Entry inferredTypearg : mapping.entries()) {
+ if (!seen.add(inferredTypearg.getValue())) {
+ continue; // avoid duplicate reports for the same type
+ }
+ state.reportMatch(
+ checkForUnsafeWildcards(
+ tree,
+ "Unsafe wildcard in inferred type argument for callee's type parameter "
+ + inferredTypearg.getKey()
+ + ": ",
+ inferredTypearg.getValue(),
+ state));
+ }
+ }
+
+ int paramIndex = 0;
+ for (ExpressionTree arg : tree.getArguments()) {
+ // Check null arguments against parameter type
+ // NB: this will be an array type for vararg parameters, but checkForUnsafeNullAssignment
+ // sees through array types, so there's no need to unwrap the type here
+ Type paramType = mtype.argtypes.get(paramIndex);
+ state.reportMatch(checkForUnsafeNullAssignment(paramType, arg, state));
+ paramIndex = min(paramIndex + 1, mtype.argtypes.size() - 1);
+ }
+ return Description.NO_MATCH;
+ }
+
+ @Override
+ public Description matchNewClass(NewClassTree tree, VisitorState state) {
+ // MethodType of this invocation gives us the args' target types with any type parameters
+ // substituted (unlike the method's symbol, which doesn't give us effective target types).
+ MethodType mtype = (MethodType) ((JCNewClass) tree).constructorType;
+ // mtype contains type of outer object in some cases, which we can skip since null enclosing
+ // expression would cause exception.
+ int paramIndex = tree.getClassBody() != null && tree.getEnclosingExpression() != null ? 1 : 0;
+ for (ExpressionTree arg : tree.getArguments()) {
+ // Check null arguments against parameter type
+ // NB: this will be an array type for vararg parameters, but checkForUnsafeNullAssignment
+ // sees through array types, so there's no need to unwrap the type here
+ Type paramType = mtype.argtypes.get(paramIndex);
+ state.reportMatch(checkForUnsafeNullAssignment(paramType, arg, state));
+ paramIndex = min(paramIndex + 1, mtype.argtypes.size() - 1);
+ }
+ // Check type arguments for problematic wildcards (visiting class type will recursively visit
+ // its arguments
+ return checkForUnsafeWildcards(
+ tree, "Unsafe wildcard type argument: ", ((JCNewClass) tree).type, state);
+ }
+
+ @Override
+ public Description matchParenthesized(ParenthesizedTree tree, VisitorState state) {
+ // Treat (null) like null
+ return checkForUnsafeNullAssignment(((JCParens) tree).type, tree.getExpression(), state);
+ }
+
+ @Override
+ public Description matchReturn(ReturnTree tree, VisitorState state) {
+ // Check "return null" against return type
+ if (tree.getExpression() == null
+ || ((JCExpression) tree.getExpression()).type.getKind() != TypeKind.NULL) {
+ return Description.NO_MATCH;
+ }
+
+ // Figure out return type of surrounding method or lambda and check it
+ Tree method = state.findEnclosing(MethodTree.class, LambdaExpressionTree.class);
+ if (method instanceof MethodTree) {
+ return checkForUnsafeNullAssignment(
+ ((JCMethodDecl) method).getReturnType().type, tree.getExpression(), state);
+ } else if (method instanceof LambdaExpressionTree) {
+ Type targetType = ((JCLambda) method).getDescriptorType(state.getTypes()).getReturnType();
+ return checkForUnsafeNullAssignment(targetType, tree.getExpression(), state);
+ }
+ return Description.NO_MATCH;
+ }
+
+ @Override
+ public Description matchTypeCast(TypeCastTree tree, VisitorState state) {
+ // Check explicit casts of null
+ return checkForUnsafeNullAssignment(
+ ((JCTypeCast) tree).getType().type, tree.getExpression(), state);
+ }
+
+ @Override
+ public Description matchVariable(VariableTree tree, VisitorState state) {
+ // Check initializer like assignment
+ if (tree.getInitializer() != null) {
+ return checkForUnsafeNullAssignment(
+ ((JCVariableDecl) tree).type, tree.getInitializer(), state);
+ }
+
+ VarSymbol symbol = ASTHelpers.getSymbol(tree);
+ if (symbol.getKind() == ElementKind.FIELD && (symbol.flags() & Flags.FINAL) == 0) {
+ // Fields start out as null, so check their type as if this was a null assignment. While all
+ // fields start out null, we ignore them here if they're final or we checked their
+ // initializer above. In either case we know we'll see at least one assignment to the field,
+ // and if those check out they guarantees us that the field's type is ok: non-null
+ // assignments guarantee that there is a concrete type compatible with any
+ // wildcards in this field's type, and null assignments will be checked separately (possibly
+ // right above).
+ return checkForUnsafeWildcards(
+ tree,
+ "Uninitialized field with unsafe wildcard type: ",
+ ((JCVariableDecl) tree).type,
+ state);
+ }
+ return Description.NO_MATCH;
+ }
+
+ /**
+ * Checks for unsafe wildcards in {@code targetType} if the given expression is `null`.
+ *
+ * @return diagnostic for {@code tree} if unsafe wildcard is found, {@link Description#NO_MATCH}
+ * otherwise.
+ * @see #checkForUnsafeWildcards
+ */
+ private Description checkForUnsafeNullAssignment(
+ Type targetType, ExpressionTree tree, VisitorState state) {
+ if (!targetType.isReference() || ((JCExpression) tree).type.getKind() != TypeKind.NULL) {
+ return Description.NO_MATCH;
+ }
+ return checkForUnsafeWildcards(tree, "Cast to wildcard type unsafe: ", targetType, state);
+ }
+
+ /**
+ * Recursively looks through {@code targetType} for any wildcard whose lower bounds isn't known to
+ * be a subtype of the corresponding type parameter's upper bound.
+ *
+ * @return diagnostic for {@code tree} with given {@code messageHeader} if unsafe wildcard found,
+ * {@link Description#NO_MATCH} otherwise.
+ */
+ private Description checkForUnsafeWildcards(
+ Tree tree, String messageHeader, Type targetType, VisitorState state) {
+ while (targetType instanceof ArrayType) {
+ // Check array component type
+ targetType = ((ArrayType) targetType).getComponentType();
+ }
+ int i = 0;
+ for (Type arg : targetType.getTypeArguments()) {
+ // Check components of generic types (getTypeArguments() is empty for other kinds of types)
+ if (arg instanceof WildcardType && ((WildcardType) arg).getSuperBound() != null) {
+ Type lowerBound = ((WildcardType) arg).getSuperBound();
+ // We only check lower bounds that are themselves type variables with trivial upper bounds.
+ // Javac already checks other lower bounds, namely lower bounds that are concrete types or
+ // type variables with non-trivial upper bounds, to be in bounds of the corresponding type
+ // parameter (boundVar below).
+ // We skip these cases because the subtype check below can spuriously fail for them because
+ // it doesn't correctly substitute type variables when comparing lowerBound and boundVar's
+ // upper bound.
+ // Note javax.lang.model.type.TypeVariable#getUpperBound() guarantees the result to be non-
+ // null for type variables, so we use null check as a proxy for whether lowerBound is a type
+ // variable.
+ // TODO(kmb): avoid counting on compiler's handling of non-trivial upper bounds here
+ if (lowerBound.getUpperBound() != null
+ && lowerBound.getUpperBound().toString().endsWith("java.lang.Object")) {
+ Type boundVar = targetType.tsym.type.getTypeArguments().get(i);
+
+ if (!state.getTypes().isSubtypeNoCapture(lowerBound, boundVar.getUpperBound())) {
+ return buildDescription(tree)
+ .setMessage(
+ messageHeader
+ + targetType
+ + " because of type argument "
+ + i
+ + " with implicit upper bound "
+ + boundVar.getUpperBound())
+ .build();
+ }
+ }
+ // Also check the super bound itself
+ Description contained =
+ checkForUnsafeWildcards(tree, messageHeader + i + " nested: ", lowerBound, state);
+ if (contained != Description.NO_MATCH) {
+ return contained;
+ }
+ } else if (arg instanceof WildcardType && ((WildcardType) arg).getExtendsBound() != null) {
+ // Check the wildcard's bound
+ Description contained =
+ checkForUnsafeWildcards(
+ tree,
+ messageHeader + i + " nested: ",
+ ((WildcardType) arg).getExtendsBound(),
+ state);
+ if (contained != Description.NO_MATCH) {
+ return contained;
+ }
+ } else {
+ // Check for wildcards in the type argument
+ Description contained =
+ checkForUnsafeWildcards(tree, messageHeader + i + " nested: ", arg, state);
+ if (contained != Description.NO_MATCH) {
+ return contained;
+ }
+ }
+ ++i;
+ }
+ // For union and intersection types, check their components.
+ if (targetType instanceof IntersectionClassType) {
+ for (Type bound : ((IntersectionClassType) targetType).getExplicitComponents()) {
+ Description contained =
+ checkForUnsafeWildcards(tree, messageHeader + "bound ", bound, state);
+ if (contained != Description.NO_MATCH) {
+ return contained;
+ }
+ }
+ }
+ if (targetType instanceof UnionClassType) {
+ for (Type alternative : ((UnionClassType) targetType).getAlternativeTypes()) {
+ Description contained =
+ checkForUnsafeWildcards(tree, messageHeader + "alternative ", alternative, state);
+ if (contained != Description.NO_MATCH) {
+ return contained;
+ }
+ }
+ }
+ return Description.NO_MATCH;
+ }
+}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullable.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullable.java
index 629746f5de2..132a7f15514 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullable.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullable.java
@@ -110,9 +110,6 @@ private static boolean isInNullMarkedScope(VisitorState state) {
for (Tree tree : state.getPath()) {
if (tree.getKind().asInterface().equals(ClassTree.class) || tree.getKind() == METHOD) {
Symbol enclosingElement = getSymbol(tree);
- if (tree == null) {
- continue;
- }
return NullnessUtils.isInNullMarkedScope(enclosingElement, state);
}
}
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 b360b0979b3..18fc8419f68 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
@@ -251,6 +251,7 @@ static DCLInfo findDCL(IfTree outerIf) {
/**
* Matches comparisons to null (e.g. {@code foo == null}) and returns the expression being tested.
*/
+ @Nullable
private static ExpressionTree getNullCheckedExpression(ExpressionTree condition) {
condition = stripParentheses(condition);
if (!(condition instanceof BinaryTree)) {
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 c9b60f4d745..3fab23a8264 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
@@ -35,6 +35,7 @@
import com.sun.tools.javac.util.Names;
import java.util.Optional;
import javax.lang.model.element.Name;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* A binder from {@code @GuardedBy} annotations to {@link GuardedByExpression}s.
@@ -320,7 +321,8 @@ private GuardedByExpression normalizeBase(
* Returns the owner if the given member is declared in a lexically enclosing scope, and
* {@code null} otherwise.
*/
- private ClassSymbol isEnclosedIn(ClassSymbol startingClass, Symbol member, Types types) {
+ private @Nullable ClassSymbol isEnclosedIn(
+ ClassSymbol startingClass, Symbol member, Types types) {
for (ClassSymbol scope = startingClass.owner.enclClass();
scope != null;
scope = scope.owner.enclClass()) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByChecker.java
index 55900c9057d..aba19a60735 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByChecker.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByChecker.java
@@ -21,7 +21,6 @@
import com.google.common.base.Joiner;
import com.google.errorprone.BugPattern;
-import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.LambdaExpressionTreeMatcher;
@@ -41,6 +40,7 @@
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Type;
+import org.checkerframework.checker.nullness.qual.Nullable;
/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@BugPattern(
@@ -55,16 +55,6 @@ public class GuardedByChecker extends BugChecker
private final GuardedByFlags flags = GuardedByFlags.allOn();
- private final boolean reportMissingGuards;
- private final boolean checkTryWithResources;
-
- public GuardedByChecker(ErrorProneFlags errorProneFlags) {
- reportMissingGuards =
- errorProneFlags.getBoolean("GuardedByChecker:reportMissingGuards").orElse(true);
- checkTryWithResources =
- errorProneFlags.getBoolean("GuardedByChecker:checkTryWithResources").orElse(true);
- }
-
@Override
public Description matchMethod(MethodTree tree, VisitorState state) {
// Constructors (and field initializers, instance initializers, and class initializers) are free
@@ -89,9 +79,7 @@ private void analyze(VisitorState state) {
(ExpressionTree tree, GuardedByExpression guard, HeldLockSet live) ->
report(GuardedByChecker.this.checkGuardedAccess(tree, guard, live, state), state),
tree1 -> isSuppressed(tree1, state),
- flags,
- reportMissingGuards,
- checkTryWithResources);
+ flags);
}
@Override
@@ -175,7 +163,7 @@ private static String buildMessage(GuardedByExpression guard, HeldLockSet locks)
return message.toString();
}
- private static Select findOuterInstance(GuardedByExpression expr) {
+ private static @Nullable Select findOuterInstance(GuardedByExpression expr) {
while (expr.kind() == Kind.SELECT) {
Select select = (Select) expr;
if (select.sym().name.contentEquals(GuardedByExpression.ENCLOSING_INSTANCE_NAME)) {
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedBySymbolResolver.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedBySymbolResolver.java
index 820920f1064..a4d83510f7e 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedBySymbolResolver.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedBySymbolResolver.java
@@ -225,6 +225,7 @@ private VarSymbol getParam(@Nullable MethodInfo method, String name) {
return null;
}
+ @Nullable
@Override
public Symbol resolveTypeLiteral(ExpressionTree expr) {
checkGuardedBy(expr instanceof IdentifierTree, "bad type literal: %s", expr);
@@ -266,6 +267,7 @@ private Symbol resolveType(String name, SearchSuperTypes searchSuperTypes) {
return type;
}
+ @Nullable
private Symbol getSuperType(Symbol symbol, String name) {
for (Type t : types.closure(symbol.type)) {
if (t.asElement().getSimpleName().contentEquals(name)) {
@@ -275,6 +277,7 @@ private Symbol getSuperType(Symbol symbol, String name) {
return null;
}
+ @Nullable
private static Symbol getLexicallyEnclosing(ClassSymbol symbol, String name) {
Symbol current = symbol.owner;
while (true) {
@@ -296,6 +299,7 @@ private Symbol attribIdent(String name) {
return attr.attribIdent(tm.Ident(visitorState.getName(name)), compilationUnit);
}
+ @Nullable
@Override
public Symbol resolveEnclosingClass(ExpressionTree expr) {
checkGuardedBy(expr instanceof IdentifierTree, "bad type literal: %s", expr);
@@ -350,6 +354,7 @@ static MethodInfo create(MethodSymbol sym, ImmutableList argumen
return new AutoValue_GuardedBySymbolResolver_MethodInfo(sym, arguments);
}
+ @Nullable
static MethodInfo create(Tree tree, VisitorState visitorState) {
Symbol sym = ASTHelpers.getSymbol(tree);
if (!(sym instanceof MethodSymbol)) {
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 b50ecb72ee6..6040e296098 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
@@ -34,6 +34,7 @@
import java.util.List;
import java.util.Optional;
import java.util.stream.Stream;
+import org.checkerframework.checker.nullness.qual.Nullable;
/**
* @author cushon@google.com (Liam Miller-Cushon)
@@ -137,7 +138,7 @@ public static GuardedByValidationResult isGuardedByValid(
return GuardedByValidationResult.ok();
}
- public static Symbol bindGuardedByString(
+ public static @Nullable Symbol bindGuardedByString(
Tree tree, String guard, VisitorState visitorState, GuardedByFlags flags) {
Optional bound =
GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, visitorState), flags);
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 1daff1c5725..28dd4bdcd16 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
@@ -85,14 +85,10 @@ public static void analyze(
VisitorState state,
LockEventListener listener,
Predicate isSuppressed,
- GuardedByFlags flags,
- boolean reportMissingGuards,
- boolean checkTryWithResources) {
+ GuardedByFlags flags) {
HeldLockSet locks = HeldLockSet.empty();
locks = handleMonitorGuards(state, locks, flags);
- new LockScanner(
- state, listener, isSuppressed, flags, reportMissingGuards, checkTryWithResources)
- .scan(state.getPath(), locks);
+ new LockScanner(state, listener, isSuppressed, flags).scan(state.getPath(), locks);
}
// Don't use Class#getName() for inner classes, we don't want `Monitor$Guard`
@@ -127,8 +123,6 @@ private static class LockScanner extends TreePathScanner {
private final LockEventListener listener;
private final Predicate isSuppressed;
private final GuardedByFlags flags;
- private final boolean reportMissingGuards;
- private final boolean checkTryWithResources;
private static final GuardedByExpression.Factory F = new GuardedByExpression.Factory();
@@ -136,15 +130,11 @@ private LockScanner(
VisitorState visitorState,
LockEventListener listener,
Predicate isSuppressed,
- GuardedByFlags flags,
- boolean reportMissingGuards,
- boolean checkTryWithResources) {
+ GuardedByFlags flags) {
this.visitorState = visitorState;
this.listener = listener;
this.isSuppressed = isSuppressed;
this.flags = flags;
- this.reportMissingGuards = reportMissingGuards;
- this.checkTryWithResources = checkTryWithResources;
}
@Override
@@ -187,12 +177,9 @@ public Void visitTry(TryTree tree, HeldLockSet locks) {
// are held for the entirety of the try and catch statements.
Collection releasedLocks =
ReleasedLockFinder.find(tree.getFinallyBlock(), visitorState, flags);
- // We don't know what to do with the try-with-resources block.
// TODO(cushon) - recognize common try-with-resources patterns. Currently there is no
// standard implementation of an AutoCloseable lock resource to detect.
- if (checkTryWithResources || resources.isEmpty()) {
- scan(tree.getBlock(), locks.plusAll(releasedLocks));
- }
+ scan(tree.getBlock(), locks.plusAll(releasedLocks));
scan(tree.getCatches(), locks.plusAll(releasedLocks));
scan(tree.getFinallyBlock(), locks);
return null;
@@ -254,9 +241,7 @@ private void checkMatch(ExpressionTree tree, HeldLockSet locks) {
GuardedBySymbolResolver.from(tree, visitorState.withPath(getCurrentPath())),
flags);
if (!guard.isPresent()) {
- if (reportMissingGuards) {
- invalidLock(tree, locks, guardString);
- }
+ invalidLock(tree, locks, guardString);
continue;
}
Optional boundGuard =
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableAnalysis.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableAnalysis.java
index 62c6107a9ce..496e2663b46 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableAnalysis.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableAnalysis.java
@@ -47,6 +47,7 @@
import javax.lang.model.element.ElementKind;
import javax.lang.model.element.Modifier;
import javax.lang.model.type.TypeKind;
+import org.checkerframework.checker.nullness.qual.Nullable;
/** Analyzes types for deep immutability. */
public class ImmutableAnalysis {
@@ -323,7 +324,7 @@ AnnotationInfo getImmutableAnnotation(Symbol sym, VisitorState state) {
* Gets the {@link Tree}'s {@code @Immutable} annotation info, either from an annotation on the
* symbol or from the list of well-known immutable types.
*/
- AnnotationInfo getImmutableAnnotation(Tree tree, VisitorState state) {
+ @Nullable AnnotationInfo getImmutableAnnotation(Tree tree, VisitorState state) {
Symbol sym = ASTHelpers.getSymbol(tree);
return sym == null ? null : threadSafety.getMarkerOrAcceptedAnnotation(sym, state);
}
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java
index 1337cb52a9a..df41d97c401 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java
@@ -50,8 +50,6 @@
import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher;
import com.google.errorprone.bugpatterns.threadsafety.ThreadSafety.Violation;
-import com.google.errorprone.fixes.Fix;
-import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ClassTree;
@@ -84,6 +82,7 @@
import java.util.Optional;
import java.util.Set;
import javax.lang.model.element.ElementKind;
+import org.checkerframework.checker.nullness.qual.Nullable;
/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@BugPattern(
@@ -101,7 +100,6 @@ public class ImmutableChecker extends BugChecker
private final WellKnownMutability wellKnownMutability;
private final ImmutableSet immutableAnnotations;
- private final boolean handleAnonymousClasses;
ImmutableChecker(ImmutableSet immutableAnnotations) {
this(ErrorProneFlags.empty(), immutableAnnotations);
@@ -114,8 +112,6 @@ public ImmutableChecker(ErrorProneFlags flags) {
private ImmutableChecker(ErrorProneFlags flags, ImmutableSet immutableAnnotations) {
this.wellKnownMutability = WellKnownMutability.fromFlags(flags);
this.immutableAnnotations = immutableAnnotations;
- this.handleAnonymousClasses =
- flags.getBoolean("ImmutableChecker:HandleAnonymousClasses").orElse(true);
}
@Override
@@ -259,15 +255,16 @@ public Description matchClass(ClassTree tree, VisitorState state) {
if (tree.getSimpleName().length() == 0) {
// anonymous classes have empty names
- // TODO(cushon): once Java 8 happens, require @Immutable on anonymous classes
return handleAnonymousClass(tree, state, analysis);
}
- AnnotationInfo annotation = analysis.getImmutableAnnotation(tree, state);
+ AnnotationInfo annotation = getImmutableAnnotation(analysis, tree, state);
if (annotation == null) {
- // If the type isn't annotated we don't check for immutability, but we do
- // report an error if it extends/implements any @Immutable-annotated types.
- return checkSubtype(tree, state);
+ // If the type isn't annotated, and doesn't extend anything annotated, there's nothing to do.
+ // An earlier version of the check required an explicit annotation on classes that extended
+ // @Immutable classes, but didn't enforce the subtyping requirement for interfaces. We now
+ // don't require the explicit annotations on any subtypes.
+ return NO_MATCH;
}
// Special-case visiting declarations of known-immutable types; these uses
@@ -323,7 +320,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
describeClass(matched, sym, annotation, violation));
Type superType = immutableSupertype(sym, state);
- if (handleAnonymousClasses && superType != null && isLocal(sym)) {
+ if (superType != null && isLocal(sym)) {
checkClosedTypes(tree, state, superType.tsym, analysis);
}
@@ -381,9 +378,7 @@ private Description handleAnonymousClass(
return NO_MATCH;
}
- if (handleAnonymousClasses) {
- checkClosedTypes(tree, state, superType.tsym, analysis);
- }
+ checkClosedTypes(tree, state, superType.tsym, analysis);
// We don't need to check that the superclass has an immutable instantiation.
// The anonymous instance can only be referred to using a superclass type, so
// the type arguments will be validated at any type use site where we care about
@@ -546,28 +541,30 @@ private Description.Builder describeAnonymous(Tree tree, Type superType, Violati
// Strong behavioural subtyping
- /** Check for classes without {@code @Immutable} that have immutable supertypes. */
- private Description checkSubtype(ClassTree tree, VisitorState state) {
+ /**
+ * Check for classes with {@code @Immutable}, or that inherited it from a super class or
+ * interface.
+ */
+ private @Nullable AnnotationInfo getImmutableAnnotation(
+ ImmutableAnalysis analysis, ClassTree tree, VisitorState state) {
+ AnnotationInfo annotation = analysis.getImmutableAnnotation(tree, state);
+ if (annotation != null) {
+ return annotation;
+ }
+ // getImmutableAnnotation inherits annotations from classes, but not interfaces.
ClassSymbol sym = getSymbol(tree);
Type superType = immutableSupertype(sym, state);
- if (superType == null) {
- return NO_MATCH;
+ if (superType != null) {
+ return analysis.getImmutableAnnotation(superType.tsym, state);
}
- String message =
- format("Class extends @Immutable type %s, but is not annotated as immutable", superType);
- Fix fix =
- SuggestedFix.builder()
- .prefixWith(tree, "@Immutable ")
- .addImport(Immutable.class.getName())
- .build();
- return buildDescription(tree).setMessage(message).addFix(fix).build();
+ return null;
}
/**
* Returns the type of the first superclass or superinterface in the hierarchy annotated with
* {@code @Immutable}, or {@code null} if no such super type exists.
*/
- private Type immutableSupertype(Symbol sym, VisitorState state) {
+ private @Nullable Type immutableSupertype(Symbol sym, VisitorState state) {
for (Type superType : state.getTypes().closure(sym.type)) {
if (superType.tsym.equals(sym.type.tsym)) {
continue;
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafety.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafety.java
index 7231fe529ea..4519338204a 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafety.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafety.java
@@ -29,6 +29,7 @@
import com.google.common.collect.Sets;
import com.google.common.collect.Streams;
import com.google.errorprone.VisitorState;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.bugpatterns.CanBeStaticAnalyzer;
import com.google.errorprone.suppliers.Supplier;
import com.google.errorprone.util.ASTHelpers;
@@ -145,12 +146,14 @@ private Builder() {}
@Nullable private Class typeParameterAnnotation;
/** See {@link Purpose}. */
+ @CanIgnoreReturnValue
public Builder setPurpose(Purpose purpose) {
this.purpose = purpose;
return this;
}
/** Information about known types and whether they're known to be safe or unsafe. */
+ @CanIgnoreReturnValue
public Builder knownTypes(KnownTypes knownTypes) {
this.knownTypes = knownTypes;
return this;
@@ -160,11 +163,13 @@ public Builder knownTypes(KnownTypes knownTypes) {
* Annotations that will cause a class to be tested with this {@link ThreadSafety} instance; for
* example, when testing a class for immutability, this should be @Immutable.
*/
+ @CanIgnoreReturnValue
public Builder markerAnnotations(Set markerAnnotations) {
return markerAnnotations(ImmutableSet.copyOf(markerAnnotations));
}
// TODO(ringwalt): Remove this constructor. We need it for binary compatibility.
+ @CanIgnoreReturnValue
public Builder markerAnnotations(ImmutableSet markerAnnotations) {
checkNotNull(markerAnnotations);
this.markerAnnotations = markerAnnotations;
@@ -177,11 +182,13 @@ public Builder markerAnnotations(ImmutableSet markerAnnotations) {
* annotation, @Immutable would be included in this list, as an immutable class is by definition
* thread-safe.
*/
+ @CanIgnoreReturnValue
public Builder acceptedAnnotations(Set acceptedAnnotations) {
return acceptedAnnotations(ImmutableSet.copyOf(acceptedAnnotations));
}
// TODO(ringwalt): Remove this constructor. We need it for binary compatibility.
+ @CanIgnoreReturnValue
public Builder acceptedAnnotations(ImmutableSet acceptedAnnotations) {
checkNotNull(acceptedAnnotations);
this.acceptedAnnotations = acceptedAnnotations;
@@ -189,6 +196,7 @@ public Builder acceptedAnnotations(ImmutableSet acceptedAnnotations) {
}
/** An annotation which marks a generic parameter as a container type. */
+ @CanIgnoreReturnValue
public Builder containerOfAnnotation(Class containerOfAnnotation) {
checkNotNull(containerOfAnnotation);
this.containerOfAnnotation = containerOfAnnotation;
@@ -196,6 +204,7 @@ public Builder containerOfAnnotation(Class containerOfAnno
}
/** An annotation which, when found on a class, should suppress the test */
+ @CanIgnoreReturnValue
public Builder suppressAnnotation(Class suppressAnnotation) {
checkNotNull(suppressAnnotation);
this.suppressAnnotation = suppressAnnotation;
@@ -206,6 +215,7 @@ public Builder suppressAnnotation(Class suppressAnnotation
* An annotation which, when found on a type parameter, indicates that the type parameter may
* only be instantiated with thread-safe types.
*/
+ @CanIgnoreReturnValue
public Builder typeParameterAnnotation(Class typeParameterAnnotation) {
checkNotNull(typeParameterAnnotation);
checkArgument(
@@ -232,47 +242,6 @@ public ThreadSafety build(VisitorState state) {
}
}
- /** Use {@link #builder()} instead. */
- // TODO(ghm): Delete after a JB release.
- @Deprecated
- public ThreadSafety(
- VisitorState state,
- KnownTypes knownTypes,
- Set markerAnnotations,
- Set acceptedAnnotations,
- @Nullable Class containerOfAnnotation,
- @Nullable Class suppressAnnotation) {
- this(
- state,
- knownTypes,
- markerAnnotations,
- acceptedAnnotations,
- containerOfAnnotation,
- suppressAnnotation,
- /* typeParameterAnnotation= */ null);
- }
-
- /** Use {@link #builder()} instead. */
- @Deprecated
- public ThreadSafety(
- VisitorState state,
- KnownTypes knownTypes,
- Set markerAnnotations,
- Set acceptedAnnotations,
- @Nullable Class containerOfAnnotation,
- @Nullable Class suppressAnnotation,
- @Nullable Class typeParameterAnnotation) {
- this(
- state,
- Purpose.FOR_IMMUTABLE_CHECKER,
- knownTypes,
- markerAnnotations,
- acceptedAnnotations,
- containerOfAnnotation,
- suppressAnnotation,
- typeParameterAnnotation);
- }
-
private ThreadSafety(
VisitorState state,
Purpose purpose,
@@ -651,6 +620,7 @@ public AnnotationInfo getMarkerOrAcceptedAnnotation(Symbol sym, VisitorState sta
}
/** Returns an enclosing instance for the specified type if it is thread-safe. */
+ @Nullable
public Type mutableEnclosingInstance(Optional tree, ClassType type) {
if (tree.isPresent()
&& !CanBeStaticAnalyzer.referencesOuter(
@@ -716,6 +686,7 @@ public Set threadSafeTypeParametersInScope(Symbol sym) {
return result.build();
}
+ @Nullable
private AnnotationInfo getAnnotation(
Symbol sym, ImmutableSet annotationsToCheck, VisitorState state) {
for (String annotation : annotationsToCheck) {
@@ -727,6 +698,7 @@ private AnnotationInfo getAnnotation(
return null;
}
+ @Nullable
private AnnotationInfo getAnnotation(
Symbol sym,
VisitorState state,
@@ -742,6 +714,7 @@ private AnnotationInfo getAnnotation(
if (attr.isPresent()) {
ImmutableList containerElements = containerOf(state, attr.get());
if (elementAnnotation != null && containerElements.isEmpty()) {
+
containerElements =
sym.getTypeParameters().stream()
.filter(p -> p.getAnnotation(elementAnnotation) != null)
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 cc947940433..42352e603ae 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
@@ -27,6 +27,7 @@
import com.google.common.primitives.Primitives;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.annotations.Immutable;
import com.google.errorprone.bugpatterns.ImmutableCollections;
import com.google.errorprone.suppliers.Supplier;
@@ -96,6 +97,7 @@ public Set getKnownUnsafeClasses() {
static class Builder {
final ImmutableMap.Builder mapBuilder = ImmutableMap.builder();
+ @CanIgnoreReturnValue
public Builder addClasses(Set> clazzs) {
for (Class clazz : clazzs) {
add(clazz);
@@ -103,6 +105,7 @@ public Builder addClasses(Set> clazzs) {
return this;
}
+ @CanIgnoreReturnValue
public Builder addStrings(List classNames) {
for (String className : classNames) {
add(className);
@@ -110,6 +113,7 @@ public Builder addStrings(List classNames) {
return this;
}
+ @CanIgnoreReturnValue
public Builder add(Class clazz, String... containerOf) {
ImmutableSet containerTyParams = ImmutableSet.copyOf(containerOf);
HashSet actualTyParams = new HashSet<>();
@@ -129,6 +133,7 @@ public Builder add(Class clazz, String... containerOf) {
return this;
}
+ @CanIgnoreReturnValue
public Builder add(String className, String... containerOf) {
mapBuilder.put(
className, AnnotationInfo.create(className, ImmutableList.copyOf(containerOf)));
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/time/InvalidJavaTimeConstant.java b/core/src/main/java/com/google/errorprone/bugpatterns/time/InvalidJavaTimeConstant.java
index 8bdbd284f14..72b3a551c78 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/time/InvalidJavaTimeConstant.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/time/InvalidJavaTimeConstant.java
@@ -41,6 +41,7 @@
import com.google.common.collect.Maps;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.matchers.Description;
@@ -111,6 +112,7 @@ public abstract static class Builder {
abstract ImmutableList.Builder methodsBuilder();
+ @CanIgnoreReturnValue
public Builder addStaticMethod(String methodName, Param... params) {
methodsBuilder()
.add(
@@ -123,6 +125,7 @@ public Builder addStaticMethod(String methodName, Param... params) {
return this;
}
+ @CanIgnoreReturnValue
public Builder addInstanceMethod(String methodName, Param... params) {
methodsBuilder()
.add(
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatch.java b/core/src/main/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatch.java
index 0d291d8bb7b..38329e38085 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatch.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatch.java
@@ -371,6 +371,7 @@ private static String extractArgumentName(ExpressionTree expr) {
isSameType("java.lang.Long"),
isSameType("java.lang.Double"));
+ @Nullable
@VisibleForTesting
static TimeUnit unitSuggestedByName(String name) {
// Tuple types, especially Pair, trip us up. Skip APIs that might be from them.
diff --git a/core/src/main/java/com/google/errorprone/refaster/Bindings.java b/core/src/main/java/com/google/errorprone/refaster/Bindings.java
index 6904e63ee69..1b4754dfdb8 100644
--- a/core/src/main/java/com/google/errorprone/refaster/Bindings.java
+++ b/core/src/main/java/com/google/errorprone/refaster/Bindings.java
@@ -23,6 +23,7 @@
import com.google.common.collect.ForwardingMap;
import com.google.common.collect.Maps;
import com.google.common.reflect.TypeToken;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
@@ -128,6 +129,8 @@ public V putBinding(Key key, V value) {
return (V) super.put(key, value);
}
+ @CanIgnoreReturnValue
+ @Nullable
@Override
public Object put(Key key, Object value) {
checkNotNull(key, "key");
diff --git a/core/src/main/java/com/google/errorprone/refaster/Choice.java b/core/src/main/java/com/google/errorprone/refaster/Choice.java
index 6e1c27ec600..b385314b3d0 100644
--- a/core/src/main/java/com/google/errorprone/refaster/Choice.java
+++ b/core/src/main/java/com/google/errorprone/refaster/Choice.java
@@ -25,6 +25,7 @@
import com.google.common.base.Predicate;
import com.google.common.collect.Iterables;
import com.google.common.collect.Iterators;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.annotations.ForOverride;
import java.util.Collection;
import java.util.Collections;
@@ -78,6 +79,7 @@ public Choice
*/
@SafeVarargs
+ @CanIgnoreReturnValue
public static {@code ... }`:
+
+```java
+/**
+ * Summary fragment.
+ *
+ * {@code
+ * Your code here.
+ * Can include .
+ * You can even include snippets that contain annotations, e.g.:
+ * @Override public String toString() { ... }
+ * }
+ *
+ * Following paragraph.
+ */
+```
+
## Suppression
Suppress by applying `@SuppressWarnings("InvalidInlineTag")` to the element
diff --git a/pom.xml b/pom.xml
index f6e3708749b..870d37b63a1 100644
--- a/pom.xml
+++ b/pom.xml
@@ -21,7 +21,7 @@
Error message keys that don't match any diagnostics will cause test to fail.
*/
+ @CanIgnoreReturnValue
public CompilationTestHelper expectErrorMessage(String key, Predicate matcher) {
diagnosticHelper.expectErrorMessage(key, matcher);
return this;
diff --git a/type_annotations/pom.xml b/type_annotations/pom.xml
index b2495a2167f..50fa69762ee 100644
--- a/type_annotations/pom.xml
+++ b/type_annotations/pom.xml
@@ -21,7 +21,7 @@