SONARJAVA-7240: Improve S9398 to allow helper and builder types nested in interfaces

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
 ],

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
 ],

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
 ]
 }

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
+//      ^^^^^^^^^^^^^^^^^^^^^
+}

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> wrapped();
+
+  void consume(List<Map<String, Element>> 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<String> qualifiedForeignType();
+
+  <Element> Element typeParameter();
+
+  class Optional {} // Noncompliant
+//      ^^^^^^^^
+
+  class Element {} // Noncompliant
+//      ^^^^^^^
+}
+
+interface WithImplementationsUsedInSignatures {
+
+  WithImplementationsUsedInSignatures copy(Nested other);
+
+  Generic<String> generic();
+
+  Qualified qualified();
+
+  class Nested implements WithImplementationsUsedInSignatures { // Noncompliant
+//      ^^^^^^
+    @Override
+    public WithImplementationsUsedInSignatures copy(Nested other) {
+      return this;
+    }
+    @Override
+    public Generic<String> generic() {
+      return null;
+    }
+    @Override
+    public Qualified qualified() {
+      return null;
+    }
+  }
+
+  class Generic<T> implements WithImplementationsUsedInSignatures, Comparable<Generic<T>> { // Noncompliant
+//      ^^^^^^^
+    @Override
+    public WithImplementationsUsedInSignatures copy(Nested other) {
+      return this;
+    }
+    @Override
+    public Generic<String> generic() {
+      return null;
+    }
+    @Override
+    public Qualified qualified() {
+      return null;
+    }
+    @Override
+    public int compareTo(Generic<T> o) {
+      return 0;
+    }
+  }
+
+  class Qualified implements checks.WithImplementationsUsedInSignatures { // Noncompliant
+//      ^^^^^^^^^
+    @Override
+    public WithImplementationsUsedInSignatures copy(Nested other) {
+      return this;
+    }
+    @Override
+    public Generic<String> generic() {
+      return null;
+    }
+    @Override
+    public Qualified qualified() {
+      return null;
+    }
+  }
+}
+
+interface ComparableUser {
+
+  Helper helper();
+
+  class Helper implements Comparable<ComparableUser> { // 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
 

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<Tree.Kind> 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<Symbol> 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<Symbol> signatureTypes) {
+    return classTree.simpleName().name().endsWith(BUILDER_SUFFIX) || signatureTypes.contains(classTree.symbol());
+  }
+
+  private static Set<Symbol> 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<Symbol> 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);
+      }
     }
   }
 

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();
+  }
+
 }