Skip to content

Commit

Permalink
[RefasterJS] Add support for correctly handling goog.require inside a…
Browse files Browse the repository at this point in the history 
… goog.module.

- Attempts to detect whether a file should use const foo = goog.require or just
goog.require.
- If using short names, rewrite the template to use aliases as appropriate.
- Fix sorting so the goog.require statements added by SuggestedFix are handled
bette...

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=160654194
  • Loading branch information
@brad4d
tbreisacher authored and brad4d committed Jul 5, 2017
1 parent 40967b4 commit cd1dd52
Show file tree
Hide file tree
Showing 18 changed files with 466 additions and 44 deletions.
29 changes: 19 additions & 10 deletions src/com/google/javascript/jscomp/ClosureRewriteModule.java
View file
Expand Up @@ -198,6 +198,7 @@ final class ClosureRewriteModule implements HotSwapCompilerPass {

private final AbstractCompiler compiler;
private final PreprocessorSymbolTable preprocessorSymbolTable;
private final boolean preserveSugar;

/**
* Indicates where new nodes should be added in relation to some other node.
Expand Down Expand Up @@ -646,6 +647,7 @@ void removeRoot(Node toRemove) {
this.compiler = compiler;
this.preprocessorSymbolTable = preprocessorSymbolTable;
this.rewriteState = moduleRewriteState != null ? moduleRewriteState : new GlobalRewriteState();
this.preserveSugar = compiler.getOptions().shouldPreserveGoogModule();
}

private class UnwrapGoogLoadModule extends NodeTraversal.AbstractPreOrderCallback {
Expand Down Expand Up @@ -993,7 +995,7 @@ private void updateGoogModule(Node call) {
exportTheEmptyBinaryNamespaceAt(NodeUtil.getEnclosingStatement(call), AddAt.AFTER);
}

if (!currentScript.declareLegacyNamespace) {
if (!currentScript.declareLegacyNamespace && !preserveSugar) {
// Otherwise it's a regular module and the goog.module() line can be removed.
compiler.reportChangeToEnclosingScope(call);
NodeUtil.getEnclosingStatement(call).detach();
Expand Down Expand Up @@ -1056,9 +1058,11 @@ && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {

if (currentScript.isModule || targetIsNonLegacyGoogModule) {
if (isDestructuring) {
// Delete the goog.require() because we're going to inline its alias later.
compiler.reportChangeToEnclosingScope(statementNode);
statementNode.detach();
if (!preserveSugar) {
// Delete the goog.require() because we're going to inline its alias later.
compiler.reportChangeToEnclosingScope(statementNode);
statementNode.detach();
}
} else if (targetIsNonLegacyGoogModule) {
if (!isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {
// Rewrite
Expand All @@ -1069,11 +1073,15 @@ && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {
call.replaceWith(binaryNamespaceName);
compiler.reportChangeToEnclosingScope(binaryNamespaceName);
} else if (importHasAlias || !rewriteState.isLegacyModule(legacyNamespace)) {
// Delete the goog.require() because we're going to inline its alias later.
compiler.reportChangeToEnclosingScope(statementNode);
statementNode.detach();
if (!preserveSugar) {
// Delete the goog.require() because we're going to inline its alias later.
compiler.reportChangeToEnclosingScope(statementNode);
statementNode.detach();
}
}
} else {
// TODO(bangert): make this compatible with preserveSugar. const B = goog.require('b') runs
// into problems because the type checker cannot handle const.
// Rewrite
// "var B = goog.require('B');" to
// "goog.require('B');"
Expand All @@ -1083,7 +1091,7 @@ && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {
statementNode.replaceWith(IR.exprResult(call));
compiler.reportChangeToEnclosingScope(call);
}
if (targetIsNonLegacyGoogModule) {
if (targetIsNonLegacyGoogModule && !preserveSugar) {
// 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 @@ -1472,7 +1480,6 @@ private void maybeSplitMultiVar(Node rhsNode) {
Node nameNode = rhsNode.getParent();
nameNode.detach();
rhsNode.detach();

statementNode.getParent().addChildBefore(IR.var(nameNode, rhsNode), statementNode);
}

Expand Down Expand Up @@ -1529,7 +1536,9 @@ private void reportUnrecognizedRequires() {
legacyNamespace));
// Remove the require node so this problem isn't reported all over again in
// ProcessClosurePrimitives.
NodeUtil.getEnclosingStatement(requireNode).detach();
if (!preserveSugar) {
NodeUtil.getEnclosingStatement(requireNode).detach();
}
continue;
}

Expand Down
25 changes: 20 additions & 5 deletions src/com/google/javascript/jscomp/CompilerOptions.java
View file
Expand Up @@ -755,7 +755,7 @@ public void setReplaceMessagesWithChromeI18n(
public boolean closurePass;

/** Do not strip goog.provide()/goog.require() calls from the code. */
private boolean preserveGoogProvidesAndRequires;
private boolean preserveClosurePrimitives;

/** Processes AngularJS-specific annotations */
boolean angularPass;
Expand Down Expand Up @@ -1263,7 +1263,7 @@ public CompilerOptions() {
locale = null;
markAsCompiled = false;
closurePass = false;
preserveGoogProvidesAndRequires = false;
preserveClosurePrimitives = false;
angularPass = false;
polymerVersion = null;
dartPass = false;
Expand Down Expand Up @@ -2426,12 +2426,27 @@ public void setClosurePass(boolean closurePass) {
this.closurePass = closurePass;
}

/** Preserve closure primitives.
*
* For now, this only preserves goog.provide(), goog.require() and goog.module() calls.
*/
public void setPreserveClosurePrimitives(boolean preserveClosurePrimitives) {
this.preserveClosurePrimitives = preserveClosurePrimitives;
}

// TODO(bangert): Delete this alias once it has been deprecated for 3 months.
/** Preserve goog.provide(), goog.require() and goog.module() calls. */
@Deprecated
public void setPreserveGoogProvidesAndRequires(boolean preserveGoogProvidesAndRequires) {
this.preserveGoogProvidesAndRequires = preserveGoogProvidesAndRequires;
setPreserveClosurePrimitives(preserveGoogProvidesAndRequires);
}

public boolean shouldPreservesGoogProvidesAndRequires() {
return this.preserveGoogProvidesAndRequires || this.shouldGenerateTypedExterns();
return this.preserveClosurePrimitives || this.shouldGenerateTypedExterns();
}

public boolean shouldPreserveGoogModule() {
return this.preserveClosurePrimitives;
}

public void setPreserveTypeAnnotations(boolean preserveTypeAnnotations) {
Expand Down Expand Up @@ -2874,7 +2889,7 @@ public String toString() {
.add("preferSingleQuotes", preferSingleQuotes)
.add("preferStableNames", preferStableNames)
.add("preserveDetailedSourceInfo", preservesDetailedSourceInfo())
.add("preserveGoogProvidesAndRequires", preserveGoogProvidesAndRequires)
.add("preserveGoogProvidesAndRequires", preserveClosurePrimitives)
.add("preserveTypeAnnotations", preserveTypeAnnotations)
.add("prettyPrint", prettyPrint)
.add("preventLibraryInjection", preventLibraryInjection)
Expand Down
7 changes: 5 additions & 2 deletions src/com/google/javascript/jscomp/ProcessClosurePrimitives.java
View file
Expand Up @@ -375,8 +375,11 @@ public void visit(NodeTraversal t, Node n, Node parent) {

private boolean validPrimitiveCall(NodeTraversal t, Node n) {
if (!n.getParent().isExprResult() || !t.inGlobalHoistScope()) {
compiler.report(t.makeError(n, INVALID_CLOSURE_CALL_ERROR));
return false;
// Ignore invalid primitives if we didn't strip module sugar.
if (!compiler.getOptions().shouldPreserveGoogModule()) {
compiler.report(t.makeError(n, INVALID_CLOSURE_CALL_ERROR));
return false;
}
}
return true;
}
Expand Down
4 changes: 3 additions & 1 deletion src/com/google/javascript/jscomp/TemplateAstMatcher.java
View file
Expand Up @@ -440,7 +440,9 @@ private boolean matchesNode(Node template, Node ast) {
// subsequent usages of the same named node are equivalent.
return ast.getString().equals(this.localVarMatches.get(paramIndex));
} else {
this.localVarMatches.set(paramIndex, ast.getString());
String originalName = ast.getOriginalName();
String name = (originalName != null) ? originalName : ast.getString();
this.localVarMatches.set(paramIndex, name);
}
} else if (isTemplateParameterStringLiteralNode(template)) {
int paramIndex = (int) (template.getDouble());
Expand Down
6 changes: 5 additions & 1 deletion src/com/google/javascript/refactoring/Matchers.java
View file
Expand Up @@ -221,12 +221,16 @@ public boolean matches(Node node, NodeMetadata metadata) {
};
}

public static Matcher googModule() {
return functionCall("goog.module");
}

public static Matcher googRequire() {
return functionCall("goog.require");
}

public static Matcher googModuleOrProvide() {
return anyOf(functionCall("goog.module"), functionCall("goog.provide"));
return anyOf(googModule(), functionCall("goog.provide"));
}

/**
Expand Down
9 changes: 6 additions & 3 deletions src/com/google/javascript/refactoring/RefactoringDriver.java
View file
Expand Up @@ -106,10 +106,13 @@ public static CompilerOptions getCompilerOptions() {
options.setCheckSuspiciousCode(true);
options.setCheckSymbols(true);
options.setCheckTypes(true);
options.setClosurePass(true);
options.setBrokenClosureRequiresLevel(CheckLevel.OFF);
// TODO(bangert): Remove this -- we want to rewrite code before closure syntax is removed.
// Unfortunately, setClosurePass is required, or code doesn't type check.
options.setClosurePass(true);
options.setGenerateExports(true);
options.setPreserveGoogProvidesAndRequires(true);

options.setPreserveClosurePrimitives(true);
options.setWarningLevel(DiagnosticGroups.STRICT_MISSING_REQUIRE, CheckLevel.WARNING);
options.setWarningLevel(DiagnosticGroups.EXTRA_REQUIRE, CheckLevel.WARNING);
options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, CheckLevel.WARNING);
Expand Down
36 changes: 27 additions & 9 deletions src/com/google/javascript/refactoring/RefasterJsScanner.java
View file
Expand Up @@ -29,6 +29,7 @@
import com.google.javascript.jscomp.NodeUtil;
import com.google.javascript.jscomp.SourceFile;
import com.google.javascript.jscomp.TypeMatchingStrategy;
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.JSDocInfo;
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.TypeIRegistry;
Expand Down Expand Up @@ -140,10 +141,18 @@ public List<SuggestedFix> processMatch(Match match) {
.isEquivalentTo(matchedTemplate.afterTemplate.getLastChild())) {
return ImmutableList.of();
}

HashMap<String, String> shortNames = new HashMap<>();
for (String require : matchedTemplate.getGoogRequiresToAdd()) {
fix.addGoogRequire(match, require);
shortNames.put(require, fix.getRequireName(match, require));
}

Node newNode =
transformNode(
matchedTemplate.afterTemplate.getLastChild(),
matchedTemplate.matcher.getTemplateNodeToMatchMap());
matchedTemplate.matcher.getTemplateNodeToMatchMap(),
shortNames);
Node nodeToReplace = match.getNode();
fix.attachMatchedNodeInfo(nodeToReplace, match.getMetadata().getCompiler());
fix.replace(nodeToReplace, newNode, match.getMetadata().getCompiler());
Expand All @@ -161,21 +170,21 @@ public List<SuggestedFix> processMatch(Match match) {
fix.delete(n);
n = n.getNext();
}
// Add/remove any goog.requires
for (String require : matchedTemplate.getGoogRequiresToAdd()) {
fix.addGoogRequire(match, require);
}

for (String require : matchedTemplate.getGoogRequiresToRemove()) {
fix.removeGoogRequire(match, require);
}
return ImmutableList.of(fix.build());
}

/**
* Transforms the template node to a replacement node by mapping the template names to
* the ones that were matched against in the JsSourceMatcher.
* Transforms the template node to a replacement node by mapping the template names to the ones
* that were matched against in the JsSourceMatcher.
*/
private Node transformNode(Node templateNode, Map<String, Node> templateNodeToMatchMap) {
private Node transformNode(
Node templateNode,
Map<String, Node> templateNodeToMatchMap,
Map<String, String> shortNames) {
Node clone = templateNode.cloneNode();
if (templateNode.isName()) {
String name = templateNode.getString();
Expand All @@ -201,8 +210,17 @@ private Node transformNode(Node templateNode, Map<String, Node> templateNodeToMa
clone.putBooleanProp(Node.FREE_CALL, false);
}
}
if (templateNode.isQualifiedName()) {
String name = templateNode.getQualifiedName();
if (shortNames.containsKey(name)) {
String shortName = shortNames.get(name);
if (!shortName.equals(name)) {
return IR.name(shortNames.get(name));
}
}
}
for (Node child : templateNode.children()) {
clone.addChildToBack(transformNode(child, templateNodeToMatchMap));
clone.addChildToBack(transformNode(child, templateNodeToMatchMap, shortNames));
}
return clone;
}
Expand Down

0 comments on commit cd1dd52

Please sign in to comment.