Skip to content

Commit

Permalink
Fix PureFunctionIdentifier's detection of destructuring aliasing of n…
Browse files Browse the repository at this point in the history 
…on-locals

Fixes #3499

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=294724972
  • Loading branch information
@lauraharker @rrdelaney
lauraharker authored and rrdelaney committed Feb 13, 2020
1 parent 585cc73 commit 9704bb4
Show file tree
Hide file tree
Showing 4 changed files with 146 additions and 16 deletions.
6 changes: 5 additions & 1 deletion src/com/google/javascript/jscomp/AstAnalyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
23 changes: 22 additions & 1 deletion src/com/google/javascript/jscomp/PureFunctionIdentifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Node> 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++;
Expand Down Expand Up @@ -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.
Expand Down
12 changes: 12 additions & 0 deletions test/com/google/javascript/jscomp/AstAnalyzerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -629,6 +632,15 @@ public static Iterable<AnalysisCase> 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.
Expand Down
121 changes: 107 additions & 14 deletions test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {",
Expand All @@ -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) {",
Expand All @@ -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) {",
Expand All @@ -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
Expand Down

0 comments on commit 9704bb4

Please sign in to comment.