From 3ee67b1cdcd52074c887fca317ce9a290430e2bf Mon Sep 17 00:00:00 2001 From: sdh Date: Mon, 28 Jan 2019 15:17:34 -0800 Subject: [PATCH] Allow assigning the result of goog.define Adds a new AST property to store the identifier passed to goog.define, since the string is otherwise thrown away from the AST. Updates ProcessClosurePrimitives to no longer throw an error for this case. Updates ProcessDefines to look for and use the property. This is important to support using goog.define in modules, where we no longer want to assign a global value. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=231298527 --- .../javascript/jscomp/DefaultPassConfig.java | 20 +- .../jscomp/J2clUtilGetDefineRewriterPass.java | 2 +- .../jscomp/ProcessClosurePrimitives.java | 63 +++++-- .../javascript/jscomp/ProcessDefines.java | 171 ++++++++++-------- src/com/google/javascript/rhino/Node.java | 12 ++ .../jscomp/ProcessClosurePrimitivesTest.java | 24 ++- .../javascript/jscomp/ProcessDefinesTest.java | 33 +++- 7 files changed, 201 insertions(+), 124 deletions(-) diff --git a/src/com/google/javascript/jscomp/DefaultPassConfig.java b/src/com/google/javascript/jscomp/DefaultPassConfig.java index f3c72de2275..b46b533484c 100644 --- a/src/com/google/javascript/jscomp/DefaultPassConfig.java +++ b/src/com/google/javascript/jscomp/DefaultPassConfig.java @@ -32,7 +32,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.javascript.jscomp.AbstractCompiler.LifeCycleStage; import com.google.javascript.jscomp.CompilerOptions.ExtractPrototypeMemberDeclarationsMode; @@ -2158,18 +2157,13 @@ protected FeatureSet featureSet() { new PassFactory("processDefines", true) { @Override protected CompilerPass create(final AbstractCompiler compiler) { - return new CompilerPass() { - @Override - public void process(Node externs, Node jsRoot) { - HashMap replacements = new HashMap<>(); - replacements.putAll(compiler.getDefaultDefineValues()); - replacements.putAll(getAdditionalReplacements(options)); - replacements.putAll(options.getDefineReplacements()); - new ProcessDefines(compiler, ImmutableMap.copyOf(replacements), !options.checksOnly) - .injectNamespace(namespaceForChecks) - .process(externs, jsRoot); - } - }; + return new ProcessDefines.Builder(compiler) + .putReplacements(compiler.getDefaultDefineValues()) + .putReplacements(getAdditionalReplacements(options)) + .putReplacements(options.getDefineReplacements()) + .checksOnly(options.checksOnly) + .injectNamespace(() -> namespaceForChecks) + .build(); } @Override diff --git a/src/com/google/javascript/jscomp/J2clUtilGetDefineRewriterPass.java b/src/com/google/javascript/jscomp/J2clUtilGetDefineRewriterPass.java index f57837be863..6bfe8c9666e 100644 --- a/src/com/google/javascript/jscomp/J2clUtilGetDefineRewriterPass.java +++ b/src/com/google/javascript/jscomp/J2clUtilGetDefineRewriterPass.java @@ -35,7 +35,7 @@ public void process(Node externs, Node root) { if (!J2clSourceFileChecker.shouldRunJ2clPasses(compiler)) { return; } - defines = new ProcessDefines(compiler, null, true).collectDefines(externs, root).keySet(); + defines = new ProcessDefines.Builder(compiler).build().collectDefines(externs, root).keySet(); NodeTraversal.traverse(compiler, root, this); } diff --git a/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java b/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java index 4fa30555716..6cf1a63f0dc 100644 --- a/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java +++ b/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java @@ -272,16 +272,32 @@ private Node getAnyValueOfType(JSDocInfo jsdoc) { */ private void replaceGoogDefines(Node n) { Node parent = n.getParent(); - checkState(parent.isExprResult()); String name = n.getSecondChild().getString(); JSDocInfo jsdoc = n.getJSDocInfo(); Node value = n.isFromExterns() ? getAnyValueOfType(jsdoc).srcref(n) : n.getChildAtIndex(2).detach(); - Node replacement = NodeUtil.newQNameDeclaration(compiler, name, value, jsdoc); - replacement.useSourceInfoIfMissingFromForTree(parent); - parent.replaceWith(replacement); - compiler.reportChangeToEnclosingScope(replacement); + switch (parent.getToken()) { + case EXPR_RESULT: + Node replacement = NodeUtil.newQNameDeclaration(compiler, name, value, jsdoc); + replacement.useSourceInfoIfMissingFromForTree(parent); + parent.replaceWith(replacement); + compiler.reportChangeToEnclosingScope(replacement); + break; + case NAME: + parent.setDefineName(name); + n.replaceWith(value); + compiler.reportChangeToEnclosingScope(parent); + break; + case ASSIGN: + checkState(n == parent.getLastChild()); + parent.getFirstChild().setDefineName(name); + n.replaceWith(value); + compiler.reportChangeToEnclosingScope(parent); + break; + default: + throw new IllegalStateException("goog.define outside of EXPR_RESULT, NAME, or ASSIGN"); + } } @Override @@ -308,9 +324,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { processBaseClassCall(t, n); break; case "define": - if (validateUnaliasablePrimitiveCall(t, n, methodName)) { - processDefineCall(t, n, parent); - } + processDefineCall(t, n, parent); break; case "require": case "requireType": @@ -448,7 +462,7 @@ private boolean validatePrimitiveCallWithMessage( if (!t.inGlobalHoistScope()) { compiler.report(t.makeError(n, INVALID_CLOSURE_CALL_SCOPE_ERROR)); return false; - } else if (!n.getParent().isExprResult()) { + } else if (!n.getParent().isExprResult() && !"goog.define".equals(methodName)) { // If the call is in the global hoist scope, but the result is used compiler.report(t.makeError(n, invalidAliasingError, GOOG + "." + methodName)); return false; @@ -1142,15 +1156,30 @@ private boolean verifyProvide(NodeTraversal t, Node methodName, Node arg) { } /** - * Verifies that a provide method call has exactly one argument, - * and that it's a string literal and that the contents of the string are - * valid JS tokens. Reports a compile error if it doesn't. + * Verifies that a goog.define method call has exactly two arguments, with the first a string + * literal whose contents is a valid JS qualified name. Reports a compile error if it doesn't. * * @return Whether the argument checked out okay */ - private boolean verifyDefine(NodeTraversal t, - Node expr, - Node methodName, Node args) { + private boolean verifyDefine(NodeTraversal t, Node parent, Node methodName, Node args) { + // Calls to goog.define must be in the global hoist scope. This is copied from + // validate(Un)aliasablePrimitiveCall. + // TODO(sdh): loosen this restriction if the results are assigned? + if (!compiler.getOptions().shouldPreserveGoogModule() && !t.inGlobalHoistScope()) { + compiler.report(t.makeError(methodName.getParent(), INVALID_CLOSURE_CALL_SCOPE_ERROR)); + return false; + } + + // It is an error for goog.define to show up anywhere except on its own or immediately after =. + if (parent.isAssign() && parent.getParent().isExprResult()) { + parent = parent.getParent(); + } + if (parent.isName() && NodeUtil.isNameDeclaration(parent.getParent())) { + parent = parent.getGrandparent(); + } else if (!parent.isExprResult()) { + compiler.report(t.makeError(methodName.getParent(), INVALID_CLOSURE_CALL_SCOPE_ERROR)); + return false; + } // Verify first arg Node arg = args; @@ -1172,9 +1201,9 @@ private boolean verifyDefine(NodeTraversal t, return false; } - JSDocInfo info = expr.getFirstChild().getJSDocInfo(); + JSDocInfo info = parent.getFirstChild().getJSDocInfo(); if (info == null || !info.isDefine()) { - compiler.report(t.makeError(expr, MISSING_DEFINE_ANNOTATION)); + compiler.report(t.makeError(parent, MISSING_DEFINE_ANNOTATION)); return false; } return true; diff --git a/src/com/google/javascript/jscomp/ProcessDefines.java b/src/com/google/javascript/jscomp/ProcessDefines.java index cafb3db1a7c..0bd45b3c4f7 100644 --- a/src/com/google/javascript/jscomp/ProcessDefines.java +++ b/src/com/google/javascript/jscomp/ProcessDefines.java @@ -19,6 +19,8 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.javascript.rhino.jstype.JSTypeNative.NUMBER_STRING_BOOLEAN; +import com.google.common.base.MoreObjects; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.javascript.jscomp.GlobalNamespace.Name; @@ -34,9 +36,11 @@ import java.util.ArrayList; import java.util.Deque; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Supplier; import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.Nullable; @@ -61,9 +65,8 @@ class ProcessDefines implements CompilerPass { private final AbstractCompiler compiler; private final Map dominantReplacements; - private final boolean doReplacements; - - private GlobalNamespace namespace = null; + private final boolean checksOnly; + private final Supplier namespaceSupplier; // Warnings static final DiagnosticType UNKNOWN_DEFINE_WARNING = DiagnosticType.warning( @@ -94,30 +97,48 @@ class ProcessDefines implements CompilerPass { private static final MessageFormat REASON_DEFINE_NOT_ASSIGNABLE = new MessageFormat("line {0} of {1}"); - /** - * Create a pass that overrides define constants. - * - * TODO(nicksantos): Write a builder to help JSCompiler induce - * {@code replacements} from command-line flags - * - * @param replacements A hash table of names of defines to their replacements. - * All replacements must be literals. - */ - ProcessDefines( - AbstractCompiler compiler, Map replacements, boolean doReplacements) { - this.compiler = compiler; - this.dominantReplacements = replacements; - this.doReplacements = doReplacements; + /** Create a pass that overrides define constants. */ + private ProcessDefines(Builder builder) { + this.compiler = builder.compiler; + this.dominantReplacements = ImmutableMap.copyOf(builder.replacements); + this.checksOnly = builder.checksOnly; + this.namespaceSupplier = builder.namespaceSupplier; } - /** - * Injects a pre-computed global namespace, so that the same namespace - * can be re-used for multiple check passes. Returns {@code this} for - * easy chaining. - */ - ProcessDefines injectNamespace(GlobalNamespace namespace) { - this.namespace = namespace; - return this; + /** Builder for ProcessDefines. */ + static class Builder { + private final AbstractCompiler compiler; + private final Map replacements = new LinkedHashMap<>(); + private boolean checksOnly; + private Supplier namespaceSupplier; + + Builder(AbstractCompiler compiler) { + this.compiler = compiler; + } + + Builder putReplacements(Map replacements) { + this.replacements.putAll(replacements); + return this; + } + + Builder checksOnly(boolean checksOnly) { + this.checksOnly = checksOnly; + return this; + } + + /** + * Injects a pre-computed global namespace, so that the same namespace can be re-used for + * multiple check passes. Accepts a supplier because the namespace may not exist at + * pass-creation time. + */ + Builder injectNamespace(Supplier namespaceSupplier) { + this.namespaceSupplier = namespaceSupplier; + return this; + } + + ProcessDefines build() { + return new ProcessDefines(this); + } } @Override @@ -126,7 +147,7 @@ public void process(Node externs, Node root) { } private void overrideDefines(Map allDefines) { - if (doReplacements) { + if (!checksOnly) { for (Map.Entry def : allDefines.entrySet()) { String defineName = def.getKey(); DefineInfo info = def.getValue(); @@ -173,15 +194,24 @@ private boolean isValidDefineType(JSTypeExpression expression) { } /** - * Finds all defines, and creates a {@link DefineInfo} data structure for - * each one. + * Finds all defines, and creates a {@link DefineInfo} data structure for each one. + * * @return A map of {@link DefineInfo} structures, keyed by name. */ Map collectDefines(Node externs, Node root) { + GlobalNamespace namespace = null; + if (namespaceSupplier != null) { + namespace = namespaceSupplier.get(); + } if (namespace == null) { namespace = new GlobalNamespace(compiler, externs, root); } + // namespace = + // namespaceSupplier != nul + // ? namespaceSupplier.get() + // : new GlobalNamespace(compiler, externs, root); + // Find all the global names with a @define annotation List allDefines = new ArrayList<>(); for (Name name : namespace.getNameIndex().values()) { @@ -218,18 +248,14 @@ Map collectDefines(Node externs, Node root) { } } - CollectDefines pass = new CollectDefines(compiler, allDefines); + CollectDefines pass = new CollectDefines(allDefines); NodeTraversal.traverseRoots(compiler, pass, externs, root); return pass.getAllDefines(); } - /** - * Finds all assignments to @defines, and figures out the last value of - * the @define. - */ - private static final class CollectDefines implements Callback { + /** Finds all assignments to @defines, and figures out the last value of the @define. */ + private final class CollectDefines implements Callback { - private final AbstractCompiler compiler; private final Map assignableDefines; private final Map allDefines; private final Map allRefInfo; @@ -243,8 +269,7 @@ private static final class CollectDefines implements Callback { // a define is allowed. Otherwise, it's not allowed. private final Deque assignAllowed; - CollectDefines(AbstractCompiler compiler, List listOfDefines) { - this.compiler = compiler; + CollectDefines(List listOfDefines) { this.allDefines = new HashMap<>(); assignableDefines = new HashMap<>(); @@ -298,7 +323,12 @@ public void visit(NodeTraversal t, Node n, Node parent) { if (refInfo != null) { Ref ref = refInfo.ref; Name name = refInfo.name; - String fullName = name.getFullName(); + // If the (qualified) name node had a DEFINE_NAME prop added to it (i.e. by the closure + // pass) then use that name instead of the name assigned in the AST. This happens any time + // the result of goog.define is assigned to something (i.e. all the time, once it stops + // exporting the global variable). This allows goog.define to have more flexibility than + // simple @define. + String fullName = MoreObjects.firstNonNull(ref.node.getDefineName(), name.getFullName()); switch (ref.type) { case SET_FROM_GLOBAL: case SET_FROM_LOCAL: @@ -454,45 +484,40 @@ private boolean processDefineAssignment(NodeTraversal t, return false; } + } - /** - * Gets the parent node of the value for any assignment to a Name. - * For example, in the assignment - * {@code var x = 3;} - * the parent would be the NAME node. - */ - private static Node getValueParent(Ref ref) { - // there are two types of declarations: VARs, ASSIGNs, and CONSTs - return ref.node.getParent() != null - && (ref.node.getParent().isVar() || ref.node.getParent().isConst()) - ? ref.node - : ref.node.getParent(); - } + /** + * Gets the parent node of the value for any assignment to a Name. For example, in the assignment + * {@code var x = 3;} the parent would be the NAME node. + */ + private static Node getValueParent(Ref ref) { + // there are two types of declarations: VARs, ASSIGNs, and CONSTs + return ref.node.getParent() != null + && (ref.node.getParent().isVar() || ref.node.getParent().isConst()) + ? ref.node + : ref.node.getParent(); + } - /** - * Records the fact that because of the current node in the node traversal, - * the define can't ever be assigned again. - * - * @param info Represents the define variable. - * @param t The current traversal. - */ - private static void setDefineInfoNotAssignable(DefineInfo info, NodeTraversal t) { - info.setNotAssignable(format(REASON_DEFINE_NOT_ASSIGNABLE, - t.getLineNumber(), t.getSourceName())); - } + /** + * Records the fact that because of the current node in the node traversal, the define can't ever + * be assigned again. + * + * @param info Represents the define variable. + * @param t The current traversal. + */ + private static void setDefineInfoNotAssignable(DefineInfo info, NodeTraversal t) { + info.setNotAssignable( + format(REASON_DEFINE_NOT_ASSIGNABLE, t.getLineNumber(), t.getSourceName())); + } - /** - * A simple data structure for associating a Ref with the name - * that it references. - */ - private static class RefInfo { - final Ref ref; - final Name name; + /** A simple data structure for associating a Ref with the name that it references. */ + private static class RefInfo { + final Ref ref; + final Name name; - RefInfo(Ref ref, Name name) { - this.ref = ref; - this.name = name; - } + RefInfo(Ref ref, Name name) { + this.ref = ref; + this.name = name; } } diff --git a/src/com/google/javascript/rhino/Node.java b/src/com/google/javascript/rhino/Node.java index 2535adcd898..803e497c0c6 100644 --- a/src/com/google/javascript/rhino/Node.java +++ b/src/com/google/javascript/rhino/Node.java @@ -201,6 +201,8 @@ private enum Prop { // Record the type associated with a @typedef to enable looking up typedef in the AST possible // without saving the type scope. TYPEDEF_TYPE, + // Original name of a goog.define call. + DEFINE_NAME, } // TODO(sdh): Get rid of these by using accessor methods instead. @@ -2828,6 +2830,16 @@ public final boolean isEs6Class() { return isClass() || getBooleanProp(Prop.IS_ES6_CLASS); } + /** Returns any goog.define'd name corresponding to this NAME or GETPROP node. */ + public final String getDefineName() { + return (String) getProp(Prop.DEFINE_NAME); + } + + /** Sets the goog.define name for a NAME or GETPROP node. */ + public final void setDefineName(String name) { + putProp(Prop.DEFINE_NAME, name); + } + // There are four values of interest: // global state changes // this state changes diff --git a/test/com/google/javascript/jscomp/ProcessClosurePrimitivesTest.java b/test/com/google/javascript/jscomp/ProcessClosurePrimitivesTest.java index 8240cfbb8c4..214cd9cad47 100644 --- a/test/com/google/javascript/jscomp/ProcessClosurePrimitivesTest.java +++ b/test/com/google/javascript/jscomp/ProcessClosurePrimitivesTest.java @@ -1063,19 +1063,6 @@ public void testInvalidForwardDeclare() { INVALID_CLOSURE_CALL_SCOPE_ERROR); } - @Test - public void testInvalidDefine() { - testError( - "goog.provide('a.b'); var x = x || goog.define('goog.DEBUG', true);", - CLOSURE_CALL_CANNOT_BE_ALIASED_ERROR); - testError( - "goog.provide('a.b'); x = goog.define('goog.DEBUG', true);", - CLOSURE_CALL_CANNOT_BE_ALIASED_ERROR); - testError( - "goog.provide('a.b'); function f() { goog.define('goog.DEBUG', true); }", - INVALID_CLOSURE_CALL_SCOPE_ERROR); - } - @Test public void testInvalidAddDependency() { testError( @@ -1578,6 +1565,7 @@ public void testDefineCases() { String jsdoc = "/** @define {number} */\n"; test(jsdoc + "goog.define('name', 1);", jsdoc + "var name = 1"); test(jsdoc + "goog.define('ns.name', 1);", jsdoc + "ns.name = 1"); + test(jsdoc + "const x = goog.define('ns.name', 1);", jsdoc + "const x = 1"); } @Test @@ -1607,6 +1595,16 @@ public void testDefineInExterns() { testErrorExterns("/** @define {!number} */ goog.define('name');"); } + @Test + public void testInvalidDefine() { + testError( + "goog.provide('a.b'); var x = x || goog.define('goog.DEBUG', true);", + INVALID_CLOSURE_CALL_SCOPE_ERROR); + testError( + "goog.provide('a.b'); function f() { goog.define('goog.DEBUG', true); }", + INVALID_CLOSURE_CALL_SCOPE_ERROR); + } + private void testErrorExterns(String externs) { testNoWarning(externs, ""); } diff --git a/test/com/google/javascript/jscomp/ProcessDefinesTest.java b/test/com/google/javascript/jscomp/ProcessDefinesTest.java index 2eb785445e2..2f046ba2cef 100644 --- a/test/com/google/javascript/jscomp/ProcessDefinesTest.java +++ b/test/com/google/javascript/jscomp/ProcessDefinesTest.java @@ -38,14 +38,14 @@ public ProcessDefinesTest() { private final Map overrides = new HashMap<>(); private GlobalNamespace namespace; - private boolean doReplacements; + private boolean checksOnly; @Override @Before public void setUp() throws Exception { super.setUp(); overrides.clear(); - doReplacements = true; + checksOnly = false; // ProcessDefines emits warnings if the user tries to re-define a constant, // but the constant is not defined anywhere in the binary. @@ -94,13 +94,13 @@ public void testDefineBadType() { @Test public void testChecksOnlyProducesErrors() { - doReplacements = false; + checksOnly = true; testError("/** @define {Object} */ var DEF = {}", ProcessDefines.INVALID_DEFINE_TYPE_ERROR); } @Test public void testChecksOnlyProducesUnknownDefineWarning() { - doReplacements = false; + checksOnly = true; overrides.put("a.B", new Node(Token.TRUE)); test("var a = {};", "var a = {};", warning(ProcessDefines.UNKNOWN_DEFINE_WARNING)); } @@ -418,6 +418,21 @@ public void testNamespacedDefine4() { "var a = {}; /** @define {boolean} */ a.B = true;"); } + @Test + public void testGoogDefine_notOverridden() { + test( + "/** @define {boolean} */ const B = goog.define('a.B', false);", + "/** @define {boolean} */ const B = false;"); + } + + @Test + public void testGoogDefine_overridden() { + overrides.put("a.B", new Node(Token.TRUE)); + test( + "/** @define {boolean} */ const B = goog.define('a.B', false);", + "/** @define {boolean} */ const B = true;"); + } + @Test public void testOverrideAfterAlias() { testError("var x; /** @define {boolean} */var DEF=true; x=DEF; DEF=false;", @@ -448,7 +463,7 @@ public void testConstOverriding2() { @Test public void testConstProducesUnknownDefineWarning() { - doReplacements = false; + checksOnly = true; overrides.put("a.B", new Node(Token.TRUE)); test("const a = {};", "const a = {};", warning(ProcessDefines.UNKNOWN_DEFINE_WARNING)); } @@ -479,9 +494,13 @@ public ProcessDefinesWithInjectedNamespace(Compiler compiler) { @Override public void process(Node externs, Node js) { + new ProcessClosurePrimitives(compiler, null, CheckLevel.ERROR, true).process(externs, js); namespace = new GlobalNamespace(compiler, externs, js); - new ProcessDefines(compiler, overrides, doReplacements) - .injectNamespace(namespace) + new ProcessDefines.Builder(compiler) + .putReplacements(overrides) + .checksOnly(checksOnly) + .injectNamespace(() -> namespace) + .build() .process(externs, js); } }