diff --git a/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java b/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java index d2fd78e2421..39c86db3332 100644 --- a/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java +++ b/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java @@ -156,8 +156,7 @@ protected TernaryValue getSideEffectFreeBooleanValue(Node n) { * the current node's type is one of the reason's why a subtree has side effects. */ protected boolean nodeTypeMayHaveSideEffects(Node n) { - checkNotNull(compiler); - return NodeUtil.nodeTypeMayHaveSideEffects(n, compiler); + return astAnalyzer.nodeTypeMayHaveSideEffects(n); } /** diff --git a/src/com/google/javascript/jscomp/AstAnalyzer.java b/src/com/google/javascript/jscomp/AstAnalyzer.java index 80aacf8e5bc..ec38c3d1b5c 100644 --- a/src/com/google/javascript/jscomp/AstAnalyzer.java +++ b/src/com/google/javascript/jscomp/AstAnalyzer.java @@ -113,4 +113,14 @@ boolean constructorCallHasSideEffects(Node newNode) { Node nameNode = newNode.getFirstChild(); return !nameNode.isName() || !CONSTRUCTORS_WITHOUT_SIDE_EFFECTS.contains(nameNode.getString()); } + + /** + * Returns true if the current node's type implies side effects. + * + *

This is a non-recursive version of the may have side effects check; used to check wherever + * the current node's type is one of the reasons why a subtree has side effects. + */ + boolean nodeTypeMayHaveSideEffects(Node n) { + return NodeUtil.nodeTypeMayHaveSideEffects(n, compiler); + } } diff --git a/src/com/google/javascript/jscomp/NodeIterators.java b/src/com/google/javascript/jscomp/NodeIterators.java index 4e29141269f..3291538903a 100644 --- a/src/com/google/javascript/jscomp/NodeIterators.java +++ b/src/com/google/javascript/jscomp/NodeIterators.java @@ -279,7 +279,7 @@ private void advanceLookAhead(boolean atStart) { // var a = b; // var b = 3; // alert(a); - if ((NodeUtil.nodeTypeMayHaveSideEffects(nextNode, compiler) && type != Token.NAME) + if ((compiler.getAstAnalyzer().nodeTypeMayHaveSideEffects(nextNode) && type != Token.NAME) || (type == Token.NAME && nextParent.isCatch())) { lookAhead = null; return; diff --git a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java index 413328b209e..fc05ad1588b 100644 --- a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java +++ b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java @@ -664,7 +664,7 @@ public boolean shouldTraverse(NodeTraversal traversal, Node node, Node parent) { @Override public void visit(NodeTraversal traversal, Node node, Node parent) { - if (!NodeUtil.nodeTypeMayHaveSideEffects(node, compiler) && !node.isReturn()) { + if (!compiler.getAstAnalyzer().nodeTypeMayHaveSideEffects(node) && !node.isReturn()) { return; } diff --git a/test/com/google/javascript/jscomp/AstAnalyzerTest.java b/test/com/google/javascript/jscomp/AstAnalyzerTest.java index 4b123d11bc4..96cbfe00680 100644 --- a/test/com/google/javascript/jscomp/AstAnalyzerTest.java +++ b/test/com/google/javascript/jscomp/AstAnalyzerTest.java @@ -19,15 +19,30 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.truth.Truth.assertThat; import static com.google.javascript.jscomp.DiagnosticGroups.ES5_STRICT; +import static com.google.javascript.rhino.Token.ADD; +import static com.google.javascript.rhino.Token.ASSIGN; +import static com.google.javascript.rhino.Token.ASSIGN_ADD; import static com.google.javascript.rhino.Token.AWAIT; +import static com.google.javascript.rhino.Token.BITOR; import static com.google.javascript.rhino.Token.CALL; +import static com.google.javascript.rhino.Token.CLASS; import static com.google.javascript.rhino.Token.COMPUTED_PROP; +import static com.google.javascript.rhino.Token.DEC; +import static com.google.javascript.rhino.Token.DELPROP; import static com.google.javascript.rhino.Token.FOR_AWAIT_OF; +import static com.google.javascript.rhino.Token.FOR_IN; +import static com.google.javascript.rhino.Token.FOR_OF; +import static com.google.javascript.rhino.Token.FUNCTION; import static com.google.javascript.rhino.Token.GETTER_DEF; +import static com.google.javascript.rhino.Token.INC; import static com.google.javascript.rhino.Token.MEMBER_FUNCTION_DEF; +import static com.google.javascript.rhino.Token.NAME; import static com.google.javascript.rhino.Token.NEW; +import static com.google.javascript.rhino.Token.OR; import static com.google.javascript.rhino.Token.REST; import static com.google.javascript.rhino.Token.SETTER_DEF; +import static com.google.javascript.rhino.Token.SPREAD; +import static com.google.javascript.rhino.Token.TAGGED_TEMPLATELIT; import static com.google.javascript.rhino.Token.THROW; import static com.google.javascript.rhino.Token.YIELD; @@ -534,6 +549,81 @@ public void testMayHaveSideEffects_regExp() { } } + @RunWith(Parameterized.class) + public static final class NodeTypeMayHaveSideEffects { + @Parameter(0) + public String js; + + @Parameter(1) + public Token token; + + @Parameter(2) + public Boolean expectedResult; + + @Parameters(name = "{1} node in \"{0}\" side-effects: {2}") + public static Iterable cases() { + return ImmutableList.copyOf( + new Object[][] { + {"x = y", ASSIGN, true}, + {"x += y", ASSIGN_ADD, true}, + {"delete x.y", DELPROP, true}, + {"x++", INC, true}, + {"x--", DEC, true}, + {"for (prop in obj) {}", FOR_IN, true}, + {"for (item of iterable) {}", FOR_OF, true}, + // name declaration has a side effect when a value is actually assigned + {"var x = 1;", NAME, true}, + {"let x = 1;", NAME, true}, + {"const x = 1;", NAME, true}, + // don't consider name declaration to have a side effect if there's no assignment + {"var x;", NAME, false}, + {"let x;", NAME, false}, + + // NOTE: CALL and NEW nodes are delegated to functionCallHasSideEffects() and + // constructorCallHasSideEffects(), respectively. The cases below are just a few + // representative examples that are convenient to test here. + // + // in general function and constructor calls are assumed to have side effects + {"foo();", CALL, true}, + {"new Foo();", NEW, true}, + // Object() is known not to have side-effects, though + {"Object();", CALL, false}, + {"new Object();", NEW, false}, + // TAGGED_TEMPLATELIT is just a special syntax for a CALL. + {"foo`template`;", TAGGED_TEMPLATELIT, true}, + + // NOTE: REST and SPREAD are delegated to NodeUtil.iteratesImpureIterable() + // Test cases here are just easy to test representative examples. + {"[...[1, 2, 3]]", SPREAD, false}, + {"[...someIterable]", SPREAD, true}, // unknown, so assume side-effects + // we just assume the rhs of an array pattern may have iteration side-effects + // without looking too closely. + {"let [...rest] = [1, 2, 3];", REST, true}, + // REST in parameter list does not trigger iteration at the function definition, so + // it has no side effects. + {"function foo(...rest) {}", REST, false}, + + // defining a class or a function is not considered to be a side-effect + {"function foo() {}", FUNCTION, false}, + {"class Foo {}", CLASS, false}, + + // arithmetic, logic, and bitwise operations do not have side-effects + {"x + y", ADD, false}, + {"x || y", OR, false}, + {"x | y", BITOR, false}, + }); + } + + @Test + public void test() { + ParseHelper helper = new ParseHelper(); + + Node n = helper.parseFirst(token, js); + AstAnalyzer astAnalyzer = helper.getAstAnalyzer(); + assertThat(astAnalyzer.nodeTypeMayHaveSideEffects(n)).isEqualTo(expectedResult); + } + } + @RunWith(JUnit4.class) public static final class FunctionCallHasSideEffects { @Test