diff --git a/src/com/google/javascript/jscomp/RescopeGlobalSymbols.java b/src/com/google/javascript/jscomp/RescopeGlobalSymbols.java index 3f1414ebe9a..169d49d5814 100644 --- a/src/com/google/javascript/jscomp/RescopeGlobalSymbols.java +++ b/src/com/google/javascript/jscomp/RescopeGlobalSymbols.java @@ -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 @@ -287,7 +289,7 @@ 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); } } @@ -295,32 +297,99 @@ public void visit(NodeTraversal t, Node n, Node parent) { } /** - * 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. * *
var a = 1, b = 2, c = 3;
+ * * becomes + * *
var NS.a = 1, NS.b = 2, NS.c = 4
+ * * (The var token is removed in a later traversal.) * *
a + b
+ * * becomes + * *
NS.a + NS.b
* *
a()
+ * * becomes + * *
(0,NS.a)()
+ * * Notice the special syntax here to preserve the *this* semantics in the function call. + * + *
var {a: b} = {}
+ * + * becomes + * + *
var {a: NS.b} = {}
+ * + * (This is invalid syntax, but the VAR token is removed later). */ private class RewriteScopeCallback extends AbstractPostOrderCallback { List 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 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()) { @@ -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; } @@ -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); @@ -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) { diff --git a/test/com/google/javascript/jscomp/RescopeGlobalSymbolsTest.java b/test/com/google/javascript/jscomp/RescopeGlobalSymbolsTest.java index 91427908839..6cf0b1fa954 100644 --- a/test/com/google/javascript/jscomp/RescopeGlobalSymbolsTest.java +++ b/test/com/google/javascript/jscomp/RescopeGlobalSymbolsTest.java @@ -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"}); @@ -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( @@ -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() { @@ -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() { @@ -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)()"); @@ -317,7 +369,7 @@ public void testTryCatch() { "try{_.a = 1}catch(e){throw e}"); } - public void testShadow() { + public void testShadowInFunctionScope() { test( "var _ = 1; (function () { _ = 2 })()", "_._ = 1; (function () { _._ = 2 })()"); @@ -325,10 +377,15 @@ public void testShadow() { "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; }", @@ -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() {