Merge pull request #1085 from forcedotcom/rm/fixNestedLoopMMSLR

rmohan20 · web-flow · commit ce8880cd2f01 · 2023-06-02T14:08:08.000-07:00
FIX (GraphEngine): @W-13363157@: Handles loop exclusions more effectively
diff --git a/sfge/src/main/java/com/salesforce/collections/CollectionUtil.java b/sfge/src/main/java/com/salesforce/collections/CollectionUtil.java
@@ -3,12 +3,7 @@
 import com.google.common.collect.ImmutableList;
 import com.salesforce.exception.UnexpectedException;
 import com.salesforce.graph.ops.TypeableUtil;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.List;
-import java.util.Map;
-import java.util.TreeMap;
-import java.util.TreeSet;
+import java.util.*;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.ConcurrentSkipListMap;
 import java.util.function.BiFunction;
@@ -186,5 +181,20 @@ public static <T, U> List<U> newImmutableListOf(
                 .collect(Collectors.collectingAndThen(Collectors.toList(), ImmutableList::copyOf));
     }
 
+    /**
+     * Peek into the stack to get the last element. Converts value into an Optional instead of
+     * throwing an {@link EmptyStackException} when stack is empty.
+     *
+     * @param stack to look at
+     * @param <T> Stack's generic
+     * @return last element added to the stack
+     */
+    public static <T> Optional<T> peek(Stack<T> stack) {
+        if (stack.isEmpty()) {
+            return Optional.empty();
+        }
+        return Optional.of(stack.peek());
+    }
+
     private CollectionUtil() {}
 }
diff --git a/sfge/src/main/java/com/salesforce/graph/build/StaticBlockUtil.java b/sfge/src/main/java/com/salesforce/graph/build/StaticBlockUtil.java
@@ -5,6 +5,7 @@
 import com.salesforce.collections.CollectionUtil;
 import com.salesforce.exception.ProgrammingException;
 import com.salesforce.graph.Schema;
+import com.salesforce.graph.vertex.MethodCallExpressionVertex;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -68,7 +69,9 @@
 public final class StaticBlockUtil {
     private static final Logger LOGGER = LogManager.getLogger(StaticBlockUtil.class);
 
-    static final String SYNTHETIC_STATIC_BLOCK_METHOD_NAME = "SyntheticStaticBlock_%d";
+    private static final String SYNTHETIC_STATIC_BLOCK_METHOD_BASE = "SyntheticStaticBlock__";
+    static final String SYNTHETIC_STATIC_BLOCK_METHOD_NAME =
+            SYNTHETIC_STATIC_BLOCK_METHOD_BASE + "%d";
     static final String STATIC_BLOCK_INVOKER_METHOD = "StaticBlockInvoker";
 
     private StaticBlockUtil() {}
@@ -374,4 +377,9 @@ private static void verifyType(Vertex vertex, String expectedType) {
             throw new ProgrammingException("Incorrect vertex type: " + vertex.label());
         }
     }
+
+    public static boolean isStaticBlockMethodCall(MethodCallExpressionVertex vertex) {
+        final String methodName = vertex.getMethodName();
+        return methodName.startsWith(SYNTHETIC_STATIC_BLOCK_METHOD_BASE);
+    }
 }
diff --git a/sfge/src/main/java/com/salesforce/rules/ops/boundary/BoundaryDetector.java b/sfge/src/main/java/com/salesforce/rules/ops/boundary/BoundaryDetector.java
@@ -1,10 +1,12 @@
 package com.salesforce.rules.ops.boundary;
 
+import com.salesforce.collections.CollectionUtil;
 import com.salesforce.exception.ProgrammingException;
 import com.salesforce.exception.UnexpectedException;
-import com.salesforce.rules.ops.visitor.LoopDetectionVisitor;
+import java.util.List;
 import java.util.Optional;
 import java.util.Stack;
+import java.util.stream.Collectors;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
@@ -16,26 +18,15 @@
  * @param <T> Boundary type associated with the implementation.
  */
 public abstract class BoundaryDetector<T extends Boundary<R>, R> {
-    private static final Logger LOGGER = LogManager.getLogger(LoopDetectionVisitor.class);
-    private final Stack<T> boundaries;
+    private static final Logger LOGGER = LogManager.getLogger(BoundaryDetector.class);
+    protected final Stack<T> boundaries;
 
     protected BoundaryDetector() {
         this.boundaries = new Stack<>();
     }
 
-    private void push(T boundary) {
-        this.boundaries.push(boundary);
-    }
-
-    private T pop() {
-        return this.boundaries.pop();
-    }
-
     public Optional<T> peek() {
-        if (boundaries.isEmpty()) {
-            return Optional.empty();
-        }
-        return Optional.of(this.boundaries.peek());
+        return CollectionUtil.peek(boundaries);
     }
 
     /**
@@ -47,7 +38,7 @@ public void pushBoundary(T boundary) {
         if (LOGGER.isDebugEnabled()) {
             LOGGER.debug("Entering loop boundary with boundaryItem=" + boundary.getBoundaryItem());
         }
-        push(boundary);
+        this.boundaries.push(boundary);
     }
 
     /**
@@ -78,6 +69,13 @@ public void popBoundary(R boundaryItem) {
         }
 
         // Remove the boundary
-        pop();
+        this.boundaries.pop();
+    }
+
+    /**
+     * @return items in the boundary stack. Items are ordered TODO ?
+     */
+    public List<T> getBoundaryItems() {
+        return boundaries.stream().collect(Collectors.toList());
     }
 }
diff --git a/sfge/src/main/java/com/salesforce/rules/ops/boundary/LoopBoundaryDetector.java b/sfge/src/main/java/com/salesforce/rules/ops/boundary/LoopBoundaryDetector.java
@@ -1,21 +1,47 @@
 package com.salesforce.rules.ops.boundary;
 
+import com.salesforce.collections.CollectionUtil;
 import com.salesforce.graph.vertex.SFVertex;
 import java.util.Optional;
+import java.util.Stack;
 
 /** Implementation of boundary detection for loops. */
 public class LoopBoundaryDetector extends BoundaryDetector<LoopBoundary, SFVertex> {
+
+    /**
+     * Check if the visitor is currently inside a loop. Takes exclusion boundary into account.
+     *
+     * @return Optional of the last effective boundary vertex. If none exist, return empty to
+     *     indicate that exclusion is effective.
+     */
     public Optional<? extends SFVertex> isInsideLoop() {
-        final Optional<LoopBoundary> loopBoundaryOpt = peek();
+        // TODO: This can be done without the copy stack as well, but it adds more complexity
+        //  than I'm willing to handle at the moment. Add more tests if revisiting.
+        final Stack<LoopBoundary> copyStack = new Stack<>();
+        copyStack.addAll(super.boundaries);
+        return this._isInsideLoop(copyStack);
+    }
+
+    private Optional<? extends SFVertex> _isInsideLoop(
+            Stack<? extends LoopBoundary> boundaryStack) {
+        final Optional<? extends LoopBoundary> loopBoundaryOpt = CollectionUtil.peek(boundaryStack);
         if (loopBoundaryOpt.isPresent()) {
+            final LoopBoundary loopBoundary = loopBoundaryOpt.get();
             // Check if we are inside a loop exclusion.
-            if (loopBoundaryOpt.get() instanceof LoopExclusionBoundary) {
-                // This is exclusion zone. We consider this not a loop.
+
+            if (loopBoundary instanceof PermanentLoopExclusionBoundary) {
+                // All the previous boundaries don't matter
                 return Optional.empty();
+            } else if (loopBoundary instanceof OverridableLoopExclusionBoundary) {
+                // Pop out the OverridableLoopExclusionBoundary.
+                boundaryStack.pop();
+                // Pop out the corresponding ForEach Loop Boundary.
+                boundaryStack.pop();
+                return _isInsideLoop(boundaryStack);
+            } else {
+                // Inside a loop.
+                return Optional.of(loopBoundary.getBoundaryItem());
             }
-
-            // Inside a loop.
-            return Optional.of(loopBoundaryOpt.get().getBoundaryItem());
         }
 
         // Not inside a loop
diff --git a/sfge/src/main/java/com/salesforce/rules/ops/boundary/LoopExclusionBoundary.java b/sfge/src/main/java/com/salesforce/rules/ops/boundary/LoopExclusionBoundary.java
diff --git a/sfge/src/main/java/com/salesforce/rules/ops/boundary/OverridableLoopExclusionBoundary.java b/sfge/src/main/java/com/salesforce/rules/ops/boundary/OverridableLoopExclusionBoundary.java
@@ -0,0 +1,17 @@
+package com.salesforce.rules.ops.boundary;
+
+import com.salesforce.graph.vertex.SFVertex;
+
+/**
+ * Indicates parts of the loop that get run only once. However, a nested loop scenario can still
+ * execute this portion multiple times. For example:
+ *
+ * <ul>
+ *   <li>getValues() method in: <code>for (String s: getValues())</code>
+ * </ul>
+ */
+public class OverridableLoopExclusionBoundary extends LoopBoundary {
+    public OverridableLoopExclusionBoundary(SFVertex loopVertex) {
+        super(loopVertex);
+    }
+}
diff --git a/sfge/src/main/java/com/salesforce/rules/ops/boundary/PermanentLoopExclusionBoundary.java b/sfge/src/main/java/com/salesforce/rules/ops/boundary/PermanentLoopExclusionBoundary.java
@@ -0,0 +1,15 @@
+package com.salesforce.rules.ops.boundary;
+
+import com.salesforce.graph.vertex.SFVertex;
+
+/**
+ * Indicates portions of code that will be executed only once no matter how many loops this is
+ * nested under.
+ *
+ * <p>For example: Static initialization of a class
+ */
+public class PermanentLoopExclusionBoundary extends LoopBoundary {
+    public PermanentLoopExclusionBoundary(SFVertex loopVertex) {
+        super(loopVertex);
+    }
+}
diff --git a/sfge/src/main/java/com/salesforce/rules/ops/visitor/LoopDetectionVisitor.java b/sfge/src/main/java/com/salesforce/rules/ops/visitor/LoopDetectionVisitor.java
@@ -1,11 +1,13 @@
 package com.salesforce.rules.ops.visitor;
 
+import com.salesforce.graph.build.StaticBlockUtil;
 import com.salesforce.graph.symbols.SymbolProvider;
 import com.salesforce.graph.vertex.*;
 import com.salesforce.graph.visitor.DefaultNoOpPathVertexVisitor;
 import com.salesforce.rules.ops.boundary.LoopBoundary;
 import com.salesforce.rules.ops.boundary.LoopBoundaryDetector;
-import com.salesforce.rules.ops.boundary.LoopExclusionBoundary;
+import com.salesforce.rules.ops.boundary.OverridableLoopExclusionBoundary;
+import com.salesforce.rules.ops.boundary.PermanentLoopExclusionBoundary;
 import java.util.Optional;
 
 /** Visitor that gets notified when a loop vertex is invoked in the path. */
@@ -78,28 +80,49 @@ public boolean visit(MethodCallExpressionVertex vertex, SymbolProvider symbols)
         final Optional<LoopBoundary> currentLoopBoundaryOpt = loopBoundaryDetector.peek();
         if (currentLoopBoundaryOpt.isPresent()) {
             final SFVertex loopBoundaryItem = currentLoopBoundaryOpt.get().getBoundaryItem();
+
+            createPermanentLoopExclusionIfApplicable(vertex, loopBoundaryItem);
+
+            // If permanent loop exclusion was added, overridable loop exclusion will never get
+            // added.
+            // Leaving the conditional check here to be more obvious.
             if (loopBoundaryItem instanceof ForEachStatementVertex) {
-                // We are within a ForEach statement.
-                // Check if the method calls parent is the same as this ForEach statement.
-                // If they are the same, this method would get invoked only once.
-                BaseSFVertex parentVertex = vertex.getParent();
-                if (parentVertex instanceof ForEachStatementVertex
-                        && parentVertex.equals(loopBoundaryItem)) {
-                    // This method would get invoked only once.
-                    loopBoundaryDetector.pushBoundary(new LoopExclusionBoundary(vertex));
-                }
+                createOverridableLoopExclusion(vertex, loopBoundaryItem);
             }
         }
         return true;
     }
 
+    private void createPermanentLoopExclusionIfApplicable(
+            MethodCallExpressionVertex vertex, SFVertex loopBoundaryItem) {
+        if (StaticBlockUtil.isStaticBlockMethodCall(vertex)) {
+            // All nested loops before this don't get counted as a loop context.
+            loopBoundaryDetector.pushBoundary(new PermanentLoopExclusionBoundary(vertex));
+        }
+    }
+
+    private void createOverridableLoopExclusion(
+            MethodCallExpressionVertex vertex, SFVertex loopBoundaryItem) {
+        // We are within a ForEach statement.
+        // Check if the method calls parent is the same as this ForEach statement.
+        // If they are the same, this method would get invoked only once.
+        BaseSFVertex parentVertex = vertex.getParent();
+        if (parentVertex instanceof ForEachStatementVertex
+                && parentVertex.equals(loopBoundaryItem)) {
+            // This method would get invoked only once within the immediate surrounding loop
+            // context
+            loopBoundaryDetector.pushBoundary(new OverridableLoopExclusionBoundary(vertex));
+        }
+    }
+
     @Override
     public void afterVisit(MethodCallExpressionVertex vertex, SymbolProvider symbols) {
         // If within a method call loop exclusion, pop boundary here.
         final Optional<LoopBoundary> currentLoopBoundaryOpt = loopBoundaryDetector.peek();
         if (currentLoopBoundaryOpt.isPresent()) {
             final LoopBoundary loopBoundary = currentLoopBoundaryOpt.get();
-            if (loopBoundary instanceof LoopExclusionBoundary) {
+            if (loopBoundary instanceof OverridableLoopExclusionBoundary
+                    || loopBoundary instanceof PermanentLoopExclusionBoundary) {
                 // We are in exclusion zone. Check if the method call on the loop exclusion boundary
                 // is the same as the current method call.
                 if (vertex.equals(loopBoundary.getBoundaryItem())) {
diff --git a/sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleTest.java b/sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleTest.java
@@ -283,8 +283,7 @@ public void testForLoopConditionalOnClassInstance() {
                         ASTConstants.NodeType.FOR_EACH_STATEMENT));
     }
 
-    @Test // TODO: Check if this is a false positive. Static block should get invoked only once
-    @Disabled
+    @Test
     public void testLoopFromStaticBlock() {
         // spotless:off
         String[] sourceCode = {
@@ -309,6 +308,39 @@ public void testLoopFromStaticBlock() {
         assertNoViolation(RULE, sourceCode);
     }
 
+    @Test
+    public void testLoopFromStaticBlock_nestedLoop() {
+        // spotless:off
+        String[] sourceCode = {
+            "public class MyClass {\n"
+                + "   void foo(String[] objectNames) {\n"
+                + "       for (Integer i = 0; i < objectNames.size; i++) {\n"
+                + "           FirstClass fc = new FirstClass();\n"
+                + "           fc.redirect(i);\n"
+                + "       }\n"
+                + "   }\n"
+                + "}\n",
+            "public class FirstClass {\n" +
+                "   void redirect(Integer lim) {\n" +
+                "       for (Integer i = 0; i < lim; i++) {\n" +
+                "           AnotherClass.doNothing();\n" +
+                "       }\n" +
+                "   }\n" +
+                "}\n",
+            "public class AnotherClass {\n"
+                + "   static {\n"
+                + "       Schema.getGlobalDescribe();\n"
+                + "   }\n"
+                + "   static void doNothing() {\n"
+                + "   // do nothing \n"
+                + "   }\n"
+                + "}\n"
+        };
+        // spotless:on
+
+        assertNoViolation(RULE, sourceCode);
+    }
+
     @CsvSource({
         "ForEachStatement, for (String s : myList)",
         "ForLoopStatement, for (Integer i; i < s.size; s++)",
diff --git a/sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/SchemaLookupInAnotherMethodTest.java b/sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/SchemaLookupInAnotherMethodTest.java
