From 13f31d23f400887b18b373fed6480bff2ef9d4a0 Mon Sep 17 00:00:00 2001 From: blickly Date: Wed, 25 May 2016 13:03:26 -0700 Subject: [PATCH] Fully support destructuring imports inside goog.module. Previously, we were allowing it, but not fully typechecking it in JSDoc and for checks based on qualified-names like goog.asserts.assert. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=123243993 --- .../jscomp/ClosureRewriteModule.java | 117 +++++++++++------- .../jscomp/ClosureRewriteModuleTest.java | 109 +++++++++++----- 2 files changed, 144 insertions(+), 82 deletions(-) diff --git a/src/com/google/javascript/jscomp/ClosureRewriteModule.java b/src/com/google/javascript/jscomp/ClosureRewriteModule.java index 28dfd6d5c2e..b9f7a46d2d9 100644 --- a/src/com/google/javascript/jscomp/ClosureRewriteModule.java +++ b/src/com/google/javascript/jscomp/ClosureRewriteModule.java @@ -218,7 +218,7 @@ private final class ScriptDescription { final Set topLevelNames = new HashSet<>(); // For prefixed content renaming. final Deque childScripts = new LinkedList<>(); // For goog.loadModule() final Set googModuleGettedNamespaces = new HashSet<>(); // {"some.goog.module", ...} - final Map legacyNamespacesByAlias = new HashMap<>(); // For alias inlining. + final Map importedNamesByAlias = new HashMap<>(); // For alias inlining. /** * Transient state. @@ -343,12 +343,6 @@ public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { } break; - case Token.NAME: - maybeUpdateTopLevelName(t, n); - maybeUpdateExportDeclaration(t, n); - maybeUpdateExportNameRef(n); - break; - case Token.RETURN: if (isTopLevel(t, n, ScopeType.EXEC_CONTEXT)) { updateModuleReturn(n); @@ -369,6 +363,12 @@ public void visit(NodeTraversal t, Node n, Node parent) { case Token.SCRIPT: updateEndScript(); break; + + case Token.NAME: + maybeUpdateTopLevelName(t, n); + maybeUpdateExportDeclaration(t, n); + maybeUpdateExportNameRef(n); + break; } } } @@ -419,10 +419,9 @@ public void visit(Node typeRefNode) { // "{module$exports$bar$Foo}" or // "{bar.Foo}" boolean nameIsAnAlias = - currentScript.legacyNamespacesByAlias.containsKey(prefixTypeName); + currentScript.importedNamesByAlias.containsKey(prefixTypeName); if (nameIsAnAlias) { - String legacyNamespace = currentScript.legacyNamespacesByAlias.get(prefixTypeName); - String aliasedNamespace = rewriteState.getExportedNamespace(legacyNamespace); + String aliasedNamespace = currentScript.importedNamesByAlias.get(prefixTypeName); safeSetString(typeRefNode, aliasedNamespace + suffix); return; } @@ -491,9 +490,15 @@ boolean isLegacyModule(String legacyNamespace) { return script == null ? null : script.getBinaryNamespace(); } - String getExportedNamespace(String legacyNamespace) { - String binaryNamespace = getBinaryNamespace(legacyNamespace); - return binaryNamespace != null ? binaryNamespace : legacyNamespace; + @Nullable String getExportedNamespace(String legacyNamespace) { + if (legacyScriptNamespaces.contains(legacyNamespace)) { + return legacyNamespace; + } + ScriptDescription script = scriptDescriptionsByGoogModuleNamespace.get(legacyNamespace); + if (script != null) { + return script.declareLegacyNamespace ? legacyNamespace : script.getBinaryNamespace(); + } + return null; } void removeRoot(Node toRemove) { @@ -637,27 +642,12 @@ private void recordGoogRequire(NodeTraversal t, Node call, boolean reportBadRequ maybeSplitMultiVar(call); Node legacyNamespaceNode = call.getLastChild(); - Node statementNode = NodeUtil.getEnclosingStatement(call); if (!legacyNamespaceNode.isString()) { t.report(legacyNamespaceNode, INVALID_REQUIRE_NAMESPACE); return; } String legacyNamespace = legacyNamespaceNode.getString(); - // If the current script is a module or the require statement has a return value that is stored - // in an alias then the require is goog.module() style. - boolean currentScriptIsAModule = currentScript.isModule; - // "var Foo = goog.require("bar.Foo");" style. - boolean requireStoredInSimpleAlias = - call.getParent().isName() && NodeUtil.isNameDeclaration(call.getGrandparent()); - if (currentScriptIsAModule - && requireStoredInSimpleAlias - && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) { - // Record alias -> legacyNamespace associations for possible later inlining. - String aliasName = statementNode.getFirstChild().getString(); - recordImportAlias(aliasName, legacyNamespace); - } - // Maybe report an error if there is an attempt to import something that is expected to be a // goog.module() but no such goog.module() has been defined. boolean targetIsAModule = rewriteState.containsModule(legacyNamespace); @@ -826,7 +816,7 @@ private void updateGoogRequire(NodeTraversal t, Node call) { && !rewriteState.isLegacyModule(legacyNamespace); boolean targetIsGoogModuleGetted = currentScript.googModuleGettedNamespaces.contains(legacyNamespaceNode.getString()); - boolean importHasAlias = currentScript.legacyNamespacesByAlias.containsValue(legacyNamespace); + boolean importHasAlias = NodeUtil.isNameDeclaration(statementNode); boolean isDestructuring = statementNode.getFirstChild().isDestructuringLhs(); // Is "(.*)goog.require("bar.Foo").Foo;" style?. boolean immediatePropertyAccess = @@ -834,19 +824,41 @@ private void updateGoogRequire(NodeTraversal t, Node call) { && call.getGrandparent().isName() && NodeUtil.isNameDeclaration(call.getGrandparent().getParent()); + // If the current script is a module or the require statement has a return value that is stored + // in an alias then the require is goog.module() style. + boolean currentScriptIsAModule = currentScript.isModule; + // "var Foo = goog.require("bar.Foo");" or "const {Foo} = goog.require('bar');" style. + boolean requireDirectlyStoredInAlias = NodeUtil.isNameDeclaration(call.getGrandparent()); + if (currentScriptIsAModule + && requireDirectlyStoredInAlias + && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) { + // Record alias -> exportedNamespace associations for later inlining. + Node lhs = call.getParent(); + String exportedNamespace = rewriteState.getExportedNamespace(legacyNamespace); + if (exportedNamespace == null) { + // There's nothing to inline. The missing provide/module will be reported elsewhere. + } else if (lhs.isName()) { + // `var Foo` case + String aliasName = statementNode.getFirstChild().getString(); + recordImportAlias(aliasName, exportedNamespace); + } else if (lhs.isDestructuringLhs() && lhs.getFirstChild().isObjectPattern()) { + // `const {Foo}` case + for (Node importSpec : lhs.getFirstChild().children()) { + String importedProperty = importSpec.getString(); + String aliasName = + importSpec.hasChildren() ? importSpec.getFirstChild().getString() : importedProperty; + String fullName = exportedNamespace + "." + importedProperty; + recordImportAlias(aliasName, fullName); + } + } else { + throw new RuntimeException("Illegal goog.module import: " + lhs); + } + } + if (currentScript.isModule || (targetIsNonLegacyGoogModule && targetIsGoogModuleGetted)) { if (isDestructuring) { - // Rewrite - // "var {a, b} = goog.require('foo.Bar');" to - // "var {a, b} = module$exports$foo$Bar;" or - // "var {a, b} = foo.Bar;" - String exportedNamespace = rewriteState.getExportedNamespace(legacyNamespace); - Node replacementNamespaceName = NodeUtil.newQName(compiler, exportedNamespace); - replacementNamespaceName.putProp(Node.ORIGINALNAME_PROP, legacyNamespace); - replacementNamespaceName.putBooleanProp(Node.GOOG_MODULE_REQUIRE, true); - replacementNamespaceName.srcrefTree(call); - call.getParent().replaceChild(call, replacementNamespaceName); - markConst(statementNode); + // Delete the goog.require() because we're going to inline its alias later. + statementNode.detachFromParent(); } else if (targetIsNonLegacyGoogModule) { if (immediatePropertyAccess || !isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) { // Rewrite @@ -917,9 +929,8 @@ private void updateGoogModuleGetCall(Node call) { // In a non-module script goog.module.get() is used inside of a goog.scope() and it's // imported alias need only be made available within that scope. // Replace "goog.module.get('pkg.Foo');" with "module$exports$pkg$Foo;". - Node exportedNamespaceName = - NodeUtil.newQName(compiler, rewriteState.getExportedNamespace(legacyNamespace)) - .srcrefTree(call); + String exportedNamespace = rewriteState.getExportedNamespace(legacyNamespace); + Node exportedNamespaceName = NodeUtil.newQName(compiler, exportedNamespace).srcrefTree(call); exportedNamespaceName.putProp(Node.ORIGINALNAME_PROP, legacyNamespace); call.getParent().replaceChild(call, exportedNamespaceName); } @@ -970,12 +981,21 @@ private void maybeUpdateTopLevelName(NodeTraversal t, Node nameNode) { return; } + // If the name is part of a destructuring import, the import rewriting will take care of it + if (var.getNameNode() == nameNode + && nameNode.getParent().isStringKey() + && nameNode.getGrandparent().isObjectPattern()) { + Node destructuringLhsNode = nameNode.getGrandparent().getParent(); + if (isCallTo(destructuringLhsNode.getLastChild(), "goog.require")) { + return; + } + } + // If the name is an alias for an imported namespace rewrite from // "new Foo;" to "new module$exports$Foo;" - boolean nameIsAnAlias = currentScript.legacyNamespacesByAlias.containsKey(name); + boolean nameIsAnAlias = currentScript.importedNamesByAlias.containsKey(name); if (nameIsAnAlias && var.getNode() != nameNode) { - String legacyNamespace = currentScript.legacyNamespacesByAlias.get(name); - String namespaceToInline = rewriteState.getExportedNamespace(legacyNamespace); + String namespaceToInline = currentScript.importedNamesByAlias.get(name); safeSetMaybeQualifiedString(nameNode, namespaceToInline); // Make sure this action won't shadow a local variable. @@ -987,7 +1007,7 @@ private void maybeUpdateTopLevelName(NodeTraversal t, Node nameNode) { shadowedVar.getNode(), IMPORT_INLINING_SHADOWS_VAR, shadowedVar.getName(), - legacyNamespace); + namespaceToInline); } } return; @@ -1260,7 +1280,8 @@ private void markConstAndCopyJsDoc(Node from, Node target, Node value) { } private void recordImportAlias(String aliasName, String legacyNamespace) { - currentScript.legacyNamespacesByAlias.put(aliasName, legacyNamespace); + Preconditions.checkNotNull(legacyNamespace); + currentScript.importedNamesByAlias.put(aliasName, legacyNamespace); } /** diff --git a/test/com/google/javascript/jscomp/ClosureRewriteModuleTest.java b/test/com/google/javascript/jscomp/ClosureRewriteModuleTest.java index 3a69dfb6ca9..45a7f4a2924 100644 --- a/test/com/google/javascript/jscomp/ClosureRewriteModuleTest.java +++ b/test/com/google/javascript/jscomp/ClosureRewriteModuleTest.java @@ -123,48 +123,88 @@ public void testDestructuringInsideModule() { } public void testDestructuringImports() { - // TODO(blickly): Inline destructuring-based imports so that they can be used - // for importing type names like other imports. - - // Var destructuring of both module and script goog.require() targets. testEs6( new String[] { - "goog.module('ns.b');", - "goog.provide('ns.c');", - LINE_JOINER.join( - "goog.module('ns.a');", - "var {foo, bar} = goog.require('ns.b');", - "var {baz, qux} = goog.require('ns.c');")}, + "goog.module('ns.b'); /** @constructor */ exports.Foo = function() {};", + LINE_JOINER.join( + "goog.module('ns.a');", + "", + "var {Foo} = goog.require('ns.b');", + "", + "/** @type {Foo} */", + "var f = new Foo;") + }, + new String[] { + LINE_JOINER.join( + "/** @const */ var module$exports$ns$b = {};", + "/** @constructor */ module$exports$ns$b.Foo = function() {};"), + LINE_JOINER.join( + "/** @const */ var module$exports$ns$a = {}", + "/** @type {module$exports$ns$b.Foo} */", + "var module$contents$ns$a_f = new module$exports$ns$b.Foo;")}); + testEs6( new String[] { - "/** @const */ var module$exports$ns$b = {}", - "goog.provide('ns.c');", - LINE_JOINER.join( - "/** @const */ var module$exports$ns$a = {}", - "/** @const */ var {foo: module$contents$ns$a_foo,", - " bar: module$contents$ns$a_bar} = module$exports$ns$b;", - "/** @const */ var {baz: module$contents$ns$a_baz,", - " qux: module$contents$ns$a_qux} = ns.c;")}); + "goog.provide('ns.b'); /** @constructor */ ns.b.Foo = function() {};", + LINE_JOINER.join( + "goog.module('ns.a');", + "", + "var {Foo} = goog.require('ns.b');", + "", + "/** @type {Foo} */", + "var f = new Foo;") + }, + new String[] { + "goog.provide('ns.b'); /** @constructor */ ns.b.Foo = function() {};", + LINE_JOINER.join( + "/** @const */ var module$exports$ns$a = {}", + "/** @type {ns.b.Foo} */", + "var module$contents$ns$a_f = new ns.b.Foo;")}); - // Const destructuring of both module and script goog.require() targets. testEs6( new String[] { - "goog.module('ns.b');", - "goog.provide('ns.c');", - LINE_JOINER.join( - "goog.module('ns.a');", - "const {foo, bar} = goog.require('ns.b');", - "const {baz, qux} = goog.require('ns.c');")}, + LINE_JOINER.join( + "goog.module('ns.b');", + "goog.module.declareLegacyNamespace();", + "", + "/** @constructor */ exports.Foo = function() {};"), + LINE_JOINER.join( + "goog.module('ns.a');", + "", + "var {Foo} = goog.require('ns.b');", + "", + "/** @type {Foo} */", + "var f = new Foo;") + }, + new String[] { + LINE_JOINER.join( + "goog.provide('ns.b');", + "/** @constructor */ ns.b.Foo = function() {};"), + LINE_JOINER.join( + "/** @const */ var module$exports$ns$a = {}", + "/** @type {ns.b.Foo} */", + "var module$contents$ns$a_f = new ns.b.Foo;")}); + testEs6( new String[] { - "/** @const */ var module$exports$ns$b = {}", - "goog.provide('ns.c');", - LINE_JOINER.join( - "/** @const */ var module$exports$ns$a = {}", - "/** @const */ const {foo: module$contents$ns$a_foo,", - " bar: module$contents$ns$a_bar} = module$exports$ns$b;", - "/** @const */ const {baz: module$contents$ns$a_baz,", - " qux: module$contents$ns$a_qux} = ns.c;")}); + "goog.module('ns.b'); /** @constructor */ exports.Foo = function() {};", + LINE_JOINER.join( + "goog.module('ns.a');", + "", + "var {Foo: Bar} = goog.require('ns.b');", + "", + "/** @type {Bar} */", + "var f = new Bar;") + }, + new String[] { + LINE_JOINER.join( + "/** @const */ var module$exports$ns$b = {};", + "/** @constructor */ module$exports$ns$b.Foo = function() {};"), + LINE_JOINER.join( + "/** @const */ var module$exports$ns$a = {}", + "/** @type {module$exports$ns$b.Foo} */", + "var module$contents$ns$a_f = new module$exports$ns$b.Foo;")}); + } public void testDeclareLegacyNamespace() { @@ -1285,7 +1325,8 @@ public void testGoogModuleReferencedWithGlobalName() { public void testGoogModuleValidReferences() { test( new String[] { - "goog.module('a.b.c');", "goog.module('x.y.z'); var c = goog.require('a.b.c'); use(c);" + "goog.module('a.b.c');", + "goog.module('x.y.z'); var c = goog.require('a.b.c'); use(c);" }, new String[] { "/** @const */ var module$exports$a$b$c={};",