SONARJS-709 Use live variable analysis to avoid execution duplicated paths (#282)

* SONARJS-709 Extract LVA from DeadStoreCheck

* SONARJS-709 Use live variable analysis to avoid execution duplicated paths

* SONARJS-709 Add unit test for LVA

* SONARJS-709 Clean program state with live IN symbols

* Fix some quality flaws

* Keep constraint for top expression stack element

Author: vilchik-elena

its/ruling/src/test/expected/javascript-S2259.json

@@ -1,4 +1,10 @@
 {
+'project:dojo-1.8.1/dijit/_editor/RichText.js.uncompressed.js':[
+2577,
+],
+'project:dojo-1.8.1/dijit/dijit-all.js.uncompressed.js':[
+14184,
+],
 'project:dojo-1.8.1/dijit/tree/dndSource.js.uncompressed.js':[
 440,
 ],
@@ -154,6 +160,9 @@
 'project:open-layers-2.12/OpenLayers/Map.js':[
 1877,
 ],
+'project:open-layers-2.12/OpenLayers/Renderer/SVG.js':[
+338,
+],
 'project:open-layers-2.12/OpenLayers/Renderer/VML.js':[
 297,
 ],
@@ -165,6 +174,7 @@
 7633,
 ],
 'project:processing-1.3.6.js':[
+2072,
 2871,
 2879,
 2999,

its/ruling/src/test/expected/javascript-S2583.json

@@ -3,6 +3,9 @@
 4811,
 8964,
 ],
+'project:dojo-1.8.1/dijit/_TemplatedMixin.js.uncompressed.js':[
+202,
+],
 'project:dojo-1.8.1/dijit/_base/focus.js.uncompressed.js':[
 146,
 ],
@@ -13,6 +16,7 @@
 'project:dojo-1.8.1/dijit/dijit.js.uncompressed.js':[
 2523,
 4931,
+5406,
 5736,
 6770,
 ],
@@ -214,6 +218,7 @@
 14477,
 14483,
 15461,
+16164,
 16520,
 ],
 'project:dojo-1.8.1/dojox/collections/BinaryTree.js.uncompressed.js':[
@@ -284,6 +289,12 @@
 'project:dojo-1.8.1/dojox/fx/split.js.uncompressed.js':[
 520,
 ],
+'project:dojo-1.8.1/dojox/gantt/GanttProjectControl.js.uncompressed.js':[
+735,
+750,
+794,
+812,
+],
 'project:dojo-1.8.1/dojox/gantt/GanttTaskControl.js.uncompressed.js':[
 1254,
 ],
@@ -322,6 +333,7 @@
 'project:dojo-1.8.1/dojox/grid/DataGrid.js.uncompressed.js':[
 2885,
 6522,
+7812,
 12288,
 13871,
 ],
@@ -580,26 +592,37 @@
 ],
 'project:yui-3.7.3/simpleyui/simpleyui.js':[
 1056,
+5595,
 6597,
 12788,
+19860,
+],
+'project:yui-3.7.3/stylesheet/stylesheet.js':[
+136,
 ],
 'project:yui-3.7.3/widget-modality/widget-modality.js':[
 518,
 ],
 'project:yui-3.7.3/yui-base/yui-base.js':[
 1056,
+5595,
 ],
 'project:yui-3.7.3/yui-core/yui-core.js':[
 1056,
 ],
+'project:yui-3.7.3/yui-log/yui-log.js':[
+54,
+],
 'project:yui-3.7.3/yui-nodejs/yui-nodejs.js':[
 1056,
+4516,
 6477,
 6719,
 6754,
 ],
 'project:yui-3.7.3/yui/yui.js':[
 1056,
+5595,
 7476,
 7718,
 7753,

javascript-checks/src/main/java/org/sonar/javascript/checks/DeadStoreCheck.java

@@ -19,21 +19,14 @@
  */
 package org.sonar.javascript.checks;
 
-import com.google.common.collect.HashMultimap;
 import com.google.common.collect.Lists;
-import com.google.common.collect.SetMultimap;
-import java.util.ArrayDeque;
-import java.util.Deque;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
 import java.util.Set;
-import javax.annotation.CheckForNull;
-import javax.annotation.Nullable;
 import org.sonar.check.Priority;
 import org.sonar.check.Rule;
 import org.sonar.javascript.cfg.CfgBlock;
 import org.sonar.javascript.cfg.ControlFlowGraph;
+import org.sonar.javascript.se.LiveVariableAnalysis;
+import org.sonar.javascript.se.LiveVariableAnalysis.Usages;
 import org.sonar.javascript.tree.symbols.Scope;
 import org.sonar.plugins.javascript.api.symbols.Symbol;
 import org.sonar.plugins.javascript.api.symbols.Usage;
@@ -43,14 +36,15 @@
 import org.sonar.plugins.javascript.api.tree.declaration.FunctionTree;
 import org.sonar.plugins.javascript.api.tree.declaration.MethodDeclarationTree;
 import org.sonar.plugins.javascript.api.tree.expression.ArrowFunctionTree;
-import org.sonar.plugins.javascript.api.tree.expression.AssignmentExpressionTree;
 import org.sonar.plugins.javascript.api.tree.expression.FunctionExpressionTree;
-import org.sonar.plugins.javascript.api.tree.expression.IdentifierTree;
 import org.sonar.plugins.javascript.api.tree.statement.BlockTree;
 import org.sonar.plugins.javascript.api.visitors.DoubleDispatchVisitorCheck;
 import org.sonar.squidbridge.annotations.ActivatedByDefault;
 import org.sonar.squidbridge.annotations.SqaleConstantRemediation;
 
+import static org.sonar.javascript.se.LiveVariableAnalysis.isRead;
+import static org.sonar.javascript.se.LiveVariableAnalysis.isWrite;
+
 @Rule(
   key = "S1854",
   name = "Dead Stores should be removed",
@@ -97,215 +91,50 @@ private void checkFunction(FunctionTree functionTree) {
   // Identifying dead stores is basically "live variable analysis".
   // See https://en.wikipedia.org/wiki/Live_variable_analysis
   private void checkCFG(ControlFlowGraph cfg, FunctionTree functionTree) {
-    Usages usages = new Usages(functionTree);
-    Set<BlockLiveness> livenesses = new HashSet<>();
-    buildUsagesAndLivenesses(cfg, usages, livenesses);
+    Scope scope = getContext().getSymbolModel().getScope(functionTree);
+    LiveVariableAnalysis lva = LiveVariableAnalysis.create(cfg, scope);
+    Usages usages = lva.getUsages();
 
-    for (BlockLiveness blockLiveness : livenesses) {
-      checkBlock(blockLiveness, usages);
-    }
+    for (CfgBlock cfgBlock : cfg.blocks()) {
+      Set<Symbol> live = lva.getLiveOutSymbols(cfgBlock);
 
-    for (Symbol symbol : usages.neverReadSymbols()) {
-      for (Usage usage : symbol.usages()) {
-        if (isWrite(usage)) {
-          addIssue(usage.identifierTree(), symbol);
+      for (Tree element : Lists.reverse(cfgBlock.elements())) {
+        Usage usage = usages.getUsage(element);
+        if (usage != null) {
+          checkUsage(usage, live, usages);
         }
       }
     }
-  }
-
-  private void checkBlock(BlockLiveness blockLiveness, Usages usages) {
-    Set<Symbol> live = blockLiveness.liveOut;
-
-    for (Tree element : Lists.reverse(blockLiveness.block.elements())) {
-      Usage usage = usages.getUsage(element);
 
-      if (isWrite(usage)) {
-
-        Symbol symbol = symbol(usage);
-        if (!live.contains(symbol) && !usages.hasUsagesInNestedFunctions(symbol) && !usages.isNeverRead(symbol)) {
-          addIssue(usage.identifierTree(), symbol);
-        }
-        live.remove(symbol);
-
-      } else if (isRead(usage)) {
-        live.add(symbol(usage));
-      }
-    }
+    raiseIssuesForNeverReadSymbols(usages);
   }
 
-  private void addIssue(Tree element, Symbol symbol) {
-    addIssue(element, String.format(MESSAGE, symbol.name()));
-  }
+  private void checkUsage(Usage usage, Set<Symbol> liveSymbols, Usages usages) {
+    Symbol symbol = usage.symbol();
 
-  private void buildUsagesAndLivenesses(ControlFlowGraph cfg, Usages usages, Set<BlockLiveness> livenesses) {
-    Map<CfgBlock, BlockLiveness> livenessPerBlock = new HashMap<>();
-    for (CfgBlock block : cfg.blocks()) {
-      BlockLiveness blockLiveness = new BlockLiveness(block, usages);
-      livenessPerBlock.put(block, blockLiveness);
-      livenesses.add(blockLiveness);
-    }
-
-    // To compute the set of variables which are live OUT of a block, we need to have the
-    // set of variables which are live IN its successors.
-    // As the CFG may contain cycles between blocks (that's the case for loops), we use a queue
-    // to keep track of blocks which may need to be updated.
-    // See "worklist algorithm" in http://www.cs.cornell.edu/courses/cs4120/2013fa/lectures/lec26-fa13.pdf
-    Deque<CfgBlock> queue = new ArrayDeque(cfg.blocks());
-    while (!queue.isEmpty()) {
-      CfgBlock block = queue.pop();
-      BlockLiveness blockLiveness = livenessPerBlock.get(block);
-      boolean changed = blockLiveness.updateLiveInAndOut(cfg, livenessPerBlock);
-      if (changed) {
-        for (CfgBlock predecessor : block.predecessors()) {
-          queue.push(predecessor);
-        }
+    if (isWrite(usage)) {
+      if (!liveSymbols.contains(symbol) && !usages.hasUsagesInNestedFunctions(symbol) && !usages.neverReadSymbols().contains(symbol)) {
+        addIssue(usage.identifierTree(), symbol);
       }
-    }
-  }
-
-  @CheckForNull
-  public static Symbol symbol(Usage usage) {
-    return usage.identifierTree().symbol();
-  }
-
-  private static boolean isRead(Usage usage) {
-    if (usage == null) {
-      return false;
-    }
-    return usage.kind() == Usage.Kind.READ || usage.kind() == Usage.Kind.READ_WRITE;
-  }
+      liveSymbols.remove(symbol);
 
-  private static boolean isWrite(Usage usage) {
-    if (usage == null) {
-      return false;
+    } else if (isRead(usage)) {
+      liveSymbols.add(symbol);
     }
-    return usage.kind() == Usage.Kind.WRITE || usage.kind() == Usage.Kind.DECLARATION_WRITE;
   }
 
-  private static class BlockLiveness {
-  
-    private final CfgBlock block;
-    private final Usages usages;
-    private final Set<Symbol> liveOut = new HashSet<>();
-    private Set<Symbol> liveIn = new HashSet<>();
-  
-    public BlockLiveness(CfgBlock block, Usages usages) {
-      this.usages = usages;
-      this.block = block;
-  
-      for (Tree element : block.elements()) {
-        if (element instanceof IdentifierTree) {
-          usages.add((IdentifierTree) element);
-        }
-        if (element.is(Kind.ASSIGNMENT)) {
-          usages.addAssignment((AssignmentExpressionTree) element);
-        }
-      }
-    }
-
-    public boolean updateLiveInAndOut(ControlFlowGraph cfg, Map<CfgBlock, BlockLiveness> livenessPerBlock) {
-      liveOut.clear();
-      for (CfgBlock successor : block.successors()) {
-        liveOut.addAll(livenessPerBlock.get(successor).liveIn);
-      }
-
-      Set<Symbol> oldIn = liveIn;
-      liveIn = new HashSet<>(liveOut);
-
-      for (Tree element : Lists.reverse(block.elements())) {
-        Usage usage = usages.getUsage(element);
+  private void raiseIssuesForNeverReadSymbols(Usages usages) {
+    for (Symbol symbol : usages.neverReadSymbols()) {
+      for (Usage usage : symbol.usages()) {
         if (isWrite(usage)) {
-          liveIn.remove(symbol(usage));
-        } else if (isRead(usage)) {
-          liveIn.add(symbol(usage));
+          addIssue(usage.identifierTree(), symbol);
         }
       }
-
-      return !oldIn.equals(liveIn);
     }
   }
 
-  private class Usages {
-
-    private final Scope functionScope;
-    private final Set<Symbol> symbols = new HashSet<>();
-    private final Map<IdentifierTree, Usage> localVariableUsages = new HashMap<>();
-    private final Set<Symbol> neverReadSymbols = new HashSet<>();
-    private final SetMultimap<Symbol, Usage> usagesInCFG = HashMultimap.create();
-    private final Set<Tree> assignmentVariables = new HashSet<>();
-
-    public Usages(FunctionTree function) {
-      this.functionScope = getContext().getSymbolModel().getScope(function);
-    }
-
-    public Usage getUsage(Tree element) {
-      if (assignmentVariables.contains(element)) {
-        return null;
-      }
-      if (element.is(Kind.ASSIGNMENT)) {
-        return localVariableUsages.get(((AssignmentExpressionTree) element).variable());
-      }
-      return localVariableUsages.get(element);
-    }
-
-    public boolean hasUsagesInNestedFunctions(Symbol symbol) {
-      return usagesInCFG.get(symbol).size() != symbol.usages().size();
-    }
-
-    @CheckForNull
-    public Usage add(IdentifierTree identifier) {
-      addSymbol(identifier.symbol());
-      Usage usage = localVariableUsages.get(identifier);
-      if (usage != null) {
-        usagesInCFG.put(identifier.symbol(), usage);
-      }
-      return usage;
-    }
-
-    private void addSymbol(@Nullable Symbol symbol) {
-      if (symbol == null || symbols.contains(symbol)) {
-        return;
-      }
-
-      symbols.add(symbol);
-
-      if (isLocalVariable(symbol)) {
-        boolean readAtLeastOnce = false;
-        for (Usage usage : symbol.usages()) {
-          localVariableUsages.put(usage.identifierTree(), usage);
-          if (isRead(usage)) {
-            readAtLeastOnce = true;
-          }
-        }
-        if (!readAtLeastOnce) {
-          neverReadSymbols.add(symbol);
-        }
-      }
-    }
-
-    private boolean isLocalVariable(Symbol symbol) {
-      Scope scope = symbol.scope();
-      while (!scope.isGlobal()) {
-        if (scope.equals(functionScope)) {
-          return true;
-        }
-        scope = scope.outer();
-      }
-      return false;
-    }
-
-    public Set<Symbol> neverReadSymbols() {
-      return neverReadSymbols;
-    }
-
-    public boolean isNeverRead(Symbol symbol) {
-      return neverReadSymbols.contains(symbol);
-    }
-
-    public void addAssignment(AssignmentExpressionTree tree) {
-      assignmentVariables.add(tree.variable());
-    }
+  private void addIssue(Tree element, Symbol symbol) {
+    addIssue(element, String.format(MESSAGE, symbol.name()));
   }
 
 }