Merge pull request #1020 from forcedotcom/d/W-12696959

FIX (GraphEngine): @W-12696959@: UnusedMethodRule now fully supports constructors.

Author: jfeingold35

sfge/src/main/java/com/salesforce/rules/UnusedMethodRule.java

@@ -144,11 +144,9 @@ private List<Violation> convertMethodsToViolations() {
      * for filtering the list of all possible candidates into just the eligible ones.
      */
     private boolean methodIsIneligible(MethodVertex vertex) {
-        // TODO: At this time, only static methods, private instance methods, and private/protected
-        //       constructors are supported. This limit will be loosened over time, and eventually
-        //       removed entirely.
-        if (!vertex.isStatic()
-                && !(vertex.isPrivate() || (vertex.isProtected() && vertex.isConstructor()))) {
+        // TODO: At this time, only static methods, constructors, and private instance methods are
+        //       supported. This limit will be loosened over time, and eventually removed entirely.
+        if (!vertex.isStatic() && !vertex.isConstructor() && !vertex.isPrivate()) {
             return true;
         }
 

sfge/src/main/java/com/salesforce/rules/unusedmethod/ConstructorMethodCallValidator.java

@@ -68,7 +68,16 @@ protected boolean internalUsageDetected() {
      */
     @Override
     protected boolean externalUsageDetected() {
-        // TODO: IMPLEMENT THIS METHOD
+        // Get all `new Whatever()` calls for the defining type.
+        List<NewObjectExpressionVertex> potentialExternalCalls =
+                ruleStateTracker.getConstructionsOfType(targetMethod.getDefiningType());
+        // Test each one to see whether its parameters match.
+        for (NewObjectExpressionVertex potentialExternalCall : potentialExternalCalls) {
+            if (parametersAreValid(potentialExternalCall)) {
+                return true;
+            }
+        }
+        // If we're here, it's because we couldn't find any matches.
         return false;
     }
 }

sfge/src/main/java/com/salesforce/rules/unusedmethod/RuleStateTracker.java

@@ -13,8 +13,10 @@
 import com.salesforce.graph.vertex.*;
 import java.util.*;
 import java.util.stream.Collectors;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal;
 import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
 import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
 
 /**
  * A helper class for {@link com.salesforce.rules.UnusedMethodRule}, which tracks various elements
@@ -238,6 +240,48 @@ boolean classInheritsMatchingMethod(String definingType, String signature) {
         return MethodUtil.getMethodWithSignature(g, definingType, signature, true).isPresent();
     }
 
+    /**
+     * Get all {@link NewObjectExpressionVertex} instances representing instantiations of an object
+     * whose type is {@code constructedType}.
+     */
+    List<NewObjectExpressionVertex> getConstructionsOfType(String constructedType) {
+        // Start off with all NewObjectExpressionVertex instances whose declared type references the
+        // desired type.
+        List<NewObjectExpressionVertex> results =
+                SFVertexFactory.loadVertices(
+                        g,
+                        g.V()
+                                .where(
+                                        H.has(
+                                                NodeType.NEW_OBJECT_EXPRESSION,
+                                                Schema.TYPE,
+                                                constructedType)));
+
+        // Inner types can be referenced by outer/sibling types by just the inner name rather than
+        // the full name.
+        // This means we need to do more, but what that "more" is depends on whether we're looking
+        // at an inner or outer type.
+        boolean typeIsInner = constructedType.contains(".");
+        if (typeIsInner) {
+            // If the type is inner, then we need to add inner-name-only references occurring in
+            // other classes.
+            String[] portions = constructedType.split("\\.");
+            String outerType = portions[0];
+            String innerType = portions[1];
+            GraphTraversal<Vertex, Vertex> traversal =
+                    g.V().where(H.has(NodeType.NEW_OBJECT_EXPRESSION, Schema.TYPE, innerType));
+            List<NewObjectExpressionVertex> additionalResults =
+                    getAliasedInnerTypeUsages(outerType, NodeType.NEW_OBJECT_EXPRESSION, traversal);
+            results.addAll(additionalResults);
+        } else {
+            // If the type isn't an inner type, then it's possible that some of the references we
+            // found are actually
+            // inner-name-only references to inner types. So we need to remove those.
+            results = removeInnerTypeCollisions(results, constructedType);
+        }
+        return results;
+    }
+
     /**
      * Get all {@link MethodCallExpressionVertex} instances representing invocations of a method
      * named {@code methodName} on a thing called {@code referencedType}.
@@ -262,45 +306,17 @@ List<MethodCallExpressionVertex> getInvocationsOnType(
             String[] portions = referencedType.split("\\.");
             String outerType = portions[0];
             String innerType = portions[1];
+            GraphTraversal<Vertex, Vertex> traversal =
+                    TraversalUtil.traverseInvocationsOf(
+                            g, new ArrayList<>(), innerType, methodName);
             List<MethodCallExpressionVertex> additionalResults =
-                    SFVertexFactory.loadVertices(
-                            g,
-                            TraversalUtil.traverseInvocationsOf(
-                                            g, new ArrayList<>(), innerType, methodName)
-                                    .where(
-                                            __.or(
-                                                    H.has(
-                                                            NodeType.METHOD_CALL_EXPRESSION,
-                                                            Schema.DEFINING_TYPE,
-                                                            outerType),
-                                                    H.hasStartingWith(
-                                                            NodeType.METHOD_CALL_EXPRESSION,
-                                                            Schema.DEFINING_TYPE,
-                                                            outerType + "."))));
+                    getAliasedInnerTypeUsages(
+                            outerType, NodeType.METHOD_CALL_EXPRESSION, traversal);
             results.addAll(additionalResults);
         } else {
             // If the type isn't an inner type, then it's possible some of the references we found
             // are actually inner-name-only references to inner types. So we need to remove those.
-            results =
-                    results.stream()
-                            .filter(
-                                    v -> {
-                                        // Convert the result's definingType into an outer type by
-                                        // getting everything before the first period.
-                                        String outerType = v.getDefiningType().split("\\.")[0];
-                                        // Get any inner classes for the outer type and cast their
-                                        // names to lowercase.
-                                        Set<String> innerClassNames =
-                                                getInnerClasses(outerType).stream()
-                                                        .map(i -> i.getName().toLowerCase())
-                                                        .collect(Collectors.toSet());
-                                        // If the set lacks an entry for our referenced type, then
-                                        // there's no conflicting inner class and we can keep this
-                                        // result.
-                                        return !innerClassNames.contains(
-                                                referencedType.toLowerCase());
-                                    })
-                            .collect(Collectors.toList());
+            results = removeInnerTypeCollisions(results, referencedType);
         }
         return results;
     }
@@ -371,4 +387,59 @@ Optional<BaseSFVertex> getDeclarationOfReferencedValue(MethodCallExpressionVerte
         // Optional and let the caller figure out what to do with it.
         return Optional.empty();
     }
+
+    /**
+     * An inner class's sibling/outer classes can reference it by its inner type alone. This method
+     * accepts a traversal containing all potential references to the inner name, and filters for
+     * those that occur in the sibling/outer classes and therefore refer to the inner type.
+     *
+     * @param outerType - The name of the outer class.
+     * @param nodeType - The node type being targeted.
+     * @param initialTraversal - A traversal containing all nodes of {@code nodeType} that
+     *     potentially reference the inner type.
+     * @return - A list of {@link BaseSFVertex} instances representing the subset of {@code
+     *     initialTraversal} that are aliased inner type references.
+     * @param <T> - An extension of {@link BaseSFVertex}.
+     */
+    private <T extends BaseSFVertex> List<T> getAliasedInnerTypeUsages(
+            String outerType, String nodeType, GraphTraversal<Vertex, Vertex> initialTraversal) {
+        return SFVertexFactory.loadVertices(
+                g,
+                initialTraversal.where(
+                        __.or(
+                                H.has(nodeType, Schema.DEFINING_TYPE, outerType),
+                                H.hasStartingWith(
+                                        nodeType, Schema.DEFINING_TYPE, outerType + "."))));
+    }
+
+    /**
+     * If an inner class shares the name of an outer class, then references to the inner class that
+     * occur in its outer/sibling classes can resemble references to the outer class. This class
+     * filters such false positives from the results of a query.
+     *
+     * @param unfilteredResults - The results, pre-filtering
+     * @param referencedType - The outer type with which inner types may collide
+     * @return - {@code unfilteredResults}, with all colliding references removed
+     */
+    private <T extends BaseSFVertex> List<T> removeInnerTypeCollisions(
+            List<T> unfilteredResults, String referencedType) {
+        return unfilteredResults.stream()
+                .filter(
+                        v -> {
+                            // Convert the result's definingType into an outer type by getting
+                            // everything before the first period.
+                            String outerType = v.getDefiningType().split("\\.")[0];
+                            // Get any inner classes belonging to this outer type in a
+                            // case-insensitive set.
+                            Set<String> innerClassNames =
+                                    CollectionUtil.newTreeSetOf(
+                                            getInnerClasses(outerType).stream()
+                                                    .map(UserClassVertex::getName)
+                                                    .collect(Collectors.toList()));
+                            // If the set lacks an entry for our referenced type, then there's no
+                            // conflicting inner class, and we can keep this result.
+                            return !innerClassNames.contains(referencedType);
+                        })
+                .collect(Collectors.toList());
+    }
 }

sfge/src/test/java/com/salesforce/rules/unusedmethod/ConstructorsTest.java

@@ -6,7 +6,6 @@
 import com.salesforce.rules.Violation;
 import java.util.function.Consumer;
 import org.apache.commons.lang3.StringUtils;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.CsvSource;
 import org.junit.jupiter.params.provider.ValueSource;
@@ -23,13 +22,12 @@ public class ConstructorsTest extends BaseUnusedMethodTest {
      * @param declaration - The declaration of the tested constructor
      * @param arity - The constructor's arity
      */
-    // TODO: ENABLE MORE TESTS AS WE ADD MORE FUNCTIONALITY
     @CsvSource({
         // One test per constructor, per visibility scope.
         // EXCEPTION: No test for private 0-arity, since such methods are ineligible.
-        // "public MyClass(), 0",
+        "public MyClass(), 0",
         "protected MyClass(), 0",
-        // "public MyClass(boolean b), 1",
+        "public MyClass(boolean b), 1",
         "protected MyClass(boolean b), 1",
         "private MyClass(boolean b), 1"
     })
@@ -54,12 +52,7 @@ public void constructorWithoutInvocation_expectViolation(String declaration, int
      *
      * @param visibility - The visibility of the target constructor.
      */
-    @ValueSource(
-            strings = {
-                // "public",
-                "protected",
-                "private"
-            })
+    @ValueSource(strings = {"public", "protected", "private"})
     @ParameterizedTest(name = "{displayName}: {0} constructor")
     public void constructorCalledViaThis_expectNoViolation(String visibility) {
         // spotless:off
@@ -85,12 +78,7 @@ public void constructorCalledViaThis_expectNoViolation(String visibility) {
      * @param visibility - Visibility of the parent constructor
      * @param arity - Arity of the parent constructor
      */
-    @CsvSource({
-        // "public, 0",
-        // "public, 1",
-        "protected, 0",
-        "protected, 1"
-    })
+    @CsvSource({"public, 0", "public, 1", "protected, 0", "protected, 1"})
     @ParameterizedTest(name = "{displayName}: {0} constructor with arity {1}")
     public void constructorCalledViaSuper_expectNoViolation(String visibility, Integer arity) {
         // spotless:off
@@ -149,6 +137,35 @@ public void constructorUncalledByGrandchildSuper_expectViolation(Integer arity)
         assertViolations(sourceCodes, assertion);
     }
 
+    /**
+     * Simple tests for cases where one class's constructor is called somewhere in another class.
+     *
+     * @param arity - The arity of the tested constructor.
+     */
+    @ValueSource(ints = {0, 1})
+    @ParameterizedTest(name = "{displayName}: Arity {0}")
+    public void constructorCalledViaNew_expectNoViolation(Integer arity) {
+        // spotless:off
+        String[] sourceCodes = new String[]{
+            // Declare our tested class.
+            "global class TestedClass {\n"
+            // Declare a constructor with the expected arity that does nothing in particular.
+          + "    public TestedClass(" + StringUtils.repeat("boolean b", arity) + ") {}\n"
+          + "}",
+            // Declare a class to invoke the tested constructor.
+            "global class InvokerClass {\n"
+            // Annotate the invoker method to avoid tripping the rule.
+          + "    /* sfge-disable-stack UnusedMethodRule */\n"
+          + "    public static TestedClass invokeTestedConstructor() {\n"
+            // Invoke the tested constructor with however many parameters it expects.
+          + "        return new TestedClass(" + StringUtils.repeat("true", arity) + ");\n"
+          + "    }\n"
+          + "}"
+        };
+        // spotless:on
+        assertNoViolations(sourceCodes, 1);
+    }
+
     /**
      * Tests for cases where an inner class's constructor is called in the outer class.
      *
@@ -162,7 +179,6 @@ public void constructorUncalledByGrandchildSuper_expectViolation(Integer arity)
         "InnerClass, InnerClass"
     })
     @ParameterizedTest(name = "{displayName}: Declared as {0}, constructed as new {1}()")
-    @Disabled
     public void innerConstructorUsedByOuter_expectNoViolation(String variable, String constructor) {
         // spotless:off
         String sourceCode =
@@ -194,7 +210,6 @@ public void innerConstructorUsedByOuter_expectNoViolation(String variable, Strin
         "InnerClass, InnerClass"
     })
     @ParameterizedTest(name = "{displayName}: Declared as {0}, constructed as new {1}()")
-    @Disabled
     public void innerConstructorUsedBySiblingInner_expectNoViolation(
             String variable, String constructor) {
         // spotless:off
@@ -216,4 +231,47 @@ public void innerConstructorUsedBySiblingInner_expectNoViolation(
         // spotless:on
         assertNoViolations(sourceCode, 1);
     }
+
+    /**
+     * These tests cover variants of a weird edge case: If an inner class and an outer class share
+     * the same name (e.g., {@code Whatever} and {@code Outer.Whatever}), then calling {@code new
+     * Whatever()} in the inner class's outer/sibling classes should count as an invocation for the
+     * inner class, not the outer class.
+     *
+     * @param arity - The arity of the tested constructor.
+     */
+    @ValueSource(ints = {0, 1})
+    @ParameterizedTest(name = "{displayName}: Constructor of arity {0}")
+    public void externalReferenceSyntaxCollision_expectViolation(Integer arity) {
+        // spotless:off
+        String[] sourceCodes = new String[]{
+            // Declare an outer class with a constructor.
+            "global class CollidingName {\n"
+          + "    public CollidingName(" + StringUtils.repeat("boolean b", arity) + ") {}\n"
+          + "}",
+            // Declare another outer class.
+            "global class OuterClass {\n"
+            // Give the outer class an inner class whose name collides with the tested outer class.
+          + "    global class CollidingName {\n"
+            // Give the inner class a constructor with the same params as the outer class.
+          + "        public CollidingName(" + StringUtils.repeat("boolean b", arity) + ") {}\n"
+          + "    }\n"
+          + "    \n"
+            // Give the outer class a static method that instantiates the inner class.
+          + "    /* sfge-disable-stack UnusedMethodRule */\n"
+          + "    public static boolean callConstructor() {\n"
+            // IMPORTANT: This is where the constructor is called.
+          + "        CollidingName obj = new CollidingName(" + StringUtils.repeat("true", arity) +");\n"
+          + "        return true;\n"
+          + "    }\n"
+          + "}"
+        };
+        // spotless:on
+        assertViolations(
+                sourceCodes,
+                v -> {
+                    assertEquals("<init>", v.getSourceVertexName());
+                    assertEquals("CollidingName", v.getSourceDefiningType());
+                });
+    }
 }