Skip to content

Commit

Permalink
PureFunctionIdentifier: use Var methods to correct handling of ES6 fe…
Browse files Browse the repository at this point in the history 
…atures

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=185067174
  • Loading branch information
@brad4d @blickly
brad4d authored and blickly committed Feb 9, 2018
1 parent ad17433 commit 584aed0
Show file tree
Hide file tree
Showing 2 changed files with 146 additions and 52 deletions.
5 changes: 2 additions & 3 deletions src/com/google/javascript/jscomp/PureFunctionIdentifier.java
View file
Expand Up @@ -555,8 +555,7 @@ public void exitScope(NodeTraversal t) {
}

for (Var v : t.getScope().getVarIterable()) {
boolean param = v.getParentNode().isParamList();
if (param
if (v.isParam()
&& !blacklistedVarsByFunction.containsEntry(function, v)
&& taintedVarsByFunction.containsEntry(function, v)) {
sideEffectInfo.setTaintsArguments();
Expand All @@ -565,7 +564,7 @@ public void exitScope(NodeTraversal t) {

boolean localVar = false;
// Parameters and catch values can come from other scopes.
if (v.getParentNode().isVar()) {
if (!v.isParam() && !v.isCatch()) {
// TODO(johnlenz): create a useful parameter list
// sideEffectInfo.addKnownLocal(v.getName());
localVar = true;
Expand Down
193 changes: 144 additions & 49 deletions test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java
View file
Expand Up @@ -924,10 +924,19 @@ public void testLocalizedSideEffects3() throws Exception {

public void testLocalizedSideEffects4() throws Exception {
// An array is a local object, assigning a local array is not a global side-effect.
String source = lines(
"function f() {var x = []; x[0] = 1;}",
"f()");
assertPureCallsMarked(source, ImmutableList.of("f"));
assertPureCallsMarked(
lines(
"function f() {var x = []; x[0] = 1;}", // preserve newline
"f()"),
ImmutableList.of("f"));

// TODO(bradfordcsmith): Remove NEITHER when type checker understands let/const
mode = TypeInferenceMode.NEITHER;
assertPureCallsMarked(
lines(
"function f() {const x = []; x[0] = 1;}", // preserve newline
"f()"),
ImmutableList.of("f"));
}

public void testLocalizedSideEffects5() throws Exception {
Expand All @@ -944,26 +953,47 @@ public void testLocalizedSideEffects5() throws Exception {
public void testLocalizedSideEffects6() throws Exception {
// Returning a local object that has been modified
// is not a global side-effect.
String source = lines(
"function f() {",
" var x = {}; x.foo = 1; return x;",
"}",
"f()"
);
assertPureCallsMarked(source, ImmutableList.of("f"));
assertPureCallsMarked(
lines(
"function f() {", // preserve newline
" var x = {}; x.foo = 1; return x;",
"}",
"f()"),
ImmutableList.of("f"));

// TODO(bradfordcsmith): Remove NEITHER when type checker understands let/const
mode = TypeInferenceMode.NEITHER;
assertPureCallsMarked(
lines(
"function f() {", // preserve newline
" const x = {}; x.foo = 1; return x;",
"}",
"f()"),
ImmutableList.of("f"));
}

public void testLocalizedSideEffects7() throws Exception {
// Returning a local object that has been modified
// is not a global side-effect.
String source = lines(
"/** @constructor A */ function A() {};",
"function f() {",
" var a = []; a[1] = 1; return a;",
"}",
"f()"
);
assertPureCallsMarked(source, ImmutableList.of("f"));
assertPureCallsMarked(
lines(
"/** @constructor A */ function A() {};",
"function f() {",
" var a = []; a[1] = 1; return a;",
"}",
"f()"),
ImmutableList.of("f"));

// TODO(bradfordcsmith): Remove NEITHER when type checker understands let/const
mode = TypeInferenceMode.NEITHER;
assertPureCallsMarked(
lines(
"/** @constructor A */ function A() {};",
"function f() {",
" const a = []; a[1] = 1; return a;",
"}",
"f()"),
ImmutableList.of("f"));
}

public void testLocalizedSideEffects8() throws Exception {
Expand Down Expand Up @@ -1034,10 +1064,19 @@ public void testLocalizedSideEffects11() throws Exception {
public void testLocalizedSideEffects12() throws Exception {
// An array is a local object, assigning a local array is not a global
// side-effect. This tests the behavior if the access is in a block scope.
String source = lines(
"function f() {var x = []; { x[0] = 1; } }",
"f()");
assertPureCallsMarked(source, ImmutableList.of("f"));
assertPureCallsMarked(
lines(
"function f() {var x = []; { x[0] = 1; } }", // preserve newline
"f()"),
ImmutableList.of("f"));

// TODO(bradfordcsmith): Remove NEITHER when type checker understands let/const
mode = TypeInferenceMode.NEITHER;
assertPureCallsMarked(
lines(
"function f() {const x = []; { x[0] = 1; } }", // preserve newline
"f()"),
ImmutableList.of("f"));
}

public void testLocalizedSideEffects13() {
Expand Down Expand Up @@ -1105,10 +1144,10 @@ public void testLocalizedSideEffects20() {
}

public void testLocalizedSideEffects21() {
// TODO(bradfordcsmith): Remove NEITHER when type checkers understand destructuring and
// let/const.
this.mode = TypeInferenceMode.NEITHER;
String source = lines(
"function f(values) { var x = {}; [x.y, x.z] = values; }",
"f()");
String source = lines("function f(values) { const x = {}; [x.y, x.z] = values; }", "f()");
assertPureCallsMarked(source, ImmutableList.of("f"));
}

Expand All @@ -1121,10 +1160,13 @@ public void testLocalizedSideEffects22() {
}

public void testLocalizedSideEffects23() {
// TODO(bradfordcsmith): Remove NEITHER when type checkers understand destructuring and
// let/const.
this.mode = TypeInferenceMode.NEITHER;
String source = lines(
"function f(values) { var x = {}; [x.y, x.z = defaultNoSideEffects] = values; }",
"f()");
String source =
lines(
"function f(values) { const x = {}; [x.y, x.z = defaultNoSideEffects] = values; }",
"f()");
assertPureCallsMarked(source, ImmutableList.of("f"));
}

Expand Down Expand Up @@ -1152,10 +1194,19 @@ public void testUnaryOperators2() throws Exception {
}

public void testUnaryOperators3() throws Exception {
String source = lines(
"function f() {var x = {foo : 0}; x.foo++}",
"f()");
assertPureCallsMarked(source, ImmutableList.of("f"));
assertPureCallsMarked(
lines(
"function f() {var x = {foo : 0}; x.foo++}", // preserve newline
"f()"),
ImmutableList.of("f"));

// TODO(bradfordcsmith): Remove NEITHER when type checker understands let/const
mode = TypeInferenceMode.NEITHER;
assertPureCallsMarked(
lines(
"function f() {const x = {foo : 0}; x.foo++}", // preserve newline
"f()"),
ImmutableList.of("f"));
}

public void testUnaryOperators4() throws Exception {
Expand All @@ -1167,10 +1218,19 @@ public void testUnaryOperators4() throws Exception {
}

public void testUnaryOperators5() throws Exception {
String source = lines(
"function f(x) {x.foo++}",
"f({foo : 0})");
assertPureCallsMarked(source, ImmutableList.of("f"));
assertPureCallsMarked(
lines(
"function f(x) {x.foo++}", // preserve newline
"f({foo : 0})"),
ImmutableList.of("f"));

// TODO(bradfordcsmith): Remove NEITHER when type checker understands destructured parameters
mode = TypeInferenceMode.NEITHER;
assertPureCallsMarked(
lines(
"function f({x}) {x.foo++}", // preserve newline
"f({x: {foo : 0}})"),
ImmutableList.of("f"));
}

public void testDeleteOperator1() throws Exception {
Expand Down Expand Up @@ -1450,7 +1510,7 @@ public void testAmbiguousDefinitionsMutatesGlobalArgument() throws Exception {
}

public void testAmbiguousDefinitionsMutatesLocalArgument() throws Exception {
String source =
assertPureCallsMarked(
lines(
"// Mutates argument",
"A.a = function(argument) {",
Expand All @@ -1461,8 +1521,24 @@ public void testAmbiguousDefinitionsMutatesLocalArgument() throws Exception {
"var b = function() {",
" C.a({});",
"};",
"b();");
assertPureCallsMarked(source, ImmutableList.of("C.a", "b"));
"b();"),
ImmutableList.of("C.a", "b"));

// TODO(bradfordcsmith): Remove NEITHER when type checker understands destructuring parameters
mode = TypeInferenceMode.NEITHER;
assertPureCallsMarked(
lines(
"// Mutates argument",
"A.a = function([argument]) {",
" argument.x = 2;",
"};",
"// No side effects",
"B.a = function() {};",
"var b = function() {",
" C.a([{}]);",
"};",
"b();"),
ImmutableList.of("C.a", "b"));
}

public void testAmbiguousExternDefinitions() {
Expand Down Expand Up @@ -1621,10 +1697,19 @@ public void testCallFunctionThatModifiesThis() throws Exception {
}

public void testMutatesArguments1() throws Exception {
String source = lines(
"function f(x) { x.y = 1; }",
"f({});");
assertPureCallsMarked(source, ImmutableList.of("f"));
assertPureCallsMarked(
lines(
"function f(x) { x.y = 1; }", // preserve newline
"f({});"),
ImmutableList.of("f"));

// TODO(bradfordcsmith): Remove NEITHER when type checker understands destructuring parameters
mode = TypeInferenceMode.NEITHER;
assertPureCallsMarked(
lines(
"function f([x]) { x.y = 1; }", // preserve newline
"f([{}]);"),
ImmutableList.of("f"));
}

public void testMutatesArguments2() throws Exception {
Expand All @@ -1644,11 +1729,21 @@ public void testMutatesArguments3() throws Exception {
}

public void testMutatesArguments4() throws Exception {
String source = lines(
"function f(x) { x.y = 1; }",
"function g(x) { f({}); x.y = 1; }",
"g({});");
assertPureCallsMarked(source, ImmutableList.of("f", "g"));
assertPureCallsMarked(
lines(
"function f(x) { x.y = 1; }", // preserve newline
"function g(x) { f({}); x.y = 1; }",
"g({});"),
ImmutableList.of("f", "g"));

// TODO(bradfordcsmith): Remove NEITHER when type checker understands destructuring parameters
mode = TypeInferenceMode.NEITHER;
assertPureCallsMarked(
lines(
"function f([x]) { x.y = 1; }", // preserve newline
"function g([x]) { f([{}]); x.y = 1; }",
"g([{}]);"),
ImmutableList.of("f", "g"));
}

public void testMutatesArguments5() throws Exception {
Expand Down

0 comments on commit 584aed0

Please sign in to comment.