diff --git a/src/com/google/javascript/jscomp/ConvertToTypedInterface.java b/src/com/google/javascript/jscomp/ConvertToTypedInterface.java index 4f12d827516..60262b0b00f 100644 --- a/src/com/google/javascript/jscomp/ConvertToTypedInterface.java +++ b/src/com/google/javascript/jscomp/ConvertToTypedInterface.java @@ -19,8 +19,12 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import com.google.common.base.Function; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.ListMultimap; +import com.google.common.collect.MultimapBuilder; +import com.google.common.collect.Ordering; import com.google.javascript.jscomp.NodeTraversal.AbstractShallowStatementCallback; import com.google.javascript.rhino.IR; import com.google.javascript.rhino.JSDocInfo; @@ -30,6 +34,7 @@ import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; import java.util.ArrayList; +import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -470,26 +475,35 @@ void simplify(AbstractCompiler compiler) { private static class FileInfo { private final Set providedNamespaces = new HashSet<>(); private final Set requiredLocalNames = new HashSet<>(); - private final Set seenNames = new HashSet<>(); private final List constructorsToProcess = new ArrayList<>(); - private final List declarations = new ArrayList<>(); + private final ListMultimap declarations = + MultimapBuilder.linkedHashKeys().arrayListValues().build(); void recordDeclaration(Node qnameNode, Scope scope) { checkArgument(qnameNode.isQualifiedName(), qnameNode); - declarations.add(PotentialDeclaration.from(qnameNode, scope)); + String name = + isThisProp(qnameNode) + ? getPrototypeNameOfThisProp(qnameNode) + : qnameNode.getQualifiedName(); + declarations.put(name, PotentialDeclaration.from(qnameNode, scope)); } void recordDefine(Node callNode, Scope scope) { checkArgument(NodeUtil.isCallTo(callNode, "goog.define")); - declarations.add(PotentialDeclaration.from(callNode, scope)); + String name = callNode.getSecondChild().getString(); + declarations.put(name, PotentialDeclaration.from(callNode, scope)); + } + + void recordDeclaration(String name, PotentialDeclaration decl) { + declarations.put(name, decl); } void recordImport(String localName) { requiredLocalNames.add(localName); } - boolean isNameProcessed(String fullyQualifiedName) { - return seenNames.contains(fullyQualifiedName); + boolean isNameDeclared(String fullyQualifiedName) { + return declarations.containsKey(fullyQualifiedName); } static boolean containsPrefix(String fullyQualifiedName, Iterable prefixNamespaces) { @@ -499,27 +513,30 @@ static boolean containsPrefix(String fullyQualifiedName, Iterable prefix } } return false; - } boolean isPrefixProvided(String fullyQualifiedName) { - return containsPrefix(fullyQualifiedName, Iterables.concat(seenNames, providedNamespaces)); + return containsPrefix(fullyQualifiedName, providedNamespaces); } boolean isPrefixRequired(String fullyQualifiedName) { return containsPrefix(fullyQualifiedName, requiredLocalNames); } + boolean isStrictPrefixDeclared(String fullyQualifiedName) { + for (String prefix : declarations.keySet()) { + if (fullyQualifiedName.startsWith(prefix + ".")) { + return true; + } + } + return false; + } + void markConstructorToProcess(Node ctorNode) { checkArgument(ctorNode.isFunction(), ctorNode); constructorsToProcess.add(ctorNode); } - void markNameProcessed(String fullyQualifiedName) { - checkNotNull(fullyQualifiedName); - seenNames.add(fullyQualifiedName); - } - void markProvided(String providedName) { checkNotNull(providedName); providedNamespaces.add(providedName); @@ -530,19 +547,56 @@ private static class SimplifyDeclarations { private final AbstractCompiler compiler; private final FileInfo currentFile; + static final Ordering SHORT_TO_LONG = + Ordering.natural() + .onResultOf( + new Function() { + @Override + public Integer apply(String name) { + return name.replaceAll("[^.]", "").length(); + } + }); + + static final Ordering DECLARATIONS_FIRST = + Ordering.natural() + .onResultOf( + new Function() { + @Override + public Boolean apply(PotentialDeclaration decl) { + return decl.getJsDoc() == null; + } + }); + SimplifyDeclarations(AbstractCompiler compiler, FileInfo currentFile) { this.compiler = compiler; this.currentFile = currentFile; } + private void removeDuplicateDeclarations() { + for (String name : currentFile.declarations.keySet()) { + if (name.startsWith("this.")) { + continue; + } + List declList = currentFile.declarations.get(name); + Collections.sort(declList, DECLARATIONS_FIRST); + while (declList.size() > 1) { + // Don't remove the first declaration (at index 0) + PotentialDeclaration decl = declList.remove(1); + decl.remove(compiler); + } + } + } + public void simplifyAll() { + // Remove duplicate assignments to the same symbol + removeDuplicateDeclarations(); + // Simplify all names in the top-level scope. - for (PotentialDeclaration decl : currentFile.declarations) { - if (NodeUtil.isCallTo(decl.lhs, "goog.define")) { - NodeUtil.deleteNode(decl.lhs.getLastChild(), compiler); - continue; + List seenNames = SHORT_TO_LONG.immutableSortedCopy(currentFile.declarations.keySet()); + for (String name : seenNames) { + for (PotentialDeclaration decl : currentFile.declarations.get(name)) { + processName(name, decl); } - processName(decl); } // Simplify all names inside constructors. for (Node ctorNode : currentFile.constructorsToProcess) { @@ -550,14 +604,12 @@ public void simplifyAll() { } } - private void processName(PotentialDeclaration decl) { - checkArgument(decl.lhs.isQualifiedName()); - Node lhs = decl.lhs; - if (isThisProp(lhs)) { - // Will be handled in processConstructors + private void processName(String qname, PotentialDeclaration decl) { + if (NodeUtil.isCallTo(decl.lhs, "goog.define")) { + NodeUtil.deleteNode(decl.lhs.getLastChild(), compiler); return; } - switch (shouldRemove(decl)) { + switch (shouldRemove(qname, decl)) { case PRESERVE_ALL: if (decl.rhs != null && decl.rhs.isFunction()) { processFunction(decl.rhs); @@ -572,7 +624,6 @@ private void processName(PotentialDeclaration decl) { decl.remove(compiler); break; } - currentFile.markNameProcessed(decl.lhs.getQualifiedName()); } private void processClass(Node n) { @@ -607,8 +658,7 @@ private void processFunctionParameters(Node paramList) { } private void processConstructor(final Node function) { - final String className = getClassName(function); - if (className == null) { + if (getClassName(function) == null) { return; } final Node insertionPoint = NodeUtil.getEnclosingStatement(function); @@ -621,33 +671,20 @@ public void visit(NodeTraversal t, Node n, Node parent) { if (n.isExprResult()) { Node expr = n.getFirstChild(); Node name = expr.isAssign() ? expr.getFirstChild() : expr; - if (!isThisProp(name)) { - return; - } - String pname = name.getLastChild().getString(); - String fullyQualifiedName = className + ".prototype." + pname; - if (currentFile.isNameProcessed(fullyQualifiedName)) { - return; - } - JSDocInfo jsdoc = NodeUtil.getBestJSDocInfo(name); - if (jsdoc == null) { - jsdoc = JsdocUtil.getAllTypeJSDoc(); - } else if (isConstToBeInferred(jsdoc, name, false)) { - jsdoc = JsdocUtil.pullJsdocTypeFromAst(compiler, jsdoc, name); + if (isThisProp(name)) { + String fullyQualifiedName = getPrototypeNameOfThisProp(name); + JSDocInfo jsdoc = NodeUtil.getBestJSDocInfo(name); + Node newProtoAssignStmt = + NodeUtil.newQNameDeclaration(compiler, fullyQualifiedName, null, jsdoc); + newProtoAssignStmt.useSourceInfoIfMissingFromForTree(expr); + // TODO(blickly): Preserve the declaration order of the this properties. + insertionPoint.getParent().addChildAfter(newProtoAssignStmt, insertionPoint); + compiler.reportChangeToEnclosingScope(newProtoAssignStmt); } - Node newProtoAssignStmt = - NodeUtil.newQNameDeclaration(compiler, fullyQualifiedName, null, jsdoc); - newProtoAssignStmt.useSourceInfoIfMissingFromForTree(expr); - // TODO(blickly): Preserve the declaration order of the this properties. - insertionPoint.getParent().addChildAfter(newProtoAssignStmt, insertionPoint); - compiler.reportChangeToEnclosingScope(newProtoAssignStmt); - currentFile.markNameProcessed(fullyQualifiedName); + NodeUtil.deleteNode(n, compiler); } } }); - final Node functionBody = function.getLastChild(); - checkState(functionBody.isNormalBlock()); - NodeUtil.deleteChildren(functionBody, compiler); } enum RemovalType { @@ -665,7 +702,7 @@ private static boolean isExportLhs(Node lhs) { || (lhs.isGetProp() && lhs.getFirstChild().matchesQualifiedName("exports")); } - private RemovalType shouldRemove(PotentialDeclaration decl) { + private RemovalType shouldRemove(String fullyQualifiedName, PotentialDeclaration decl) { Node nameNode = decl.lhs; Node rhs = decl.rhs; if (NodeUtil.getRootOfQualifiedName(nameNode).matchesQualifiedName("$jscomp")) { @@ -687,13 +724,10 @@ private RemovalType shouldRemove(PotentialDeclaration decl) { && (jsdoc == null || !JsdocUtil.hasAnnotatedType(jsdoc)))) { return RemovalType.PRESERVE_ALL; } - if (!isExport - && (jsdoc == null || !jsdoc.containsDeclaration())) { - String fullyQualifiedName = nameNode.getQualifiedName(); - if (currentFile.isNameProcessed(fullyQualifiedName)) { - return RemovalType.REMOVE_ALL; - } - if (isDeclaration(nameNode) || currentFile.isPrefixProvided(fullyQualifiedName)) { + if (!isExport && (jsdoc == null || !jsdoc.containsDeclaration())) { + if (isDeclaration(nameNode) + || currentFile.isPrefixProvided(fullyQualifiedName) + || currentFile.isStrictPrefixDeclared(fullyQualifiedName)) { jsdocNode.setJSDocInfo(JsdocUtil.getAllTypeJSDoc()); return RemovalType.SIMPLIFY_RHS; } @@ -702,17 +736,9 @@ private RemovalType shouldRemove(PotentialDeclaration decl) { if (isConstToBeInferred(jsdoc, nameNode, isExport)) { if (rhs.isQualifiedName() && (currentFile.isPrefixRequired(rhs.getQualifiedName()) - || currentFile.isNameProcessed(rhs.getQualifiedName()))) { + || currentFile.isNameDeclared(rhs.getQualifiedName()))) { return RemovalType.PRESERVE_ALL; } - - Var originalDecl = findNameDeclaration(decl.scope, rhs); - if (originalDecl != null) { - if (originalDecl.isClass() || JsdocUtil.hasAnnotatedType(originalDecl.getJSDocInfo())) { - return RemovalType.PRESERVE_ALL; - } - } - jsdocNode.setJSDocInfo(JsdocUtil.pullJsdocTypeFromAst(compiler, jsdoc, nameNode)); } return RemovalType.SIMPLIFY_RHS; @@ -730,6 +756,14 @@ private static boolean isThisProp(Node getprop) { return getprop.isGetProp() && getprop.getFirstChild().isThis(); } + private static String getPrototypeNameOfThisProp(Node getprop) { + checkArgument(isThisProp(getprop)); + Node function = NodeUtil.getEnclosingFunction(getprop); + String className = getClassName(function); + checkState(className != null && !className.isEmpty()); + return className + ".prototype." + getprop.getLastChild().getString(); + } + private static void replaceRhsWithUnknown(Node rhs) { rhs.replaceWith(IR.cast(IR.number(0), JsdocUtil.getQmarkTypeJSDoc()).srcrefTree(rhs)); } diff --git a/test/com/google/javascript/jscomp/ConvertToTypedInterfaceTest.java b/test/com/google/javascript/jscomp/ConvertToTypedInterfaceTest.java index db84c3cbe02..159edf783c8 100644 --- a/test/com/google/javascript/jscomp/ConvertToTypedInterfaceTest.java +++ b/test/com/google/javascript/jscomp/ConvertToTypedInterfaceTest.java @@ -189,6 +189,33 @@ public void testConstJsdocPropagationForConstructorNames() { "/** @const {number} */ Foo.prototype.x;")); } + public void testMultipleSameNamedThisProperties() { + test( + lines( + "class Foo {", + " constructor(/** number */ x) {", + " /** @const */ this.x = x;", + " }", + "}", + "/** @template T */", + "class Bar {", + " constructor(/** T */ x) {", + " /** @const */ this.x = x;", + " }", + "}"), + lines( + "class Foo {", + " constructor(/** number */ x) {}", + "}", + "/** @const {number} */ Foo.prototype.x;", + "/** @template T */", + "class Bar {", + " constructor(/** T */ x) {}", + "}", + "/** @const {T} */ Bar.prototype.x;")); + + } + public void testLegacyGoogModule() { testSame( lines( @@ -357,6 +384,24 @@ public void testConstJsdocPropagationForNames_optional() { } + public void testNotConfusedByOutOfOrderDeclarations() { + test( + lines( + "/** @constructor */", + "function Foo(/** boolean= */ opt_tag) {", + " if (opt_tag) {", + " Foo.tag = opt_tag;", + " }", + "}", + "/** @type {boolean} */ Foo.tag = true;", + ""), + lines( + "/** @constructor */", + "function Foo(/** boolean= */ opt_tag) {}", + "/** @type {boolean} */ Foo.tag;")); + + } + public void testConstJsdocPropagationForNames_rest() { test( lines(