diff --git a/src/com/google/javascript/jscomp/ProcessClosureProvidesAndRequires.java b/src/com/google/javascript/jscomp/ProcessClosureProvidesAndRequires.java index e9730d947ef..d097f532864 100644 --- a/src/com/google/javascript/jscomp/ProcessClosureProvidesAndRequires.java +++ b/src/com/google/javascript/jscomp/ProcessClosureProvidesAndRequires.java @@ -331,7 +331,7 @@ private void processProvideCall(NodeTraversal t, Node n, Node parent) { if (providedNames.containsKey(ns)) { ProvidedName previouslyProvided = providedNames.get(ns); - if (!previouslyProvided.isExplicitlyProvided()) { + if (!previouslyProvided.isExplicitlyProvided() || previouslyProvided.isPreviouslyProvided) { previouslyProvided.addProvide(parent, t.getModule(), true); } else { String explicitSourceName = previouslyProvided.explicitNode.getSourceFileName(); @@ -373,6 +373,7 @@ private void handleStubDefinition(NodeTraversal t, Node n) { // This implies we're in hotswap pass and the current typedef is a provided namespace. ProvidedName provided = new ProvidedName(name, n, t.getModule(), true, true); providedNames.put(name, provided); + provided.addDefinition(n, t.getModule()); } } } @@ -420,21 +421,25 @@ private void deleteNamespaceInitializationsFromPreviousProvides() { *

TODO(b/128120127): delete this method */ private void processProvideFromPreviousPass(NodeTraversal t, String name, Node parent) { - if (!providedNames.containsKey(name)) { + JSModule module = t.getModule(); + if (providedNames.containsKey(name)) { + ProvidedName provided = providedNames.get(name); + provided.addDefinition(parent, module); + if (isNamespacePlaceholder(parent)) { + // Remove this later if it is a simple object literal. Replacing the corresponding + // ProvidedName will create a new definition. + // Don't add this as a 'definition' of the provided name to support pushing provides + // into earlier modules. + previouslyProvidedDefinitions.add(parent); + } + } else { // Record this provide created on a previous pass. This can happen if the previous pass had // goog.provide('foo.bar');, but all we have now is the rewritten `foo.bar = {};`. - - JSModule module = t.getModule(); registerAnyProvidedPrefixes(name, parent, module); ProvidedName provided = new ProvidedName(name, parent, module, true, true); providedNames.put(name, provided); provided.addDefinition(parent, module); - } else if (isNamespacePlaceholder(parent)) { - // Remove this later if it is a simple object literal. Replacing the corresponding - // ProvidedName will create a new definition. Note that non-object-literal definitions will - // not be removed and will be duplicates. - previouslyProvidedDefinitions.add(parent); } } @@ -660,7 +665,7 @@ class ProvidedName { void addProvide(Node node, JSModule module, boolean explicit) { if (explicit) { // goog.provide('name.space'); - checkState(explicitNode == null); + checkState(explicitNode == null || isPreviouslyProvided); checkArgument( node.isExprResult() || (NodeUtil.isNameDeclaration(node) && isPreviouslyProvided), node); @@ -682,6 +687,14 @@ boolean isFromExterns() { return explicitNode.isFromExterns(); } + private boolean hasCandidateDefinitionNotFromPreviousPass() { + // Exclude 'candidate definitions' that were added by previous pass runs because when + // rewriting provides, we can erase definitions that the compiler itself added, but not + // definitions that a user added. + return candidateDefinition != null + && !previouslyProvidedDefinitions.contains(candidateDefinition); + } + /** * Records function declaration, variable declarations, and assignments that refer to this * provided namespace. @@ -728,7 +741,7 @@ private void replace() { // Handle the case where there is a duplicate definition for an explicitly // provided symbol. - if (candidateDefinition != null && explicitNode != null) { + if (hasCandidateDefinitionNotFromPreviousPass() && explicitNode != null) { JSDocInfo info; if (candidateDefinition.isExprResult()) { info = candidateDefinition.getFirstChild().getJSDocInfo(); @@ -758,21 +771,16 @@ private void replace() { if (candidateDefinition.isExprResult()) { Node exprNode = candidateDefinition.getOnlyChild(); if (exprNode.isAssign()) { - // namespace = value; - candidateDefinition.putBooleanProp(Node.IS_NAMESPACE, true); Node nameNode = exprNode.getFirstChild(); if (nameNode.isName()) { - // Need to convert this assign to a var declaration. - Node valueNode = nameNode.getNext(); - exprNode.removeChild(nameNode); - exprNode.removeChild(valueNode); - nameNode.addChildToFront(valueNode); - Node varNode = IR.var(nameNode); - varNode.useSourceInfoFrom(candidateDefinition); - candidateDefinition.replaceWith(varNode); - varNode.setJSDocInfo(exprNode.getJSDocInfo()); - compiler.reportChangeToEnclosingScope(varNode); - replacementNode = varNode; + // In the case of a simple name, `name = value;`, we need to ensure the name is + // actually declared with `var`. + convertProvideAssignmentToVarDeclaration(exprNode, nameNode); + } else { + // `some.provided.namespace = value;` + // We don't need to change the definition, but mark it as 'IS_NAMESPACE' so that + // future passes know this was originally provided. + candidateDefinition.putBooleanProp(Node.IS_NAMESPACE, true); } } else { // /** @typedef {something} */ name.space.Type; @@ -802,6 +810,23 @@ private void replace() { } } + private void convertProvideAssignmentToVarDeclaration(Node assignNode, Node nameNode) { + // Convert `providedName = value;` into `var providedName = value;`. + checkArgument(assignNode.isAssign(), assignNode); + checkArgument(nameNode.isName(), nameNode); + Node valueNode = nameNode.getNext(); + assignNode.removeChild(nameNode); + assignNode.removeChild(valueNode); + + Node varNode = IR.var(nameNode, valueNode).useSourceInfoFrom(candidateDefinition); + varNode.setJSDocInfo(assignNode.getJSDocInfo()); + varNode.putBooleanProp(Node.IS_NAMESPACE, true); + + candidateDefinition.replaceWith(varNode); + replacementNode = varNode; + compiler.reportChangeToEnclosingScope(varNode); + } + /** Adds an assignment or declaration to this namespace to the AST, using the provided value */ private void createNamespaceInitialization(Node replacement) { replacementNode = replacement; @@ -853,7 +878,7 @@ private Node makeVarDeclNode(Node value) { if (compiler.getCodingConvention().isConstant(namespace)) { name.putBooleanProp(Node.IS_CONSTANT_NAME, true); } - if (candidateDefinition == null) { + if (!hasCandidateDefinitionNotFromPreviousPass()) { decl.setJSDocInfo(NodeUtil.createConstantJsDoc()); } @@ -872,7 +897,7 @@ private Node makeAssignmentExprNode(Node value) { namespace); Node decl = IR.exprResult(IR.assign(lhs, value)); decl.putBooleanProp(Node.IS_NAMESPACE, true); - if (candidateDefinition == null) { + if (!hasCandidateDefinitionNotFromPreviousPass()) { decl.getFirstChild().setJSDocInfo(NodeUtil.createConstantJsDoc()); } checkState(isNamespacePlaceholder(decl)); diff --git a/test/com/google/javascript/jscomp/ProcessClosureProvidesAndRequiresTest.java b/test/com/google/javascript/jscomp/ProcessClosureProvidesAndRequiresTest.java index fa98753d260..a038a85fe8b 100644 --- a/test/com/google/javascript/jscomp/ProcessClosureProvidesAndRequiresTest.java +++ b/test/com/google/javascript/jscomp/ProcessClosureProvidesAndRequiresTest.java @@ -393,6 +393,11 @@ public void testProvideAfterDeclarationError() { test("var x = 42; goog.provide('x');", "var x = 42; /** @const */ var x = {}"); } + @Test + public void testProvideAfterDeclaration_noErrorInExterns() { + test(externs("var x = {};"), srcs("goog.provide('x');"), expected("/** @const */ var x = {}")); + } + @Test public void testProvideErrorCases() { testError("goog.provide('foo'); goog.provide('foo');", DUPLICATE_NAMESPACE_ERROR); @@ -401,6 +406,21 @@ public void testProvideErrorCases() { DUPLICATE_NAMESPACE_ERROR); } + @Test + public void testPreserveGoogProvidesAndRequires_withSecondPassRun_noDuplicateNamespaceWarning() { + additionalCode = ""; + preserveGoogProvidesAndRequires = true; + + test( + lines( + "goog.provide('foo');", // + "foo.baz = function() {};"), + lines( + "/** @const */ var foo = {};", // + "goog.provide('foo');", + "foo.baz = function() {};")); + } + @Test public void testProvideErrorCases2() { test( @@ -606,6 +626,73 @@ public void testSimpleAdditionalProvideAtEnd() { "/** @const */ var a={}; a.A={}; /** @const */ var b={};b.B={};"); } + @Test + public void testSimpleAdditionalProvide_withPreserveGoogProvidesAndRequires() { + additionalCode = "goog.provide('b.B'); b.B = {};"; + preserveGoogProvidesAndRequires = true; + + test( + "goog.provide('a.A'); a.A = {};", + lines( + "/** @const */ var b={};", + "goog.provide('b.B');", + "b.B={};", + "/** @const */ var a={};", + "/** @const */ a.A={};", + "goog.provide('a.A'); ")); + } + + @Test + public void testNonNamespaceAdditionalProvide_withPreserveGoogProvidesAndRequires() { + additionalCode = "goog.provide('b.B'); b.B = {};"; + preserveGoogProvidesAndRequires = true; + + test( + "goog.provide('a.A'); a.A = function() {}", + lines( + "/** @const */ var b={};", + "goog.provide('b.B');", + "b.B={};", + "/** @const */ var a={};", + "goog.provide('a.A');", + "a.A = function() {};")); + } + + @Test + public void testNamespaceInExterns() { + // Note: This style is not recommended but the compiler sort-of supports it. + test( + externs("var root = {}; /** @type {number} */ root.someProperty;"), + srcs( + lines( + "goog.provide('root.branch.Leaf')", // + "root.branch.Leaf = class {};")), + expected( + lines( + "/** @const */", + "var root = {};", + "/** @const */ root.branch = {};", + "root.branch.Leaf = class {};"))); + } + + @Test + public void testNamespaceInExterns_withExplicitNamespaceReinitialization() { + // Note: This style is not recommended but the compiler sort-of supports it. + test( + externs("var root = {}; /** @type {number} */ root.someProperty;"), + srcs( + lines( + "goog.provide('root.branch.Leaf')", // + "var root = {};", + "root.branch.Leaf = class {};")), + expected( + lines( + "var root = {};", + "/** @const */ root.branch = {};", + "var root = {};", + "root.branch.Leaf = class {};"))); + } + // Tests providing additional code with non-overlapping dotted namespace. @Test public void testSimpleDottedAdditionalProvide() { @@ -625,7 +712,38 @@ public void testSimpleDottedAdditionalProvide() { } @Test - public void testTypedefAdditionalProvide() { + public void testAdditionalCode_onSingleNameNamespaceWithoutVar() { + additionalEndCode = "goog.provide('Name.child'); Name.child = 1;"; + + test( + lines( + "goog.provide('Name');", // + "Name = class {};"), + lines( + "var Name = class {};", // + "Name.child = 1;")); + } + + @Test + public void testTypedefProvide() { + test( + lines( + "goog.provide('foo.Bar');", + "goog.provide('foo.Bar.Baz');", + "/** @typedef {!Array} */", + "foo.Bar;", + "foo.Bar.Baz = {};"), + lines( + "/** @const */ var foo = {};", // + // Cast to unknown to support also having @typedef. We want the type system to treat + // this as a typedef, but need an actual namespace to hang foo.Bar.Baz on. + "foo.Bar = /** @type {?} */ ({});", + "/** @typedef {!Array} */ foo.Bar;", + "foo.Bar.Baz = {}")); + } + + @Test + public void testTypedefAdditionalProvide_noChildNamespace() { additionalEndCode = "goog.require('foo.Cat'); goog.require('foo.Bar');"; test( @@ -637,9 +755,29 @@ public void testTypedefAdditionalProvide() { "foo.Cat={};"), lines( "/** @const */ var foo={};", // - "/** @const */ foo.Bar={};", // "/** @typedef {!Array} */ foo.Bar;", // - "foo.Cat={}")); + "foo.Cat = {};")); + } + + @Test + public void testTypedefAdditionalProvide_withChildNamespace() { + additionalEndCode = "goog.require('foo.Cat'); goog.require('foo.Bar');"; + + test( + lines( + "goog.provide('foo.Cat');", + "goog.provide('foo.Bar');", + "goog.provide('foo.Bar.Baz');", + "/** @typedef {!Array} */", + "foo.Bar;", + "foo.Cat={};", + "foo.Bar.Baz = {};"), + lines( + "/** @const */ var foo={};", // + "foo.Bar = /** @type {?} */ ({});", // + "/** @typedef {!Array} */ foo.Bar;", // + "foo.Cat = {};", + "foo.Bar.Baz = {};")); } // Tests providing additional code with overlapping var namespace. @@ -752,7 +890,6 @@ public void testProvideOrder1() { lines( "/** @const */", "var a = {};", - "/** @const */", "a.b = {};", "/** @const */", "a.b.c = {};", @@ -775,7 +912,6 @@ public void testProvideOrder2() { lines( "/** @const */", "var a = {};", - "/** @const */", "a.b = {};", "/** @const */", "a.b.c = {};",