From 670cb8705d795482faaa5b1032272a7054fac76e Mon Sep 17 00:00:00 2001 From: blickly Date: Thu, 26 Jan 2017 12:00:33 -0800 Subject: [PATCH] Make the .i.js generation pass completely local. This should make it faster as well as simpler, as it can work on a single file at a time without looking at externs. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=145702601 --- .../jscomp/ConvertToTypedInterface.java | 143 +++++++++++------- .../jscomp/ConvertToTypedInterfaceTest.java | 34 +++++ 2 files changed, 119 insertions(+), 58 deletions(-) diff --git a/src/com/google/javascript/jscomp/ConvertToTypedInterface.java b/src/com/google/javascript/jscomp/ConvertToTypedInterface.java index 51b19845e02..3bc654c1d24 100644 --- a/src/com/google/javascript/jscomp/ConvertToTypedInterface.java +++ b/src/com/google/javascript/jscomp/ConvertToTypedInterface.java @@ -16,7 +16,7 @@ package com.google.javascript.jscomp; import com.google.common.base.Preconditions; -import com.google.javascript.jscomp.NodeTraversal.AbstractModuleCallback; +import com.google.common.collect.Iterables; import com.google.javascript.jscomp.NodeTraversal.AbstractShallowStatementCallback; import com.google.javascript.rhino.IR; import com.google.javascript.rhino.JSDocInfo; @@ -25,7 +25,6 @@ import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; import com.google.javascript.rhino.jstype.JSType; - import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -174,38 +173,60 @@ private static JSDocInfo getJSDocForName(Var decl, JSDocInfo oldJSDoc) { } - private static class RemoveCode extends AbstractModuleCallback { - private final AbstractCompiler compiler; - private final Set globalSeenNames = new HashSet<>(); - private final List globalConstructorsToProcess = new ArrayList(); - private Node currentModule = null; - private Set moduleSeenNames; - private List moduleConstructorsToProcess; + /** + * Class to keep track of what has been seen so far in a given file. + * + * This is cleared after each file to make sure that the analysis is working on a per-file basis. + */ + private static class FileInfo { + private final Set prefixNames = new HashSet<>(); + private final Set seenNames = new HashSet<>(); + private final List constructorsToProcess = new ArrayList<>(); + + boolean isNameProcessed(String fullyQualifiedName) { + return seenNames.contains(fullyQualifiedName); + } - RemoveCode(AbstractCompiler compiler) { - this.compiler = compiler; + boolean isPrefixProvided(String fullyQualifiedName) { + for (String prefix : Iterables.concat(seenNames, prefixNames)) { + if (fullyQualifiedName.startsWith(prefix)) { + return true; + } + } + return false; } - void process(Node externs, Node root) { - NodeTraversal.traverseRootsEs6(compiler, this, externs, root); + void markConstructorToProcess(Node ctorNode) { + Preconditions.checkArgument(ctorNode.isFunction()); + constructorsToProcess.add(ctorNode); + } - processConstructors(globalConstructorsToProcess); + void markNameProcessed(String fullyQualifiedName) { + seenNames.add(fullyQualifiedName); } - @Override - public void enterModule(NodeTraversal t, Node scopeRoot) { - currentModule = scopeRoot; - moduleSeenNames = new HashSet<>(); - moduleConstructorsToProcess = new ArrayList<>(); + void markPrefixDefined(String namespacePrefix) { + prefixNames.add(namespacePrefix); } - @Override - public void exitModule(NodeTraversal t, Node scopeRoot) { - processConstructors(moduleConstructorsToProcess); + void clear() { + prefixNames.clear(); + seenNames.clear(); + constructorsToProcess.clear(); + } + + } + + private static class RemoveCode implements NodeTraversal.Callback { + private final AbstractCompiler compiler; + private final FileInfo currentFile = new FileInfo(); - currentModule = null; - moduleSeenNames = null; - moduleConstructorsToProcess = null; + RemoveCode(AbstractCompiler compiler) { + this.compiler = compiler; + } + + void process(Node externs, Node root) { + NodeTraversal.traverseEs6(compiler, root, this); } private void processConstructors(List constructorNodes) { @@ -217,15 +238,22 @@ private void processConstructors(List constructorNodes) { @Override public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { switch (n.getToken()) { + case SCRIPT: + currentFile.clear(); + break; + case CLASS: case FUNCTION: { if (parent.isCall()) { Preconditions.checkState(!parent.getFirstChild().matchesQualifiedName("goog.scope"), parent); } + if (NodeUtil.isStatementParent(parent)) { + currentFile.markNameProcessed(n.getFirstChild().getString()); + } Node body = n.getLastChild(); - if (body.isBlock() && body.hasChildren()) { + if (body.isNormalBlock() && body.hasChildren()) { if (isConstructor(n)) { - markConstructorToProcess(n); + currentFile.markConstructorToProcess(n); return false; } n.getLastChild().removeChildren(); @@ -247,6 +275,7 @@ public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { t.report(n, UNSUPPORTED_GOOG_SCOPE); return false; } else if (callee.matchesQualifiedName("goog.provide")) { + currentFile.markPrefixDefined(expr.getLastChild().getString()); Node childBefore; while (null != (childBefore = n.getPrevious()) && childBefore.getBooleanProp(Node.IS_NAMESPACE)) { @@ -304,6 +333,9 @@ public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { @Override public void visit(NodeTraversal t, Node n, Node parent) { switch (n.getToken()) { + case SCRIPT: + processConstructors(currentFile.constructorsToProcess); + break; case TRY: case DEFAULT_CASE: parent.replaceChild(n, n.getFirstChild().detach()); @@ -346,31 +378,6 @@ public void visit(NodeTraversal t, Node n, Node parent) { } } - private boolean isNameProcessed(String fullyQualifiedName) { - if (currentModule == null) { - return globalSeenNames.contains(fullyQualifiedName); - } else { - return moduleSeenNames.contains(fullyQualifiedName); - } - } - - private void markConstructorToProcess(Node ctorNode) { - Preconditions.checkArgument(ctorNode.isFunction()); - if (currentModule == null) { - globalConstructorsToProcess.add(ctorNode); - } else { - moduleConstructorsToProcess.add(ctorNode); - } - } - - private void markNameProcessed(String fullyQualifiedName) { - if (currentModule == null) { - globalSeenNames.add(fullyQualifiedName); - } else { - moduleSeenNames.add(fullyQualifiedName); - } - } - private void processConstructor(final Node function) { final String className = getClassName(function); if (className == null) { @@ -391,7 +398,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { } String pname = name.getLastChild().getString(); String fullyQualifiedName = className + ".prototype." + pname; - if (isNameProcessed(fullyQualifiedName)) { + if (currentFile.isNameProcessed(fullyQualifiedName)) { return; } JSDocInfo jsdoc = NodeUtil.getBestJSDocInfo(name); @@ -406,7 +413,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { // TODO(blickly): Preserve the declaration order of the this properties. insertionPoint.getParent().addChildAfter(newProtoAssignStmt, insertionPoint); compiler.reportCodeChange(); - markNameProcessed(fullyQualifiedName); + currentFile.markNameProcessed(fullyQualifiedName); } } }); @@ -455,11 +462,15 @@ private RemovalType shouldRemove(Node nameNode) { } if (jsdoc == null || !jsdoc.containsDeclaration()) { - if (isNameProcessed(nameNode.getQualifiedName())) { + String fullyQualifiedName = nameNode.getQualifiedName(); + if (currentFile.isNameProcessed(fullyQualifiedName)) { return RemovalType.REMOVE_ALL; } - jsdocNode.setJSDocInfo(getAllTypeJSDoc()); - return RemovalType.REMOVE_RHS; + if (isDeclaration(nameNode) || currentFile.isPrefixProvided(fullyQualifiedName)) { + jsdocNode.setJSDocInfo(getAllTypeJSDoc()); + return RemovalType.REMOVE_RHS; + } + return RemovalType.REMOVE_ALL; } if (isInferrableConst(jsdoc, nameNode)) { jsdocNode.setJSDocInfo(pullJsdocTypeFromAst(compiler, jsdoc, nameNode)); @@ -485,7 +496,7 @@ private void processName(Node nameNode, Node statement) { maybeRemoveRhs(nameNode, statement, jsdocNode.getJSDocInfo()); break; } - markNameProcessed(nameNode.getQualifiedName()); + currentFile.markNameProcessed(nameNode.getQualifiedName()); } private void removeNode(Node n) { @@ -521,6 +532,22 @@ private void removeEnumValues(Node objLit) { } } + // TODO(blickly): Move to NodeUtil if it makes more sense there. + private static boolean isDeclaration(Node nameNode) { + Preconditions.checkArgument(nameNode.isQualifiedName()); + Node parent = nameNode.getParent(); + switch (parent.getToken()) { + case VAR: + case LET: + case CONST: + case CLASS: + case FUNCTION: + return true; + default: + return false; + } + } + private static boolean isInferrableConst(JSDocInfo jsdoc, Node nameNode) { boolean isConst = nameNode.getParent().isConst() || (jsdoc != null && jsdoc.hasConstAnnotation()); diff --git a/test/com/google/javascript/jscomp/ConvertToTypedInterfaceTest.java b/test/com/google/javascript/jscomp/ConvertToTypedInterfaceTest.java index b890a9810af..a37ec9c999b 100644 --- a/test/com/google/javascript/jscomp/ConvertToTypedInterfaceTest.java +++ b/test/com/google/javascript/jscomp/ConvertToTypedInterfaceTest.java @@ -331,6 +331,40 @@ public void testGoogModules() { }); } + public void testCrossFileModifications() { + test( + LINE_JOINER.join( + "goog.module('a.b.c');", + "othermodule.modify.something = othermodule.modify.something + 1;"), + "goog.module('a.b.c');"); + + testEs6( + LINE_JOINER.join( + "goog.module('a.b.c');", + "class Foo {}", + "Foo.something = othermodule.modify.something + 1;", + "exports = Foo;"), + LINE_JOINER.join( + "goog.module('a.b.c');", + "class Foo {}", + "/** @const {*} */ Foo.something", + "exports = Foo;")); + + test( + LINE_JOINER.join( + "goog.provide('a.b.c');", + "otherfile.modify.something = otherfile.modify.something + 1;"), + "goog.provide('a.b.c');"); + + test( + LINE_JOINER.join( + "goog.provide('a.b.c');", + "a.b.c.something = otherfile.modify.something + 1;"), + LINE_JOINER.join( + "goog.provide('a.b.c');", + "/** @const {*} */ a.b.c.something;")); + } + public void testRemoveCalls() { test("alert('hello'); window.clearTimeout();", "");