Skip to content

Commit

Permalink
Handle object destructuring patterns in RescopeGlobalVariables.
Browse files Browse the repository at this point in the history 
RescopeGlobalVariables doesn't correctly handle object property shorthand yet.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=177621248
  • Loading branch information
@lauraharker @blickly
lauraharker authored and blickly committed Dec 4, 2017
1 parent a500208 commit ab69fcb
Show file tree
Hide file tree
Showing 2 changed files with 163 additions and 24 deletions.
90 changes: 80 additions & 10 deletions src/com/google/javascript/jscomp/RescopeGlobalSymbols.java
View file
Expand Up @@ -271,7 +271,9 @@ public void visit(NodeTraversal t, Node n, Node parent) {
} else if (parent.isFunction()) {
value = parent;
}
if (value == null) {
if (value == null && !NodeUtil.isLhsByDestructuring(n)) {
// If n is assigned in a destructuring pattern, don't bother finding its value and just
// assume it may reference this.
return;
}
// We already added this symbol. Done after checks above because those
Expand All @@ -287,40 +289,107 @@ public void visit(NodeTraversal t, Node n, Node parent) {
// If anything but a function is assignment we assume that possibly
// a function referencing this is being assignment. Otherwise we
// check whether the function that is being assigned references this.
if (!value.isFunction() || NodeUtil.referencesThis(value)) {
if (value == null || !value.isFunction() || NodeUtil.referencesThis(value)) {
maybeReferencesThis.add(name);
}
}
}
}

/**
* Visits each NAME token and checks whether it refers to a global variable.
* If yes, rewrites the name to be a property access on the "globalSymbolNamespace".
* If the NAME is an extern variable, it becomes a property access on window.
* Visits each NAME token and checks whether it refers to a global variable. If yes, rewrites the
* name to be a property access on the "globalSymbolNamespace". If the NAME is an extern variable,
* it becomes a property access on window.
*
* <pre>var a = 1, b = 2, c = 3;</pre>
*
* becomes
*
* <pre>var NS.a = 1, NS.b = 2, NS.c = 4</pre>
*
* (The var token is removed in a later traversal.)
*
* <pre>a + b</pre>
*
* becomes
*
* <pre>NS.a + NS.b</pre>
*
* <pre>a()</pre>
*
* becomes
*
* <pre>(0,NS.a)()</pre>
*
* Notice the special syntax here to preserve the *this* semantics in the function call.
*
* <pre>var {a: b} = {}</pre>
*
* becomes
*
* <pre>var {a: NS.b} = {}</pre>
*
* (This is invalid syntax, but the VAR token is removed later).
*/
private class RewriteScopeCallback extends AbstractPostOrderCallback {
List<ModuleGlobal> preDeclarations = new ArrayList<>();

@Override
public void visit(NodeTraversal t, Node n, Node parent) {
if (!n.isName()) {
if (n.isName() && !NodeUtil.isLhsByDestructuring(n)) {
visitName(t, n, parent);
} else if (n.isDestructuringPattern()) {
visitDestructuringPattern(t, n, parent);
}
}

private void visitDestructuringPattern(NodeTraversal t, Node n, Node parent) {
if (!(parent.isAssign() || parent.isParamList() || parent.isDestructuringLhs())) {
// Don't handle patterns that are nested within another pattern.
// TODO(b/69868034): What about for loop initializers or other assignments?
return;
}
List<Node> lhsNodes = NodeUtil.findLhsNodesInNode(n.getParent());
boolean isDeclaration = n.getParent().isDestructuringLhs();
boolean hasCrossModuleName = false;
for (Node lhs : lhsNodes) {
if (!lhs.isName()) {
continue;
}
visitName(t, lhs, lhs.getParent());
hasCrossModuleName = hasCrossModuleName || isCrossModuleName(lhs.getString());
}

// If this is a global cross-module declaration, we need to wrap it in an ASSIGN.
// The LET/CONST/VAR node gets removed in RemoveGlobalVarCallback.
Node nameDeclaration = parent.getParent();
if (isDeclaration
&& hasCrossModuleName
&& (t.inGlobalScope() || (nameDeclaration.isVar() && t.inGlobalHoistScope()))) {
Node value = n.getNext();
parent.removeChild(n);
parent.removeChild(value);
Node assign = IR.assign(n, value).srcref(n);
nameDeclaration.replaceChild(parent, assign);
compiler.reportChangeToEnclosingScope(nameDeclaration);

// If there are any declared names that are not cross module, they need to be declared
// before the destructuring pattern, since we converted their declaration to an assignment.
CompilerInput input = t.getInput();
for (Node lhs : lhsNodes) {
if (!lhs.isName()) {
continue;
}
String name = lhs.getString();
if (!isCrossModuleName(name)) {
preDeclarations.add(
new ModuleGlobal(input.getAstRoot(compiler), IR.name(name).srcref(lhs)));
}
}
}
}

private void visitName(NodeTraversal t, Node n, Node parent) {
String name = n.getString();
// Ignore anonymous functions
if (parent.isFunction() && name.isEmpty()) {
Expand Down Expand Up @@ -359,6 +428,7 @@ private void replaceSymbol(NodeTraversal t, Node node, String name, CompilerInpu
if (!isCrossModule) {
// When a non cross module name appears outside a var declaration we
// never have to do anything.
// If it's inside a destructuring pattern declaration, then it's handled elsewhere.
if (!NodeUtil.isNameDeclaration(parent)) {
return;
}
Expand Down Expand Up @@ -497,10 +567,10 @@ public void visit(NodeTraversal t, Node n, Node parent) {
// As opposed to regular var nodes, there are always assignments
// because the previous traversal in RewriteScopeCallback creates
// them.
boolean allName = true;
boolean allNameOrDestructuring = true;
for (Node c : n.children()) {
if (!c.isName()) {
allName = false;
if (!c.isName() && !c.isDestructuringLhs()) {
allNameOrDestructuring = false;
}
if (c.isAssign() || NodeUtil.isAnyFor(parent)) {
interestingChildren.add(c);
Expand All @@ -509,7 +579,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
// If every child of a var declares a name, it must stay in place.
// This is the case if none of the declared variables cross module
// boundaries.
if (allName) {
if (allNameOrDestructuring) {
return;
}
for (Node c : interestingChildren) {
Expand Down
97 changes: 83 additions & 14 deletions test/com/google/javascript/jscomp/RescopeGlobalSymbolsTest.java
View file
Expand Up @@ -185,7 +185,7 @@ public void testConstDeclarations_export() {
test("const _dumpException = 1;", "_._dumpException = 1");
}

public void testconstDeclarations_acrossModules() {
public void testConstDeclarations_acrossModules() {
assumeCrossModuleNames = false;
// test references across modules.
test(createModules("const a = 1;", "a"), new String[] {"_.a = 1", "_.a"});
Expand All @@ -208,6 +208,57 @@ public void testconstDeclarations_acrossModules() {
new String[] {"var a, c; 1;a = 1; _.b = 1;c = 2", "_.b; if (true) { const b = 3; b; }"});
}

public void testObjectDestructuringDeclarations() {
// TODO(b/69868034): Fix commented-out tests before marking this pass ES6-compatible.
// This pass is not guaranteed to be normalized so it must handle the object literal shorthand.
// test("var {a} = {}; a;", "({a: _.a} = {}); _.a;");
test("var {key: a} = {}; a;", "({key: _.a} = {}); _.a;");
// test("var {a: {b}} = {}; b;", "({a: {b: _.b}} = {}); _.b;");
test("var {a: {key: b}} = {}; b;", "({a: {key: _.b}} = {}); _.b;");
test("var {['computed']: a} = {}; a;", "({['computed']: _.a} = {}); _.a;");

// test("var {a = 3} = {}; a;", "({a: _.a = 3} = {}); _.a");
test("var {key: a = 3} = {}; a;", "({key: _.a = 3} = {}); _.a");
}

public void testObjectDestructuringDeclarations_allSameModule() {
assumeCrossModuleNames = false;
// TODO(b/69868034): Fix commented-out test before marking this pass ES6-compatible.
// test("var {a} = {}; a;", "var {a: a} = {}; a;");
testSame("var {a: a} = {}; a;");
testSame("var {key: a} = {}; a;");
testSame("var {a: {b: b}} = {}; b;");
testSame("var {['computed']: a} = {}; a;");
testSame("var {a: a = 3} = {}; a;");
testSame("var {key: a = 3} = {}; a;");
testSame("var {['computed']: a = 3} = {}; a;");
}

public void testObjectDestructuringDeclarations_acrossModules() {
assumeCrossModuleNames = false;
test(createModules("var {a: a} = {};", "a"), new String[] {"({a: _.a} = {});", "_.a"});
test(
createModules("var {a: a, b: b, c: c} = {};", "a; c;"),
new String[] {"var b;({a: _.a, b: b, c: _.c} = {});", "_.a; _.c"});
test(
createModules("var {a: a, b: b, c: c} = {};", "b; c;"),
new String[] {"var a;({a: a, b: _.b, c: _.c} = {});", "_.b; _.c"});
test(
createModules("var {a: a, b: b, c: c} = {}; b; c;", "a; c;"),
new String[] {"var b;({a: _.a, b: b, c: _.c} = {});b;_.c;", "_.a; _.c"});
}

public void testObjectDestructuringAssignments() {
test("var a, b; ({key1: a, key2: b} = {}); a; b;", "({key1: _.a, key2: _.b} = {}); _.a; _.b;");
// Test a destructuring assignment with mixed global and local variables.
test(
"var a; if (true) { let b; ({key1: a, key2: b} = {}); b; } a;",
"if (true) { let b; ({key1: _.a, key2: b} = {}); b; } _.a;");
test("var obj = {}; ({a: obj.a} = {}); obj.a;", "_.obj = {}; ({a: _.obj.a} = {}); _.obj.a;");
}

// TODO(b/69868034): Test array patterns.

public void testForLoops() {
assumeCrossModuleNames = false;
test(createModules(
Expand All @@ -217,6 +268,7 @@ public void testForLoops() {
"for (var i = 0, c = 2; i < 1000; i++);", "i"),
new String[] {"var c;for (_.i = 0, c = 2; _.i < 1000; _.i++);",
"_.i"});
// TODO(b/69868034): Test destructuring in for loops..
}

public void testForLoops_acrossModules() {
Expand Down Expand Up @@ -266,6 +318,8 @@ public void testFunctionStatements_freeCallSemantics1() throws Exception {
test(
"var a=function(){this};a()",
"_.a=function(){this};(0,_.a)()");
// Always trigger free calls for variables assigned through destructuring.
test("var {a: a} = {a: function() {}}; a();", "({a:_.a}={a:function(){}});(0,_.a)()");
}

public void testFunctionStatements_freeCallSemantics2() {
Expand All @@ -284,10 +338,8 @@ public void testFunctionStatements_freeCallSemantics2() {
public void testFunctionStatements_freeCallSemantics3() {
disableCompareAsTree();

// Ambigious cases.
test(
"var a=1;a=function(){};a()",
"_.a=1;_.a=function(){};(0,_.a)()");
// Ambiguous cases.
test("var a=1;a=function(){};a()", "_.a=1;_.a=function(){};(0,_.a)()");
test(
"var b;var a=b;a()",
"_.a=_.b;(0,_.a)()");
Expand Down Expand Up @@ -317,18 +369,23 @@ public void testTryCatch() {
"try{_.a = 1}catch(e){throw e}");
}

public void testShadow() {
public void testShadowInFunctionScope() {
test(
"var _ = 1; (function () { _ = 2 })()",
"_._ = 1; (function () { _._ = 2 })()");
test(
"function foo() { var _ = {}; _.foo = foo; _.bar = 1; }",
"_.foo = function () { var _$ = {}; _$.foo = _.foo; _$.bar = 1}");
test(
"function foo() { var _ = {}; _.foo = foo; _.bar = 1; "
+ "(function() { var _ = 0;})() }",
"_.foo = function () { var _$ = {}; _$.foo = _.foo; _$.bar = 1; "
+ "(function() { var _$ = 0;})() }");
"function foo() { var {key: _} = {}; _.foo = foo; _.bar = 1; }",
"_.foo = function () { var {key: _$} = {}; _$.foo = _.foo; _$.bar = 1}");
test(
lines(
"function foo() { var _ = {}; _.foo = foo; _.bar = 1; ",
"(function() { var _ = 0;})() }"),
lines(
"_.foo = function () { var _$ = {}; _$.foo = _.foo; _$.bar = 1; ",
"(function() { var _$ = 0;})() }"));
test(
"function foo() { var _ = {}; _.foo = foo; _.bar = 1; "
+ "var _$ = 1; }",
Expand All @@ -347,13 +404,25 @@ public void testShadow() {
+ "var _$$ = 1, _$$$ = 2 (function() { _$ = _$$ = _$$$; "
+ "var _$$, _$$$$ })() }");
test(
"function foo() { var _a = 1;}",
"_.foo = function () { var _a = 1;}");
"var a = 5; function foo(_) { return _; }", "_.a = 5; _.foo = function(_$) { return _$; }");
test(
"var a = 5; function foo({key: _}) { return _; }",
"_.a = 5; _.foo = function({key: _$}) { return _$; }");
// We accept this unnecessary renaming as acceptable to simplify pattern
// matching in the traversal.
test("function foo() { var _$a = 1;}", "_.foo = function () { var _$a$ = 1;}");
}

public void testShadowInBlockScope() {
test(
"var foo = 1; if (true) { const _ = {}; _.foo = foo; _.bar = 1; }",
"_.foo = 1; if (true) { const _$ = {}; _$.foo = _.foo; _$.bar = 1}");
test(
"var foo = 1; if (true) { const {key: _} = {}; _.foo = foo; _.bar = 1; }",
"_.foo = 1; if (true) { const {key: _$} = {}; _$.foo = _.foo; _$.bar = 1}");
test(
"function foo() { var _$a = 1;}",
"_.foo = function () { var _$a$ = 1;}");
"var foo = 1; if (true) { const _ = {}; _.foo = foo; _.bar = 1; const _$ = 1; }",
"_.foo = 1; if (true) { const _$ = {}; _$.foo = _.foo; _$.bar = 1; const _$$ = 1; }");
}

public void testExterns() {
Expand Down

0 comments on commit ab69fcb

Please sign in to comment.