From 50333fc497aab5abce1cbf6adb7faa222fc0ee1a Mon Sep 17 00:00:00 2001 From: Marco Kaufmann <83189575+kaufco@users.noreply.github.com> Date: Mon, 22 Apr 2024 12:17:20 +0200 Subject: [PATCH] SONARJAVA-4943 FP on S1144 if private method is referenced by name in annotations (#4776) --- .../main/java/checks/UnusedPrivateMethod.java | 111 +++++++- .../unused/UnusedPrivateMethodCheck.java | 263 ++++++++++-------- 2 files changed, 262 insertions(+), 112 deletions(-) diff --git a/java-checks-test-sources/default/src/main/java/checks/UnusedPrivateMethod.java b/java-checks-test-sources/default/src/main/java/checks/UnusedPrivateMethod.java index e2d10796c0d..0013ce7d3d4 100644 --- a/java-checks-test-sources/default/src/main/java/checks/UnusedPrivateMethod.java +++ b/java-checks-test-sources/default/src/main/java/checks/UnusedPrivateMethod.java @@ -16,7 +16,7 @@ @interface Observes {} class UnusedPrivateMethodCheck { - + private void init(@Observes Object object, String test) {} // Noncompliant private void init(@javax.enterprise.event.Observes Object object) {} //Compliant, javax.enterprise.event.Observes is an exception to the rule private void jakartaInit(@jakarta.enterprise.event.Observes Object object) {} //Compliant, jakarta.enterprise.event.Observes is an exception to the rule @@ -234,3 +234,112 @@ public UnusedPrivateMethodCheckMyClass build() { } } } + +class CheckAnnotations { + @interface ProxyMethod { + public String value(); + } + + @interface MethodProvided { + public String value(); + } + + @interface Getter { + public String getterMethod(); + } + + @interface Setter { + public String method(); + } + + @interface ArgumentIsNotAString { + public int method(); + } + + abstract static class MethodReferencedInAnnotation1 { + + private void foo1() {} // Compliant + + @ProxyMethod("foo1") + abstract void bar1(); + } + + @MethodProvided(value = "foo2") + abstract static class MethodReferencedInAnnotation2 { + + private void foo2() {} // Compliant + + @ProxyMethod("foo2") + abstract void bar2(); + + private void baz2() {} // Noncompliant + } + + static class MethodReferencedInAnnotation3 { + + @Getter(getterMethod = "foo3") + int bar3; + + private int foo3() { // Compliant + return 42; + } + } + + static class MethodReferencedInAnnotation4 { + + @Setter(method = "foo4") + int bar4; + + private void foo4(int value) {} // Compliant + + private void bar4(int value) {} // Noncompliant + } + + static class MethodReferencedInAnnotation5 { + + @Getter(getterMethod = "foo52") + @Setter(method = "foo54") + int bar5; + + private void foo51(int value) {} // Noncompliant + + private int foo52() { // Compliant + return 42; + } + + private void foo53(int value) {} // Noncompliant + + private void foo54(int value) {} // Compliant + + private void foo55(int value) {} // Noncompliant + } + + abstract static class Coverage1 { + + private void foo6() {} // Noncompliant, text blocks are ignored + + @ProxyMethod(""" + foo6""") + abstract void notStringLitNorAssignExpr(); + + @Getter(getterMethod = """ + foo6""") + int assignExprWithNoStringLit; + } + + @ArgumentIsNotAString(method = 42) + static class Coverage2 { + private void foo7() {} // Noncompliant + } + + @MethodProvided(value = "foo8") + static class TN { + private void foo8() {} // Compliant, method is referenced in annotation + + private void foo8(int param) {} // Compliant, method is referenced in annotation + } + + static class FN { + private void foo8() {} // Noncompliant + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/unused/UnusedPrivateMethodCheck.java b/java-checks/src/main/java/org/sonar/java/checks/unused/UnusedPrivateMethodCheck.java index 05067f974e6..bdfd9c86b42 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/unused/UnusedPrivateMethodCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/unused/UnusedPrivateMethodCheck.java @@ -20,10 +20,12 @@ package org.sonar.java.checks.unused; import java.util.ArrayList; -import java.util.Arrays; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.sonar.check.Rule; import org.sonar.java.checks.helpers.ExpressionsHelper; import org.sonar.java.checks.helpers.QuickFixHelper; @@ -32,12 +34,15 @@ import org.sonar.java.reporting.JavaQuickFix; import org.sonar.java.reporting.JavaTextEdit; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; -import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.semantic.Symbol; import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.AnnotationTree; import org.sonar.plugins.java.api.tree.Arguments; +import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; +import org.sonar.plugins.java.api.tree.ClassTree; import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.LiteralTree; import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; import org.sonar.plugins.java.api.tree.MethodInvocationTree; import org.sonar.plugins.java.api.tree.MethodReferenceTree; @@ -55,33 +60,33 @@ @Rule(key = "S1144") public class UnusedPrivateMethodCheck extends IssuableSubscriptionVisitor { - private final List unusedPrivateMethods = new ArrayList<>(); - private final Set unresolvedMethodNames = new HashSet<>(); - - private static final Set PARAM_ANNOTATION_EXCEPTIONS = Set.of( - "javax.enterprise.event.Observes", - "jakarta.enterprise.event.Observes" - ); - @Override public List nodesToVisit() { - return Arrays.asList( - // declarations - Tree.Kind.METHOD, Tree.Kind.CONSTRUCTOR, - // usages - Tree.Kind.METHOD_INVOCATION, Tree.Kind.METHOD_REFERENCE, Tree.Kind.NEW_CLASS); + return List.of(Tree.Kind.CLASS, Tree.Kind.ENUM); } @Override - public void leaveFile(JavaFileScannerContext context) { - reportUnusedPrivateMethods(); - unusedPrivateMethods.clear(); - unresolvedMethodNames.clear(); + public void visitNode(Tree tree) { + reportUnusedPrivateMethods(findUnusedPrivateMethods((ClassTree) tree)); } - private void reportUnusedPrivateMethods() { - unusedPrivateMethods.stream() - .filter(methodTree -> !unresolvedMethodNames.contains(methodTree.simpleName().name())) + private static Stream findUnusedPrivateMethods(ClassTree tree) { + var collector = new UnusedMethodCollector(); + tree.members().forEach(it -> it.accept(collector)); + var unusedPrivateMethods = collector.unusedPrivateMethods; + if (unusedPrivateMethods.isEmpty()) { + return Stream.empty(); + } + + var methodNames = unusedPrivateMethods.stream().map(it -> it.simpleName().name()).collect(Collectors.toSet()); + methodNames.removeAll(collector.unresolvedMethodNames); + var filter = new MethodsUsedInAnnotationsFilter(methodNames); + tree.accept(filter); + return unusedPrivateMethods.stream().filter(it -> filter.filteredNames.contains(it.simpleName().name())); + } + + private void reportUnusedPrivateMethods(Stream methods) { + methods .forEach(methodTree -> { IdentifierTree simpleName = methodTree.simpleName(); String methodType = methodTree.is(Tree.Kind.CONSTRUCTOR) ? "constructor" : "method"; @@ -96,114 +101,150 @@ private void reportUnusedPrivateMethods() { }); } - @Override - public void visitNode(Tree tree) { - switch (tree.kind()) { - case METHOD: - case CONSTRUCTOR: - checkIfUnused((MethodTree) tree); - break; - case NEW_CLASS: - checkIfUnknown((NewClassTree) tree); - break; - case METHOD_INVOCATION: - checkIfUnknown((MethodInvocationTree) tree); - break; - case METHOD_REFERENCE: - checkIfUnknown((MethodReferenceTree) tree); - break; - default: - // not registered - can not happen + private static class UnusedMethodCollector extends BaseTreeVisitor { + + public final List unusedPrivateMethods = new ArrayList<>(); + public final Set unresolvedMethodNames = new HashSet<>(); + + private static final Set PARAM_ANNOTATION_EXCEPTIONS = Set.of( + "javax.enterprise.event.Observes", + "jakarta.enterprise.event.Observes" + ); + + @Override + public void visitClass(ClassTree tree) { + // cut visitation of inner classes } - } - private void checkIfUnknown(MethodInvocationTree mit) { - String name = ExpressionUtils.methodName(mit).name(); - addIfArgumentsAreUnknown(mit.arguments(), name); - addIfUnknownOrAmbiguous(mit.methodSymbol(), name); - } + @Override + public void visitMethod(MethodTree methodTree) { + super.visitMethod(methodTree); + Symbol symbol = methodTree.symbol(); + if (isUnusedPrivate(symbol) && hasNoAnnotation(methodTree) && (isConstructorWithParameters(methodTree) || isNotMethodFromSerializable(methodTree, symbol))) { + unusedPrivateMethods.add(methodTree); + } + } - private void checkIfUnknown(NewClassTree nct) { - String name = constructorName(nct.identifier()); - addIfArgumentsAreUnknown(nct.arguments(), name); - addIfUnknownOrAmbiguous(nct.methodSymbol(), name); - } + @Override + public void visitMethodInvocation(MethodInvocationTree mit) { + super.visitMethodInvocation(mit); + String name = ExpressionUtils.methodName(mit).name(); + addIfArgumentsAreUnknown(mit.arguments(), name); + addIfUnknownOrAmbiguous(mit.methodSymbol(), name); + } - private void checkIfUnknown(MethodReferenceTree mref) { - IdentifierTree methodIdentifier = mref.method(); - addIfUnknownOrAmbiguous(methodIdentifier.symbol(), methodIdentifier.name()); - } + @Override + public void visitMethodReference(MethodReferenceTree mref) { + super.visitMethodReference(mref); + IdentifierTree methodIdentifier = mref.method(); + addIfUnknownOrAmbiguous(methodIdentifier.symbol(), methodIdentifier.name()); + } - private void addIfArgumentsAreUnknown(Arguments arguments, String name) { - // In case of broken semantic, if the argument is unknown, the method call will not have the correct reference. - if (arguments.stream().anyMatch(arg -> arg.symbolType().isUnknown())) { - unresolvedMethodNames.add(name); + @Override + public void visitNewClass(NewClassTree nct) { + super.visitNewClass(nct); + String name = constructorName(nct.identifier()); + addIfArgumentsAreUnknown(nct.arguments(), name); + addIfUnknownOrAmbiguous(nct.methodSymbol(), name); } - } - private void addIfUnknownOrAmbiguous(Symbol symbol, String name) { - // In case of broken semantic (overload with unknown args), ECJ wrongly link the symbol to the good overload. - if (symbol.isUnknown() || (symbol.isMethodSymbol() && ((Symbol.MethodSymbol) symbol).parameterTypes().stream().anyMatch(Type::isUnknown))) { - unresolvedMethodNames.add(name); + private void addIfArgumentsAreUnknown(Arguments arguments, String name) { + // In case of broken semantic, if the argument is unknown, the method call will not have the correct reference. + if (arguments.stream().anyMatch(arg -> arg.symbolType().isUnknown())) { + unresolvedMethodNames.add(name); + } } - } - private static String constructorName(TypeTree typeTree) { - switch (typeTree.kind()) { - case PARAMETERIZED_TYPE: - return constructorName(((ParameterizedTypeTree) typeTree).type()); - case MEMBER_SELECT: - return ((MemberSelectExpressionTree) typeTree).identifier().name(); - case IDENTIFIER: - return ((IdentifierTree) typeTree).name(); - default: - throw new IllegalStateException("Unexpected TypeTree used as constructor."); + private void addIfUnknownOrAmbiguous(Symbol symbol, String name) { + // In case of broken semantic (overload with unknown args), ECJ wrongly link the symbol to the good overload. + if (symbol.isUnknown() || (symbol.isMethodSymbol() && ((Symbol.MethodSymbol) symbol).parameterTypes().stream().anyMatch(Type::isUnknown))) { + unresolvedMethodNames.add(name); + } } - } - private void checkIfUnused(MethodTree methodTree) { - Symbol symbol = methodTree.symbol(); - if (isUnusedPrivate(symbol) && hasNoAnnotation(methodTree) && (isConstructorWithParameters(methodTree) || isNotMethodFromSerializable(methodTree, symbol))) { - unusedPrivateMethods.add(methodTree); + private static String constructorName(TypeTree typeTree) { + return switch (typeTree.kind()) { + case PARAMETERIZED_TYPE -> constructorName(((ParameterizedTypeTree) typeTree).type()); + case MEMBER_SELECT -> ((MemberSelectExpressionTree) typeTree).identifier().name(); + case IDENTIFIER -> ((IdentifierTree) typeTree).name(); + default -> throw new IllegalStateException("Unexpected TypeTree used as constructor."); + }; } - } - private static boolean isUnusedPrivate(Symbol symbol) { - return symbol.isPrivate() && symbol.usages().isEmpty(); - } + private static boolean isUnusedPrivate(Symbol symbol) { + return symbol.isPrivate() && symbol.usages().isEmpty(); + } - private static boolean hasNoAnnotation(MethodTree methodTree) { - return methodTree.modifiers().annotations().isEmpty() && methodTree.parameters().stream().noneMatch(UnusedPrivateMethodCheck::hasAllowedAnnotation); - } - - private static boolean hasAllowedAnnotation(VariableTree variableTree) { - List annotations = variableTree.modifiers().annotations(); - return !annotations.isEmpty() && annotations.stream().anyMatch(UnusedPrivateMethodCheck::isAllowedAnnotation); - } - - private static boolean isAllowedAnnotation(AnnotationTree annotation) { - Type annotationSymbolType = annotation.symbolType(); - if (PARAM_ANNOTATION_EXCEPTIONS.stream().anyMatch(annotationSymbolType::is)) { - return true; - } - if (annotationSymbolType.isUnknown()) { - TypeTree annotationType = annotation.annotationType(); - if (annotationType.is(Tree.Kind.IDENTIFIER)) { - return "Observes".equals(((IdentifierTree) annotationType).name()); + private static boolean hasNoAnnotation(MethodTree methodTree) { + return methodTree.modifiers().annotations().isEmpty() && methodTree.parameters().stream().noneMatch(UnusedMethodCollector::hasAllowedAnnotation); + } + + private static boolean hasAllowedAnnotation(VariableTree variableTree) { + List annotations = variableTree.modifiers().annotations(); + return !annotations.isEmpty() && annotations.stream().anyMatch(UnusedMethodCollector::isAllowedAnnotation); + } + + private static boolean isAllowedAnnotation(AnnotationTree annotation) { + Type annotationSymbolType = annotation.symbolType(); + if (PARAM_ANNOTATION_EXCEPTIONS.stream().anyMatch(annotationSymbolType::is)) { + return true; } - if (annotationType.is(Tree.Kind.MEMBER_SELECT)) { - String concatenatedAnnotation = ExpressionsHelper.concatenate((MemberSelectExpressionTree) annotationType); - return PARAM_ANNOTATION_EXCEPTIONS.stream().anyMatch(concatenatedAnnotation::equals); + if (annotationSymbolType.isUnknown()) { + TypeTree annotationType = annotation.annotationType(); + if (annotationType.is(Tree.Kind.IDENTIFIER)) { + return "Observes".equals(((IdentifierTree) annotationType).name()); + } + if (annotationType.is(Tree.Kind.MEMBER_SELECT)) { + String concatenatedAnnotation = ExpressionsHelper.concatenate((MemberSelectExpressionTree) annotationType); + return PARAM_ANNOTATION_EXCEPTIONS.stream().anyMatch(concatenatedAnnotation::equals); + } } + return false; } - return false; - } - private static boolean isConstructorWithParameters(MethodTree methodTree) { - return methodTree.is(Tree.Kind.CONSTRUCTOR) && !methodTree.parameters().isEmpty(); + private static boolean isConstructorWithParameters(MethodTree methodTree) { + return methodTree.is(Tree.Kind.CONSTRUCTOR) && !methodTree.parameters().isEmpty(); + } + + private static boolean isNotMethodFromSerializable(MethodTree methodTree, Symbol symbol) { + return methodTree.is(Tree.Kind.METHOD) && !SerializableContract.SERIALIZABLE_CONTRACT_METHODS.contains(symbol.name()); + } } - private static boolean isNotMethodFromSerializable(MethodTree methodTree, Symbol symbol) { - return methodTree.is(Tree.Kind.METHOD) && !SerializableContract.SERIALIZABLE_CONTRACT_METHODS.contains(symbol.name()); + private static class MethodsUsedInAnnotationsFilter extends BaseTreeVisitor { + + public MethodsUsedInAnnotationsFilter(Set methodNames) { + this.filteredNames = methodNames; + } + + private final Set filteredNames; + + private static boolean isNameIndicatingMethod(String name) { + return name.toLowerCase(Locale.getDefault()).contains("method"); + } + + private void removeMethodName(LiteralTree literal) { + filteredNames.remove(removeQuotes(literal.value())); + } + + private static String removeQuotes(String withQuotes) { + return withQuotes.substring(1, withQuotes.length() - 1); + } + + @Override + public void visitAnnotation(AnnotationTree annotationTree) { + var isMethodAnnotation = isNameIndicatingMethod(annotationTree.annotationType().symbolType().name()); + for (var arg : annotationTree.arguments()) { + if (arg.is(Tree.Kind.STRING_LITERAL)) { + if (isMethodAnnotation) { + removeMethodName((LiteralTree) arg); + } + } else if (arg instanceof AssignmentExpressionTree asgn && asgn.expression().is(Tree.Kind.STRING_LITERAL) && ( + isMethodAnnotation || isNameIndicatingMethod(((IdentifierTree) asgn.variable()).name()) + )) { + removeMethodName((LiteralTree) asgn.expression()); + } + } + } } }