Skip to content

Commit

Permalink
When processing rewritten code, add generated provide definitions to …
Browse files Browse the repository at this point in the history 
…the ProvidedName

This pass has special handling for erasing compiler-generated
definitions of provides when running on previously rewritten code.
Previously, ProvidedNames did not even track those definitions; now they
do.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=242523120
  • Loading branch information
@lauraharker @brad4d
lauraharker authored and brad4d committed Apr 9, 2019
1 parent d1749c1 commit e036fe1
Show file tree
Hide file tree
Showing 2 changed files with 192 additions and 31 deletions.
77 changes: 51 additions & 26 deletions src/com/google/javascript/jscomp/ProcessClosureProvidesAndRequires.java
View file
Expand Up @@ -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();
Expand Down Expand Up @@ -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());
}
}
}
Expand Down Expand Up @@ -420,21 +421,25 @@ private void deleteNamespaceInitializationsFromPreviousProvides() {
* <p>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);
}
}

Expand Down Expand Up @@ -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);
Expand All @@ -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.
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}

Expand All @@ -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));
Expand Down
146 changes: 141 additions & 5 deletions test/com/google/javascript/jscomp/ProcessClosureProvidesAndRequiresTest.java
View file
Expand Up @@ -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);
Expand All @@ -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(
Expand Down Expand Up @@ -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() {
Expand All @@ -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<string>} */",
"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<string>} */ foo.Bar;",
"foo.Bar.Baz = {}"));
}

@Test
public void testTypedefAdditionalProvide_noChildNamespace() {
additionalEndCode = "goog.require('foo.Cat'); goog.require('foo.Bar');";

test(
Expand All @@ -637,9 +755,29 @@ public void testTypedefAdditionalProvide() {
"foo.Cat={};"),
lines(
"/** @const */ var foo={};", //
"/** @const */ foo.Bar={};", //
"/** @typedef {!Array<string>} */ 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<string>} */",
"foo.Bar;",
"foo.Cat={};",
"foo.Bar.Baz = {};"),
lines(
"/** @const */ var foo={};", //
"foo.Bar = /** @type {?} */ ({});", //
"/** @typedef {!Array<string>} */ foo.Bar;", //
"foo.Cat = {};",
"foo.Bar.Baz = {};"));
}

// Tests providing additional code with overlapping var namespace.
Expand Down Expand Up @@ -752,7 +890,6 @@ public void testProvideOrder1() {
lines(
"/** @const */",
"var a = {};",
"/** @const */",
"a.b = {};",
"/** @const */",
"a.b.c = {};",
Expand All @@ -775,7 +912,6 @@ public void testProvideOrder2() {
lines(
"/** @const */",
"var a = {};",
"/** @const */",
"a.b = {};",
"/** @const */",
"a.b.c = {};",
Expand Down

0 comments on commit e036fe1

Please sign in to comment.