Skip to content

Commit

Permalink
Don't create a binary namespace for legacy modules.
Browse files Browse the repository at this point in the history 
Update module rewriting to not create a binary module name for those modules
declared with goog.module.declareLegacyNamespace. This also fixes issues
where the wrong goog.exportSymbol name would be generated in some legacy
goog.module cases (see the new integration tests).
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=122542046
  • Loading branch information
@blickly
blickly committed May 17, 2016
1 parent 5c3ab1c commit c972e27
Show file tree
Hide file tree
Showing 3 changed files with 244 additions and 119 deletions.
165 changes: 88 additions & 77 deletions src/com/google/javascript/jscomp/ClosureRewriteModule.java
View file
Expand Up @@ -39,6 +39,8 @@
import java.util.Map;
import java.util.Set;

import javax.annotation.Nullable;

/**
* Process aliases in goog.modules.
* <pre>
Expand Down Expand Up @@ -210,7 +212,6 @@ private final class ScriptDescription {
boolean isModule;
boolean declareLegacyNamespace;
String legacyNamespace; // "a.b.c"
String binaryNamespace; // "module$exports$a$b$c
String contentsPrefix; // "module$contents$a$b$c_
final Set<String> topLevelNames = new HashSet<>(); // For prefixed content renaming.
final Deque<ScriptDescription> childScripts = new LinkedList<>(); // For goog.loadModule()
Expand All @@ -235,6 +236,14 @@ public void addChildScript(ScriptDescription childScript) {
public ScriptDescription removeFirstChildScript() {
return childScripts.removeFirst();
}

// "module$exports$a$b$c" for non-legacy modules
@Nullable String getBinaryNamespace() {
if (this.declareLegacyNamespace) {
return null;
}
return MODULE_EXPORTS_PREFIX + this.legacyNamespace.replace('.', '$');
}
}

private class ScriptRecorder extends AbstractPreOrderCallback {
Expand Down Expand Up @@ -411,11 +420,7 @@ public void visit(Node typeRefNode) {
currentScript.legacyNamespacesByAlias.containsKey(prefixTypeName);
if (nameIsAnAlias) {
String legacyNamespace = currentScript.legacyNamespacesByAlias.get(prefixTypeName);
boolean targetNamespaceIsAGoogModule = rewriteState.containsModule(legacyNamespace);
String aliasedNamespace =
targetNamespaceIsAGoogModule
? toBinaryNamespace(legacyNamespace)
: legacyNamespace;
String aliasedNamespace = rewriteState.getExportedNamespace(legacyNamespace);
safeSetString(typeRefNode, aliasedNamespace + suffix);
return;
}
Expand All @@ -428,9 +433,10 @@ public void visit(Node typeRefNode) {
return;
}

boolean targetIsAGoogModule = rewriteState.containsModule(prefixTypeName);

String binaryNamespaceIfModule = rewriteState.getBinaryNamespace(prefixTypeName);
if (legacyScriptNamespacesAndPrefixes.contains(prefixTypeName)
&& !targetIsAGoogModule) {
&& binaryNamespaceIfModule == null) {
// This thing is definitely coming from a legacy script and so the fully qualified
// type name will always resolve as is.
return;
Expand All @@ -439,8 +445,8 @@ public void visit(Node typeRefNode) {
// If the typeName is a reference to a fully qualified legacy namespace like
// "foo.bar.Baz" of something that is actually a module then rewrite the JsDoc reference
// to "module$exports$Bar".
if (targetIsAGoogModule) {
safeSetString(typeRefNode, toBinaryNamespace(prefixTypeName) + suffix);
if (binaryNamespaceIfModule != null) {
safeSetString(typeRefNode, binaryNamespaceIfModule + suffix);
return;
}

Expand Down Expand Up @@ -477,6 +483,16 @@ boolean isLegacyModule(String legacyNamespace) {
return scriptDescriptionsByGoogModuleNamespace.get(legacyNamespace).declareLegacyNamespace;
}

@Nullable String getBinaryNamespace(String legacyNamespace) {
ScriptDescription script = scriptDescriptionsByGoogModuleNamespace.get(legacyNamespace);
return script == null ? null : script.getBinaryNamespace();
}

String getExportedNamespace(String legacyNamespace) {
String binaryNamespace = getBinaryNamespace(legacyNamespace);
return binaryNamespace != null ? binaryNamespace : legacyNamespace;
}

void trackModule(String legacyNamespace, ScriptDescription description) {
scriptDescriptionsByGoogModuleNamespace.put(legacyNamespace, description);
moduleNamesByRootNode.put(description.rootNode, legacyNamespace);
Expand Down Expand Up @@ -576,7 +592,6 @@ private void recordGoogModule(NodeTraversal t, Node call) {
currentScript.isModule = true;
currentScript.rootNode = NodeUtil.getEnclosingStatement(call).getParent();
currentScript.legacyNamespace = legacyNamespace;
currentScript.binaryNamespace = toBinaryNamespace(legacyNamespace);
currentScript.contentsPrefix = toModuleContentsPrefix(legacyNamespace);

// If some other script is advertising itself as a goog.module() with this same namespace.
Expand Down Expand Up @@ -726,7 +741,7 @@ private void recordGoogModuleGet(NodeTraversal t, Node call) {

if (aliasName != null
&& isTopLevel(t, NodeUtil.getEnclosingStatement(call), ScopeType.EXEC_CONTEXT)) {
// Record alias -> binaryNamespace associations for later inlining.
// Record alias -> legacyNamespace associations for later inlining.
recordImportAlias(aliasName, legacyNamespace);
}
}
Expand All @@ -751,7 +766,10 @@ private void recordTopLevelVarNames(Node varNode) {
}

private void maybeRecordExportDeclaration(Node n) {
if (!currentScript.isModule || !n.getString().equals("exports") || !isAssignTarget(n)) {
if (!currentScript.isModule
|| currentScript.declareLegacyNamespace
|| !n.getString().equals("exports")
|| !isAssignTarget(n)) {
return;
}

Expand Down Expand Up @@ -810,8 +828,10 @@ private void updateGoogRequire(NodeTraversal t, Node call) {
Node statementNode = NodeUtil.getEnclosingStatement(call);
String legacyNamespace = legacyNamespaceNode.getString();

boolean targetIsGoogModule = rewriteState.containsModule(legacyNamespace);
boolean targetIsAGoogModuleGetted =
boolean targetIsNonLegacyGoogModule =
rewriteState.containsModule(legacyNamespace)
&& !rewriteState.isLegacyModule(legacyNamespace);
boolean targetIsGoogModuleGetted =
currentScript.googModuleGettedNamespaces.contains(legacyNamespaceNode.getString());
boolean importHasAlias = currentScript.legacyNamespacesByAlias.containsValue(legacyNamespace);
boolean isDestructuring = statementNode.getFirstChild().isDestructuringLhs();
Expand All @@ -821,30 +841,28 @@ private void updateGoogRequire(NodeTraversal t, Node call) {
&& call.getGrandparent().isName()
&& NodeUtil.isNameDeclaration(call.getGrandparent().getParent());

if (currentScript.isModule || (targetIsGoogModule && targetIsAGoogModuleGetted)) {
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;"
Node replacementNamespaceName =
NodeUtil.newQName(
compiler,
targetIsGoogModule ? toBinaryNamespace(legacyNamespace) : legacyNamespace);
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);
} else if (targetIsGoogModule) {
} else if (targetIsNonLegacyGoogModule) {
if (immediatePropertyAccess || !isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {
// Rewrite
// "var Foo = goog.require("bar.Foo").Foo;" to
// "var Foo = module$exports$bar$Foo.Foo;"
// or
// "function() {var Foo = goog.require("bar.Foo");}" to
// "function() {var Foo = module$exports$bar$Foo;}"
Node binaryNamespaceName = IR.name(toBinaryNamespace(legacyNamespace));
Node binaryNamespaceName = IR.name(rewriteState.getBinaryNamespace(legacyNamespace));
binaryNamespaceName.putProp(Node.ORIGINALNAME_PROP, legacyNamespace);
call.getParent().replaceChild(call, binaryNamespaceName);
} else if (importHasAlias || !rewriteState.isLegacyModule(legacyNamespace)) {
Expand Down Expand Up @@ -874,7 +892,7 @@ private void updateGoogRequire(NodeTraversal t, Node call) {
statementNode.getParent().replaceChild(statementNode, IR.exprResult(call));
}
}
if (targetIsGoogModule) {
if (targetIsNonLegacyGoogModule) {
// Add goog.require() and namespace name to preprocessor table because they're removed
// by current pass. If target is not a module then goog.require() is retained for
// ProcessClosurePrimitives pass and symbols will be added there instead.
Expand Down Expand Up @@ -906,9 +924,11 @@ 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 binaryNamespaceName = IR.name(toBinaryNamespace(legacyNamespace));
binaryNamespaceName.putProp(Node.ORIGINALNAME_PROP, legacyNamespace);
call.getParent().replaceChild(call, binaryNamespaceName);
Node exportedNamespaceName =
NodeUtil.newQName(compiler, rewriteState.getExportedNamespace(legacyNamespace))
.srcrefTree(call);
exportedNamespaceName.putProp(Node.ORIGINALNAME_PROP, legacyNamespace);
call.getParent().replaceChild(call, exportedNamespaceName);
}
compiler.reportCodeChange();
}
Expand All @@ -924,7 +944,11 @@ private void updateExportsPropertyAssignment(Node getpropNode) {
// Update "exports.foo = Foo" to "module$exports$pkg$Foo.foo = Foo";
Node exportsNameNode = getpropNode.getFirstChild();
Preconditions.checkState(exportsNameNode.getString().equals("exports"));
safeSetString(exportsNameNode, currentScript.binaryNamespace);
String exportedNamespace =
currentScript.declareLegacyNamespace
? currentScript.legacyNamespace
: currentScript.getBinaryNamespace();
safeSetMaybeQualifiedString(exportsNameNode, exportedNamespace);

Node jsdocNode = parent.isAssign() ? parent : getpropNode;
markConstAndCopyJsDoc(jsdocNode, jsdocNode, parent.getLastChild());
Expand Down Expand Up @@ -958,9 +982,7 @@ private void maybeUpdateTopLevelName(NodeTraversal t, Node nameNode) {
boolean nameIsAnAlias = currentScript.legacyNamespacesByAlias.containsKey(name);
if (nameIsAnAlias && var.getNode() != nameNode) {
String legacyNamespace = currentScript.legacyNamespacesByAlias.get(name);
boolean targetNamespaceIsAGoogModule = rewriteState.containsModule(legacyNamespace);
String namespaceToInline =
targetNamespaceIsAGoogModule ? toBinaryNamespace(legacyNamespace) : legacyNamespace;
String namespaceToInline = rewriteState.getExportedNamespace(legacyNamespace);
safeSetMaybeQualifiedString(nameNode, namespaceToInline);

// Make sure this action won't shadow a local variable.
Expand Down Expand Up @@ -1044,39 +1066,54 @@ private void maybeUpdateExportDeclToNode(NodeTraversal t, Node target, Node valu
* In module "foo.Bar", rewrite "exports = Bar" to "var module$exports$foo$Bar = Bar".
*/
private void maybeUpdateExportDeclaration(NodeTraversal t, Node n) {
if (!currentScript.isModule || !n.getString().equals("exports") || !isAssignTarget(n)) {
if (!currentScript.isModule
|| !n.getString().equals("exports")
|| !isAssignTarget(n)) {
return;
}

// Rewrite "exports = ..." as "var module$exports$foo$Bar = ..."
Node assignNode = n.getParent();
Node exprResultNode = assignNode.getParent();
Node rhs = assignNode.getLastChild();
rhs.detachFromParent();
Node binaryNamespaceName = IR.name(currentScript.binaryNamespace);
binaryNamespaceName.putProp(Node.ORIGINALNAME_PROP, currentScript.legacyNamespace);
Node exportsObjectCreationNode = IR.var(binaryNamespaceName, rhs);
exportsObjectCreationNode.srcrefTree(exprResultNode);
exportsObjectCreationNode.putBooleanProp(Node.IS_NAMESPACE, true);
exprResultNode.getParent().replaceChild(exprResultNode, exportsObjectCreationNode);
Node jsdocNode;
if (currentScript.declareLegacyNamespace) {
Node legacyQname = NodeUtil.newQName(compiler, currentScript.legacyNamespace).srcrefTree(n);
assignNode.replaceChild(n, legacyQname);
jsdocNode = assignNode;
} else {
rhs.detachFromParent();
Node exprResultNode = assignNode.getParent();
Node binaryNamespaceName = IR.name(currentScript.getBinaryNamespace());
binaryNamespaceName.putProp(Node.ORIGINALNAME_PROP, currentScript.legacyNamespace);
Node exportsObjectCreationNode = IR.var(binaryNamespaceName, rhs);
exportsObjectCreationNode.srcrefTree(exprResultNode);
exportsObjectCreationNode.putBooleanProp(Node.IS_NAMESPACE, true);
exprResultNode.getParent().replaceChild(exprResultNode, exportsObjectCreationNode);
jsdocNode = exportsObjectCreationNode;
currentScript.hasCreatedExportObject = true;
}
markConstAndCopyJsDoc(assignNode, jsdocNode, rhs);
compiler.reportCodeChange();
currentScript.hasCreatedExportObject = true;

markConstAndCopyJsDoc(assignNode, exportsObjectCreationNode, rhs);
maybeExportLegacyNamespaceAfter(exportsObjectCreationNode, assignNode);
maybeUpdateExportObjectLiteral(t, rhs);
return;
}

private void maybeUpdateExportNameRef(Node n) {
if (!currentScript.isModule || !"exports".equals(n.getString())) {
if (!currentScript.isModule || !"exports".equals(n.getString()) || n.getParent() == null) {
return;
}
if (n.getParent().isParamList()) {
return;
}

safeSetString(n, currentScript.binaryNamespace);
if (currentScript.declareLegacyNamespace) {
Node legacyQname = NodeUtil.newQName(compiler, currentScript.legacyNamespace).srcrefTree(n);
n.getParent().replaceChild(n, legacyQname);
return;
}

safeSetString(n, currentScript.getBinaryNamespace());

// Either this module is going to create it's own exports object at some point or else if it's
// going to be defensively created automatically then that should have occurred at the top of
Expand Down Expand Up @@ -1110,7 +1147,8 @@ private void updateEndScript() {

private void updateEndModule() {
Preconditions.checkState(currentScript.isModule);
Preconditions.checkState(currentScript.hasCreatedExportObject);
Preconditions.checkState(
currentScript.declareLegacyNamespace || currentScript.hasCreatedExportObject);
}

/**
Expand Down Expand Up @@ -1138,7 +1176,11 @@ private void popScript() {
* Add the missing "var module$exports$pkg$Foo = {};" line.
*/
private void exportTheEmptyBinaryNamespaceAt(Node atNode, AddAt addAt) {
Node binaryNamespaceName = IR.name(currentScript.binaryNamespace);
if (currentScript.declareLegacyNamespace) {
return;
}

Node binaryNamespaceName = IR.name(currentScript.getBinaryNamespace());
binaryNamespaceName.putProp(Node.ORIGINALNAME_PROP, currentScript.legacyNamespace);
Node binaryNamespaceExportNode = IR.var(binaryNamespaceName, IR.objectlit());
if (addAt == AddAt.BEFORE) {
Expand All @@ -1151,33 +1193,6 @@ private void exportTheEmptyBinaryNamespaceAt(Node atNode, AddAt addAt) {
markConst(binaryNamespaceExportNode);
compiler.reportCodeChange();
currentScript.hasCreatedExportObject = true;

maybeExportLegacyNamespaceAfter(binaryNamespaceExportNode, null);
}

/**
* Maybe add a "foo.Bar = module$exports$foo$Bar;" statement to satisfy legacy namespace refs.
*/
private void maybeExportLegacyNamespaceAfter(Node afterNode, Node jsDocSourceNode) {
if (!currentScript.declareLegacyNamespace) {
return;
}

Node binaryNamespaceName = IR.name(currentScript.binaryNamespace);
binaryNamespaceName.putProp(Node.ORIGINALNAME_PROP, currentScript.legacyNamespace);
Node assignmentNode =
IR.assign(NodeUtil.newQName(compiler, currentScript.legacyNamespace), binaryNamespaceName);
Node binaryToLegacyBridgeStatement = IR.exprResult(assignmentNode);
binaryToLegacyBridgeStatement.srcrefTree(afterNode);
if (jsDocSourceNode != null) {
markConstAndCopyJsDoc(jsDocSourceNode, assignmentNode, binaryNamespaceName);
} else {
markConst(assignmentNode);
}
// Do NOT mark this statement 'IS_NAMESPACE' because that would prevent
// ProcessClosurePrimitives from properly desugaring goog.provide().
afterNode.getParent().addChildAfter(binaryToLegacyBridgeStatement, afterNode);
compiler.reportCodeChange();
}

/**
Expand Down Expand Up @@ -1337,10 +1352,6 @@ private boolean isTopLevel(NodeTraversal t, Node n, ScopeType scopeType) {
}
}

private static String toBinaryNamespace(String legacyNamespace) {
return MODULE_EXPORTS_PREFIX + legacyNamespace.replace('.', '$');
}

private static String toModuleContentsPrefix(String legacyNamespace) {
return MODULE_CONTENTS_PREFIX + legacyNamespace.replace('.', '$') + "_";
}
Expand Down

0 comments on commit c972e27

Please sign in to comment.