From f5e1fad56ec09cbda378a7d8c6493417baecb1b9 Mon Sep 17 00:00:00 2001 From: blickly Date: Fri, 5 Jan 2018 17:10:14 -0800 Subject: [PATCH] Clean up SimplifyDeclarations.shouldRemove and JsdocUtil 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 --- .../javascript/jscomp/ijs/ClassUtil.java | 2 +- .../jscomp/ijs/ConvertToTypedInterface.java | 69 +++++++++++-------- .../javascript/jscomp/ijs/FileInfo.java | 11 ++- .../javascript/jscomp/ijs/JsdocUtil.java | 41 ++--------- .../jscomp/ijs/PotentialDeclaration.java | 16 ++++- .../ijs/ConvertToTypedInterfaceTest.java | 2 +- 6 files changed, 67 insertions(+), 74 deletions(-) diff --git a/src/com/google/javascript/jscomp/ijs/ClassUtil.java b/src/com/google/javascript/jscomp/ijs/ClassUtil.java index 17ebf9607d9..81a1921f17f 100644 --- a/src/com/google/javascript/jscomp/ijs/ClassUtil.java +++ b/src/com/google/javascript/jscomp/ijs/ClassUtil.java @@ -76,7 +76,7 @@ static boolean hasNamedClass(Node functionNode) { return getClassName(functionNode) != null; } - static String getClassName(Node functionNode) { + private static String getClassName(Node functionNode) { checkArgument(functionNode.isFunction()); if (isClassMethod(functionNode)) { Node parent = functionNode.getParent(); diff --git a/src/com/google/javascript/jscomp/ijs/ConvertToTypedInterface.java b/src/com/google/javascript/jscomp/ijs/ConvertToTypedInterface.java index e7ffd2a0bcc..a746471e909 100644 --- a/src/com/google/javascript/jscomp/ijs/ConvertToTypedInterface.java +++ b/src/com/google/javascript/jscomp/ijs/ConvertToTypedInterface.java @@ -25,6 +25,7 @@ import com.google.javascript.jscomp.AbstractCompiler; import com.google.javascript.jscomp.CompilerPass; import com.google.javascript.jscomp.DiagnosticType; +import com.google.javascript.jscomp.JSError; import com.google.javascript.jscomp.NodeTraversal; import com.google.javascript.jscomp.NodeUtil; import com.google.javascript.jscomp.Scope; @@ -71,6 +72,16 @@ public ConvertToTypedInterface(AbstractCompiler 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) { for (Node script : Iterables.concat(externs.children(), root.children())) { if (!script.hasChildren()) { @@ -88,7 +99,7 @@ public void process(Node externs, Node root) { } } - public void processFile(Node scriptNode) { + private void processFile(Node scriptNode) { checkArgument(scriptNode.isScript()); FileInfo currentFile = new FileInfo(); NodeTraversal.traverseEs6(compiler, scriptNode, new RemoveNonDeclarations()); @@ -290,14 +301,14 @@ public void visit(NodeTraversal t, Node n, Node parent) { switch (n.getToken()) { case CLASS: if (NodeUtil.isStatementParent(parent)) { - currentFile.recordNameDeclaration(n.getFirstChild(), t.getScope()); + currentFile.recordNameDeclaration(n.getFirstChild()); } break; case FUNCTION: if (NodeUtil.isStatementParent(parent)) { - currentFile.recordNameDeclaration(n.getFirstChild(), t.getScope()); + currentFile.recordNameDeclaration(n.getFirstChild()); } else if (ClassUtil.isClassMethod(n) && ClassUtil.hasNamedClass(n)) { - currentFile.recordMethod(n, t.getScope()); + currentFile.recordMethod(n); } break; case EXPR_RESULT: @@ -311,16 +322,16 @@ public void visit(NodeTraversal t, Node n, Node parent) { } else if (callee.matchesQualifiedName("goog.require")) { currentFile.recordImport(expr.getLastChild().getString()); } else if (callee.matchesQualifiedName("goog.define")) { - currentFile.recordDefine(expr, t.getScope()); + currentFile.recordDefine(expr); } break; case ASSIGN: Node lhs = expr.getFirstChild(); propagateJsdocAtName(t, lhs); - currentFile.recordNameDeclaration(lhs, t.getScope()); + currentFile.recordNameDeclaration(lhs); break; case GETPROP: - currentFile.recordNameDeclaration(expr, t.getScope()); + currentFile.recordNameDeclaration(expr); break; default: throw new RuntimeException("Unexpected declaration: " + expr); @@ -331,7 +342,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { case LET: checkState(n.hasOneChild()); propagateJsdocAtName(t, n.getFirstChild()); - recordNameDeclaration(t, n); + recordNameDeclaration(n); break; case STRING_KEY: if (n.hasOneChild()) { @@ -343,7 +354,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { } } - void recordNameDeclaration(NodeTraversal t, Node decl) { + private void recordNameDeclaration(Node decl) { checkArgument(NodeUtil.isNameDeclaration(decl)); Node rhs = decl.getFirstChild().getLastChild(); boolean isImport = isImportRhs(rhs); @@ -351,7 +362,7 @@ void recordNameDeclaration(NodeTraversal t, Node decl) { if (isImport) { currentFile.recordImport(name.getString()); } else { - currentFile.recordNameDeclaration(name, t.getScope()); + currentFile.recordNameDeclaration(name); } } } @@ -361,18 +372,18 @@ private void propagateJsdocAtName(NodeTraversal t, Node nameNode) { nameNode.isQualifiedName() || nameNode.isStringKey() || nameNode.isDestructuringLhs(), nameNode); Node jsdocNode = NodeUtil.getBestJSDocInfoNode(nameNode); - JSDocInfo jsdoc = jsdocNode.getJSDocInfo(); - if (!isConstToBeInferred(jsdoc, nameNode, false)) { + JSDocInfo originalJsdoc = jsdocNode.getJSDocInfo(); + if (!isConstToBeInferred(originalJsdoc, nameNode)) { return; } Node rhs = NodeUtil.getRValueOfLValue(nameNode); if (rhs == null) { return; } - JSDocInfo newJsdoc = JsdocUtil.getJSDocForRhs(rhs, jsdoc); + JSDocInfo newJsdoc = JsdocUtil.getJSDocForRhs(rhs, originalJsdoc); if (newJsdoc == null && ClassUtil.isThisProp(nameNode)) { Var decl = findNameDeclaration(t.getScope(), rhs); - newJsdoc = JsdocUtil.getJSDocForName(decl, jsdoc); + newJsdoc = JsdocUtil.getJSDocForName(decl, originalJsdoc); } if (newJsdoc != null) { jsdocNode.setJSDocInfo(newJsdoc); @@ -495,11 +506,6 @@ private static boolean isClass(Node n) { 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) { int dotIndex = qualifiedName.indexOf('.'); if (dotIndex == -1) { @@ -523,7 +529,6 @@ private RemovalType shouldRemove(PotentialDeclaration decl) { return RemovalType.SIMPLIFY_RHS; } if (PotentialDeclaration.isTypedRhs(rhs) - || NodeUtil.isCallTo(rhs, "goog.defineClass") || isImportRhs(rhs) || (isExport && (rhs.isQualifiedName() || rhs.isObjectLit())) || (jsdoc != null && jsdoc.isConstructor() && rhs.isQualifiedName()) @@ -536,27 +541,26 @@ private RemovalType shouldRemove(PotentialDeclaration decl) { if (NodeUtil.isNamespaceDecl(nameNode)) { return RemovalType.SIMPLIFY_RHS; } - if (isConstToBeInferred(jsdoc, nameNode, isExport)) { - Node jsdocNode = NodeUtil.getBestJSDocInfoNode(nameNode); - jsdocNode.setJSDocInfo(JsdocUtil.pullJsdocTypeFromAst(compiler, jsdoc, nameNode)); + Node jsdocNode = NodeUtil.getBestJSDocInfoNode(nameNode); + if (isConstToBeInferred(jsdoc, nameNode)) { + jsdocNode.setJSDocInfo(JsdocUtil.getUnusableTypeJSDoc(jsdoc)); + maybeWarnForConstWithoutExplicitType(compiler, jsdoc, nameNode); return RemovalType.SIMPLIFY_RHS; } if (jsdoc == null || !jsdoc.containsDeclaration()) { - Node jsdocNode = NodeUtil.getBestJSDocInfoNode(nameNode); if (!isDeclaration(nameNode) && !currentFile.isPrefixProvided(fullyQualifiedName) && !currentFile.isStrictPrefixDeclared(fullyQualifiedName)) { // This looks like an update rather than a declaration in this file. return RemovalType.REMOVE_ALL; } - jsdocNode.setJSDocInfo(JsdocUtil.getAllTypeJSDoc()); + jsdocNode.setJSDocInfo(JsdocUtil.getUnusableTypeJSDoc(jsdoc)); } return RemovalType.SIMPLIFY_RHS; } private boolean isAliasDefinition(PotentialDeclaration decl) { - boolean isExport = isExportLhs(decl.getLhs()); - if (isConstToBeInferred(decl.getJsDoc(), decl.getLhs(), isExport) + if (isConstToBeInferred(decl.getJsDoc(), decl.getLhs()) && decl.getRhs().isQualifiedName()) { String aliasedName = decl.getRhs().getQualifiedName(); return currentFile.isPrefixRequired(aliasedName) || currentFile.isNameDeclared(aliasedName); @@ -582,16 +586,21 @@ private static boolean isDeclaration(Node nameNode) { } static boolean isConstToBeInferred( - JSDocInfo jsdoc, Node nameNode, boolean isImpliedConst) { + JSDocInfo jsdoc, Node nameNode) { boolean isConst = - isImpliedConst - || nameNode.getParent().isConst() + nameNode.getParent().isConst() + || isExportLhs(nameNode) || (jsdoc != null && jsdoc.hasConstAnnotation()); return isConst && !JsdocUtil.hasAnnotatedType(jsdoc) && !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) { if (rhs == null || !rhs.isCall()) { return false; diff --git a/src/com/google/javascript/jscomp/ijs/FileInfo.java b/src/com/google/javascript/jscomp/ijs/FileInfo.java index d24905e0254..c2fee388ad2 100644 --- a/src/com/google/javascript/jscomp/ijs/FileInfo.java +++ b/src/com/google/javascript/jscomp/ijs/FileInfo.java @@ -19,7 +19,6 @@ import com.google.common.collect.ListMultimap; import com.google.common.collect.MultimapBuilder; -import com.google.javascript.jscomp.Scope; import com.google.javascript.rhino.Node; import java.util.HashSet; import java.util.Set; @@ -33,15 +32,15 @@ final class FileInfo { private final ListMultimap declarations = MultimapBuilder.linkedHashKeys().arrayListValues().build(); - void recordNameDeclaration(Node qnameNode, Scope scope) { - recordDeclaration(PotentialDeclaration.fromName(qnameNode)); + void recordNameDeclaration(Node qualifiedNameNode) { + recordDeclaration(PotentialDeclaration.fromName(qualifiedNameNode)); } - void recordMethod(Node functionNode, Scope scope) { + void recordMethod(Node functionNode) { recordDeclaration(PotentialDeclaration.fromMethod(functionNode)); } - void recordDefine(Node callNode, Scope scope) { + void recordDefine(Node callNode) { recordDeclaration(PotentialDeclaration.fromDefine(callNode)); } @@ -49,7 +48,7 @@ ListMultimap getDeclarations() { return declarations; } - void recordDeclaration(PotentialDeclaration decl) { + private void recordDeclaration(PotentialDeclaration decl) { declarations.put(decl.getFullyQualifiedName(), decl); } diff --git a/src/com/google/javascript/jscomp/ijs/JsdocUtil.java b/src/com/google/javascript/jscomp/ijs/JsdocUtil.java index 238b0018ba8..5f1c80b709e 100644 --- a/src/com/google/javascript/jscomp/ijs/JsdocUtil.java +++ b/src/com/google/javascript/jscomp/ijs/JsdocUtil.java @@ -15,10 +15,6 @@ */ 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.Var; import com.google.javascript.rhino.IR; @@ -36,38 +32,12 @@ final class JsdocUtil { private JsdocUtil() {} - static JSDocInfo updateJsdoc(AbstractCompiler compiler, Node nameNode) { - 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) { + static boolean isPrivate(@Nullable JSDocInfo jsdoc) { return jsdoc != null && jsdoc.getVisibility().equals(Visibility.PRIVATE); } - static JSDocInfo getAllTypeJSDoc() { - return getConstJSDoc(null, new Node(Token.STAR)); + static JSDocInfo getUnusableTypeJSDoc(JSDocInfo oldJSDoc) { + return getConstJSDoc(oldJSDoc, new Node(Token.STAR)); } static JSDocInfo getQmarkTypeJSDoc() { @@ -122,7 +92,10 @@ static JSDocInfo getJSDocForRhs(Node rhs, JSDocInfo oldJSDoc) { return getConstJSDoc(oldJSDoc, new Node(Token.BANG, IR.string("RegExp"))); } break; - default: + case UNDETERMINED: + if (oldJSDoc != null && oldJSDoc.getDescription() != null) { + return getConstJSDoc(oldJSDoc, "string"); + } break; } return null; diff --git a/src/com/google/javascript/jscomp/ijs/PotentialDeclaration.java b/src/com/google/javascript/jscomp/ijs/PotentialDeclaration.java index b4a13d7fa9f..794c15988d7 100644 --- a/src/com/google/javascript/jscomp/ijs/PotentialDeclaration.java +++ b/src/com/google/javascript/jscomp/ijs/PotentialDeclaration.java @@ -125,7 +125,7 @@ final void remove(AbstractCompiler compiler) { * A potential declaration that has a fully qualified name to describe it. * This includes things like: * var/let/const/function/class declarations, - * assignments to a fully qualfied name, + * assignments to a fully qualified name, * and goog.module exports * This is the most common type of potential declaration. */ @@ -162,8 +162,10 @@ void simplify(AbstractCompiler compiler) { if (objLit.hasChildren()) { for (Node key : objLit.children()) { if (!isTypedRhs(key.getLastChild())) { + ConvertToTypedInterface + .maybeWarnForConstWithoutExplicitType(compiler, key.getJSDocInfo(), key); removeStringKeyValue(key); - JsdocUtil.updateJsdoc(compiler, key); + maybeUpdateJsdoc(key); compiler.reportChangeToEnclosingScope(key); } } @@ -196,6 +198,15 @@ private static void removeStringKeyValue(Node stringKey) { 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)); + } + } } /** @@ -259,6 +270,7 @@ Node getRemovableNode() { static boolean isTypedRhs(Node rhs) { return rhs.isFunction() || rhs.isClass() + || NodeUtil.isCallTo(rhs, "goog.defineClass") || (rhs.isQualifiedName() && rhs.matchesQualifiedName("goog.abstractMethod")) || (rhs.isQualifiedName() && rhs.matchesQualifiedName("goog.nullFunction")); } diff --git a/test/com/google/javascript/jscomp/ijs/ConvertToTypedInterfaceTest.java b/test/com/google/javascript/jscomp/ijs/ConvertToTypedInterfaceTest.java index d8b27cbb2bf..ee370485299 100644 --- a/test/com/google/javascript/jscomp/ijs/ConvertToTypedInterfaceTest.java +++ b/test/com/google/javascript/jscomp/ijs/ConvertToTypedInterfaceTest.java @@ -1353,7 +1353,7 @@ public void testRedeclarationOfClassMethodDoesntCrash() { "class Foo {", " constructor() {}", "}", - // TODO(blickly): Would be better if this included the type anntoation. + // TODO(blickly): Would be better if this included the type annotation. "/** @private */", "Foo.prototype.handleEvent_", ""));