diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f4c9e8cc024..d980631e04c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,6 +33,9 @@ jobs: - os: ubuntu-latest java: 21-ea experimental: true + - os: ubuntu-latest + java: 22-ea + experimental: true runs-on: ${{ matrix.os }} continue-on-error: ${{ matrix.experimental }} steps: diff --git a/annotations/src/main/java/com/google/errorprone/annotations/RestrictedApi.java b/annotations/src/main/java/com/google/errorprone/annotations/RestrictedApi.java index 547f50a84a1..e5fbba8a2aa 100644 --- a/annotations/src/main/java/com/google/errorprone/annotations/RestrictedApi.java +++ b/annotations/src/main/java/com/google/errorprone/annotations/RestrictedApi.java @@ -85,8 +85,8 @@ /** Explanation why the API is restricted, to be inserted into the compiler output. */ String explanation(); - /** Link explaining why the API is restricted */ - String link(); + /** Optional link explaining why the API is restricted. */ + String link() default ""; /** * Allow the restricted API on paths matching this regular expression. diff --git a/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java b/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java index c4e03f89597..a9afc76ef51 100644 --- a/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java +++ b/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java @@ -52,8 +52,8 @@ public class ErrorProneOptions { private static final String PATCH_IMPORT_ORDER_PREFIX = "-XepPatchImportOrder:"; private static final String EXCLUDED_PATHS_PREFIX = "-XepExcludedPaths:"; private static final String IGNORE_LARGE_CODE_GENERATORS = "-XepIgnoreLargeCodeGenerators:"; - private static final String ERRORS_AS_WARNINGS_FLAG = "-XepAllErrorsAsWarnings"; + private static final String SUGGESTIONS_AS_WARNINGS_FLAG = "-XepAllSuggestionsAsWarnings"; private static final String ENABLE_ALL_CHECKS = "-XepAllDisabledChecksAsWarnings"; private static final String IGNORE_SUPPRESSION_ANNOTATIONS = "-XepIgnoreSuppressionAnnotations"; private static final String DISABLE_ALL_CHECKS = "-XepDisableAllChecks"; @@ -75,6 +75,7 @@ public static int isSupportedOption(String option) { || option.equals(IGNORE_UNKNOWN_CHECKS_FLAG) || option.equals(DISABLE_WARNINGS_IN_GENERATED_CODE_FLAG) || option.equals(ERRORS_AS_WARNINGS_FLAG) + || option.equals(SUGGESTIONS_AS_WARNINGS_FLAG) || option.equals(ENABLE_ALL_CHECKS) || option.equals(DISABLE_ALL_CHECKS) || option.equals(IGNORE_SUPPRESSION_ANNOTATIONS) @@ -163,6 +164,7 @@ final PatchingOptions build() { private final boolean disableWarningsInGeneratedCode; private final boolean disableAllWarnings; private final boolean dropErrorsToWarnings; + private final boolean suggestionsAsWarnings; private final boolean enableAllChecksAsWarnings; private final boolean disableAllChecks; private final boolean isTestOnlyTarget; @@ -180,6 +182,7 @@ private ErrorProneOptions( boolean disableWarningsInGeneratedCode, boolean disableAllWarnings, boolean dropErrorsToWarnings, + boolean suggestionsAsWarnings, boolean enableAllChecksAsWarnings, boolean disableAllChecks, boolean isTestOnlyTarget, @@ -195,6 +198,7 @@ private ErrorProneOptions( this.disableWarningsInGeneratedCode = disableWarningsInGeneratedCode; this.disableAllWarnings = disableAllWarnings; this.dropErrorsToWarnings = dropErrorsToWarnings; + this.suggestionsAsWarnings = suggestionsAsWarnings; this.enableAllChecksAsWarnings = enableAllChecksAsWarnings; this.disableAllChecks = disableAllChecks; this.isTestOnlyTarget = isTestOnlyTarget; @@ -230,6 +234,10 @@ public boolean isDropErrorsToWarnings() { return dropErrorsToWarnings; } + public boolean isSuggestionsAsWarnings() { + return suggestionsAsWarnings; + } + public boolean isTestOnlyTarget() { return isTestOnlyTarget; } @@ -263,6 +271,7 @@ private static class Builder { private boolean disableAllWarnings = false; private boolean disableWarningsInGeneratedCode = false; private boolean dropErrorsToWarnings = false; + private boolean suggestionsAsWarnings = false; private boolean enableAllChecksAsWarnings = false; private boolean disableAllChecks = false; private boolean isTestOnlyTarget = false; @@ -319,6 +328,10 @@ public void setDropErrorsToWarnings(boolean dropErrorsToWarnings) { this.dropErrorsToWarnings = dropErrorsToWarnings; } + public void setSuggestionsAsWarnings(boolean suggestionsAsWarnings) { + this.suggestionsAsWarnings = suggestionsAsWarnings; + } + public void setDisableAllWarnings(boolean disableAllWarnings) { severityMap.entrySet().stream() .filter(e -> e.getValue() == Severity.WARN) @@ -364,6 +377,7 @@ public ErrorProneOptions build(ImmutableList remainingArgs) { disableWarningsInGeneratedCode, disableAllWarnings, dropErrorsToWarnings, + suggestionsAsWarnings, enableAllChecksAsWarnings, disableAllChecks, isTestOnlyTarget, @@ -420,6 +434,9 @@ public static ErrorProneOptions processArgs(Iterable args) { case ERRORS_AS_WARNINGS_FLAG: builder.setDropErrorsToWarnings(true); break; + case SUGGESTIONS_AS_WARNINGS_FLAG: + builder.setSuggestionsAsWarnings(true); + break; case ENABLE_ALL_CHECKS: builder.setEnableAllChecksAsWarnings(true); break; diff --git a/check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java b/check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java index 63f2a63890b..4693b70ef84 100644 --- a/check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java +++ b/check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java @@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableRangeSet; import com.google.common.collect.Iterables; import com.google.common.collect.Range; +import com.google.common.collect.TreeRangeSet; import com.google.errorprone.BugCheckerInfo; import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.ErrorProneOptions; @@ -306,7 +307,7 @@ public boolean isSuppressed(Symbol sym, VisitorState state) { /** Computes a RangeSet of code regions which are suppressed by this bug checker. */ public ImmutableRangeSet suppressedRegions(VisitorState state) { - ImmutableRangeSet.Builder suppressedRegions = ImmutableRangeSet.builder(); + TreeRangeSet suppressedRegions = TreeRangeSet.create(); new TreeScanner() { @Override public Void scan(Tree tree, Void unused) { @@ -318,7 +319,7 @@ public Void scan(Tree tree, Void unused) { return null; } }.scan(state.getPath().getCompilationUnit(), null); - return suppressedRegions.build(); + return ImmutableRangeSet.copyOf(suppressedRegions); } public interface AnnotationTreeMatcher extends Suppressible { 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 a17548bc8fa..10aa81a162a 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 @@ -103,6 +103,7 @@ public boolean canAlias(JavaExpression a, JavaExpression b) { public String visualize(CFGVisualizer, ?> cfgVisualizer) { throw new UnsupportedOperationException("DOT output not supported"); } + /** * Builder for {@link AccessPathStore} instances. To obtain an instance, obtain a {@link * AccessPathStore} (such as {@link AccessPathStore#empty()}), and call {@link diff --git a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/inference/NullnessQualifierInference.java b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/inference/NullnessQualifierInference.java index 5c0ceb4fc18..8b47db97e17 100644 --- a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/inference/NullnessQualifierInference.java +++ b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/inference/NullnessQualifierInference.java @@ -301,23 +301,23 @@ public Void visitMethodInvocation(MethodInvocationTree node, Void unused) { // allow the inference to conclude later that x[0] = m(x)[0, 0], meaning the nullness qualifier // for x's is the same as the one for m(x)'s . for (TypeVariableSymbol typeVar : callee.getTypeParameters()) { - TypeVariableInferenceVar typeVarIV = TypeVariableInferenceVar.create(typeVar, node); + TypeVariableInferenceVar typeVarIv = TypeVariableInferenceVar.create(typeVar, node); visitUnannotatedTypeVarRefsAndEquateInferredComponents( - typeVarIV, + typeVarIv, callee.getReturnType(), callee, node, - iv -> qualifierConstraints.putEdge(typeVarIV, iv)); + iv -> qualifierConstraints.putEdge(typeVarIv, iv)); Streams.forEachPair( formalParameters.stream(), node.getArguments().stream(), (formal, actual) -> visitUnannotatedTypeVarRefsAndEquateInferredComponents( - typeVarIV, + typeVarIv, formal.type(), formal.symbol(), actual, - iv -> qualifierConstraints.putEdge(iv, typeVarIV))); + iv -> qualifierConstraints.putEdge(iv, typeVarIv))); } return super.visitMethodInvocation(node, unused); } 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 4f6980cff9d..6b2e28d3562 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 @@ -67,6 +67,7 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.InstanceOfTree; +import com.sun.source.tree.MemberReferenceTree; import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; @@ -785,6 +786,17 @@ public Void visitMemberSelect(MemberSelectTree tree, Void unused) { } return super.visitMemberSelect(tree, null); } + + @Override + public Void visitMemberReference(MemberReferenceTree tree, Void unused) { + if (sym.equals(getSymbol(tree))) { + fix.replace( + state.getEndPosition(tree.getQualifierExpression()), + state.getEndPosition(tree), + "::" + replacement); + } + return super.visitMemberReference(tree, unused); + } }.scan(state.getPath().getCompilationUnit(), null); return fix.build(); } @@ -1047,6 +1059,7 @@ public static void addSuppressWarnings( fixBuilder.prefixWith(suppressibleNode, replacement); } } + /** * Modifies {@code fixBuilder} to either remove a {@code warningToRemove} warning from the closest * {@code SuppressWarning} node or remove the entire {@code SuppressWarning} node if {@code 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 85b98bb1bac..718bdd7ddeb 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 @@ -206,6 +206,7 @@ private static boolean hasJUnitAttr(MethodSymbol methodSym) { isJunit3TestCase, hasAnnotation(JUNIT4_TEST_ANNOTATION), hasAnnotation(JUNIT4_THEORY_ANNOTATION)); + /** * A list of test runners that this matcher should look for in the @RunWith annotation. Subclasses * of the test runners are also matched. diff --git a/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java b/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java index 58fce102e7d..a1177144eab 100644 --- a/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java +++ b/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java @@ -1294,6 +1294,12 @@ public static Matcher isDirectImplementationOf(String clazz) { return new IsDirectImplementationOf(isProvidedType); } + /** Matches any node that is a direct extension of the given class. */ + public static Matcher isExtensionOf(String clazz) { + Matcher isProvidedType = isSameType(clazz); + return new IsExtensionOf(isProvidedType); + } + @SafeVarargs public static Matcher hasAnyAnnotation(Class... annotations) { ArrayList> matchers = new ArrayList<>(annotations.length); @@ -1332,6 +1338,7 @@ public static boolean isThrowingFunctionalInterface(Type clazzType, VisitorState return CLASSES_CONSIDERED_THROWING.get(state).stream() .anyMatch(t -> isSubtype(clazzType, t, state)); } + /** * {@link FunctionalInterface}s that are generally used as a lambda expression for 'a block of * code that's going to fail', e.g.: @@ -1368,6 +1375,22 @@ protected Iterable getChildNodes(ClassTree classTree, VisitorSta } } + private static class IsExtensionOf extends ChildMultiMatcher { + IsExtensionOf(Matcher classMatcher) { + super(MatchType.AT_LEAST_ONE, classMatcher); + } + + @Override + protected Iterable getChildNodes(ClassTree classTree, VisitorState state) { + List matched = new ArrayList<>(); + Tree extendsClause = classTree.getExtendsClause(); + if (extendsClause != null) { + matched.add(extendsClause); + } + return matched; + } + } + /** Matches an AST node whose compilation unit's package name matches the given predicate. */ public static Matcher packageMatches(Predicate predicate) { return (tree, state) -> predicate.test(getPackageFullName(state)); diff --git a/check_api/src/main/java/com/google/errorprone/matchers/MultiMatcher.java b/check_api/src/main/java/com/google/errorprone/matchers/MultiMatcher.java index 29187006c30..25b9d39f86e 100644 --- a/check_api/src/main/java/com/google/errorprone/matchers/MultiMatcher.java +++ b/check_api/src/main/java/com/google/errorprone/matchers/MultiMatcher.java @@ -43,8 +43,10 @@ public interface MultiMatcher extends Matcher @AutoValue abstract class MultiMatchResult { MultiMatchResult() {} + /** True if the MultiMatcher matched the nodes expected. */ public abstract boolean matches(); + /** * The list of nodes which matched the MultiMatcher's expectations (could be empty if the match * type was ALL and there were no child nodes). Only sensical if {@link #matches()} is true. diff --git a/check_api/src/main/java/com/google/errorprone/predicates/TypePredicates.java b/check_api/src/main/java/com/google/errorprone/predicates/TypePredicates.java index 7b50cf6adce..4042bc56f33 100644 --- a/check_api/src/main/java/com/google/errorprone/predicates/TypePredicates.java +++ b/check_api/src/main/java/com/google/errorprone/predicates/TypePredicates.java @@ -30,6 +30,16 @@ /** A collection of {@link TypePredicate}s. */ public final class TypePredicates { + /** Matches nothing (always {@code false}). */ + public static TypePredicate nothing() { + return (type, state) -> false; + } + + /** Matches everything (always {@code true}). */ + public static TypePredicate anything() { + return (type, state) -> true; + } + /** Match arrays. */ public static TypePredicate isArray() { return Array.INSTANCE; @@ -55,16 +65,16 @@ public static TypePredicate isDescendantOf(Supplier type) { return new DescendantOf(type); } - /** Match types that are a sub-type of one of the given types. */ - public static TypePredicate isDescendantOfAny(Iterable types) { - return new DescendantOfAny(fromStrings(types)); - } - /** Match sub-types of the given type. */ public static TypePredicate isDescendantOf(String type) { return isDescendantOf(Suppliers.typeFromString(type)); } + /** Match types that are a sub-type of one of the given types. */ + public static TypePredicate isDescendantOfAny(Iterable types) { + return new DescendantOfAny(fromStrings(types)); + } + public static TypePredicate allOf(TypePredicate... predicates) { return (type, state) -> { for (TypePredicate predicate : predicates) { diff --git a/check_api/src/main/java/com/google/errorprone/scanner/ScannerSupplier.java b/check_api/src/main/java/com/google/errorprone/scanner/ScannerSupplier.java index 1bb26212bb3..35eee823f0a 100644 --- a/check_api/src/main/java/com/google/errorprone/scanner/ScannerSupplier.java +++ b/check_api/src/main/java/com/google/errorprone/scanner/ScannerSupplier.java @@ -147,7 +147,8 @@ public ScannerSupplier applyOverrides(ErrorProneOptions errorProneOptions) { && !errorProneOptions.isEnableAllChecksAsWarnings() && !errorProneOptions.isDropErrorsToWarnings() && !errorProneOptions.isDisableAllChecks() - && !errorProneOptions.isDisableAllWarnings()) { + && !errorProneOptions.isDisableAllWarnings() + && !errorProneOptions.isSuggestionsAsWarnings()) { return this; } @@ -168,6 +169,12 @@ public ScannerSupplier applyOverrides(ErrorProneOptions errorProneOptions) { .forEach(c -> severities.put(c.canonicalName(), SeverityLevel.WARNING)); } + if (errorProneOptions.isSuggestionsAsWarnings()) { + getAllChecks().values().stream() + .filter(c -> c.defaultSeverity() == SeverityLevel.SUGGESTION) + .forEach(c -> severities.put(c.canonicalName(), SeverityLevel.WARNING)); + } + if (errorProneOptions.isDisableAllWarnings()) { checks.values().stream() .filter(c -> c.defaultSeverity() == SeverityLevel.WARNING) diff --git a/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java b/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java index 2d04be455f1..15dc8893dfb 100644 --- a/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java +++ b/check_api/src/test/java/com/google/errorprone/ErrorProneOptionsTest.java @@ -152,6 +152,13 @@ public void recognizesDemoteErrorToWarning() { assertThat(options.isDropErrorsToWarnings()).isTrue(); } + @Test + public void recognizesAllSuggestionsAsWarnings() { + ErrorProneOptions options = + ErrorProneOptions.processArgs(new String[] {"-XepAllSuggestionsAsWarnings"}); + assertThat(options.isSuggestionsAsWarnings()).isTrue(); + } + @Test public void recognizesDisableAllChecks() { ErrorProneOptions options = diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ASTHelpersSuggestions.java b/core/src/main/java/com/google/errorprone/bugpatterns/ASTHelpersSuggestions.java index d375a117687..b75e4e485e4 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ASTHelpersSuggestions.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ASTHelpersSuggestions.java @@ -50,7 +50,8 @@ public class ASTHelpersSuggestions extends BugChecker implements MethodInvocatio anyOf( instanceMethod() .onDescendantOf("com.sun.tools.javac.code.Symbol") - .namedAnyOf("isDirectlyOrIndirectlyLocal", "isLocal", "packge"), + .namedAnyOf( + "isDirectlyOrIndirectlyLocal", "isLocal", "packge", "getEnclosedElements"), instanceMethod() .onClass((t, s) -> isSubtype(MODULE_SYMBOL.get(s), t, s)) .namedAnyOf("isStatic")); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractBanUnsafeAPIChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractBanUnsafeAPIChecker.java new file mode 100644 index 00000000000..fc5bc7c6e5c --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractBanUnsafeAPIChecker.java @@ -0,0 +1,37 @@ +/* + * Copyright 2023 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 com.google.errorprone.VisitorState; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.Tree; + +/** An abstract class that detects use of the unsafe APIs. */ +public abstract class AbstractBanUnsafeAPIChecker extends BugChecker { + + protected Description matchHelper( + T tree, VisitorState state, Matcher matcher) { + if (state.errorProneOptions().isTestOnlyTarget() || !matcher.matches(tree, state)) { + return Description.NO_MATCH; + } + + Description.Builder description = buildDescription(tree); + + return description.build(); + } +} 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 6326e96a041..e65fafd6230 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java @@ -91,6 +91,7 @@ public abstract class AbstractMustBeClosedChecker extends BugChecker { static final class NameSuggester { private final Multiset assignedNamesInThisMethod = HashMultiset.create(); + /** Returns basename if there are no conflicts, then basename + "2", then basename + "3"... */ String uniquifyName(String basename) { int numPreviousConflicts = assignedNamesInThisMethod.add(basename, 1); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AddressSelection.java b/core/src/main/java/com/google/errorprone/bugpatterns/AddressSelection.java new file mode 100644 index 00000000000..3e2be15e29a --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AddressSelection.java @@ -0,0 +1,110 @@ +/* + * Copyright 2023 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.getOnlyElement; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.fixes.SuggestedFixes.qualifyType; +import static com.google.errorprone.fixes.SuggestedFixes.renameMethodInvocation; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.method.MethodMatchers.constructor; +import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static com.google.errorprone.util.ASTHelpers.constValue; + +import com.google.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import java.util.Objects; +import java.util.function.Supplier; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = + "Prefer InetAddress.getAllName to APIs that convert a hostname to a single IP address", + severity = WARNING) +public final class AddressSelection extends BugChecker + implements NewClassTreeMatcher, MethodInvocationTreeMatcher { + + private static final Matcher CONSTRUCTORS = + Matchers.anyOf( + constructor().forClass("java.net.Socket").withParameters("java.lang.String", "int"), + constructor() + .forClass("java.net.InetSocketAddress") + .withParameters("java.lang.String", "int")); + private static final Matcher METHODS = + staticMethod() + .onClass("java.net.InetAddress") + .named("getByName") + .withParameters("java.lang.String"); + + @Override + public Description matchNewClass(NewClassTree tree, VisitorState state) { + if (!CONSTRUCTORS.matches(tree, state)) { + return NO_MATCH; + } + ExpressionTree argument = tree.getArguments().get(0); + return handleMatch( + argument, + argument, + () -> { + SuggestedFix.Builder fix = SuggestedFix.builder(); + fix.replace( + argument, qualifyType(state, fix, "java.net.InetAddress") + ".getLoopbackAddress()"); + return fix.build(); + }); + } + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!METHODS.matches(tree, state)) { + return NO_MATCH; + } + ExpressionTree argument = getOnlyElement(tree.getArguments()); + return handleMatch( + argument, + tree, + () -> + SuggestedFix.builder() + .merge(renameMethodInvocation(tree, "getLoopbackAddress", state)) + .delete(argument) + .build()); + } + + private static final ImmutableSet LOOPBACK = ImmutableSet.of("127.0.0.1", "::1"); + + private Description handleMatch( + ExpressionTree argument, ExpressionTree replacement, Supplier fix) { + String value = constValue(argument, String.class); + if (Objects.equals(value, "localhost")) { + return NO_MATCH; + } + Description.Builder description = buildDescription(replacement); + if (LOOPBACK.contains(value)) { + description.addFix(fix.get()); + } + return description.build(); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AttemptedNegativeZero.java b/core/src/main/java/com/google/errorprone/bugpatterns/AttemptedNegativeZero.java new file mode 100644 index 00000000000..b6f7aef32b3 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AttemptedNegativeZero.java @@ -0,0 +1,64 @@ +/* + * Copyright 2023 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.util.ASTHelpers.constValue; +import static com.google.errorprone.util.ASTHelpers.targetType; +import static com.sun.source.tree.Tree.Kind.UNARY_MINUS; +import static com.sun.tools.javac.code.TypeTag.DOUBLE; +import static com.sun.tools.javac.code.TypeTag.FLOAT; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.UnaryTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.UnaryTree; + +/** A BugPattern; see the summary. */ +@BugPattern( + severity = WARNING, + summary = "-0 is the same as 0. For the floating-point negative zero, use -0.0.") +public class AttemptedNegativeZero extends BugChecker implements UnaryTreeMatcher { + @Override + public Description matchUnary(UnaryTree tree, VisitorState state) { + if (tree.getKind() != UNARY_MINUS) { + return NO_MATCH; + } + Object negatedConstValue = constValue(tree.getExpression()); + if (negatedConstValue == null) { + return NO_MATCH; + } + if (!negatedConstValue.equals(0) && !negatedConstValue.equals(0L)) { + return NO_MATCH; + } + String replacement; + switch (targetType(state).type().getTag()) { + case DOUBLE: + replacement = "-0.0"; + break; + case FLOAT: + replacement = "-0.0f"; + break; + default: + return NO_MATCH; + } + return describeMatch(tree, SuggestedFix.builder().replace(tree, replacement).build()); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/BanClassLoader.java b/core/src/main/java/com/google/errorprone/bugpatterns/BanClassLoader.java index e5a1be03bd4..f7528a7dd9f 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/BanClassLoader.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/BanClassLoader.java @@ -18,15 +18,23 @@ import static com.google.errorprone.matchers.Matchers.anyMethod; import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.constructor; +import static com.google.errorprone.matchers.Matchers.isExtensionOf; +import static com.google.errorprone.predicates.TypePredicates.allOf; +import static com.google.errorprone.predicates.TypePredicates.isDescendantOf; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.ClassTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; /** A {@link BugChecker} that detects use of the unsafe JNDI API system. */ @BugPattern( @@ -34,15 +42,12 @@ "Using dangerous ClassLoader APIs may deserialize untrusted user input into bytecode," + " leading to remote code execution vulnerabilities", severity = SeverityLevel.ERROR) -public final class BanClassLoader extends BugChecker implements MethodInvocationTreeMatcher { +public final class BanClassLoader extends AbstractBanUnsafeAPIChecker + implements MethodInvocationTreeMatcher, NewClassTreeMatcher, ClassTreeMatcher { - /** Checks for direct or indirect calls to context.lookup() via the JDK */ - private static final Matcher MATCHER = + private static final Matcher METHOD_MATCHER = anyOf( - anyMethod() - .onDescendantOf("java.lang.ClassLoader") - // findClass in ClassLoader is abstract, but in URLClassLoader it's dangerous - .namedAnyOf("defineClass", "findClass"), + anyMethod().onDescendantOf("java.lang.ClassLoader").named("defineClass"), anyMethod().onDescendantOf("java.lang.invoke.MethodHandles.Lookup").named("defineClass"), anyMethod() .onDescendantOf("java.rmi.server.RMIClassLoader") @@ -51,14 +56,24 @@ public final class BanClassLoader extends BugChecker implements MethodInvocation .onDescendantOf("java.rmi.server.RMIClassLoaderSpi") .namedAnyOf("loadClass", "loadProxyClass")); + private static final Matcher CONSTRUCTOR_MATCHER = + constructor().forClass(allOf(isDescendantOf("java.net.URLClassLoader"))); + + private static final Matcher EXTEND_CLASS_MATCHER = + isExtensionOf("java.net.URLClassLoader"); + @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (state.errorProneOptions().isTestOnlyTarget() || !MATCHER.matches(tree, state)) { - return Description.NO_MATCH; - } + return matchHelper(tree, state, METHOD_MATCHER); + } - Description.Builder description = buildDescription(tree); + @Override + public Description matchNewClass(NewClassTree tree, VisitorState state) { + return matchHelper(tree, state, CONSTRUCTOR_MATCHER); + } - return description.build(); + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + return matchHelper(tree, state, EXTEND_CLASS_MATCHER); } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/BanJNDI.java b/core/src/main/java/com/google/errorprone/bugpatterns/BanJNDI.java index 3b327a4a7e1..6d8152a91ea 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/BanJNDI.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/BanJNDI.java @@ -25,7 +25,6 @@ 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; /** A {@link BugChecker} that detects use of the unsafe JNDI API system. */ @@ -34,10 +33,11 @@ "Using JNDI may deserialize user input via the `Serializable` API which is extremely" + " dangerous", severity = SeverityLevel.ERROR) -public final class BanJNDI extends BugChecker implements MethodInvocationTreeMatcher { +public final class BanJNDI extends AbstractBanUnsafeAPIChecker + implements MethodInvocationTreeMatcher { /** Checks for direct or indirect calls to context.lookup() via the JDK */ - private static final Matcher MATCHER = + private static final Matcher MATCHER = anyOf( anyMethod() .onDescendantOf("javax.naming.directory.DirContext") @@ -70,12 +70,6 @@ public final class BanJNDI extends BugChecker implements MethodInvocationTreeMat @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (state.errorProneOptions().isTestOnlyTarget() || !MATCHER.matches(tree, state)) { - return Description.NO_MATCH; - } - - Description.Builder description = buildDescription(tree); - - return description.build(); + return this.matchHelper(tree, state, MATCHER); } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/BanSerializableRead.java b/core/src/main/java/com/google/errorprone/bugpatterns/BanSerializableRead.java index 8e5d3ab1901..c050c9f9606 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/BanSerializableRead.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/BanSerializableRead.java @@ -32,16 +32,16 @@ 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; /** A {@link BugChecker} that detects use of the unsafe {@link java.io.Serializable} API. */ @BugPattern( summary = "Deserializing user input via the `Serializable` API is extremely dangerous", severity = SeverityLevel.ERROR) -public final class BanSerializableRead extends BugChecker implements MethodInvocationTreeMatcher { +public final class BanSerializableRead extends AbstractBanUnsafeAPIChecker + implements MethodInvocationTreeMatcher { - private static final Matcher EXEMPT = + private static final Matcher EXEMPT = anyOf( // This is called through ObjectInputStream; a call further up the callstack will have // been exempt. @@ -56,7 +56,7 @@ public final class BanSerializableRead extends BugChecker implements MethodInvoc methodTree.getName().toString())))); /** Checks for unsafe deserialization calls on an ObjectInputStream in an ExpressionTree. */ - private static final Matcher OBJECT_INPUT_STREAM_DESERIALIZE_MATCHER = + private static final Matcher OBJECT_INPUT_STREAM_DESERIALIZE_MATCHER = allOf( anyOf( // this matches calls to the ObjectInputStream to read some objects @@ -91,16 +91,11 @@ public final class BanSerializableRead extends BugChecker implements MethodInvoc not(EXEMPT)); /** Checks for unsafe uses of the Java deserialization API. */ - private static final Matcher MATCHER = OBJECT_INPUT_STREAM_DESERIALIZE_MATCHER; + private static final Matcher MATCHER = + OBJECT_INPUT_STREAM_DESERIALIZE_MATCHER; @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (state.errorProneOptions().isTestOnlyTarget() || !MATCHER.matches(tree, state)) { - return Description.NO_MATCH; - } - - Description.Builder description = buildDescription(tree); - - return description.build(); + return this.matchHelper(tree, state, MATCHER); } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/CatchAndPrintStackTrace.java b/core/src/main/java/com/google/errorprone/bugpatterns/CatchAndPrintStackTrace.java index ef23e4d6f8d..285525f24e4 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/CatchAndPrintStackTrace.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/CatchAndPrintStackTrace.java @@ -42,7 +42,10 @@ public class CatchAndPrintStackTrace extends BugChecker implements CatchTreeMatc private static final Matcher MATCHER = matchSingleStatementBlock( expressionStatement( - instanceMethod().onDescendantOf("java.lang.Throwable").named("printStackTrace"))); + instanceMethod() + .onDescendantOf("java.lang.Throwable") + .named("printStackTrace") + .withNoParameters())); @Override public Description matchCatch(CatchTree tree, VisitorState state) { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DeduplicateConstants.java b/core/src/main/java/com/google/errorprone/bugpatterns/DeduplicateConstants.java index 9b0ef9382d4..19e4a1dfe14 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/DeduplicateConstants.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/DeduplicateConstants.java @@ -59,6 +59,7 @@ static class Scope { /** A map from string literals to constant declarations. */ private final HashMap values = new HashMap<>(); + /** Declarations that are hidden in the current scope. */ private final Set hidden = new HashSet<>(); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DifferentNameButSame.java b/core/src/main/java/com/google/errorprone/bugpatterns/DifferentNameButSame.java index f16941d65fb..ff98ae9a804 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/DifferentNameButSame.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/DifferentNameButSame.java @@ -128,6 +128,7 @@ private void handle(Tree tree) { if (!(symbol instanceof ClassSymbol)) { return; } + @SuppressWarnings("TreeToString") // TODO(ghm): fix String name = tree.toString(); List treePaths = names.get(symbol, name); if (treePaths == null) { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DoNotUseRuleChain.java b/core/src/main/java/com/google/errorprone/bugpatterns/DoNotUseRuleChain.java index e4b0e8f582a..657caa10955 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/DoNotUseRuleChain.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/DoNotUseRuleChain.java @@ -49,7 +49,10 @@ */ @BugPattern( tags = {StandardTags.REFACTORING}, - summary = "Do not use RuleChain", + summary = + "Prefer using `@Rule` with an explicit order over declaring a `RuleChain`. " + + "RuleChain was the only way to declare ordered rules before JUnit 4.13. Newer " + + "versions should use the cleaner individual `@Rule(order = n)` option.", severity = WARNING) public class DoNotUseRuleChain extends BugChecker implements VariableTreeMatcher { @@ -126,6 +129,7 @@ private static SuggestedFix refactor(VariableTree tree, VisitorState state) { replacement += extractRuleFromExpression(expression, i, tree, state); } + fix.removeImport(JUNIT_RULE_CHAIN_IMPORT_PATH); fix.replace(tree, replacement); return fix.build(); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/Finalize.java b/core/src/main/java/com/google/errorprone/bugpatterns/Finalize.java index 828cabf7be5..d7734ad0344 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/Finalize.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/Finalize.java @@ -31,7 +31,7 @@ import javax.lang.model.element.Modifier; /** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ -@BugPattern(summary = "Do not override finalize", severity = WARNING) +@BugPattern(summary = "Do not override finalize", severity = WARNING, documentSuppression = false) public class Finalize extends BugChecker implements MethodTreeMatcher { @Override diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ICCProfileGetInstance.java b/core/src/main/java/com/google/errorprone/bugpatterns/ICCProfileGetInstance.java new file mode 100644 index 00000000000..6ab77883d55 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ICCProfileGetInstance.java @@ -0,0 +1,64 @@ +/* + * Copyright 2023 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.method.MethodMatchers.staticMethod; + +import com.google.common.collect.Iterables; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; +import com.google.errorprone.VisitorState; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = + "This method searches the class path for the given file, prefer to read the file and call" + + " getInstance(byte[]) or getInstance(InputStream)", + severity = WARNING, + tags = StandardTags.PERFORMANCE) +public class ICCProfileGetInstance extends BugChecker + implements BugChecker.MethodInvocationTreeMatcher { + + private static final Matcher MATCHER = + staticMethod() + .onClass("java.awt.color.ICC_Profile") + .named("getInstance") + .withParameters("java.lang.String"); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!MATCHER.matches(tree, state)) { + return NO_MATCH; + } + ExpressionTree arg = Iterables.getOnlyElement(tree.getArguments()); + return describeMatch( + tree, + SuggestedFix.builder() + .prefixWith(arg, "Files.readAllBytes(Paths.get(") + .postfixWith(arg, "))") + .addImport("java.nio.file.Files") + .addImport("java.nio.file.Paths") + .build()); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/InlineTrivialConstant.java b/core/src/main/java/com/google/errorprone/bugpatterns/InlineTrivialConstant.java new file mode 100644 index 00000000000..1341efc7b12 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/InlineTrivialConstant.java @@ -0,0 +1,137 @@ +/* + * Copyright 2023 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.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.isSameType; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ListMultimap; +import com.google.common.collect.MultimapBuilder; +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.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.LiteralTree; +import com.sun.source.tree.MemberSelectTree; +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.Map; +import java.util.Objects; +import java.util.Optional; +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.Modifier; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern(summary = "Consider inlining this constant", severity = WARNING) +public class InlineTrivialConstant extends BugChecker implements CompilationUnitTreeMatcher { + + @AutoValue + abstract static class TrivialConstant { + abstract VariableTree tree(); + + abstract String replacement(); + + static TrivialConstant create(VariableTree tree, String replacement) { + return new AutoValue_InlineTrivialConstant_TrivialConstant(tree, replacement); + } + } + + @Override + public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { + Map fields = new HashMap<>(); + new SuppressibleTreePathScanner(state) { + @Override + public Void visitVariable(VariableTree tree, Void unused) { + VarSymbol sym = getSymbol(tree); + isTrivialConstant(tree, sym, state) + .ifPresent(r -> fields.put(sym, TrivialConstant.create(tree, r))); + return super.visitVariable(tree, null); + } + }.scan(state.getPath(), null); + ListMultimap uses = MultimapBuilder.hashKeys().arrayListValues().build(); + new TreePathScanner() { + @Override + public Void visitMemberSelect(MemberSelectTree tree, Void unused) { + handle(tree); + return super.visitMemberSelect(tree, null); + } + + @Override + public Void visitIdentifier(IdentifierTree tree, Void unused) { + handle(tree); + return super.visitIdentifier(tree, null); + } + + private void handle(Tree tree) { + Symbol sym = getSymbol(tree); + if (sym == null) { + return; + } + if (fields.containsKey(sym)) { + uses.put(sym, tree); + } + } + }.scan(state.getPath(), null); + for (Map.Entry e : fields.entrySet()) { + SuggestedFix.Builder fix = SuggestedFix.builder(); + TrivialConstant value = e.getValue(); + fix.delete(value.tree()); + uses.get(e.getKey()).forEach(x -> fix.replace(x, value.replacement())); + state.reportMatch(describeMatch(value.tree(), fix.build())); + } + return NO_MATCH; + } + + private static final ImmutableSet EMPTY_STRING_VARIABLE_NAMES = + ImmutableSet.of("EMPTY", "EMPTY_STR", "EMPTY_STRING"); + + private static Optional isTrivialConstant( + VariableTree tree, VarSymbol sym, VisitorState state) { + if (!(sym.getKind().equals(ElementKind.FIELD) + && sym.getModifiers().contains(Modifier.PRIVATE) + && sym.isStatic() + && sym.getModifiers().contains(Modifier.FINAL))) { + return Optional.empty(); + } + if (!EMPTY_STRING_VARIABLE_NAMES.contains(tree.getName().toString())) { + return Optional.empty(); + } + if (!isSameType(sym.asType(), state.getSymtab().stringType, state)) { + return Optional.empty(); + } + ExpressionTree initializer = tree.getInitializer(); + if (initializer.getKind().equals(Tree.Kind.STRING_LITERAL)) { + String value = (String) ((LiteralTree) initializer).getValue(); + if (Objects.equals(value, "")) { + return Optional.of("\"\""); + } + } + return Optional.empty(); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/InvalidTimeZoneID.java b/core/src/main/java/com/google/errorprone/bugpatterns/InvalidTimeZoneID.java index 94f2173b978..6ced9cfcbe7 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/InvalidTimeZoneID.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/InvalidTimeZoneID.java @@ -69,7 +69,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState // Value isn't a compile-time constant, so we can't know if it's unsafe. return Description.NO_MATCH; } - if (isValidID(value)) { + if (isValidId(value)) { // Value is supported on this JVM. return Description.NO_MATCH; } @@ -80,7 +80,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState // Try to see if it's just been mistyped with spaces instead of underscores - if so, offer this // as a potential fix. String spacesToUnderscores = value.replace(' ', '_'); - if (isValidID(spacesToUnderscores)) { + if (isValidId(spacesToUnderscores)) { builder.addFix( SuggestedFix.replace( tree.getArguments().get(0), String.format("\"%s\"", spacesToUnderscores))); @@ -89,7 +89,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return builder.build(); } - private static boolean isValidID(String value) { + private static boolean isValidId(String value) { if (AVAILABLE_IDS.contains(value)) { // Value is in TimeZone.getAvailableIDs(), so it's supported on this JVM. return true; diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/InvalidZoneId.java b/core/src/main/java/com/google/errorprone/bugpatterns/InvalidZoneId.java index 543d6c7360b..f05b8f20a21 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/InvalidZoneId.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/InvalidZoneId.java @@ -57,7 +57,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState // Value isn't a compile-time constant, so we can't know if it's unsafe. return Description.NO_MATCH; } - if (isValidID(value)) { + if (isValidId(value)) { return Description.NO_MATCH; } @@ -65,7 +65,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState } // https://docs.oracle.com/javase/8/docs/api/java/time/ZoneId.html#of-java.lang.String- - private static boolean isValidID(String value) { + private static boolean isValidId(String value) { try { ZoneId.of(value); } catch (DateTimeException e) { // ZoneRulesException is subclass of DateTimeException diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MemberName.java b/core/src/main/java/com/google/errorprone/bugpatterns/MemberName.java index 0184aa9184c..d8c6c00942e 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/MemberName.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MemberName.java @@ -36,6 +36,7 @@ import static javax.lang.model.element.ElementKind.LOCAL_VARIABLE; import static javax.lang.model.element.ElementKind.RESOURCE_VARIABLE; +import com.google.common.base.Ascii; import com.google.common.base.CaseFormat; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableSet; @@ -48,6 +49,7 @@ import com.google.errorprone.suppliers.Supplier; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.VariableTree; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; @@ -87,6 +89,10 @@ public final class MemberName extends BugChecker implements MethodTreeMatcher, V "Static variables should be named in UPPER_SNAKE_CASE if deeply immutable or lowerCamelCase" + " if not."; + private static final String INITIALISM_DETAIL = + ", with acronyms treated as words" + + " (https://google.github.io/styleguide/javaguide.html#s5.3-camel-case)"; + @Override public Description matchMethod(MethodTree tree, VisitorState state) { MethodSymbol symbol = getSymbol(tree); @@ -121,16 +127,19 @@ public Description matchMethod(MethodTree tree, VisitorState state) { if (isConformant(symbol, name)) { return NO_MATCH; } - String suggested = suggestedRename(symbol, name); - return suggested.equals(name) || !canBeRemoved(symbol, state) - ? buildDescription(tree) - .setMessage( - String.format( - "Methods and non-static variables should be named in lowerCamelCase; did you" - + " mean '%s'?", - suggested)) - .build() - : describeMatch(tree, renameMethodWithInvocations(tree, suggested, state)); + String renamed = suggestedRename(symbol, name); + String suggested = fixInitialisms(renamed); + boolean fixable = !suggested.equals(name) && canBeRemoved(symbol, state); + String diagnostic = + "Methods and non-static variables should be named in lowerCamelCase" + + (suggested.equals(renamed) ? "" : INITIALISM_DETAIL); + return buildDescription(tree) + .setMessage( + fixable + ? diagnostic + : diagnostic + String.format("; did you" + " mean '%s'?", suggested)) + .addFix(fixable ? renameMethodWithInvocations(tree, suggested, state) : emptyFix()) + .build(); } private static boolean hasTestAnnotation(MethodSymbol symbol) { @@ -142,7 +151,9 @@ private static boolean hasTestAnnotation(MethodSymbol symbol) { public Description matchVariable(VariableTree tree, VisitorState state) { VarSymbol symbol = getSymbol(tree); String name = tree.getName().toString(); - if (symbol.owner instanceof MethodSymbol && symbol.getKind().equals(ElementKind.PARAMETER)) { + if (symbol.owner instanceof MethodSymbol + && symbol.getKind() == ElementKind.PARAMETER + && state.getPath().getParentPath().getLeaf().getKind() != Kind.LAMBDA_EXPRESSION) { var methodSymbol = (MethodSymbol) symbol.owner; int index = methodSymbol.getParameters().indexOf(symbol); var maybeSuper = ASTHelpers.streamSuperMethods(methodSymbol, state.getTypes()).findFirst(); @@ -158,24 +169,35 @@ public Description matchVariable(VariableTree tree, VisitorState state) { } } // Try to avoid dual-matching with ConstantCaseForConstants. - if (UPPER_UNDERSCORE_PATTERN.matcher(name).matches() && !symbol.isStatic()) { + if (isConformantStaticVariableName(name) && !symbol.isStatic()) { return NO_MATCH; } if (isConformant(symbol, name)) { return NO_MATCH; } - String suggested = suggestedRename(symbol, name); + if (EXEMPTED_VARIABLE_NAMES.contains(name)) { + return NO_MATCH; + } + String renamed = suggestedRename(symbol, name); + String suggested = fixInitialisms(renamed); + boolean fixable = !suggested.equals(name) && canBeRenamed(symbol); + String diagnostic = + (isStaticVariable(symbol) ? STATIC_VARIABLE_FINDING : message()) + + (suggested.equals(renamed) ? "" : INITIALISM_DETAIL); return buildDescription(tree) - .addFix( - suggested.equals(name) || !canBeRenamed(symbol) - ? emptyFix() - : renameVariable(tree, suggested, state)) - .setMessage(isStaticVariable(symbol) ? STATIC_VARIABLE_FINDING : message()) + .setMessage( + fixable + ? diagnostic + : diagnostic + String.format("; did you" + " mean '%s'?", suggested)) + .addFix(fixable ? renameVariable(tree, suggested, state) : emptyFix()) .build(); } + private static final ImmutableSet EXEMPTED_VARIABLE_NAMES = + ImmutableSet.of("serialVersionUID"); + private static String suggestedRename(Symbol symbol, String name) { - if (!isStaticVariable(symbol) && UPPER_UNDERSCORE_PATTERN.matcher(name).matches()) { + if (!isStaticVariable(symbol) && isConformantStaticVariableName(name)) { return CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_CAMEL, name); } if (LOWER_UNDERSCORE_PATTERN.matcher(name).matches()) { @@ -198,17 +220,38 @@ private static boolean canBeRenamed(Symbol symbol) { ImmutableSet.of(LOCAL_VARIABLE, RESOURCE_VARIABLE, EXCEPTION_PARAMETER); private static boolean isConformant(Symbol symbol, String name) { - if (isStaticVariable(symbol) && UPPER_UNDERSCORE_PATTERN.matcher(name).matches()) { + if (isStaticVariable(symbol) && isConformantStaticVariableName(name)) { return true; } - return !name.contains("_") && !isUpperCase(name.charAt(0)); + return isConformantLowerCamelName(name); + } + + private static boolean isConformantStaticVariableName(String name) { + return UPPER_UNDERSCORE_PATTERN.matcher(name).matches(); + } + + private static boolean isConformantLowerCamelName(String name) { + return !name.contains("_") + && !isUpperCase(name.charAt(0)) + && !PROBABLE_INITIALISM.matcher(name).find(); } private static boolean isStaticVariable(Symbol symbol) { return symbol instanceof VarSymbol && isStatic(symbol); } + private static String fixInitialisms(String input) { + return PROBABLE_INITIALISM.matcher(input).replaceAll(r -> titleCase(r.group(1)) + r.group(2)); + } + + private static String titleCase(String input) { + var lower = Ascii.toLowerCase(input); + return Ascii.toUpperCase(lower.charAt(0)) + lower.substring(1); + } + private static final Pattern LOWER_UNDERSCORE_PATTERN = Pattern.compile("[a-z0-9_]+"); private static final Pattern UPPER_UNDERSCORE_PATTERN = Pattern.compile("[A-Z0-9_]+"); + private static final java.util.regex.Pattern PROBABLE_INITIALISM = + java.util.regex.Pattern.compile("([A-Z]{2,})([A-Z][^A-Z]|$)"); private static final Splitter UNDERSCORE_SPLITTER = Splitter.on('_'); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MemoizeConstantVisitorStateLookups.java b/core/src/main/java/com/google/errorprone/bugpatterns/MemoizeConstantVisitorStateLookups.java index 80aa91d5cee..459b6b4d09e 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/MemoizeConstantVisitorStateLookups.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MemoizeConstantVisitorStateLookups.java @@ -137,10 +137,13 @@ private Description fixLookupsInClass(ClassTree tree, VisitorState state) { private static final class CallSite { /** The method on VisitorState being called. */ final Name method; + /** The compile-time constant value being passed to that method. */ final String argumentValue; + /** The actual expression with that value: a string literal, or a constant with such a value. */ final ExpressionTree argumentExpression; + /** The entire invocation of the VisitorState method. */ final MethodInvocationTree entireTree; diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MissingRefasterAnnotation.java b/core/src/main/java/com/google/errorprone/bugpatterns/MissingRefasterAnnotation.java new file mode 100644 index 00000000000..244b847d71c --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MissingRefasterAnnotation.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.BugPattern.StandardTags.LIKELY_ERROR; +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.anyOf; +import static com.google.errorprone.matchers.Matchers.isType; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.MultiMatcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = "The Refaster template contains a method without any Refaster annotations", + severity = WARNING, + tags = LIKELY_ERROR) +public final class MissingRefasterAnnotation extends BugChecker implements ClassTreeMatcher { + private static final MultiMatcher HAS_REFASTER_ANNOTATION = + annotations( + AT_LEAST_ONE, + anyOf( + isType("com.google.errorprone.refaster.annotation.Placeholder"), + isType("com.google.errorprone.refaster.annotation.BeforeTemplate"), + isType("com.google.errorprone.refaster.annotation.AfterTemplate"))); + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + long methodTypeCount = + tree.getMembers().stream() + .filter(member -> member.getKind() == Tree.Kind.METHOD) + .map(MethodTree.class::cast) + .filter(method -> !ASTHelpers.isGeneratedConstructor(method)) + .map(method -> HAS_REFASTER_ANNOTATION.matches(method, state)) + .distinct() + .count(); + + return methodTypeCount < 2 ? Description.NO_MATCH : describeMatch(tree); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MutableGuiceModule.java b/core/src/main/java/com/google/errorprone/bugpatterns/MutableGuiceModule.java new file mode 100644 index 00000000000..afcb60cbbdf --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MutableGuiceModule.java @@ -0,0 +1,97 @@ +/* + * Copyright 2023 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.fixes.SuggestedFixes.prettyType; +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 com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; +import com.google.errorprone.bugpatterns.threadsafety.WellKnownMutability; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.suppliers.Supplier; +import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.VarSymbol; +import com.sun.tools.javac.code.Type; +import javax.inject.Inject; +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.Modifier; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern(summary = "Fields in Guice modules should be final", severity = WARNING) +public class MutableGuiceModule extends BugChecker implements VariableTreeMatcher { + + private final WellKnownMutability wellKnownMutability; + + @Inject + MutableGuiceModule(WellKnownMutability wellKnownMutability) { + this.wellKnownMutability = wellKnownMutability; + } + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + if (state.errorProneOptions().isTestOnlyTarget()) { + return NO_MATCH; + } + VarSymbol sym = getSymbol(tree); + if (sym == null) { + return NO_MATCH; + } + if (!sym.getKind().equals(ElementKind.FIELD)) { + return NO_MATCH; + } + Symbol abstractModule = ABSTRACT_MODULE.get(state); + if (abstractModule == null) { + return NO_MATCH; + } + if (!enclosingClass(sym).isSubClass(abstractModule, state.getTypes())) { + return NO_MATCH; + } + if (sym.isStatic()) { + return NO_MATCH; + } + if (!tree.getModifiers().getFlags().contains(Modifier.FINAL)) { + Description.Builder description = buildDescription(tree); + SuggestedFixes.addModifiers(tree, state, Modifier.FINAL) + .filter(f -> SuggestedFixes.compilesWithFix(f, state)) + .ifPresent(description::addFix); + state.reportMatch(description.build()); + } + Type type = getType(tree); + String nameStr = type.tsym.flatName().toString(); + if (wellKnownMutability.getKnownMutableClasses().contains(nameStr)) { + state.reportMatch( + buildDescription(tree) + .setMessage( + String.format( + "Fields in Guice modules should be immutable, but %s is mutable", + prettyType(type, state))) + .build()); + } + return NO_MATCH; + } + + private static final Supplier ABSTRACT_MODULE = + VisitorState.memoize(state -> state.getSymbolFromString("com.google.inject.AbstractModule")); +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NamedLikeContextualKeyword.java b/core/src/main/java/com/google/errorprone/bugpatterns/NamedLikeContextualKeyword.java new file mode 100644 index 00000000000..18b3249bf30 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NamedLikeContextualKeyword.java @@ -0,0 +1,171 @@ +/* + * 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.fixes.SuggestedFixes.qualifyType; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.methodIsConstructor; +import static com.google.errorprone.matchers.Matchers.methodIsNamed; +import static com.google.errorprone.matchers.Matchers.not; +import static com.google.errorprone.util.ASTHelpers.findPathFromEnclosingNodeToTopLevel; +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; +import com.google.errorprone.ErrorProneFlags; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +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.ClassTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.util.TreePath; +import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import java.util.Collections; +import javax.inject.Inject; + +/** + * Warns on classes or methods being named similarly to contextual keywords, or invoking such + * methods. + */ +@BugPattern( + severity = WARNING, + summary = + "Avoid naming of classes and methods that is similar to contextual keywords. When invoking" + + " such a method, qualify it.") +public final class NamedLikeContextualKeyword extends BugChecker + implements ClassTreeMatcher, MethodTreeMatcher, MethodInvocationTreeMatcher { + + // Refer to JLS 19 §3.9. + // Note that "non-sealed" is not a valid class name + private static final ImmutableSet DISALLOWED_CLASS_NAMES = + ImmutableSet.of( + "exports", + "opens", + "requires", + "uses", + "module", + "permits", + "sealed", + "var", + "provides", + "to", + "with", + "open", + "record", + "transitive", + "yield"); + private static final Matcher DISALLOWED_METHOD_NAME_MATCHER = + allOf(not(methodIsConstructor()), methodIsNamed("yield")); + private static final ImmutableSet AUTO_PROCESSORS = + ImmutableSet.of( + "com.google.auto.value.processor.AutoValueProcessor", + "com.google.auto.value.processor.AutoOneOfProcessor"); + + private final boolean enableMethodNames; + private final boolean enableClassNames; + + @Inject + NamedLikeContextualKeyword(ErrorProneFlags flags) { + this.enableMethodNames = + flags.getBoolean("NamedLikeContextualKeyword:EnableMethodNames").orElse(false); + this.enableClassNames = + flags.getBoolean("NamedLikeContextualKeyword:EnableClassNames").orElse(false); + } + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + if (!this.enableMethodNames) { + return NO_MATCH; + } + + MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree); + + // Don't alert if an @Auto... class (safe since reference always qualified). + if (isInGeneratedAutoCode(state)) { + return NO_MATCH; + } + + // Don't alert if method is an override (this includes interfaces) + if (!streamSuperMethods(methodSymbol, state.getTypes()).findAny().isPresent() + && DISALLOWED_METHOD_NAME_MATCHER.matches(tree, state)) { + return describeMatch(tree); + } + + return NO_MATCH; + } + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + if (!this.enableClassNames) { + return NO_MATCH; + } + + if (DISALLOWED_CLASS_NAMES.contains(tree.getSimpleName().toString())) { + return describeMatch(tree); + } + + return NO_MATCH; + } + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + ExpressionTree select = tree.getMethodSelect(); + if (!(select instanceof IdentifierTree)) { + return NO_MATCH; + } + if (!((IdentifierTree) select).getName().contentEquals("yield")) { + return NO_MATCH; + } + SuggestedFix.Builder fix = SuggestedFix.builder(); + String qualifier = getQualifier(state, getSymbol(tree), fix); + return describeMatch(tree, fix.prefixWith(select, qualifier + ".").build()); + } + + private static String getQualifier( + VisitorState state, MethodSymbol sym, SuggestedFix.Builder fix) { + if (sym.isStatic()) { + return qualifyType(state, fix, sym.owner.enclClass()); + } + TreePath path = findPathFromEnclosingNodeToTopLevel(state.getPath(), ClassTree.class); + if (sym.isMemberOf(getSymbol((ClassTree) path.getLeaf()), state.getTypes())) { + return "this"; + } + while (true) { + path = findPathFromEnclosingNodeToTopLevel(path, ClassTree.class); + ClassSymbol enclosingClass = getSymbol((ClassTree) path.getLeaf()); + if (sym.isMemberOf(enclosingClass, state.getTypes())) { + return qualifyType(state, fix, enclosingClass) + ".this"; + } + } + } + + private static boolean isInGeneratedAutoCode(VisitorState state) { + return !Collections.disjoint(ASTHelpers.getGeneratedBy(state), AUTO_PROCESSORS); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NestedInstanceOfConditions.java b/core/src/main/java/com/google/errorprone/bugpatterns/NestedInstanceOfConditions.java index d6a0df9c218..8aa398165e2 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/NestedInstanceOfConditions.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NestedInstanceOfConditions.java @@ -126,7 +126,9 @@ public boolean matches(Tree tree, VisitorState state) { types.erasure(ASTHelpers.getType(typeTree))); boolean isSameExpression = - instanceOfTree.getExpression().toString().equals(expressionTree.toString()); + state + .getSourceForNode(instanceOfTree.getExpression()) + .equals(state.getSourceForNode(expressionTree)); return isSameExpression && !isCastable; } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NonApiType.java b/core/src/main/java/com/google/errorprone/bugpatterns/NonApiType.java new file mode 100644 index 00000000000..571d8d52834 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NonApiType.java @@ -0,0 +1,309 @@ +/* + * Copyright 2023 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.predicates.TypePredicates.anyOf; +import static com.google.errorprone.predicates.TypePredicates.anything; +import static com.google.errorprone.predicates.TypePredicates.isDescendantOf; +import static com.google.errorprone.predicates.TypePredicates.isExactType; +import static com.google.errorprone.predicates.TypePredicates.not; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.ASTHelpers.isSameType; +import static com.google.errorprone.util.ASTHelpers.methodIsPublicAndNotAnOverride; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.predicates.TypePredicate; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Type; +import java.util.ArrayDeque; +import java.util.Deque; + +/** Flags instances of non-API types from being accepted or returned in APIs. */ +@BugPattern( + summary = "Certain types should not be passed across API boundaries.", + // something about reducing build visibility + severity = WARNING) +public final class NonApiType extends BugChecker implements MethodTreeMatcher { + // TODO(kak): consider creating an annotation (e.g., `@NonApiType` or `@NotForPublicApi`) that + // users could apply to their own types. + + private static final String FLOGGER_LINK = ""; + private static final String TYPE_GENERALITY_LINK = ""; + private static final String INTERFACES_NOT_IMPLS_LINK = ""; + private static final String PRIMITIVE_ARRAYS_LINK = ""; + private static final String PROTO_TIME_SERIALIZATION_LINK = ""; + private static final String ITERATOR_LINK = ""; + private static final String STREAM_LINK = ""; + private static final String OPTIONAL_AS_PARAM_LINK = ""; + private static final String PREFER_JDK_OPTIONAL_LINK = ""; + + private static final TypePredicate NON_GRAPH_WRAPPER = + not(isDescendantOf("com.google.apps.framework.producers.GraphWrapper")); + + private static final ImmutableSet NON_API_TYPES = + ImmutableSet.of( + // primitive arrays + withPublicVisibility( + anyOf( + (t, s) -> isSameType(t, s.getTypes().makeArrayType(s.getSymtab().intType), s), + (t, s) -> isSameType(t, makeArrayType("java.lang.Integer", s), s)), + "Prefer an ImmutableIntArray instead. " + PRIMITIVE_ARRAYS_LINK, + ApiElementType.ANY), + withPublicVisibility( + anyOf( + (t, s) -> isSameType(t, s.getTypes().makeArrayType(s.getSymtab().doubleType), s), + (t, s) -> isSameType(t, makeArrayType("java.lang.Double", s), s)), + "Prefer an ImmutableDoubleArray instead. " + PRIMITIVE_ARRAYS_LINK, + ApiElementType.ANY), + withPublicVisibility( + anyOf( + (t, s) -> isSameType(t, s.getTypes().makeArrayType(s.getSymtab().longType), s), + (t, s) -> isSameType(t, makeArrayType("java.lang.Long", s), s)), + "Prefer an ImmutableLongArray instead. " + PRIMITIVE_ARRAYS_LINK, + ApiElementType.ANY), + + // Optionals + withPublicVisibility( + isExactType("java.util.Optional"), + "Avoid Optional parameters. " + OPTIONAL_AS_PARAM_LINK, + ApiElementType.PARAMETER), + withPublicVisibility( + isExactType("com.google.common.base.Optional"), + "Prefer a java.util.Optional instead. " + PREFER_JDK_OPTIONAL_LINK, + ApiElementType.ANY), + + // ImmutableFoo as params + withPublicVisibility( + isExactType("com.google.common.collect.ImmutableCollection"), + NON_GRAPH_WRAPPER, + "Consider accepting a java.util.Collection or Iterable instead. " + + TYPE_GENERALITY_LINK, + ApiElementType.PARAMETER), + withPublicVisibility( + isExactType("com.google.common.collect.ImmutableList"), + NON_GRAPH_WRAPPER, + "Consider accepting a java.util.List or Iterable instead. " + TYPE_GENERALITY_LINK, + ApiElementType.PARAMETER), + withPublicVisibility( + isExactType("com.google.common.collect.ImmutableSet"), + NON_GRAPH_WRAPPER, + "Consider accepting a java.util.Set or Iterable instead. " + TYPE_GENERALITY_LINK, + ApiElementType.PARAMETER), + withPublicVisibility( + isExactType("com.google.common.collect.ImmutableMap"), + NON_GRAPH_WRAPPER, + "Consider accepting a java.util.Map instead. " + TYPE_GENERALITY_LINK, + ApiElementType.PARAMETER), + + // collection implementation classes + withAnyVisibility( + anyOf(isExactType("java.util.ArrayList"), isExactType("java.util.LinkedList")), + "Prefer a java.util.List instead. " + INTERFACES_NOT_IMPLS_LINK, + ApiElementType.ANY), + withAnyVisibility( + anyOf( + isExactType("java.util.HashSet"), + isExactType("java.util.LinkedHashSet"), + isExactType("java.util.TreeSet")), + "Prefer a java.util.Set instead. " + INTERFACES_NOT_IMPLS_LINK, + ApiElementType.ANY), + withAnyVisibility( + anyOf( + isExactType("java.util.HashMap"), + isExactType("java.util.LinkedHashMap"), + isExactType("java.util.TreeMap")), + "Prefer a java.util.Map instead. " + INTERFACES_NOT_IMPLS_LINK, + ApiElementType.ANY), + + // Iterators + withPublicVisibility( + isDescendantOf("java.util.Iterator"), + "Prefer returning a Stream (or collecting to an ImmutableList/ImmutableSet) instead. " + + ITERATOR_LINK, + ApiElementType.RETURN_TYPE), + // TODO(b/279464660): consider also warning on an Iterator as a ApiElementType.PARAMETER + + // Streams + withPublicVisibility( + isDescendantOf("java.util.stream.Stream"), + "Prefer accepting an Iterable or Collection instead. " + STREAM_LINK, + ApiElementType.PARAMETER), + + // ProtoTime + withPublicVisibility( + isExactType("com.google.protobuf.Duration"), + "Prefer a java.time.Duration instead. " + PROTO_TIME_SERIALIZATION_LINK, + ApiElementType.ANY), + withPublicVisibility( + isExactType("com.google.protobuf.Timestamp"), + "Prefer a java.time.Instant instead. " + PROTO_TIME_SERIALIZATION_LINK, + ApiElementType.ANY), + withPublicVisibility( + isExactType("com.google.type.Date"), + "Prefer a java.time.LocalDate instead. " + PROTO_TIME_SERIALIZATION_LINK, + ApiElementType.ANY), + withPublicVisibility( + isExactType("com.google.type.DateTime"), + "Prefer a java.time.LocalDateTime instead. " + PROTO_TIME_SERIALIZATION_LINK, + ApiElementType.ANY), + withPublicVisibility( + isExactType("com.google.type.DayOfWeek"), + "Prefer a java.time.DayOfWeek instead. " + PROTO_TIME_SERIALIZATION_LINK, + ApiElementType.ANY), + withPublicVisibility( + isExactType("com.google.type.Month"), + "Prefer a java.time.Month instead. " + PROTO_TIME_SERIALIZATION_LINK, + ApiElementType.ANY), + withPublicVisibility( + isExactType("com.google.type.TimeOfDay"), + "Prefer a java.time.LocalTime instead. " + PROTO_TIME_SERIALIZATION_LINK, + ApiElementType.ANY), + withPublicVisibility( + isExactType("com.google.type.TimeZone"), + "Prefer a java.time.ZoneId instead. " + PROTO_TIME_SERIALIZATION_LINK, + ApiElementType.ANY), + // TODO(kak): consider com.google.type.Interval -> Range + + // Flogger + withAnyVisibility( + anyOf( + isDescendantOf("com.google.common.flogger.FluentLogger"), + isDescendantOf("com.google.common.flogger.GoogleLogger"), + isDescendantOf("com.google.common.flogger.android.AndroidFluentLogger")), + "There is no advantage to passing around a logger rather than declaring one in the" + + " class that needs it. " + + FLOGGER_LINK, + ApiElementType.ANY)); + + private static Type makeArrayType(String typeName, VisitorState state) { + return state.getTypes().makeArrayType(state.getTypeFromString(typeName)); + } + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + Type enclosingType = getSymbol(tree).owner.type; + + boolean isPublicApi = + methodIsPublicAndNotAnOverride(getSymbol(tree), state) + && state.errorProneOptions().isPubliclyVisibleTarget(); + + for (Tree parameter : tree.getParameters()) { + checkType(parameter, ApiElementType.PARAMETER, isPublicApi, enclosingType, state); + } + checkType(tree.getReturnType(), ApiElementType.RETURN_TYPE, isPublicApi, enclosingType, state); + + // the accumulated matches (if any) are reported via state.reportMatch(...) + return NO_MATCH; + } + + private void checkType( + Tree tree, + ApiElementType elementType, + boolean isPublicApi, + Type enclosingType, + VisitorState state) { + if (isSuppressed(tree, state)) { + return; + } + Type type = getType(tree); + if (type == null) { + return; + } + for (TypeToCheck typeToCheck : NON_API_TYPES) { + if (typeToCheck.matches(type, enclosingType, state)) { + if (typeToCheck.elementType() == ApiElementType.ANY + || typeToCheck.elementType() == elementType) { + if (isPublicApi || typeToCheck.visibility() == ApiVisibility.ANY) { + state.reportMatch( + buildDescription(tree).setMessage(typeToCheck.failureMessage()).build()); + } + } + } + } + } + + enum ApiElementType { + PARAMETER, + RETURN_TYPE, + ANY + } + + enum ApiVisibility { + PUBLIC, + ANY + } + + private static TypeToCheck withPublicVisibility( + TypePredicate typePredicate, String failureMessage, ApiElementType elementType) { + return withPublicVisibility(typePredicate, anything(), failureMessage, elementType); + } + + private static TypeToCheck withPublicVisibility( + TypePredicate typePredicate, + TypePredicate enclosingTypePredicate, + String failureMessage, + ApiElementType elementType) { + return new AutoValue_NonApiType_TypeToCheck( + typePredicate, enclosingTypePredicate, failureMessage, ApiVisibility.PUBLIC, elementType); + } + + private static TypeToCheck withAnyVisibility( + TypePredicate typePredicate, String failureMessage, ApiElementType elementType) { + return new AutoValue_NonApiType_TypeToCheck( + typePredicate, anything(), failureMessage, ApiVisibility.ANY, elementType); + } + + @AutoValue + abstract static class TypeToCheck { + + final boolean matches(Type type, Type enclosingType, VisitorState state) { + // only fire this check inside certain subtypes + if (enclosingTypePredicate().apply(enclosingType, state)) { + Deque types = new ArrayDeque<>(); + types.add(type); + while (!types.isEmpty()) { + Type head = types.poll(); + if (typePredicate().apply(head, state)) { + return true; + } + types.addAll(head.getTypeArguments()); + } + } + // TODO(kak): do we want to check var-args as well? + return false; + } + + abstract TypePredicate typePredicate(); + + abstract TypePredicate enclosingTypePredicate(); + + abstract String failureMessage(); + + abstract ApiVisibility visibility(); + + abstract ApiElementType elementType(); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NullableOptional.java b/core/src/main/java/com/google/errorprone/bugpatterns/NullableOptional.java new file mode 100644 index 00000000000..99f949c6be7 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NullableOptional.java @@ -0,0 +1,81 @@ +/* + * Copyright 2023 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 com.google.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.predicates.TypePredicate; +import com.google.errorprone.predicates.TypePredicates; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.ModifiersTree; +import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Type; + +/** + * A Checker that catches {@link java.util.Optional}/{@link com.google.common.base.Optional} with + * {@code Nullable} annotation. + */ +@BugPattern( + summary = + "Using an Optional variable which is expected to possibly be null is discouraged. It is" + + " best to indicate the absence of the value by assigning it an empty optional.", + severity = SeverityLevel.WARNING) +public final class NullableOptional extends BugChecker + implements MethodTreeMatcher, VariableTreeMatcher { + private static final TypePredicate IS_OPTIONAL_TYPE = + TypePredicates.isExactTypeAny( + ImmutableSet.of( + java.util.Optional.class.getCanonicalName(), + com.google.common.base.Optional.class.getCanonicalName())); + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + if (hasNullableAnnotation(tree.getModifiers()) + && isOptional(ASTHelpers.getType(tree.getReturnType()), state)) { + return describeMatch(tree); + } + return Description.NO_MATCH; + } + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + if (hasNullableAnnotation(tree.getModifiers()) && isOptional(ASTHelpers.getType(tree), state)) { + return describeMatch(tree); + } + return Description.NO_MATCH; + } + + /** Check if the input ModifiersTree has any kind of "Nullable" annotation. */ + private static boolean hasNullableAnnotation(ModifiersTree modifiersTree) { + return ASTHelpers.getAnnotationWithSimpleName(modifiersTree.getAnnotations(), "Nullable") + != null; + } + + /** + * Check if the input Type is either {@link java.util.Optional} or{@link + * com.google.common.base.Optional}. + */ + private static boolean isOptional(Type type, VisitorState state) { + return IS_OPTIONAL_TYPE.apply(type, state); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesChecker.java new file mode 100644 index 00000000000..c275ef7695a --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesChecker.java @@ -0,0 +1,94 @@ +/* + * Copyright 2023 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.ImmutableBiMap.toImmutableBiMap; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.util.ASTHelpers.findSuperMethods; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static java.util.stream.Collectors.joining; +import static java.util.stream.IntStream.range; + +import com.google.common.collect.ImmutableBiMap; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.MethodTree; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import java.util.Optional; +import javax.lang.model.element.Name; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = "Arguments of overriding method are inconsistent with overridden method.", + severity = WARNING) +public class OverridingMethodInconsistentArgumentNamesChecker extends BugChecker + implements MethodTreeMatcher { + + @Override + public Description matchMethod(MethodTree methodTree, VisitorState state) { + MethodSymbol methodSymbol = getSymbol(methodTree); + Optional superMethod = + findSuperMethods(methodSymbol, state.getTypes()).stream().findFirst(); + if (superMethod.isEmpty() || methodSymbol.isVarArgs() || superMethod.get().isVarArgs()) { + return Description.NO_MATCH; + } + + ImmutableBiMap params = getParams(methodSymbol); + ImmutableBiMap superParams = getParams(superMethod.get()); + + for (Name param : params.keySet()) { + int position = params.get(param); + if (!superParams.containsKey(param) || position == superParams.get(param)) { + continue; + } + Name samePositionSuperParam = superParams.inverse().get(position); + if (params.containsKey(samePositionSuperParam)) { + return buildDescription(methodTree).setMessage(getDescription(superMethod.get())).build(); + } + } + + return Description.NO_MATCH; + } + + private static ImmutableBiMap getParams(MethodSymbol methodSymbol) { + return range(0, methodSymbol.getParameters().size()) + .boxed() + .collect(toImmutableBiMap(i -> methodSymbol.getParameters().get(i).name, i -> i)); + } + + /** + * Provides a violation description with suggested parameter ordering. + * + *

We're not returning a suggested fix as re-ordering is not safe and might break the code. + */ + public String getDescription(MethodSymbol methodSymbol) { + return "The parameters of this method are inconsistent with the overridden method." + + " A consistent order would be: " + + getSuggestedSignature(methodSymbol); + } + + private static String getSuggestedSignature(MethodSymbol methodSymbol) { + return String.format( + "%s(%s)", methodSymbol.getSimpleName(), getSuggestedParameters(methodSymbol)); + } + + private static String getSuggestedParameters(MethodSymbol methodSymbol) { + return methodSymbol.getParameters().stream().map(p -> p.name).collect(joining(", ")); + } +} 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 5b625b0d5bc..f04fafa0b72 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/RestrictedApiChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/RestrictedApiChecker.java @@ -232,14 +232,19 @@ private Description checkRestriction( boolean allow = Matchers.enclosingNode(shouldAllow(attribute)).matches(where, state); if (warn && allow) { // TODO(bangert): Clarify this message if possible. - return buildDescription(where) - .setMessage( - "The Restricted API (" - + restriction.explanation() - + ") call here is both allowlisted-as-warning and " - + "silently allowlisted. " - + "Please remove one of the conflicting suppression annotations.") - .build(); + var descriptionBuilder = + buildDescription(where) + .setMessage( + "The Restricted API (" + + restriction.explanation() + + ") call here is both allowlisted-as-warning and " + + "silently allowlisted. " + + "Please remove one of the conflicting suppression annotations."); + + if (!restriction.link().isEmpty()) { + descriptionBuilder.setLinkUrl(restriction.link()); + } + return descriptionBuilder.build(); } if (allow) { return NO_MATCH; diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java index 63e2b0f620b..56f330f5d4f 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java @@ -116,6 +116,7 @@ private static enum CaseFallThru { MAYBE_FALLS_THRU, DEFINITELY_DOES_FALL_THRU }; + // Tri-state to represent whether cases within a single switch statement meet an (unspecified) // qualification predicate static enum CaseQualifications { @@ -1084,10 +1085,13 @@ static AnalysisResult of( abstract static class AssignmentSwitchAnalysisResult { // Whether the statement switch can be converted to an assignment switch abstract boolean canConvertToAssignmentSwitch(); + // Target of the assignment switch, if any abstract Optional assignmentTargetOptional(); + // Kind of assignment made by the assignment switch, if any abstract Optional assignmentKindOptional(); + // Java source code of the assignment switch's operator, e.g. "+=" abstract Optional assignmentSourceCodeOptional(); @@ -1108,10 +1112,13 @@ static AssignmentSwitchAnalysisResult of( abstract static class AssignmentSwitchAnalysisState { // Overall qualification of the switch statement for conversion to an assignment switch abstract CaseQualifications assignmentSwitchCaseQualifications(); + // Target of the first assignment seen, if any abstract Optional assignmentTargetOptional(); + // Kind of the first assignment seen, if any abstract Optional assignmentExpressionKindOptional(); + // ExpressionTree of the first assignment seen, if any abstract Optional assignmentTreeOptional(); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StringCaseLocaleUsage.java b/core/src/main/java/com/google/errorprone/bugpatterns/StringCaseLocaleUsage.java index c5721e3917b..079454f8b73 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/StringCaseLocaleUsage.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StringCaseLocaleUsage.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.FRAGILE_CODE; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; import static com.google.errorprone.util.ASTHelpers.getReceiver; import static com.google.errorprone.util.ASTHelpers.getSymbol; @@ -46,8 +45,7 @@ "Specify a `Locale` when calling `String#to{Lower,Upper}Case`. (Note: there are multiple" + " suggested fixes; the third may be most appropriate if you're dealing with ASCII" + " Strings.)", - severity = WARNING, - tags = FRAGILE_CODE) + severity = WARNING) public final class StringCaseLocaleUsage extends BugChecker implements MethodInvocationTreeMatcher { private static final Matcher DEFAULT_LOCALE_CASE_CONVERSION = instanceMethod() diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TreeToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/TreeToString.java index 5b19dc735af..786e9d96754 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/TreeToString.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/TreeToString.java @@ -17,13 +17,13 @@ package com.google.errorprone.bugpatterns; import static com.google.common.collect.Iterables.getOnlyElement; +import static com.google.common.collect.Streams.stream; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.matchers.Matchers.instanceMethod; import static com.google.errorprone.util.ASTHelpers.getReceiver; import static com.google.errorprone.util.ASTHelpers.isSubtype; import com.google.errorprone.BugPattern; -import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; import com.google.errorprone.fixes.Fix; import com.google.errorprone.fixes.SuggestedFix; @@ -66,12 +66,12 @@ public class TreeToString extends AbstractToString { .named("Literal") .withParameters("java.lang.Object"); - private final boolean transitiveEnclosingBugchecker; - @Inject - TreeToString(ErrorProneFlags errorProneFlags) { - this.transitiveEnclosingBugchecker = - errorProneFlags.getBoolean("TreeToString:transitiveEnclosingBugchecker").orElse(true); + TreeToString() {} + + @Override + protected TypePredicate typePredicate() { + return this::treeToStringInBugChecker; } private boolean treeToStringInBugChecker(Type type, VisitorState state) { @@ -79,22 +79,8 @@ private boolean treeToStringInBugChecker(Type type, VisitorState state) { } private boolean enclosingBugChecker(VisitorState state) { - for (Tree tree : state.getPath()) { - if (tree instanceof ClassTree) { - if (IS_BUGCHECKER.matches((ClassTree) tree, state)) { - return true; - } - if (!transitiveEnclosingBugchecker) { - break; - } - } - } - return false; - } - - @Override - protected TypePredicate typePredicate() { - return this::treeToStringInBugChecker; + return stream(state.getPath()) + .anyMatch(t -> t instanceof ClassTree && IS_BUGCHECKER.matches((ClassTree) t, state)); } @Override diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterNaming.java b/core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterNaming.java index e83a674d64d..b260d6936ba 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterNaming.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterNaming.java @@ -222,6 +222,7 @@ private static String suggestedSingleLetter(String id, Tree tree) { return Character.toString(firstLetter); } + // T -> T2 // T2 -> T3 // T -> T4 (if T2 and T3 already exist) 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 3fe13f366fc..be9332b9479 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryLambda.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryLambda.java @@ -174,6 +174,7 @@ public Void visitIdentifier(IdentifierTree node, Void unused) { "com.google.errorprone.matchers", "java.util.function", "java.lang"); + /** * Check if the only methods invoked on the functional interface type are the descriptor method, * e.g. don't rewrite uses of {@link Predicate} in compilation units that call other methods like diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilder.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilder.java new file mode 100644 index 00000000000..57dd19f8846 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilder.java @@ -0,0 +1,194 @@ +/* + * Copyright 2023 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.getOnlyElement; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.method.MethodMatchers.constructor; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.ASTHelpers.isSubtype; +import static com.google.errorprone.util.ASTHelpers.requiresParentheses; +import static com.google.errorprone.util.ASTHelpers.targetType; +import static java.util.stream.Collectors.joining; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.util.ASTHelpers.TargetType; +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.NewClassTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; +import com.sun.source.util.TreePath; +import com.sun.source.util.TreePathScanner; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.util.Position; +import java.util.ArrayList; +import java.util.List; +import javax.lang.model.element.ElementKind; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = + "Prefer string concatenation over explicitly using `StringBuilder#append`, since `+` reads" + + " better and has equivalent or better performance.", + severity = BugPattern.SeverityLevel.WARNING) +public class UnnecessaryStringBuilder extends BugChecker implements NewClassTreeMatcher { + private static final Matcher MATCHER = + constructor().forClass("java.lang.StringBuilder"); + + private static final Matcher APPEND = + instanceMethod().onExactClass("java.lang.StringBuilder").named("append"); + + private static final Matcher TO_STRING = + instanceMethod().onExactClass("java.lang.StringBuilder").named("toString"); + + @Override + public Description matchNewClass(NewClassTree tree, VisitorState state) { + if (!MATCHER.matches(tree, state)) { + return NO_MATCH; + } + List parts = new ArrayList<>(); + switch (tree.getArguments().size()) { + case 0: + break; + case 1: + ExpressionTree argument = getOnlyElement(tree.getArguments()); + if (isSubtype(getType(argument), JAVA_LANG_CHARSEQUENCE.get(state), state)) { + parts.add(argument); + } + break; + default: + return NO_MATCH; + } + TreePath path = state.getPath(); + while (true) { + TreePath parentPath = path.getParentPath(); + if (!(parentPath.getLeaf() instanceof MemberSelectTree)) { + break; + } + TreePath grandParent = parentPath.getParentPath(); + if (!(grandParent.getLeaf() instanceof MethodInvocationTree)) { + break; + } + MethodInvocationTree methodInvocationTree = (MethodInvocationTree) grandParent.getLeaf(); + if (!methodInvocationTree.getMethodSelect().equals(parentPath.getLeaf())) { + break; + } + if (APPEND.matches(methodInvocationTree, state)) { + if (methodInvocationTree.getArguments().size() != 1) { + // an append method that doesn't transliterate to concat + return NO_MATCH; + } + parts.add(getOnlyElement(methodInvocationTree.getArguments())); + path = parentPath.getParentPath(); + } else if (TO_STRING.matches(methodInvocationTree, state)) { + return describeMatch( + methodInvocationTree, + SuggestedFix.replace(methodInvocationTree, replacement(state, parts))); + } else { + // another instance method on StringBuilder + return NO_MATCH; + } + } + ASTHelpers.TargetType target = ASTHelpers.targetType(state.withPath(path)); + if (!isUsedAsStringBuilder(state, target)) { + return describeMatch( + path.getLeaf(), SuggestedFix.replace(path.getLeaf(), replacement(state, parts))); + } + Tree leaf = target.path().getLeaf(); + if (leaf instanceof VariableTree) { + VariableTree variableTree = (VariableTree) leaf; + if (isRewritableVariable(variableTree, state)) { + SuggestedFix.Builder fix = SuggestedFix.builder(); + if (state.getEndPosition(variableTree.getType()) != Position.NOPOS) { + // If the variable is declared with `var`, there's no declaration type to change + fix.replace(variableTree.getType(), "String"); + } + fix.replace(variableTree.getInitializer(), replacement(state, parts)); + return describeMatch(variableTree, fix.build()); + } + } + return NO_MATCH; + } + + /** + * Returns true if the StringBuilder is assigned to a variable, and the type of the variable can + * safely be refactored to be a String. + */ + boolean isRewritableVariable(VariableTree variableTree, VisitorState state) { + Symbol sym = getSymbol(variableTree); + if (!sym.getKind().equals(ElementKind.LOCAL_VARIABLE)) { + return false; + } + boolean[] ok = {true}; + new TreePathScanner() { + @Override + public Void visitIdentifier(IdentifierTree tree, Void unused) { + if (sym.equals(getSymbol(tree))) { + TargetType target = targetType(state.withPath(getCurrentPath())); + if (isUsedAsStringBuilder(state, target)) { + ok[0] = false; + } + } + return super.visitIdentifier(tree, unused); + } + }.scan(state.getPath().getCompilationUnit(), null); + return ok[0]; + } + + private static boolean isUsedAsStringBuilder(VisitorState state, TargetType target) { + if (target.path().getLeaf().getKind().equals(Tree.Kind.MEMBER_REFERENCE)) { + // e.g. sb::append + return true; + } + return ASTHelpers.isSubtype(target.type(), JAVA_LANG_APPENDABLE.get(state), state); + } + + private static String replacement(VisitorState state, List parts) { + if (parts.isEmpty()) { + return "\"\""; + } + return parts.stream() + .map( + x -> { + String source = state.getSourceForNode(x); + if (requiresParentheses(x, state)) { + source = String.format("(%s)", source); + } + return source; + }) + .collect(joining(" + ")); + } + + private static final Supplier JAVA_LANG_APPENDABLE = + VisitorState.memoize(state -> state.getTypeFromString("java.lang.Appendable")); + + private static final Supplier JAVA_LANG_CHARSEQUENCE = + VisitorState.memoize(state -> state.getTypeFromString("java.lang.CharSequence")); +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnqualifiedYield.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnqualifiedYield.java deleted file mode 100644 index 7186eae9d58..00000000000 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnqualifiedYield.java +++ /dev/null @@ -1,77 +0,0 @@ -/* - * 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.fixes.SuggestedFixes.qualifyType; -import static com.google.errorprone.matchers.Description.NO_MATCH; -import static com.google.errorprone.util.ASTHelpers.findPathFromEnclosingNodeToTopLevel; -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.fixes.SuggestedFix; -import com.google.errorprone.matchers.Description; -import com.sun.source.tree.ClassTree; -import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.IdentifierTree; -import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.util.TreePath; -import com.sun.tools.javac.code.Symbol.ClassSymbol; -import com.sun.tools.javac.code.Symbol.MethodSymbol; - -/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ -@BugPattern( - summary = - "In recent versions of Java, 'yield' is a contextual keyword, and calling an unqualified" - + " method with that name is an error.", - severity = WARNING) -public class UnqualifiedYield extends BugChecker implements MethodInvocationTreeMatcher { - - @Override - public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - ExpressionTree select = tree.getMethodSelect(); - if (!(select instanceof IdentifierTree)) { - return NO_MATCH; - } - if (!((IdentifierTree) select).getName().contentEquals("yield")) { - return NO_MATCH; - } - SuggestedFix.Builder fix = SuggestedFix.builder(); - String qualifier = getQualifier(state, getSymbol(tree), fix); - return describeMatch(tree, fix.prefixWith(select, qualifier + ".").build()); - } - - private static String getQualifier( - VisitorState state, MethodSymbol sym, SuggestedFix.Builder fix) { - if (sym.isStatic()) { - return qualifyType(state, fix, sym.owner.enclClass()); - } - TreePath path = findPathFromEnclosingNodeToTopLevel(state.getPath(), ClassTree.class); - if (sym.isMemberOf(getSymbol((ClassTree) path.getLeaf()), state.getTypes())) { - return "this"; - } - while (true) { - path = findPathFromEnclosingNodeToTopLevel(path, ClassTree.class); - ClassSymbol enclosingClass = getSymbol((ClassTree) path.getLeaf()); - if (sym.isMemberOf(enclosingClass, state.getTypes())) { - return qualifyType(state, fix, enclosingClass) + ".this"; - } - } - } -} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java index b95feb47d9a..674a9214b8e 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java @@ -48,7 +48,10 @@ import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.common.collect.Multimaps; +import com.google.common.collect.Range; +import com.google.common.collect.RangeSet; import com.google.common.collect.Streams; +import com.google.common.collect.TreeRangeSet; import com.google.errorprone.BugPattern; import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; @@ -475,9 +478,10 @@ private static ImmutableList buildUnusedParameterFixes( Symbol varSymbol, List usagePaths, VisitorState state) { MethodSymbol methodSymbol = (MethodSymbol) varSymbol.owner; int index = methodSymbol.params.indexOf(varSymbol); - SuggestedFix.Builder fix = SuggestedFix.builder(); + RangeSet deletions = TreeRangeSet.create(); for (TreePath path : usagePaths) { - fix.delete(path.getLeaf()); + deletions.add( + Range.closed(getStartPosition(path.getLeaf()), state.getEndPosition(path.getLeaf()))); } new TreePathScanner() { @Override @@ -507,7 +511,7 @@ private void removeByIndex(List trees) { // TODO(b/118437729): handle bogus source positions in enum declarations return; } - fix.delete(tree); + deletions.add(Range.closed(getStartPosition(tree), state.getEndPosition(tree))); return; } int startPos; @@ -526,9 +530,11 @@ private void removeByIndex(List trees) { // TODO(b/118437729): handle bogus source positions in enum declarations return; } - fix.replace(startPos, endPos, ""); + deletions.add(Range.closed(startPos, endPos)); } }.scan(state.getPath().getCompilationUnit(), null); + SuggestedFix.Builder fix = SuggestedFix.builder(); + deletions.asRanges().forEach(x -> fix.replace(x.lowerEndpoint(), x.upperEndpoint(), "")); return ImmutableList.of(fix.build()); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/android/MislabeledAndroidString.java b/core/src/main/java/com/google/errorprone/bugpatterns/android/MislabeledAndroidString.java index 0db61b44a83..b500f72419a 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/android/MislabeledAndroidString.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/android/MislabeledAndroidString.java @@ -45,12 +45,14 @@ public class MislabeledAndroidString extends BugChecker implements MemberSelectTreeMatcher { private static final String R_STRING_CLASSNAME = "android.R.string"; + /** Maps problematic resources defined in {@value #R_STRING_CLASSNAME} to their replacements. */ @VisibleForTesting static final ImmutableMap MISLEADING = ImmutableMap.of( "yes", "ok", "no", "cancel"); + /** Maps all resources appearing in {@link #MISLEADING} to an assumed meaning. */ @VisibleForTesting static final ImmutableMap ASSUMED_MEANINGS = diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/android/WakelockReleasedDangerously.java b/core/src/main/java/com/google/errorprone/bugpatterns/android/WakelockReleasedDangerously.java index 53de72637ed..143c576e690 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/android/WakelockReleasedDangerously.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/android/WakelockReleasedDangerously.java @@ -60,9 +60,9 @@ @BugPattern( tags = StandardTags.FRAGILE_CODE, summary = - "A wakelock acquired with a timeout may be released by the system before calling" - + " `release`, even after checking `isHeld()`. If so, it will throw a RuntimeException." - + " Please wrap in a try/catch block.", + "On Android versions < P, a wakelock acquired with a timeout may be released by the system" + + " before calling `release`, even after checking `isHeld()`. If so, it will throw a" + + " RuntimeException. Please wrap in a try/catch block.", severity = SeverityLevel.WARNING) public class WakelockReleasedDangerously extends BugChecker implements MethodInvocationTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerPassedAround.java b/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerPassedAround.java deleted file mode 100644 index 8361de2a9a1..00000000000 --- a/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerPassedAround.java +++ /dev/null @@ -1,61 +0,0 @@ -/* - * Copyright 2020 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.flogger; - -import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; -import static com.google.errorprone.matchers.Description.NO_MATCH; -import static com.google.errorprone.predicates.TypePredicates.anyOf; -import static com.google.errorprone.predicates.TypePredicates.isDescendantOf; -import static com.google.errorprone.util.ASTHelpers.getType; - -import com.google.errorprone.BugPattern; -import com.google.errorprone.BugPattern.LinkType; -import com.google.errorprone.VisitorState; -import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; -import com.google.errorprone.matchers.Description; -import com.google.errorprone.predicates.TypePredicate; -import com.sun.source.tree.MethodTree; -import com.sun.source.tree.Tree; -import com.sun.tools.javac.code.Type; - -/** Flags flogger instances being passed around. */ -@BugPattern( - summary = - "There is no advantage to passing around a logger rather than declaring one in the class" - + " that needs it.", - linkType = LinkType.CUSTOM, - link = "https://google.github.io/flogger/best_practice#one-per-class", - severity = WARNING) -public final class FloggerPassedAround extends BugChecker implements MethodTreeMatcher { - private static final TypePredicate LOGGER_TYPE = - anyOf( - isDescendantOf("com.google.common.flogger.FluentLogger"), - isDescendantOf("com.google.common.flogger.FluentLogger"), - isDescendantOf("com.google.common.flogger.android.AndroidFluentLogger")); - - @Override - public Description matchMethod(MethodTree tree, VisitorState state) { - for (Tree parameter : tree.getParameters()) { - Type type = getType(parameter); - if (type != null && LOGGER_TYPE.apply(type, state) && !isSuppressed(parameter, state)) { - state.reportMatch(describeMatch(parameter)); - } - } - return NO_MATCH; - } -} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerRequiredModifiers.java b/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerRequiredModifiers.java index 2b532c7ad64..73919c6f551 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerRequiredModifiers.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerRequiredModifiers.java @@ -35,7 +35,6 @@ import com.google.common.collect.Iterables; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.LinkType; -import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.IdentifierTreeMatcher; @@ -120,90 +119,11 @@ public final class FloggerRequiredModifiers extends BugChecker private static final Matcher CONTAINS_INIT_LOGGER = Matchers.contains(ExpressionTree.class, INIT_LOGGER); - /** - * Constructs a checker configured by {@code flags}. The only flag this constructor looks at is - * {@code "FloggerRequiredModifiers:Goal"}. It expects that flag to have one of the following - * values: - * - *

- * - *

    - *
  • {@code REHOME_FOREIGN_LOGGERS} - *
  • {@code HIDE_LOGGERS_IN_INTERFACES} - *
  • {@code HOIST_CONSTANT_EXPRESSIONS} - *
  • {@code ADD_FINAL} - *
  • {@code ADD_STATIC} - *
  • {@code MAKE_PRIVATE} - *
  • {@code DEFAULT_ALL_GOALS} - *
- * - *

The default value is {@code DEFAULT_ALL_GOALS}, which enables all features; other legal - * values check only a subset of the Flogger best practices, and are designed to generate a more - * fine grained adjustments. - */ @Inject - FloggerRequiredModifiers(ErrorProneFlags flags) { - this(flags.getEnum("FloggerRequiredModifiers:Goal", Goal.class).orElse(Goal.DEFAULT_ALL_GOALS)); - } - - FloggerRequiredModifiers(Goal goal) { - this.goal = goal; - } - - enum Goal { - REHOME_FOREIGN_LOGGERS, - HIDE_LOGGERS_IN_INTERFACES, - HOIST_CONSTANT_EXPRESSIONS, - ADD_FINAL, - ADD_STATIC, - MAKE_PRIVATE, - /** Combines all other goals to be maximally picky and do a full rewrite at once. */ - DEFAULT_ALL_GOALS, - } - - private final Goal goal; - - private boolean shouldRehomeForeignLoggers() { - return goal.equals(Goal.REHOME_FOREIGN_LOGGERS); - } - - private boolean shouldHideInterfaceLoggers() { - switch (goal) { - case HIDE_LOGGERS_IN_INTERFACES: - case DEFAULT_ALL_GOALS: - return true; - default: - return false; - } - } - - private boolean shouldHoistConstantExpressions() { - switch (goal) { - case DEFAULT_ALL_GOALS: - case HOIST_CONSTANT_EXPRESSIONS: - return true; - default: - return false; - } - } - - private boolean shouldStandardizeModifiers() { - switch (goal) { - case DEFAULT_ALL_GOALS: - case ADD_FINAL: - case ADD_STATIC: - case MAKE_PRIVATE: - return true; - default: - return false; - } - } + FloggerRequiredModifiers() {} @Override public Description matchVariable(VariableTree tree, VisitorState state) { - if (!shouldStandardizeModifiers()) { - return NO_MATCH; - } Type loggerType = LOGGER_TYPE.get(state); if (!ASTHelpers.isSameType(loggerType, ASTHelpers.getType(tree), state)) { return NO_MATCH; @@ -216,7 +136,8 @@ public Description matchVariable(VariableTree tree, VisitorState state) { // Static fields with no initializer, or fields initialized to a constant FluentLogger if (initializer == null ? sym.isStatic() - : isConstantLogger(initializer, (ClassSymbol) sym.owner, state)) { + : isConstantLogger(initializer, (ClassSymbol) sym.owner, state) + && !sym.owner.isInterface()) { return fixModifier(tree, (ClassTree) state.getPath().getParentPath().getLeaf(), state); } @@ -241,9 +162,6 @@ private static boolean isConstantLogger( @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!shouldHoistConstantExpressions()) { - return NO_MATCH; - } if (!INIT_LOGGER.matches(tree, state)) { return NO_MATCH; } @@ -252,7 +170,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return NO_MATCH; } - // An expression should always be inside of a method or a variable (initializer blocks count as + // An expression should always be inside a method or a variable (initializer blocks count as // a method), but just in case we use class as a final backstop. TreePath owner = state.findPathToEnclosing(ClassTree.class, MethodTree.class, VariableTree.class); @@ -288,10 +206,6 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return replaceWithFieldLookup(tree, state); } - private boolean modifierChangesInclude(Goal g) { - return goal == Goal.DEFAULT_ALL_GOALS || goal == g; - } - private Description fixModifier(VariableTree field, ClassTree owningClass, VisitorState state) { ModifiersTree modifiers = field.getModifiers(); Set flags = modifiers.getFlags(); @@ -303,16 +217,10 @@ private Description fixModifier(VariableTree field, ClassTree owningClass, Visit // We have to add all the modifiers as a single SuggestedFix or they conflict ImmutableSet.Builder toAdd = ImmutableSet.builder(); SuggestedFix.Builder fix = SuggestedFix.builder(); - if (modifierChangesInclude(Goal.MAKE_PRIVATE)) { - removeModifiers(field, state, PUBLIC, PROTECTED).ifPresent(fix::merge); - toAdd.add(PRIVATE); - } - if (modifierChangesInclude(Goal.ADD_FINAL)) { - toAdd.add(Modifier.FINAL); - } - if (modifierChangesInclude(Goal.ADD_STATIC) - && (flags.contains(FINAL) || modifierChangesInclude(Goal.ADD_FINAL)) - && canHaveStaticFields(ASTHelpers.getSymbol(owningClass))) { + removeModifiers(field, state, PUBLIC, PROTECTED).ifPresent(fix::merge); + toAdd.add(PRIVATE); + toAdd.add(Modifier.FINAL); + if (flags.contains(FINAL) && canHaveStaticFields(ASTHelpers.getSymbol(owningClass))) { // We only add static to fields which are already final, or that we're also making final. // It's a bit dangerous to quietly make something static if it might be getting reassigned. toAdd.add(STATIC); @@ -521,9 +429,6 @@ public Description matchMemberSelect(MemberSelectTree tree, VisitorState state) * refer to a logger in this file, defining one if necessary. */ private Description rehomeLogger(ExpressionTree tree, VisitorState state) { - if (!shouldRehomeForeignLoggers() && !shouldHideInterfaceLoggers()) { - return NO_MATCH; - } Symbol sym = ASTHelpers.getSymbol(tree); if (sym == null) { return NO_MATCH; @@ -545,8 +450,7 @@ private Description rehomeLogger(ExpressionTree tree, VisitorState state) { /* Loggers owned by public interfaces should be moved regardless of whether they're defined in the current file, because fields in interfaces must be public, but loggers should be private. */ - boolean needsMoveFromInterface = shouldHideInterfaceLoggers() && owner.isInterface(); - boolean local = false; + boolean needsMoveFromInterface = owner.isInterface(); Symbol outermostClassOfLogger = findUltimateOwningClass(owner); ClassTree outermostClassOfFile = null; for (Tree parent : state.getPath()) { @@ -556,7 +460,6 @@ private Description rehomeLogger(ExpressionTree tree, VisitorState state) { if (!needsMoveFromInterface) { return NO_MATCH; } - local = true; } if (parent instanceof ClassTree) { outermostClassOfFile = (ClassTree) parent; @@ -569,10 +472,6 @@ private Description rehomeLogger(ExpressionTree tree, VisitorState state) { return NO_MATCH; } - if (!local && !shouldRehomeForeignLoggers()) { - return NO_MATCH; - } - return replaceWithFieldLookup(tree, state); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/NotJavadoc.java b/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/NotJavadoc.java index 465c04a931d..d0bcc9a09d8 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/NotJavadoc.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/NotJavadoc.java @@ -17,7 +17,6 @@ package com.google.errorprone.bugpatterns.javadoc; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; -import static com.google.errorprone.BugPattern.StandardTags.STYLE; import static com.google.errorprone.bugpatterns.javadoc.Utils.getDiagnosticPosition; import static com.google.errorprone.fixes.SuggestedFix.replace; import static com.google.errorprone.matchers.Description.NO_MATCH; @@ -27,6 +26,8 @@ import static com.sun.tools.javac.parser.Tokens.Comment.CommentStyle.JAVADOC; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableRangeSet; +import com.google.common.collect.Range; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -42,6 +43,7 @@ import com.sun.source.tree.PackageTree; 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.MethodSymbol; import com.sun.tools.javac.parser.Tokens.Comment; import java.util.HashMap; @@ -50,14 +52,14 @@ /** A BugPattern; see the summary. */ @BugPattern( - summary = "Avoid using /** for comments which aren't actually Javadoc.", + summary = "Avoid using `/**` for comments which aren't actually Javadoc.", severity = WARNING, - tags = STYLE, documentSuppression = false) public final class NotJavadoc extends BugChecker implements CompilationUnitTreeMatcher { @Override public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { - ImmutableMap javadocableTrees = getJavadoccableTrees(tree, state); + ImmutableMap javadocableTrees = getJavadoccableTrees(tree); + ImmutableRangeSet suppressedRegions = suppressedRegions(state); for (ErrorProneToken token : getTokens(state.getSourceCode().toString(), state.context)) { for (Comment comment : token.comments()) { if (!comment.getStyle().equals(JAVADOC) || comment.getText().equals("/**/")) { @@ -66,19 +68,29 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s if (javadocableTrees.containsKey(token.pos())) { continue; } + + if (suppressedRegions.intersects( + Range.closed( + comment.getSourcePos(0), comment.getSourcePos(comment.getText().length() - 1)))) { + continue; + } + + int endPos = 2; + while (comment.getText().charAt(endPos) == '*') { + endPos++; + } state.reportMatch( describeMatch( getDiagnosticPosition(comment.getSourcePos(0), tree), - replace(comment.getSourcePos(1), comment.getSourcePos(2), ""))); + replace(comment.getSourcePos(1), comment.getSourcePos(endPos - 1), ""))); } } return NO_MATCH; } - private ImmutableMap getJavadoccableTrees( - CompilationUnitTree tree, VisitorState state) { + private ImmutableMap getJavadoccableTrees(CompilationUnitTree tree) { Map javadoccablePositions = new HashMap<>(); - new SuppressibleTreePathScanner(state) { + new TreePathScanner() { @Override public Void visitPackage(PackageTree packageTree, Void unused) { javadoccablePositions.put(getStartPosition(packageTree), packageTree); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/DereferenceWithNullBranch.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/DereferenceWithNullBranch.java new file mode 100644 index 00000000000..c97d151741d --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/DereferenceWithNullBranch.java @@ -0,0 +1,83 @@ +/* + * Copyright 2023 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 com.google.errorprone.VisitorState.memoize; +import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.hasDefinitelyNullBranch; +import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.varsProvenNullByParentTernary; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static javax.lang.model.element.ElementKind.PACKAGE; + +import com.google.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MemberSelectTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.suppliers.Supplier; +import com.sun.source.tree.MemberSelectTree; +import com.sun.tools.javac.code.Symbol; +import javax.lang.model.element.Name; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern(summary = "Dereference of an expression with a null branch", severity = ERROR) +public final class DereferenceWithNullBranch extends BugChecker implements MemberSelectTreeMatcher { + private static final Supplier CLASS_KEYWORD = memoize(state -> state.getName("class")); + + @Override + public Description matchMemberSelect(MemberSelectTree select, VisitorState state) { + if (!memberSelectExpressionIsATrueExpression(select, state)) { + return NO_MATCH; + } + + if (!hasDefinitelyNullBranch( + select.getExpression(), + /* + * TODO(cpovirk): Precompute sets of definitelyNullVars instead passing an empty set, and + * include and varsProvenNullByParentIf alongside varsProvenNullByParentTernary. + */ + ImmutableSet.of(), + varsProvenNullByParentTernary(state.getPath()), + state)) { + return NO_MATCH; + } + + return describeMatch(select); + } + + private static boolean memberSelectExpressionIsATrueExpression( + MemberSelectTree select, VisitorState state) { + // We use the same logic here as we do in + // https://github.com/jspecify/jspecify-reference-checker/blob/06e85b1eb79ecbb9aa6f5713bc759fb5cf402975/src/main/java/com/google/jspecify/nullness/NullSpecVisitor.java#L195-L206 + // (Here, we might not need the isInterface() check, but we keep it for consistency.) + // I could also imagine checking for `getKind() != MODULE`, but it's been working without. + + if (select.getIdentifier().equals(CLASS_KEYWORD.get(state))) { + return false; + } + + Symbol symbol = getSymbol(select.getExpression()); + if (symbol == null) { + return true; + } + return !symbol.getKind().isClass() + && !symbol.getKind().isInterface() + && symbol.getKind() != PACKAGE; + } +} 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 03cd4154604..90f8ce83125 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 @@ -23,6 +23,7 @@ import static com.google.errorprone.bugpatterns.nullness.NullnessUtils.NullableAnnotationToUse.annotationWithoutImporting; import static com.google.errorprone.fixes.SuggestedFix.emptyFix; import static com.google.errorprone.matchers.Matchers.instanceMethod; +import static com.google.errorprone.matchers.Matchers.staticMethod; 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; @@ -31,6 +32,7 @@ import static com.google.errorprone.util.ASTHelpers.stripParentheses; import static com.sun.source.tree.Tree.Kind.ANNOTATED_TYPE; import static com.sun.source.tree.Tree.Kind.ARRAY_TYPE; +import static com.sun.source.tree.Tree.Kind.CONDITIONAL_EXPRESSION; import static com.sun.source.tree.Tree.Kind.IDENTIFIER; import static com.sun.source.tree.Tree.Kind.NULL_LITERAL; import static com.sun.source.tree.Tree.Kind.PARAMETERIZED_TYPE; @@ -95,6 +97,8 @@ private NullnessUtils() {} instanceMethod().onDescendantOf("com.google.common.base.Optional").named("orNull"); private static final Matcher OPTIONAL_OR_ELSE = instanceMethod().onDescendantOf("java.util.Optional").named("orElse"); + private static final Matcher EMPTY_TO_NULL = + staticMethod().onClass("com.google.common.base.Strings").named("emptyToNull"); /** * Returns {@code true} if the flags request that we look to add @Nullable annotations only where @@ -514,7 +518,9 @@ public Boolean visitIdentifier(IdentifierTree tree, Void unused) { @Override public Boolean visitMethodInvocation(MethodInvocationTree tree, Void unused) { - return super.visitMethodInvocation(tree, unused) || isOptionalOrNull(tree); + return super.visitMethodInvocation(tree, unused) + || isOptionalOrNull(tree) + || isStringsEmptyToNull(tree); } @Override @@ -557,6 +563,10 @@ boolean isOptionalOrNull(MethodInvocationTree tree) { */ } + boolean isStringsEmptyToNull(MethodInvocationTree tree) { + return EMPTY_TO_NULL.matches(tree, stateForCompilationUnit); + } + boolean isSwitchExpressionWithDefinitelyNullBranch(Tree tree) { return tree.getKind().name().equals("SWITCH_EXPRESSION") && getCases(tree).stream() @@ -633,6 +643,32 @@ static ImmutableSet varsProvenNullByParentIf(TreePath path) { return ImmutableSet.of(nullCheck.bareIdentifier()); } + /** Returns x if the path's leaf is inside {@code (x == null) ? ... : ...}. */ + public static ImmutableSet varsProvenNullByParentTernary(TreePath path) { + Tree child = path.getLeaf(); + for (Tree tree : path.getParentPath()) { + if (!(tree instanceof ExpressionTree)) { + break; + } + if (tree.getKind() == CONDITIONAL_EXPRESSION) { + ConditionalExpressionTree ternary = (ConditionalExpressionTree) tree; + NullCheck nullCheck = getNullCheck(ternary.getCondition()); + if (nullCheck == null) { + return ImmutableSet.of(); + } + if (child != nullCheck.nullCase(ternary)) { + return ImmutableSet.of(); + } + if (nullCheck.bareIdentifier() == null) { + return ImmutableSet.of(); + } + return ImmutableSet.of(nullCheck.bareIdentifier()); + } + child = tree; + } + return ImmutableSet.of(); + } + @Nullable static VariableTree findDeclaration(VisitorState state, Symbol sym) { JavacProcessingEnvironment javacEnv = JavacProcessingEnvironment.instance(state.context); 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 17e4181c312..8f066e93afd 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 @@ -84,17 +84,14 @@ public class ReturnMissingNullable extends BugChecker implements CompilationUnit anyOf( anyMethod().anyClass().withNameMatching(compile("throw.*Exception")), staticMethod() - .onClassAny( - "org.junit.Assert", - "junit.framework.Assert", - /* - * I'm not sure if TestCase is necessary, as it doesn't define its own fail() - * method, but it commonly appears in lists like this one, so I've included - * it. (Maybe the method was defined on TestCase many versions ago?) - * - * TODO(cpovirk): Confirm need, or remove from everywhere. - */ - "junit.framework.TestCase") + /* + * b/285157761: The reason to look at descendants of the listed classes is mostly + * to catch non-canonical static imports: While TestCase doesn't define its own + * fail() method, javac still lets users import it with "import static + * junit.framework.TestCase.fail." And when users do that, javac acts as if fail() + * is a member of TestCase. We still want to cover it here. + */ + .onDescendantOfAny("org.junit.Assert", "junit.framework.Assert") .named("fail"), staticMethod().onClass("java.lang.System").named("exit"))); 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 18fc8419f68..1561241f2a7 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 @@ -64,7 +64,7 @@ public class DoubleCheckedLocking extends BugChecker implements IfTreeMatcher { @Override public Description matchIf(IfTree outerIf, VisitorState state) { - DCLInfo info = findDCL(outerIf); + DCLInfo info = findDcl(outerIf); if (info == null) { return Description.NO_MATCH; } @@ -219,7 +219,7 @@ static DCLInfo create( * null-checks are accepted in either order. */ @Nullable - static DCLInfo findDCL(IfTree outerIf) { + private static DCLInfo findDcl(IfTree outerIf) { // TODO(cushon): Optional.ifPresent... ExpressionTree outerIfTest = getNullCheckedExpression(outerIf.getCondition()); if (outerIfTest == null) { 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 605708f181b..bfe118b3249 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 @@ -127,7 +127,7 @@ protected Description checkGuardedAccess( // members guarded by ReadWriteLocks. We could allow accesses when either the // read or write locks are held, but that's not much better than enforcing // nothing. - if (isRWLock(guard, state)) { + if (isRwLock(guard, state)) { return NO_MATCH; } @@ -210,7 +210,7 @@ private static boolean enclosingInstance(GuardedByExpression expr) { * Returns true if the lock expression corresponds to a {@code * java.util.concurrent.locks.ReadWriteLock}. */ - private static boolean isRWLock(GuardedByExpression guard, VisitorState state) { + private static boolean isRwLock(GuardedByExpression guard, VisitorState state) { Type guardType = guard.type(); if (guardType == null) { return false; 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 9a79b507654..57fb8b5ebc3 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 @@ -246,6 +246,7 @@ public Builder typeParameterAnnotation(Class typeParameter this.typeParameterAnnotation = ImmutableSet.of(typeParameterAnnotation.getName()); return this; } + /** * An annotation which, when found on a type parameter, indicates that the type parameter may * only be instantiated with thread-safe types. 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 a0e461df7d2..8488c4ea95b 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 @@ -176,6 +176,7 @@ private static ImmutableMap buildImmutableClasses( .add("android.net.Uri") .add("android.util.Size") .add("java.awt.Color") + .add("java.lang.invoke.MethodHandle") .add("java.util.AbstractMap$SimpleImmutableEntry", "K", "V") .add("java.util.Optional", "T") .add("java.util.OptionalDouble") diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownThreadSafety.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownThreadSafety.java index 94caf64a05f..632f57cd62c 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownThreadSafety.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownThreadSafety.java @@ -98,6 +98,7 @@ private static ImmutableMap buildThreadSafeClasses( .add(java.util.concurrent.Future.class, "V") .add(java.util.concurrent.Semaphore.class) .add(java.util.concurrent.ScheduledExecutorService.class) + .add(java.util.concurrent.locks.Condition.class) .add(java.util.concurrent.locks.Lock.class) .add(java.util.concurrent.locks.ReadWriteLock.class) .add(java.util.concurrent.locks.ReentrantLock.class) @@ -127,6 +128,8 @@ private static ImmutableMap buildThreadSafeClasses( .add(Throwable.class) // Unsafe due to initCause, but generally used across threads .add("java.lang.ThreadLocal") .add("java.lang.invoke.MethodHandle") + .add(java.lang.reflect.Method.class) + .add(java.lang.reflect.Field.class) .add("com.github.benmanes.caffeine.cache.Cache", "K", "V") .add("com.github.benmanes.caffeine.cache.LoadingCache", "K", "V") .add("com.github.benmanes.caffeine.cache.AsyncLoadingCache", "K", "V") diff --git a/core/src/main/java/com/google/errorprone/refaster/Template.java b/core/src/main/java/com/google/errorprone/refaster/Template.java index 7c1a2712e65..d48cf85e10f 100644 --- a/core/src/main/java/com/google/errorprone/refaster/Template.java +++ b/core/src/main/java/com/google/errorprone/refaster/Template.java @@ -88,8 +88,6 @@ public abstract class Template implements Serializable { private static final Logger logger = Logger.getLogger(Template.class.toString()); - public static final boolean AUTOBOXING_DEFAULT = true; - public abstract ImmutableClassToInstanceMap annotations(); public abstract ImmutableList templateTypeVariables(); diff --git a/core/src/main/java/com/google/errorprone/refaster/URepeated.java b/core/src/main/java/com/google/errorprone/refaster/URepeated.java index 36059637d31..4a0d64c7691 100644 --- a/core/src/main/java/com/google/errorprone/refaster/URepeated.java +++ b/core/src/main/java/com/google/errorprone/refaster/URepeated.java @@ -43,7 +43,7 @@ protected Choice defaultAction(Tree node, @Nullable Unifier unifier) { @Override public JCExpression inline(Inliner inliner) throws CouldNotResolveImportException { throw new UnsupportedOperationException( - "@CountConstraint variables should be inlined inside method invocations or newArray"); + "@Repeated variables should be inlined inside method invocations or newArray"); } @Override diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index aaab3feeaf7..5efa078e05b 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -22,6 +22,7 @@ import com.google.common.collect.Streams; import com.google.errorprone.BugCheckerInfo; import com.google.errorprone.bugpatterns.ASTHelpersSuggestions; +import com.google.errorprone.bugpatterns.AddressSelection; import com.google.errorprone.bugpatterns.AlreadyChecked; import com.google.errorprone.bugpatterns.AlwaysThrows; import com.google.errorprone.bugpatterns.AmbiguousMethodReference; @@ -40,6 +41,7 @@ import com.google.errorprone.bugpatterns.AssertionFailureIgnored; import com.google.errorprone.bugpatterns.AsyncCallableReturnsNull; import com.google.errorprone.bugpatterns.AsyncFunctionReturnsNull; +import com.google.errorprone.bugpatterns.AttemptedNegativeZero; import com.google.errorprone.bugpatterns.AutoValueBuilderDefaultsInConstructor; import com.google.errorprone.bugpatterns.AutoValueFinalMethods; import com.google.errorprone.bugpatterns.AutoValueImmutableFields; @@ -153,6 +155,7 @@ import com.google.errorprone.bugpatterns.GetClassOnEnum; import com.google.errorprone.bugpatterns.HashtableContains; import com.google.errorprone.bugpatterns.HidingField; +import com.google.errorprone.bugpatterns.ICCProfileGetInstance; import com.google.errorprone.bugpatterns.IdentityBinaryExpression; import com.google.errorprone.bugpatterns.IdentityHashMapBoxing; import com.google.errorprone.bugpatterns.IdentityHashMapUsage; @@ -172,6 +175,7 @@ import com.google.errorprone.bugpatterns.InfiniteRecursion; import com.google.errorprone.bugpatterns.InitializeInline; import com.google.errorprone.bugpatterns.InjectOnBugCheckers; +import com.google.errorprone.bugpatterns.InlineTrivialConstant; import com.google.errorprone.bugpatterns.InputStreamSlowMultibyteRead; import com.google.errorprone.bugpatterns.InsecureCipherMode; import com.google.errorprone.bugpatterns.InstanceOfAndCastMatchWrongType; @@ -227,6 +231,7 @@ import com.google.errorprone.bugpatterns.MissingFail; import com.google.errorprone.bugpatterns.MissingImplementsComparable; import com.google.errorprone.bugpatterns.MissingOverride; +import com.google.errorprone.bugpatterns.MissingRefasterAnnotation; import com.google.errorprone.bugpatterns.MissingSuperCall; import com.google.errorprone.bugpatterns.MissingTestCall; import com.google.errorprone.bugpatterns.MisusedDayOfYear; @@ -245,14 +250,17 @@ import com.google.errorprone.bugpatterns.MultipleTopLevelClasses; import com.google.errorprone.bugpatterns.MultipleUnaryOperatorsInMethodCall; import com.google.errorprone.bugpatterns.MustBeClosedChecker; +import com.google.errorprone.bugpatterns.MutableGuiceModule; import com.google.errorprone.bugpatterns.MutablePublicArray; import com.google.errorprone.bugpatterns.NCopiesOfChar; +import com.google.errorprone.bugpatterns.NamedLikeContextualKeyword; import com.google.errorprone.bugpatterns.NarrowCalculation; import com.google.errorprone.bugpatterns.NarrowingCompoundAssignment; import com.google.errorprone.bugpatterns.NegativeCharLiteral; import com.google.errorprone.bugpatterns.NestedInstanceOfConditions; import com.google.errorprone.bugpatterns.NewFileSystem; import com.google.errorprone.bugpatterns.NoAllocationChecker; +import com.google.errorprone.bugpatterns.NonApiType; import com.google.errorprone.bugpatterns.NonAtomicVolatileUpdate; import com.google.errorprone.bugpatterns.NonCanonicalStaticImport; import com.google.errorprone.bugpatterns.NonCanonicalStaticMemberImport; @@ -264,6 +272,7 @@ import com.google.errorprone.bugpatterns.NullTernary; import com.google.errorprone.bugpatterns.NullableConstructor; import com.google.errorprone.bugpatterns.NullableOnContainingClass; +import com.google.errorprone.bugpatterns.NullableOptional; import com.google.errorprone.bugpatterns.NullablePrimitive; import com.google.errorprone.bugpatterns.NullablePrimitiveArray; import com.google.errorprone.bugpatterns.NullableVoid; @@ -280,6 +289,7 @@ import com.google.errorprone.bugpatterns.OutlineNone; import com.google.errorprone.bugpatterns.OverrideThrowableToString; import com.google.errorprone.bugpatterns.Overrides; +import com.google.errorprone.bugpatterns.OverridingMethodInconsistentArgumentNamesChecker; import com.google.errorprone.bugpatterns.PackageInfo; import com.google.errorprone.bugpatterns.PackageLocation; import com.google.errorprone.bugpatterns.ParameterComment; @@ -391,9 +401,9 @@ import com.google.errorprone.bugpatterns.UnnecessaryParentheses; import com.google.errorprone.bugpatterns.UnnecessarySetDefault; import com.google.errorprone.bugpatterns.UnnecessaryStaticImport; +import com.google.errorprone.bugpatterns.UnnecessaryStringBuilder; import com.google.errorprone.bugpatterns.UnnecessaryTestMethodPrefix; import com.google.errorprone.bugpatterns.UnnecessaryTypeArgument; -import com.google.errorprone.bugpatterns.UnqualifiedYield; import com.google.errorprone.bugpatterns.UnsafeFinalization; import com.google.errorprone.bugpatterns.UnsafeLocaleUsage; import com.google.errorprone.bugpatterns.UnsafeReflectiveConstructionCast; @@ -451,7 +461,6 @@ import com.google.errorprone.bugpatterns.flogger.FloggerLogVarargs; import com.google.errorprone.bugpatterns.flogger.FloggerLogWithCause; import com.google.errorprone.bugpatterns.flogger.FloggerMessageFormat; -import com.google.errorprone.bugpatterns.flogger.FloggerPassedAround; import com.google.errorprone.bugpatterns.flogger.FloggerRedundantIsEnabled; import com.google.errorprone.bugpatterns.flogger.FloggerRequiredModifiers; import com.google.errorprone.bugpatterns.flogger.FloggerSplitLogStatement; @@ -514,6 +523,7 @@ import com.google.errorprone.bugpatterns.javadoc.UnescapedEntity; import com.google.errorprone.bugpatterns.javadoc.UnrecognisedJavadocTag; import com.google.errorprone.bugpatterns.javadoc.UrlInSee; +import com.google.errorprone.bugpatterns.nullness.DereferenceWithNullBranch; import com.google.errorprone.bugpatterns.nullness.EqualsBrokenForNull; import com.google.errorprone.bugpatterns.nullness.EqualsMissingNullable; import com.google.errorprone.bugpatterns.nullness.ExtendsObject; @@ -612,6 +622,14 @@ public static ScannerSupplier errorChecks() { return allChecks().filter(Predicates.in(ENABLED_ERRORS)); } + /** + * Returns a {@link ScannerSupplier} with the {@link BugChecker}s that are in the ENABLED_WARNINGS + * list. + */ + public static ScannerSupplier warningChecks() { + return allChecks().filter(Predicates.in(ENABLED_WARNINGS)); + } + /** A list of all checks with severity ERROR that are on by default. */ public static final ImmutableSet ENABLED_ERRORS = getSuppliers( @@ -650,6 +668,7 @@ public static ScannerSupplier errorChecks() { DangerousLiteralNullChecker.class, DeadException.class, DeadThread.class, + DereferenceWithNullBranch.class, DiscardedPostfixExpression.class, DoNotCallChecker.class, DoNotMockChecker.class, @@ -786,7 +805,6 @@ public static ScannerSupplier errorChecks() { UnsafeWildcard.class, UnusedAnonymousClass.class, UnusedCollectionModifiedInPlace.class, - UseCorrectAssertInTests.class, Validator.class, VarTypeName.class, WrongOneof.class, @@ -800,6 +818,7 @@ public static ScannerSupplier errorChecks() { getSuppliers( // keep-sorted start ASTHelpersSuggestions.class, + AddressSelection.class, AlmostJavadoc.class, AlreadyChecked.class, AmbiguousMethodReference.class, @@ -810,6 +829,7 @@ public static ScannerSupplier errorChecks() { AssertThrowsMultipleStatements.class, AssertionFailureIgnored.class, AssistedInjectAndInjectOnSameConstructor.class, + AttemptedNegativeZero.class, AutoValueFinalMethods.class, AutoValueImmutableFields.class, AutoValueSubclassLeaked.class, @@ -873,6 +893,7 @@ public static ScannerSupplier errorChecks() { FutureReturnValueIgnored.class, GetClassOnEnum.class, HidingField.class, + ICCProfileGetInstance.class, IdentityHashMapUsage.class, IgnoredPureGetter.class, ImmutableAnnotationChecker.class, @@ -886,6 +907,7 @@ public static ScannerSupplier errorChecks() { InjectOnConstructorOfAbstractClass.class, InjectedConstructorAnnotations.class, InlineFormatString.class, + InlineTrivialConstant.class, Inliner.class, InputStreamSlowMultibyteRead.class, InstanceOfAndCastMatchWrongType.class, @@ -937,6 +959,7 @@ public static ScannerSupplier errorChecks() { MissingFail.class, MissingImplementsComparable.class, MissingOverride.class, + MissingRefasterAnnotation.class, MissingSummary.class, MixedMutabilityReturnType.class, MockNotUsedInProduction.class, @@ -946,17 +969,20 @@ public static ScannerSupplier errorChecks() { MultipleParallelOrSequentialCalls.class, MultipleUnaryOperatorsInMethodCall.class, MutablePublicArray.class, + NamedLikeContextualKeyword.class, NarrowCalculation.class, NarrowingCompoundAssignment.class, NegativeCharLiteral.class, NestedInstanceOfConditions.class, NewFileSystem.class, + NonApiType.class, NonAtomicVolatileUpdate.class, NonCanonicalType.class, NonOverridingEquals.class, NotJavadoc.class, NullOptional.class, NullableConstructor.class, + NullableOptional.class, NullablePrimitive.class, NullablePrimitiveArray.class, NullableVoid.class, @@ -971,6 +997,7 @@ public static ScannerSupplier errorChecks() { OverrideThrowableToString.class, Overrides.class, OverridesGuiceInjectableMethod.class, + OverridingMethodInconsistentArgumentNamesChecker.class, ParameterName.class, PreconditionsCheckNotNullRepeated.class, PrimitiveAtomicReference.class, @@ -1023,7 +1050,7 @@ public static ScannerSupplier errorChecks() { UnnecessaryMethodInvocationMatcher.class, UnnecessaryMethodReference.class, UnnecessaryParentheses.class, - UnqualifiedYield.class, + UnnecessaryStringBuilder.class, UnrecognisedJavadocTag.class, UnsafeFinalization.class, UnsafeReflectiveConstructionCast.class, @@ -1084,7 +1111,6 @@ public static ScannerSupplier errorChecks() { FieldMissingNullable.class, FloggerLogWithCause.class, FloggerMessageFormat.class, - FloggerPassedAround.class, FloggerRedundantIsEnabled.class, FloggerRequiredModifiers.class, FloggerWithCause.class, @@ -1115,6 +1141,7 @@ public static ScannerSupplier errorChecks() { MoreThanOneQualifier.class, MultiVariableDeclaration.class, MultipleTopLevelClasses.class, + MutableGuiceModule.class, NoAllocationChecker.class, NonCanonicalStaticMemberImport.class, PackageLocation.class, @@ -1172,6 +1199,7 @@ public static ScannerSupplier errorChecks() { UnsafeLocaleUsage.class, UnusedException.class, UrlInSee.class, + UseCorrectAssertInTests.class, UseEnumSwitch.class, UsingJsr305CheckReturnValue.class, VarChecker.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ASTHelpersSuggestionsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ASTHelpersSuggestionsTest.java index e0be410138b..cfc4ad9e499 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ASTHelpersSuggestionsTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ASTHelpersSuggestionsTest.java @@ -87,4 +87,29 @@ public void onSymbolSubtype() { "jdk.compiler/com.sun.tools.javac.code", "jdk.compiler/com.sun.tools.javac.util") .doTest(); } + + @Test + public void symbolGetEnclosedElements() { + testHelper + .addInputLines( + "Test.java", + "import com.sun.tools.javac.code.Symbol.ClassSymbol;", + "class Test {", + " void f(ClassSymbol s) {", + " s.getEnclosedElements();", + " }", + "}") + .addOutputLines( + "Test.java", + "import static com.google.errorprone.util.ASTHelpers.getEnclosedElements;", + "import com.sun.tools.javac.code.Symbol.ClassSymbol;", + "class Test {", + " void f(ClassSymbol s) {", + " getEnclosedElements(s);", + " }", + "}") + .addModules( + "jdk.compiler/com.sun.tools.javac.code", "jdk.compiler/com.sun.tools.javac.util") + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/AddressSelectionTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/AddressSelectionTest.java new file mode 100644 index 00000000000..0d1537570ac --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/AddressSelectionTest.java @@ -0,0 +1,126 @@ +/* + * Copyright 2023 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 com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class AddressSelectionTest { + + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(AddressSelection.class, getClass()); + + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(AddressSelection.class, getClass()); + + @Test + public void positive() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.net.InetAddress;", + "import java.net.InetSocketAddress;", + "import java.net.Socket;", + "class Test {", + " void f() throws Exception{", + " // BUG: Diagnostic contains:", + " InetAddress.getByName(\"example.com\");", + " // BUG: Diagnostic contains:", + " new Socket(\"example.com\", 80);", + " // BUG: Diagnostic contains:", + " new InetSocketAddress(\"example.com\", 80);", + " }", + "}") + .doTest(); + } + + @Test + public void negative() throws Exception { + compilationHelper + .addSourceLines( + "Test.java", + "import java.net.InetAddress;", + "import java.net.InetSocketAddress;", + "import java.net.Socket;", + "class Test {", + " void f() throws Exception{", + " new Socket(InetAddress.getLoopbackAddress(), 80);", + " InetAddress.getAllByName(\"example.com\");", + " new InetSocketAddress(InetAddress.getLoopbackAddress(), 80);", + " }", + "}") + .doTest(); + } + + @Test + public void negativeLocalhost() throws Exception { + compilationHelper + .addSourceLines( + "Test.java", + "import java.net.InetAddress;", + "import java.net.InetSocketAddress;", + "import java.net.Socket;", + "class Test {", + " void f() throws Exception{", + " new Socket(\"localhost\", 80);", + " InetAddress.getByName(\"localhost\");", + " new InetSocketAddress(\"localhost\", 80);", + " }", + "}") + .doTest(); + } + + @Test + public void refactor() throws Exception { + refactoringTestHelper + .addInputLines( + "Test.java", + "import java.net.InetAddress;", + "import java.net.InetSocketAddress;", + "import java.net.Socket;", + "class Test {", + " void f() throws Exception{", + " new Socket(\"127.0.0.1\", 80);", + " InetAddress.getByName(\"127.0.0.1\");", + " new InetSocketAddress(\"127.0.0.1\", 80);", + " new Socket(\"::1\", 80);", + " InetAddress.getByName(\"::1\");", + " new InetSocketAddress(\"::1\", 80);", + " }", + "}") + .addOutputLines( + "Test.java", + "import java.net.InetAddress;", + "import java.net.InetSocketAddress;", + "import java.net.Socket;", + "class Test {", + " void f() throws Exception{", + " new Socket(InetAddress.getLoopbackAddress(), 80);", + " InetAddress.getLoopbackAddress();", + " new InetSocketAddress(InetAddress.getLoopbackAddress(), 80);", + " new Socket(InetAddress.getLoopbackAddress(), 80);", + " InetAddress.getLoopbackAddress();", + " new InetSocketAddress(InetAddress.getLoopbackAddress(), 80);", + " }", + "}") + .doTest(); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/AttemptedNegativeZeroTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/AttemptedNegativeZeroTest.java new file mode 100644 index 00000000000..4b9d2c149a7 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/AttemptedNegativeZeroTest.java @@ -0,0 +1,57 @@ +/* + * Copyright 2023 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 com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** {@link AttemptedNegativeZero}Test. */ +@RunWith(JUnit4.class) +public final class AttemptedNegativeZeroTest { + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(AttemptedNegativeZero.class, getClass()); + + @Test + public void positive() { + compilationHelper + .addSourceLines( + "Foo.java", + "class Foo {", + " // BUG: Diagnostic contains: ", + " double x = -0;", + " // BUG: Diagnostic contains: ", + " double y = -0L;", // I didn't see the `long` case in the wild. + "}") + .doTest(); + } + + @Test + public void negative() { + compilationHelper + .addSourceLines( + "Foo.java", + "class Foo {", + " int w = -0;", // weird but not likely to be hiding any bugs + " double x = -0.0;", + " double y = 0;", + " double z = +0;", + "}") + .doTest(); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/BanClassLoaderTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/BanClassLoaderTest.java index e6604e44944..fc1118f7655 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/BanClassLoaderTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/BanClassLoaderTest.java @@ -22,7 +22,7 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** {@link BanRMI}Test */ +/** {@link BanClassLoader}Test */ @RunWith(JUnit4.class) public class BanClassLoaderTest { private final CompilationTestHelper compilationHelper = diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/CatchAndPrintStackTraceTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/CatchAndPrintStackTraceTest.java index 7d7146be4ef..0406d364350 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/CatchAndPrintStackTraceTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/CatchAndPrintStackTraceTest.java @@ -41,12 +41,6 @@ public void positive() { " // BUG: Diagnostic contains:", " t.printStackTrace();", " }", - " try {", - " System.err.println();", - " } catch (Throwable t) {", - " // BUG: Diagnostic contains:", - " t.printStackTrace(System.err);", - " }", " }", "}") .doTest(); @@ -68,6 +62,11 @@ public void negative() { " t.printStackTrace();", " t.printStackTrace();", " }", + " try {", + " System.err.println();", + " } catch (Throwable t) {", + " t.printStackTrace(System.err);", + " }", " }", "}") .doTest(); diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/DirectInvocationOnMockTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/DirectInvocationOnMockTest.java index c7c87940625..29f490ac6cb 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/DirectInvocationOnMockTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/DirectInvocationOnMockTest.java @@ -159,7 +159,7 @@ public void directInvocationOnMock_withinWhen_noFinding() { @Test public void directInvocationOnMock_withinGiven_noFinding() { - assumeTrue(isBDDMockitoAvailable()); + assumeTrue(isBddMockitoAvailable()); helper .addSourceLines( "Test.java", @@ -194,7 +194,7 @@ public void directInvocationOnMock_setUpToCallRealMethod_noFinding() { @Test public void directInvocationOnMock_setUpToCallRealMethodUsingGiven_noFinding() { - assumeTrue(isBDDMockitoAvailable()); + assumeTrue(isBddMockitoAvailable()); helper .addSourceLines( "Test.java", @@ -229,7 +229,7 @@ public void directInvocationOnMock_setUpWithDoCallRealMethod_noFinding() { @Test public void directInvocationOnMock_setUpWithDoCallRealMethodUsingGiven_noFinding() { - assumeTrue(isBDDMockitoAvailable()); + assumeTrue(isBddMockitoAvailable()); helper .addSourceLines( "Test.java", @@ -267,7 +267,7 @@ public void directInvocationOnMock_withinCustomWhen_noFinding() { @Test public void directInvocationOnMock_withinCustomGiven_noFinding() { - assumeTrue(isBDDMockitoAvailable()); + assumeTrue(isBddMockitoAvailable()); helper .addSourceLines( "Test.java", @@ -305,7 +305,7 @@ public void directInvocationOnMock_withinWhenWithCast_noFinding() { @Test public void directInvocationOnMock_withinGivenWithCast_noFinding() { - assumeTrue(isBDDMockitoAvailable()); + assumeTrue(isBddMockitoAvailable()); helper .addSourceLines( "Test.java", @@ -336,7 +336,7 @@ public void finalMethodInvoked_noFinding() { .doTest(); } - private static boolean isBDDMockitoAvailable() { + private static boolean isBddMockitoAvailable() { try { Class.forName("org.mockito.BDDMockito"); return true; diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/DoNotUseRuleChainTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/DoNotUseRuleChainTest.java index 3c2a8a2f003..fe92b85fc9c 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/DoNotUseRuleChainTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/DoNotUseRuleChainTest.java @@ -243,7 +243,6 @@ public void refactoringOrderedRulesChain() { "OrderedRuleChain.java", "package orderedrulechain;", "import org.junit.Rule;", - "import org.junit.rules.RuleChain;", "import org.junit.rules.TestRule;", "import org.junit.runner.Description;", "import org.junit.runners.model.Statement;", @@ -287,7 +286,6 @@ public void refactoringUnorderedRuleChainWithNewObjects() { "UnorderedRuleChainWithNewObjects.java", "package orderedrulechainwithnewobjects;", "import org.junit.Rule;", - "import org.junit.rules.RuleChain;", "import org.junit.rules.TestRule;", "import org.junit.runner.Description;", "import org.junit.runners.model.Statement;", @@ -336,7 +334,6 @@ public void refactoringUnorderedRuleChainWithExistingObjects() { "UnorderedRuleChainWithExistingObjects.java", "package orderedrulechainwithexistingobjects;", "import org.junit.Rule;", - "import org.junit.rules.RuleChain;", "import org.junit.rules.TestRule;", "import org.junit.runner.Description;", "import org.junit.runners.model.Statement;", @@ -375,7 +372,6 @@ public void refactoringUnorderedRuleChainWithGenericClass() { "UnorderedRuleChainWithGenericClass.java", "package orderedrulechainwithgenericclass;", "import org.junit.Rule;", - "import org.junit.rules.RuleChain;", "import org.junit.rules.TestRule;", "import org.junit.runner.Description;", "import org.junit.runners.model.Statement;", @@ -418,7 +414,6 @@ public void refactoringUnorderedTwoRuleChainWithClassRule() { "package orderedrulechainwithclassrule;", "import org.junit.ClassRule;", "import org.junit.Rule;", - "import org.junit.rules.RuleChain;", "import org.junit.rules.TestRule;", "import org.junit.runner.Description;", "import org.junit.runners.model.Statement;", @@ -458,7 +453,6 @@ public void refactoringUnorderedRuleChainWithoutTestRuleImport() { "UnorderedRuleChainWithoutTestRuleImport.java", "package orderedrulechainwithouttestruleimport;", "import org.junit.Rule;", - "import org.junit.rules.RuleChain;", "import org.junit.rules.TestRule;", "import org.junit.runners.model.Statement;", "public class UnorderedRuleChainWithoutTestRuleImport {", diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/FallThroughTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/FallThroughTest.java index 203f5c40e1c..774e4f2791d 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/FallThroughTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/FallThroughTest.java @@ -20,6 +20,7 @@ import com.google.errorprone.CompilationTestHelper; import com.google.errorprone.util.RuntimeVersion; +import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -117,4 +118,28 @@ public void arrowSwitch() { "}") .doTest(); } + + @Ignore("https://github.com/google/error-prone/issues/2638") + @Test + public void i2118() { + assumeTrue(RuntimeVersion.isAtLeast14()); + testHelper + .addSourceLines( + "Test.java", + "class Test {", + " enum Case { ONE, TWO }", + " void m(Case c) {", + " switch (c) {", + " case ONE:", + " switch (c) {", + " case ONE -> m(c);", + " case TWO -> m(c);", + " }", + " default:", + " assert false;", + " }", + " }", + "}") + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ICCProfileGetInstanceTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ICCProfileGetInstanceTest.java new file mode 100644 index 00000000000..0e36e8a0e73 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ICCProfileGetInstanceTest.java @@ -0,0 +1,57 @@ +/* + * Copyright 2023 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 com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class ICCProfileGetInstanceTest { + private final CompilationTestHelper testHelper = + CompilationTestHelper.newInstance(ICCProfileGetInstance.class, getClass()); + + @Test + public void positive() { + testHelper + .addSourceLines( + "Test.java", + "import java.awt.color.ICC_Profile;", + "class Test {", + " void f(String s) throws Exception {", + " // BUG: Diagnostic contains:", + " ICC_Profile.getInstance(s);", + " }", + "}") + .doTest(); + } + + @Test + public void negative() { + testHelper + .addSourceLines( + "Test.java", + "import java.awt.color.ICC_Profile;", + "class Test {", + " void f(byte[] b) throws Exception {", + " ICC_Profile.getInstance(b);", + " }", + "}") + .doTest(); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/InlineTrivialConstantTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/InlineTrivialConstantTest.java new file mode 100644 index 00000000000..3a29e06541b --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/InlineTrivialConstantTest.java @@ -0,0 +1,75 @@ +/* + * Copyright 2023 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 com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class InlineTrivialConstantTest { + @Test + public void positive() { + BugCheckerRefactoringTestHelper.newInstance(InlineTrivialConstant.class, getClass()) + .addInputLines( + "Test.java", + "class Test {", + " private static final String EMPTY_STRING = \"\";", + " void f() {", + " System.err.println(EMPTY_STRING);", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " void f() {", + " System.err.println(\"\");", + " }", + "}") + .doTest(); + } + + @Test + public void negative() { + CompilationTestHelper.newInstance(InlineTrivialConstant.class, getClass()) + .addSourceLines( + "Test.java", + "class Test {", + " static class NonPrivate {", + " static final String EMPTY_STRING = \"\";", + " }", + " static class NonStatic {", + " private final String EMPTY_STRING = \"\";", + " }", + " static class NonFinal {", + " private static String EMPTY_STRING = \"\";", + " }", + " static class NonString {", + " private static final Object EMPTY_STRING = \"\";", + " }", + " static class WrongName {", + " private static final String LAUNCH_CODES = \"\";", + " }", + " static class WrongValue {", + " private static final String EMPTY_STRING = \"hello\";", + " }", + "}") + .doTest(); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/MemberNameTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/MemberNameTest.java index 24648a255b9..6e58cfdb57a 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/MemberNameTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/MemberNameTest.java @@ -52,6 +52,21 @@ public void nameWithUnderscores() { .doTest(); } + @Test + public void nameWithUnderscores_findingEmphasisesInitialism() { + helper + .addSourceLines( + "Test.java", + "class Test {", + " // BUG: Diagnostic contains: acronyms", + " private int misnamedRPCClient;", + " int get() {", + " return misnamedRPCClient;", + " }", + "}") + .doTest(); + } + @Test public void staticFields() { refactoringHelper @@ -374,4 +389,78 @@ public void nonConformantOverride_nameDoesNotMatchSuper_flagged() { "}") .doTest(); } + + @Test + public void initialismsInMethodNames_partOfCamelCase() { + helper + .addSourceLines( + "Test.java", + "interface Test {", + " // BUG: Diagnostic contains: getRpcPolicy", + " int getRPCPolicy();", + " // BUG: Diagnostic contains: getRpc", + " int getRPC();", + "}") + .doTest(); + } + + @Test + public void initialismsInVariableNames_partOfCamelCase() { + helper + .addSourceLines( + "Test.java", + "class Test {", + " // BUG: Diagnostic contains: getRpcPolicy", + " int getRPCPolicy;", + " // BUG: Diagnostic contains: getRpc", + " int getRPC;", + "}") + .doTest(); + } + + @Test + public void initialismsInVariableNames_magicNamesExempt() { + helper + .addSourceLines( + "Test.java", // + "class Test {", + " private static final long serialVersionUID = 0;", + "}") + .doTest(); + } + + @Test + public void lambdaExpressionParameterInsideOverridingMethod() { + helper + .addSourceLines( + "Test.java", + "import java.util.function.Function;", + "class Test {", + " @Override", + " public String toString() {", + " // BUG: Diagnostic contains: fooBar", + " Function f = foo_bar -> foo_bar;", + " return f.apply(\"foo\");", + " }", + "}") + .doTest(); + } + + @Test + public void methodReference() { + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " private void foo_bar() {}", + " private Runnable r = this::foo_bar;", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " private void fooBar() {}", + " private Runnable r = this::fooBar;", + "}") + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/MissingRefasterAnnotationTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/MissingRefasterAnnotationTest.java new file mode 100644 index 00000000000..29f8ccb9ba4 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/MissingRefasterAnnotationTest.java @@ -0,0 +1,112 @@ +/* + * 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 com.google.common.base.Predicates; +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link MissingRefasterAnnotation}. */ +@RunWith(JUnit4.class) +public final class MissingRefasterAnnotationTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(MissingRefasterAnnotation.class, getClass()) + .expectErrorMessage( + "X", + Predicates.containsPattern( + "The Refaster template contains a method without any Refaster annotations")); + + @Test + public void testIdentification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import com.google.errorprone.refaster.annotation.AfterTemplate;", + "import com.google.errorprone.refaster.annotation.AlsoNegation;", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "import java.util.Map;", + "", + "class A {", + " // BUG: Diagnostic matches: X", + " static final class MethodLacksBeforeTemplateAnnotation {", + " @BeforeTemplate", + " boolean before1(String string) {", + " return string.equals(\"\");", + " }", + "", + " // @BeforeTemplate is missing", + " boolean before2(String string) {", + " return string.length() == 0;", + " }", + "", + " @AfterTemplate", + " @AlsoNegation", + " boolean after(String string) {", + " return string.isEmpty();", + " }", + " }", + "", + " // BUG: Diagnostic matches: X", + " static final class MethodLacksAfterTemplateAnnotation {", + " @BeforeTemplate", + " boolean before(String string) {", + " return string.equals(\"\");", + " }", + "", + " // @AfterTemplate is missing", + " boolean after(String string) {", + " return string.isEmpty();", + " }", + " }", + "", + " // BUG: Diagnostic matches: X", + " abstract class MethodLacksPlaceholderAnnotation {", + " // @Placeholder is missing", + " abstract V function(K key);", + "", + " @BeforeTemplate", + " void before(Map map, K key) {", + " if (!map.containsKey(key)) {", + " map.put(key, function(key));", + " }", + " }", + "", + " @AfterTemplate", + " void after(Map map, K key) {", + " map.computeIfAbsent(key, k -> function(k));", + " }", + " }", + "", + " static final class ValidRefasterTemplate {", + " @BeforeTemplate", + " void unusedPureFunctionCall(Object o) {", + " o.toString();", + " }", + " }", + "", + " static final class NotARefasterTemplate {", + " @Override", + " public String toString() {", + " return \"This is not a Refaster template\";", + " }", + " }", + "}") + .doTest(); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/MutableGuiceModuleTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/MutableGuiceModuleTest.java new file mode 100644 index 00000000000..0cc9e3e3b57 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/MutableGuiceModuleTest.java @@ -0,0 +1,77 @@ +/* + * Copyright 2023 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 com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class MutableGuiceModuleTest { + private final CompilationTestHelper testHelper = + CompilationTestHelper.newInstance(MutableGuiceModule.class, getClass()); + + @Test + public void positive() { + testHelper + .addSourceLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "class Test extends AbstractModule {", + " // BUG: Diagnostic contains:", + " String x = new String();", + "}") + .doTest(); + } + + @Test + public void positiveType() { + testHelper + .addSourceLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "class Test extends AbstractModule {", + " // BUG: Diagnostic contains: Object is mutable", + " final Object x = new Object();", + "}") + .doTest(); + } + + @Test + public void negativeFinal() { + testHelper + .addSourceLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "class Test extends AbstractModule {", + " final String x = new String();", + "}") + .doTest(); + } + + @Test + public void negativeNotAModule() { + testHelper + .addSourceLines( + "Test.java", // + "class Test {", + " String x = new String();", + "}") + .doTest(); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/NamedLikeContextualKeywordTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/NamedLikeContextualKeywordTest.java new file mode 100644 index 00000000000..da56f24f412 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/NamedLikeContextualKeywordTest.java @@ -0,0 +1,326 @@ +/* + * Copyright 2023 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 org.junit.Assume.assumeTrue; + +import com.google.common.collect.ImmutableList; +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.util.RuntimeVersion; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link NamedLikeContextualKeywordTest}. */ +@RunWith(JUnit4.class) +public final class NamedLikeContextualKeywordTest { + private final CompilationTestHelper helper = + CompilationTestHelper.newInstance(NamedLikeContextualKeyword.class, getClass()); + + @Test + public void instanceMethodName_error() { + helper + .addSourceLines( + "Test.java", + "class Test {", + " static Throwable foo;", + " public Test() {", + " }", + " ", + " // BUG: Diagnostic contains: [NamedLikeContextualKeyword]", + " public void yield() { ", + " foo = new NullPointerException(\"uh oh\");", + " }", + "}") + .setArgs(ImmutableList.of("-XepOpt:NamedLikeContextualKeyword:EnableMethodNames")) + .doTest(); + } + + @Test + public void staticMethodName_error() { + helper + .addSourceLines( + "Test.java", + "class Test {", + " static Throwable foo;", + " public Test() {", + " }", + " ", + " // BUG: Diagnostic contains: [NamedLikeContextualKeyword]", + " public static void yield() { ", + " foo = new NullPointerException(\"uh oh\");", + " }", + "}") + .setArgs(ImmutableList.of("-XepOpt:NamedLikeContextualKeyword:EnableMethodNames")) + .doTest(); + } + + @Test + public void autoOneOfMethodName_noError() { + helper + .addSourceLines( + "Test.java", + "import javax.annotation.processing.Generated;", + "@Generated(\"com.google.auto.value.processor.AutoOneOfProcessor\")", + "class Test {", + " static Throwable foo;", + " public Test() {", + " }", + " ", + " public static void yield() { ", + " foo = new NullPointerException(\"uh oh\");", + " }", + "}") + .setArgs("-XepOpt:NamedLikeContextualKeyword:EnableMethodNames") + .doTest(); + } + + @Test + public void autoValueMethodName_noError() { + helper + .addSourceLines( + "Test.java", + "import javax.annotation.processing.Generated;", + "@Generated(\"com.google.auto.value.processor.AutoValueProcessor\")", + "class Test {", + " static Throwable foo;", + " public Test() {", + " }", + " ", + " public static void yield() { ", + " foo = new NullPointerException(\"uh oh\");", + " }", + "}") + .setArgs("-XepOpt:NamedLikeContextualKeyword:EnableMethodNames") + .doTest(); + } + + @Test + public void generatedButNotAuto_error() { + helper + .addSourceLines( + "Test.java", + "import javax.annotation.processing.Generated;", + "@Generated(\"com.google.foo.Bar\")", + "class Test {", + " static Throwable foo;", + " public Test() {", + " }", + " ", + " // BUG: Diagnostic contains: [NamedLikeContextualKeyword]", + " public static void yield() { ", + " foo = new NullPointerException(\"uh oh\");", + " }", + "}") + .setArgs("-XepOpt:NamedLikeContextualKeyword:EnableMethodNames") + .doTest(); + } + + @Test + public void className_error() { + helper + .addSourceLines( + "module.java", + "// BUG: Diagnostic contains: [NamedLikeContextualKeyword]", + "class module {", + " public module() {", + " }", + "}") + .setArgs(ImmutableList.of("-XepOpt:NamedLikeContextualKeyword:EnableClassNames")) + .doTest(); + } + + @Test + public void yieldInSwitch_noError() { + + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " public Test(int foo) {", + " int x = switch(foo) {", + " case 17: ", + " yield 17;", + " default:", + " yield 0;", + " };", + " }", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:NamedLikeContextualKeyword:EnableMethodNames", + "-XepOpt:NamedLikeContextualKeyword:EnableClassNames")) + .doTest(); + } + + @Test + public void interfaceImplementation_noError() { + helper + .addSourceLines( + "Test.java", + "class Test implements RegrettablyNamedInterface {", + " static Throwable foo;", + " public Test() {", + " }", + " ", + " public void yield() { ", + " foo = new NullPointerException(\"uh oh\");", + " }", + "}") + .addSourceLines( + "RegrettablyNamedInterface.java", + "interface RegrettablyNamedInterface {", + " @SuppressWarnings(\"NamedLikeContextualKeyword\")", + " void yield();", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:NamedLikeContextualKeyword:EnableMethodNames", + "-XepOpt:NamedLikeContextualKeyword:EnableClassNames")) + .doTest(); + } + + @Test + public void nonAnnotatedOverride_noError() { + helper + .addSourceLines( + "Test.java", + "class Test extends RegrettablyNamedClass {", + " static Throwable foo;", + " public Test() {", + " }", + " ", + " public void yield() { ", + " foo = new NullPointerException(\"uh oh\");", + " }", + "}") + .addSourceLines( + "RegrettablyNamedClass.java", + "class RegrettablyNamedClass {", + " @SuppressWarnings(\"NamedLikeContextualKeyword\")", + " void yield() {}", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:NamedLikeContextualKeyword:EnableMethodNames", + "-XepOpt:NamedLikeContextualKeyword:EnableClassNames")) + .doTest(); + } + + @Test + public void annotatedOverride_noError() { + helper + .addSourceLines( + "Test.java", + "class Test extends RegrettablyNamedClass {", + " static Throwable foo;", + " public Test() {", + " }", + " ", + " @Override", + " public void yield() { ", + " foo = new NullPointerException(\"uh oh\");", + " }", + "}") + .addSourceLines( + "RegrettablyNamedClass.java", + "class RegrettablyNamedClass {", + " @SuppressWarnings(\"NamedLikeContextualKeyword\")", + " void yield() {}", + "}") + .setArgs( + ImmutableList.of( + "-XepOpt:NamedLikeContextualKeyword:EnableMethodNames", + "-XepOpt:NamedLikeContextualKeyword:EnableClassNames")) + .doTest(); + } + + @Test + public void positive() { + helper + .addSourceLines( + "Test.java", // + "class Test {", + " // BUG: Diagnostic contains: [NamedLikeContextualKeyword]", + " void yield() {}", + " {", + " // BUG: Diagnostic contains: this.yield();", + " yield();", + " }", + "}") + .setArgs( + ImmutableList.of( + "--release", + "11", + "-XepOpt:NamedLikeContextualKeyword:EnableMethodNames", + "-XepOpt:NamedLikeContextualKeyword:EnableClassNames")) + .doTest(); + } + + @Test + public void enclosing() { + helper + .addSourceLines( + "Test.java", + "class Test {", + " class A {", + " // BUG: Diagnostic contains: [NamedLikeContextualKeyword]", + " void yield() {}", + " class B {", + " class C {", + " {", + " // BUG: Diagnostic contains: A.this.yield();", + " yield();", + " }", + " }", + " }", + " }", + "}") + .setArgs( + ImmutableList.of( + "--release", + "11", + "-XepOpt:NamedLikeContextualKeyword:EnableMethodNames", + "-XepOpt:NamedLikeContextualKeyword:EnableClassNames")) + .doTest(); + } + + @Test + public void staticMethod() { + helper + .addSourceLines( + "Test.java", + "class Test {", + " // BUG: Diagnostic contains: [NamedLikeContextualKeyword]", + " static void yield() {}", + " static class I {", + " {", + " // BUG: Diagnostic contains: Test.yield();", + " yield();", + " }", + " }", + "}") + .setArgs( + ImmutableList.of( + "--release", + "11", + "-XepOpt:NamedLikeContextualKeyword:EnableMethodNames", + "-XepOpt:NamedLikeContextualKeyword:EnableClassNames")) + .doTest(); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/NonApiTypeTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/NonApiTypeTest.java new file mode 100644 index 00000000000..e68a3d659c0 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/NonApiTypeTest.java @@ -0,0 +1,264 @@ +/* + * Copyright 2023 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 com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link NonApiType}. */ +@RunWith(JUnit4.class) +public final class NonApiTypeTest { + private final CompilationTestHelper helper = + CompilationTestHelper.newInstance(NonApiType.class, getClass()) + // Indicate that we're compiling "publicly visible code" + // See ErrorProneOptions.COMPILING_PUBLICLY_VISIBLE_CODE + .setArgs("-XepCompilingPubliclyVisibleCode"); + + @Test + public void listImplementations() { + helper + .addSourceLines( + "Test.java", + "public class Test {", + " // BUG: Diagnostic contains: java.util.List", + " private void test1(java.util.ArrayList value) {}", + " // BUG: Diagnostic contains: java.util.List", + " private void test1(java.util.LinkedList value) {}", + "}") + .doTest(); + } + + @Test + public void setImplementations() { + helper + .addSourceLines( + "Test.java", + "public class Test {", + " // BUG: Diagnostic contains: java.util.Set", + " private void test1(java.util.HashSet value) {}", + " // BUG: Diagnostic contains: java.util.Set", + " private void test1(java.util.LinkedHashSet value) {}", + " // BUG: Diagnostic contains: java.util.Set", + " private void test1(java.util.TreeSet value) {}", + "}") + .doTest(); + } + + @Test + public void mapImplementations() { + helper + .addSourceLines( + "Test.java", + "public class Test {", + " // BUG: Diagnostic contains: java.util.Map", + " private void test1(java.util.HashMap value) {}", + " // BUG: Diagnostic contains: java.util.Map", + " private void test1(java.util.LinkedHashMap value) {}", + " // BUG: Diagnostic contains: java.util.Map", + " private void test1(java.util.TreeMap value) {}", + "}") + .doTest(); + } + + @Test + public void guavaOptionals() { + helper + .addSourceLines( + "Test.java", + "import com.google.common.base.Optional;", + "public class Test {", + " // BUG: Diagnostic contains: java.util.Optional", + " public Optional middleName() { return Optional.of(\"alfred\"); }", + " // BUG: Diagnostic contains: java.util.Optional", + " public void setMiddleName(Optional middleName) {}", + "}") + .doTest(); + } + + @Test + public void jdkOptionals() { + helper + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "public class Test {", + " public Optional middleName() { return Optional.of(\"alfred\"); }", + " // BUG: Diagnostic contains: Avoid Optional parameters", + " public void setMiddleName(Optional middleName) {}", + "}") + .doTest(); + } + + @Test + public void immutableFoos() { + helper + .addSourceLines( + "Test.java", + "import com.google.common.collect.ImmutableCollection;", + "import com.google.common.collect.ImmutableList;", + "import com.google.common.collect.ImmutableMap;", + "import com.google.common.collect.ImmutableSet;", + "public class Test {", + // ImmutableFoos as return types are great! + " public ImmutableCollection testImmutableCollection() { return null; }", + " public ImmutableList testImmutableList() { return null; }", + " public ImmutableMap testImmutableMap() { return null; }", + " public ImmutableSet testImmutableSet() { return null; }", + // ImmutableFoos as method parameters are less great... + " // BUG: Diagnostic contains: java.util.Collection", + " public void test1(ImmutableCollection values) {}", + " // BUG: Diagnostic contains: java.util.List", + " public void test2(ImmutableList values) {}", + " // BUG: Diagnostic contains: java.util.Map", + " public void test3(ImmutableMap values) {}", + " // BUG: Diagnostic contains: java.util.Set", + " public void test4(ImmutableSet values) {}", + "}") + .doTest(); + } + + @Test + public void primitiveArrays() { + helper + .addSourceLines( + "Test.java", + "public class Test {", + " // BUG: Diagnostic contains: ImmutableIntArray", + " public int[] testInts() { return null; }", + " // BUG: Diagnostic contains: ImmutableDoubleArray", + " public void testDoubles1(double[] values) {}", + " // BUG: Diagnostic contains: ImmutableDoubleArray", + " public void testDoubles2(Double[] values) {}", + "}") + .doTest(); + } + + @Test + public void protoTime() { + helper + .addSourceLines( + "Test.java", + "import com.google.protobuf.Duration;", + "import com.google.protobuf.Timestamp;", + "public class Test {", + " // BUG: Diagnostic contains: java.time.Duration", + " public Duration test() { return null; }", + " // BUG: Diagnostic contains: java.time.Instant", + " public void test(Timestamp timestamp) {}", + "}") + .doTest(); + } + + @Test + public void varargs() { + helper + .addSourceLines( + "Test.java", + "import com.google.protobuf.Timestamp;", + "public class Test {", + // TODO(kak): we should _probably_ flag this too + " public void test(Timestamp... timestamps) {}", + "}") + .doTest(); + } + + @Test + public void typeArguments() { + helper + .addSourceLines( + "Test.java", + "import com.google.protobuf.Timestamp;", + "import java.util.List;", + "import java.util.Map;", + "public class Test {", + " // BUG: Diagnostic contains: java.time.Instant", + " public void test1(List timestamps) {}", + " // BUG: Diagnostic contains: java.time.Instant", + " public void test2(List> timestamps) {}", + "}") + .doTest(); + } + + @Test + public void nonPublicApisInPublicClassAreNotFlagged() { + helper + .addSourceLines( + "Test.java", + "import com.google.protobuf.Timestamp;", + "public class Test {", + " void test1(Timestamp timestamp) {}", + " protected void test2(Timestamp timestamp) {}", + " private void test3(Timestamp timestamp) {}", + "}") + .doTest(); + } + + @Test + public void publicApisInNonPublicClassAreNotFlagged() { + helper + .addSourceLines( + "Test.java", + "import com.google.protobuf.Timestamp;", + "class Test {", + " public void test1(Timestamp timestamp) {}", + "}") + .doTest(); + } + + @Test + public void normalApisAreNotFlagged() { + helper + .addSourceLines( + "Test.java", + "public class Test {", + " public Test(int a) {}", + " public int doSomething() { return 42; }", + " public void doSomething(int a) {}", + "}") + .doTest(); + } + + @Test + public void streams() { + helper + .addSourceLines( + "Test.java", + "import java.util.stream.Stream;", + "public class Test {", + " // BUG: Diagnostic contains: NonApiType", + " public Test(Stream iterator) {}", + " // BUG: Diagnostic contains: NonApiType", + " public void methodParam(Stream iterator) {}", + "}") + .doTest(); + } + + @Test + public void iterators() { + helper + .addSourceLines( + "Test.java", + "import java.util.Iterator;", + "public class Test {", + " // BUG: Diagnostic contains: NonApiType", + " public Iterator returnType() { return null; }", + "}") + .doTest(); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/NullableOptionalTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/NullableOptionalTest.java new file mode 100644 index 00000000000..8da09a598ab --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/NullableOptionalTest.java @@ -0,0 +1,160 @@ +/* + * Copyright 2023 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 com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class NullableOptionalTest { + + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(NullableOptional.class, getClass()); + + @Test + public void optionalFieldWithNullableAnnotation_showsError() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "import javax.annotation.Nullable;", + "final class Test {", + " // BUG: Diagnostic contains:", + " @Nullable private Optional foo;", + "}") + .doTest(); + } + + @Test + public void guavaOptionalFieldWithNullableAnnotation_showsError() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.common.base.Optional;", + "import javax.annotation.Nullable;", + "final class Test {", + " @Nullable", + " // BUG: Diagnostic contains:", + " private Optional foo;", + "}") + .doTest(); + } + + @Test + public void methodReturnsOptionalWithNullableAnnotation_showsError() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "import javax.annotation.Nullable;", + "final class Test {", + " @Nullable", + " // BUG: Diagnostic contains:", + " private Optional foo() {", + " return Optional.empty();", + " }", + "}") + .doTest(); + } + + @Test + public void methodReturnsOptionalWithAnotherNullableAnnotation_showsError() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "import org.jspecify.nullness.Nullable;", + "final class Test {", + " @Nullable", + " // BUG: Diagnostic contains:", + " private Optional foo() {", + " return Optional.empty();", + " }", + "}") + .doTest(); + } + + @Test + public void methodHasNullableOptionalAsParameter_showsError() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "import javax.annotation.Nullable;", + "final class Test {", + " // BUG: Diagnostic contains:", + " private void foo(@Nullable Optional optional) {}", + "}") + .doTest(); + } + + @Test + public void objectFieldWithNullableAnnotation_noError() { + compilationHelper + .addSourceLines( + "Test.java", + "import javax.annotation.Nullable;", + "final class Test {", + " @Nullable Object field;", + "}") + .doTest(); + } + + @Test + public void methodReturnsNonOptionalWithNullableAnnotation_noError() { + compilationHelper + .addSourceLines( + "Test.java", + "import javax.annotation.Nullable;", + "final class Test {", + " @Nullable", + " private Object foo() {", + " return null;", + " }", + "}") + .doTest(); + } + + @Test + public void methodReturnsNonOptionalWithAnotherNullableAnnotation_noError() { + compilationHelper + .addSourceLines( + "Test.java", + "import org.jspecify.nullness.Nullable;", + "final class Test {", + " @Nullable", + " private Object foo() {", + " return null;", + " }", + "}") + .doTest(); + } + + @Test + public void methodHasNullableNonOptionalAsParameter_noError() { + compilationHelper + .addSourceLines( + "Test.java", + "import org.jspecify.nullness.Nullable;", + "final class Test {", + " private void foo(@Nullable Object object) {}", + "}") + .doTest(); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesCheckerTest.java new file mode 100644 index 00000000000..ad4be765359 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesCheckerTest.java @@ -0,0 +1,76 @@ +/* + * Copyright 2023 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 com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class OverridingMethodInconsistentArgumentNamesCheckerTest { + + private final CompilationTestHelper testHelper = + CompilationTestHelper.newInstance( + OverridingMethodInconsistentArgumentNamesChecker.class, getClass()); + + @Test + public void positiveSwap() { + testHelper + .addSourceLines("A.java", "class A {", " void m(int p1, int p2) {}", "}") + .addSourceLines( + "B.java", + "class B extends A {", + " @Override", + " // BUG: Diagnostic contains: A consistent order would be: m(p1, p2)", + " void m(int p2, int p1) {}", + "}") + .doTest(); + } + + @Test + public void positivePermutation() { + testHelper + .addSourceLines("A.java", "class A {", " void m(int p1, int p2, int p3) {}", "}") + .addSourceLines( + "B.java", + "class B extends A {", + " @Override", + " // BUG: Diagnostic contains: A consistent order would be: m(p1, p2, p3)", + " void m(int p3, int p1, int p2) {}", + "}") + .doTest(); + } + + @Test + public void negative() { + testHelper + .addSourceLines("A.java", "class A {", " void m(int p1, int p2) {}", "}") + .addSourceLines( + "B.java", "class B extends A {", " @Override", " void m(int p1, int p2) {}", "}") + .doTest(); + } + + @Test + public void negative2() { + testHelper + .addSourceLines("A.java", "class A {", " void m(int p1, int p2) {}", "}") + .addSourceLines( + "B.java", "class B extends A {", " @Override", " void m(int p1, int p3) {}", "}") + .doTest(); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java index 4ea94a8ecd3..3a17a34d683 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitchTest.java @@ -1070,6 +1070,83 @@ public void dynamicWithThrowableDuringInitializationFromMethod_noMatch() { .doTest(); } + @Test + public void switchByEnum_exampleInDocumentation_error() { + // This code appears as an example in the documentation (added surrounding class) + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS};", + " public Test() {}", + " private void foo(Suit suit) {", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(suit) {", + " case HEARTS:", + " System.out.println(\"Red hearts\");", + " break;", + " case DIAMONDS:", + " System.out.println(\"Red diamonds\");", + " break;", + " case SPADES:", + " // Fall through", + " case CLUBS:", + " bar();", + " System.out.println(\"Black suit\");", + " }", + " }", + " private void bar() {}", + "}") + .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion") + .doTest(); + + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS};", + " public Test() {}", + " private void foo(Suit suit) {", + " switch(suit) {", + " case HEARTS:", + " System.out.println(\"Red hearts\");", + " break;", + " case DIAMONDS:", + " System.out.println(\"Red diamonds\");", + " break;", + " case SPADES:", + " // Fall through", + " case CLUBS:", + " bar();", + " System.out.println(\"Black suit\");", + " }", + " }", + " private void bar() {}", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS};", + " public Test() {}", + " private void foo(Suit suit) {", + " switch(suit) {", + " case HEARTS ->", + " System.out.println(\"Red hearts\");", + " case DIAMONDS ->", + " System.out.println(\"Red diamonds\");", + " case SPADES, CLUBS -> {", + " bar();", + " System.out.println(\"Black suit\");", + " }", + " }", + " }", + " private void bar() {}", + "}") + .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion") + .doTest(); + } + /********************************** * * Return switch test cases @@ -2525,6 +2602,77 @@ public void switchByEnum_exhaustiveCompoundAssignmentSwitch_error() { .doTest(); } + @Test + public void switchByEnum_compoundAssignmentExampleInDocumentation_error() { + // This code appears as an example in the documentation (added surrounding class) + assumeTrue(RuntimeVersion.isAtLeast14()); + helper + .addSourceLines( + "Test.java", + "class Test {", + " enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS};", + " int score = 0;", + " public Test() {}", + " private void updateScore(Suit suit) {", + " // BUG: Diagnostic contains: [StatementSwitchToExpressionSwitch]", + " switch(suit) {", + " case HEARTS:", + " // Fall thru", + " case DIAMONDS:", + " score += -1;", + " break;", + " case SPADES:", + " score += 2;", + " break;", + " case CLUBS:", + " score += 3;", + " break;", + " }", + " }", + "}") + .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion") + .doTest(); + + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS};", + " int score = 0;", + " public Test() {}", + " private void updateScore(Suit suit) {", + " switch(suit) {", + " case HEARTS:", + " // Fall thru", + " case DIAMONDS:", + " score += -1;", + " break;", + " case SPADES:", + " score += 2;", + " break;", + " case CLUBS:", + " score += 3;", + " }", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS};", + " int score = 0;", + " public Test() {}", + " private void updateScore(Suit suit) {", + " score += switch(suit) {", + " case HEARTS, DIAMONDS -> -1;", + " case SPADES -> 2;", + " case CLUBS -> 3;", + " };", + " }", + "}") + .setArgs("-XepOpt:StatementSwitchToExpressionSwitch:EnableAssignmentSwitchConversion") + .doTest(); + } + @Test public void switchByEnum_exhaustiveAssignmentSwitchCaseList_error() { // Statement switch has cases with multiple values diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilderTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilderTest.java new file mode 100644 index 00000000000..a022aae7247 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilderTest.java @@ -0,0 +1,167 @@ +/* + * Copyright 2023 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 com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class UnnecessaryStringBuilderTest { + private final BugCheckerRefactoringTestHelper refactoringHelper = + BugCheckerRefactoringTestHelper.newInstance(UnnecessaryStringBuilder.class, getClass()); + private final CompilationTestHelper testHelper = + CompilationTestHelper.newInstance(UnnecessaryStringBuilder.class, getClass()); + + @Test + public void positive() { + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " void f(String hello) {", + " System.err.println(new StringBuilder().append(hello).append(\"world\"));", + " System.err.println(new StringBuilder(hello).append(\"world\"));", + " System.err.println(new StringBuilder(10).append(hello).append(\"world\"));", + " System.err.println(new StringBuilder(hello).append(\"world\").toString());", + " System.err.println(new StringBuilder().toString());", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " void f(String hello) {", + " System.err.println(hello + \"world\");", + " System.err.println(hello + \"world\");", + " System.err.println(hello + \"world\");", + " System.err.println(hello + \"world\");", + " System.err.println(\"\");", + " }", + "}") + .doTest(); + } + + @Test + public void variable() { + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " void f(String hello) {", + " String a = new StringBuilder().append(hello).append(\"world\").toString();", + " StringBuilder b = new StringBuilder().append(hello).append(\"world\");", + " StringBuilder c = new StringBuilder().append(hello).append(\"world\");", + " System.err.println(b);", + " System.err.println(b + \"\");", + " System.err.println(c);", + " c.append(\"goodbye\");", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " void f(String hello) {", + " String a = hello + \"world\";", + " String b = hello + \"world\";", + " StringBuilder c = new StringBuilder().append(hello).append(\"world\");", + " System.err.println(b);", + " System.err.println(b + \"\");", + " System.err.println(c);", + " c.append(\"goodbye\");", + " }", + "}") + .doTest(); + } + + @Test + public void negative() { + testHelper + .addSourceLines( + "Test.java", + "class Test {", + " void f(Iterable xs) {", + " StringBuilder sb = new StringBuilder();", + " for (String s : xs) {", + " sb.append(s);", + " }", + " System.err.println(sb);", + " }", + "}") + .doTest(); + } + + @Test + public void negativeMethodReference() { + testHelper + .addSourceLines( + "Test.java", + "class Test {", + " void f(Iterable xs) {", + " StringBuilder sb = new StringBuilder();", + " xs.forEach(sb::append);", + " System.err.println(sb);", + " }", + "}") + .doTest(); + } + + @Test + public void needsParens() { + refactoringHelper + .addInputLines( + "Test.java", + "abstract class Test {", + " abstract void g(String x);", + " void f(boolean b, String hello) {", + " g(new StringBuilder().append(b ? hello : \"\").append(\"world\").toString());", + " }", + "}") + .addOutputLines( + "Test.java", + "abstract class Test {", + " abstract void g(String x);", + " void f(boolean b, String hello) {", + " g((b ? hello : \"\") + \"world\");", + " }", + "}") + .doTest(); + } + + @Test + public void varType() { + refactoringHelper + .addInputLines( + "Test.java", + "abstract class Test {", + " void f() {", + " var sb = new StringBuilder().append(\"hello\");", + " System.err.println(sb);", + " }", + "}") + .addOutputLines( + "Test.java", + "abstract class Test {", + " void f() {", + " var sb = \"hello\";", + " System.err.println(sb);", + " }", + "}") + .doTest(); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnqualifiedYieldTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnqualifiedYieldTest.java deleted file mode 100644 index 292ad93514a..00000000000 --- a/core/src/test/java/com/google/errorprone/bugpatterns/UnqualifiedYieldTest.java +++ /dev/null @@ -1,85 +0,0 @@ -/* - * Copyright 2013 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 com.google.errorprone.CompilationTestHelper; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -@RunWith(JUnit4.class) -public class UnqualifiedYieldTest { - - private final CompilationTestHelper compilationHelper = - CompilationTestHelper.newInstance(UnqualifiedYield.class, getClass()); - - @Test - public void positive() { - compilationHelper - .addSourceLines( - "Test.java", // - "class Test {", - " void yield() {}", - " {", - " // BUG: Diagnostic contains: this.yield();", - " yield();", - " }", - "}") - .setArgs("--release", "11") - .doTest(); - } - - @Test - public void enclosing() { - compilationHelper - .addSourceLines( - "Test.java", - "class Test {", - " class A {", - " void yield() {}", - " class B {", - " class C {", - " {", - " // BUG: Diagnostic contains: A.this.yield();", - " yield();", - " }", - " }", - " }", - " }", - "}") - .setArgs("--release", "11") - .doTest(); - } - - @Test - public void staticMethod() { - compilationHelper - .addSourceLines( - "Test.java", - "class Test {", - " static void yield() {}", - " static class I {", - " {", - " // BUG: Diagnostic contains: Test.yield();", - " yield();", - " }", - " }", - "}") - .setArgs("--release", "11") - .doTest(); - } -} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java index 0201ea7304b..004ab6db757 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java @@ -1563,4 +1563,26 @@ public void parcelableCreator_noFinding() { "}") .doTest(); } + + @Test + public void nestedParameterRemoval() { + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " private int foo(int a, int b) { return b; }", + " void test() {", + " foo(foo(1, 2), 2);", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " private int foo(int b) { return b; }", + " void test() {", + " foo(2);", + " }", + "}") + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/flogger/FloggerPassedAroundTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/flogger/FloggerPassedAroundTest.java deleted file mode 100644 index cd7db6ae093..00000000000 --- a/core/src/test/java/com/google/errorprone/bugpatterns/flogger/FloggerPassedAroundTest.java +++ /dev/null @@ -1,67 +0,0 @@ -/* - * Copyright 2020 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.flogger; - -import com.google.errorprone.CompilationTestHelper; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests for {@link FloggerPassedAround}. */ -@RunWith(JUnit4.class) -public final class FloggerPassedAroundTest { - private final CompilationTestHelper helper = - CompilationTestHelper.newInstance(FloggerPassedAround.class, getClass()); - - @Test - public void loggerAcceptedAsParameter() { - helper - .addSourceLines( - "Test.java", - "import com.google.common.flogger.FluentLogger;", - "class Test {", - " // BUG: Diagnostic contains:", - " public void test(FluentLogger logger) {}", - "}") - .doTest(); - } - - @Test - public void loggerAcceptedAsParameterToConstructor() { - helper - .addSourceLines( - "Test.java", - "import com.google.common.flogger.FluentLogger;", - "class Test {", - " // BUG: Diagnostic contains:", - " public Test(FluentLogger logger) {}", - "}") - .doTest(); - } - - @Test - public void nonLoggerParameter() { - helper - .addSourceLines( - "Test.java", - "import com.google.common.flogger.FluentLogger;", - "class Test {", - " public Test(int a) {}", - "}") - .doTest(); - } -} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/flogger/FloggerRequiredModifiersTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/flogger/FloggerRequiredModifiersTest.java index aebc0737eb4..257a08c64e8 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/flogger/FloggerRequiredModifiersTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/flogger/FloggerRequiredModifiersTest.java @@ -16,9 +16,7 @@ package com.google.errorprone.bugpatterns.flogger; -import com.google.common.collect.ImmutableList; import com.google.errorprone.BugCheckerRefactoringTestHelper; -import com.google.errorprone.bugpatterns.flogger.FloggerRequiredModifiers.Goal; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -26,72 +24,55 @@ /** Tests for {@link FloggerRequiredModifiers}. */ @RunWith(JUnit4.class) public class FloggerRequiredModifiersTest { - private BugCheckerRefactoringTestHelper refactoringHelper(Goal goal) { - return BugCheckerRefactoringTestHelper.newInstance( - new FloggerRequiredModifiers(goal), getClass()); + private BugCheckerRefactoringTestHelper refactoringHelper() { + return BugCheckerRefactoringTestHelper.newInstance(FloggerRequiredModifiers.class, getClass()); } @Test public void negative() { - for (Goal goal : Goal.values()) { - // Re-initialize the bugchecker so we get a fresh run each time - refactoringHelper(goal) - .addInputLines( - "Holder.java", - "import com.google.common.flogger.FluentLogger;", - "class Holder {", - " public FluentLogger logger;", - " public FluentLogger get() {return logger;}", - "}") - .expectUnchanged() - .addInputLines( - "Test.java", - "import com.google.common.flogger.FluentLogger;", - "class Test {", - " private static final FluentLogger logger = FluentLogger.forEnclosingClass();", - " public void log(FluentLogger l) {l.atInfo().log(\"bland\");}", - " public void delegate(Holder h) {h.logger.atInfo().log(\"held\");}", - " public void read(Holder h) {h.get().atInfo().log(\"got\");}", - "}") - .expectUnchanged() - .doTest(); - } - } - - @Test - public void positive_addsStatic() { - refactoringHelper(Goal.ADD_STATIC) + refactoringHelper() .addInputLines( - "in/Test.java", + "Holder.java", "import com.google.common.flogger.FluentLogger;", - "class Test {", - " final FluentLogger logger = FluentLogger.forEnclosingClass();", + "class Holder {", + " public FluentLogger logger;", + " public FluentLogger get() {return logger;}", "}") - .addOutputLines( - "out/Test.java", + .expectUnchanged() + .addInputLines( + "Test.java", "import com.google.common.flogger.FluentLogger;", "class Test {", - " static final FluentLogger logger = FluentLogger.forEnclosingClass();", + " private static final FluentLogger logger = FluentLogger.forEnclosingClass();", + " public void log(FluentLogger l) {l.atInfo().log(\"bland\");}", + " public void delegate(Holder h) {h.logger.atInfo().log(\"held\");}", + " public void read(Holder h) {h.get().atInfo().log(\"got\");}", "}") + .expectUnchanged() .doTest(); } @Test - public void negative_leavesNonFinalNonStatic() { - refactoringHelper(Goal.ADD_STATIC) + public void positive_addsStatic() { + refactoringHelper() .addInputLines( "in/Test.java", "import com.google.common.flogger.FluentLogger;", "class Test {", - " private FluentLogger logger = FluentLogger.forEnclosingClass();", + " final FluentLogger logger = FluentLogger.forEnclosingClass();", + "}") + .addOutputLines( + "out/Test.java", + "import com.google.common.flogger.FluentLogger;", + "class Test {", + " private static final FluentLogger logger = FluentLogger.forEnclosingClass();", "}") - .expectUnchanged() .doTest(); } @Test public void positive_extractsExpression() { - refactoringHelper(Goal.HOIST_CONSTANT_EXPRESSIONS) + refactoringHelper() .addInputLines( "in/Test.java", "import com.google.common.flogger.FluentLogger;", @@ -110,7 +91,7 @@ public void positive_extractsExpression() { @Test public void negative_doesntCreateSelfAssignment() { - refactoringHelper(Goal.HOIST_CONSTANT_EXPRESSIONS) + refactoringHelper() .addInputLines( "in/Test.java", "import com.google.common.flogger.FluentLogger;", @@ -127,7 +108,7 @@ public void negative_doesntCreateSelfAssignment() { @Test public void negative_doesntIndirectWrappers() { - refactoringHelper(Goal.HOIST_CONSTANT_EXPRESSIONS) + refactoringHelper() .addInputLines( "in/Test.java", "import com.google.common.flogger.FluentLogger;", @@ -135,21 +116,28 @@ public void negative_doesntIndirectWrappers() { " private static FluentLogger logger = register(FluentLogger.forEnclosingClass());", " private static T register(T t) {return t;}", "}") - .expectUnchanged() + .addOutputLines( + "out/Test.java", + "import com.google.common.flogger.FluentLogger;", + "class Test {", + " private static final FluentLogger logger =" + + " register(FluentLogger.forEnclosingClass());", + " private static T register(T t) {return t;}", + "}") .doTest(); } // People who do this generally do it for good reason, and for interfaces it's required. @Test public void negative_allowsSiblingLoggerUse() { - refactoringHelper(Goal.REHOME_FOREIGN_LOGGERS) + refactoringHelper() .addInputLines( "in/Test.java", "import com.google.common.flogger.FluentLogger;", "class Test {", " static class A { public A() {B.logger.atInfo().log();}}", " static class B {", - " private static FluentLogger logger = FluentLogger.forEnclosingClass();", + " private static final FluentLogger logger = FluentLogger.forEnclosingClass();", " }", "}") .expectUnchanged() @@ -158,7 +146,7 @@ public void negative_allowsSiblingLoggerUse() { @Test public void positive_hidesLoggersFromInterfaces() { - refactoringHelper(Goal.HIDE_LOGGERS_IN_INTERFACES) + refactoringHelper() .addInputLines( "in/Test.java", "import com.google.common.flogger.FluentLogger;", @@ -183,7 +171,7 @@ public void positive_hidesLoggersFromInterfaces() { @Test public void positive_extractsHiddenLoggersForInterfaces() { - refactoringHelper(Goal.HOIST_CONSTANT_EXPRESSIONS) + refactoringHelper() .addInputLines( "in/Test.java", "import com.google.common.flogger.FluentLogger;", @@ -207,7 +195,7 @@ public void positive_extractsHiddenLoggersForInterfaces() { @Test public void positive_fixesVisibility() { - refactoringHelper(Goal.MAKE_PRIVATE) + refactoringHelper() .addInputLines( "in/Test.java", "import com.google.common.flogger.FluentLogger;", @@ -225,7 +213,7 @@ public void positive_fixesVisibility() { @Test public void positive_goalsDontConflict() { - refactoringHelper(Goal.DEFAULT_ALL_GOALS) + refactoringHelper() .addInputLines( "in/Test.java", "import com.google.common.flogger.FluentLogger;", @@ -236,26 +224,22 @@ public void positive_goalsDontConflict() { "out/Test.java", "import com.google.common.flogger.FluentLogger;", "class Test {", - " private static final FluentLogger logger = FluentLogger.forEnclosingClass();", + " private final FluentLogger logger = FluentLogger.forEnclosingClass();", "}") .doTest(); } @Test public void positive_replacesInheritedLogger() { - refactoringHelper(Goal.REHOME_FOREIGN_LOGGERS) + refactoringHelper() .addInputLines( "in/Parent.java", "import com.google.common.flogger.FluentLogger;", + "@SuppressWarnings(\"FloggerRequiredModifiers\")", "class Parent {", " protected static final FluentLogger logger = FluentLogger.forEnclosingClass();", "}") - .addOutputLines( - "out/Parent.java", - "import com.google.common.flogger.FluentLogger;", - "class Parent {", - " protected static final FluentLogger logger = FluentLogger.forEnclosingClass();", - "}") + .expectUnchanged() .addInputLines( "in/Child.java", "class Child extends Parent {", @@ -277,10 +261,11 @@ public void positive_replacesInheritedLogger() { @Test public void positive_doesntCreateSelfReference() { - refactoringHelper(Goal.REHOME_FOREIGN_LOGGERS) + refactoringHelper() .addInputLines( "in/Parent.java", "import com.google.common.flogger.FluentLogger;", + "@SuppressWarnings(\"FloggerRequiredModifiers\")", "class Parent {", " protected static final FluentLogger logger = FluentLogger.forEnclosingClass();", "}") @@ -296,7 +281,6 @@ public void positive_doesntCreateSelfReference() { "out/Child.java", "import com.google.common.flogger.FluentLogger;", "class Child extends Parent {", - // Not an ideal fix, but good enough " private static final FluentLogger flogger = FluentLogger.forEnclosingClass();", " private static final FluentLogger logger = flogger;", " Child() {logger.atInfo().log(\"child\");}", @@ -306,7 +290,7 @@ public void positive_doesntCreateSelfReference() { @Test public void positive_handlesRewritesInMultipleFiles() { - refactoringHelper(Goal.HIDE_LOGGERS_IN_INTERFACES) + refactoringHelper() .addInputLines( "in/Parent.java", "import com.google.common.flogger.FluentLogger;", @@ -352,7 +336,7 @@ public void positive_handlesRewritesInMultipleFiles() { @Test public void negative_allowsSiblingLoggers() { - refactoringHelper(Goal.REHOME_FOREIGN_LOGGERS) + refactoringHelper() .addInputLines( "in/Test.java", "import com.google.common.flogger.FluentLogger;", @@ -368,22 +352,19 @@ public void negative_allowsSiblingLoggers() { @Test public void negative_doesntNeedlesslyMoveLoggersToInterfaces() { - for (Goal goal : - ImmutableList.of(Goal.REHOME_FOREIGN_LOGGERS, Goal.HIDE_LOGGERS_IN_INTERFACES)) { - refactoringHelper(goal) - .addInputLines( - "in/Test.java", - "import com.google.common.flogger.FluentLogger;", - "interface Test {", - " class Inner {", - " private static final FluentLogger logger = FluentLogger.forEnclosingClass();", - " private static final class MoreInner {", - " private void go() {logger.atInfo().log();}", - " }", - " }", - "}") - .expectUnchanged() - .doTest(); - } + refactoringHelper() + .addInputLines( + "in/Test.java", + "import com.google.common.flogger.FluentLogger;", + "interface Test {", + " class Inner {", + " private static final FluentLogger logger = FluentLogger.forEnclosingClass();", + " private static final class MoreInner {", + " private void go() {logger.atInfo().log();}", + " }", + " }", + "}") + .expectUnchanged() + .doTest(); } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/javadoc/NotJavadocTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/javadoc/NotJavadocTest.java index 0cf56242856..651becd6c32 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/javadoc/NotJavadocTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/javadoc/NotJavadocTest.java @@ -86,6 +86,28 @@ public void notJavadocOnLocalClass() { .doTest(TEXT_MATCH); } + @Test + public void notJavadocWithLotsOfAsterisks() { + helper + .addInputLines( + "Test.java", + "class Test {", + " void test() {", + " /******** Not Javadoc. */", + " class A {}", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " void test() {", + " /* Not Javadoc. */", + " class A {}", + " }", + "}") + .doTest(TEXT_MATCH); + } + @Test public void actuallyJavadoc() { helper @@ -135,4 +157,19 @@ public void moduleLevel() { .expectUnchanged() .doTest(TEXT_MATCH); } + + @Test + public void suppression() { + helper + .addInputLines( + "Test.java", // + "class Test {", + " @SuppressWarnings(\"NotJavadoc\")", + " void test() {", + " /** Not Javadoc. */", + " }", + "}") + .expectUnchanged() + .doTest(TEXT_MATCH); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/DereferenceWithNullBranchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/DereferenceWithNullBranchTest.java new file mode 100644 index 00000000000..4a45db86e8a --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/DereferenceWithNullBranchTest.java @@ -0,0 +1,124 @@ +/* + * Copyright 2023 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 com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** {@link DereferenceWithNullBranch}Test */ +@RunWith(JUnit4.class) +public class DereferenceWithNullBranchTest { + private final CompilationTestHelper helper = + CompilationTestHelper.newInstance(DereferenceWithNullBranch.class, getClass()); + + @Test + public void positive() { + helper + .addSourceLines( + "Foo.java", + "import java.util.Optional;", + "class Foo {", + " void foo(Optional o) {", + " // BUG: Diagnostic contains: ", + " o.orElse(null).intValue();", + " }", + "}") + .doTest(); + } + + @Test + public void positiveTernary() { + helper + .addSourceLines( + "Foo.java", + "import java.util.Optional;", + "class Foo {", + " int foo(String s) {", + " // BUG: Diagnostic contains: ", + " return (s == null) ? s.length() : 0;", + " }", + "}") + .doTest(); + } + + @Test + public void negativeNoNullBranch() { + helper + .addSourceLines( + "Foo.java", + "import java.util.Optional;", + "class Foo {", + " void foo() {", + " Optional.of(7).get().intValue();", + " }", + "}") + .doTest(); + } + + @Test + public void negativeVoid() { + helper + .addSourceLines( + "Foo.java", + "class Foo {", + " void foo() {", + " Class c;", + " c = Void.class;", + " c = void.class;", + " c = Void.TYPE;", + " }", + "}") + .doTest(); + } + + @Test + public void noCrashOnQualifiedClass() { + helper + .addSourceLines( + "Foo.java", // + "class Foo {", + " class Bar {}", + " Foo.Bar bar;", + "}") + .doTest(); + } + + @Test + public void noCrashOnQualifiedInterface() { + helper + .addSourceLines( + "Foo.java", + "import java.util.Map;", + "interface Foo {", + " void foo(Map.Entry o);", + "}") + .doTest(); + } + + @Test + public void noCrashOnModule() { + helper + .addSourceLines( + "module-info.java", // + "module foo.bar {", + " requires java.logging;", + "}") + .doTest(); + } +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java index 468a33f168f..37daeb2cb79 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java @@ -584,6 +584,22 @@ public void orElseNull() { .doTest(); } + @Test + public void emptyToNull() { + createCompilationTestHelper() + .addSourceLines( + "com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java", + "package com.google.errorprone.bugpatterns.nullness;", + "import static com.google.common.base.Strings.emptyToNull;", + "class LiteralNullReturnTest {", + " public String getMessage(String s) {", + " // BUG: Diagnostic contains: @Nullable", + " return emptyToNull(s);", + " }", + "}") + .doTest(); + } + @Test public void implementsMap() { createCompilationTestHelper() @@ -1373,6 +1389,22 @@ public void negativeCases_unreachableFail() { .doTest(); } + @Test + public void negativeCases_unreachableFailNonCanonicalImport() { + createCompilationTestHelper() + .addSourceLines( + "com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java", + "package com.google.errorprone.bugpatterns.nullness;", + "import static junit.framework.TestCase.fail;", + "class LiteralNullReturnTest {", + " public String getMessage() {", + " fail();", + " return null;", + " }", + "}") + .doTest(); + } + @Test public void negativeCases_unreachableThrowExceptionMethod() { createCompilationTestHelper() diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderNegativeCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderNegativeCases.java index f46ba040188..849083ccc24 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderNegativeCases.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderNegativeCases.java @@ -16,23 +16,19 @@ package com.google.errorprone.bugpatterns.testdata; -import java.net.MalformedURLException; -import java.net.URL; -import java.net.URLClassLoader; +import java.security.SecureClassLoader; class BanClassLoaderPositiveCases { - public static final Class overrideURLClassLoader() - throws ClassNotFoundException, MalformedURLException { - URLClassLoader loader = - new URLClassLoader(new URL[] {new URL("eval.com")}) { - @Override - protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { - return super.loadClass(name); - } - }; + /** OK to extend SecureClassLoader */ + class AnotherSecureClassLoader extends SecureClassLoader {} + + /** OK to call loadClass if it's not on RMIClassLoader */ + public final Class overrideClassLoader() throws ClassNotFoundException { + SecureClassLoader loader = new AnotherSecureClassLoader(); return loader.loadClass("BadClass"); } + /** OK to define loadClass */ private class NotClassLoader { protected void loadClass() {} } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases.java index 3b2ba7aafd7..a895af2e9b9 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases.java @@ -24,17 +24,21 @@ import java.net.URLClassLoader; class BanClassLoaderPositiveCases { - /** Load a class using URLClassLoader. */ - public static final Class find() throws ClassNotFoundException, MalformedURLException { - URLClassLoader loader = - new URLClassLoader(new URL[] {new URL("eval.com")}) { - @Override - protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { - // BUG: Diagnostic contains: BanClassLoader - return findClass(name); - } - }; - return loader.loadClass("BadClass"); + /** Override loadClass with an insecure implementation. */ + // BUG: Diagnostic contains: BanClassLoader + class InsecureClassLoader extends URLClassLoader { + public InsecureClassLoader() { + super(new URL[0]); + } + + @Override + protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { + try { + addURL(new URL("jar:https://evil.com/bad.jar")); + } catch (MalformedURLException e) { + } + return findClass(name); + } } /** Calling static methods in java.rmi.server.RMIClassLoader. */ @@ -43,6 +47,13 @@ public static final Class loadRMI() throws ClassNotFoundException, MalformedU return loadClass("evil.com", "BadClass"); } + /** Calling constructor of java.net.URLClassLoader. */ + public ClassLoader loadFromURL() throws MalformedURLException { + // BUG: Diagnostic contains: BanClassLoader + URLClassLoader loader = new URLClassLoader(new URL[] {new URL("jar:https://evil.com/bad.jar")}); + return loader; + } + /** Calling methods of nested class. */ public static final Class methodHandlesDefineClass(byte[] bytes) throws IllegalAccessException { diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases_expected.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases_expected.java index df6cb2fb838..055fafcf882 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases_expected.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases_expected.java @@ -25,18 +25,22 @@ import java.net.URLClassLoader; class BanClassLoaderPositiveCases { - /** Load a class using URLClassLoader. */ - public static final Class find() throws ClassNotFoundException, MalformedURLException { - URLClassLoader loader = - new URLClassLoader(new URL[] {new URL("eval.com")}) { - @SuppressBanClassLoaderCompletedSecurityReview - @Override - protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { - // BUG: Diagnostic contains: BanClassLoader - return findClass(name); - } - }; - return loader.loadClass("BadClass"); + /** Override loadClass with an insecure implementation. */ + // BUG: Diagnostic contains: BanClassLoader + @SuppressBanClassLoaderCompletedSecurityReview + class InsecureClassLoader extends URLClassLoader { + public InsecureClassLoader() { + super(new URL[0]); + } + + @Override + protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { + try { + addURL(new URL("jar:https://evil.com/bad.jar")); + } catch (MalformedURLException e) { + } + return findClass(name); + } } /** Calling static methods in java.rmi.server.RMIClassLoader. */ @@ -46,6 +50,14 @@ public static final Class loadRMI() throws ClassNotFoundException, MalformedU return loadClass("evil.com", "BadClass"); } + /** Calling constructor of java.net.URLClassLoader. */ + @SuppressBanClassLoaderCompletedSecurityReview + public ClassLoader loadFromURL() throws MalformedURLException { + // BUG: Diagnostic contains: BanClassLoader + URLClassLoader loader = new URLClassLoader(new URL[] {new URL("jar:https://evil.com/bad.jar")}); + return loader; + } + /** Calling methods of nested class. */ @SuppressBanClassLoaderCompletedSecurityReview public static final Class methodHandlesDefineClass(byte[] bytes) diff --git a/core/src/test/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessPropagationTest.java b/core/src/test/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessPropagationTest.java index 96467b29b94..f37615d843b 100644 --- a/core/src/test/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessPropagationTest.java +++ b/core/src/test/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessPropagationTest.java @@ -116,6 +116,7 @@ public static void triggerNullnessCheckerOnPrimitive(short s) {} /** For {@link #testConstantsDefinedInOtherCompilationUnits}. */ public static final String COMPILE_TIME_CONSTANT = "not null"; + /** For {@link #testConstantsDefinedInOtherCompilationUnits} as constant outside compilation. */ @SuppressWarnings("GoodTime") // false positive public static final Integer NOT_COMPILE_TIME_CONSTANT = 421; diff --git a/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java b/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java index bcad4438de3..4b562290964 100644 --- a/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java +++ b/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java @@ -42,12 +42,15 @@ import com.google.errorprone.InvalidCommandLineOptionException; import com.google.errorprone.bugpatterns.ArrayEquals; import com.google.errorprone.bugpatterns.BadShiftAmount; +import com.google.errorprone.bugpatterns.BooleanParameter; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.ChainingConstructorIgnoresParameter; +import com.google.errorprone.bugpatterns.ConstantField; import com.google.errorprone.bugpatterns.DepAnn; import com.google.errorprone.bugpatterns.EqualsIncompatibleType; import com.google.errorprone.bugpatterns.LongLiteralLowerCaseSuffix; import com.google.errorprone.bugpatterns.MethodCanBeStatic; +import com.google.errorprone.bugpatterns.MissingBraces; import com.google.errorprone.bugpatterns.PackageLocation; import com.google.errorprone.bugpatterns.ReferenceEquality; import com.google.errorprone.bugpatterns.StaticQualifiedUsingExpression; @@ -557,6 +560,30 @@ public void allChecksAsWarningsWorks() { assertScanner(withOverrides).hasSeverities(expectedSeverities); } + @Test + public void allSuggestionsAsWarnings() { + ScannerSupplier ss = + ScannerSupplier.fromBugCheckerClasses( + BooleanParameter.class, ConstantField.class, MissingBraces.class); + + assertScanner(ss) + .hasEnabledChecks(BooleanParameter.class, ConstantField.class, MissingBraces.class); + + ErrorProneOptions epOptions = + ErrorProneOptions.processArgs(ImmutableList.of("-XepAllSuggestionsAsWarnings")); + + ImmutableMap expectedSeverities = + ImmutableMap.of( + "BooleanParameter", + SeverityLevel.WARNING, + "ConstantField", + SeverityLevel.WARNING, + "MissingBraces", + SeverityLevel.WARNING); + + assertScanner(ss.applyOverrides(epOptions)).hasSeverities(expectedSeverities); + } + @Test public void canSuppressViaAltName() { ScannerSupplier ss = ScannerSupplier.fromBugCheckerClasses(WithAltName.class); diff --git a/docgen/pom.xml b/docgen/pom.xml index a57b821986f..8cddb3257e1 100644 --- a/docgen/pom.xml +++ b/docgen/pom.xml @@ -89,7 +89,7 @@ org.yaml snakeyaml - 1.30 + 2.0 junit diff --git a/docgen_processor/src/main/java/com/google/errorprone/DocGenProcessor.java b/docgen_processor/src/main/java/com/google/errorprone/DocGenProcessor.java index 59ecddae763..02cd5b84650 100644 --- a/docgen_processor/src/main/java/com/google/errorprone/DocGenProcessor.java +++ b/docgen_processor/src/main/java/com/google/errorprone/DocGenProcessor.java @@ -20,9 +20,14 @@ import com.google.auto.service.AutoService; import com.google.gson.Gson; +import java.io.BufferedWriter; import java.io.IOException; +import java.io.OutputStream; import java.io.OutputStreamWriter; import java.io.PrintWriter; +import java.io.UncheckedIOException; +import java.util.HashMap; +import java.util.Map; import java.util.Set; import javax.annotation.processing.AbstractProcessor; import javax.annotation.processing.ProcessingEnvironment; @@ -51,42 +56,44 @@ public SourceVersion getSupportedSourceVersion() { return SourceVersion.latest(); } - private final Gson gson = new Gson(); - - private PrintWriter pw; + private final Map bugPatterns = new HashMap<>(); /** {@inheritDoc} */ @Override public synchronized void init(ProcessingEnvironment processingEnv) { super.init(processingEnv); - try { - FileObject manifest = - processingEnv - .getFiler() - .createResource(StandardLocation.SOURCE_OUTPUT, "", "bugPatterns.txt"); - pw = new PrintWriter(new OutputStreamWriter(manifest.openOutputStream(), UTF_8), true); - } catch (IOException e) { - throw new RuntimeException(e); - } } /** {@inheritDoc} */ @Override public boolean process(Set annotations, RoundEnvironment roundEnv) { for (Element element : roundEnv.getElementsAnnotatedWith(BugPattern.class)) { - gson.toJson(BugPatternInstance.fromElement(element), pw); - pw.println(); + BugPatternInstance bugPattern = BugPatternInstance.fromElement(element); + bugPatterns.put(bugPattern.name, bugPattern); } if (roundEnv.processingOver()) { - // this was the last round, do cleanup - cleanup(); + try { + FileObject manifest = + processingEnv + .getFiler() + .createResource(StandardLocation.SOURCE_OUTPUT, "", "bugPatterns.txt"); + try (OutputStream os = manifest.openOutputStream(); + PrintWriter pw = + new PrintWriter(new BufferedWriter(new OutputStreamWriter(os, UTF_8)))) { + Gson gson = new Gson(); + bugPatterns.entrySet().stream() + .sorted(Map.Entry.comparingByKey()) + .forEachOrdered( + e -> { + gson.toJson(e.getValue(), pw); + pw.println(); + }); + } + } catch (IOException e) { + throw new UncheckedIOException(e); + } } return false; } - - /** Perform cleanup after last round of annotation processing. */ - private void cleanup() { - pw.close(); - } } diff --git a/docs/bugpattern/AddressSelection.md b/docs/bugpattern/AddressSelection.md new file mode 100644 index 00000000000..37c6905496f --- /dev/null +++ b/docs/bugpattern/AddressSelection.md @@ -0,0 +1,82 @@ +Avoid APIs that convert a hostname to a single IP address: + +* [`java.net.Socket(String,int)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/Socket.html#%3Cinit%3E\(java.lang.String,int,boolean\)) +* [`java.net.InetSocketAddress(String,int)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetSocketAddress.html#%3Cinit%3E\(java.lang.String,int\)) +* [`java.net.InetAddress.html#getByName(String)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetAddress.html#getByName\(java.lang.String\)) + +Depending on the value of the +[`-Djava.net.preferIPv6Addresses=true`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/doc-files/net-properties.html) +system property, those APIs will return an IPv4 or IPv6 address. If a client +only has IPv4 connectivity, it will fail to connect with +`-Djava.net.preferIPv6Addresses=true`. If a client only has IPv6 connectivity, +it will fail to connect with `-Djava.net.preferIPv6Addresses=false`. + +The preferred alternative is for clients to consider all addresses returned by +[`java.net.InetAddress.html#getAllByName(String)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetAddress.html#getAllByName\(java.lang.String\)), +and try to connect to each one until a successful connection is made. + +TIP: To resolve a loopback address, prefer `InetAddress.getLoopbackAddress()` +over hard-coding an IPv4 or IPv6 loopback address with +`InetAddress.getByName("127.0.0.1")` or `InetAddress.getByName("::1")`. + +This is, prefer this: + +```java + Socket doConnect(String hostname, int port) throws IOException { + IOException exception = null; + for (InetAddress address : InetAddress.getAllByName(hostname)) { + try { + return new Socket(address, port); + } catch (IOException e) { + if (exception == null) { + exception = e; + } else { + exception.addSuppressed(e); + } + } + } + throw exception; + } +``` + +```java + Socket doConnect(String hostname, int port) throws IOException { + IOException exception = null; + for (InetAddress address : InetAddress.getAllByName(hostname)) { + try { + Socket s = new Socket(); + s.connect(new InetSocketAddress(address, port)); + return s; + } catch (IOException e) { + if (exception == null) { + exception = e; + } else { + exception.addSuppressed(e); + } + } + } + throw exception; + } +``` + +instead of this: + +```java + Socket doConnect(String hostname, int port) throws IOException { + return new Socket(hostname, port); + } +``` + +```java + void doConnect(String hostname, int port) throws IOException { + Socket s = new Socket(); + s.connect(new InetSocketAddress(hostname, port)); + } +``` + +```java + void doConnect(String hostname, int port) throws IOException { + Socket s = new Socket(); + s.connect(new InetSocketAddress(InetAddress.getByName(hostname), port)); + } +``` diff --git a/docs/bugpattern/AttemptedNegativeZero.md b/docs/bugpattern/AttemptedNegativeZero.md new file mode 100644 index 00000000000..ab89251a3aa --- /dev/null +++ b/docs/bugpattern/AttemptedNegativeZero.md @@ -0,0 +1,9 @@ +Because `0` is an integer constant, `-0` is an integer, too. Integers have no +concept of "negative zero," so it is the same as plain `0`. + +The value is then widened to a floating-point number. And while floating-point +numbers have a concept of "negative zero," the integral `0` is widened to the +floating-point "positive" zero. + +To write a negative zero, you have to write a constant that is a floating-point +number. One simple way to do that is to write `-0.0`. diff --git a/docs/bugpattern/ICCProfileGetInstance.md b/docs/bugpattern/ICCProfileGetInstance.md new file mode 100644 index 00000000000..9c101d0ecf2 --- /dev/null +++ b/docs/bugpattern/ICCProfileGetInstance.md @@ -0,0 +1,5 @@ +`ICC_Profile.getInstance(String)` searches the entire classpath, which is often +unnecessary and can result in slow performance for applications with long +classpaths. Prefer `getInstance(byte[])` or `getInstance(InputStream)` instead. + +See also https://bugs.openjdk.org/browse/JDK-8191622. diff --git a/docs/bugpattern/InlineTrivialConstant.md b/docs/bugpattern/InlineTrivialConstant.md new file mode 100644 index 00000000000..4636fafd369 --- /dev/null +++ b/docs/bugpattern/InlineTrivialConstant.md @@ -0,0 +1,20 @@ +Constants should be given names that emphasize the semantic meaning of the +value. If the name of the constant doesn't convey any information that isn't +clear from the value, consider inlining it. + +For example, prefer this: + +```java +System.err.println(1); +System.err.println(""); +``` + +to this: + +```java +private static final int ONE = 1; +private static final String EMPTY_STRING = ""; +... +System.err.println(ONE); +System.err.println(EMPTY_STRING); +``` diff --git a/docs/bugpattern/LockOnNonEnclosingClassLiteral.md b/docs/bugpattern/LockOnNonEnclosingClassLiteral.md index c6a3b82e2ad..feb089084d9 100644 --- a/docs/bugpattern/LockOnNonEnclosingClassLiteral.md +++ b/docs/bugpattern/LockOnNonEnclosingClassLiteral.md @@ -1,18 +1,33 @@ -Lock on the class other than the enclosing class of the code block can -unintentionally prevent the locked class or other classing using the same lock -being used properly. A issue can be caused when the locked class being edited or -deleted. And it can also be exhausting and time consuming for the others who is -using the same class literal as lock to make sure the synchronized blocks can -work properly as expected. Hence locking on the class other than the enclosing -class of the synchronized code block is disencouraged by the error prone. -Locking on the enclosing class or an instance is a preferred practice. +Having a lock on a class, other than the enclosing class of the code block, can +unintentionally prevent the locked class from being used properly when other +classes effectively lock on the same resource. From a maintainability +perspective, it can be time-consuming to ensure the `synchronized` blocks are +working as expected. Hence, locking on a class other than the enclosing class of +the `synchronized` code block is discouraged by Error Prone. Locking on the +enclosing class or an instance is a preferred practice. -For example, lock on `Other.class` rather than `Example.class` will trigger the -error prone warning: ``` class Example { +For example, a lock on `Other.class` rather than `Example.class` will trigger an +Error Prone warning: -method() { synchronized (Other.class) { } } } ``` +```java +class Example { + void method() { + synchronized (Other.class) { + } + } +} +``` -Lock on instance or the enclosing class of the synchronized code block will not -trigger the warning: ``` class Example { +A lock on the instance or the enclosing class of the `synchronized` code block +will not trigger the warning: -method() { synchronized (Example.class) {} synchronized (this) {} } } ``` +```java +class Example { + void method() { + synchronized (Example.class) { + } + synchronized (this) { + } + } +} +``` diff --git a/docs/bugpattern/MissingRefasterAnnotation.md b/docs/bugpattern/MissingRefasterAnnotation.md new file mode 100644 index 00000000000..4d80d5b3f8d --- /dev/null +++ b/docs/bugpattern/MissingRefasterAnnotation.md @@ -0,0 +1,23 @@ +A Refaster template consists of multiple methods. Typically, each method in the +class has an annotation. If a method has no annotation, this is likely an +oversight. + +```java +static final class MethodLacksBeforeTemplateAnnotation { + @BeforeTemplate + boolean before1(String string) { + return string.equals(""); + } + + // @BeforeTemplate is missing + boolean before2(String string) { + return string.length() == 0; + } + + @AfterTemplate + @AlsoNegation + boolean after(String string) { + return string.isEmpty(); + } +} +``` diff --git a/docs/bugpattern/MutableGuiceModule.md b/docs/bugpattern/MutableGuiceModule.md new file mode 100644 index 00000000000..a617a3f3fa1 --- /dev/null +++ b/docs/bugpattern/MutableGuiceModule.md @@ -0,0 +1 @@ +Guice modules should not be mutable. diff --git a/docs/bugpattern/UnqualifiedYield.md b/docs/bugpattern/NamedLikeContextualKeyword.md similarity index 56% rename from docs/bugpattern/UnqualifiedYield.md rename to docs/bugpattern/NamedLikeContextualKeyword.md index a8eb96fa482..ab6afecc91b 100644 --- a/docs/bugpattern/UnqualifiedYield.md +++ b/docs/bugpattern/NamedLikeContextualKeyword.md @@ -1,3 +1,32 @@ +The problem we're trying to prevent is clashes between the names of +classes/methods and contextual keywords. Clashes can occur when (1.) naming a +class/method, or (2.) when invoking. + +# When naming + +Change problematic names for classes and methods. + +```java +class Foo { + ... + // This can clash with the contextual keyword "yield" + void yield() { + ... + } +} +``` + +Another example: + +```java +// This can clash with Java modules (JPMS) +static class module { + ... +} +``` + +# When invoking + In recent versions of Java, `yield` is a restricted identifier: ```java diff --git a/docs/bugpattern/NullableOptional.md b/docs/bugpattern/NullableOptional.md new file mode 100644 index 00000000000..0a7c3b6a148 --- /dev/null +++ b/docs/bugpattern/NullableOptional.md @@ -0,0 +1,9 @@ +`Optional` is a container object which may or may not contain a value. The +presence or absence of the contained value should be demonstrated by the +`Optional` object itself. + +Using an Optional variable which is expected to possibly be null is discouraged. +An nullable Optional which uses `null` to indicate the absence of the value will +lead to extra work for `null` checking when using the object and even cause +exceptions such as `NullPointerException`. It is best to indicate the absence of +the value by assigning it an empty optional. diff --git a/docs/bugpattern/OverridingMethodInconsistentArgumentNamesChecker.md b/docs/bugpattern/OverridingMethodInconsistentArgumentNamesChecker.md new file mode 100644 index 00000000000..78012784b72 --- /dev/null +++ b/docs/bugpattern/OverridingMethodInconsistentArgumentNamesChecker.md @@ -0,0 +1,14 @@ +Inconsistently ordered parameters in method overrides mostly indicate an +accidental bug in the overriding method. An example for an overriding method +with inconsistent parameter names: + +```java +class A { + public void foo(int foo, int baz) { ... } +} + +class B extends A { + @Override + public void foo(int baz, int foo) { ... } +} +``` diff --git a/docs/bugpattern/StatementSwitchToExpressionSwitch.md b/docs/bugpattern/StatementSwitchToExpressionSwitch.md index 6c9de535cc6..406fab386c3 100644 --- a/docs/bugpattern/StatementSwitchToExpressionSwitch.md +++ b/docs/bugpattern/StatementSwitchToExpressionSwitch.md @@ -34,12 +34,14 @@ enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS}; private void foo(Suit suit) { switch(suit) { case HEARTS: -System.out.println("Red hearts"); + System.out.println("Red hearts"); + break; case DIAMONDS: -System.out.println("Red diamonds"); + System.out.println("Red diamonds"); + break; case SPADES: // Fall through - case DIAMONDS: + case CLUBS: bar(); System.out.println("Black suit"); } @@ -55,7 +57,7 @@ private void foo(Suit suit) { switch(suit) { case HEARTS -> System.out.println("Red hearts"); case DIAMONDS -> System.out.println("Red diamonds"); - case CLUBS, SPADES -> { + case SPADES, CLUBS -> { bar(); System.out.println("Black suit"); } @@ -133,8 +135,10 @@ private void updateScore(Suit suit) { // Fall thru case DIAMONDS: score += -1; + break; case SPADES: score += 2; + break; case CLUBS: score += 3; } @@ -150,7 +154,7 @@ int score = 0; private void updateScore(Suit suit) { score += switch(suit) { - case HEARTS, DIAMONDS -> 1; + case HEARTS, DIAMONDS -> -1; case SPADES -> 2; case CLUBS -> 3; }; diff --git a/docs/bugpattern/javadoc/InvalidBlockTag.md b/docs/bugpattern/javadoc/InvalidBlockTag.md index bce3d081b0b..3833e95aaae 100644 --- a/docs/bugpattern/javadoc/InvalidBlockTag.md +++ b/docs/bugpattern/javadoc/InvalidBlockTag.md @@ -18,11 +18,23 @@ int twoTimes(int n) { } ``` -Note that any Javadoc line starting with `@`, even embedded inside `
` and
-`{@code ...}`, is interpereted as a block tag by the Javadoc parser. As such, if
-you wish your Javadoc to include a code block containing an annotation, you
-should generally avoid `{@code ...}` and instead write the HTML yourself,
-manually escaping the `@` entity.
+Note that any Javadoc line starting with `@`, even embedded inside `
` is
+interpreted as a block tag by the Javadoc parser. As such, if you wish your
+Javadoc to include a code block containing an annotation, you should surround
+the snippet with `{@code ...}`. Alternatively, and if you are using a version of
+javadoc prior to JDK 15, you may escape the symbol using `{@literal @}`
+
+```java
+/**
+ * Designed to be overridden, such as:
+ *
+ * 
{@code
+ * class Foo {
+ *   @Override public String toString() {return "";}
+ * }
+ * }

+ */
+```
 
 ```java
 /**
@@ -30,7 +42,7 @@ manually escaping the `@` entity.
  *
  * 
  * class Foo {
- *   @Override public String toString() {return "";}
+ *   {@literal @}Override public String toString() {return "";}
  * }
  * 

  */
diff --git a/docs/bugpattern/javadoc/NotJavadoc.md b/docs/bugpattern/javadoc/NotJavadoc.md
index 6f2bea726cf..6800fc55395 100644
--- a/docs/bugpattern/javadoc/NotJavadoc.md
+++ b/docs/bugpattern/javadoc/NotJavadoc.md
@@ -1,6 +1,18 @@
 This comment starts with `/**`, but isn't actually Javadoc.
 
-Consider making it a single-line `//` or a multi-line `/*` comment instead.
+Javadoc comments
+[must precede a class, field, constructor, or method declaration](https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#format).
+
+Using `/**` comments in locations where Javadoc is not supported is confusing
+and unnecessary.
+
+Suggested solutions:
+
+*   If the comment is intended to be part of the API documentation, move it to
+    the appropriate class, field, constructor, or method declaration.
+
+*   If the comment is intended to be an implementation comment, use a
+    single-line `//` or a multi-line `/*` comment instead.
 
 ## Suppression
 
diff --git a/pom.xml b/pom.xml
index e778c089e93..248c8b26c22 100644
--- a/pom.xml
+++ b/pom.xml
@@ -29,7 +29,7 @@
 
   
     UTF-8
-    31.1-jre
+    32.1.1-jre
     2.10.0
     1.1.3
     1.0.1
@@ -43,8 +43,8 @@
     3.3.1
     3.2.1
     1.6.13
-    3.19.2
-    1.43.2
+    3.19.6
+    1.43.3
     0.2.0
     5.1.0
   
diff --git a/test_helpers/src/main/java/com/google/errorprone/BugCheckerRefactoringTestHelper.java b/test_helpers/src/main/java/com/google/errorprone/BugCheckerRefactoringTestHelper.java
index caa2768ed0d..4d3405c3286 100644
--- a/test_helpers/src/main/java/com/google/errorprone/BugCheckerRefactoringTestHelper.java
+++ b/test_helpers/src/main/java/com/google/errorprone/BugCheckerRefactoringTestHelper.java
@@ -300,13 +300,13 @@ private JCCompilationUnit doCompile(
                 context);
     Iterable trees = task.parse();
     task.analyze();
-    ImmutableMap byURI =
+    ImmutableMap byUri =
         stream(trees).collect(toImmutableMap(t -> t.getSourceFile().toUri(), t -> t));
-    URI inputURI = input.toUri();
+    URI inputUri = input.toUri();
     assertWithMessage(out + Joiner.on('\n').join(diagnosticsCollector.getDiagnostics()))
-        .that(byURI)
-        .containsKey(inputURI);
-    JCCompilationUnit tree = (JCCompilationUnit) byURI.get(inputURI);
+        .that(byUri)
+        .containsKey(inputUri);
+    JCCompilationUnit tree = (JCCompilationUnit) byUri.get(inputUri);
     Iterable> errorDiagnostics =
         Iterables.filter(
             diagnosticsCollector.getDiagnostics(), d -> d.getKind() == Diagnostic.Kind.ERROR);
diff --git a/test_helpers/src/main/java/com/google/errorprone/CompilationTestHelper.java b/test_helpers/src/main/java/com/google/errorprone/CompilationTestHelper.java
index c8e706aae16..6c58b493562 100644
--- a/test_helpers/src/main/java/com/google/errorprone/CompilationTestHelper.java
+++ b/test_helpers/src/main/java/com/google/errorprone/CompilationTestHelper.java
@@ -193,6 +193,10 @@ public CompilationTestHelper addSourceLines(String path, String... lines) {
    *
    * 
See {@link #addSourceLines} for how expected diagnostics should be specified.
    *
+   * 
For most uses, {@link #addSourceLines} is preferred. Using separate source files to denote
+   * positive/negative examples tends to bloat individual tests. Prefer writing smaller tests using
+   * {@link #addSourceLines} which test a single behaviour in isolation.
+   *
    * @param path the path to the source file
    */
   @CanIgnoreReturnValue
diff --git a/test_helpers/src/main/java/com/google/errorprone/DiagnosticTestHelper.java b/test_helpers/src/main/java/com/google/errorprone/DiagnosticTestHelper.java
index f79e9ae44db..74b2601a49e 100644
--- a/test_helpers/src/main/java/com/google/errorprone/DiagnosticTestHelper.java
+++ b/test_helpers/src/main/java/com/google/errorprone/DiagnosticTestHelper.java
@@ -73,9 +73,9 @@ public DiagnosticTestHelper(String checkName) {
       new ClearableDiagnosticCollector<>();
 
   public static Matcher> suggestsRemovalOfLine(
-      URI fileURI, int line) {
+      URI fileUri, int line) {
     return allOf(
-        diagnosticOnLine(fileURI, line), diagnosticMessage(containsString("remove this line")));
+        diagnosticOnLine(fileUri, line), diagnosticMessage(containsString("remove this line")));
   }
 
   public List> getDiagnostics() {
@@ -136,7 +136,7 @@ public void describeTo(Description description) {
   }
 
   public static Matcher> diagnosticOnLine(
-      URI fileURI, long line) {
+      URI fileUri, long line) {
     return new TypeSafeDiagnosingMatcher>() {
       @Override
       public boolean matchesSafely(
@@ -148,8 +148,8 @@ public boolean matchesSafely(
           return false;
         }
 
-        if (!item.getSource().toUri().equals(fileURI)) {
-          mismatchDescription.appendText("diagnostic not in file ").appendValue(fileURI);
+        if (!item.getSource().toUri().equals(fileUri)) {
+          mismatchDescription.appendText("diagnostic not in file ").appendValue(fileUri);
           return false;
         }
 
@@ -171,7 +171,7 @@ public void describeTo(Description description) {
   }
 
   public static Matcher> diagnosticOnLine(
-      URI fileURI, long line, Predicate matcher) {
+      URI fileUri, long line, Predicate matcher) {
     return new TypeSafeDiagnosingMatcher>() {
       @Override
       public boolean matchesSafely(
@@ -183,8 +183,8 @@ public boolean matchesSafely(
           return false;
         }
 
-        if (!item.getSource().toUri().equals(fileURI)) {
-          mismatchDescription.appendText("diagnostic not in file ").appendValue(fileURI);
+        if (!item.getSource().toUri().equals(fileUri)) {
+          mismatchDescription.appendText("diagnostic not in file ").appendValue(fileUri);
           return false;
         }
 
diff --git a/test_helpers/src/test/java/com/google/errorprone/BugCheckerRefactoringTestHelperTest.java b/test_helpers/src/test/java/com/google/errorprone/BugCheckerRefactoringTestHelperTest.java
index 2fd76ca090e..46de170dd02 100644
--- a/test_helpers/src/test/java/com/google/errorprone/BugCheckerRefactoringTestHelperTest.java
+++ b/test_helpers/src/test/java/com/google/errorprone/BugCheckerRefactoringTestHelperTest.java
@@ -192,6 +192,7 @@ public void annotationFullName() {
         .addOutputLines("out/foo/Bar.java", "import bar.Foo;", "public  class Bar {", "}")
         .doTest(TestMode.TEXT_MATCH);
   }
+
   /** Mock {@link BugChecker} for testing only. */
   @BugPattern(
       summary = "Mock refactoring that replaces all returns with 'return null;' statement.",
@@ -203,6 +204,7 @@ public Description matchReturn(ReturnTree tree, VisitorState state) {
       return describeMatch(tree, SuggestedFix.replace(tree, "return null;"));
     }
   }
+
   /** Mock {@link BugChecker} for testing only. */
   @BugPattern(
       summary = "Mock refactoring that removes all annotations declared in package bar ",