Skip to content

Commit

Permalink
Update .i.js generation to handle assigning the result of goog.define
Browse files Browse the repository at this point in the history 
Also removes the special case for value-less goog.defines in externs by instead generating the empty value in the .i.js file.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=233452529
  • Loading branch information
@shicks @tjgq
shicks authored and tjgq committed Feb 12, 2019
1 parent adc0b3b commit a958d21
Show file tree
Hide file tree
Showing 5 changed files with 105 additions and 62 deletions.
29 changes: 4 additions & 25 deletions src/com/google/javascript/jscomp/ProcessClosurePrimitives.java
View file
Expand Up @@ -248,34 +248,14 @@ private void checkForLateOrMissingProvide(UnrecognizedRequire r) {
compiler.report(JSError.make(r.requireNode, requiresLevel, error, r.namespace));
}

private Node getAnyValueOfType(JSDocInfo jsdoc) {
checkArgument(jsdoc.hasType());
Node typeAst = jsdoc.getType().getRoot();
if (typeAst.getToken() == Token.BANG) {
typeAst = typeAst.getLastChild();
}
checkState(typeAst.isString(), typeAst);
switch (typeAst.getString()) {
case "boolean":
return IR.falseNode();
case "string":
return IR.string("");
case "number":
return IR.number(0);
default:
throw new RuntimeException(typeAst.getString());
}
}

/**
* @param n
*/
private void replaceGoogDefines(Node n) {
Node parent = n.getParent();
String name = n.getSecondChild().getString();
JSDocInfo jsdoc = n.getJSDocInfo();
Node value =
n.isFromExterns() ? getAnyValueOfType(jsdoc).srcref(n) : n.getChildAtIndex(2).detach();
Node value = n.getChildAtIndex(2).detach();

switch (parent.getToken()) {
case EXPR_RESULT:
Expand Down Expand Up @@ -1173,9 +1153,8 @@ private boolean verifyDefine(NodeTraversal t, Node parent, Node methodName, Node
// 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.isName() && NodeUtil.isNameDeclaration(parent.getParent())) {
parent = parent.getParent();
} else if (!parent.isExprResult()) {
compiler.report(t.makeError(methodName.getParent(), INVALID_CLOSURE_CALL_SCOPE_ERROR));
return false;
Expand All @@ -1201,7 +1180,7 @@ private boolean verifyDefine(NodeTraversal t, Node parent, Node methodName, Node
return false;
}

JSDocInfo info = parent.getFirstChild().getJSDocInfo();
JSDocInfo info = (parent.isExprResult() ? parent.getFirstChild() : parent).getJSDocInfo();
if (info == null || !info.isDefine()) {
compiler.report(t.makeError(parent, MISSING_DEFINE_ANNOTATION));
return false;
Expand Down
60 changes: 54 additions & 6 deletions src/com/google/javascript/jscomp/ijs/PotentialDeclaration.java
View file
Expand Up @@ -22,7 +22,9 @@
import com.google.javascript.jscomp.NodeUtil;
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.JSDocInfo;
import com.google.javascript.rhino.JSTypeExpression;
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.Token;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -75,7 +77,7 @@ static PotentialDeclaration fromStringKey(Node stringKeyNode) {

static PotentialDeclaration fromDefine(Node callNode) {
checkArgument(NodeUtil.isCallTo(callNode, "goog.define"));
return new DefineDeclaration(callNode);
return DefineDeclaration.from(callNode);
}

String getFullyQualifiedName() {
Expand Down Expand Up @@ -261,19 +263,65 @@ void simplify(AbstractCompiler compiler) {
}
}


/**
* A declaration declared by a call to `goog.define`. Note that a let, const, or var declaration
* annotated with @define in its JSDoc would be a NameDeclaration instead.
* annotated with @define in its JSDoc and no 'goog.define' would be a NameDeclaration instead.
*/
private static class DefineDeclaration extends PotentialDeclaration {
DefineDeclaration(Node callNode) {
super(callNode.getSecondChild().getString(), callNode, callNode.getLastChild());
DefineDeclaration(String qualifiedName, Node lhs, Node rhs) {
super(qualifiedName, lhs, rhs);
}

@Override
void simplify(AbstractCompiler compiler) {
NodeUtil.deleteNode(getLhs().getLastChild(), compiler);
JSDocInfo info = getJsDoc();
if (info != null && info.getType() != null) {
Node newRhs = makeEmptyValueNode(info.getType());
if (newRhs != null) {
getRhs().replaceWith(newRhs);
compiler.reportChangeToEnclosingScope(newRhs);
return;
}
}
NodeUtil.deleteNode(getRemovableNode(), compiler);
}

static DefineDeclaration from(Node callNode) {
// Match a few different forms, depending on the call node's parent:
// 1. EXPR_RESULT: goog.define('foo', 1);
// 2. ASSIGN: a.b = goog.define('c', 2);
// 3. NAME: var x = goog.define('d', 3);
switch (callNode.getParent().getToken()) {
case EXPR_RESULT:
return new DefineDeclaration(
callNode.getSecondChild().getString(), callNode, callNode.getLastChild());
case ASSIGN:
Node previous = callNode.getPrevious();
return new DefineDeclaration(
previous.getQualifiedName(), previous, callNode.getLastChild());
case NAME:
Node parent = callNode.getParent();
return new DefineDeclaration(parent.getString(), parent, callNode.getLastChild());
default:
throw new IllegalStateException("Unexpected parent: " + callNode.getParent().getToken());
}
}

static Node makeEmptyValueNode(JSTypeExpression type) {
Node n = type.getRoot();
while (n != null && n.getToken() != Token.STRING && n.getToken() != Token.NAME) {
n = n.getFirstChild();
}
switch (n != null ? n.getString() : "") {
case "boolean":
return new Node(Token.FALSE);
case "number":
return Node.newNumber(0);
case "string":
return Node.newString("");
default:
return null;
}
}
}

Expand Down
43 changes: 29 additions & 14 deletions src/com/google/javascript/jscomp/ijs/ProcessConstJsdocCallback.java
View file
Expand Up @@ -21,6 +21,7 @@
import com.google.javascript.jscomp.NodeTraversal;
import com.google.javascript.jscomp.NodeUtil;
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.QualifiedName;

/**
* A callback that calls the abstract method on every "inferrable const". This is a constant
Expand Down Expand Up @@ -58,19 +59,23 @@ public void visit(NodeTraversal t, Node n, Node parent) {
switch (expr.getToken()) {
case CALL:
Node callee = expr.getFirstChild();
if (callee.matchesQualifiedName("goog.provide")) {
if (GOOG_PROVIDE.matches(callee)) {
currentFile.markProvided(expr.getLastChild().getString());
} else if (callee.matchesQualifiedName("goog.require")
|| callee.matchesQualifiedName("require")) {
} else if (GOOG_REQUIRE.matches(callee) || CJS_REQUIRE.matches(callee)) {
currentFile.recordImport(expr.getLastChild().getString());
} else if (callee.matchesQualifiedName("goog.define")) {
} else if (GOOG_DEFINE.matches(callee)) {
currentFile.recordDefine(expr);
}
break;
case ASSIGN:
Node lhs = expr.getFirstChild();
currentFile.recordNameDeclaration(lhs);
processDeclarationWithRhs(t, lhs);
Node rhs = expr.getLastChild();
if (rhs.isCall() && GOOG_DEFINE.matches(rhs.getFirstChild()) && lhs.isQualifiedName()) {
currentFile.recordDefine(rhs);
} else {
currentFile.recordNameDeclaration(lhs);
processDeclarationWithRhs(t, lhs, expr.getLastChild());
}
break;
case GETPROP:
currentFile.recordNameDeclaration(expr);
Expand All @@ -83,14 +88,20 @@ public void visit(NodeTraversal t, Node n, Node parent) {
case CONST:
case LET:
checkState(n.hasOneChild(), n);
recordNameDeclaration(n);
if (n.getFirstChild().isName() && n.getFirstChild().hasChildren()) {
processDeclarationWithRhs(t, n.getFirstChild());
Node name = n.getFirstChild();
Node rhs = name.getFirstChild();
if (rhs != null && rhs.isCall() && GOOG_DEFINE.matches(name.getFirstFirstChild())) {
currentFile.recordDefine(rhs);
} else {
recordNameDeclaration(n);
if (name.isName() && name.hasChildren()) {
processDeclarationWithRhs(t, name, rhs);
}
}
break;
case STRING_KEY:
if (parent.isObjectLit() && n.hasOneChild()) {
processDeclarationWithRhs(t, n);
processDeclarationWithRhs(t, n, n.getFirstChild());
currentFile.recordStringKeyDeclaration(n);
}
break;
Expand All @@ -99,6 +110,11 @@ public void visit(NodeTraversal t, Node n, Node parent) {
}
}

private static final QualifiedName GOOG_DEFINE = QualifiedName.of("goog.define");
private static final QualifiedName GOOG_PROVIDE = QualifiedName.of("goog.provide");
private static final QualifiedName GOOG_REQUIRE = QualifiedName.of("goog.require");
private static final QualifiedName CJS_REQUIRE = QualifiedName.of("require");

private void recordNameDeclaration(Node decl) {
checkArgument(NodeUtil.isNameDeclaration(decl));
Node rhs = decl.getFirstChild().getLastChild();
Expand All @@ -112,15 +128,14 @@ private void recordNameDeclaration(Node decl) {
}
}

private void processDeclarationWithRhs(NodeTraversal t, Node lhs) {
private void processDeclarationWithRhs(NodeTraversal t, Node lhs, Node rhs) {
checkArgument(
lhs.isQualifiedName() || lhs.isStringKey() || lhs.isDestructuringLhs(),
lhs);
checkState(NodeUtil.getRValueOfLValue(lhs) != null, lhs);
if (!PotentialDeclaration.isConstToBeInferred(lhs)) {
return;
if (PotentialDeclaration.isConstToBeInferred(lhs)) {
processConstWithRhs(t, lhs);
}
processConstWithRhs(t, lhs);
}

protected abstract void processConstWithRhs(NodeTraversal t, Node lhs);
Expand Down
14 changes: 0 additions & 14 deletions test/com/google/javascript/jscomp/ProcessClosurePrimitivesTest.java
View file
Expand Up @@ -1545,20 +1545,6 @@ public void testDefineErrorCases() {
testError(jsdoc + "goog.define(`${template}Name`, 1);", INVALID_ARGUMENT_ERROR);
}

@Test
public void testDefineInExterns() {
String jsdoc = "/** @define {number} */\n";
allowExternsChanges();
testErrorExterns(jsdoc + "goog.define('value');");

testErrorExterns("goog.define('name');", MISSING_DEFINE_ANNOTATION);
testErrorExterns(jsdoc + "goog.define('name.2');", INVALID_DEFINE_NAME_ERROR);
testErrorExterns(jsdoc + "goog.define();", NULL_ARGUMENT_ERROR);
testErrorExterns(jsdoc + "goog.define(5);", INVALID_ARGUMENT_ERROR);

testErrorExterns("/** @define {!number} */ goog.define('name');");
}

@Test
public void testInvalidDefine() {
testError(
Expand Down
21 changes: 18 additions & 3 deletions test/com/google/javascript/jscomp/ijs/ConvertToTypedInterfaceTest.java
View file
Expand Up @@ -1171,10 +1171,25 @@ public void testConstants() {
@Test
public void testDefines() {
// NOTE: This is another pattern that is only allowed in externs.
test("/** @define {number} */ var x = 5;", "/** @define {number} */ var x;");
test(
"/** @define {number} */ var x = 5;", //
"/** @define {number} */ var x;");

test(
"/** @define {number} */ goog.define('goog.BLAH', 5);",
"/** @define {number} */ goog.define('goog.BLAH', 0);");

test(
"/** @define {string} */ const BLAH = goog.define('goog.BLAH', 'blah');",
"/** @define {string} */ const BLAH = goog.define('goog.BLAH', '');");

test(
"/** @define {boolean} */ goog.BLECH = goog.define('goog.BLAH', true);",
"/** @define {boolean} */ goog.BLECH = goog.define('goog.BLAH', false);");

test("/** @define {number} */ goog.define('goog.BLAH', 5);",
"/** @define {number} */ goog.define('goog.BLAH');");
test(
"/** @define {number|boolean} */ const X = goog.define('goog.XYZ', true);",
"/** @define {number|boolean} */ const X = goog.define('goog.XYZ', 0);");
}

@Test
Expand Down

0 comments on commit a958d21

Please sign in to comment.