SONARJAVA-5985 Improve S6207's logic for constructors (#5512)

Author: aurelien-coet-sonarsource

java-checks-test-sources/default/src/main/java/checks/RedundantRecordMethodsCheckSample.java

@@ -14,11 +14,7 @@ record RedundantConstructorAndGetters(String name, int age) {
 
     RedundantConstructorAndGetters(String name, int age) { // Noncompliant {{Remove this redundant constructor which is the same as a default one.}}
 //  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-      System.out.println("Just printing something...");
       this.name = name;
-      int x = 42;
-      variable = new Object();
-      this.someOtherVariable = new Object();
       this.age = age;
     }
 
@@ -50,6 +46,28 @@ public int age() { // Noncompliant {{Remove this redundant method which is the s
     }
   }
 
+  record ConstructorWithSideEffects(String name, int age) {
+    ConstructorWithSideEffects(String name, int age) { // Noncompliant {{Consider using a compact constructor here.}}
+      sideEffect();
+      this.name = name;
+      this.age = age;
+    }
+
+    public void sideEffect() {
+      System.out.println("Here's a side effect!");
+    }
+  }
+
+  record ThrowingConstructor(String name, int age) {
+    ThrowingConstructor(String name, int age) { // Noncompliant {{Consider using a compact constructor here.}}
+      if (age < 0) {
+        throw new IllegalArgumentException("Negative age");
+      }
+      this.name = name;
+      this.age = age;
+    }
+  }
+
   record ParameterMismatch(String name, String address) {
     ParameterMismatch(String name, String address) { // Compliant
       this.name = address;
@@ -85,7 +103,7 @@ record ConstructorAssignsStaticValue(String name, int age) {
   }
 
   record EmptyConstructorAndRedundantGetter(String name, int age) {
-    EmptyConstructorAndRedundantGetter { // Noncompliant {{Remove this redundant constructor which is the same as a default one.}}
+    EmptyConstructorAndRedundantGetter { // Noncompliant {{Remove this useless empty compact constructor.}}
 //  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     }
 
@@ -105,6 +123,17 @@ record CompliantConstructorWithAddedValue(String name, int age) {
     }
   }
 
+  record CompliantConstructorWithSideEffect(String name, int age) {
+    CompliantConstructorWithSideEffect(String name, int age) { // Compliant: the constructor includes side effects after assignments
+      if (age < 0) {
+        throw new IllegalArgumentException("Negative age");
+      }
+      this.name = name.toLowerCase(Locale.ROOT);
+      this.age = age;
+      System.out.println("Hello");
+    }
+  }
+
   record CompliantConstructorComplementAndTransformativeGetter(String name, int age) {
     CompliantConstructorComplementAndTransformativeGetter { // Compliant
       if (age < 0) {

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

@@ -62,18 +62,35 @@ public void visitNode(Tree tree) {
 
     for (Tree member : targetRecord.members()) {
       if (member.is(Tree.Kind.CONSTRUCTOR)) {
-        checkConstructor((MethodTree) member, components);
+        checkConstructor((MethodTree) member, componentNames);
       } else if (member.is(Tree.Kind.METHOD)) {
         checkMethod((MethodTree) member, components, componentNames);
       }
     }
   }
 
-  private void checkConstructor(MethodTree constructor, List<Symbol.VariableSymbol> components) {
-    if(isAnnotated(constructor)) {
+  private void checkConstructor(MethodTree constructor, Set<String> componentNames) {
+    if (isAnnotated(constructor)) {
       return;
     }
-    if (constructor.block().body().isEmpty() || onlyDoesSimpleAssignments(constructor, components)) {
+    if (constructor.parameters().isEmpty() && constructor.block().body().isEmpty()) {
+      reportIssue(constructor.simpleName(), "Remove this useless empty compact constructor.");
+      return;
+    }
+    if (constructor.parameters().size() != componentNames.size()
+      || constructor.parameters().stream().anyMatch(parameter -> !componentNames.contains(parameter.symbol().name()))) {
+      return;
+    }
+
+    ConstructorExecutionState executionState = new ConstructorExecutionState(componentNames);
+    constructor.block().body().forEach(executionState::applyStatement);
+
+    if (!executionState.componentsAreFullyAssigned() || executionState.logicAfterAssignments) {
+      return;
+    }
+    if (executionState.logicBeforeAssignments) {
+      reportIssue(constructor.simpleName(), "Consider using a compact constructor here.");
+    } else {
       reportIssue(constructor.simpleName(), "Remove this redundant constructor which is the same as a default one.");
     }
   }
@@ -112,55 +129,101 @@ private static Optional<ReturnStatementTree> getFirstReturnStatement(MethodTree
       .findFirst();
   }
 
-  public static boolean onlyDoesSimpleAssignments(MethodTree constructor, List<Symbol.VariableSymbol> components) {
-    if (constructor.parameters().size() != components.size()) {
-      return false;
-    }
-    List<String> componentNames = components.stream().map(Symbol.VariableSymbol::name).toList();
-    ConstructorExecutionState executionState = new ConstructorExecutionState(componentNames);
-    constructor.block().body().forEach(executionState::applyStatement);
-    return executionState.componentsAreFullyAssigned();
-  }
-
   private static boolean isAnnotated(MethodTree method) {
     return !method.modifiers().annotations().isEmpty();
   }
 
   /**
-   * Class to perform a simple symbolic execution of a record constructor. The state keeps track of which components have been
-   * assigned to the corresponding parameters and which parameters have been changed from their initial values.
+   * Class to perform a simple symbolic execution of a record constructor. The state keeps track of which components have been assigned to the corresponding parameters and which
+   * parameters have been changed from their initial values, as well as if the constructor contains logic before or after assignments to components.
    */
   private static class ConstructorExecutionState {
     /**
-     * Immutable list of the components in the record
+     * Immutable set of the names of components in the record.
+     */
+    private final Set<String> componentNames;
+
+    /**
+     * Set of names of components of the record that have been assigned to their corresponding parameter so far.
      */
-    final List<String> componentNames;
+    private final Set<String> assignedComponents = new HashSet<>();
 
     /**
-     * Set of names of components of the record that have been assigned to the corresponding parameter so far
+     * Set of names of parameters that still hold the value of the argument passed to the constructor.
      */
-    Set<String> assignedComponents;
+    private final Set<String> unchangedParameters = new HashSet<>();
 
     /**
-     * Set of name of parameters that still hold the value of the argument passed to the constructor
+     * Flag indicating whether the constructor has logic before any of the component assignments.
      */
-    Set<String> unchangedParameters;
+    boolean logicBeforeAssignments = false;
 
-    private ConstructorExecutionState(List<String> componentNames) {
-      assignedComponents = new HashSet<>();
+    /**
+     * Flag indicating whether the constructor has logic after any of the component assignments.
+     */
+    boolean logicAfterAssignments = false;
+
+    private ConstructorExecutionState(Set<String> componentNames) {
       this.componentNames = componentNames;
-      unchangedParameters = new HashSet<>();
       unchangedParameters.addAll(componentNames);
     }
 
-    private boolean isParameter(Symbol symbol) {
-      return componentNames.contains(symbol.name());
+    public boolean componentsAreFullyAssigned() {
+      return assignedComponents.size() == componentNames.size();
+    }
+
+    public void applyStatement(StatementTree statement) {
+      if (statement instanceof ExpressionStatementTree expression
+        && expression.expression() instanceof AssignmentExpressionTree assignment) {
+        applyAssignment(assignment.variable(), assignment.expression());
+      } else if (statement instanceof IfStatementTree ifStatement) {
+        applyIf(ifStatement);
+      } else if (statement instanceof BlockTree block) {
+        block.body().forEach(this::applyStatement);
+      } else if (this.assignedComponents.isEmpty()) {
+        applyLogic();
+      }
+    }
+
+    private void applyAssignment(ExpressionTree lhs, ExpressionTree rhs) {
+      if (rhs instanceof IdentifierTree identifier
+        && isParameter(identifier.symbol())
+        && unchangedParameters.contains(identifier.name())) {
+        Optional<String> component = getComponent(lhs).filter(name -> name.equals(identifier.name()));
+        if (component.isPresent()) {
+          assignedComponents.add(component.get());
+        } else {
+          applyLogic();
+        }
+      } else if (lhs instanceof IdentifierTree identifier && isParameter(identifier.symbol())) {
+        unchangedParameters.remove(identifier.name());
+      } else {
+        applyLogic();
+      }
+    }
+
+    private void applyIf(IfStatementTree ifStatement) {
+      ConstructorExecutionState stateCopy = copy();
+      applyStatement(ifStatement.thenStatement());
+      StatementTree elseStatement = ifStatement.elseStatement();
+      if (elseStatement != null) {
+        stateCopy.applyStatement(elseStatement);
+      }
+      mergeWith(stateCopy);
+    }
+
+    private void applyLogic() {
+      if (this.assignedComponents.isEmpty()) {
+        logicBeforeAssignments = true;
+      } else {
+        logicAfterAssignments = true;
+      }
     }
 
     /**
      * Return the name of the component if the given expression is of the form `this.name`.
      */
-    public Optional<String> getComponent(ExpressionTree expression) {
+    private Optional<String> getComponent(ExpressionTree expression) {
       if (expression instanceof MemberSelectExpressionTree memberSelect) {
         String name = memberSelect.identifier().name();
         if (componentNames.contains(name)) {
@@ -170,54 +233,30 @@ public Optional<String> getComponent(ExpressionTree expression) {
       return Optional.empty();
     }
 
-    private void applyAssignment(ExpressionTree lhs, ExpressionTree rhs) {
-      if (rhs instanceof IdentifierTree identifier
-        && isParameter(identifier.symbol())
-        && unchangedParameters.contains(identifier.name())) {
-        getComponent(lhs)
-          .filter(name -> name.equals(identifier.name()))
-          .ifPresent(assignedComponents::add);
-      } else if (lhs instanceof IdentifierTree identifier && isParameter(identifier.symbol())) {
-        unchangedParameters.remove(identifier.name());
-      }
+    private boolean isParameter(Symbol symbol) {
+      return componentNames.contains(symbol.name());
     }
 
     private ConstructorExecutionState copy() {
       var copy = new ConstructorExecutionState(componentNames);
       copy.unchangedParameters.clear();
       copy.unchangedParameters.addAll(unchangedParameters);
       copy.assignedComponents.addAll(assignedComponents);
+      copy.logicBeforeAssignments = logicBeforeAssignments;
+      copy.logicAfterAssignments = logicAfterAssignments;
       return copy;
     }
 
     /**
      * When joining two branches of the execution, a component is considered assigned if it was assigned in both branches.
      * A parameter is considered unchanged if it was unchanged on both branches.
+     * Logic before or after assignments is considered present if either branch has them.
      */
     private void mergeWith(ConstructorExecutionState other) {
       assignedComponents.removeIf(component -> !other.assignedComponents.contains(component));
       unchangedParameters.removeIf(component -> !other.unchangedParameters.contains(component));
-    }
-
-    public void applyStatement(StatementTree statement) {
-      if (statement instanceof ExpressionStatementTree expression
-        && expression.expression() instanceof AssignmentExpressionTree assignment) {
-        applyAssignment(assignment.variable(), assignment.expression());
-      } else if (statement instanceof IfStatementTree ifStatement) {
-        ConstructorExecutionState stateCopy = copy();
-        applyStatement(ifStatement.thenStatement());
-        StatementTree elseStatement = ifStatement.elseStatement();
-        if (elseStatement != null) {
-          stateCopy.applyStatement(elseStatement);
-        }
-        mergeWith(stateCopy);
-      } else if (statement instanceof BlockTree block) {
-        block.body().forEach(this::applyStatement);
-      }
-    }
-
-    public boolean componentsAreFullyAssigned() {
-      return assignedComponents.size() == componentNames.size();
+      logicBeforeAssignments = logicBeforeAssignments || other.logicBeforeAssignments;
+      logicAfterAssignments = logicAfterAssignments || other.logicAfterAssignments;
     }
   }
 }

sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6207.html

@@ -19,6 +19,16 @@ <h3>Noncompliant code example</h3>
     return name;
   }
 }
+
+record Person(String name, int age) {
+  Person(String name, int age) { // Noncompliant, this can be implemented with a compact constructor
+    if (age &lt; 0) {
+      throw new IllegalArgumentException("Negative age");
+    }
+    this.name = name;
+    this.age = age;
+  }
+}
 </pre>
 <h3>Compliant solution</h3>
 <pre>