SONARJAVA-6186 Fix FPs in S6207 for accessor methods (#5516)

Author: aurelien-coet-sonarsource

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,89 @@ 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 call methods
+      validate(age);
+      return age;
+    }
+
+    void validate(int age) {
+      // Do something with potential side effects.
+    }
+  }
 }

java-checks/src/main/java/org/sonar/java/checks/RedundantRecordMethodsCheck.java

@@ -16,17 +16,19 @@
  */
 package org.sonar.java.checks;
 
-import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
 import org.sonar.check.Rule;
+import org.sonar.java.model.ExpressionUtils;
 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 +39,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 +67,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 +98,30 @@ private void checkConstructor(MethodTree constructor, Set<String> componentNames
     }
   }
 
-  private void checkMethod(MethodTree method, List<Symbol.VariableSymbol> components, Set<String> componentsByName) {
-    String methodName = method.symbol().name();
-    if (!componentsByName.contains(methodName)) {
+  private void checkMethod(MethodTree method, List<Symbol.VariableSymbol> 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<Symbol.VariableSymbol> components) {
-    Optional<ReturnStatementTree> 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<ReturnStatementTree> 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 (ExpressionUtils.skipParentheses(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 +254,47 @@ private void mergeWith(ConstructorExecutionState other) {
       logicAfterAssignments = logicAfterAssignments || other.logicAfterAssignments;
     }
   }
+
+  private static class AccessorVisitor extends BaseTreeVisitor {
+
+    Set<ExpressionTree> 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);
+    }
+  }
 }