diff --git a/src/com/google/javascript/jscomp/AbstractScope.java b/src/com/google/javascript/jscomp/AbstractScope.java index 6559ee7d1f8..903cd1a0d20 100644 --- a/src/com/google/javascript/jscomp/AbstractScope.java +++ b/src/com/google/javascript/jscomp/AbstractScope.java @@ -429,7 +429,8 @@ S getCommonParent(S other) { * the two scopes would be equivalent in a pre-ES2015-block-scopes view of the world. */ boolean hasSameContainerScope(S other) { - return getClosestContainerScope() == other.getClosestContainerScope(); + // Do identity check first as a shortcut. + return this == other || getClosestContainerScope() == other.getClosestContainerScope(); } /** diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index 2956cbd5f62..a55b347c906 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -1498,7 +1498,7 @@ static boolean newHasLocalResult(Node n) { * *

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 reason's why a subtree has side effects. + * the reasons why a subtree has side effects. */ static boolean nodeTypeMayHaveSideEffects(Node n) { return nodeTypeMayHaveSideEffects(n, null); @@ -1516,6 +1516,8 @@ static boolean nodeTypeMayHaveSideEffects(Node n, AbstractCompiler compiler) { case YIELD: case THROW: case AWAIT: + case FOR_IN: // assigns to a loop LHS + case FOR_OF: // assigns to a loop LHS return true; case CALL: return NodeUtil.functionCallHasSideEffects(n, compiler); @@ -4236,6 +4238,14 @@ private static void getLhsNodesHelper(Node n, List lhsNodes) { return; case EMPTY: return; + case FOR_IN: + case FOR_OF: + // Enhanced for loops assign to variables in their first child + // e.g. + // for (some.prop in someObj) {... + // for ({a, b} of someIterable) {... + getLhsNodesHelper(n.getFirstChild(), lhsNodes); + return; default: if (isAssignmentOp(n)) { getLhsNodesHelper(n.getFirstChild(), lhsNodes); @@ -4245,19 +4255,25 @@ private static void getLhsNodesHelper(Node n, List lhsNodes) { } } - /** Retrieves lhs nodes declared in the current declaration or ASSIGN statement. */ - public static List findLhsNodesInNode(Node declNode) { + /** + * Retrieves lhs nodes declared or assigned in a given assigning parent node. + * + *

An assigning parent node is one that assigns a value to one or more LHS nodes. + */ + public static List findLhsNodesInNode(Node assigningParent) { checkArgument( - isNameDeclaration(declNode) - || declNode.isParamList() - || isAssignmentOp(declNode) - || declNode.isCatch() - || declNode.isDestructuringLhs() - || declNode.isDefaultValue() - || declNode.isImport(), - declNode); + isNameDeclaration(assigningParent) + || assigningParent.isParamList() + || isAssignmentOp(assigningParent) + || assigningParent.isCatch() + || assigningParent.isDestructuringLhs() + || assigningParent.isDefaultValue() + || assigningParent.isImport() + // enhanced for loops assign to loop variables + || isEnhancedFor(assigningParent), + assigningParent); ArrayList lhsNodes = new ArrayList<>(); - getLhsNodesHelper(declNode, lhsNodes); + getLhsNodesHelper(assigningParent, lhsNodes); return lhsNodes; } diff --git a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java index 253c9c0c74f..8237b09fe9b 100644 --- a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java +++ b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java @@ -49,6 +49,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Predicate; import javax.annotation.Nullable; /** @@ -435,6 +436,14 @@ private void markPureFunctionCalls() { } } + private static final Predicate RHS_IS_ALWAYS_LOCAL = lhs -> true; + private static final Predicate RHS_IS_NEVER_LOCAL = lhs -> false; + private static final Predicate FIND_RHS_AND_CHECK_FOR_LOCAL_VALUE = lhs -> { + Node rhs = NodeUtil.getRValueOfLValue(lhs); + return rhs == null || NodeUtil.evaluatesToLocalValue(rhs); + }; + + /** * Gather list of functions, functions with @nosideeffects annotations, call sites, and functions * that may mutate variables not defined in the local scope. @@ -478,11 +487,12 @@ public void visit(NodeTraversal traversal, Node node, Node parent) { allFunctionCalls.add(node); } - // TODO: This may be more expensive than necessary. - Node enclosingFunction = traversal.getEnclosingFunction(); - if (enclosingFunction == null) { + Scope containerScope = traversal.getScope().getClosestContainerScope(); + if (!containerScope.isFunctionScope()) { + // We only need to look at nodes in function scopes. return; } + Node enclosingFunction = containerScope.getRootNode(); for (FunctionInformation sideEffectInfo : functionSideEffectMap.get(enclosingFunction)) { checkNotNull(sideEffectInfo); @@ -495,9 +505,64 @@ public void updateSideEffectsForNode( NodeTraversal traversal, Node node, Node enclosingFunction) { - if (NodeUtil.isAssignmentOp(node) || node.isInc() || node.isDelProp() || node.isDec()) { - visitAssignmentOrUnaryOperator( - sideEffectInfo, traversal.getScope(), node, enclosingFunction); + if (node.isAssign()) { + // e.g. + // lhs = rhs; + // ({x, y} = object); + visitLhsNodes( + sideEffectInfo, + traversal.getScope(), + enclosingFunction, + NodeUtil.findLhsNodesInNode(node), + FIND_RHS_AND_CHECK_FOR_LOCAL_VALUE); + } else if (NodeUtil.isCompoundAssignmentOp(node)) { + // e.g. + // x += 3; + visitLhsNodes( + sideEffectInfo, + traversal.getScope(), + enclosingFunction, + ImmutableList.of(node.getFirstChild()), + // The update assignments (e.g. `+=) always assign primitive, and therefore local, + // values. + RHS_IS_ALWAYS_LOCAL); + } else if (node.isInc() || node.isDelProp() || node.isDec()) { + // e.g. + // x++; + visitLhsNodes( + sideEffectInfo, + traversal.getScope(), + enclosingFunction, + ImmutableList.of(node.getOnlyChild()), + // The value assigned by a unary op is always local. + RHS_IS_ALWAYS_LOCAL); + } else if (node.isForOf()) { + // e.g. + // for (const {prop1, prop2} of iterable) {...} + // for ({prop1: x.p1, prop2: x.p2} of iterable) {...} + // + // TODO(bradfordcsmith): Possibly we should try to determine whether the iteration itself + // could have side effects. + visitLhsNodes( + sideEffectInfo, + traversal.getScope(), + enclosingFunction, + NodeUtil.findLhsNodesInNode(node), + // The RHS of a for-of must always be an iterable, making it a container, so we can't + // consider its contents to be local + RHS_IS_NEVER_LOCAL); + } else if (node.isForIn()) { + // e.g. + // for (prop in obj) {...} + // Also this, though not very useful or readable. + // for ([char1, char2, ...x.rest] in obj) {...} + visitLhsNodes( + sideEffectInfo, + traversal.getScope(), + enclosingFunction, + NodeUtil.findLhsNodesInNode(node), + // A for-in always assigns a string, which is a local value by definition. + RHS_IS_ALWAYS_LOCAL); } else if (NodeUtil.isCallOrNew(node)) { visitCall(sideEffectInfo, node); } else if (node.isName()) { @@ -537,14 +602,12 @@ public void enterScope(NodeTraversal t) { @Override public void exitScope(NodeTraversal t) { - if (!t.getScope().isFunctionBlockScope() && !t.getScope().isFunctionScope()) { - return; - } - - Node function = NodeUtil.getEnclosingFunction(t.getScopeRoot()); - if (function == null) { + Scope closestContainerScope = t.getScope().getClosestContainerScope(); + if (!closestContainerScope.isFunctionScope()) { + // Only functions and the scopes within them are of interest to us. return; } + Node function = closestContainerScope.getRootNode(); // Handle deferred local variable modifications: for (FunctionInformation sideEffectInfo : functionSideEffectMap.get(function)) { @@ -589,34 +652,28 @@ public void exitScope(NodeTraversal t) { } } - private boolean isVarDeclaredInScope(@Nullable Var v, Scope scope) { - if (v == null) { - return false; - } - if (v.scope == scope) { - return true; - } - Node declarationRoot = NodeUtil.getEnclosingFunction(v.scope.getRootNode()); - Node scopeRoot = NodeUtil.getEnclosingFunction(scope.getRootNode()); - return declarationRoot == scopeRoot; + private boolean isVarDeclaredInSameContainerScope(@Nullable Var v, Scope scope) { + return v != null && v.scope.hasSameContainerScope(scope); } /** - * Record information about the side effects caused by an assignment or mutating unary operator. + * Record information about the side effects caused by assigning a value to a given LHS. * *

If the operation modifies this or taints global state, mark the enclosing function as * having those side effects. * - * @param op operation being performed. + * @param sideEffectInfo Function side effect record to be updated + * @param scope variable scope in which the variable assignment occurs + * @param enclosingFunction FUNCTION node for the enclosing function + * @param lhsNodes LHS nodes that are all assigned values by a given parent node + * @param hasLocalRhs Predicate indicating whether a given LHS is being assigned a local value */ - private void visitAssignmentOrUnaryOperator( - FunctionInformation sideEffectInfo, Scope scope, Node op, Node enclosingFunction) { - Iterable lhsNodes; - if (isIncDec(op) || op.isDelProp()) { - lhsNodes = ImmutableList.of(op.getOnlyChild()); - } else { - lhsNodes = NodeUtil.findLhsNodesInNode(op); - } + private void visitLhsNodes( + FunctionInformation sideEffectInfo, + Scope scope, + Node enclosingFunction, + Iterable lhsNodes, + Predicate hasLocalRhs) { for (Node lhs : lhsNodes) { if (NodeUtil.isGet(lhs)) { if (lhs.getFirstChild().isThis()) { @@ -625,7 +682,7 @@ private void visitAssignmentOrUnaryOperator( Node objectNode = lhs.getFirstChild(); if (objectNode.isName()) { Var var = scope.getVar(objectNode.getString()); - if (isVarDeclaredInScope(var, scope)) { + if (isVarDeclaredInSameContainerScope(var, scope)) { // Maybe a local object modification. We won't know for sure until // we exit the scope and can validate the value of the local. taintedVarsByFunction.put(enclosingFunction, var); @@ -633,22 +690,18 @@ private void visitAssignmentOrUnaryOperator( sideEffectInfo.setTaintsGlobalState(); } } else { - // TODO(tdeegan): Perhaps handle multi level locals: local.prop.prop2++; + // Don't track multi level locals: local.prop.prop2++; sideEffectInfo.setTaintsGlobalState(); } } } else { + checkState(lhs.isName(), lhs); Var var = scope.getVar(lhs.getString()); - if (isVarDeclaredInScope(var, scope)) { - // Assignment to local, if the value isn't a safe local value, - // a literal or new object creation, add it to the local blacklist. - // parameter values depend on the caller. - - // Note: other ops result in the name or prop being assigned a local - // value (x++ results in a number, for instance) - checkState(NodeUtil.isAssignmentOp(op) || isIncDec(op) || op.isDelProp()); - Node rhs = NodeUtil.getRValueOfLValue(lhs); - if (rhs != null && op.isAssign() && !NodeUtil.evaluatesToLocalValue(rhs)) { + if (isVarDeclaredInSameContainerScope(var, scope)) { + if (!hasLocalRhs.test(lhs)) { + // Assigned value is not guaranteed to be a local value, + // so if we see any property assignments on this variable, + // they could be tainting a non-local value. blacklistedVarsByFunction.put(enclosingFunction, var); } } else { @@ -685,11 +738,6 @@ private void visitCall(FunctionInformation sideEffectInfo, Node node) { } } - private static boolean isIncDec(Node n) { - Token type = n.getToken(); - return (type == Token.INC || type == Token.DEC); - } - private static boolean isCallOrApply(Node callSite) { return NodeUtil.isFunctionObjectCall(callSite) || NodeUtil.isFunctionObjectApply(callSite); } diff --git a/src/com/google/javascript/jscomp/testing/NodeSubject.java b/src/com/google/javascript/jscomp/testing/NodeSubject.java index 292d7a40efe..4c5d9903fe5 100644 --- a/src/com/google/javascript/jscomp/testing/NodeSubject.java +++ b/src/com/google/javascript/jscomp/testing/NodeSubject.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertAbout; import static org.junit.Assert.assertEquals; +import com.google.common.truth.Fact; import com.google.common.truth.FailureMetadata; import com.google.common.truth.Subject; import com.google.javascript.rhino.Node; @@ -55,6 +56,23 @@ public void hasType(Token type) { assertEquals(message, type, actual().getToken()); } + public void isName(String name) { + Node node = actual(); + if (!node.isName()) { + failWithActual(Fact.simpleFact("expected to be a NAME node")); + } + if (!node.getString().equals(name)) { + failWithActual("expected name to be", name); + } + } + + public void matchesQualifiedName(String qname) { + Node node = actual(); + if (!node.matchesQualifiedName(qname)) { + failWithActual("expected qualified name", qname); + } + } + public void hasCharno(int charno) { assertEquals(charno, actual().getCharno()); } diff --git a/test/com/google/javascript/jscomp/IntegrationTest.java b/test/com/google/javascript/jscomp/IntegrationTest.java index 8deb66f6eed..fb18a581c2f 100644 --- a/test/com/google/javascript/jscomp/IntegrationTest.java +++ b/test/com/google/javascript/jscomp/IntegrationTest.java @@ -49,6 +49,33 @@ public final class IntegrationTest extends IntegrationTestCase { private static final String CLOSURE_COMPILED = "var COMPILED = true; var goog$exportSymbol = function() {};"; + public void testForOfDoesNotFoolSideEffectDetection() { + CompilerOptions options = createCompilerOptions(); + CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options); + options.setLanguageIn(LanguageMode.ECMASCRIPT_NEXT); + options.setLanguageOut(LanguageMode.ECMASCRIPT_NEXT); + // we don't want to see injected library code in the output + useNoninjectingCompiler = true; + testSame( + options, + lines( + "class C {", + " constructor(elements) {", + " this.elements = elements;", + " this.m1();", // this call should not be removed, because it has side effects + " }", + // m1() must be considered to have side effects because it taints a non-local object + // through basically this.elements[i].sompProp. + " m1() {", + " for (const element of this.elements) {", + " element.someProp = 1;", + " }", + " }", + "}", + "new C([]);", + "")); + } + public void testIssue2822() { CompilerOptions options = createCompilerOptions(); CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options); diff --git a/test/com/google/javascript/jscomp/NodeUtilTest.java b/test/com/google/javascript/jscomp/NodeUtilTest.java index 2f693088c85..f0fd12a158b 100644 --- a/test/com/google/javascript/jscomp/NodeUtilTest.java +++ b/test/com/google/javascript/jscomp/NodeUtilTest.java @@ -42,6 +42,7 @@ import java.util.Collection; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; @@ -3368,6 +3369,36 @@ public void testFindLhsNodesInNode() { assertThat(findLhsNodesInNode("x.y *= 2;")).hasSize(1); } + public void testFindLhsNodesInForOfWithDeclaration() { + Iterable lhsNodes = findLhsNodesInNode("for (const {x, y} of iterable) {}"); + assertThat(lhsNodes).hasSize(2); + Iterator nodeIterator = lhsNodes.iterator(); + assertNode(nodeIterator.next()).isName("x"); + assertNode(nodeIterator.next()).isName("y"); + } + + public void testFindLhsNodesInForOfWithoutDeclaration() { + Iterable lhsNodes = findLhsNodesInNode("for ({x, y: a.b} of iterable) {}"); + assertThat(lhsNodes).hasSize(2); + Iterator nodeIterator = lhsNodes.iterator(); + assertNode(nodeIterator.next()).isName("x"); + assertNode(nodeIterator.next()).matchesQualifiedName("a.b"); + } + + public void testFindLhsNodesInForInWithDeclaration() { + Iterable lhsNodes = findLhsNodesInNode("for (const x in obj) {}"); + assertThat(lhsNodes).hasSize(1); + Iterator nodeIterator = lhsNodes.iterator(); + assertNode(nodeIterator.next()).isName("x"); + } + + public void testFindLhsNodesInForInWithoutDeclaration() { + Iterable lhsNodes = findLhsNodesInNode("for (a.b in iterable) {}"); + assertThat(lhsNodes).hasSize(1); + Iterator nodeIterator = lhsNodes.iterator(); + assertNode(nodeIterator.next()).matchesQualifiedName("a.b"); + } + public void testIsConstructor() { assertTrue(NodeUtil.isConstructor(getFunctionNode("/** @constructor */ function Foo() {}"))); assertTrue(NodeUtil.isConstructor(getFunctionNode( diff --git a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java index 3a5ef3e9706..0a02bbb1a7b 100644 --- a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java +++ b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java @@ -1923,6 +1923,232 @@ public void testClassMethod3() { assertPureCallsMarked(source, ImmutableList.of("this.m1", "NEW STRING m2")); } + public void testGlobalScopeTaintedByWayOfThisPropertyAndForOfLoop() { + // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. + disableTypeCheck(); + String source = + lines( + "class C {", + " constructor(elements) {", + " this.elements = elements;", + " this.m1();", + " }", + " m1() {", + " for (const element of this.elements) {", + " element.someProp = 1;", + " }", + " }", + "}", + "new C([]).m1()", + ""); + assertPureCallsMarked(source, ImmutableList.of()); + } + + public void testGlobalScopeTaintedByWayOfThisPropertyAndForOfLoopWithDestructuring() { + // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. + disableTypeCheck(); + String source = + lines( + "class C {", + " constructor(elements) {", + " this.elements = elements;", + " this.m1();", + " }", + " m1() {", + " this.elements[0].someProp = 1;", + " for (const {prop} of this.elements) {", + " prop.someProp = 1;", + " }", + " }", + "}", + "new C([]).m1()", + ""); + assertPureCallsMarked(source, ImmutableList.of()); + } + + public void testArgumentTaintedByWayOfFunctionScopedLet() { + // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. + disableTypeCheck(); + String source = + lines( + "function m1(elements) {", + " let e;", + " for (let i = 0; i < elements.length; ++i) {", + " e = elements[i];", + " e.someProp = 1;", + " }", + "}", + "m1([]);", + ""); + assertPureCallsMarked(source, ImmutableList.of()); + } + + public void testArgumentTaintedByWayOfFunctionScopedLetAssignedInForOf() { + // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. + disableTypeCheck(); + String source = + lines( + "function m1(elements) {", + " let e;", + " for (e of elements) {", + " e.someProp = 1;", + " }", + "}", + "m1([]);", + ""); + assertPureCallsMarked(source, ImmutableList.of()); + } + + public void testArgumentTaintedByWayOfFunctionScopedLetAssignedInForOfWithDestructuring() { + // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. + disableTypeCheck(); + String source = + lines( + "function m1(elements) {", + " let e;", + " for ({e} of elements) {", + " e.someProp = 1;", + " }", + "}", + "m1([]);", + ""); + assertPureCallsMarked(source, ImmutableList.of()); + } + + public void testArgumentTaintedByWayOfForOfAssignmentToQualifiedName() { + // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. + disableTypeCheck(); + String source = + lines( + "function m1(obj, elements) {", + " for (obj.e of elements) {", + " if (obj.e != null) break;", + " }", + "}", + "var globalObj = {};", + "m1(globalObj, []);", + ""); + assertPureCallsMarked(source, ImmutableList.of()); + } + + public void testArgumentTaintedByWayOfForInAssignmentToQualifiedName() { + // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. + String source = + lines( + "/**", + " *@param {!Object} obj", + " *@param {!Object} propObj", + " *@returns {number}", + " */", + "function m1(/** ? */ obj, /** Object */ propObj) {", + " var propCharCount = 0;", + // TODO(bradfordcsmith): Fix JSC_POSSIBLE_INEXISTENT_PROPERTY warning that occurs + // if the assignment to obj.e before the loop is dropped from this test case. + " obj.e = '';", + " for (obj.e in propObj) {", + " propCharCount += obj.e.length;", + " }", + " return propCharCount;", + "}", + "var globalObj = {};", + "m1(globalObj, {x: 1});", + ""); + assertPureCallsMarked(source, ImmutableList.of()); + } + + public void testArgumentTaintedByWayOfBlockScopedLet() { + // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. + disableTypeCheck(); + String source = + lines( + "function m1(elements) {", + " for (let i = 0; i < elements.length; ++i) {", + " let e;", + " e = elements[i];", + " e.someProp = 1;", + " }", + "}", + "m1([]);", + ""); + assertPureCallsMarked(source, ImmutableList.of()); + } + + public void testArgumentTaintedByWayOfBlockScopedConst() { + // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. + disableTypeCheck(); + String source = + lines( + "function m1(elements) {", + " for (let i = 0; i < elements.length; ++i) {", + " const e = elements[i];", + " e.someProp = 1;", + " }", + "}", + "m1([]);", + ""); + assertPureCallsMarked(source, ImmutableList.of()); + } + + public void testArgumentTaintedByWayOfForOfScopedConst() { + // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. + disableTypeCheck(); + String source = + lines( + "function m1(elements) {", + " for (const e of elements) {", + " e.someProp = 1;", + " }", + "}", + "m1([]);", + ""); + assertPureCallsMarked(source, ImmutableList.of()); + } + + public void testArgumentTaintedByWayOfNameDeclaration() { + // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. + disableTypeCheck(); + String source = + lines( + "function m1(obj) {", + " let p = obj.p;", + " p.someProp = 1;", + "}", + "m1([]);", + ""); + assertPureCallsMarked(source, ImmutableList.of()); + } + + public void testArgumentTaintedByWayOfDestructuringNameDeclaration() { + // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. + disableTypeCheck(); + String source = + lines( + "function m1(obj) {", + " let {p} = obj;", + " p.someProp = 1;", + "}", + "m1([]);", + ""); + // TODO(bradfordcsmith): Fix this logic for destructuring declarations. + assertPureCallsMarked(source, ImmutableList.of("m1")); + } + + public void testArgumentTaintedByWayOfDestructuringAssignment() { + // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. + disableTypeCheck(); + String source = + lines( + "function m1(obj) {", + " let p;", + " ({p} = obj);", + " p.someProp = 1;", + "}", + "m1([]);", + ""); + // TODO(bradfordcsmith): Fix this logic for destructuring. + assertPureCallsMarked(source, ImmutableList.of("m1")); + } + public void testFunctionProperties1() throws Exception { String source = lines( "/** @constructor */",