From 97bdafb28d1b6490b7e703a4a3a5efceee01d9e8 Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Mon, 26 Oct 2026 12:34:20 +0100 Subject: [PATCH 1/3] SONARJAVA-7240 Improve S9398: allow helper and builder types nested in interfaces Nested classes are no longer reported when they are builders or appear in the signature (return type, parameter types, thrown exceptions) of a method of the enclosing interface, unless they implement that interface. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../InnerClassInInterfaceCheckSample.java | 141 ++++++++++++++++++ .../checks/InnerClassInInterfaceCheck.java | 78 +++++++++- 2 files changed, 218 insertions(+), 1 deletion(-) diff --git a/java-checks-test-sources/default/src/main/java/checks/InnerClassInInterfaceCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/InnerClassInInterfaceCheckSample.java index 5eb3687dcb8..e7731d10418 100644 --- a/java-checks-test-sources/default/src/main/java/checks/InnerClassInInterfaceCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/InnerClassInInterfaceCheckSample.java @@ -1,5 +1,9 @@ package checks; +import java.util.List; +import java.util.Map; +import java.util.Optional; + interface InnerClassInInterfaceCheckSample { void doSomething(); @@ -51,6 +55,143 @@ class Second {} // Noncompliant // ^^^^^^ } +interface WithHelperTypes { + + Result compute(Request request) throws ComputationException; + + Optional wrapped(); + + void consume(List> elements); + + void consumeQualified(WithHelperTypes.Qualified qualified); + + default Defaulted defaulted() { + return new Defaulted(); + } + + static Created create() { + return new Created(); + } + + class Result {} // Compliant - return type of an interface method + + class Request {} // Compliant - parameter type of an interface method + + class ComputationException extends Exception {} // Compliant - exception thrown by an interface method + + class Wrapped {} // Compliant - type argument of a return type + + class Element {} // Compliant - nested type argument of a parameter type + + class Qualified {} // Compliant - qualified parameter type + + class Defaulted {} // Compliant - return type of a default method + + class Created {} // Compliant - return type of a static method + + class Unused {} // Noncompliant +// ^^^^^^ + + class UsedInBodyOnly {} // Noncompliant +// ^^^^^^^^^^^^^^ + default void useInBody() { + UsedInBodyOnly local = new UsedInBodyOnly(); + } + + class ParameterName {} // Noncompliant +// ^^^^^^^^^^^^^ + void named(String ParameterName); +} + +interface WithImplementationsUsedInSignatures { + + WithImplementationsUsedInSignatures copy(Nested other); + + Generic generic(); + + Qualified qualified(); + + class Nested implements WithImplementationsUsedInSignatures { // Noncompliant +// ^^^^^^ + @Override + public WithImplementationsUsedInSignatures copy(Nested other) { + return this; + } + @Override + public Generic generic() { + return null; + } + @Override + public Qualified qualified() { + return null; + } + } + + class Generic implements WithImplementationsUsedInSignatures, Comparable> { // Noncompliant +// ^^^^^^^ + @Override + public WithImplementationsUsedInSignatures copy(Nested other) { + return this; + } + @Override + public Generic generic() { + return null; + } + @Override + public Qualified qualified() { + return null; + } + @Override + public int compareTo(Generic o) { + return 0; + } + } + + class Qualified implements checks.WithImplementationsUsedInSignatures { // Noncompliant +// ^^^^^^^^^ + @Override + public WithImplementationsUsedInSignatures copy(Nested other) { + return this; + } + @Override + public Generic generic() { + return null; + } + @Override + public Qualified qualified() { + return null; + } + } +} + +interface ComparableUser { + + Helper helper(); + + class Helper implements Comparable { // Compliant - helper type, does not implement the enclosing interface + @Override + public int compareTo(ComparableUser o) { + return 0; + } + } +} + +interface WithBuilder { + + String name(); + + class Builder { // Compliant - builder pattern + private String name; + + public Builder name(String name) { + this.name = name; + return this; + } + } + + class WithBuilderBuilder {} // Compliant - builder pattern +} + class TopLevelClass { // Compliant - not inside an interface class InnerClass {} // Compliant - class inside class diff --git a/java-checks/src/main/java/org/sonar/java/checks/InnerClassInInterfaceCheck.java b/java-checks/src/main/java/org/sonar/java/checks/InnerClassInInterfaceCheck.java index a4bf3bc9b82..9c535cd2b6a 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/InnerClassInInterfaceCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/InnerClassInInterfaceCheck.java @@ -17,17 +17,30 @@ package org.sonar.java.checks; import java.util.Collections; +import java.util.HashSet; import java.util.List; +import java.util.Set; +import java.util.stream.Stream; +import javax.annotation.Nullable; import org.sonar.check.Rule; import org.sonar.java.model.ModifiersUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.semantic.Type; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; +import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.Modifier; +import org.sonar.plugins.java.api.tree.ParameterizedTypeTree; import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.TypeTree; @Rule(key = "S9398") public class InnerClassInInterfaceCheck extends IssuableSubscriptionVisitor { + private static final String BUILDER_SUFFIX = "Builder"; + @Override public List nodesToVisit() { return Collections.singletonList(Tree.Kind.CLASS); @@ -40,9 +53,72 @@ public void visitNode(Tree tree) { return; } Tree parent = tree.parent(); - if (parent != null && parent.is(Tree.Kind.INTERFACE)) { + if (parent == null || !parent.is(Tree.Kind.INTERFACE)) { + return; + } + ClassTree interfaceTree = (ClassTree) parent; + if (implementsInterface(classTree, interfaceTree) || !isHelperType(classTree, interfaceTree)) { reportIssue(classTree.simpleName(), "Move this class out of this interface."); } } + private static boolean implementsInterface(ClassTree classTree, ClassTree interfaceTree) { + Type classType = classTree.symbol().type(); + Type interfaceType = interfaceTree.symbol().type(); + if (!classType.isUnknown() && !interfaceType.isUnknown()) { + return classType.isSubtypeOf(interfaceType); + } + String interfaceName = interfaceTree.simpleName().name(); + return Stream.concat(Stream.ofNullable(classTree.superClass()), classTree.superInterfaces().stream()) + .anyMatch(superType -> interfaceName.equals(mainTypeName(superType))); + } + + @Nullable + private static String mainTypeName(TypeTree typeTree) { + if (typeTree instanceof ParameterizedTypeTree parameterizedType) { + return mainTypeName(parameterizedType.type()); + } + if (typeTree instanceof MemberSelectExpressionTree memberSelect) { + return memberSelect.identifier().name(); + } + if (typeTree instanceof IdentifierTree identifier) { + return identifier.name(); + } + return null; + } + + /** + * A nested class is considered a helper type of its enclosing interface when it is a builder, + * or when it appears in the signature (return type, parameter types or thrown exceptions) + * of a method declared by the interface. + */ + private static boolean isHelperType(ClassTree classTree, ClassTree interfaceTree) { + String className = classTree.simpleName().name(); + if (className.endsWith(BUILDER_SUFFIX)) { + return true; + } + TypeNamesCollector collector = new TypeNamesCollector(); + interfaceTree.members().stream() + .filter(MethodTree.class::isInstance) + .map(MethodTree.class::cast) + .forEach(collector::scanSignature); + return collector.names.contains(className); + } + + private static class TypeNamesCollector extends BaseTreeVisitor { + + private final Set names = new HashSet<>(); + + void scanSignature(MethodTree methodTree) { + scan(methodTree.returnType()); + methodTree.parameters().forEach(parameter -> scan(parameter.type())); + scan(methodTree.throwsClauses()); + } + + @Override + public void visitIdentifier(IdentifierTree tree) { + names.add(tree.name()); + } + } + } From 6d7439057679654da57999a9ec14261aec1a3281 Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Mon, 26 Oct 2026 13:29:06 +0100 Subject: [PATCH 2/3] SONARJAVA-7240 Use resolved symbols for helper detection and scan interface once Helper types are now matched by symbol instead of simple name, so unrelated qualified types and method type parameters no longer hide issues. The check visits interfaces and collects signature types once per interface. The name-based fallback for unknown types is removed, and ruling expectations are updated. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../expected/eclipse-jetty/java-S9398.json | 3 - .../expected/sonarqube-6.5/java-S9398.json | 20 ----- .../InnerClassInInterfaceCheckSample.java | 17 +++++ .../InnerClassInInterfaceCheckSample.java | 13 ++++ .../checks/InnerClassInInterfaceCheck.java | 75 ++++++------------- .../InnerClassInInterfaceCheckTest.java | 9 +++ 6 files changed, 63 insertions(+), 74 deletions(-) create mode 100644 java-checks-test-sources/default/src/main/files/non-compiling/checks/InnerClassInInterfaceCheckSample.java diff --git a/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9398.json b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9398.json index 9475e9d3983..932dcb4d0e7 100644 --- a/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9398.json +++ b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9398.json @@ -14,9 +14,6 @@ "eclipse-jetty:jetty-io/src/main/java/org/eclipse/jetty/io/Connection.java": [ 161 ], -"eclipse-jetty:jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslHandshakeListener.java": [ -57 -], "eclipse-jetty:jetty-server/src/main/java/org/eclipse/jetty/server/Authentication.java": [ 37 ], diff --git a/its/ruling/src/test/resources/expected/sonarqube-6.5/java-S9398.json b/its/ruling/src/test/resources/expected/sonarqube-6.5/java-S9398.json index f1139e98fb2..a092ecfe5ae 100644 --- a/its/ruling/src/test/resources/expected/sonarqube-6.5/java-S9398.json +++ b/its/ruling/src/test/resources/expected/sonarqube-6.5/java-S9398.json @@ -1,25 +1,5 @@ { -"sonarqube-6.5:src/main/java/org/sonar/server/authentication/event/AuthenticationEvent.java": [ -97 -], -"sonarqube-6.5:src/main/java/org/sonar/server/computation/task/projectanalysis/filemove/FileSimilarity.java": [ -28 -], -"sonarqube-6.5:src/main/java/org/sonar/server/computation/task/projectanalysis/filemove/MovedFilesRepository.java": [ -37 -], -"sonarqube-6.5:src/main/java/org/sonar/server/es/IndexDefinition.java": [ -31 -], "sonarqube-6.5:src/main/java/org/sonar/server/organization/BillingValidations.java": [ -47, 65 -], -"sonarqube-6.5:src/main/java/org/sonar/server/organization/OrganizationCreation.java": [ -120, -126 -], -"sonarqube-6.5:src/main/java/org/sonar/server/permission/ws/template/DefaultTemplatesResolver.java": [ -43 ] } diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/InnerClassInInterfaceCheckSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/InnerClassInInterfaceCheckSample.java new file mode 100644 index 00000000000..0d7f6ddcda5 --- /dev/null +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/InnerClassInInterfaceCheckSample.java @@ -0,0 +1,17 @@ +package checks; + +interface InnerClassInInterfaceCheckSample { + + Result compute(Unknown unknown); + + Unknown.Result unknownQualified(); + + class Result extends UnknownParent {} // Compliant - return type of an interface method + + class UnknownSuperclass extends UnknownParent implements InnerClassInInterfaceCheckSample { // Noncompliant +// ^^^^^^^^^^^^^^^^^ + } + + class UnknownSuperinterface implements UnknownInterface {} // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^ +} diff --git a/java-checks-test-sources/default/src/main/java/checks/InnerClassInInterfaceCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/InnerClassInInterfaceCheckSample.java index e7731d10418..28470b6fe04 100644 --- a/java-checks-test-sources/default/src/main/java/checks/InnerClassInInterfaceCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/InnerClassInInterfaceCheckSample.java @@ -103,6 +103,19 @@ class ParameterName {} // Noncompliant void named(String ParameterName); } +interface WithNameCollisionsInSignatures { + + java.util.Optional qualifiedForeignType(); + + Element typeParameter(); + + class Optional {} // Noncompliant +// ^^^^^^^^ + + class Element {} // Noncompliant +// ^^^^^^^ +} + interface WithImplementationsUsedInSignatures { WithImplementationsUsedInSignatures copy(Nested other); diff --git a/java-checks/src/main/java/org/sonar/java/checks/InnerClassInInterfaceCheck.java b/java-checks/src/main/java/org/sonar/java/checks/InnerClassInInterfaceCheck.java index 9c535cd2b6a..b757fba0e37 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/InnerClassInInterfaceCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/InnerClassInInterfaceCheck.java @@ -20,21 +20,16 @@ import java.util.HashSet; import java.util.List; import java.util.Set; -import java.util.stream.Stream; -import javax.annotation.Nullable; import org.sonar.check.Rule; import org.sonar.java.model.ModifiersUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; -import org.sonar.plugins.java.api.semantic.Type; +import org.sonar.plugins.java.api.semantic.Symbol; import org.sonar.plugins.java.api.tree.BaseTreeVisitor; import org.sonar.plugins.java.api.tree.ClassTree; import org.sonar.plugins.java.api.tree.IdentifierTree; -import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.Modifier; -import org.sonar.plugins.java.api.tree.ParameterizedTypeTree; import org.sonar.plugins.java.api.tree.Tree; -import org.sonar.plugins.java.api.tree.TypeTree; @Rule(key = "S9398") public class InnerClassInInterfaceCheck extends IssuableSubscriptionVisitor { @@ -43,48 +38,23 @@ public class InnerClassInInterfaceCheck extends IssuableSubscriptionVisitor { @Override public List nodesToVisit() { - return Collections.singletonList(Tree.Kind.CLASS); + return Collections.singletonList(Tree.Kind.INTERFACE); } @Override public void visitNode(Tree tree) { - ClassTree classTree = (ClassTree) tree; - if (ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.ABSTRACT)) { - return; - } - Tree parent = tree.parent(); - if (parent == null || !parent.is(Tree.Kind.INTERFACE)) { - return; - } - ClassTree interfaceTree = (ClassTree) parent; - if (implementsInterface(classTree, interfaceTree) || !isHelperType(classTree, interfaceTree)) { - reportIssue(classTree.simpleName(), "Move this class out of this interface."); - } + ClassTree interfaceTree = (ClassTree) tree; + Set signatureTypes = signatureTypes(interfaceTree); + interfaceTree.members().stream() + .filter(member -> member.is(Tree.Kind.CLASS)) + .map(ClassTree.class::cast) + .filter(classTree -> !ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.ABSTRACT)) + .filter(classTree -> implementsInterface(classTree, interfaceTree) || !isHelperType(classTree, signatureTypes)) + .forEach(classTree -> reportIssue(classTree.simpleName(), "Move this class out of this interface.")); } private static boolean implementsInterface(ClassTree classTree, ClassTree interfaceTree) { - Type classType = classTree.symbol().type(); - Type interfaceType = interfaceTree.symbol().type(); - if (!classType.isUnknown() && !interfaceType.isUnknown()) { - return classType.isSubtypeOf(interfaceType); - } - String interfaceName = interfaceTree.simpleName().name(); - return Stream.concat(Stream.ofNullable(classTree.superClass()), classTree.superInterfaces().stream()) - .anyMatch(superType -> interfaceName.equals(mainTypeName(superType))); - } - - @Nullable - private static String mainTypeName(TypeTree typeTree) { - if (typeTree instanceof ParameterizedTypeTree parameterizedType) { - return mainTypeName(parameterizedType.type()); - } - if (typeTree instanceof MemberSelectExpressionTree memberSelect) { - return memberSelect.identifier().name(); - } - if (typeTree instanceof IdentifierTree identifier) { - return identifier.name(); - } - return null; + return classTree.symbol().type().isSubtypeOf(interfaceTree.symbol().type()); } /** @@ -92,22 +62,22 @@ private static String mainTypeName(TypeTree typeTree) { * or when it appears in the signature (return type, parameter types or thrown exceptions) * of a method declared by the interface. */ - private static boolean isHelperType(ClassTree classTree, ClassTree interfaceTree) { - String className = classTree.simpleName().name(); - if (className.endsWith(BUILDER_SUFFIX)) { - return true; - } - TypeNamesCollector collector = new TypeNamesCollector(); + private static boolean isHelperType(ClassTree classTree, Set signatureTypes) { + return classTree.simpleName().name().endsWith(BUILDER_SUFFIX) || signatureTypes.contains(classTree.symbol()); + } + + private static Set signatureTypes(ClassTree interfaceTree) { + TypeSymbolsCollector collector = new TypeSymbolsCollector(); interfaceTree.members().stream() .filter(MethodTree.class::isInstance) .map(MethodTree.class::cast) .forEach(collector::scanSignature); - return collector.names.contains(className); + return collector.symbols; } - private static class TypeNamesCollector extends BaseTreeVisitor { + private static class TypeSymbolsCollector extends BaseTreeVisitor { - private final Set names = new HashSet<>(); + private final Set symbols = new HashSet<>(); void scanSignature(MethodTree methodTree) { scan(methodTree.returnType()); @@ -117,7 +87,10 @@ void scanSignature(MethodTree methodTree) { @Override public void visitIdentifier(IdentifierTree tree) { - names.add(tree.name()); + Symbol symbol = tree.symbol(); + if (symbol.isTypeSymbol()) { + symbols.add(symbol); + } } } diff --git a/java-checks/src/test/java/org/sonar/java/checks/InnerClassInInterfaceCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/InnerClassInInterfaceCheckTest.java index 64619075780..7d5cc670206 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/InnerClassInInterfaceCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/InnerClassInInterfaceCheckTest.java @@ -20,6 +20,7 @@ import org.sonar.java.checks.verifier.CheckVerifier; import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; +import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath; class InnerClassInInterfaceCheckTest { @@ -40,4 +41,12 @@ void test_without_semantic() { .verifyIssues(); } + @Test + void test_non_compiling() { + CheckVerifier.newVerifier() + .onFile(nonCompilingTestSourcesPath("checks/InnerClassInInterfaceCheckSample.java")) + .withCheck(new InnerClassInInterfaceCheck()) + .verifyIssues(); + } + } From af2783e0ffb0a0cb69ba5ade57e901af6cce75bf Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Mon, 26 Oct 2026 13:43:06 +0100 Subject: [PATCH 3/3] SONARJAVA-7240 Update S9398 ruling expectations for eclipse-jetty-similar-to-main Co-Authored-By: Claude Opus 5.5 (1M context) --- .../expected/eclipse-jetty-similar-to-main/java-S9398.json | 3 --- 1 file changed, 3 deletions(-) diff --git a/its/ruling/src/test/resources/expected/eclipse-jetty-similar-to-main/java-S9398.json b/its/ruling/src/test/resources/expected/eclipse-jetty-similar-to-main/java-S9398.json index d1153f94980..6af8160de39 100644 --- a/its/ruling/src/test/resources/expected/eclipse-jetty-similar-to-main/java-S9398.json +++ b/its/ruling/src/test/resources/expected/eclipse-jetty-similar-to-main/java-S9398.json @@ -14,9 +14,6 @@ "eclipse-jetty:jetty-io/src/main/java/org/eclipse/jetty/io/Connection.java": [ 161 ], -"eclipse-jetty:jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslHandshakeListener.java": [ -57 -], "eclipse-jetty:jetty-server/src/main/java/org/eclipse/jetty/server/Authentication.java": [ 37 ],