From e625c6a67ebac558a83bb9e73f37659f38df8a92 Mon Sep 17 00:00:00 2001 From: dimvar Date: Wed, 28 Jun 2017 15:16:16 -0700 Subject: [PATCH] [NTI] Build the workset iteratively instead of recursively to avoid blowing the stack in very large programs. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=160458087 --- .../google/javascript/jscomp/NTIWorkset.java | 190 ++++++++++++++++++ .../javascript/jscomp/NewTypeInference.java | 129 +----------- .../jscomp/NewTypeInferenceTest.java | 31 ++- 3 files changed, 226 insertions(+), 124 deletions(-) create mode 100644 src/com/google/javascript/jscomp/NTIWorkset.java diff --git a/src/com/google/javascript/jscomp/NTIWorkset.java b/src/com/google/javascript/jscomp/NTIWorkset.java new file mode 100644 index 00000000000..f5398d37d45 --- /dev/null +++ b/src/com/google/javascript/jscomp/NTIWorkset.java @@ -0,0 +1,190 @@ +/* + * Copyright 2017 The Closure Compiler Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.javascript.jscomp; + +import com.google.common.base.Preconditions; +import com.google.javascript.jscomp.ControlFlowGraph.Branch; +import com.google.javascript.jscomp.graph.DiGraph.DiGraphEdge; +import com.google.javascript.jscomp.graph.DiGraph.DiGraphNode; +import com.google.javascript.rhino.Node; +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Deque; +import java.util.Iterator; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; + +/** + * Represents the workset used by the flow-sensitive analysis in NTI. + * The workset is computed happens iteratively, otherwise large programs can cause stack overflow. + */ +public class NTIWorkset { + private final ControlFlowGraph cfg; + // What this class computes. Represents the workset used by the flow-sensitive analysis in NTI. + private final List> ntiWorkset; + // The algorithm that computes the NTI workset itself uses a workset. + private Deque> workset; + // If a node is in this set, don't revisit it. + private Set> seen; + + NTIWorkset(ControlFlowGraph cfg) { + this.cfg = cfg; + this.ntiWorkset = new ArrayList<>(); + this.workset = new ArrayDeque<>(); + this.seen = new LinkedHashSet<>(); + buildWorkset(); + } + + private void buildWorkset() { + Preconditions.checkState(ntiWorkset.isEmpty()); + workset.push(cfg.getEntry()); + while (!workset.isEmpty()) { + processGraphNode(); + } + workset = null; + seen = null; + } + + Iterable> forward() { + Preconditions.checkState(!ntiWorkset.isEmpty()); + return ntiWorkset; + } + + /** + * The backwards analysis in NTI traverses the workset in the reverse direction. + */ + private class BackwardIterator implements Iterator> { + int i = ntiWorkset.size() - 1; + + @Override + public boolean hasNext() { + return i >= 0; + } + + @Override + public DiGraphNode next() { + return ntiWorkset.get(i--); + } + + @Override + public void remove() { + throw new UnsupportedOperationException(); + } + } + + Iterable> backward() { + Preconditions.checkState(!ntiWorkset.isEmpty()); + return new Iterable>() { + @Override + public Iterator> iterator() { + return new BackwardIterator(); + } + }; + } + + private void processGraphNode() { + DiGraphNode dn = workset.pop(); + if (seen.contains(dn) || dn == cfg.getImplicitReturn()) { + return; + } + switch (dn.getValue().getToken()) { + case DO: + case WHILE: + case FOR: + case FOR_IN: + case FOR_OF: { + List> outEdges = dn.getOutEdges(); + // The workset is a stack. If we want to analyze nodeA after nodeB, we need to push nodeA + // before nodeB. For this reason, we push the code after a loop before the loop body. + for (DiGraphEdge outEdge : outEdges) { + if (outEdge.getValue() == ControlFlowGraph.Branch.ON_FALSE) { + workset.push(outEdge.getDestination()); + } + } + for (DiGraphEdge outEdge : outEdges) { + if (outEdge.getValue() == ControlFlowGraph.Branch.ON_TRUE) { + workset.push(outEdge.getDestination()); + } + } + // The loop condition must be analyzed first, so it's pushed last. + seen.add(dn); + ntiWorkset.add(dn); + return; + } + default: { + for (DiGraphEdge inEdge : dn.getInEdges()) { + DiGraphNode source = inEdge.getSource(); + Node sourceNode = source.getValue(); + // Wait for all other incoming edges at join nodes. + if (!seen.contains(inEdge.getSource()) && !sourceNode.isDo()) { + return; + } + // The loop header has already been added, and will be analyzed before the loop body. + // Here, we want to add it again, so that we analyze the header after the loop body, + // and before the code following the loop. + if (NodeUtil.isLoopStructure(sourceNode) && !sourceNode.isDo() + && inEdge.getValue() == ControlFlowGraph.Branch.ON_FALSE) { + ntiWorkset.add(source); + } + } + seen.add(dn); + if (cfg.getEntry() != dn) { + ntiWorkset.add(dn); + } + Node n = dn.getValue(); + List> succs = cfg.getDirectedSuccNodes(dn); + // Currently, the ELSE branch of an IF is analyzed before the THEN branch. + // To do it the other way around, the ELSE branch has to be pushed to the workset + // *before* the THEN branch, so we need to reverse succs. But the order doesn't impact + // correctness, so we don't do the reversal. + for (DiGraphNode succ : succs) { + workset.push(succ); + if (succ == cfg.getImplicitReturn()) { + if (n.getNext() != null) { + processDeadNode(n.getNext()); + } + } + } + if (n.isTry()) { + processDeadNode(n.getSecondChild()); + } else if (n.isBreak() || n.isContinue() || n.isThrow()) { + processDeadNode(n.getNext()); + } + } + } + } + + /** + * Analyze dead code, such as a catch that is never executed or a statement following a + * return/break/continue. This code can be a predecessor of live code in the cfg. We wait + * on incoming edges before adding nodes to the workset, and don't want dead code to block + * live code from being analyzed. + */ + private void processDeadNode(Node maybeDeadNode) { + if (maybeDeadNode == null) { + return; + } + DiGraphNode cfgNode = cfg.getDirectedGraphNode(maybeDeadNode); + if (cfgNode == null) { + return; + } + if (cfg.getDirectedPredNodes(cfgNode).isEmpty()) { + workset.push(cfgNode); + } + } +} diff --git a/src/com/google/javascript/jscomp/NewTypeInference.java b/src/com/google/javascript/jscomp/NewTypeInference.java index 8ccd3f84d37..a8576c228a9 100644 --- a/src/com/google/javascript/jscomp/NewTypeInference.java +++ b/src/com/google/javascript/jscomp/NewTypeInference.java @@ -49,10 +49,8 @@ import com.google.javascript.rhino.TypeI; import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.LinkedHashMap; import java.util.LinkedHashSet; -import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Objects; @@ -747,115 +745,6 @@ private JSType getSummaryOfLocalFunDef(String name) { return changeTypeIfFunctionNamespace(fnScope, fnType); } - private void buildWorkset( - DiGraphNode dn, - List> workset) { - buildWorksetHelper(dn, workset, - new LinkedHashSet>()); - } - - private void buildWorksetHelper( - DiGraphNode dn, - List> workset, - Set> seen) { - if (seen.contains(dn) || dn == this.cfg.getImplicitReturn()) { - return; - } - switch (dn.getValue().getToken()) { - case DO: - case WHILE: - case FOR: - case FOR_IN: - case FOR_OF: - // Do the loop body first, then the loop follow. - // For DO loops, we do BODY-CONDT-CONDF-FOLLOW - // Since CONDT is currently unused, this could be optimized. - List> outEdges = dn.getOutEdges(); - seen.add(dn); - workset.add(dn); - for (DiGraphEdge outEdge : outEdges) { - if (outEdge.getValue() == ControlFlowGraph.Branch.ON_TRUE) { - buildWorksetHelper(outEdge.getDestination(), workset, seen); - } - } - workset.add(dn); - for (DiGraphEdge outEdge : outEdges) { - if (outEdge.getValue() == ControlFlowGraph.Branch.ON_FALSE) { - buildWorksetHelper(outEdge.getDestination(), workset, seen); - } - } - break; - default: { - // Wait for all other incoming edges at join nodes. - for (DiGraphEdge inEdge : - dn.getInEdges()) { - if (!seen.contains(inEdge.getSource()) - && !inEdge.getSource().getValue().isDo()) { - return; - } - } - seen.add(dn); - if (this.cfg.getEntry() != dn) { - workset.add(dn); - } - // Don't recur for straight-line code - while (true) { - Node n = dn.getValue(); - if (n.isTry()) { - maybeAddDeadCode(workset, seen, n.getSecondChild()); - } else if (n.isBreak() || n.isContinue() || n.isThrow()) { - maybeAddDeadCode(workset, seen, n.getNext()); - } - List> succs = - this.cfg.getDirectedSuccNodes(dn); - if (succs.size() != 1) { - break; - } - DiGraphNode succ = succs.get(0); - if (succ == this.cfg.getImplicitReturn()) { - if (n.getNext() != null) { - maybeAddDeadCode(workset, seen, n.getNext()); - } - return; - } - // Make sure that succ isn't a join node - if (this.cfg.getDirectedPredNodes(succ).size() > 1) { - break; - } - workset.add(succ); - seen.add(succ); - dn = succ; - } - for (DiGraphNode succ : - this.cfg.getDirectedSuccNodes(dn)) { - buildWorksetHelper(succ, workset, seen); - } - break; - } - } - } - - // Analyze dead code, such as a catch that is never executed or a statement - // following a return/break/continue. This code can be a predecessor of live - // code in the cfg. We wait on incoming edges before adding nodes to the - // workset, and don't want dead code to block live code from being analyzed. - private void maybeAddDeadCode( - List> workset, - Set> seen, - Node maybeDeadNode) { - if (maybeDeadNode == null) { - return; - } - DiGraphNode cfgNode = - this.cfg.getDirectedGraphNode(maybeDeadNode); - if (cfgNode == null) { - return; - } - if (this.cfg.getDirectedPredNodes(cfgNode).isEmpty()) { - buildWorksetHelper(cfgNode, workset, seen); - } - } - private void analyzeFunction(NTIScope scope) { println("=== Analyzing function: ", scope.getReadableName(), " ==="); currentScope = scope; @@ -865,13 +754,10 @@ private void analyzeFunction(NTIScope scope) { println(this.cfg); // The size is > 1 when multiple files are compiled // Preconditions.checkState(cfg.getEntry().getOutEdges().size() == 1); - List> workset = - new LinkedList<>(); - buildWorkset(this.cfg.getEntry(), workset); + NTIWorkset workset = new NTIWorkset(this.cfg); /* println("Workset: ", workset); */ this.typeEnvFromDeclaredTypes = getTypeEnvFromDeclaredTypes(); if (scope.isFunction() && scope.hasUndeclaredFormalsOrOuters()) { - Collections.reverse(workset); // Ideally, we would like to only set the in-edges of the implicit return // rather than all edges. However, we cannot do that because of a bug in // workset construction. (The test testBadWorksetConstruction would fail.) @@ -884,7 +770,6 @@ private void analyzeFunction(NTIScope scope) { envs.put(e, this.typeEnvFromDeclaredTypes); } analyzeFunctionBwd(workset); - Collections.reverse(workset); // TODO(dimvar): Revisit what we throw away after the bwd analysis TypeEnv entryEnv = getEntryTypeEnv(); initEdgeEnvsFwd(entryEnv); @@ -905,9 +790,8 @@ private void analyzeFunction(NTIScope scope) { } } - private void analyzeFunctionBwd( - List> workset) { - for (DiGraphNode dn : workset) { + private void analyzeFunctionBwd(NTIWorkset workset) { + for (DiGraphNode dn : workset.backward()) { Node n = dn.getValue(); TypeEnv outEnv = checkNotNull(getOutEnv(dn)); TypeEnv inEnv; @@ -999,9 +883,8 @@ private void analyzeFunctionBwd( } } - private void analyzeFunctionFwd( - List> workset) { - for (DiGraphNode dn : workset) { + private void analyzeFunctionFwd(NTIWorkset workset) { + for (DiGraphNode dn : workset.forward()) { Node n = dn.getValue(); Node parent = n.getParent(); checkState(n != null, "Implicit return should not be in workset."); @@ -4213,7 +4096,7 @@ static EnvTypePair join(EnvTypePair p1, EnvTypePair p2) { } private static JSType envGetType(TypeEnv env, String pname) { - checkArgument(!pname.contains(".")); + checkArgument(!pname.contains("."), pname); return env.getType(pname); } diff --git a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java index 7b65969dc6a..a949a087dee 100644 --- a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java +++ b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java @@ -13214,7 +13214,7 @@ public void testAutoconvertBoxedBooleanToBoolean() { typeCheck(LINE_JOINER.join( "function f(/** !Boolean */ x) {", - " if (x) { return 123; };", + " if (x) { return 123; }", "}")); } @@ -20792,4 +20792,33 @@ public void testTaggedTemplateBadTagFunction() { NewTypeInference.INVALID_OPERAND_TYPE ); } + + public void testReanalyzeLoopConditionAfterLoopBody() { + typeCheckMessageContents( + LINE_JOINER.join( + "var x = 123;", + "var y = 234;", + "for (; x = y;) {", + " y = 'asdf';", + "}", + "var z = x - 1;"), + NewTypeInference.INVALID_OPERAND_TYPE, + LINE_JOINER.join( + "Invalid type(s) for operator SUB.", + "Expected : number", + "Found : number|string", + "More details:", + "The found type is a union that includes an unexpected type: string")); + } + + public void testDoWhileDontCrash() { + typeCheck(LINE_JOINER.join( + "function f() {", + " var i = 0;", + " if (2 < 5) {", + " do {} while (i < 5);", + " }", + " return 3;", + "}")); + } }