diff --git a/java-checks-test-sources/default/src/main/java/checks/RedundantRecordMethodsCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/RedundantRecordMethodsCheckSample.java index 20b15b799b..31b1ccedb1 100644 --- a/java-checks-test-sources/default/src/main/java/checks/RedundantRecordMethodsCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/RedundantRecordMethodsCheckSample.java @@ -165,7 +165,8 @@ public String something() { // Compliant } record GetterWithBranches(String name, int age) { - public String name() { + public String name() { // Noncompliant {{Remove this redundant method which is the same as a default one.}} +// ^^^^ if ((new Random()).nextBoolean()) { return this.name; } else { @@ -228,4 +229,91 @@ public BranchInConstructor2(int arg1, int arg2) { // Compliant } } } + + record AccessorWithLoop(String name) { + public String name() { // Noncompliant {{Remove this redundant method which is the same as a default one.}} +// ^^^^ + while (true) { + return name; + } + } + } + + interface NamedThing { + + String name(); + + } + + record AnnotatedAccessor(String name) implements NamedThing { + @Override + public String name() { // Compliant, no issues are raised for annotated accessors + return name; + } + } + + record DifferentlyNamedAccessor(String name) { + public String getName() { // Compliant, no issues are raised for accessors with different names + return name; + } + } + + record AccessorSettingField(String name) { + private static int AGE; + + public String name() { + AGE = 0; + return name; + } + } + + record AccessorWithParameter(String name) { + public String name(int x) { // Compliant, no issues are raised for accessors with parameters + return name; + } + } + + record AccessorWithLogic(String name) { + public String name() { // Compliant, no issues are raised for accessors with logic + System.out.println("Side effects !"); + return name; + } + } + + record AccessorWithLogic2(String name) { + public String name() { + int x = 42; + System.out.println(x); + return name; + } + } + + record AccessorWithAssertion(String name) { + public String name() { // Compliant, no issues are raised for accessors with assertions + assert name != null; + return name; + } + } + + record AccessorWithThrow(int age) { + public int age() { // Compliant, no issues are raised for accessors that throw exceptions + if (age < 0) { + throw new IllegalStateException("Negative age"); + } + return age; + } + } + + record AccessorWithValidation(int age) { + public int age() { // Compliant, no issues are raised for accessors that validate input + validate(age); + return age; + } + + void validate(int age) { + if (this.age < 0) { + throw new IllegalStateException("Negative age"); + } + } + } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/RedundantRecordMethodsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/RedundantRecordMethodsCheck.java index 09c7bfe1f0..45f672798c 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/RedundantRecordMethodsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/RedundantRecordMethodsCheck.java @@ -16,7 +16,6 @@ */ package org.sonar.java.checks; -import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -26,7 +25,9 @@ import org.sonar.check.Rule; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.tree.AssertStatementTree; import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; import org.sonar.plugins.java.api.tree.BlockTree; import org.sonar.plugins.java.api.tree.ClassTree; import org.sonar.plugins.java.api.tree.ExpressionStatementTree; @@ -37,6 +38,7 @@ import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.ReturnStatementTree; import org.sonar.plugins.java.api.tree.StatementTree; +import org.sonar.plugins.java.api.tree.ThrowStatementTree; import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.VariableTree; @@ -64,7 +66,7 @@ public void visitNode(Tree tree) { if (member.is(Tree.Kind.CONSTRUCTOR)) { checkConstructor((MethodTree) member, componentNames); } else if (member.is(Tree.Kind.METHOD)) { - checkMethod((MethodTree) member, components, componentNames); + checkMethod((MethodTree) member, components); } } } @@ -95,38 +97,30 @@ private void checkConstructor(MethodTree constructor, Set componentNames } } - private void checkMethod(MethodTree method, List components, Set componentsByName) { - String methodName = method.symbol().name(); - if (!componentsByName.contains(methodName)) { + private void checkMethod(MethodTree method, List components) { + if (isAnnotated(method) || !method.parameters().isEmpty()) { return; } - if (onlyReturnsRawValue(method, components)) { - reportIssue(method.simpleName(), "Remove this redundant method which is the same as a default one."); + + String methodName = method.symbol().name(); + Symbol accessedComponent = components.stream().filter(c -> c.name().equals(methodName)).findFirst().orElse(null); + if (accessedComponent == null) { + return; } - } - public static boolean onlyReturnsRawValue(MethodTree method, Collection components) { - Optional returnStatement = getFirstReturnStatement(method); - if (!returnStatement.isPresent()) { - return false; + var accessorVisitor = new AccessorVisitor(); + method.block().accept(accessorVisitor); + if (accessorVisitor.containsLogic) { + return; } - ExpressionTree expression = returnStatement.get().expression(); - Symbol identifierSymbol; - if (expression.is(Tree.Kind.IDENTIFIER)) { - identifierSymbol = ((IdentifierTree) expression).symbol(); - } else if (expression.is(Tree.Kind.MEMBER_SELECT)) { - identifierSymbol = (((MemberSelectExpressionTree) expression).identifier()).symbol(); - } else { - return false; + if (accessorVisitor.returnedExpressions.stream().allMatch(e -> isComponent(e, accessedComponent))) { + reportIssue(method.simpleName(), "Remove this redundant method which is the same as a default one."); } - return components.stream().anyMatch(identifierSymbol::equals); } - private static Optional getFirstReturnStatement(MethodTree method) { - return method.block().body().stream() - .filter(statement -> statement.is(Tree.Kind.RETURN_STATEMENT)) - .map(ReturnStatementTree.class::cast) - .findFirst(); + private static boolean isComponent(ExpressionTree expression, Symbol component) { + return (expression instanceof IdentifierTree identifier && component.equals(identifier.symbol())) + || (expression instanceof MemberSelectExpressionTree memberSelect && component.equals(memberSelect.identifier().symbol())); } private static boolean isAnnotated(MethodTree method) { @@ -259,4 +253,47 @@ private void mergeWith(ConstructorExecutionState other) { logicAfterAssignments = logicAfterAssignments || other.logicAfterAssignments; } } + + private static class AccessorVisitor extends BaseTreeVisitor { + + Set returnedExpressions = new HashSet<>(); + + boolean containsLogic = false; + + @Override + public void visitReturnStatement(ReturnStatementTree tree) { + returnedExpressions.add(tree.expression()); + super.visitReturnStatement(tree); + } + + @Override + public void visitAssertStatement(AssertStatementTree tree) { + containsLogic = true; + super.visitAssertStatement(tree); + } + + @Override + public void visitThrowStatement(ThrowStatementTree tree) { + containsLogic = true; + super.visitThrowStatement(tree); + } + + @Override + public void visitExpressionStatement(ExpressionStatementTree tree) { + containsLogic = true; + super.visitExpressionStatement(tree); + } + + @Override + public void visitVariable(VariableTree tree) { + containsLogic = true; + super.visitVariable(tree); + } + + @Override + public void visitAssignmentExpression(AssignmentExpressionTree tree) { + containsLogic = true; + super.visitAssignmentExpression(tree); + } + } }