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 ], 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 5eb3687dcb8..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 @@ -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,156 @@ 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 WithNameCollisionsInSignatures { + + java.util.Optional qualifiedForeignType(); + + Element typeParameter(); + + class Optional {} // Noncompliant +// ^^^^^^^^ + + class Element {} // Noncompliant +// ^^^^^^^ +} + +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..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 @@ -17,31 +17,80 @@ package org.sonar.java.checks; import java.util.Collections; +import java.util.HashSet; import java.util.List; +import java.util.Set; 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.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.MethodTree; import org.sonar.plugins.java.api.tree.Modifier; import org.sonar.plugins.java.api.tree.Tree; @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); + return Collections.singletonList(Tree.Kind.INTERFACE); } @Override public void visitNode(Tree tree) { - ClassTree classTree = (ClassTree) tree; - if (ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.ABSTRACT)) { - return; + 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) { + return classTree.symbol().type().isSubtypeOf(interfaceTree.symbol().type()); + } + + /** + * 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, 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.symbols; + } + + private static class TypeSymbolsCollector extends BaseTreeVisitor { + + private final Set symbols = new HashSet<>(); + + void scanSignature(MethodTree methodTree) { + scan(methodTree.returnType()); + methodTree.parameters().forEach(parameter -> scan(parameter.type())); + scan(methodTree.throwsClauses()); } - Tree parent = tree.parent(); - if (parent != null && parent.is(Tree.Kind.INTERFACE)) { - reportIssue(classTree.simpleName(), "Move this class out of this interface."); + + @Override + public void visitIdentifier(IdentifierTree tree) { + 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(); + } + }