From 066066d4c51354204dfd2bf0d07cca7eadea4ecd Mon Sep 17 00:00:00 2001 From: bradfordcsmith Date: Wed, 29 May 2019 09:45:58 -0700 Subject: [PATCH] Move side-effect logic from NodeUtil to AstAnalyzer This completes the move of the primary side-effect logic methods from NodeUtil to AstAnalyzer. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250509319 --- .../google/javascript/jscomp/AstAnalyzer.java | 367 ++++++++++++++- .../google/javascript/jscomp/NodeUtil.java | 424 ------------------ .../javascript/jscomp/NodeUtilTest.java | 390 ---------------- 3 files changed, 361 insertions(+), 820 deletions(-) diff --git a/src/com/google/javascript/jscomp/AstAnalyzer.java b/src/com/google/javascript/jscomp/AstAnalyzer.java index ec38c3d1b5c..988921779f7 100644 --- a/src/com/google/javascript/jscomp/AstAnalyzer.java +++ b/src/com/google/javascript/jscomp/AstAnalyzer.java @@ -18,9 +18,12 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; import com.google.common.collect.ImmutableSet; import com.google.javascript.rhino.Node; +import com.google.javascript.rhino.jstype.JSType; +import com.google.javascript.rhino.jstype.JSTypeNative; /** * Logic for answering questions about portions of the AST. @@ -52,6 +55,17 @@ public class AstAnalyzer { private static final ImmutableSet CONSTRUCTORS_WITHOUT_SIDE_EFFECTS = ImmutableSet.of("Array", "Date", "Error", "Object", "RegExp", "XMLHttpRequest"); + // A list of built-in object creation or primitive type cast functions that + // can also be called as constructors but lack side-effects. + // TODO(johnlenz): consider adding an extern annotation for this. + private static final ImmutableSet BUILTIN_FUNCTIONS_WITHOUT_SIDEEFFECTS = + ImmutableSet.of("Object", "Array", "String", "Number", "Boolean", "RegExp", "Error"); + private static final ImmutableSet OBJECT_METHODS_WITHOUT_SIDEEFFECTS = + ImmutableSet.of("toString", "valueOf"); + private static final ImmutableSet REGEXP_METHODS = ImmutableSet.of("test", "exec"); + private static final ImmutableSet STRING_REGEXP_METHODS = + ImmutableSet.of("match", "replace", "search", "split"); + private final AbstractCompiler compiler; AstAnalyzer(AbstractCompiler compiler) { @@ -64,8 +78,7 @@ public class AstAnalyzer { * @see XKCD Cartoon */ boolean mayEffectMutableState(Node n) { - // TODO(bradfordcsmith): Move the implementation into this class when possible. - return NodeUtil.mayEffectMutableState(n, compiler); + return checkForStateChangeHelper(n, /* checkForNewObjects= */ true); } /** @@ -73,8 +86,7 @@ boolean mayEffectMutableState(Node n) { * "safe" assumptions when the compiler object is not provided (RegExp have side-effects, etc). */ public boolean mayHaveSideEffects(Node n) { - // TODO(b/131178806): Move implementation here when possible - return NodeUtil.mayHaveSideEffects(n, compiler); + return checkForStateChangeHelper(n, /* checkForNewObjects= */ false); } /** @@ -86,7 +98,318 @@ public boolean mayHaveSideEffects(Node n) { * @param callNode - function call node */ boolean functionCallHasSideEffects(Node callNode) { - return NodeUtil.functionCallHasSideEffects(callNode, compiler); + checkState(callNode.isCall() || callNode.isTaggedTemplateLit(), callNode); + + if (callNode.isNoSideEffectsCall()) { + return false; + } + + if (callNode.isOnlyModifiesArgumentsCall() && NodeUtil.allArgsUnescapedLocal(callNode)) { + return false; + } + + Node callee = callNode.getFirstChild(); + + // Built-in functions with no side effects. + if (callee.isName()) { + String name = callee.getString(); + if (BUILTIN_FUNCTIONS_WITHOUT_SIDEEFFECTS.contains(name)) { + return false; + } + } else if (callee.isGetProp()) { + if (callNode.hasOneChild() + && OBJECT_METHODS_WITHOUT_SIDEEFFECTS.contains(callee.getLastChild().getString())) { + return false; + } + + if (callNode.isOnlyModifiesThisCall() + && NodeUtil.evaluatesToLocalValue(callee.getFirstChild())) { + return false; + } + + // Many common Math functions have no side-effects. + // TODO(nicksantos): This is a terrible terrible hack, until + // I create a definitionProvider that understands namespacing. + if (callee.getFirstChild().isName() + && callee.isQualifiedName() + && callee.getFirstChild().getString().equals("Math")) { + switch (callee.getLastChild().getString()) { + case "abs": + case "acos": + case "acosh": + case "asin": + case "asinh": + case "atan": + case "atanh": + case "atan2": + case "cbrt": + case "ceil": + case "cos": + case "cosh": + case "exp": + case "expm1": + case "floor": + case "hypot": + case "log": + case "log10": + case "log1p": + case "log2": + case "max": + case "min": + case "pow": + case "round": + case "sign": + case "sin": + case "sinh": + case "sqrt": + case "tan": + case "tanh": + case "trunc": + return false; + case "random": + return !callNode.hasOneChild(); // no parameters + default: + // Unknown Math.* function, so fall out of this switch statement. + } + } + + if (!compiler.hasRegExpGlobalReferences()) { + if (callee.getFirstChild().isRegExp() + && REGEXP_METHODS.contains(callee.getLastChild().getString())) { + return false; + } else if (isTypedAsString(callee.getFirstChild())) { + // Unlike regexs, string methods don't need to be hosted on a string literal + // to avoid leaking mutating global state changes, it is just necessary that + // the regex object can't be referenced. + String method = callee.getLastChild().getString(); + Node param = callee.getNext(); + if (param != null) { + if (param.isString()) { + if (STRING_REGEXP_METHODS.contains(method)) { + return false; + } + } else if (param.isRegExp()) { + if ("replace".equals(method)) { + // Assume anything but a string constant has side-effects + return !param.getNext().isString(); + } else if (STRING_REGEXP_METHODS.contains(method)) { + return false; + } + } + } + } + } + } + + return true; + } + + private boolean isTypedAsString(Node n) { + if (n.isString()) { + return true; + } + + if (compiler.getOptions().useTypesForLocalOptimization) { + JSType type = n.getJSType(); + if (type != null) { + JSType nativeStringType = + compiler.getTypeRegistry().getNativeType(JSTypeNative.STRING_TYPE); + if (type.isEquivalentTo(nativeStringType)) { + return true; + } + } + } + + return false; + } + + /** + * Returns true if some node in n's subtree changes application state. If {@code + * checkForNewObjects} is true, we assume that newly created mutable objects (like object + * literals) change state. Otherwise, we assume that they have no side effects. + */ + private boolean checkForStateChangeHelper(Node n, boolean checkForNewObjects) { + // Rather than id which ops may have side effects, id the ones + // that we know to be safe + switch (n.getToken()) { + case THROW: + // Throw is a side-effect by definition. + case YIELD: + case AWAIT: + case FOR_AWAIT_OF: + // Context switches can conceal side-effects. + case FOR_OF: + case FOR_IN: + // Enhanced for loops are almost always side-effectful; it's not worth checking them + // further. Particularly, they represent a kind of assignment op. + case VAR: + case LET: + case CONST: + case EXPORT: + // Variable declarations are side-effects. + return true; + + case OBJECTLIT: + case ARRAYLIT: + case REGEXP: + if (checkForNewObjects) { + return true; + } + break; + + case REST: + case SPREAD: + if (NodeUtil.iteratesImpureIterable(n)) { + return true; + } + break; + + case NAME: + // TODO(b/129564961): Consider EXPORT declarations. + if (n.hasChildren()) { + // This is the left side of a var/let/const + return true; + } + break; + + case FUNCTION: + // Function expressions don't have side-effects, but function + // declarations change the namespace. Either way, we don't need to + // check the children, since they aren't executed at declaration time. + return checkForNewObjects || NodeUtil.isFunctionDeclaration(n); + + case GETTER_DEF: + case SETTER_DEF: + case MEMBER_FUNCTION_DEF: + // simply defining a member function, getter, or setter has no side effects + return false; + + case CLASS: + return checkForNewObjects + || NodeUtil.isClassDeclaration(n) + // Check the extends clause for side effects. + || checkForStateChangeHelper(n.getSecondChild(), checkForNewObjects) + // Check for class members that are computed properties with side effects. + || checkForStateChangeHelper(n.getLastChild(), checkForNewObjects); + + case CLASS_MEMBERS: + for (Node member = n.getFirstChild(); member != null; member = member.getNext()) { + if (member.isComputedProp() + && checkForStateChangeHelper(member.getFirstChild(), checkForNewObjects)) { + return true; + } + } + return false; + + case NEW: + if (checkForNewObjects) { + return true; + } + + if (!constructorCallHasSideEffects(n)) { + // loop below will see if the constructor parameters have + // side-effects + break; + } + return true; + + case CALL: + // calls to functions that have no side effects have the no + // side effect property set. + if (!functionCallHasSideEffects(n)) { + // loop below will see if the function parameters have + // side-effects + break; + } + return true; + + case TAGGED_TEMPLATELIT: + // TODO(b/128527671): Inspect the children of the expression for side-effects. + return functionCallHasSideEffects(n); + + case CAST: + case AND: + case BLOCK: + case ROOT: + case EXPR_RESULT: + case HOOK: + case IF: + case PARAM_LIST: + case NUMBER: + case OR: + case THIS: + case TRUE: + case FALSE: + case NULL: + case STRING: + case STRING_KEY: + case SWITCH: + case TEMPLATELIT_SUB: + case TRY: + case EMPTY: + case TEMPLATELIT: + case TEMPLATELIT_STRING: + case COMPUTED_PROP: + break; + + default: + if (NodeUtil.isSimpleOperator(n)) { + break; + } + + if (NodeUtil.isAssignmentOp(n)) { + Node assignTarget = n.getFirstChild(); + if (assignTarget.isName()) { + return true; + } + + // Assignments will have side effects if + // a) The RHS has side effects, or + // b) The LHS has side effects, or + // c) A name on the LHS will exist beyond the life of this statement. + if (checkForStateChangeHelper(n.getFirstChild(), checkForNewObjects) + || checkForStateChangeHelper(n.getLastChild(), checkForNewObjects)) { + return true; + } + + if (NodeUtil.isGet(assignTarget)) { + // If the object being assigned to is a local object, don't + // consider this a side-effect as it can't be referenced + // elsewhere. Don't do this recursively as the property might + // be an alias of another object, unlike a literal below. + Node current = assignTarget.getFirstChild(); + if (NodeUtil.evaluatesToLocalValue(current)) { + return false; + } + + // A literal value as defined by "isLiteralValue" is guaranteed + // not to be an alias, or any components which are aliases of + // other objects. + // If the root object is a literal don't consider this a + // side-effect. + while (NodeUtil.isGet(current)) { + current = current.getFirstChild(); + } + + return !NodeUtil.isLiteralValue(current, true); + } else { + // TODO(johnlenz): remove this code and make this an exception. This + // is here only for legacy reasons, the AST is not valid but + // preserve existing behavior. + return !NodeUtil.isLiteralValue(assignTarget, true); + } + } + + return true; + } + + for (Node c = n.getFirstChild(); c != null; c = c.getNext()) { + if (checkForStateChangeHelper(c, checkForNewObjects)) { + return true; + } + } + + return false; } /** @@ -121,6 +444,38 @@ boolean constructorCallHasSideEffects(Node newNode) { * 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); + checkNotNull(compiler); + if (NodeUtil.isAssignmentOp(n)) { + return true; + } + + switch (n.getToken()) { + case DELPROP: + case DEC: + case INC: + case YIELD: + case THROW: + case AWAIT: + case FOR_IN: // assigns to a loop LHS + case FOR_OF: // assigns to a loop LHS, runs an iterator + case FOR_AWAIT_OF: // assigns to a loop LHS, runs an iterator, async operations. + return true; + case CALL: + case TAGGED_TEMPLATELIT: + return functionCallHasSideEffects(n); + case NEW: + return constructorCallHasSideEffects(n); + case NAME: + // A variable definition. + // TODO(b/129564961): Consider EXPORT declarations. + return n.hasChildren(); + case REST: + case SPREAD: + return NodeUtil.iteratesImpureIterable(n); + default: + break; + } + + return false; } } diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index 06a2144aaa3..8f72a9c8ccf 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -43,7 +43,6 @@ import com.google.javascript.rhino.TokenUtil; import com.google.javascript.rhino.dtoa.DToA; import com.google.javascript.rhino.jstype.JSType; -import com.google.javascript.rhino.jstype.JSTypeNative; import com.google.javascript.rhino.jstype.TernaryValue; import java.util.ArrayList; import java.util.Collection; @@ -70,10 +69,6 @@ public final class NodeUtil { static final char LARGEST_BASIC_LATIN = 0x7f; - /** the set of builtin constructors that don't have side effects. */ - private static final ImmutableSet CONSTRUCTORS_WITHOUT_SIDE_EFFECTS = - ImmutableSet.of("Array", "Date", "Error", "Object", "RegExp", "XMLHttpRequest"); - private static final Node googModuleDeclareLegacyNamespace = IR.getprop(IR.getprop(IR.name("goog"), "module"), "declareLegacyNamespace"); @@ -1028,217 +1023,6 @@ static Node newExpr(Node child) { return IR.exprResult(child).srcref(child); } - /** - * Returns true if the node may create new mutable state, or change existing state. - * - * @see XKCD Cartoon - * @deprecated use {@link AstAnalyzer#mayEffectMutableState(Node)}. - */ - @Deprecated - static boolean mayEffectMutableState(Node n, AbstractCompiler compiler) { - checkNotNull(compiler); - return checkForStateChangeHelper(n, true, compiler); - } - - /** - * Returns true if the node which may have side effects when executed. - * This version default to the "safe" assumptions when the compiler object is not - * provided (RegExp have side-effects, etc). - */ - public static boolean mayHaveSideEffects(Node n, AbstractCompiler compiler) { - return checkForStateChangeHelper(n, false, compiler); - } - - /** - * Returns true if some node in n's subtree changes application state. - * If {@code checkForNewObjects} is true, we assume that newly created - * mutable objects (like object literals) change state. Otherwise, we assume - * that they have no side effects. - */ - private static boolean checkForStateChangeHelper( - Node n, boolean checkForNewObjects, AbstractCompiler compiler) { - // Rather than id which ops may have side effects, id the ones - // that we know to be safe - switch (n.getToken()) { - case THROW: - // Throw is a side-effect by definition. - case YIELD: - case AWAIT: - case FOR_AWAIT_OF: - // Context switches can conceal side-effects. - case FOR_OF: - case FOR_IN: - // Enhanced for loops are almost always side-effectful; it's not worth checking them - // further. Particularly, they represent a kind of assignment op. - case VAR: - case LET: - case CONST: - case EXPORT: - // Variable declarations are side-effects. - return true; - - case OBJECTLIT: - case ARRAYLIT: - case REGEXP: - if (checkForNewObjects) { - return true; - } - break; - - case REST: - case SPREAD: - if (iteratesImpureIterable(n)) { - return true; - } - break; - - case NAME: - // TODO(b/129564961): Consider EXPORT declarations. - if (n.hasChildren()) { - // This is the left side of a var/let/const - return true; - } - break; - - case FUNCTION: - // Function expressions don't have side-effects, but function - // declarations change the namespace. Either way, we don't need to - // check the children, since they aren't executed at declaration time. - return checkForNewObjects || isFunctionDeclaration(n); - - case GETTER_DEF: - case SETTER_DEF: - case MEMBER_FUNCTION_DEF: - // simply defining a member function, getter, or setter has no side effects - return false; - - case CLASS: - return checkForNewObjects || isClassDeclaration(n) - // Check the extends clause for side effects. - || checkForStateChangeHelper(n.getSecondChild(), checkForNewObjects, compiler) - // Check for class members that are computed properties with side effects. - || checkForStateChangeHelper(n.getLastChild(), checkForNewObjects, compiler); - - case CLASS_MEMBERS: - for (Node member = n.getFirstChild(); member != null; member = member.getNext()) { - if (member.isComputedProp() - && checkForStateChangeHelper(member.getFirstChild(), checkForNewObjects, compiler)) { - return true; - } - } - return false; - - case NEW: - if (checkForNewObjects) { - return true; - } - - if (!constructorCallHasSideEffects(n)) { - // loop below will see if the constructor parameters have - // side-effects - break; - } - return true; - - case CALL: - // calls to functions that have no side effects have the no - // side effect property set. - if (!functionCallHasSideEffects(n, compiler)) { - // loop below will see if the function parameters have - // side-effects - break; - } - return true; - - case TAGGED_TEMPLATELIT: - // TODO(b/128527671): Inspect the children of the expression for side-effects. - return functionCallHasSideEffects(n, compiler); - - case CAST: - case AND: - case BLOCK: - case ROOT: - case EXPR_RESULT: - case HOOK: - case IF: - case PARAM_LIST: - case NUMBER: - case OR: - case THIS: - case TRUE: - case FALSE: - case NULL: - case STRING: - case STRING_KEY: - case SWITCH: - case TEMPLATELIT_SUB: - case TRY: - case EMPTY: - case TEMPLATELIT: - case TEMPLATELIT_STRING: - case COMPUTED_PROP: - break; - - default: - if (isSimpleOperator(n)) { - break; - } - - if (isAssignmentOp(n)) { - Node assignTarget = n.getFirstChild(); - if (assignTarget.isName()) { - return true; - } - - // Assignments will have side effects if - // a) The RHS has side effects, or - // b) The LHS has side effects, or - // c) A name on the LHS will exist beyond the life of this statement. - if (checkForStateChangeHelper(n.getFirstChild(), checkForNewObjects, compiler) - || checkForStateChangeHelper(n.getLastChild(), checkForNewObjects, compiler)) { - return true; - } - - if (isGet(assignTarget)) { - // If the object being assigned to is a local object, don't - // consider this a side-effect as it can't be referenced - // elsewhere. Don't do this recursively as the property might - // be an alias of another object, unlike a literal below. - Node current = assignTarget.getFirstChild(); - if (evaluatesToLocalValue(current)) { - return false; - } - - // A literal value as defined by "isLiteralValue" is guaranteed - // not to be an alias, or any components which are aliases of - // other objects. - // If the root object is a literal don't consider this a - // side-effect. - while (isGet(current)) { - current = current.getFirstChild(); - } - - return !isLiteralValue(current, true); - } else { - // TODO(johnlenz): remove this code and make this an exception. This - // is here only for legacy reasons, the AST is not valid but - // preserve existing behavior. - return !isLiteralValue(assignTarget, true); - } - } - - return true; - } - - for (Node c = n.getFirstChild(); c != null; c = c.getNext()) { - if (checkForStateChangeHelper(c, checkForNewObjects, compiler)) { - return true; - } - } - - return false; - } - /** * Returns {@code true} if {@code node} might execute an `Iterable` iteration that has * side-effects, {@code false} if there are definitely no such side-effects. @@ -1315,171 +1099,6 @@ private static boolean isPureIterable(Node node) { } } - /** - * Do calls to this constructor have side effects? - * - * @param callNode - constructor call node - */ - static boolean constructorCallHasSideEffects(Node callNode) { - checkArgument(callNode.isNew(), "Expected NEW node, got %s", callNode.getToken()); - - if (callNode.isNoSideEffectsCall()) { - return false; - } - - if (callNode.isOnlyModifiesArgumentsCall() && allArgsUnescapedLocal(callNode)) { - return false; - } - - Node nameNode = callNode.getFirstChild(); - return !nameNode.isName() || !CONSTRUCTORS_WITHOUT_SIDE_EFFECTS.contains(nameNode.getString()); - } - - // A list of built-in object creation or primitive type cast functions that - // can also be called as constructors but lack side-effects. - // TODO(johnlenz): consider adding an extern annotation for this. - private static final ImmutableSet BUILTIN_FUNCTIONS_WITHOUT_SIDEEFFECTS = - ImmutableSet.of("Object", "Array", "String", "Number", "Boolean", "RegExp", "Error"); - private static final ImmutableSet OBJECT_METHODS_WITHOUT_SIDEEFFECTS = - ImmutableSet.of("toString", "valueOf"); - private static final ImmutableSet REGEXP_METHODS = ImmutableSet.of("test", "exec"); - private static final ImmutableSet STRING_REGEXP_METHODS = - ImmutableSet.of("match", "replace", "search", "split"); - - /** - * Returns true if calls to this function have side effects. - * - * @param callNode The call node to inspected. - * @param compiler A compiler object to provide program state changing - * context information. Can be null. - */ - static boolean functionCallHasSideEffects( - Node callNode, @Nullable AbstractCompiler compiler) { - checkState(callNode.isCall() || callNode.isTaggedTemplateLit(), callNode); - - if (callNode.isNoSideEffectsCall()) { - return false; - } - - if (callNode.isOnlyModifiesArgumentsCall() - && allArgsUnescapedLocal(callNode)) { - return false; - } - - Node callee = callNode.getFirstChild(); - - // Built-in functions with no side effects. - if (callee.isName()) { - String name = callee.getString(); - if (BUILTIN_FUNCTIONS_WITHOUT_SIDEEFFECTS.contains(name)) { - return false; - } - } else if (callee.isGetProp()) { - if (callNode.hasOneChild() - && OBJECT_METHODS_WITHOUT_SIDEEFFECTS.contains(callee.getLastChild().getString())) { - return false; - } - - if (callNode.isOnlyModifiesThisCall() - && evaluatesToLocalValue(callee.getFirstChild())) { - return false; - } - - // Many common Math functions have no side-effects. - // TODO(nicksantos): This is a terrible terrible hack, until - // I create a definitionProvider that understands namespacing. - if (callee.getFirstChild().isName() && callee.isQualifiedName() - && callee.getFirstChild().getString().equals("Math")) { - switch(callee.getLastChild().getString()) { - case "abs": - case "acos": - case "acosh": - case "asin": - case "asinh": - case "atan": - case "atanh": - case "atan2": - case "cbrt": - case "ceil": - case "cos": - case "cosh": - case "exp": - case "expm1": - case "floor": - case "hypot": - case "log": - case "log10": - case "log1p": - case "log2": - case "max": - case "min": - case "pow": - case "round": - case "sign": - case "sin": - case "sinh": - case "sqrt": - case "tan": - case "tanh": - case "trunc": - return false; - case "random": - return !callNode.hasOneChild(); // no parameters - default: - // Unknown Math.* function, so fall out of this switch statement. - } - } - - if (compiler != null && !compiler.hasRegExpGlobalReferences()) { - if (callee.getFirstChild().isRegExp() - && REGEXP_METHODS.contains(callee.getLastChild().getString())) { - return false; - } else if (isTypedAsString(callee.getFirstChild(), compiler)) { - // Unlike regexs, string methods don't need to be hosted on a string literal - // to avoid leaking mutating global state changes, it is just necessary that - // the regex object can't be referenced. - String method = callee.getLastChild().getString(); - Node param = callee.getNext(); - if (param != null) { - if (param.isString()) { - if (STRING_REGEXP_METHODS.contains(method)) { - return false; - } - } else if (param.isRegExp()) { - if ("replace".equals(method)) { - // Assume anything but a string constant has side-effects - return !param.getNext().isString(); - } else if (STRING_REGEXP_METHODS.contains(method)) { - return false; - } - } - } - } - } - } - - return true; - } - - private static boolean isTypedAsString(Node n, AbstractCompiler compiler) { - if (n.isString()) { - return true; - } - - if (compiler.getOptions().useTypesForLocalOptimization) { - JSType type = n.getJSType(); - if (type != null) { - JSType nativeStringType = compiler.getTypeRegistry() - .getNativeType(JSTypeNative.STRING_TYPE); - if (type.isEquivalentTo(nativeStringType)) { - return true; - } - } - } - - return false; - } - /** * @return Whether the call has a local result. */ @@ -1496,49 +1115,6 @@ static boolean newHasLocalResult(Node n) { return n.isOnlyModifiesThisCall(); } - /** - * 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. - */ - static boolean nodeTypeMayHaveSideEffects(Node n, AbstractCompiler compiler) { - checkNotNull(compiler); - if (isAssignmentOp(n)) { - return true; - } - - switch (n.getToken()) { - case DELPROP: - case DEC: - case INC: - case YIELD: - case THROW: - case AWAIT: - case FOR_IN: // assigns to a loop LHS - case FOR_OF: // assigns to a loop LHS, runs an iterator - case FOR_AWAIT_OF: // assigns to a loop LHS, runs an iterator, async operations. - return true; - case CALL: - case TAGGED_TEMPLATELIT: - return NodeUtil.functionCallHasSideEffects(n, compiler); - case NEW: - return NodeUtil.constructorCallHasSideEffects(n); - case NAME: - // A variable definition. - // TODO(b/129564961): Consider EXPORT declarations. - return n.hasChildren(); - case REST: - case SPREAD: - return NodeUtil.iteratesImpureIterable(n); - default: - break; - } - - return false; - } - static boolean allArgsUnescapedLocal(Node callOrNew) { for (Node arg = callOrNew.getSecondChild(); arg != null; arg = arg.getNext()) { if (!evaluatesToLocalValue(arg)) { diff --git a/test/com/google/javascript/jscomp/NodeUtilTest.java b/test/com/google/javascript/jscomp/NodeUtilTest.java index 01f0c3409c3..9b96040bc3c 100644 --- a/test/com/google/javascript/jscomp/NodeUtilTest.java +++ b/test/com/google/javascript/jscomp/NodeUtilTest.java @@ -25,7 +25,6 @@ import static com.google.javascript.rhino.Token.AWAIT; 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.DESTRUCTURING_LHS; import static com.google.javascript.rhino.Token.FOR_AWAIT_OF; import static com.google.javascript.rhino.Token.FOR_OF; @@ -36,7 +35,6 @@ import static com.google.javascript.rhino.Token.SCRIPT; import static com.google.javascript.rhino.Token.SETTER_DEF; import static com.google.javascript.rhino.Token.SPREAD; -import static com.google.javascript.rhino.Token.THROW; import static com.google.javascript.rhino.Token.YIELD; import static com.google.javascript.rhino.testing.NodeSubject.assertNode; import static java.lang.Boolean.FALSE; @@ -654,324 +652,6 @@ public void testIsClassDeclaration() { .isFalse(); } - private void assertSideEffect(boolean se, String js) { - ParseHelper helper = new ParseHelper(); - - Node n = helper.parse(js); - assertThat(NodeUtil.mayHaveSideEffects(n.getFirstChild(), helper.getCompiler())) - .isEqualTo(se); - } - - private void assertNodeHasSideEffect(boolean se, Token token, String js) { - ParseHelper helper = new ParseHelper(); - - Node node = helper.parseFirst(token, js); - assertThat(NodeUtil.mayHaveSideEffects(node, helper.getCompiler())).isEqualTo(se); - } - - private void assertSideEffect(boolean se, String js, boolean globalRegExp) { - Node n = parse(js); - Compiler compiler = new Compiler(); - compiler.initCompilerOptionsIfTesting(); - compiler.setHasRegExpGlobalReferences(globalRegExp); - assertThat(NodeUtil.mayHaveSideEffects(n.getFirstChild(), compiler)).isEqualTo(se); - } - - @Test - public void testMayHaveSideEffects_undifferentiatedCases() { - assertSideEffect(false, "[1]"); - assertSideEffect(false, "[1, 2]"); - assertSideEffect(true, "i++"); - assertSideEffect(true, "[b, [a, i++]]"); - assertSideEffect(true, "i=3"); - assertSideEffect(true, "[0, i=3]"); - assertSideEffect(true, "b()"); - assertSideEffect(true, "[1, b()]"); - assertSideEffect(true, "b.b=4"); - assertSideEffect(true, "b.b--"); - assertSideEffect(true, "i--"); - assertSideEffect(true, "a[0][i=4]"); - assertSideEffect(true, "a += 3"); - assertSideEffect(true, "a, b, z += 4"); - assertSideEffect(true, "a ? c : d++"); - assertSideEffect(true, "a + c++"); - assertSideEffect(true, "a + c - d()"); - assertSideEffect(true, "a + c - d()"); - - assertSideEffect(true, "function foo() {}"); - assertSideEffect(true, "class Foo {}"); - assertSideEffect(true, "while(true);"); - assertSideEffect(true, "if(true){a()}"); - - assertSideEffect(false, "if(true){a}"); - assertSideEffect(false, "(function() { })"); - assertSideEffect(false, "(function() { i++ })"); - assertSideEffect(false, "[function a(){}]"); - assertSideEffect(false, "(class { })"); - assertSideEffect(false, "(class { method() { i++ } })"); - assertSideEffect(true, "(class { [computedName()]() {} })"); - assertSideEffect(false, "(class { [computedName]() {} })"); - assertSideEffect(false, "(class Foo extends Bar { })"); - assertSideEffect(true, "(class extends foo() { })"); - - assertSideEffect(false, "a"); - assertSideEffect(false, "a.b"); - assertSideEffect(false, "a.b.c"); - assertSideEffect(false, "[b, c [d, [e]]]"); - assertSideEffect(false, "({a: x, b: y, c: z})"); - assertSideEffect(false, "({a, b, c})"); - assertSideEffect(false, "/abc/gi"); - assertSideEffect(false, "'a'"); - assertSideEffect(false, "0"); - assertSideEffect(false, "a + c"); - assertSideEffect(false, "'c' + a[0]"); - assertSideEffect(false, "a[0][1]"); - assertSideEffect(false, "'a' + c"); - assertSideEffect(false, "'a' + a.name"); - assertSideEffect(false, "1, 2, 3"); - assertSideEffect(false, "a, b, 3"); - assertSideEffect(false, "(function(a, b) { })"); - assertSideEffect(false, "a ? c : d"); - assertSideEffect(false, "'1' + navigator.userAgent"); - - assertSideEffect(false, "`template`"); - assertSideEffect(false, "`template${name}`"); - assertSideEffect(false, "`${name}template`"); - assertSideEffect(true, "`${naming()}template`"); - assertSideEffect(true, "templateFunction`template`"); - assertSideEffect(true, "st = `${name}template`"); - assertSideEffect(true, "tempFunc = templateFunction`template`"); - - assertSideEffect(false, "new RegExp('foobar', 'i')"); - assertSideEffect(true, "new RegExp(SomethingWacky(), 'i')"); - assertSideEffect(false, "new Array()"); - assertSideEffect(false, "new Array"); - assertSideEffect(false, "new Array(4)"); - assertSideEffect(false, "new Array('a', 'b', 'c')"); - assertSideEffect(true, "new SomeClassINeverHeardOf()"); - assertSideEffect(true, "new SomeClassINeverHeardOf()"); - - assertSideEffect(false, "({}).foo = 4"); - assertSideEffect(false, "([]).foo = 4"); - assertSideEffect(false, "(function() {}).foo = 4"); - - assertSideEffect(true, "this.foo = 4"); - assertSideEffect(true, "a.foo = 4"); - assertSideEffect(true, "(function() { return n; })().foo = 4"); - assertSideEffect(true, "([]).foo = bar()"); - - assertSideEffect(false, "undefined"); - assertSideEffect(false, "void 0"); - assertSideEffect(true, "void foo()"); - assertSideEffect(false, "-Infinity"); - assertSideEffect(false, "Infinity"); - assertSideEffect(false, "NaN"); - - assertSideEffect(false, "({}||[]).foo = 2;"); - assertSideEffect(false, "(true ? {} : []).foo = 2;"); - assertSideEffect(false, "({},[]).foo = 2;"); - - assertSideEffect(true, "delete a.b"); - - assertSideEffect(false, "Math.random();"); - assertSideEffect(true, "Math.random(seed);"); - assertSideEffect(false, "[1, 1].foo;"); - - assertSideEffect(true, "export var x = 0;"); - assertSideEffect(true, "export let x = 0;"); - assertSideEffect(true, "export const x = 0;"); - assertSideEffect(true, "export class X {};"); - assertSideEffect(true, "export function x() {};"); - assertSideEffect(true, "export {x};"); - } - - @Test - public void testMayHaveSideEffects_iterableSpread() { - // ARRAYLIT-SPREAD - assertSideEffect(false, "[...[]]"); - assertSideEffect(false, "[...[1]]"); - assertSideEffect(true, "[...[i++]]"); - assertSideEffect(false, "[...'string']"); - assertSideEffect(false, "[...`templatelit`]"); - assertSideEffect(false, "[...`templatelit ${safe}`]"); - assertSideEffect(true, "[...`templatelit ${unsafe()}`]"); - assertSideEffect(true, "[...f()]"); - assertSideEffect(true, "[...5]"); - assertSideEffect(true, "[...null]"); - assertSideEffect(true, "[...true]"); - - // CALL-SPREAD - assertSideEffect(false, "Math.sin(...[])"); - assertSideEffect(false, "Math.sin(...[1])"); - assertSideEffect(true, "Math.sin(...[i++])"); - assertSideEffect(false, "Math.sin(...'string')"); - assertSideEffect(false, "Math.sin(...`templatelit`)"); - assertSideEffect(false, "Math.sin(...`templatelit ${safe}`)"); - assertSideEffect(true, "Math.sin(...`templatelit ${unsafe()}`)"); - assertSideEffect(true, "Math.sin(...f())"); - assertSideEffect(true, "Math.sin(...5)"); - assertSideEffect(true, "Math.sin(...null)"); - assertSideEffect(true, "Math.sin(...true)"); - - // NEW-SPREAD - assertSideEffect(false, "new Object(...[])"); - assertSideEffect(false, "new Object(...[1])"); - assertSideEffect(true, "new Object(...[i++])"); - assertSideEffect(false, "new Object(...'string')"); - assertSideEffect(false, "new Object(...`templatelit`)"); - assertSideEffect(false, "new Object(...`templatelit ${safe}`)"); - assertSideEffect(true, "new Object(...`templatelit ${unsafe()}`)"); - assertSideEffect(true, "new Object(...f())"); - assertSideEffect(true, "new Object(...5)"); - assertSideEffect(true, "new Object(...null)"); - assertSideEffect(true, "new Object(...true)"); - } - - @Test - public void testMayHaveSideEffects_objectSpread() { - // OBJECT-SPREAD - assertSideEffect(false, "({...x})"); - assertSideEffect(false, "({...{}})"); - assertSideEffect(false, "({...{a:1}})"); - assertSideEffect(true, "({...{a:i++}})"); - assertSideEffect(true, "({...{a:f()}})"); - assertSideEffect(true, "({...f()})"); - } - - @Test - public void testMayHaveSideEffects_rest() { - // REST - assertNodeHasSideEffect(false, REST, "({...x} = something)"); - // We currently assume all iterable-rests are side-effectful. - assertNodeHasSideEffect(true, REST, "([...x] = 'safe')"); - assertNodeHasSideEffect(false, REST, "(function(...x) { })"); - } - - @Test - public void testMayHaveSideEffects_contextSwitch() { - assertNodeHasSideEffect(true, AWAIT, "async function f() { await 0; }"); - assertNodeHasSideEffect(true, FOR_AWAIT_OF, "(async()=>{ for await (let x of []) {} })"); - assertNodeHasSideEffect(true, THROW, "function f() { throw 'something'; }"); - assertNodeHasSideEffect(true, YIELD, "function* f() { yield 'something'; }"); - assertNodeHasSideEffect(true, YIELD, "function* f() { yield* 'something'; }"); - } - - @Test - public void testMayHaveSideEffects_enhancedForLoop() { - // These edge cases are actually side-effect free. We include them to confirm we just give up - // on enhanced for loops. - assertSideEffect(true, "for (const x in []) { }"); - assertSideEffect(true, "for (const x of []) { }"); - } - - @Test - public void testMayHaveSideEffects_computedProp() { - assertNodeHasSideEffect(false, COMPUTED_PROP, "({[a]: x})"); - assertNodeHasSideEffect(true, COMPUTED_PROP, "({[a()]: x})"); - assertNodeHasSideEffect(true, COMPUTED_PROP, "({[a]: x()})"); - - // computed property getters and setters are modeled as COMPUTED_PROP with an - // annotation to indicate getter or setter. - assertNodeHasSideEffect(false, COMPUTED_PROP, "({ get [a]() {} })"); - assertNodeHasSideEffect(true, COMPUTED_PROP, "({ get [a()]() {} })"); - - assertNodeHasSideEffect(false, COMPUTED_PROP, "({ set [a](x) {} })"); - assertNodeHasSideEffect(true, COMPUTED_PROP, "({ set [a()](x) {} })"); - } - - @Test - public void testMayHaveSideEffects_classComputedProp() { - assertNodeHasSideEffect(false, COMPUTED_PROP, "class C { [a]() {} }"); - assertNodeHasSideEffect(true, COMPUTED_PROP, "class C { [a()]() {} }"); - - // computed property getters and setters are modeled as COMPUTED_PROP with an - // annotation to indicate getter or setter. - assertNodeHasSideEffect(false, COMPUTED_PROP, "class C { get [a]() {} }"); - assertNodeHasSideEffect(true, COMPUTED_PROP, "class C { get [a()]() {} }"); - - assertNodeHasSideEffect(false, COMPUTED_PROP, "class C { set [a](x) {} }"); - assertNodeHasSideEffect(true, COMPUTED_PROP, "class C { set [a()](x) {} }"); - } - - @Test - public void testMayHaveSideEffects_getter() { - assertNodeHasSideEffect(false, GETTER_DEF, "({ get a() {} })"); - assertNodeHasSideEffect(false, GETTER_DEF, "class C { get a() {} }"); - } - - @Test - public void testMayHaveSideEffects_setter() { - assertNodeHasSideEffect(false, SETTER_DEF, "({ set a(x) {} })"); - assertNodeHasSideEffect(false, SETTER_DEF, "class C { set a(x) {} }"); - } - - @Test - public void testMayHaveSideEffects_method() { - assertNodeHasSideEffect(false, MEMBER_FUNCTION_DEF, "({ a(x) {} })"); - assertNodeHasSideEffect(false, MEMBER_FUNCTION_DEF, "class C { a(x) {} }"); - } - - @Test - public void testMayHaveSideEffects_objectMethod() { - // "toString" and "valueOf" are assumed to be side-effect free - assertSideEffect(false, "o.toString()"); - assertSideEffect(false, "o.valueOf()"); - - // other methods depend on the extern definitions - assertSideEffect(true, "o.watch()"); - } - - @Test - public void testMayHaveSideEffects_regExp() { - // A RegExp Object by itself doesn't have any side-effects - assertSideEffect(false, "/abc/gi", true); - assertSideEffect(false, "/abc/gi", false); - - // RegExp instance methods have global side-effects, so whether they are - // considered side-effect free depends on whether the global properties - // are referenced. - assertSideEffect(true, "(/abc/gi).test('')", true); - assertSideEffect(false, "(/abc/gi).test('')", false); - assertSideEffect(true, "(/abc/gi).test(a)", true); - assertSideEffect(false, "(/abc/gi).test(b)", false); - - assertSideEffect(true, "(/abc/gi).exec('')", true); - assertSideEffect(false, "(/abc/gi).exec('')", false); - - // Some RegExp object method that may have side-effects. - assertSideEffect(true, "(/abc/gi).foo('')", true); - assertSideEffect(true, "(/abc/gi).foo('')", false); - - // Try the string RegExp ops. - assertSideEffect(true, "''.match('a')", true); - assertSideEffect(false, "''.match('a')", false); - assertSideEffect(true, "''.match(/(a)/)", true); - assertSideEffect(false, "''.match(/(a)/)", false); - - assertSideEffect(true, "''.replace('a')", true); - assertSideEffect(false, "''.replace('a')", false); - - assertSideEffect(true, "''.search('a')", true); - assertSideEffect(false, "''.search('a')", false); - - assertSideEffect(true, "''.split('a')", true); - assertSideEffect(false, "''.split('a')", false); - - // Some non-RegExp string op that may have side-effects. - assertSideEffect(true, "''.foo('a')", true); - assertSideEffect(true, "''.foo('a')", false); - - // 'a' might be a RegExp object with the 'g' flag, in which case - // the state might change by running any of the string ops. - // Specifically, using these methods resets the "lastIndex" if used - // in combination with a RegExp instance "exec" method. - assertSideEffect(true, "''.match(a)", true); - assertSideEffect(true, "''.match(a)", false); - - assertSideEffect(true, "'a'.replace(/a/, function (s) {alert(s)})", false); - assertSideEffect(false, "'a'.replace(/a/, 'x')", false); - } - @Test public void testIsFunctionExpression() { assertContainsAnonFunc(true, "(function(){})"); @@ -1901,76 +1581,6 @@ public void testLocalValueYield() { assertThat(NodeUtil.evaluatesToLocalValue(expr)).isFalse(); } - @Test - public void testCallSideEffects() { - ParseHelper helper = new ParseHelper(); - - // Parens force interpretation as an expression. - Node newXDotMethodCall = helper.parseFirst(CALL, "(new x().method());"); - Compiler compiler = helper.getCompiler(); - assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall, compiler)).isTrue(); - - Node newExpr = newXDotMethodCall.getFirstFirstChild(); - checkState(newExpr.isNew()); - Node.SideEffectFlags flags = new Node.SideEffectFlags(); - - // No side effects, local result - flags.clearAllFlags(); - newExpr.setSideEffectFlags(flags); - flags.clearAllFlags(); - newXDotMethodCall.setSideEffectFlags(flags); - - assertThat(NodeUtil.evaluatesToLocalValue(newXDotMethodCall)).isTrue(); - assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall, compiler)).isFalse(); - assertThat(NodeUtil.mayHaveSideEffects(newXDotMethodCall, compiler)).isFalse(); - - // Modifies this, local result - flags.clearAllFlags(); - newExpr.setSideEffectFlags(flags); - flags.clearAllFlags(); - flags.setMutatesThis(); - newXDotMethodCall.setSideEffectFlags(flags); - - assertThat(NodeUtil.evaluatesToLocalValue(newXDotMethodCall)).isTrue(); - assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall, compiler)).isFalse(); - assertThat(NodeUtil.mayHaveSideEffects(newXDotMethodCall, compiler)).isFalse(); - - // Modifies this, non-local result - flags.clearAllFlags(); - newExpr.setSideEffectFlags(flags); - flags.clearAllFlags(); - flags.setMutatesThis(); - flags.setReturnsTainted(); - newXDotMethodCall.setSideEffectFlags(flags); - - assertThat(NodeUtil.evaluatesToLocalValue(newXDotMethodCall)).isFalse(); - assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall, compiler)).isFalse(); - assertThat(NodeUtil.mayHaveSideEffects(newXDotMethodCall, compiler)).isFalse(); - - // No modifications, non-local result - flags.clearAllFlags(); - newExpr.setSideEffectFlags(flags); - flags.clearAllFlags(); - flags.setReturnsTainted(); - newXDotMethodCall.setSideEffectFlags(flags); - - assertThat(NodeUtil.evaluatesToLocalValue(newXDotMethodCall)).isFalse(); - assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall, compiler)).isFalse(); - assertThat(NodeUtil.mayHaveSideEffects(newXDotMethodCall, compiler)).isFalse(); - - // The new modifies global state, no side-effect call, non-local result - // This call could be removed, but not the new. - flags.clearAllFlags(); - flags.setMutatesGlobalState(); - newExpr.setSideEffectFlags(flags); - flags.clearAllFlags(); - newXDotMethodCall.setSideEffectFlags(flags); - - assertThat(NodeUtil.evaluatesToLocalValue(newXDotMethodCall)).isTrue(); - assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall, compiler)).isFalse(); - assertThat(NodeUtil.mayHaveSideEffects(newXDotMethodCall, compiler)).isTrue(); - } - @Test public void testGetOctalNumberValue() { assertThat(NodeUtil.getNumberValue(parseExpr("022"))).isEqualTo(18.0);