Skip to content

Commit

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

Caused problems for internal Google projects.

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


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


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


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


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


if (currentScript.isModule || targetIsNonLegacyGoogModule) { if (currentScript.isModule || targetIsNonLegacyGoogModule) {
if (isDestructuring) { if (isDestructuring) {
if (!preserveSugar) { // Delete the goog.require() because we're going to inline its alias later.
// Delete the goog.require() because we're going to inline its alias later. compiler.reportChangeToEnclosingScope(statementNode);
compiler.reportChangeToEnclosingScope(statementNode); statementNode.detach();
statementNode.detach();
}
} else if (targetIsNonLegacyGoogModule) { } else if (targetIsNonLegacyGoogModule) {
if (!isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) { if (!isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {
// Rewrite // Rewrite
Expand All @@ -1073,15 +1069,11 @@ && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {
call.replaceWith(binaryNamespaceName); call.replaceWith(binaryNamespaceName);
compiler.reportChangeToEnclosingScope(binaryNamespaceName); compiler.reportChangeToEnclosingScope(binaryNamespaceName);
} else if (importHasAlias || !rewriteState.isLegacyModule(legacyNamespace)) { } else if (importHasAlias || !rewriteState.isLegacyModule(legacyNamespace)) {
if (!preserveSugar) { // Delete the goog.require() because we're going to inline its alias later.
// Delete the goog.require() because we're going to inline its alias later. compiler.reportChangeToEnclosingScope(statementNode);
compiler.reportChangeToEnclosingScope(statementNode); statementNode.detach();
statementNode.detach();
}
} }
} else { } else {
// TODO(bangert): make this compatible with preserveSugar. const B = goog.require('b') runs
// into problems because the type checker cannot handle const.
// Rewrite // Rewrite
// "var B = goog.require('B');" to // "var B = goog.require('B');" to
// "goog.require('B');" // "goog.require('B');"
Expand All @@ -1091,7 +1083,7 @@ && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {
statementNode.replaceWith(IR.exprResult(call)); statementNode.replaceWith(IR.exprResult(call));
compiler.reportChangeToEnclosingScope(call); compiler.reportChangeToEnclosingScope(call);
} }
if (targetIsNonLegacyGoogModule && !preserveSugar) { if (targetIsNonLegacyGoogModule) {
// Add goog.require() and namespace name to preprocessor table because they're removed // 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 // by current pass. If target is not a module then goog.require() is retained for
// ProcessClosurePrimitives pass and symbols will be added there instead. // ProcessClosurePrimitives pass and symbols will be added there instead.
Expand Down Expand Up @@ -1480,6 +1472,7 @@ private void maybeSplitMultiVar(Node rhsNode) {
Node nameNode = rhsNode.getParent(); Node nameNode = rhsNode.getParent();
nameNode.detach(); nameNode.detach();
rhsNode.detach(); rhsNode.detach();

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


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


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


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


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


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

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


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


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


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

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


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


/** /**
Expand Down
9 changes: 3 additions & 6 deletions src/com/google/javascript/refactoring/RefactoringDriver.java
View file
Expand Up @@ -106,13 +106,10 @@ public static CompilerOptions getCompilerOptions() {
options.setCheckSuspiciousCode(true); options.setCheckSuspiciousCode(true);
options.setCheckSymbols(true); options.setCheckSymbols(true);
options.setCheckTypes(true); options.setCheckTypes(true);
options.setBrokenClosureRequiresLevel(CheckLevel.OFF); options.setClosurePass(true);
// 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.setGenerateExports(true);
options.setPreserveClosurePrimitives(true); options.setPreserveGoogProvidesAndRequires(true);

options.setWarningLevel(DiagnosticGroups.STRICT_MISSING_REQUIRE, CheckLevel.WARNING); options.setWarningLevel(DiagnosticGroups.STRICT_MISSING_REQUIRE, CheckLevel.WARNING);
options.setWarningLevel(DiagnosticGroups.EXTRA_REQUIRE, CheckLevel.WARNING); options.setWarningLevel(DiagnosticGroups.EXTRA_REQUIRE, CheckLevel.WARNING);
options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, CheckLevel.WARNING); options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, CheckLevel.WARNING);
Expand Down
36 changes: 9 additions & 27 deletions src/com/google/javascript/refactoring/RefasterJsScanner.java
View file
Expand Up @@ -29,7 +29,6 @@
import com.google.javascript.jscomp.NodeUtil; import com.google.javascript.jscomp.NodeUtil;
import com.google.javascript.jscomp.SourceFile; import com.google.javascript.jscomp.SourceFile;
import com.google.javascript.jscomp.TypeMatchingStrategy; import com.google.javascript.jscomp.TypeMatchingStrategy;
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.JSDocInfo; import com.google.javascript.rhino.JSDocInfo;
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.TypeIRegistry; import com.google.javascript.rhino.TypeIRegistry;
Expand Down Expand Up @@ -141,18 +140,10 @@ public List<SuggestedFix> processMatch(Match match) {
.isEquivalentTo(matchedTemplate.afterTemplate.getLastChild())) { .isEquivalentTo(matchedTemplate.afterTemplate.getLastChild())) {
return ImmutableList.of(); 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 = Node newNode =
transformNode( transformNode(
matchedTemplate.afterTemplate.getLastChild(), matchedTemplate.afterTemplate.getLastChild(),
matchedTemplate.matcher.getTemplateNodeToMatchMap(), matchedTemplate.matcher.getTemplateNodeToMatchMap());
shortNames);
Node nodeToReplace = match.getNode(); Node nodeToReplace = match.getNode();
fix.attachMatchedNodeInfo(nodeToReplace, match.getMetadata().getCompiler()); fix.attachMatchedNodeInfo(nodeToReplace, match.getMetadata().getCompiler());
fix.replace(nodeToReplace, newNode, match.getMetadata().getCompiler()); fix.replace(nodeToReplace, newNode, match.getMetadata().getCompiler());
Expand All @@ -170,21 +161,21 @@ public List<SuggestedFix> processMatch(Match match) {
fix.delete(n); fix.delete(n);
n = n.getNext(); n = n.getNext();
} }

// Add/remove any goog.requires
for (String require : matchedTemplate.getGoogRequiresToAdd()) {
fix.addGoogRequire(match, require);
}
for (String require : matchedTemplate.getGoogRequiresToRemove()) { for (String require : matchedTemplate.getGoogRequiresToRemove()) {
fix.removeGoogRequire(match, require); fix.removeGoogRequire(match, require);
} }
return ImmutableList.of(fix.build()); return ImmutableList.of(fix.build());
} }


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

0 comments on commit af285b5

Please sign in to comment.