From 26f0eddc65493542951584e82aee1042d0f2371d Mon Sep 17 00:00:00 2001 From: blickly Date: Tue, 13 Feb 2018 12:13:50 -0800 Subject: [PATCH] Split up ConvertToTypedInterface.shouldRemove It was previously sorting out which declarations to remove, preserve, simplify, as well as attaching the unusable JSDoc (/** @const {*} */) for undeclared symbols and warning. This change splits up those different tasks across separate methods without changing the behavior. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=185567955 --- .../jscomp/ijs/ConvertToTypedInterface.java | 108 +++++++----------- .../jscomp/ijs/PotentialDeclaration.java | 33 +++++- .../ijs/ConvertToTypedInterfaceTest.java | 4 + 3 files changed, 79 insertions(+), 66 deletions(-) diff --git a/src/com/google/javascript/jscomp/ijs/ConvertToTypedInterface.java b/src/com/google/javascript/jscomp/ijs/ConvertToTypedInterface.java index 2a7f02a520b..a6d6279dacf 100644 --- a/src/com/google/javascript/jscomp/ijs/ConvertToTypedInterface.java +++ b/src/com/google/javascript/jscomp/ijs/ConvertToTypedInterface.java @@ -459,29 +459,29 @@ void simplifyAll() { // Simplify all names in the top-level scope. List seenNames = SHORT_TO_LONG.immutableSortedCopy(currentFile.getDeclarations().keySet()); + for (String name : seenNames) { for (PotentialDeclaration decl : currentFile.getDeclarations().get(name)) { - processDeclaration(decl); + processDeclaration(name, decl); } } } - private void processDeclaration(PotentialDeclaration decl) { - switch (shouldRemove(decl)) { - case PRESERVE_ALL: - if (decl.getRhs() != null && decl.getRhs().isFunction()) { - processFunction(decl.getRhs()); - } else if (decl.getRhs() != null && isClass(decl.getRhs())) { - processClass(decl.getRhs()); - } - break; - case SIMPLIFY_RHS: - decl.simplify(compiler); - break; - case REMOVE_ALL: - decl.remove(compiler); - break; + private void processDeclaration(String name, PotentialDeclaration decl) { + if (shouldRemove(name, decl)) { + decl.remove(compiler); + return; + } + if (isAliasDefinition(decl)) { + return; } + if (decl.getRhs() != null && decl.getRhs().isFunction()) { + processFunction(decl.getRhs()); + } else if (decl.getRhs() != null && isClass(decl.getRhs())) { + processClass(decl.getRhs()); + } + setUndeclaredToUnusableType(decl); + decl.simplify(compiler); } private void processClass(Node n) { @@ -512,12 +512,6 @@ private void processFunctionParameters(Node paramList) { } } - enum RemovalType { - PRESERVE_ALL, - SIMPLIFY_RHS, - REMOVE_ALL, - } - private static boolean isClass(Node n) { return n.isClass() || NodeUtil.isCallTo(n, "goog.defineClass"); } @@ -530,64 +524,48 @@ private static String rootName(String qualifiedName) { return qualifiedName.substring(0, dotIndex); } - private RemovalType shouldRemove(PotentialDeclaration decl) { - String fullyQualifiedName = decl.getFullyQualifiedName(); - if ("$jscomp".equals(rootName(fullyQualifiedName))) { + private boolean shouldRemove(String name, PotentialDeclaration decl) { + if ("$jscomp".equals(rootName(name))) { // These are created by goog.scope processing, but clash with each other // and should not be depended on. - return RemovalType.REMOVE_ALL; + return true; } + // This looks like an update rather than a declaration in this file. + return !decl.isDefiniteDeclaration() + && !currentFile.isPrefixProvided(name) + && !currentFile.isStrictPrefixDeclared(name); + } + + private void setUndeclaredToUnusableType(PotentialDeclaration decl) { Node nameNode = decl.getLhs(); Node rhs = decl.getRhs(); JSDocInfo jsdoc = decl.getJsDoc(); - boolean isExport = isExportLhs(nameNode); - if (rhs == null) { - return RemovalType.SIMPLIFY_RHS; - } - if (PotentialDeclaration.isTypedRhs(rhs) - || isImportRhs(rhs) - || (isExport && (rhs.isQualifiedName() || rhs.isObjectLit())) - || (jsdoc != null && jsdoc.isConstructor() && rhs.isQualifiedName()) - || isAliasDefinition(decl) - || (nameNode.matchesQualifiedName("goog.global") && rhs.isThis()) - || (rhs.isObjectLit() - && !rhs.hasChildren() - && (jsdoc == null || !JsdocUtil.hasAnnotatedType(jsdoc)))) { - return RemovalType.PRESERVE_ALL; - } - if (NodeUtil.isNamespaceDecl(nameNode)) { - return RemovalType.SIMPLIFY_RHS; + if (decl.shouldPreserve() + || NodeUtil.isNamespaceDecl(nameNode) + || (jsdoc != null + && jsdoc.containsDeclaration() + && !isConstToBeInferred(jsdoc, nameNode))) { + return; } + maybeWarnForConstWithoutExplicitType(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()) { - 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.getUnusableTypeJSDoc(jsdoc)); - } - return RemovalType.SIMPLIFY_RHS; + jsdocNode.setJSDocInfo(JsdocUtil.getUnusableTypeJSDoc(jsdoc)); } private boolean isAliasDefinition(PotentialDeclaration decl) { + Node rhs = decl.getRhs(); if (isConstToBeInferred(decl.getJsDoc(), decl.getLhs()) - && decl.getRhs().isQualifiedName()) { - String aliasedName = decl.getRhs().getQualifiedName(); - return currentFile.isPrefixRequired(aliasedName) || currentFile.isNameDeclared(aliasedName); + && rhs != null && rhs.isQualifiedName()) { + String aliasedName = rhs.getQualifiedName(); + return rhs.isThis() + || currentFile.isPrefixRequired(aliasedName) + || currentFile.isNameDeclared(aliasedName); } return false; } } - // TODO(blickly): Move to NodeUtil if it makes more sense there. - private static boolean isDeclaration(Node nameNode) { + static boolean isDeclaration(Node nameNode) { checkArgument(nameNode.isQualifiedName()); Node parent = nameNode.getParent(); switch (parent.getToken()) { @@ -613,12 +591,12 @@ static boolean isConstToBeInferred( && !NodeUtil.isNamespaceDecl(nameNode); } - private static boolean isExportLhs(Node lhs) { + static boolean isExportLhs(Node lhs) { return (lhs.isName() && lhs.matchesQualifiedName("exports")) || (lhs.isGetProp() && lhs.getFirstChild().matchesQualifiedName("exports")); } - private static boolean isImportRhs(@Nullable Node rhs) { + static boolean isImportRhs(@Nullable Node rhs) { if (rhs == null || !rhs.isCall()) { return false; } diff --git a/src/com/google/javascript/jscomp/ijs/PotentialDeclaration.java b/src/com/google/javascript/jscomp/ijs/PotentialDeclaration.java index 6343c42d743..ee678007fe9 100644 --- a/src/com/google/javascript/jscomp/ijs/PotentialDeclaration.java +++ b/src/com/google/javascript/jscomp/ijs/PotentialDeclaration.java @@ -172,7 +172,7 @@ void simplifyNamespace(AbstractCompiler compiler) { @Override void simplify(AbstractCompiler compiler) { - if (getRhs() == null) { + if (getRhs() == null || shouldPreserve()) { return; } Node nameNode = getLhs(); @@ -226,6 +226,23 @@ private static void maybeUpdateJsdoc(Node jsdocNode) { jsdocNode.setJSDocInfo(JsdocUtil.getUnusableTypeJSDoc(jsdoc)); } } + + @Override + boolean shouldPreserve() { + Node rhs = getRhs(); + Node nameNode = getLhs(); + JSDocInfo jsdoc = getJsDoc(); + boolean isExport = ConvertToTypedInterface.isExportLhs(nameNode); + return super.shouldPreserve() + || ConvertToTypedInterface.isImportRhs(rhs) + || (isExport && rhs != null && (rhs.isQualifiedName() || rhs.isObjectLit())) + || (jsdoc != null && jsdoc.isConstructor() && rhs != null && rhs.isQualifiedName()) + || (nameNode.matchesQualifiedName("goog.global") && rhs != null && rhs.isThis()) + || (rhs != null + && rhs.isObjectLit() + && !rhs.hasChildren() + && (jsdoc == null || !JsdocUtil.hasAnnotatedType(jsdoc))); + } } /** @@ -242,6 +259,9 @@ static class ThisPropDeclaration extends PotentialDeclaration { @Override void simplify(AbstractCompiler compiler) { + if (shouldPreserve()) { + return; + } // Just completely remove the RHS, if present, and replace with a getprop. Node newStatement = NodeUtil.newQNameDeclaration(compiler, getFullyQualifiedName(), null, getJsDoc()); @@ -286,6 +306,17 @@ Node getRemovableNode() { } } + boolean isDefiniteDeclaration() { + return ConvertToTypedInterface.isExportLhs(getLhs()) + || (getJsDoc() != null && getJsDoc().containsDeclaration()) + || (rhs != null && PotentialDeclaration.isTypedRhs(rhs)) + || ConvertToTypedInterface.isDeclaration(getLhs()); + } + + boolean shouldPreserve() { + return getRhs() != null && isTypedRhs(getRhs()); + } + static boolean isTypedRhs(Node rhs) { return rhs.isFunction() || rhs.isClass() diff --git a/test/com/google/javascript/jscomp/ijs/ConvertToTypedInterfaceTest.java b/test/com/google/javascript/jscomp/ijs/ConvertToTypedInterfaceTest.java index de8e6bcdc8a..d88a791e99a 100644 --- a/test/com/google/javascript/jscomp/ijs/ConvertToTypedInterfaceTest.java +++ b/test/com/google/javascript/jscomp/ijs/ConvertToTypedInterfaceTest.java @@ -52,6 +52,10 @@ public void testExternsDefinitionsRespected() { test(externs("/** @type {number} */ var x;"), srcs("x = 7;"), expected("")); } + public void testUnannotatedDeclaration() { + test("var x;", "/** @const {*} */ var x;"); + } + public void testSimpleConstJsdocPropagation() { test("/** @const */ var x = 5;", "/** @const {number} */ var x;"); test("/** @const */ var x = true;", "/** @const {boolean} */ var x;");