From 06c5561825232fbf13a9f17a86c76d531fa2c149 Mon Sep 17 00:00:00 2001 From: johnplaisted Date: Mon, 23 Jul 2018 09:15:57 -0700 Subject: [PATCH] Inline require'd symbols when rewriting ES6 modules. This is needed as some later stages of the compiler specifically check for well-known Closure properties, such as goog.asserts.assert. So `const {assert} = goog.require('goog.asserts'); assert(0);` must be rewritten to `goog.asserts.assert(0);` to be recognized correctly later. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=205671584 --- .../javascript/jscomp/Es6RewriteModules.java | 82 +++++++++++++++++-- .../Es6RewriteModulesWithGoogInteropTest.java | 53 +++++++----- .../module_tests/import_star_test.js | 2 +- 3 files changed, 110 insertions(+), 27 deletions(-) diff --git a/src/com/google/javascript/jscomp/Es6RewriteModules.java b/src/com/google/javascript/jscomp/Es6RewriteModules.java index e34067ad6ab..9355dce6d54 100644 --- a/src/com/google/javascript/jscomp/Es6RewriteModules.java +++ b/src/com/google/javascript/jscomp/Es6RewriteModules.java @@ -103,6 +103,36 @@ public final class Es6RewriteModules extends AbstractPostOrderCallback */ private Map importMap; + /** + * Local variable names that were goog.require'd to qualified name we need to line. We need to + * inline all required names since there are certain well-known Closure symbols (like + * goog.asserts) that later stages of the compiler check for and cannot handle aliases. + * + *

We use this to rewrite something like: + * + * 

+   *   import {x} from '';
+   *   const {assert} = goog.require('goog.asserts');
+   *   assert(x);
+   *
+ * + * To: + * + * 
+   *   import {x} from '';
+   *   goog.asserts.assert(x);
+   *
+ * + * Because if we used an alias like below the assertion would not be recognized: + * + * 
+   *   import {x} from '';
+   *   const {assert} = goog.asserts;
+   *   assert(x);
+   *
+ */ + private Map namesToInlineByAlias; + private final Set importedGoogNames; private Set typedefs; @@ -179,6 +209,7 @@ public void clearState() { this.importMap = new HashMap<>(); this.importedGoogNames.clear(); this.typedefs = new HashSet<>(); + this.namesToInlineByAlias = new HashMap<>(); } /** @@ -586,12 +617,26 @@ private void rewriteRequires(Node script) { (NodeTraversal t, Node n, Node parent) -> { if (n.isCall()) { if (n.getFirstChild().matchesQualifiedName("goog.require")) { - visitRequire(t, n, parent, true /* checkScope */); + visitRequireOrGet(t, n, parent, /* isRequire= */ true); } else if (n.getFirstChild().matchesQualifiedName("goog.module.get")) { visitGoogModuleGet(t, n, parent); } } }); + NodeTraversal.traversePostOrder( + compiler, + script, + (NodeTraversal t, Node n, Node parent) -> { + if (n.isName() && namesToInlineByAlias.containsKey(n.getString())) { + Var v = t.getScope().getVar(n.getString()); + if (v == null || v.getNameNode() != n) { + Node replacement = + NodeUtil.newQName(compiler, namesToInlineByAlias.get(n.getString())); + replacement.useSourceInfoFromForTree(n); + n.replaceWith(replacement); + } + } + }); } private void visitGoogModuleGet(NodeTraversal t, Node getCall, Node parent) { @@ -606,17 +651,18 @@ private void visitGoogModuleGet(NodeTraversal t, Node getCall, Node parent) { return; } - visitRequire(t, getCall, parent, false /* checkScope */); + visitRequireOrGet(t, getCall, parent, /* isRequire= */ false); } - private void visitRequire(NodeTraversal t, Node requireCall, Node parent, boolean checkScope) { + private void visitRequireOrGet( + NodeTraversal t, Node requireCall, Node parent, boolean isRequire) { if (!requireCall.hasTwoChildren() || !requireCall.getLastChild().isString()) { t.report(requireCall, INVALID_REQUIRE_NAMESPACE); return; } // Module has already been turned into a script at this point. - if (checkScope && !t.getScope().isGlobal()) { + if (isRequire && !t.getScope().isGlobal()) { t.report(requireCall, INVALID_CLOSURE_CALL_ERROR); return; } @@ -637,9 +683,31 @@ private void visitRequire(NodeTraversal t, Node requireCall, Node parent, boolea } if (isStoredInDeclaration) { - Node replacement = - NodeUtil.newQName(compiler, m.getGlobalName(namespace)).srcrefTree(requireCall); - parent.replaceChild(requireCall, replacement); + if (isRequire) { + if (parent.isDestructuringLhs()) { + checkState(parent.getFirstChild().isObjectPattern()); + for (Node child : parent.getFirstChild().children()) { + if (child.isStringKey()) { + checkState(child.getFirstChild().isName()); + namesToInlineByAlias.put( + child.getFirstChild().getString(), + m.getGlobalName(namespace) + "." + child.getString()); + } else { + checkState(child.isName()); + namesToInlineByAlias.put( + child.getString(), m.getGlobalName(namespace) + "." + child.getString()); + } + } + } else { + checkState(parent.isName()); + namesToInlineByAlias.put(parent.getString(), m.getGlobalName(namespace)); + } + parent.getParent().detach(); + } else { + Node replacement = + NodeUtil.newQName(compiler, m.getGlobalName(namespace)).srcrefTree(requireCall); + parent.replaceChild(requireCall, replacement); + } } else { checkState(requireCall.getParent().isExprResult()); requireCall.getParent().detach(); diff --git a/test/com/google/javascript/jscomp/Es6RewriteModulesWithGoogInteropTest.java b/test/com/google/javascript/jscomp/Es6RewriteModulesWithGoogInteropTest.java index 45292e1baee..96f9983c187 100644 --- a/test/com/google/javascript/jscomp/Es6RewriteModulesWithGoogInteropTest.java +++ b/test/com/google/javascript/jscomp/Es6RewriteModulesWithGoogInteropTest.java @@ -117,17 +117,34 @@ public void testGoogModuleGetNonStringIsError() { public void testGoogRequireForProvide() { testModules( - "const y = goog.require('closure.provide');\nexport {};", + lines("const y = goog.require('closure.provide');", "use(y, y.x)", "export {};"), lines( - "const y$$module$testcode = closure.provide;", + "use(closure.provide, closure.provide.x);", + "/** @const */ var module$testcode = {};")); + } + + public void testGoogRequireForProvideWithDestructure() { + testModules( + lines("const {a, b:c} = goog.require('closure.provide');", "use(a, a.z, c)", "export {};"), + lines( + "use(closure.provide.a, closure.provide.a.z, closure.provide.b);", "/** @const */ var module$testcode = {};")); } public void testGoogRequireForGoogModule() { testModules( - "const y = goog.require('closure.module');\nexport {};", + lines("const y = goog.require('closure.module');", "use(y, y.x)", "export {};"), lines( - "const y$$module$testcode = module$exports$closure$module;", + "use(module$exports$closure$module, module$exports$closure$module.x);", + "/** @const */ var module$testcode = {};")); + } + + public void testGoogRequireForGoogModuleWithDestructure() { + testModules( + lines("const {a, b:c} = goog.require('closure.module');", "use(a, a.z, c)", "export {};"), + lines( + "use(module$exports$closure$module.a, module$exports$closure$module.a.z,", + " module$exports$closure$module.b);", "/** @const */ var module$testcode = {};")); } @@ -141,9 +158,20 @@ public void testGoogModuleGetForGoogModule() { public void testGoogRequireForLegacyGoogModule() { testModules( - "const y = goog.require('closure.legacy.module');\nexport {};", + lines("const y = goog.require('closure.legacy.module');", "use(y, y.x)", "export {};"), + lines( + "use(closure.legacy.module, closure.legacy.module.x);", + "/** @const */ var module$testcode = {};")); + } + + public void testGoogRequireForLegacyGoogModuleWithDestructure() { + testModules( + lines( + "const {a, b:c} = goog.require('closure.legacy.module');", + "use(a, a.z, c)", + "export {};"), lines( - "const y$$module$testcode = closure.legacy.module;", + "use(closure.legacy.module.a, closure.legacy.module.a.z, closure.legacy.module.b);", "/** @const */ var module$testcode = {};")); } @@ -195,19 +223,6 @@ public void testGoogRequireLhsNonConstIsError() { testModulesError( "export var x;\nvar bar = goog.require('closure.provide');", LHS_OF_GOOG_REQUIRE_MUST_BE_CONST); - } - - public void testGoogRequiresDestructuring_rewrite() { - testModules( - lines( - "export {};", "const {foo, bar} = goog.require('closure.provide');", "use(foo, bar);"), - lines( - "const {", - " foo: foo$$module$testcode,", - " bar: bar$$module$testcode,", - "} = closure.provide;", - "use(foo$$module$testcode, bar$$module$testcode);", - "/** @const */ var module$testcode = {};")); testModulesError( lines("export {};", "var {foo, bar} = goog.require('closure.provide');", "use(foo, bar);"), diff --git a/test/com/google/javascript/jscomp/runtime_tests/module_tests/import_star_test.js b/test/com/google/javascript/jscomp/runtime_tests/module_tests/import_star_test.js index 08e9b168936..a6b1dd38eb1 100644 --- a/test/com/google/javascript/jscomp/runtime_tests/module_tests/import_star_test.js +++ b/test/com/google/javascript/jscomp/runtime_tests/module_tests/import_star_test.js @@ -26,6 +26,6 @@ testSuite({ /** @suppress {checkTypes} */ testUnexportedProperty() { // Module-scoped variable, not exported with this name. - assertEquals(undefined, m.a); + assertEquals(undefined, m['a']); }, });