Skip to content

Commit

Permalink
Make explicit the order to do our type summary traversal, rather than…
Browse files Browse the repository at this point in the history 
… relying on AST order.

In particular, this means we:
* Process declarations with unqualified names coming before qualified names, and
* Consider declarations with JSDoc to be canonical

Also, move the pruning of constructor bodies together with the rest of the pruning, and simplify processConstructor.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=176137310
  • Loading branch information
@blickly
blickly authored and Tyler Breisacher committed Nov 17, 2017
1 parent daa6b7a commit 3717cb1
Show file tree
Hide file tree
Showing 2 changed files with 146 additions and 67 deletions.
168 changes: 101 additions & 67 deletions src/com/google/javascript/jscomp/ConvertToTypedInterface.java
View file
Expand Up @@ -19,8 +19,12 @@
import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState; 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.ImmutableSet;
import com.google.common.collect.Iterables; 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.jscomp.NodeTraversal.AbstractShallowStatementCallback;
import com.google.javascript.rhino.IR; import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.JSDocInfo; import com.google.javascript.rhino.JSDocInfo;
Expand All @@ -30,6 +34,7 @@
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.Token; import com.google.javascript.rhino.Token;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet; import java.util.HashSet;
import java.util.List; import java.util.List;
import java.util.Set; import java.util.Set;
Expand Down Expand Up @@ -470,26 +475,35 @@ void simplify(AbstractCompiler compiler) {
private static class FileInfo { private static class FileInfo {
private final Set<String> providedNamespaces = new HashSet<>(); private final Set<String> providedNamespaces = new HashSet<>();
private final Set<String> requiredLocalNames = new HashSet<>(); private final Set<String> requiredLocalNames = new HashSet<>();
private final Set<String> seenNames = new HashSet<>();
private final List<Node> constructorsToProcess = new ArrayList<>(); private final List<Node> constructorsToProcess = new ArrayList<>();
private final List<PotentialDeclaration> declarations = new ArrayList<>(); private final ListMultimap<String, PotentialDeclaration> declarations =
MultimapBuilder.linkedHashKeys().arrayListValues().build();


void recordDeclaration(Node qnameNode, Scope scope) { void recordDeclaration(Node qnameNode, Scope scope) {
checkArgument(qnameNode.isQualifiedName(), qnameNode); 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) { void recordDefine(Node callNode, Scope scope) {
checkArgument(NodeUtil.isCallTo(callNode, "goog.define")); 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) { void recordImport(String localName) {
requiredLocalNames.add(localName); requiredLocalNames.add(localName);
} }


boolean isNameProcessed(String fullyQualifiedName) { boolean isNameDeclared(String fullyQualifiedName) {
return seenNames.contains(fullyQualifiedName); return declarations.containsKey(fullyQualifiedName);
} }


static boolean containsPrefix(String fullyQualifiedName, Iterable<String> prefixNamespaces) { static boolean containsPrefix(String fullyQualifiedName, Iterable<String> prefixNamespaces) {
Expand All @@ -499,27 +513,30 @@ static boolean containsPrefix(String fullyQualifiedName, Iterable<String> prefix
} }
} }
return false; return false;

} }


boolean isPrefixProvided(String fullyQualifiedName) { boolean isPrefixProvided(String fullyQualifiedName) {
return containsPrefix(fullyQualifiedName, Iterables.concat(seenNames, providedNamespaces)); return containsPrefix(fullyQualifiedName, providedNamespaces);
} }


boolean isPrefixRequired(String fullyQualifiedName) { boolean isPrefixRequired(String fullyQualifiedName) {
return containsPrefix(fullyQualifiedName, requiredLocalNames); 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) { void markConstructorToProcess(Node ctorNode) {
checkArgument(ctorNode.isFunction(), ctorNode); checkArgument(ctorNode.isFunction(), ctorNode);
constructorsToProcess.add(ctorNode); constructorsToProcess.add(ctorNode);
} }


void markNameProcessed(String fullyQualifiedName) {
checkNotNull(fullyQualifiedName);
seenNames.add(fullyQualifiedName);
}

void markProvided(String providedName) { void markProvided(String providedName) {
checkNotNull(providedName); checkNotNull(providedName);
providedNamespaces.add(providedName); providedNamespaces.add(providedName);
Expand All @@ -530,34 +547,69 @@ private static class SimplifyDeclarations {
private final AbstractCompiler compiler; private final AbstractCompiler compiler;
private final FileInfo currentFile; private final FileInfo currentFile;


static final Ordering<String> SHORT_TO_LONG =
Ordering.natural()
.onResultOf(
new Function<String, Integer>() {
@Override
public Integer apply(String name) {
return name.replaceAll("[^.]", "").length();
}
});

static final Ordering<PotentialDeclaration> DECLARATIONS_FIRST =
Ordering.natural()
.onResultOf(
new Function<PotentialDeclaration, Boolean>() {
@Override
public Boolean apply(PotentialDeclaration decl) {
return decl.getJsDoc() == null;
}
});

SimplifyDeclarations(AbstractCompiler compiler, FileInfo currentFile) { SimplifyDeclarations(AbstractCompiler compiler, FileInfo currentFile) {
this.compiler = compiler; this.compiler = compiler;
this.currentFile = currentFile; this.currentFile = currentFile;
} }


private void removeDuplicateDeclarations() {
for (String name : currentFile.declarations.keySet()) {
if (name.startsWith("this.")) {
continue;
}
List<PotentialDeclaration> 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() { public void simplifyAll() {
// Remove duplicate assignments to the same symbol
removeDuplicateDeclarations();

// Simplify all names in the top-level scope. // Simplify all names in the top-level scope.
for (PotentialDeclaration decl : currentFile.declarations) { List<String> seenNames = SHORT_TO_LONG.immutableSortedCopy(currentFile.declarations.keySet());
if (NodeUtil.isCallTo(decl.lhs, "goog.define")) { for (String name : seenNames) {
NodeUtil.deleteNode(decl.lhs.getLastChild(), compiler); for (PotentialDeclaration decl : currentFile.declarations.get(name)) {
continue; processName(name, decl);
} }
processName(decl);
} }
// Simplify all names inside constructors. // Simplify all names inside constructors.
for (Node ctorNode : currentFile.constructorsToProcess) { for (Node ctorNode : currentFile.constructorsToProcess) {
processConstructor(ctorNode); processConstructor(ctorNode);
} }
} }


private void processName(PotentialDeclaration decl) { private void processName(String qname, PotentialDeclaration decl) {
checkArgument(decl.lhs.isQualifiedName()); if (NodeUtil.isCallTo(decl.lhs, "goog.define")) {
Node lhs = decl.lhs; NodeUtil.deleteNode(decl.lhs.getLastChild(), compiler);
if (isThisProp(lhs)) {
// Will be handled in processConstructors
return; return;
} }
switch (shouldRemove(decl)) { switch (shouldRemove(qname, decl)) {
case PRESERVE_ALL: case PRESERVE_ALL:
if (decl.rhs != null && decl.rhs.isFunction()) { if (decl.rhs != null && decl.rhs.isFunction()) {
processFunction(decl.rhs); processFunction(decl.rhs);
Expand All @@ -572,7 +624,6 @@ private void processName(PotentialDeclaration decl) {
decl.remove(compiler); decl.remove(compiler);
break; break;
} }
currentFile.markNameProcessed(decl.lhs.getQualifiedName());
} }


private void processClass(Node n) { private void processClass(Node n) {
Expand Down Expand Up @@ -607,8 +658,7 @@ private void processFunctionParameters(Node paramList) {
} }


private void processConstructor(final Node function) { private void processConstructor(final Node function) {
final String className = getClassName(function); if (getClassName(function) == null) {
if (className == null) {
return; return;
} }
final Node insertionPoint = NodeUtil.getEnclosingStatement(function); final Node insertionPoint = NodeUtil.getEnclosingStatement(function);
Expand All @@ -621,33 +671,20 @@ public void visit(NodeTraversal t, Node n, Node parent) {
if (n.isExprResult()) { if (n.isExprResult()) {
Node expr = n.getFirstChild(); Node expr = n.getFirstChild();
Node name = expr.isAssign() ? expr.getFirstChild() : expr; Node name = expr.isAssign() ? expr.getFirstChild() : expr;
if (!isThisProp(name)) { if (isThisProp(name)) {
return; String fullyQualifiedName = getPrototypeNameOfThisProp(name);
} JSDocInfo jsdoc = NodeUtil.getBestJSDocInfo(name);
String pname = name.getLastChild().getString(); Node newProtoAssignStmt =
String fullyQualifiedName = className + ".prototype." + pname; NodeUtil.newQNameDeclaration(compiler, fullyQualifiedName, null, jsdoc);
if (currentFile.isNameProcessed(fullyQualifiedName)) { newProtoAssignStmt.useSourceInfoIfMissingFromForTree(expr);
return; // TODO(blickly): Preserve the declaration order of the this properties.
} insertionPoint.getParent().addChildAfter(newProtoAssignStmt, insertionPoint);
JSDocInfo jsdoc = NodeUtil.getBestJSDocInfo(name); compiler.reportChangeToEnclosingScope(newProtoAssignStmt);
if (jsdoc == null) {
jsdoc = JsdocUtil.getAllTypeJSDoc();
} else if (isConstToBeInferred(jsdoc, name, false)) {
jsdoc = JsdocUtil.pullJsdocTypeFromAst(compiler, jsdoc, name);
} }
Node newProtoAssignStmt = NodeUtil.deleteNode(n, compiler);
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);
} }
} }
}); });
final Node functionBody = function.getLastChild();
checkState(functionBody.isNormalBlock());
NodeUtil.deleteChildren(functionBody, compiler);
} }


enum RemovalType { enum RemovalType {
Expand All @@ -665,7 +702,7 @@ private static boolean isExportLhs(Node lhs) {
|| (lhs.isGetProp() && lhs.getFirstChild().matchesQualifiedName("exports")); || (lhs.isGetProp() && lhs.getFirstChild().matchesQualifiedName("exports"));
} }


private RemovalType shouldRemove(PotentialDeclaration decl) { private RemovalType shouldRemove(String fullyQualifiedName, PotentialDeclaration decl) {
Node nameNode = decl.lhs; Node nameNode = decl.lhs;
Node rhs = decl.rhs; Node rhs = decl.rhs;
if (NodeUtil.getRootOfQualifiedName(nameNode).matchesQualifiedName("$jscomp")) { if (NodeUtil.getRootOfQualifiedName(nameNode).matchesQualifiedName("$jscomp")) {
Expand All @@ -687,13 +724,10 @@ private RemovalType shouldRemove(PotentialDeclaration decl) {
&& (jsdoc == null || !JsdocUtil.hasAnnotatedType(jsdoc)))) { && (jsdoc == null || !JsdocUtil.hasAnnotatedType(jsdoc)))) {
return RemovalType.PRESERVE_ALL; return RemovalType.PRESERVE_ALL;
} }
if (!isExport if (!isExport && (jsdoc == null || !jsdoc.containsDeclaration())) {
&& (jsdoc == null || !jsdoc.containsDeclaration())) { if (isDeclaration(nameNode)
String fullyQualifiedName = nameNode.getQualifiedName(); || currentFile.isPrefixProvided(fullyQualifiedName)
if (currentFile.isNameProcessed(fullyQualifiedName)) { || currentFile.isStrictPrefixDeclared(fullyQualifiedName)) {
return RemovalType.REMOVE_ALL;
}
if (isDeclaration(nameNode) || currentFile.isPrefixProvided(fullyQualifiedName)) {
jsdocNode.setJSDocInfo(JsdocUtil.getAllTypeJSDoc()); jsdocNode.setJSDocInfo(JsdocUtil.getAllTypeJSDoc());
return RemovalType.SIMPLIFY_RHS; return RemovalType.SIMPLIFY_RHS;
} }
Expand All @@ -702,17 +736,9 @@ private RemovalType shouldRemove(PotentialDeclaration decl) {
if (isConstToBeInferred(jsdoc, nameNode, isExport)) { if (isConstToBeInferred(jsdoc, nameNode, isExport)) {
if (rhs.isQualifiedName() if (rhs.isQualifiedName()
&& (currentFile.isPrefixRequired(rhs.getQualifiedName()) && (currentFile.isPrefixRequired(rhs.getQualifiedName())
|| currentFile.isNameProcessed(rhs.getQualifiedName()))) { || currentFile.isNameDeclared(rhs.getQualifiedName()))) {
return RemovalType.PRESERVE_ALL; 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)); jsdocNode.setJSDocInfo(JsdocUtil.pullJsdocTypeFromAst(compiler, jsdoc, nameNode));
} }
return RemovalType.SIMPLIFY_RHS; return RemovalType.SIMPLIFY_RHS;
Expand All @@ -730,6 +756,14 @@ private static boolean isThisProp(Node getprop) {
return getprop.isGetProp() && getprop.getFirstChild().isThis(); 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) { private static void replaceRhsWithUnknown(Node rhs) {
rhs.replaceWith(IR.cast(IR.number(0), JsdocUtil.getQmarkTypeJSDoc()).srcrefTree(rhs)); rhs.replaceWith(IR.cast(IR.number(0), JsdocUtil.getQmarkTypeJSDoc()).srcrefTree(rhs));
} }
Expand Down
45 changes: 45 additions & 0 deletions test/com/google/javascript/jscomp/ConvertToTypedInterfaceTest.java
View file
Expand Up @@ -189,6 +189,33 @@ public void testConstJsdocPropagationForConstructorNames() {
"/** @const {number} */ Foo.prototype.x;")); "/** @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() { public void testLegacyGoogModule() {
testSame( testSame(
lines( lines(
Expand Down Expand Up @@ -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() { public void testConstJsdocPropagationForNames_rest() {
test( test(
lines( lines(
Expand Down

0 comments on commit 3717cb1

Please sign in to comment.