Skip to content

Commit

Permalink
More suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@Stephan202
Stephan202 committed Oct 28, 2024
1 parent d4b4b4b commit e75ff93
Show file tree
Hide file tree
Showing 5 changed files with 103 additions and 27 deletions.
9 changes: 3 additions & 6 deletions ...e-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FormatStringConcatenation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.List;
import java.util.Optional;
import org.jspecify.annotations.Nullable;
import tech.picnic.errorprone.utils.MoreASTHelpers;
import tech.picnic.errorprone.utils.SourceCode;

/**
Expand Down Expand Up @@ -203,14 +204,10 @@ private static boolean hasNonConstantStringConcatenationArgument(

ExpressionTree argument = ASTHelpers.stripParentheses(arguments.get(argPosition));
return argument instanceof BinaryTree
&& isStringTyped(argument, state)
&& MoreASTHelpers.isStringTyped(argument, state)
&& ASTHelpers.constValue(argument, String.class) == null;
}

private static boolean isStringTyped(ExpressionTree tree, VisitorState state) {
return ASTHelpers.isSameType(ASTHelpers.getType(tree), state.getSymtab().stringType, state);
}

private static class ReplacementArgumentsConstructor
extends SimpleTreeVisitor<@Nullable Void, VisitorState> {
private final StringBuilder formatString = new StringBuilder();
Expand All @@ -223,7 +220,7 @@ private static class ReplacementArgumentsConstructor

@Override
public @Nullable Void visitBinary(BinaryTree tree, VisitorState state) {
if (tree.getKind() == Kind.PLUS && isStringTyped(tree, state)) {
if (tree.getKind() == Kind.PLUS && MoreASTHelpers.isStringTyped(tree, state)) {
tree.getLeftOperand().accept(this, state);
tree.getRightOperand().accept(this, state);
} else {
Expand Down
54 changes: 33 additions & 21 deletions ...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import static com.google.errorprone.matchers.Matchers.instanceMethod;
import static com.google.errorprone.matchers.Matchers.staticMethod;
import static com.google.errorprone.matchers.Matchers.toType;
import static java.util.Objects.requireNonNull;
import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL;

import com.google.auto.service.AutoService;
Expand All @@ -30,14 +31,13 @@
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.ModifiersTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.Tree.Kind;
import com.sun.source.tree.VariableTree;
import com.sun.tools.javac.code.Symbol;
import java.util.EnumSet;
import java.util.Objects;
import javax.inject.Inject;
import javax.lang.model.element.Modifier;
import tech.picnic.errorprone.utils.MoreASTHelpers;

/** A {@link BugChecker} that flags non-canonical SLF4J logger declarations. */
@AutoService(BugChecker.class)
Expand All @@ -62,14 +62,14 @@ public final class Slf4jLoggerDeclaration extends BugChecker implements Variable
MethodInvocationTree.class,
allOf(
instanceMethod().anyClass().named("getClass").withNoParameters(),
Slf4jLoggerDeclaration::receiverIsEnclosingClassInstance));
Slf4jLoggerDeclaration::getClassReceiverIsEnclosingClassInstance));
private static final ImmutableSet<Modifier> INSTANCE_DECLARATION_MODIFIERS =
Sets.immutableEnumSet(Modifier.PRIVATE, Modifier.FINAL);
private static final ImmutableSet<Modifier> STATIC_DECLARATION_MODIFIERS =
Sets.immutableEnumSet(Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL);

private final String canonicalStaticName;
private final String canonicalInstanceName;
private final String canonicalStaticFieldName;
private final String canonicalInstanceFieldName;

/** Instantiates a default {@link Slf4jLoggerDeclaration} instance. */
public Slf4jLoggerDeclaration() {
Expand All @@ -83,10 +83,10 @@ public Slf4jLoggerDeclaration() {
*/
@Inject
Slf4jLoggerDeclaration(ErrorProneFlags flags) {
canonicalStaticName =
canonicalStaticFieldName =
flags.get(CANONICAL_STATIC_LOGGER_NAME_FLAG).orElse(DEFAULT_CANONICAL_LOGGER_NAME);
canonicalInstanceName =
CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_CAMEL, canonicalStaticName);
canonicalInstanceFieldName =
CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_CAMEL, canonicalStaticFieldName);
}

@Override
Expand All @@ -104,19 +104,27 @@ public Description matchVariable(VariableTree tree, VisitorState state) {

if (clazz.getModifiers().getFlags().contains(Modifier.ABSTRACT)
&& IS_DYNAMIC_ENCLOSING_CLASS_REFERENCE.matches(factoryArg, state)) {
/*
* While generally we prefer `Logger` declarations to be static and named after their
* enclosing class, we allow one exception: loggers in abstract classes with a name derived
* from `getClass()`.
*/
suggestModifiers(tree, INSTANCE_DECLARATION_MODIFIERS, fix, state);
suggestRename(tree, canonicalInstanceName, fix, state);
suggestRename(tree, canonicalInstanceFieldName, fix, state);
} else {
suggestModifiers(
tree,
clazz.getKind() == Kind.INTERFACE ? ImmutableSet.of() : STATIC_DECLARATION_MODIFIERS,
fix,
state);
suggestRename(tree, canonicalStaticName, fix, state);
suggestRename(tree, canonicalStaticFieldName, fix, state);

if (!ASTHelpers.isSameType(
ASTHelpers.getType(factoryArg), state.getSymtab().stringType, state)
if (!MoreASTHelpers.isStringTyped(factoryArg, state)
&& !IS_STATIC_ENCLOSING_CLASS_REFERENCE.matches(factoryArg, state)) {
/*
* Loggers with a custom string name are generally "special", but those with a name derived
* from a class other than the one that encloses it are likely in error.
*/
fix.merge(SuggestedFix.replace(factoryArg, clazz.getSimpleName() + ".class"));
}
}
Expand All @@ -125,12 +133,12 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
}

private static void suggestModifiers(
Tree tree,
VariableTree tree,
ImmutableSet<Modifier> modifiers,
SuggestedFix.Builder fixBuilder,
VisitorState state) {
ModifiersTree modifiersTree = ASTHelpers.getModifiers(tree);
verify(modifiersTree != null, "Given tree does not support modifiers");
ModifiersTree modifiersTree =
requireNonNull(ASTHelpers.getModifiers(tree), "`VariableTree` must have modifiers");
SuggestedFixes.addModifiers(tree, modifiersTree, state, modifiers).ifPresent(fixBuilder::merge);
SuggestedFixes.removeModifiers(
modifiersTree, state, Sets.difference(EnumSet.allOf(Modifier.class), modifiers))
Expand All @@ -145,15 +153,19 @@ private static void suggestRename(
}

private static boolean isEnclosingClassReference(ExpressionTree tree, VisitorState state) {
ClassTree enclosing = state.findEnclosing(ClassTree.class);
return enclosing != null
&& Objects.equals(ASTHelpers.getSymbol(tree), ASTHelpers.getSymbol(enclosing));
return ASTHelpers.getSymbol(getEnclosingClass(state)).equals(ASTHelpers.getSymbol(tree));
}

private static boolean receiverIsEnclosingClassInstance(
MethodInvocationTree tree, VisitorState state) {
ExpressionTree receiver = ASTHelpers.getReceiver(tree);
private static boolean getClassReceiverIsEnclosingClassInstance(
MethodInvocationTree getClassInvocationTree, VisitorState state) {
ExpressionTree receiver = ASTHelpers.getReceiver(getClassInvocationTree);
if (receiver == null) {
/*
* Method invocations without an explicit receiver either involve static methods (possibly
* statically imported), or instance methods invoked on the enclosing class. As the given
* `getClassInvocationTree` is guaranteed to be a nullary `#getClass()` invocation, the latter
* must be the case.
*/
return true;
}

Expand Down
9 changes: 9 additions & 0 deletions ...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4JLoggerDeclarationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ void identification() {
CompilationTestHelper.newInstance(Slf4jLoggerDeclaration.class, getClass())
.addSourceLines(
"A.java",
"import static java.lang.Class.forName;",
"",
"import org.slf4j.Logger;",
"import org.slf4j.LoggerFactory;",
"",
Expand Down Expand Up @@ -39,6 +41,13 @@ void identification() {
" Logger LOG = LoggerFactory.getLogger(StaticLoggerForInterface.class);",
" }",
"",
" abstract static class DynamicLoggerForWrongTypeWithoutReceiver {",
" // BUG: Diagnostic contains:",
" private final Logger log = LoggerFactory.getLogger(forName(\"A.class\"));",
"",
" DynamicLoggerForWrongTypeWithoutReceiver() throws ClassNotFoundException {}",
" }",
"",
" abstract static class DynamicLoggerForWrongTypeWithoutSymbol {",
" // BUG: Diagnostic contains:",
" private final Logger log = LoggerFactory.getLogger(\"foo\".getClass());",
Expand Down
12 changes: 12 additions & 0 deletions error-prone-utils/src/main/java/tech/picnic/errorprone/utils/MoreASTHelpers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,16 @@ public static Optional<MethodTree> findMethodExitedOnReturn(VisitorState state)
public static boolean areSameType(Tree treeA, Tree treeB, VisitorState state) {
return ASTHelpers.isSameType(ASTHelpers.getType(treeA), ASTHelpers.getType(treeB), state);
}

/**
* Tells whether the given tree is of type {@link String}.
*
* @param tree The tree of interest.
* @param state The {@link VisitorState} describing the context in which the given tree was found.
* @return Whether the specified tree has the same type as {@link
* com.sun.tools.javac.code.Symtab#stringType}.
*/
public static boolean isStringTyped(Tree tree, VisitorState state) {
return ASTHelpers.isSameType(ASTHelpers.getType(tree), state.getSymtab().stringType, state);
}
}
46 changes: 46 additions & 0 deletions error-prone-utils/src/test/java/tech/picnic/errorprone/utils/MoreASTHelpersTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,13 @@
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.ExpressionStatementTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.sun.source.tree.ExpressionStatementTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.ReturnTree;
import com.sun.source.tree.Tree;
Expand Down Expand Up @@ -137,6 +140,25 @@ void areSameType() {
.doTest();
}

@Test
void isStringTyped() {
CompilationTestHelper.newInstance(IsStringTypedTestChecker.class, getClass())
.addSourceLines(
"A.java",
"class A {",
" void m() {",
" int foo = 1;",
" // BUG: Diagnostic contains:",
" String s = \"foo\";",
"",
" hashCode();",
" // BUG: Diagnostic contains:",
" toString();",
" }",
"}")
.doTest();
}

private static String createMethodSearchDiagnosticsMessage(
BiFunction<String, VisitorState, Object> valueFunction, VisitorState state) {
return Maps.toMap(ImmutableSet.of("foo", "bar", "baz"), key -> valueFunction.apply(key, state))
Expand Down Expand Up @@ -224,4 +246,28 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
: Description.NO_MATCH;
}
}

/**
* A {@link BugChecker} that delegates to {@link MoreASTHelpers#isStringTyped(Tree,
* VisitorState)}.
*/
@BugPattern(summary = "Interacts with `MoreASTHelpers` for testing purposes", severity = ERROR)
public static final class IsStringTypedTestChecker extends BugChecker
implements MethodInvocationTreeMatcher, VariableTreeMatcher {
private static final long serialVersionUID = 1L;

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
return getDescription(tree, state);
}

@Override
public Description matchVariable(VariableTree tree, VisitorState state) {
return getDescription(tree, state);
}

private Description getDescription(Tree tree, VisitorState state) {
return MoreASTHelpers.isStringTyped(tree, state) ? describeMatch(tree) : Description.NO_MATCH;
}
}
}

0 comments on commit e75ff93

Please sign in to comment.