Skip to content

Commit

Permalink
Clean up SimplifyDeclarations.shouldRemove and JsdocUtil
Browse files Browse the repository at this point in the history 
Now it should be a little clearer where the warnings produced by
the i.js generation pass come from.
Also include a few other miscellaneous type fixes/simplifications.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=180994716
  • Loading branch information
@blickly @brad4d
blickly authored and brad4d committed Jan 8, 2018
1 parent cbeebfe commit f5e1fad
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 74 deletions.
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/ijs/ClassUtil.java
View file
Expand Up @@ -76,7 +76,7 @@ static boolean hasNamedClass(Node functionNode) {
return getClassName(functionNode) != null; return getClassName(functionNode) != null;
} }


static String getClassName(Node functionNode) { private static String getClassName(Node functionNode) {
checkArgument(functionNode.isFunction()); checkArgument(functionNode.isFunction());
if (isClassMethod(functionNode)) { if (isClassMethod(functionNode)) {
Node parent = functionNode.getParent(); Node parent = functionNode.getParent();
Expand Down
69 changes: 39 additions & 30 deletions src/com/google/javascript/jscomp/ijs/ConvertToTypedInterface.java
View file
Expand Up @@ -25,6 +25,7 @@
import com.google.javascript.jscomp.AbstractCompiler; import com.google.javascript.jscomp.AbstractCompiler;
import com.google.javascript.jscomp.CompilerPass; import com.google.javascript.jscomp.CompilerPass;
import com.google.javascript.jscomp.DiagnosticType; import com.google.javascript.jscomp.DiagnosticType;
import com.google.javascript.jscomp.JSError;
import com.google.javascript.jscomp.NodeTraversal; import com.google.javascript.jscomp.NodeTraversal;
import com.google.javascript.jscomp.NodeUtil; import com.google.javascript.jscomp.NodeUtil;
import com.google.javascript.jscomp.Scope; import com.google.javascript.jscomp.Scope;
Expand Down Expand Up @@ -71,6 +72,16 @@ public ConvertToTypedInterface(AbstractCompiler compiler) {
this.compiler = compiler; this.compiler = compiler;
} }


static void maybeWarnForConstWithoutExplicitType(
AbstractCompiler compiler, JSDocInfo jsdoc, Node nameNode) {
if (isConstToBeInferred(jsdoc, nameNode)
&& !nameNode.isFromExterns()
&& !JsdocUtil.isPrivate(jsdoc)) {
compiler.report(
JSError.make(nameNode, CONSTANT_WITHOUT_EXPLICIT_TYPE));
}
}

private void removeUselessFiles(Node externs, Node root) { private void removeUselessFiles(Node externs, Node root) {
for (Node script : Iterables.concat(externs.children(), root.children())) { for (Node script : Iterables.concat(externs.children(), root.children())) {
if (!script.hasChildren()) { if (!script.hasChildren()) {
Expand All @@ -88,7 +99,7 @@ public void process(Node externs, Node root) {
} }
} }


public void processFile(Node scriptNode) { private void processFile(Node scriptNode) {
checkArgument(scriptNode.isScript()); checkArgument(scriptNode.isScript());
FileInfo currentFile = new FileInfo(); FileInfo currentFile = new FileInfo();
NodeTraversal.traverseEs6(compiler, scriptNode, new RemoveNonDeclarations()); NodeTraversal.traverseEs6(compiler, scriptNode, new RemoveNonDeclarations());
Expand Down Expand Up @@ -290,14 +301,14 @@ public void visit(NodeTraversal t, Node n, Node parent) {
switch (n.getToken()) { switch (n.getToken()) {
case CLASS: case CLASS:
if (NodeUtil.isStatementParent(parent)) { if (NodeUtil.isStatementParent(parent)) {
currentFile.recordNameDeclaration(n.getFirstChild(), t.getScope()); currentFile.recordNameDeclaration(n.getFirstChild());
} }
break; break;
case FUNCTION: case FUNCTION:
if (NodeUtil.isStatementParent(parent)) { if (NodeUtil.isStatementParent(parent)) {
currentFile.recordNameDeclaration(n.getFirstChild(), t.getScope()); currentFile.recordNameDeclaration(n.getFirstChild());
} else if (ClassUtil.isClassMethod(n) && ClassUtil.hasNamedClass(n)) { } else if (ClassUtil.isClassMethod(n) && ClassUtil.hasNamedClass(n)) {
currentFile.recordMethod(n, t.getScope()); currentFile.recordMethod(n);
} }
break; break;
case EXPR_RESULT: case EXPR_RESULT:
Expand All @@ -311,16 +322,16 @@ public void visit(NodeTraversal t, Node n, Node parent) {
} else if (callee.matchesQualifiedName("goog.require")) { } else if (callee.matchesQualifiedName("goog.require")) {
currentFile.recordImport(expr.getLastChild().getString()); currentFile.recordImport(expr.getLastChild().getString());
} else if (callee.matchesQualifiedName("goog.define")) { } else if (callee.matchesQualifiedName("goog.define")) {
currentFile.recordDefine(expr, t.getScope()); currentFile.recordDefine(expr);
} }
break; break;
case ASSIGN: case ASSIGN:
Node lhs = expr.getFirstChild(); Node lhs = expr.getFirstChild();
propagateJsdocAtName(t, lhs); propagateJsdocAtName(t, lhs);
currentFile.recordNameDeclaration(lhs, t.getScope()); currentFile.recordNameDeclaration(lhs);
break; break;
case GETPROP: case GETPROP:
currentFile.recordNameDeclaration(expr, t.getScope()); currentFile.recordNameDeclaration(expr);
break; break;
default: default:
throw new RuntimeException("Unexpected declaration: " + expr); throw new RuntimeException("Unexpected declaration: " + expr);
Expand All @@ -331,7 +342,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
case LET: case LET:
checkState(n.hasOneChild()); checkState(n.hasOneChild());
propagateJsdocAtName(t, n.getFirstChild()); propagateJsdocAtName(t, n.getFirstChild());
recordNameDeclaration(t, n); recordNameDeclaration(n);
break; break;
case STRING_KEY: case STRING_KEY:
if (n.hasOneChild()) { if (n.hasOneChild()) {
Expand All @@ -343,15 +354,15 @@ public void visit(NodeTraversal t, Node n, Node parent) {
} }
} }


void recordNameDeclaration(NodeTraversal t, Node decl) { private void recordNameDeclaration(Node decl) {
checkArgument(NodeUtil.isNameDeclaration(decl)); checkArgument(NodeUtil.isNameDeclaration(decl));
Node rhs = decl.getFirstChild().getLastChild(); Node rhs = decl.getFirstChild().getLastChild();
boolean isImport = isImportRhs(rhs); boolean isImport = isImportRhs(rhs);
for (Node name : NodeUtil.findLhsNodesInNode(decl)) { for (Node name : NodeUtil.findLhsNodesInNode(decl)) {
if (isImport) { if (isImport) {
currentFile.recordImport(name.getString()); currentFile.recordImport(name.getString());
} else { } else {
currentFile.recordNameDeclaration(name, t.getScope()); currentFile.recordNameDeclaration(name);
} }
} }
} }
Expand All @@ -361,18 +372,18 @@ private void propagateJsdocAtName(NodeTraversal t, Node nameNode) {
nameNode.isQualifiedName() || nameNode.isStringKey() || nameNode.isDestructuringLhs(), nameNode.isQualifiedName() || nameNode.isStringKey() || nameNode.isDestructuringLhs(),
nameNode); nameNode);
Node jsdocNode = NodeUtil.getBestJSDocInfoNode(nameNode); Node jsdocNode = NodeUtil.getBestJSDocInfoNode(nameNode);
JSDocInfo jsdoc = jsdocNode.getJSDocInfo(); JSDocInfo originalJsdoc = jsdocNode.getJSDocInfo();
if (!isConstToBeInferred(jsdoc, nameNode, false)) { if (!isConstToBeInferred(originalJsdoc, nameNode)) {
return; return;
} }
Node rhs = NodeUtil.getRValueOfLValue(nameNode); Node rhs = NodeUtil.getRValueOfLValue(nameNode);
if (rhs == null) { if (rhs == null) {
return; return;
} }
JSDocInfo newJsdoc = JsdocUtil.getJSDocForRhs(rhs, jsdoc); JSDocInfo newJsdoc = JsdocUtil.getJSDocForRhs(rhs, originalJsdoc);
if (newJsdoc == null && ClassUtil.isThisProp(nameNode)) { if (newJsdoc == null && ClassUtil.isThisProp(nameNode)) {
Var decl = findNameDeclaration(t.getScope(), rhs); Var decl = findNameDeclaration(t.getScope(), rhs);
newJsdoc = JsdocUtil.getJSDocForName(decl, jsdoc); newJsdoc = JsdocUtil.getJSDocForName(decl, originalJsdoc);
} }
if (newJsdoc != null) { if (newJsdoc != null) {
jsdocNode.setJSDocInfo(newJsdoc); jsdocNode.setJSDocInfo(newJsdoc);
Expand Down Expand Up @@ -495,11 +506,6 @@ private static boolean isClass(Node n) {
return n.isClass() || NodeUtil.isCallTo(n, "goog.defineClass"); return n.isClass() || NodeUtil.isCallTo(n, "goog.defineClass");
} }


private static boolean isExportLhs(Node lhs) {
return (lhs.isName() && lhs.matchesQualifiedName("exports"))
|| (lhs.isGetProp() && lhs.getFirstChild().matchesQualifiedName("exports"));
}

private static String rootName(String qualifiedName) { private static String rootName(String qualifiedName) {
int dotIndex = qualifiedName.indexOf('.'); int dotIndex = qualifiedName.indexOf('.');
if (dotIndex == -1) { if (dotIndex == -1) {
Expand All @@ -523,7 +529,6 @@ private RemovalType shouldRemove(PotentialDeclaration decl) {
return RemovalType.SIMPLIFY_RHS; return RemovalType.SIMPLIFY_RHS;
} }
if (PotentialDeclaration.isTypedRhs(rhs) if (PotentialDeclaration.isTypedRhs(rhs)
|| NodeUtil.isCallTo(rhs, "goog.defineClass")
|| isImportRhs(rhs) || isImportRhs(rhs)
|| (isExport && (rhs.isQualifiedName() || rhs.isObjectLit())) || (isExport && (rhs.isQualifiedName() || rhs.isObjectLit()))
|| (jsdoc != null && jsdoc.isConstructor() && rhs.isQualifiedName()) || (jsdoc != null && jsdoc.isConstructor() && rhs.isQualifiedName())
Expand All @@ -536,27 +541,26 @@ private RemovalType shouldRemove(PotentialDeclaration decl) {
if (NodeUtil.isNamespaceDecl(nameNode)) { if (NodeUtil.isNamespaceDecl(nameNode)) {
return RemovalType.SIMPLIFY_RHS; return RemovalType.SIMPLIFY_RHS;
} }
if (isConstToBeInferred(jsdoc, nameNode, isExport)) { Node jsdocNode = NodeUtil.getBestJSDocInfoNode(nameNode);
Node jsdocNode = NodeUtil.getBestJSDocInfoNode(nameNode); if (isConstToBeInferred(jsdoc, nameNode)) {
jsdocNode.setJSDocInfo(JsdocUtil.pullJsdocTypeFromAst(compiler, jsdoc, nameNode)); jsdocNode.setJSDocInfo(JsdocUtil.getUnusableTypeJSDoc(jsdoc));
maybeWarnForConstWithoutExplicitType(compiler, jsdoc, nameNode);
return RemovalType.SIMPLIFY_RHS; return RemovalType.SIMPLIFY_RHS;
} }
if (jsdoc == null || !jsdoc.containsDeclaration()) { if (jsdoc == null || !jsdoc.containsDeclaration()) {
Node jsdocNode = NodeUtil.getBestJSDocInfoNode(nameNode);
if (!isDeclaration(nameNode) if (!isDeclaration(nameNode)
&& !currentFile.isPrefixProvided(fullyQualifiedName) && !currentFile.isPrefixProvided(fullyQualifiedName)
&& !currentFile.isStrictPrefixDeclared(fullyQualifiedName)) { && !currentFile.isStrictPrefixDeclared(fullyQualifiedName)) {
// This looks like an update rather than a declaration in this file. // This looks like an update rather than a declaration in this file.
return RemovalType.REMOVE_ALL; return RemovalType.REMOVE_ALL;
} }
jsdocNode.setJSDocInfo(JsdocUtil.getAllTypeJSDoc()); jsdocNode.setJSDocInfo(JsdocUtil.getUnusableTypeJSDoc(jsdoc));
} }
return RemovalType.SIMPLIFY_RHS; return RemovalType.SIMPLIFY_RHS;
} }


private boolean isAliasDefinition(PotentialDeclaration decl) { private boolean isAliasDefinition(PotentialDeclaration decl) {
boolean isExport = isExportLhs(decl.getLhs()); if (isConstToBeInferred(decl.getJsDoc(), decl.getLhs())
if (isConstToBeInferred(decl.getJsDoc(), decl.getLhs(), isExport)
&& decl.getRhs().isQualifiedName()) { && decl.getRhs().isQualifiedName()) {
String aliasedName = decl.getRhs().getQualifiedName(); String aliasedName = decl.getRhs().getQualifiedName();
return currentFile.isPrefixRequired(aliasedName) || currentFile.isNameDeclared(aliasedName); return currentFile.isPrefixRequired(aliasedName) || currentFile.isNameDeclared(aliasedName);
Expand All @@ -582,16 +586,21 @@ private static boolean isDeclaration(Node nameNode) {
} }


static boolean isConstToBeInferred( static boolean isConstToBeInferred(
JSDocInfo jsdoc, Node nameNode, boolean isImpliedConst) { JSDocInfo jsdoc, Node nameNode) {
boolean isConst = boolean isConst =
isImpliedConst nameNode.getParent().isConst()
|| nameNode.getParent().isConst() || isExportLhs(nameNode)
|| (jsdoc != null && jsdoc.hasConstAnnotation()); || (jsdoc != null && jsdoc.hasConstAnnotation());
return isConst return isConst
&& !JsdocUtil.hasAnnotatedType(jsdoc) && !JsdocUtil.hasAnnotatedType(jsdoc)
&& !NodeUtil.isNamespaceDecl(nameNode); && !NodeUtil.isNamespaceDecl(nameNode);
} }


private static boolean isExportLhs(Node lhs) {
return (lhs.isName() && lhs.matchesQualifiedName("exports"))
|| (lhs.isGetProp() && lhs.getFirstChild().matchesQualifiedName("exports"));
}

private static boolean isImportRhs(@Nullable Node rhs) { private static boolean isImportRhs(@Nullable Node rhs) {
if (rhs == null || !rhs.isCall()) { if (rhs == null || !rhs.isCall()) {
return false; return false;
Expand Down
11 changes: 5 additions & 6 deletions src/com/google/javascript/jscomp/ijs/FileInfo.java
View file
Expand Up @@ -19,7 +19,6 @@


import com.google.common.collect.ListMultimap; import com.google.common.collect.ListMultimap;
import com.google.common.collect.MultimapBuilder; import com.google.common.collect.MultimapBuilder;
import com.google.javascript.jscomp.Scope;
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;
import java.util.HashSet; import java.util.HashSet;
import java.util.Set; import java.util.Set;
Expand All @@ -33,23 +32,23 @@ final class FileInfo {
private final ListMultimap<String, PotentialDeclaration> declarations = private final ListMultimap<String, PotentialDeclaration> declarations =
MultimapBuilder.linkedHashKeys().arrayListValues().build(); MultimapBuilder.linkedHashKeys().arrayListValues().build();


void recordNameDeclaration(Node qnameNode, Scope scope) { void recordNameDeclaration(Node qualifiedNameNode) {
recordDeclaration(PotentialDeclaration.fromName(qnameNode)); recordDeclaration(PotentialDeclaration.fromName(qualifiedNameNode));
} }


void recordMethod(Node functionNode, Scope scope) { void recordMethod(Node functionNode) {
recordDeclaration(PotentialDeclaration.fromMethod(functionNode)); recordDeclaration(PotentialDeclaration.fromMethod(functionNode));
} }


void recordDefine(Node callNode, Scope scope) { void recordDefine(Node callNode) {
recordDeclaration(PotentialDeclaration.fromDefine(callNode)); recordDeclaration(PotentialDeclaration.fromDefine(callNode));
} }


ListMultimap<String, PotentialDeclaration> getDeclarations() { ListMultimap<String, PotentialDeclaration> getDeclarations() {
return declarations; return declarations;
} }


void recordDeclaration(PotentialDeclaration decl) { private void recordDeclaration(PotentialDeclaration decl) {
declarations.put(decl.getFullyQualifiedName(), decl); declarations.put(decl.getFullyQualifiedName(), decl);
} }


Expand Down
41 changes: 7 additions & 34 deletions src/com/google/javascript/jscomp/ijs/JsdocUtil.java
View file
Expand Up @@ -15,10 +15,6 @@
*/ */
package com.google.javascript.jscomp.ijs; package com.google.javascript.jscomp.ijs;


import static com.google.common.base.Preconditions.checkArgument;

import com.google.javascript.jscomp.AbstractCompiler;
import com.google.javascript.jscomp.JSError;
import com.google.javascript.jscomp.NodeUtil; import com.google.javascript.jscomp.NodeUtil;
import com.google.javascript.jscomp.Var; import com.google.javascript.jscomp.Var;
import com.google.javascript.rhino.IR; import com.google.javascript.rhino.IR;
Expand All @@ -36,38 +32,12 @@
final class JsdocUtil { final class JsdocUtil {
private JsdocUtil() {} private JsdocUtil() {}


static JSDocInfo updateJsdoc(AbstractCompiler compiler, Node nameNode) { static boolean isPrivate(@Nullable JSDocInfo jsdoc) {
checkArgument(nameNode.isStringKey(), nameNode);
Node jsdocNode = nameNode;
JSDocInfo jsdoc = jsdocNode.getJSDocInfo();
if (jsdoc == null) {
jsdoc = JsdocUtil.getAllTypeJSDoc();
} else if (ConvertToTypedInterface.isConstToBeInferred(jsdoc, nameNode, false)) {
jsdoc = JsdocUtil.pullJsdocTypeFromAst(compiler, jsdoc, nameNode);
}
jsdocNode.setJSDocInfo(jsdoc);
return jsdoc;
}

static JSDocInfo pullJsdocTypeFromAst(
AbstractCompiler compiler, JSDocInfo oldJSDoc, Node nameNode) {
checkArgument(nameNode.isQualifiedName() || nameNode.isStringKey(), nameNode);
if (oldJSDoc != null && oldJSDoc.getDescription() != null) {
return getConstJSDoc(oldJSDoc, "string");
}
if (!nameNode.isFromExterns() && !isPrivate(oldJSDoc)) {
compiler.report(
JSError.make(nameNode, ConvertToTypedInterface.CONSTANT_WITHOUT_EXPLICIT_TYPE));
}
return getConstJSDoc(oldJSDoc, new Node(Token.STAR));
}

private static boolean isPrivate(@Nullable JSDocInfo jsdoc) {
return jsdoc != null && jsdoc.getVisibility().equals(Visibility.PRIVATE); return jsdoc != null && jsdoc.getVisibility().equals(Visibility.PRIVATE);
} }


static JSDocInfo getAllTypeJSDoc() { static JSDocInfo getUnusableTypeJSDoc(JSDocInfo oldJSDoc) {
return getConstJSDoc(null, new Node(Token.STAR)); return getConstJSDoc(oldJSDoc, new Node(Token.STAR));
} }


static JSDocInfo getQmarkTypeJSDoc() { static JSDocInfo getQmarkTypeJSDoc() {
Expand Down Expand Up @@ -122,7 +92,10 @@ static JSDocInfo getJSDocForRhs(Node rhs, JSDocInfo oldJSDoc) {
return getConstJSDoc(oldJSDoc, new Node(Token.BANG, IR.string("RegExp"))); return getConstJSDoc(oldJSDoc, new Node(Token.BANG, IR.string("RegExp")));
} }
break; break;
default: case UNDETERMINED:
if (oldJSDoc != null && oldJSDoc.getDescription() != null) {
return getConstJSDoc(oldJSDoc, "string");
}
break; break;
} }
return null; return null;
Expand Down
16 changes: 14 additions & 2 deletions src/com/google/javascript/jscomp/ijs/PotentialDeclaration.java
View file
Expand Up @@ -125,7 +125,7 @@ final void remove(AbstractCompiler compiler) {
* A potential declaration that has a fully qualified name to describe it. * A potential declaration that has a fully qualified name to describe it.
* This includes things like: * This includes things like:
* var/let/const/function/class declarations, * var/let/const/function/class declarations,
* assignments to a fully qualfied name, * assignments to a fully qualified name,
* and goog.module exports * and goog.module exports
* This is the most common type of potential declaration. * This is the most common type of potential declaration.
*/ */
Expand Down Expand Up @@ -162,8 +162,10 @@ void simplify(AbstractCompiler compiler) {
if (objLit.hasChildren()) { if (objLit.hasChildren()) {
for (Node key : objLit.children()) { for (Node key : objLit.children()) {
if (!isTypedRhs(key.getLastChild())) { if (!isTypedRhs(key.getLastChild())) {
ConvertToTypedInterface
.maybeWarnForConstWithoutExplicitType(compiler, key.getJSDocInfo(), key);
removeStringKeyValue(key); removeStringKeyValue(key);
JsdocUtil.updateJsdoc(compiler, key); maybeUpdateJsdoc(key);
compiler.reportChangeToEnclosingScope(key); compiler.reportChangeToEnclosingScope(key);
} }
} }
Expand Down Expand Up @@ -196,6 +198,15 @@ private static void removeStringKeyValue(Node stringKey) {
stringKey.replaceChild(value, replacementValue); stringKey.replaceChild(value, replacementValue);
} }


private static void maybeUpdateJsdoc(Node jsdocNode) {
checkArgument(jsdocNode.isStringKey(), jsdocNode);
JSDocInfo jsdoc = jsdocNode.getJSDocInfo();
if (jsdoc == null
|| !jsdoc.containsDeclaration()
|| ConvertToTypedInterface.isConstToBeInferred(jsdoc, jsdocNode)) {
jsdocNode.setJSDocInfo(JsdocUtil.getUnusableTypeJSDoc(jsdoc));
}
}
} }


/** /**
Expand Down Expand Up @@ -259,6 +270,7 @@ Node getRemovableNode() {
static boolean isTypedRhs(Node rhs) { static boolean isTypedRhs(Node rhs) {
return rhs.isFunction() return rhs.isFunction()
|| rhs.isClass() || rhs.isClass()
|| NodeUtil.isCallTo(rhs, "goog.defineClass")
|| (rhs.isQualifiedName() && rhs.matchesQualifiedName("goog.abstractMethod")) || (rhs.isQualifiedName() && rhs.matchesQualifiedName("goog.abstractMethod"))
|| (rhs.isQualifiedName() && rhs.matchesQualifiedName("goog.nullFunction")); || (rhs.isQualifiedName() && rhs.matchesQualifiedName("goog.nullFunction"));
} }
Expand Down
2 changes: 1 addition & 1 deletion test/com/google/javascript/jscomp/ijs/ConvertToTypedInterfaceTest.java
View file
Expand Up @@ -1353,7 +1353,7 @@ public void testRedeclarationOfClassMethodDoesntCrash() {
"class Foo {", "class Foo {",
" constructor() {}", " constructor() {}",
"}", "}",
// TODO(blickly): Would be better if this included the type anntoation. // TODO(blickly): Would be better if this included the type annotation.
"/** @private */", "/** @private */",
"Foo.prototype.handleEvent_", "Foo.prototype.handleEvent_",
"")); ""));
Expand Down

0 comments on commit f5e1fad

Please sign in to comment.