diff --git a/src/com/google/javascript/jscomp/AstAnalyzer.java b/src/com/google/javascript/jscomp/AstAnalyzer.java index 5acaa825048..010a01e5a4f 100644 --- a/src/com/google/javascript/jscomp/AstAnalyzer.java +++ b/src/com/google/javascript/jscomp/AstAnalyzer.java @@ -513,9 +513,13 @@ boolean nodeTypeMayHaveSideEffects(Node n) { case NEW: return constructorCallHasSideEffects(n); case NAME: - // A variable definition. + // A variable definition that assigns a value. // TODO(b/129564961): Consider EXPORT declarations. return n.hasChildren(); + case DESTRUCTURING_LHS: + // A destructuring declaration statement or assignment. Technically these might contain no + // lvalues but that case is rare enough to be ignored. + return true; case OBJECT_REST: case OBJECT_SPREAD: // Object-rest and object-spread may trigger a getter. diff --git a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java index 852dbcacede..7f0bf4b972f 100644 --- a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java +++ b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java @@ -707,12 +707,18 @@ private void updateSideEffectsForNode( // e.g. // lhs = rhs; // ({x, y} = object); + Node lhs = node.getFirstChild(); + // Consider destructured properties or values to be nonlocal. + Predicate rhsLocality = + lhs.isDestructuringPattern() + ? RHS_IS_NEVER_LOCAL + : FIND_RHS_AND_CHECK_FOR_LOCAL_VALUE; visitLhsNodes( encloserSummary, traversal.getScope(), enclosingFunction, NodeUtil.findLhsNodesInNode(node), - FIND_RHS_AND_CHECK_FOR_LOCAL_VALUE); + rhsLocality); break; case INC: // e.g. x++; @@ -765,6 +771,21 @@ private void updateSideEffectsForNode( visitCall(encloserSummary, node); break; + case DESTRUCTURING_LHS: + if (NodeUtil.isAnyFor(node.getParent())) { + // This case is handled when visiting the enclosing for loop. + break; + } + // Assume the value assigned to each item is potentially global state. This is overly + // conservative but necessary because in the common case the rhs is not a literal. + visitLhsNodes( + encloserSummary, + traversal.getScope(), + enclosingFunction, + NodeUtil.findLhsNodesInNode(node.getParent()), + RHS_IS_NEVER_LOCAL); + break; + case NAME: // Variable definition are not side effects. Check that the name appears in the context of // a variable declaration. diff --git a/test/com/google/javascript/jscomp/AstAnalyzerTest.java b/test/com/google/javascript/jscomp/AstAnalyzerTest.java index 6874c8ef4e4..ab2f8328420 100644 --- a/test/com/google/javascript/jscomp/AstAnalyzerTest.java +++ b/test/com/google/javascript/jscomp/AstAnalyzerTest.java @@ -21,6 +21,7 @@ import static com.google.javascript.jscomp.DiagnosticGroups.ES5_STRICT; import static com.google.javascript.rhino.Token.ADD; import static com.google.javascript.rhino.Token.ARRAYLIT; +import static com.google.javascript.rhino.Token.ARRAY_PATTERN; 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; @@ -33,6 +34,7 @@ import static com.google.javascript.rhino.Token.DEC; import static com.google.javascript.rhino.Token.DEFAULT_VALUE; import static com.google.javascript.rhino.Token.DELPROP; +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_IN; import static com.google.javascript.rhino.Token.FOR_OF; @@ -50,6 +52,7 @@ import static com.google.javascript.rhino.Token.NEW; import static com.google.javascript.rhino.Token.NUMBER; import static com.google.javascript.rhino.Token.OBJECTLIT; +import static com.google.javascript.rhino.Token.OBJECT_PATTERN; import static com.google.javascript.rhino.Token.OBJECT_REST; import static com.google.javascript.rhino.Token.OBJECT_SPREAD; import static com.google.javascript.rhino.Token.OR; @@ -629,6 +632,15 @@ public static Iterable cases() { kase().js("var x;").token(NAME).expect(false), kase().js("let x;").token(NAME).expect(false), + // destructuring declarations and assignments are always considered side effectful even + // when empty + kase().js("var {x} = {};").token(DESTRUCTURING_LHS).expect(true), + kase().js("var {} = {};").token(DESTRUCTURING_LHS).expect(true), + kase().js("var [x] = [];").token(DESTRUCTURING_LHS).expect(true), + kase().js("var {y: [x]} = {};").token(OBJECT_PATTERN).expect(false), + kase().js("var {y: [x]} = {};").token(ARRAY_PATTERN).expect(false), + kase().js("[x] = arr;").token(ASSIGN).expect(true), + // 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. diff --git a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java index 19b79c0f74f..a23ed0cffcb 100644 --- a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java +++ b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java @@ -2506,8 +2506,6 @@ public void testArgumentTaintedByWayOfBlockScopedConst() { @Test public void testArgumentTaintedByWayOfForOfScopedConst() { - // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. - disableTypeCheck(); String source = lines( "function m1(elements) {", @@ -2517,13 +2515,27 @@ public void testArgumentTaintedByWayOfForOfScopedConst() { "}", "m1([]);", ""); - assertPureCallsMarked(source, ImmutableList.of()); + assertNoPureCalls(source); + } + + @Test + public void testArgumentTaintedByWayOfForOfScopedConstWithDestructuring() { + ignoreWarnings(TypeCheck.POSSIBLE_INEXISTENT_PROPERTY); + String source = + lines( + "function m1(elements) {", + " for (const {e} of elements) {", + " e.someProp = 1;", + " }", + "}", + "m1([]);", + ""); + assertNoPureCalls(source); } @Test public void testArgumentTaintedByWayOfNameDeclaration() { - // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. - disableTypeCheck(); + ignoreWarnings(TypeCheck.POSSIBLE_INEXISTENT_PROPERTY); String source = lines( "function m1(obj) {", @@ -2535,26 +2547,50 @@ public void testArgumentTaintedByWayOfNameDeclaration() { assertPureCallsMarked(source, ImmutableList.of()); } + @Test + public void testUnusedDestructuringNameDeclarationIsPure() { + ignoreWarnings(TypeCheck.POSSIBLE_INEXISTENT_PROPERTY); + String source = + lines( + "function m1(obj) {", // + " let {p} = obj;", + "}", + "m1([]);", + ""); + assertPureCallsMarked(source, ImmutableList.of("m1")); + } + @Test public void testArgumentTaintedByWayOfDestructuringNameDeclaration() { - // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. - disableTypeCheck(); + ignoreWarnings(TypeCheck.POSSIBLE_INEXISTENT_PROPERTY); String source = lines( - "function m1(obj) {", + "function m1(obj) {", // " let {p} = obj;", " p.someProp = 1;", "}", "m1([]);", ""); - // TODO(bradfordcsmith): Fix this logic for destructuring declarations. - assertPureCallsMarked(source, ImmutableList.of("m1")); + assertNoPureCalls(source); + } + + @Test + public void testArgumentTaintedByWayOfNestedDestructuringNameDeclaration() { + ignoreWarnings(TypeCheck.POSSIBLE_INEXISTENT_PROPERTY); + String source = + lines( + "function m1(obj) {", // + " let {q: {p}} = obj;", + " p.someProp = 1;", + "}", + "m1({});", + ""); + assertNoPureCalls(source); } @Test public void testArgumentTaintedByWayOfDestructuringAssignment() { - // TODO(bradfordcsmith): Enable type check when it supports the languages features used here. - disableTypeCheck(); + ignoreWarnings(TypeCheck.POSSIBLE_INEXISTENT_PROPERTY); String source = lines( "function m1(obj) {", @@ -2564,8 +2600,65 @@ public void testArgumentTaintedByWayOfDestructuringAssignment() { "}", "m1([]);", ""); - // TODO(bradfordcsmith): Fix this logic for destructuring. - assertPureCallsMarked(source, ImmutableList.of("m1")); + assertNoPureCalls(source); + } + + @Test + public void testArgumentTaintedByWayOfArrayDestructuringAssignment() { + String source = + lines( + "function m1(obj) {", + " let p;", + " ([p] = obj);", + " p.someProp = 1;", + "}", + "m1([]);", + ""); + assertNoPureCalls(source); + } + + @Test + public void testDestructuringAssignMutatingGlobalState() { + String source = + lines( + "var a = 0;", // + "function m1() {", + " ([a] = []);", + "}", + "m1();"); + assertNoPureCalls(source); + + source = + lines( + "var a = 0;", // + "function m1() {", + " ({a} = {a: 0});", + "}", + "m1();"); + assertNoPureCalls(source); + } + + @Test + public void testDefaultValueInitializers_areConsidered_whenAnalyzingDestructuring() { + assertNoPureCalls( + lines( + "function impure() { throw 'something'; }", // + "", + "function foo() { const {a = impure()} = {a: undefined}; }", + "foo();")); + + assertPureCallsMarked( + lines( + "function foo() { const {a = 0} = {a: undefined}; }", // + "foo();"), + ImmutableList.of("foo")); + + assertNoPureCalls( + lines( + "function impure() { throw 'something'; }", // + "", + "function foo() { const [{a = impure()}] = [{a: undefined}]; }", + "foo();")); } @Test