From c67233de1dbe9390f0ea4361ffaf0b43b137bfd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Coet?= Date: Wed, 4 Mar 2026 16:19:30 +0100 Subject: [PATCH 1/4] Improve the rule's logic for constructors --- .../RedundantRecordMethodsCheckSample.java | 28 +++- .../checks/RedundantRecordMethodsCheck.java | 155 +++++++++++------- 2 files changed, 119 insertions(+), 64 deletions(-) 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 da3c2fc252d..82d2137bce3 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 @@ -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.}} // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ } 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 3917d59737f..851ca676d66 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 @@ -62,18 +62,33 @@ public void visitNode(Tree tree) { for (Tree member : targetRecord.members()) { if (member.is(Tree.Kind.CONSTRUCTOR)) { - checkConstructor((MethodTree) member, components); + checkConstructor((MethodTree) member, components, componentNames); } else if (member.is(Tree.Kind.METHOD)) { checkMethod((MethodTree) member, components, componentNames); } } } - private void checkConstructor(MethodTree constructor, List components) { - if(isAnnotated(constructor)) { + private void checkConstructor(MethodTree constructor, List components, Set componentNames) { + if (constructor.parameters().isEmpty() && constructor.block().body().isEmpty()) { + reportIssue(constructor.simpleName(), "Remove this useless empty compact constructor."); + } + + if (isAnnotated(constructor) + || constructor.parameters().size() != components.size() + || !constructor.parameters().stream().allMatch(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 (constructor.block().body().isEmpty() || onlyDoesSimpleAssignments(constructor, components)) { + 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 +127,101 @@ private static Optional getFirstReturnStatement(MethodTree .findFirst(); } - public static boolean onlyDoesSimpleAssignments(MethodTree constructor, List components) { - if (constructor.parameters().size() != components.size()) { - return false; - } - List 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 componentNames; + + /** + * Set of names of components of the record that have been assigned to their corresponding parameter so far. + */ + private final Set assignedComponents = new HashSet<>(); + + /** + * Set of names of parameters that still hold the value of the argument passed to the constructor. */ - final List componentNames; + private final Set unchangedParameters = new HashSet<>(); /** - * Set of names of components of the record that have been assigned to the corresponding parameter so far + * Flag indicating whether the constructor has logic before any of the component assignments. */ - Set assignedComponents; + boolean logicBeforeAssignments = false; /** - * Set of name of parameters that still hold the value of the argument passed to the constructor + * Flag indicating whether the constructor has logic after any of the component assignments. */ - Set unchangedParameters; + boolean logicAfterAssignments = false; - private ConstructorExecutionState(List componentNames) { - assignedComponents = new HashSet<>(); + private ConstructorExecutionState(Set 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 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 getComponent(ExpressionTree expression) { + private Optional getComponent(ExpressionTree expression) { if (expression instanceof MemberSelectExpressionTree memberSelect) { String name = memberSelect.identifier().name(); if (componentNames.contains(name)) { @@ -170,16 +231,8 @@ public Optional 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() { @@ -187,37 +240,21 @@ private ConstructorExecutionState copy() { 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; } } } From 50105d3e79343dd99fc7ae7c3a7d6848cdc13852 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Coet?= Date: Wed, 4 Mar 2026 16:55:27 +0100 Subject: [PATCH 2/4] Remove useless parameter in constructor check and more tests --- .../checks/RedundantRecordMethodsCheckSample.java | 11 +++++++++++ .../java/checks/RedundantRecordMethodsCheck.java | 12 +++++++----- 2 files changed, 18 insertions(+), 5 deletions(-) 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 82d2137bce3..20b15b799b4 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 @@ -123,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) { 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 851ca676d66..ada10e7b694 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 @@ -62,20 +62,22 @@ public void visitNode(Tree tree) { for (Tree member : targetRecord.members()) { if (member.is(Tree.Kind.CONSTRUCTOR)) { - checkConstructor((MethodTree) member, components, componentNames); + checkConstructor((MethodTree) member, componentNames); } else if (member.is(Tree.Kind.METHOD)) { checkMethod((MethodTree) member, components, componentNames); } } } - private void checkConstructor(MethodTree constructor, List components, Set componentNames) { + private void checkConstructor(MethodTree constructor, Set componentNames) { + if (isAnnotated(constructor)) { + return; + } if (constructor.parameters().isEmpty() && constructor.block().body().isEmpty()) { reportIssue(constructor.simpleName(), "Remove this useless empty compact constructor."); + return; } - - if (isAnnotated(constructor) - || constructor.parameters().size() != components.size() + if (constructor.parameters().size() != componentNames.size() || !constructor.parameters().stream().allMatch(parameter -> componentNames.contains(parameter.symbol().name()))) { return; } From fe19a14f59d8ca4099a282b284c37e394842c226 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Coet?= Date: Thu, 5 Mar 2026 16:01:52 +0100 Subject: [PATCH 3/4] Apply fixes following review --- .../java/org/sonar/java/checks/RedundantRecordMethodsCheck.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ada10e7b694..09c7bfe1f02 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 @@ -78,7 +78,7 @@ private void checkConstructor(MethodTree constructor, Set componentNames return; } if (constructor.parameters().size() != componentNames.size() - || !constructor.parameters().stream().allMatch(parameter -> componentNames.contains(parameter.symbol().name()))) { + || constructor.parameters().stream().anyMatch(parameter -> !componentNames.contains(parameter.symbol().name()))) { return; } From 241fc4e6061a8a2955477586cd38b1acaea6d9c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Coet?= Date: Thu, 5 Mar 2026 16:42:52 +0100 Subject: [PATCH 4/4] Update rule S6207's description --- .../org/sonar/l10n/java/rules/java/S6207.html | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6207.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6207.html index c77160b0e10..7d16a537c10 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6207.html +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6207.html @@ -19,6 +19,16 @@

Noncompliant code example

return name; } } + +record Person(String name, int age) { + Person(String name, int age) { // Noncompliant, this can be implemented with a compact constructor + if (age < 0) { + throw new IllegalArgumentException("Negative age"); + } + this.name = name; + this.age = age; + } +}

Compliant solution