Skip to content
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -83,27 +83,21 @@ public Description matchBinary(BinaryTree tree, VisitorState state) {
}

private static boolean isExpressionThis(ExpressionTree tree) {
switch (tree.getKind()) {
case IDENTIFIER:
return ((IdentifierTree) tree).getName().contentEquals("this");
case MEMBER_SELECT:
return ((MemberSelectTree) tree).getIdentifier().contentEquals("this");
default:
return false;
}
return switch (tree.getKind()) {
case IDENTIFIER -> ((IdentifierTree) tree).getName().contentEquals("this");
case MEMBER_SELECT -> ((MemberSelectTree) tree).getIdentifier().contentEquals("this");
default -> false;
};
Comment on lines +86 to +90
Copy link
Member

Choose a reason for hiding this comment

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

Can we please save unrelated refactors for a separate PR? It makes it hard to review this PR.

Copy link
Author

Choose a reason for hiding this comment

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

This was required by error prone checks on the repo. Would you prefer I suppress in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Which check?

Copy link
Member

Choose a reason for hiding this comment

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

Oh this is a new one introduced in Error Prone.

https://errorprone.info/bugpattern/StatementSwitchToExpressionSwitch

Copy link
Contributor

Choose a reason for hiding this comment

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

We should work out if this (and any other introduced check) is something we want to disable, or are comfortable will a load of automated suppressions for them. Perhaps it has autofixes?

Copy link
Member

@pkoenig10 pkoenig10 May 7, 2025

Choose a reason for hiding this comment

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

Perhaps it has autofixes?

This check only reports a match if it has an auto-fix. So I think it should be fine to roll this out.

Copy link
Member

Choose a reason for hiding this comment

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

StringConcatToTextBlock also feels a bit 🌶️, but there are automated fixes for that too. And given that text blocks seem objectively better than string concatenation, I'm inclined to leave that one also.

}

private static Optional<EqualsZeroExpression> getEqualsZeroExpression(BinaryTree tree, VisitorState state) {
ExpressionType ret;
switch (tree.getKind()) {
case EQUAL_TO:
ret = ExpressionType.EQ;
break;
case NOT_EQUAL_TO:
ret = ExpressionType.NEQ;
break;
default:
case EQUAL_TO -> ret = ExpressionType.EQ;
case NOT_EQUAL_TO -> ret = ExpressionType.NEQ;
default -> {
return Optional.empty();
}
}
ExpressionTree leftOperand = tree.getLeftOperand();
ExpressionTree rightOperand = tree.getRightOperand();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,8 @@ public Void visitInstanceOf(InstanceOfTree node, VisitorState state) {

private boolean matchesInstanceOf(InstanceOfTree instanceOfNode, VisitorState state) {
ExpressionTree expression = instanceOfNode.getExpression();
return expression instanceof IdentifierTree
&& ((IdentifierTree) expression).getName().contentEquals(exceptionName)
return expression instanceof IdentifierTree identifierTree
&& identifierTree.getName().contentEquals(exceptionName)
&& !isTypeValid(ASTHelpers.getType(instanceOfNode.getType()), state);
}

Expand Down Expand Up @@ -282,8 +282,8 @@ private static final class AssignmentScanner extends TreeScanner<Void, Void> {
@Override
public Void visitAssignment(AssignmentTree node, Void state) {
ExpressionTree expression = node.getVariable();
if (expression instanceof IdentifierTree
&& ((IdentifierTree) expression).getName().contentEquals(exceptionName)) {
if (expression instanceof IdentifierTree identifierTree
&& identifierTree.getName().contentEquals(exceptionName)) {
variableWasAssigned = true;
}
return super.visitAssignment(node, null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ private static boolean implementsMethod(Types types, Type type, Name methodName,
MethodSymbol equals =
(MethodSymbol) state.getSymtab().objectType.tsym.members().findFirst(methodName);

return !Iterables.isEmpty(ASTHelpers.scope(types.membersClosure(type, false))
return !Iterables.isEmpty(types.membersClosure(type, false)
.getSymbolsByName(methodName, m -> m != equals && m.overrides(equals, type.tsym, types, false)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,7 @@ public Description matchClass(ClassTree classTree, VisitorState state) {

private static boolean hasArrayField(ClassTree classTree, VisitorState state) {
for (Tree member : classTree.getMembers()) {
if (member instanceof VariableTree) {
VariableTree variableTree = (VariableTree) member;

if (member instanceof VariableTree variableTree) {
if (IS_ARRAY_VARIABLE.matches(variableTree, state)) {
return true;
}
Expand All @@ -80,9 +78,7 @@ private static boolean hasNonTrivialEqualsAndHashCode(ClassTree classTree, Visit
boolean hasEquals = false;
boolean hasHashCode = false;
for (Tree member : classTree.getMembers()) {
if (member instanceof MethodTree) {
MethodTree methodTree = (MethodTree) member;

if (member instanceof MethodTree methodTree) {
// We want to check if the equals & hashCode methods have actually been overridden (i.e. don't just
// call Object.equals)
hasEquals = hasEquals || EQUALS_MATCHER.matches(methodTree, state);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,19 @@

import com.google.auto.service.AutoService;
import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.AbstractReturnValueIgnored;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.threadsafety.ConstantExpressions;
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.method.MethodMatchers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import java.util.concurrent.ExecutorService;
import javax.inject.Inject;

@AutoService(BugChecker.class)
@BugPattern(
Expand All @@ -45,6 +48,15 @@ public final class ExecutorSubmitRunnableFutureIgnored extends AbstractReturnVal
.named("submit")
.withParameters(Runnable.class.getName());

public ExecutorSubmitRunnableFutureIgnored() {
super(ConstantExpressions.fromFlags(ErrorProneFlags.empty()));
}

@Inject
ExecutorSubmitRunnableFutureIgnored(ConstantExpressions constantExpressions) {
super(constantExpressions);
}

@Override
public Matcher<? super ExpressionTree> specializedMatcher() {
return MATCHER;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,13 @@
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
linkType = BugPattern.LinkType.CUSTOM,
severity = BugPattern.SeverityLevel.ERROR,
summary = "Supplying an object which uses legacy javax types, such as javax.ws.rs to a\n"
+ "method which requires newer jakarta types is a runtime error. This check ensures\n"
+ "that you only supply proper types to these methods which generally just take an\n"
+ "untyped Object. There is no auto-fix for this check, you must fix it manually")
summary =
"""
Supplying an object which uses legacy javax types, such as javax.ws.rs to a
method which requires newer jakarta types is a runtime error. This check ensures
that you only supply proper types to these methods which generally just take an
untyped Object. There is no auto-fix for this check, you must fix it manually
""")
public final class ForbidJavaxParameterType extends BugChecker implements BugChecker.MethodInvocationTreeMatcher {

private static final String FORBID_JAVAX_TYPE = "com.palantir.errorprone.ForbidJavax";
Expand Down Expand Up @@ -102,8 +105,7 @@ private boolean hasJavaxInclusions(Type resultType, VisitorState state) {
}

private boolean hasJavaxInclusionsOnType(TypeSymbol symbol, VisitorState state) {
if (symbol instanceof ClassSymbol) {
ClassSymbol classType = (ClassSymbol) symbol;
if (symbol instanceof ClassSymbol classType) {

if (IMPLEMENTS_FEATURE.apply(classType.type, state)) {
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.method.MethodMatchers;
import com.google.errorprone.util.ASTHelpers;
import com.google.errorprone.util.ASTHelpers.TargetType;
import com.google.errorprone.util.TargetType;
import com.palantir.baseline.errorprone.safety.Safety;
import com.palantir.baseline.errorprone.safety.SafetyAnalysis;
import com.palantir.baseline.errorprone.safety.SafetyAnnotations;
Expand Down Expand Up @@ -92,8 +92,7 @@ public final class IllegalSafeLoggingArgument extends BugChecker

private static Type resolveParameterType(Type input, ExpressionTree tree, VisitorState state) {
// Important not to call getReceiver/getReceiverType on a NewClassTree, which throws.
if (input instanceof TypeVar && tree instanceof MethodInvocationTree) {
TypeVar typeVar = (TypeVar) input;
if (input instanceof TypeVar typeVar && tree instanceof MethodInvocationTree) {

Type receiver = ASTHelpers.getReceiverType(tree);
if (receiver == null) {
Expand Down Expand Up @@ -472,13 +471,13 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState

Safety resultSafety = Safety.UNKNOWN;
switch (tree.getBodyKind()) {
case EXPRESSION:
case EXPRESSION ->
resultSafety = SafetyAnalysis.of(state.withPath(new TreePath(state.getPath(), tree.getBody())));
break;
case STATEMENT:
case STATEMENT -> {
// Shortcut - statement lambdas get their return type checked in the return statement matcher
// This also allows us to indicate which return statement is bad (if any) rather than the lambda itself
return Description.NO_MATCH;
}
}

if (requiredReturnSafety.allowsValueWith(resultSafety)) {
Expand All @@ -493,7 +492,7 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState
}

private Safety getLambdaRequiredReturnSafety(LambdaExpressionTree tree, VisitorState state) {
TargetType returnType = ASTHelpers.targetType(state.withPath(new TreePath(state.getPath(), tree)));
TargetType returnType = TargetType.targetType(state.withPath(new TreePath(state.getPath(), tree)));
if (returnType == null) {
return Safety.UNKNOWN;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,8 @@ private Set<String> checkInitialization(
if (uninitializedFields.isEmpty()) {
return ImmutableSet.of();
}
if (tree instanceof MethodInvocationTree) {
MethodSymbol methodSymbol = ASTHelpers.getSymbol((MethodInvocationTree) tree);
if (tree instanceof MethodInvocationTree methodInvocationTree) {
MethodSymbol methodSymbol = ASTHelpers.getSymbol(methodInvocationTree);
if (!Objects.equals(methodSymbol.enclClass(), builderClass)) {
// This method belongs to a class other than the builder, so we are at the end of the chain
// If the only thing this method does is construct and return the builder, we continue and require
Expand All @@ -190,8 +190,7 @@ private Set<String> checkInitialization(
builderClass,
immutableClass,
interfaceClass);
} else if (tree instanceof NewClassTree) {
NewClassTree newClassTree = (NewClassTree) tree;
} else if (tree instanceof NewClassTree newClassTree) {
if (newClassTree.getArguments().isEmpty()) {
// The constructor returned the builder (otherwise we would have bailed out in a previous iteration), so
// we should have seen all the field initializations
Expand Down Expand Up @@ -253,12 +252,10 @@ private boolean methodJustConstructsBuilder(
.flatMap(filterByType(ReturnTree.class))
.map(ReturnTree::getExpression)
.map(expressionTree -> {
if (expressionTree instanceof NewClassTree) {
if (expressionTree instanceof NewClassTree newClassTree) {
// To have got here, the return type must be compatible with ImmutableType, so we don't need
// to check the class being constructed
return ((NewClassTree) expressionTree)
.getArguments()
.isEmpty();
return newClassTree.getArguments().isEmpty();
} else if (expressionTree instanceof MethodInvocationTree) {
return Optional.ofNullable(ASTHelpers.getSymbol(expressionTree))
.flatMap(filterByType(MethodSymbol.class))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,13 @@ public final class ImmutablesStyle extends BugChecker implements BugChecker.Clas
public Description matchClass(ClassTree tree, VisitorState state) {
if (STYLE_ANNOTATION.matches(tree, state)) {
switch (tree.getKind()) {
case CLASS:
case INTERFACE:
case CLASS, INTERFACE -> {
return matchStyleAnnotatedType(tree, state);
case ANNOTATION_TYPE:
}
case ANNOTATION_TYPE -> {
return matchStyleMetaAnnotation(tree, state);
default:
break;
}
default -> {}
}
}
return Description.NO_MATCH;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,13 @@ private static List<Type> getReferencedClasses(AnnotationTree tree, VisitorState
return Collections.emptyList();
}

if (value instanceof JCTree.JCFieldAccess) {
return Collections.singletonList(((JCTree.JCFieldAccess) value).selected.type);
if (value instanceof JCTree.JCFieldAccess jcFieldAccess) {
return Collections.singletonList(jcFieldAccess.selected.type);
}

if (value instanceof JCTree.JCNewArray) {
if (value instanceof JCTree.JCNewArray jcNewArray) {
ImmutableList.Builder<Type> list = ImmutableList.builder();
for (JCTree.JCExpression elem : ((JCTree.JCNewArray) value).elems) {
for (JCTree.JCExpression elem : jcNewArray.elems) {
list.add(((JCTree.JCFieldAccess) elem).selected.type);
}
return list.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,29 +65,29 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState
// that we can fix the most basic problems correctly, not to take risks and produce code
// that may not compile.
switch (bodyKind) {
case EXPRESSION:
if (!(body instanceof MethodInvocationTree)) {
case EXPRESSION -> {
if (!(body instanceof MethodInvocationTree methodInvocationTree)) {
return Description.NO_MATCH;
}
return checkMethodInvocation((MethodInvocationTree) body, tree, state);
case STATEMENT:
if (!(body instanceof BlockTree)) {
return checkMethodInvocation(methodInvocationTree, tree, state);
}
case STATEMENT -> {
if (!(body instanceof BlockTree block)) {
return Description.NO_MATCH;
}
BlockTree block = (BlockTree) body;
if (block.getStatements().size() != 1) {
return Description.NO_MATCH;
}
StatementTree statement = block.getStatements().get(0);
if (!(statement instanceof ReturnTree)) {
if (!(statement instanceof ReturnTree returnStatement)) {
return Description.NO_MATCH;
}
ReturnTree returnStatement = (ReturnTree) statement;
ExpressionTree returnExpression = returnStatement.getExpression();
if (!(returnExpression instanceof MethodInvocationTree)) {
if (!(returnExpression instanceof MethodInvocationTree methodInvocationTree)) {
return Description.NO_MATCH;
}
return checkMethodInvocation((MethodInvocationTree) returnExpression, tree, state);
return checkMethodInvocation(methodInvocationTree, tree, state);
}
}
throw new IllegalStateException("Unexpected BodyKind: " + bodyKind);
}
Expand Down Expand Up @@ -291,8 +291,8 @@ private static Optional<String> toMethodReference(String qualifiedMethodName) {
private static boolean isLocal(MethodInvocationTree methodInvocationTree) {
ExpressionTree receiver = ASTHelpers.getReceiver(methodInvocationTree);
return receiver == null
|| (receiver instanceof IdentifierTree
&& "this".equals(((IdentifierTree) receiver).getName().toString()));
|| (receiver instanceof IdentifierTree identifierTree
&& "this".equals(identifierTree.getName().toString()));
}

private static boolean isFinal(Symbol symbol) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,9 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
MethodInvocationTree getLoggerInvocation = (MethodInvocationTree) tree.getInitializer();
ExpressionTree classArgument = getLoggerInvocation.getArguments().get(0);

if (!(classArgument instanceof MemberSelectTree)) {
if (!(classArgument instanceof MemberSelectTree memberSelectTree)) {
return Description.NO_MATCH;
}
MemberSelectTree memberSelectTree = (MemberSelectTree) classArgument;
if (!memberSelectTree.getIdentifier().contentEquals("class")
|| memberSelectTree.getExpression().getKind() != Tree.Kind.IDENTIFIER) {
return Description.NO_MATCH;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState

List<? extends ExpressionTree> args = tree.getArguments();
ExpressionTree argNameExpression = args.get(0);
if (argNameExpression instanceof JCTree.JCLiteral) {
String argName = (String) ((JCTree.JCLiteral) argNameExpression).getValue();
if (argNameExpression instanceof JCTree.JCLiteral jCLiteral) {
String argName = (String) jCLiteral.getValue();
if (unsafeParamNames.stream().anyMatch(unsafeArgName -> unsafeArgName.equalsIgnoreCase(argName))) {
SuggestedFix.Builder builder = SuggestedFix.builder();
String unsafeArg = SuggestedFixes.qualifyType(state, builder, "com.palantir.logsafe.UnsafeArg");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,17 +121,16 @@ static ImmutableList<Type> expandUnion(@Nullable Type type) {
}

public static Type getResultType(Tree tree) {
return tree instanceof ExpressionTree
? ASTHelpers.getResultType((ExpressionTree) tree)
return tree instanceof ExpressionTree expressionTree
? ASTHelpers.getResultType(expressionTree)
: ASTHelpers.getType(tree);
}

public static List<? extends ExpressionTree> unwrapArray(@Nullable ExpressionTree expressionTree) {
if (expressionTree == null) {
return Collections.emptyList();
}
if (expressionTree instanceof NewArrayTree) {
NewArrayTree tree = (NewArrayTree) expressionTree;
if (expressionTree instanceof NewArrayTree tree) {
return tree.getInitializers();
}
return Collections.singletonList(expressionTree);
Expand Down
Loading