From 0b04e7532c490e52b456ee534f864a0f42e87757 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Coet?= Date: Fri, 6 Mar 2026 13:32:17 +0100 Subject: [PATCH 1/2] Fix FPs in S6207 for accessor methods --- .../RedundantRecordMethodsCheckSample.java | 84 ++++++++++++++++- .../checks/RedundantRecordMethodsCheck.java | 89 +++++++++++++------ 2 files changed, 146 insertions(+), 27 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 20b15b799b..26bb144118 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,85 @@ 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 AccessorWithParameter(String name) { + public String name(int x) { // Compliant, no issues are raised for accessors with parameters + return name; + } + } + + record AccessorWithLogic(String name) { + @Override + 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) { + @Override + public String name() { // Compliant, no issues are raised for accessors with assertions + assert name != null; + return name; + } + } + + record AccessorWithThrow(int age) { + @Override + 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 (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..b149b18e22 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); + } + } } From a0cd1cd31c2bbc99fc6459f821580109ea067996 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Coet?= Date: Fri, 6 Mar 2026 14:01:05 +0100 Subject: [PATCH 2/2] Fix the quality gate --- .../checks/RedundantRecordMethodsCheckSample.java | 14 ++++++++++---- .../java/checks/RedundantRecordMethodsCheck.java | 2 +- 2 files changed, 11 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 26bb144118..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 @@ -258,6 +258,15 @@ public String getName() { // Compliant, no issues are raised for accessors with } } + 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; @@ -265,7 +274,6 @@ public String name(int x) { // Compliant, no issues are raised for accessors wit } record AccessorWithLogic(String name) { - @Override public String name() { // Compliant, no issues are raised for accessors with logic System.out.println("Side effects !"); return name; @@ -281,7 +289,6 @@ public String name() { } record AccessorWithAssertion(String name) { - @Override public String name() { // Compliant, no issues are raised for accessors with assertions assert name != null; return name; @@ -289,7 +296,6 @@ public String name() { // Compliant, no issues are raised for accessors with ass } record AccessorWithThrow(int age) { - @Override public int age() { // Compliant, no issues are raised for accessors that throw exceptions if (age < 0) { throw new IllegalStateException("Negative age"); @@ -305,7 +311,7 @@ public int age() { // Compliant, no issues are raised for accessors that validat } void validate(int age) { - if (age < 0) { + 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 b149b18e22..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 @@ -120,7 +120,7 @@ private void checkMethod(MethodTree method, List componen 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()); + || (expression instanceof MemberSelectExpressionTree memberSelect && component.equals(memberSelect.identifier().symbol())); } private static boolean isAnnotated(MethodTree method) {