Skip to content

Commit

Permalink
Handle goog.requireType correctly in Es6RewriteModules.
Browse files Browse the repository at this point in the history 
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=215422628
  • Loading branch information
@tjgq @lauraharker
tjgq authored and lauraharker committed Oct 2, 2018
1 parent 43616e7 commit eb521a4
Show file tree
Hide file tree
Showing 6 changed files with 436 additions and 54 deletions.
5 changes: 5 additions & 0 deletions src/com/google/javascript/jscomp/ClosurePrimitiveErrors.java
View file
Expand Up @@ -46,6 +46,11 @@ private ClosurePrimitiveErrors() {}
"JSC_GOOG_MODULE_INVALID_REQUIRE_NAMESPACE", "JSC_GOOG_MODULE_INVALID_REQUIRE_NAMESPACE",
"goog.require parameter must be a string literal."); "goog.require parameter must be a string literal.");


static final DiagnosticType INVALID_REQUIRE_TYPE_NAMESPACE =
DiagnosticType.error(
"JSC_GOOG_MODULE_INVALID_REQUIRE_TYPE_NAMESPACE",
"goog.requireType parameter must be a string literal.");

static final DiagnosticType MISSING_MODULE_OR_PROVIDE = static final DiagnosticType MISSING_MODULE_OR_PROVIDE =
DiagnosticType.error( DiagnosticType.error(
"JSC_MISSING_MODULE_OR_PROVIDE", "Required namespace \"{0}\" never defined."); "JSC_MISSING_MODULE_OR_PROVIDE", "Required namespace \"{0}\" never defined.");
Expand Down
20 changes: 18 additions & 2 deletions src/com/google/javascript/jscomp/ClosureRewriteModule.java
View file
Expand Up @@ -22,6 +22,7 @@
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_GET_CALL_SCOPE; import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_GET_CALL_SCOPE;
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_GET_NAMESPACE; import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_GET_NAMESPACE;
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_REQUIRE_NAMESPACE; import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_REQUIRE_NAMESPACE;
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_REQUIRE_TYPE_NAMESPACE;
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.MISSING_MODULE_OR_PROVIDE; import static com.google.javascript.jscomp.ClosurePrimitiveErrors.MISSING_MODULE_OR_PROVIDE;


import com.google.common.base.Joiner; import com.google.common.base.Joiner;
Expand Down Expand Up @@ -376,7 +377,7 @@ public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) {
} else if (method.matchesQualifiedName(GOOG_REQUIRE)) { } else if (method.matchesQualifiedName(GOOG_REQUIRE)) {
recordGoogRequire(t, n, /* mustBeOrdered= */ true); recordGoogRequire(t, n, /* mustBeOrdered= */ true);
} else if (method.matchesQualifiedName(GOOG_REQUIRETYPE)) { } else if (method.matchesQualifiedName(GOOG_REQUIRETYPE)) {
recordGoogRequire(t, n, /* mustBeOrdered= */ false); recordGoogRequireType(t, n);
} else if (method.matchesQualifiedName(GOOG_FORWARDDECLARE) && !parent.isExprResult()) { } else if (method.matchesQualifiedName(GOOG_FORWARDDECLARE) && !parent.isExprResult()) {
recordGoogForwardDeclare(t, n); recordGoogForwardDeclare(t, n);
} else if (method.matchesQualifiedName(GOOG_MODULE_GET)) { } else if (method.matchesQualifiedName(GOOG_MODULE_GET)) {
Expand Down Expand Up @@ -864,6 +865,21 @@ private void recordGoogRequire(NodeTraversal t, Node call, boolean mustBeOrdered
} }
} }


private void recordGoogRequireType(NodeTraversal t, Node call) {
Node legacyNamespaceNode = call.getLastChild();
if (!legacyNamespaceNode.isString()) {
t.report(legacyNamespaceNode, INVALID_REQUIRE_TYPE_NAMESPACE);
return;
}

// A goog.requireType call is not required to appear after the corresponding namespace
// definition.
boolean mustBeOrdered = false;

// For purposes of import collection, goog.requireType is the same as goog.require.
recordGoogRequire(t, call, mustBeOrdered);
}

private void recordGoogForwardDeclare(NodeTraversal t, Node call) { private void recordGoogForwardDeclare(NodeTraversal t, Node call) {
Node namespaceNode = call.getLastChild(); Node namespaceNode = call.getLastChild();
if (!call.hasTwoChildren() || !namespaceNode.isString()) { if (!call.hasTwoChildren() || !namespaceNode.isString()) {
Expand All @@ -876,7 +892,7 @@ private void recordGoogForwardDeclare(NodeTraversal t, Node call) {
// goog.module.get(). To avoid reporting the error twice suppress it here. // goog.module.get(). To avoid reporting the error twice suppress it here.
boolean mustBeOrdered = false; boolean mustBeOrdered = false;


// For purposes of import collection goog.forwardDeclare is the same as goog.require; // For purposes of import collection, goog.forwardDeclare is the same as goog.require.
recordGoogRequire(t, call, mustBeOrdered); recordGoogRequire(t, call, mustBeOrdered);
} }


Expand Down
10 changes: 6 additions & 4 deletions src/com/google/javascript/jscomp/Es6RenameReferences.java
View file
Expand Up @@ -47,7 +47,7 @@ final class Es6RenameReferences extends AbstractPostOrderCallback {
@Override @Override
public void visit(NodeTraversal t, Node n, Node parent) { public void visit(NodeTraversal t, Node n, Node parent) {
if (!typesOnly && NodeUtil.isReferenceName(n)) { if (!typesOnly && NodeUtil.isReferenceName(n)) {
renameReference(t, n); renameReference(t, n, false);
} }


JSDocInfo info = n.getJSDocInfo(); JSDocInfo info = n.getJSDocInfo();
Expand All @@ -59,13 +59,13 @@ public void visit(NodeTraversal t, Node n, Node parent) {
private void renameTypeNode(NodeTraversal t, Iterable<Node> typeNodes) { private void renameTypeNode(NodeTraversal t, Iterable<Node> typeNodes) {
for (Node type : typeNodes) { for (Node type : typeNodes) {
if (type.isString()) { if (type.isString()) {
renameReference(t, type); renameReference(t, type, true);
} }
renameTypeNode(t, type.children()); renameTypeNode(t, type.children());
} }
} }


private void renameReference(NodeTraversal t, Node n) { private void renameReference(NodeTraversal t, Node n, boolean isType) {
String fullName = n.getString(); String fullName = n.getString();
List<String> split = SPLIT_ON_DOT.splitToList(fullName); List<String> split = SPLIT_ON_DOT.splitToList(fullName);
String oldName = split.get(0); String oldName = split.get(0);
Expand All @@ -75,7 +75,9 @@ private void renameReference(NodeTraversal t, Node n) {
if (newName != null) { if (newName != null) {
String rest = split.size() == 2 ? "." + split.get(1) : ""; String rest = split.size() == 2 ? "." + split.get(1) : "";
n.setString(newName + rest); n.setString(newName + rest);
t.reportCodeChange(); if (!isType) {
t.reportCodeChange();
}
return; return;
} else if (current.hasOwnSlot(oldName)) { } else if (current.hasOwnSlot(oldName)) {
return; return;
Expand Down
120 changes: 80 additions & 40 deletions src/com/google/javascript/jscomp/Es6RewriteModules.java
View file
Expand Up @@ -24,6 +24,7 @@
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_GET_CALL_SCOPE; import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_GET_CALL_SCOPE;
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_GET_NAMESPACE; import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_GET_NAMESPACE;
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_REQUIRE_NAMESPACE; import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_REQUIRE_NAMESPACE;
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_REQUIRE_TYPE_NAMESPACE;
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.MISSING_MODULE_OR_PROVIDE; import static com.google.javascript.jscomp.ClosurePrimitiveErrors.MISSING_MODULE_OR_PROVIDE;
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.MODULE_USES_GOOG_MODULE_GET; import static com.google.javascript.jscomp.ClosurePrimitiveErrors.MODULE_USES_GOOG_MODULE_GET;
import static com.google.javascript.jscomp.parsing.parser.FeatureSet.Feature.MODULES; import static com.google.javascript.jscomp.parsing.parser.FeatureSet.Feature.MODULES;
Expand Down Expand Up @@ -66,7 +67,8 @@ public final class Es6RewriteModules extends AbstractPostOrderCallback
static final DiagnosticType LHS_OF_GOOG_REQUIRE_MUST_BE_CONST = static final DiagnosticType LHS_OF_GOOG_REQUIRE_MUST_BE_CONST =
DiagnosticType.error( DiagnosticType.error(
"JSC_LHS_OF_GOOG_REQUIRE_MUST_BE_CONST", "JSC_LHS_OF_GOOG_REQUIRE_MUST_BE_CONST",
"The left side of a goog.require() must use ''const'' (not ''let'' or ''var'')"); "The left side of a goog.require() or goog.requireType() "
+ "must use ''const'' (not ''let'' or ''var'')");


static final DiagnosticType NAMESPACE_IMPORT_CANNOT_USE_STAR = static final DiagnosticType NAMESPACE_IMPORT_CANNOT_USE_STAR =
DiagnosticType.error( DiagnosticType.error(
Expand All @@ -77,6 +79,11 @@ public final class Es6RewriteModules extends AbstractPostOrderCallback
static final DiagnosticType DUPLICATE_EXPORT = static final DiagnosticType DUPLICATE_EXPORT =
DiagnosticType.error("JSC_DUPLICATE_EXPORT", "Duplicate export ''{0}''."); DiagnosticType.error("JSC_DUPLICATE_EXPORT", "Duplicate export ''{0}''.");


static final DiagnosticType REQUIRE_TYPE_FOR_ES6_SHOULD_BE_CONST =
DiagnosticType.error(
"JSC_REQUIRE_TYPE_FOR_ES6_SHOULD_BE_CONST",
"goog.requireType alias for ES6 module should be const.");

static final DiagnosticType FORWARD_DECLARE_FOR_ES6_SHOULD_BE_CONST = static final DiagnosticType FORWARD_DECLARE_FOR_ES6_SHOULD_BE_CONST =
DiagnosticType.error( DiagnosticType.error(
"JSC_FORWARD_DECLARE_FOR_ES6_SHOULD_BE_CONST", "JSC_FORWARD_DECLARE_FOR_ES6_SHOULD_BE_CONST",
Expand Down Expand Up @@ -108,9 +115,10 @@ public final class Es6RewriteModules extends AbstractPostOrderCallback
private Map<String, ModuleOriginalNamePair> importMap; private Map<String, ModuleOriginalNamePair> importMap;


/** /**
* Local variable names that were goog.require'd to qualified name we need to line. We need to * Local variable names that were goog.require'd to qualified name we need to line.
* 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. * <p>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.
* *
* <p>We use this to rewrite something like: * <p>We use this to rewrite something like:
* *
Expand Down Expand Up @@ -208,27 +216,28 @@ public void clearState() {
} }


/** /**
* Checks for goog.require + goog.module.get + goog.forwardDeclare calls in non-ES6 modules that * Checks for goog.require, goog.requireType, goog.module.get and goog.forwardDeclare calls that
* are meant to import ES6 modules and rewrites them. * are meant to import ES6 modules and rewrites them.
*/ */
private class RewriteRequiresForEs6Modules extends AbstractPostOrderCallback { private class RewriteRequiresForEs6Modules extends AbstractPostOrderCallback {
private boolean transpiled = false; private boolean transpiled = false;
private Table<Node, String, String> forwardDeclareRenameTable; // An (s, old, new) entry indicates that occurrences of `old` in scope `s` should be rewritten
// as `new`. This is used to rewrite namespaces that appear in calls to goog.requireType and
// goog.forwardDeclare.
private Table<Node, String, String> renameTable;


void rewrite(Node scriptNode) { void rewrite(Node scriptNode) {
transpiled = false; transpiled = false;
forwardDeclareRenameTable = HashBasedTable.create(); renameTable = HashBasedTable.create();
NodeTraversal.traverse(compiler, scriptNode, this); NodeTraversal.traverse(compiler, scriptNode, this);


if (transpiled) { if (transpiled) {
scriptNode.putBooleanProp(Node.TRANSPILED, true); scriptNode.putBooleanProp(Node.TRANSPILED, true);
} }


if (!forwardDeclareRenameTable.isEmpty()) { if (!renameTable.isEmpty()) {
NodeTraversal.traverse( NodeTraversal.traverse(
compiler, compiler, scriptNode, new Es6RenameReferences(renameTable, /* typesOnly= */ true));
scriptNode,
new Es6RenameReferences(forwardDeclareRenameTable, /* typesOnly= */ true));
} }
} }


Expand All @@ -239,16 +248,19 @@ public void visit(NodeTraversal t, Node n, Node parent) {
} }


boolean isRequire = n.getFirstChild().matchesQualifiedName("goog.require"); boolean isRequire = n.getFirstChild().matchesQualifiedName("goog.require");
boolean isRequireType = n.getFirstChild().matchesQualifiedName("goog.requireType");
boolean isGet = n.getFirstChild().matchesQualifiedName("goog.module.get"); boolean isGet = n.getFirstChild().matchesQualifiedName("goog.module.get");
boolean isForwardDeclare = n.getFirstChild().matchesQualifiedName("goog.forwardDeclare"); boolean isForwardDeclare = n.getFirstChild().matchesQualifiedName("goog.forwardDeclare");


if (!isRequire && !isGet && !isForwardDeclare) { if (!isRequire && !isRequireType && !isGet && !isForwardDeclare) {
return; return;
} }


if (!n.hasTwoChildren() || !n.getLastChild().isString()) { if (!n.hasTwoChildren() || !n.getLastChild().isString()) {
if (isRequire) { if (isRequire) {
t.report(n, INVALID_REQUIRE_NAMESPACE); t.report(n, INVALID_REQUIRE_NAMESPACE);
} else if (isRequireType) {
t.report(n, INVALID_REQUIRE_TYPE_NAMESPACE);
} else if (isGet) { } else if (isGet) {
t.report(n, INVALID_GET_NAMESPACE); t.report(n, INVALID_GET_NAMESPACE);
} else { } else {
Expand Down Expand Up @@ -282,41 +294,62 @@ public void visit(NodeTraversal t, Node n, Node parent) {


Node statementNode = NodeUtil.getEnclosingStatement(n); Node statementNode = NodeUtil.getEnclosingStatement(n);
boolean importHasAlias = NodeUtil.isNameDeclaration(statementNode); boolean importHasAlias = NodeUtil.isNameDeclaration(statementNode);

if (importHasAlias) { if (importHasAlias) {
if (statementNode.getFirstChild().isDestructuringLhs()) { if (statementNode.getFirstChild().isDestructuringLhs()) {
if (isForwardDeclare) { if (isForwardDeclare) {
// const {a, c:b} = goog.forwardDeclare('an.es6.namespace');
t.report(n, INVALID_DESTRUCTURING_FORWARD_DECLARE); t.report(n, INVALID_DESTRUCTURING_FORWARD_DECLARE);
return; return;
} }
// Work around a bug in the type checker where destructing can create if (isRequireType) {
// too many layers of aliases and confuse the type checker. b/112061124. if (!statementNode.isConst()) {

t.report(statementNode, REQUIRE_TYPE_FOR_ES6_SHOULD_BE_CONST);
// const {a, c:b} = goog.require('an.es6.namespace'); return;
// const a = module$es6.a; }
// const b = module$es6.c; // const {a, c:b} = goog.requireType('an.es6.namespace');
for (Node child : statementNode.getFirstFirstChild().children()) { for (Node child : statementNode.getFirstFirstChild().children()) {
checkState(child.isStringKey()); checkState(child.isStringKey());
checkState(child.getFirstChild().isName()); checkState(child.getFirstChild().isName());
Node constNode = renameTable.put(
IR.constNode( t.getScopeRoot(),
IR.name(child.getFirstChild().getString()), child.getFirstChild().getString(),
IR.getprop( ModuleRenaming.getGlobalName(moduleMetadata, name) + "." + child.getString());
IR.name(ModuleRenaming.getGlobalName(moduleMetadata, name)), }
IR.string(child.getString()))); } else {
constNode.useSourceInfoFromForTree(child); // Work around a bug in the type checker where destructing can create
statementNode.getParent().addChildBefore(constNode, statementNode); // too many layers of aliases and confuse the type checker. b/112061124.

// const {a, c:b} = goog.require('an.es6.namespace');
// const a = module$es6.a;
// const b = module$es6.c;
for (Node child : statementNode.getFirstFirstChild().children()) {
checkState(child.isStringKey());
checkState(child.getFirstChild().isName());
Node constNode =
IR.constNode(
IR.name(child.getFirstChild().getString()),
IR.getprop(
IR.name(ModuleRenaming.getGlobalName(moduleMetadata, name)),
IR.string(child.getString())));
constNode.useSourceInfoFromForTree(child);
statementNode.getParent().addChildBefore(constNode, statementNode);
}
} }
statementNode.detach(); statementNode.detach();
t.reportCodeChange(); t.reportCodeChange();
} else { } else {
if (isForwardDeclare) { if (isForwardDeclare || isRequireType) {
if (!statementNode.isConst()) { if (!statementNode.isConst()) {
t.report(statementNode, FORWARD_DECLARE_FOR_ES6_SHOULD_BE_CONST); DiagnosticType diagnostic =
isForwardDeclare
? FORWARD_DECLARE_FOR_ES6_SHOULD_BE_CONST
: REQUIRE_TYPE_FOR_ES6_SHOULD_BE_CONST;
t.report(statementNode, diagnostic);
return; return;
} }
// const namespace = goog.forwardDeclare('an.es6.namespace'); // const namespace = goog.forwardDeclare('an.es6.namespace');
forwardDeclareRenameTable.put( // const namespace = goog.requireType('an.es6.namespace');
renameTable.put(
t.getScopeRoot(), t.getScopeRoot(),
statementNode.getFirstChild().getString(), statementNode.getFirstChild().getString(),
ModuleRenaming.getGlobalName(moduleMetadata, name)); ModuleRenaming.getGlobalName(moduleMetadata, name));
Expand All @@ -332,9 +365,10 @@ public void visit(NodeTraversal t, Node n, Node parent) {
} }
} }
} else { } else {
if (isForwardDeclare) { if (isForwardDeclare || isRequireType) {
// goog.forwardDeclare('an.es6.namespace') // goog.forwardDeclare('an.es6.namespace')
forwardDeclareRenameTable.put( // goog.requireType('an.es6.namespace')
renameTable.put(
t.getScopeRoot(), name, ModuleRenaming.getGlobalName(moduleMetadata, name)); t.getScopeRoot(), name, ModuleRenaming.getGlobalName(moduleMetadata, name));
statementNode.detach(); statementNode.detach();
} else { } else {
Expand Down Expand Up @@ -714,9 +748,15 @@ private void rewriteRequires(Node script) {
script, script,
(NodeTraversal t, Node n, Node parent) -> { (NodeTraversal t, Node n, Node parent) -> {
if (n.isCall()) { if (n.isCall()) {
if (n.getFirstChild().matchesQualifiedName("goog.require")) { Node fn = n.getFirstChild();
if (fn.matchesQualifiedName("goog.require")
|| fn.matchesQualifiedName("goog.requireType")) {
// TODO(tjgq): This will rewrite both type references and code references. For
// goog.requireType, the latter are potentially broken because the symbols aren't
// guaranteed to be available at run time. A separate pass needs to be added to
// detect these incorrect uses of goog.requireType.
visitRequireOrGet(t, n, parent, /* isRequire= */ true); visitRequireOrGet(t, n, parent, /* isRequire= */ true);
} else if (n.getFirstChild().matchesQualifiedName("goog.module.get")) { } else if (fn.matchesQualifiedName("goog.module.get")) {
visitGoogModuleGet(t, n, parent); visitGoogModuleGet(t, n, parent);
} }
} }
Expand All @@ -728,7 +768,7 @@ private void rewriteRequires(Node script) {
JSDocInfo info = n.getJSDocInfo(); JSDocInfo info = n.getJSDocInfo();
if (info != null) { if (info != null) {
for (Node typeNode : info.getTypeNodes()) { for (Node typeNode : info.getTypeNodes()) {
inlineRequiredType(t, typeNode); inlineAliasedTypes(t, typeNode);
} }
} }


Expand All @@ -744,7 +784,7 @@ private void rewriteRequires(Node script) {
}); });
} }


private void inlineRequiredType(NodeTraversal t, Node typeNode) { private void inlineAliasedTypes(NodeTraversal t, Node typeNode) {
if (typeNode.isString()) { if (typeNode.isString()) {
String name = typeNode.getString(); String name = typeNode.getString();
List<String> split = Splitter.on('.').limit(2).splitToList(name); List<String> split = Splitter.on('.').limit(2).splitToList(name);
Expand All @@ -764,7 +804,7 @@ private void inlineRequiredType(NodeTraversal t, Node typeNode) {
} }
} }
for (Node child : typeNode.children()) { for (Node child : typeNode.children()) {
inlineRequiredType(t, child); inlineAliasedTypes(t, child);
} }
} }


Expand Down
3 changes: 2 additions & 1 deletion test/com/google/javascript/jscomp/ClosureRewriteModuleTest.java
View file
Expand Up @@ -19,6 +19,7 @@
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_GET_CALL_SCOPE; import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_GET_CALL_SCOPE;
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_GET_NAMESPACE; import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_GET_NAMESPACE;
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_REQUIRE_NAMESPACE; import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_REQUIRE_NAMESPACE;
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.INVALID_REQUIRE_TYPE_NAMESPACE;
import static com.google.javascript.jscomp.ClosurePrimitiveErrors.MISSING_MODULE_OR_PROVIDE; import static com.google.javascript.jscomp.ClosurePrimitiveErrors.MISSING_MODULE_OR_PROVIDE;
import static com.google.javascript.jscomp.ClosureRewriteModule.DUPLICATE_MODULE; import static com.google.javascript.jscomp.ClosureRewriteModule.DUPLICATE_MODULE;
import static com.google.javascript.jscomp.ClosureRewriteModule.DUPLICATE_NAMESPACE; import static com.google.javascript.jscomp.ClosureRewriteModule.DUPLICATE_NAMESPACE;
Expand Down Expand Up @@ -1043,7 +1044,7 @@ public void testInvalidRequire() {


@Test @Test
public void testInvalidRequireType() { public void testInvalidRequireType() {
testError("goog.module('ns.a');" + "goog.requireType(a);", INVALID_REQUIRE_NAMESPACE); testError("goog.module('ns.a');" + "goog.requireType(a);", INVALID_REQUIRE_TYPE_NAMESPACE);
} }


@Test @Test
Expand Down

0 comments on commit eb521a4

Please sign in to comment.