Skip to content

Commit

Permalink
Roll forward of "Use ModuleMetadata as common code in JsfileParser to…
Browse files Browse the repository at this point in the history 
… parse information. This has more validation and ensures imports are in order."

NEW:
- Use ordered Multisets in ModuleMetadata. Tools were expecting to see all provides/requires, including duplicates.
- Report duplicate provides in the same file. Useful for tools that don't report this.
- Check for uses of the "goog" variable that aren't just method calls.
- A file is only considered "test only" if the call to goog.setTestOnly is valid.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=200784208
  • Loading branch information
@blickly
johnplaisted authored and blickly committed Jun 15, 2018
1 parent f725964 commit c654d8e
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 118 deletions.
81 changes: 59 additions & 22 deletions src/com/google/javascript/jscomp/ModuleMetadata.java
View file
Expand Up @@ -22,7 +22,8 @@

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableMultiset;
import com.google.common.collect.LinkedHashMultiset;
import com.google.javascript.jscomp.NodeTraversal.Callback;
import com.google.javascript.jscomp.deps.ModuleLoader.ModulePath;
import com.google.javascript.jscomp.deps.ModuleLoader.ResolutionMode;
Expand Down Expand Up @@ -84,6 +85,11 @@ public enum ModuleType {
DiagnosticType.error(
"JSC_INVALID_REQUIRE_TYPE", "Argument to goog.requireType must be a string.");

static final DiagnosticType INVALID_SET_TEST_ONLY =
DiagnosticType.error(
"JSC_INVALID_SET_TEST_ONLY",
"Optional, single argument to goog.setTestOnly must be a string.");

private static final Node GOOG_PROVIDE = IR.getprop(IR.name("goog"), IR.string("provide"));
private static final Node GOOG_MODULE = IR.getprop(IR.name("goog"), IR.string("module"));
private static final Node GOOG_REQUIRE = IR.getprop(IR.name("goog"), IR.string("require"));
Expand Down Expand Up @@ -190,19 +196,19 @@ public boolean isScript() {
* Closure namespaces that this file is associated with. Created by goog.provide, goog.module,
* and goog.module.declareNamespace.
*/
public abstract ImmutableSet<String> googNamespaces();
public abstract ImmutableMultiset<String> googNamespaces();

/** Closure namespaces this file requires. e.g. all arguments to goog.require calls. */
public abstract ImmutableSet<String> requiredGoogNamespaces();
public abstract ImmutableMultiset<String> requiredGoogNamespaces();

/**
* Closure namespaces this file has weak dependencies on. e.g. all arguments to goog.requireType
* calls.
*/
public abstract ImmutableSet<String> requiredTypes();
public abstract ImmutableMultiset<String> requiredTypes();

/** Raw text of all ES6 import specifiers (includes "export from" as well). */
public abstract ImmutableSet<String> es6ImportSpecifiers();
public abstract ImmutableMultiset<String> es6ImportSpecifiers();

abstract ImmutableList<Module> nestedModules();

Expand Down Expand Up @@ -247,12 +253,13 @@ abstract static class Builder {
private Node declaresLegacyNamespace;
private Node rootNode;
private AbstractCompiler compiler;
LinkedHashMultiset<String> googNamespaces = LinkedHashMultiset.create();

abstract Module buildInternal();
abstract ImmutableSet.Builder<String> googNamespacesBuilder();
abstract ImmutableSet.Builder<String> requiredGoogNamespacesBuilder();
abstract ImmutableSet.Builder<String> requiredTypesBuilder();
abstract ImmutableSet.Builder<String> es6ImportSpecifiersBuilder();
abstract Builder googNamespaces(ImmutableMultiset<String> value);
abstract ImmutableMultiset.Builder<String> requiredGoogNamespacesBuilder();
abstract ImmutableMultiset.Builder<String> requiredTypesBuilder();
abstract ImmutableMultiset.Builder<String> es6ImportSpecifiersBuilder();
abstract ImmutableList.Builder<Module> nestedModulesBuilder();
abstract Builder path(@Nullable ModulePath value);
abstract Builder usesClosure(boolean value);
Expand Down Expand Up @@ -289,6 +296,7 @@ boolean isScript() {
}

Module build() {
googNamespaces(ImmutableMultiset.copyOf(googNamespaces));
if (!ambiguous) {
if (declaresNamespace != null && moduleType() != ModuleType.ES6_MODULE) {
compiler.report(
Expand Down Expand Up @@ -385,6 +393,9 @@ public void visit(NodeTraversal t, Node n, Node parent) {
case SCRIPT:
leaveModule();
break;
case NAME:
visitName(t, n);
break;
case CALL:
if (loadModuleCall == n) {
leaveModule();
Expand All @@ -409,6 +420,19 @@ private boolean isFromGoogImport(Var goog) {
&& nameNode.getParent().getLastChild().getString().endsWith("/goog.js");
}

private void visitName(NodeTraversal t, Node n) {
if (!"goog".equals(n.getString())) {
return;
}

Var root = t.getScope().getVar("goog");
if (root != null && !isFromGoogImport(root)) {
return;
}

currentModule.usesClosure(true);
}

private void visitGoogCall(NodeTraversal t, Node n) {
if (!n.hasChildren()
|| !n.getFirstChild().isGetProp()
Expand Down Expand Up @@ -439,17 +463,15 @@ private void visitGoogCall(NodeTraversal t, Node n) {
currentModule.moduleType(ModuleType.GOOG_PROVIDE, t, n);
if (n.hasTwoChildren() && n.getLastChild().isString()) {
String namespace = n.getLastChild().getString();
currentModule.googNamespacesBuilder().add(namespace);
checkDuplicates(namespace, t, n);
addNamespace(currentModule, namespace, t, n);
} else {
t.report(n, ClosureRewriteModule.INVALID_PROVIDE_NAMESPACE);
}
} else if (getprop.matchesQualifiedName(GOOG_MODULE)) {
currentModule.moduleType(ModuleType.GOOG_MODULE, t, n);
if (n.hasTwoChildren() && n.getLastChild().isString()) {
String namespace = n.getLastChild().getString();
currentModule.googNamespacesBuilder().add(namespace);
checkDuplicates(namespace, t, n);
addNamespace(currentModule, namespace, t, n);
} else {
t.report(n, ClosureRewriteModule.INVALID_MODULE_NAMESPACE);
}
Expand All @@ -462,8 +484,7 @@ private void visitGoogCall(NodeTraversal t, Node n) {
if (n.hasTwoChildren() && n.getLastChild().isString()) {
currentModule.recordDeclareNamespace(n);
String namespace = n.getLastChild().getString();
currentModule.googNamespacesBuilder().add(namespace);
checkDuplicates(namespace, t, n);
addNamespace(currentModule, namespace, t, n);
} else {
t.report(n, INVALID_DECLARE_NAMESPACE_CALL);
}
Expand All @@ -480,15 +501,31 @@ private void visitGoogCall(NodeTraversal t, Node n) {
t.report(n, INVALID_REQUIRE_TYPE);
}
} else if (getprop.matchesQualifiedName(GOOG_SET_TEST_ONLY)) {
currentModule.isTestOnly(true);
if (n.hasOneChild() || (n.hasTwoChildren() && n.getLastChild().isString())) {
currentModule.isTestOnly(true);
} else {
t.report(n, INVALID_SET_TEST_ONLY);
}
}
}

/** Checks if the given Closure namespace is a duplicate or not. */
private void checkDuplicates(String namespace, NodeTraversal t, Node n) {
Module existing = modulesByGoogNamespace.get(namespace);
if (existing != null) {
switch (existing.moduleType()) {
/**
* Adds the namespaces to the module and checks if the given Closure namespace is a duplicate or
* not.
*/
private void addNamespace(Module.Builder module, String namespace, NodeTraversal t, Node n) {
ModuleType existingType = null;
if (module.googNamespaces.contains(namespace)) {
existingType = module.moduleType();
} else {
Module existingModule = modulesByGoogNamespace.get(namespace);
if (existingModule != null) {
existingType = existingModule.moduleType();
}
}
currentModule.googNamespaces.add(namespace);
if (existingType != null) {
switch (existingType) {
case ES6_MODULE:
case GOOG_MODULE:
case LEGACY_GOOG_MODULE:
Expand All @@ -501,7 +538,7 @@ private void checkDuplicates(String namespace, NodeTraversal t, Node n) {
case SCRIPT:
// Fall through, error
}
throw new IllegalStateException("Unexpected module type: " + existing.moduleType());
throw new IllegalStateException("Unexpected module type: " + existingType);
}
}
}
Expand Down
147 changes: 51 additions & 96 deletions src/com/google/javascript/jscomp/gwt/client/JsfileParser.java
View file
Expand Up @@ -16,21 +16,23 @@

package com.google.javascript.jscomp.gwt.client;

import static com.google.common.base.Preconditions.checkState;

import com.google.common.base.Joiner;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Multiset;
import com.google.common.collect.Ordering;
import com.google.common.collect.TreeMultiset;
import com.google.javascript.jscomp.BasicErrorManager;
import com.google.javascript.jscomp.CheckLevel;
import com.google.javascript.jscomp.Compiler;
import com.google.javascript.jscomp.CompilerOptions;
import com.google.javascript.jscomp.NodeTraversal;
import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
import com.google.javascript.jscomp.JSError;
import com.google.javascript.jscomp.ModuleMetadata;
import com.google.javascript.jscomp.ModuleMetadata.Module;
import com.google.javascript.jscomp.SourceFile;
import com.google.javascript.jscomp.Var;
import com.google.javascript.jscomp.gwt.client.Util.JsArray;
import com.google.javascript.jscomp.gwt.client.Util.JsObject;
import com.google.javascript.jscomp.gwt.client.Util.JsRegExp;
Expand All @@ -39,8 +41,8 @@
import com.google.javascript.jscomp.parsing.parser.trees.Comment;
import com.google.javascript.rhino.ErrorReporter;
import com.google.javascript.rhino.InputId;
import com.google.javascript.rhino.Node;
import java.util.ArrayList;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
import java.util.TreeSet;
Expand Down Expand Up @@ -90,7 +92,8 @@ static final class FileInfo {

final Set<String> hasSoyDelcalls = new TreeSet<>();
final Set<String> hasSoyDeltemplates = new TreeSet<>();
final Set<String> importedModules = new TreeSet<>();
// Use a LinkedHashSet as import order matters!
final Set<String> importedModules = new LinkedHashSet<>();
final List<String> modName = new ArrayList<>();
final List<String> mods = new ArrayList<>();

Expand Down Expand Up @@ -218,9 +221,34 @@ public static JsObject<Object> gjd(String code, String filename, @Nullable Repor
/** Internal implementation to produce the {@link FileInfo} object. */
private static FileInfo parse(String code, String filename, @Nullable Reporter reporter) {
ErrorReporter errorReporter = new DelegatingReporter(reporter);
Compiler compiler = new Compiler();
Compiler compiler =
new Compiler(
new BasicErrorManager() {
@Override
public void println(CheckLevel level, JSError error) {
if (level == CheckLevel.ERROR) {
errorReporter.error(
error.description,
error.sourceName,
error.getLineNumber(),
error.getCharno());
} else if (level == CheckLevel.WARNING) {
errorReporter.warning(
error.description,
error.sourceName,
error.getLineNumber(),
error.getCharno());
}
}

@Override
protected void printSummary() {}
});
SourceFile source = SourceFile.fromCode(filename, code);
compiler.init(
ImmutableList.<SourceFile>of(), ImmutableList.<SourceFile>of(), new CompilerOptions());
ImmutableList.<SourceFile>of(),
ImmutableList.<SourceFile>of(source),
new CompilerOptions());

Config config =
ParserRunner.createConfig(
Expand All @@ -231,8 +259,6 @@ private static FileInfo parse(String code, String filename, @Nullable Reporter r
/* extraAnnotationNames */ ImmutableSet.<String>of(),
/* parseInlineSourceMaps */ true,
Config.StrictMode.SLOPPY);

SourceFile source = SourceFile.fromCode(filename, code);
FileInfo info = new FileInfo(errorReporter);
ParserRunner.ParseResult parsed = ParserRunner.parse(source, code, config, errorReporter);
parsed.ast.setInputId(new InputId(filename));
Expand All @@ -246,7 +272,21 @@ private static FileInfo parse(String code, String filename, @Nullable Reporter r
parseComment(comment, info);
}
}
NodeTraversal.traverse(compiler, parsed.ast, new Traverser(info));
ModuleMetadata moduleMetadata = new ModuleMetadata(compiler);
moduleMetadata.hotSwapScript(parsed.ast);
compiler.generateReport();
Module module = Iterables.getOnlyElement(moduleMetadata.getModulesByPath().values());
if (module.isEs6Module()) {
info.loadFlags.add(JsArray.of("module", "es6"));
} else if (module.isGoogModule()) {
info.loadFlags.add(JsArray.of("module", "goog"));
}
info.provides.addAll(module.googNamespaces());
info.requires.addAll(module.requiredGoogNamespaces());
info.typeRequires.addAll(module.requiredTypes());
info.testonly = module.isTestOnly();
info.importedModules.addAll(module.es6ImportSpecifiers());
info.goog = module.usesClosure();
return info;
}

Expand Down Expand Up @@ -319,91 +359,6 @@ private static void parseComment(Comment comment, FileInfo info) {
}
}

/** Traverser that mutates {@code #info} with information from the AST. */
private static class Traverser extends AbstractPostOrderCallback {
final FileInfo info;

Traverser(FileInfo info) {
this.info = info;
}

private boolean isFromGoogImport(Var goog) {
Node nameNode = goog.getNameNode();

// Because other tools are regex based we force importing this file as "import * as goog".
return nameNode != null
&& nameNode.isImportStar()
&& nameNode.getString().equals("goog")
&& nameNode.getParent().getFirstChild().isEmpty()
&& nameNode.getParent().getLastChild().getString().endsWith("/goog.js");
}

@Override
public void visit(NodeTraversal traversal, Node node, Node parent) {
// Look for goog.* calls
if (node.isGetProp() && node.getFirstChild().isName()
&& node.getFirstChild().getString().equals("goog")) {
Var root = traversal.getScope().getVar("goog");
if (root == null || isFromGoogImport(root)) {
info.goog = true;
if (parent.isCall() && parent.getChildCount() < 3) {
Node arg;
switch (node.getLastChild().getString()) {
case "module":
info.loadFlags.add(JsArray.of("module", "goog"));
// fall through
case "provide":
arg = parent.getSecondChild();
if (arg.isString()) {
info.provides.add(arg.getString());
} // TODO(sdh): else warning?
break;
case "require":
arg = parent.getSecondChild();
if (arg.isString()) {
info.requires.add(arg.getString());
} // TODO(sdh): else warning?
break;
case "requireType":
arg = parent.getSecondChild();
if (arg.isString()) {
info.typeRequires.add(arg.getString());
} // TODO(blickly): else warning?
break;
case "setTestOnly":
info.testonly = true;
break;
default:
// Do nothing
}
} else if (parent.isGetProp()
&& parent.matchesQualifiedName("goog.module.declareNamespace")
&& parent.getParent().isCall()) {
Node arg = parent.getParent().getSecondChild();
if (arg.isString()) {
info.provides.add(arg.getString());
} // TODO(johnplaisted): else warning?
}
}
}

// Look for ES6 import statements
if (node.isImport()) {
Node moduleSpecifier = node.getChildAtIndex(2);
// NOTE: previous tool was more forgiving here.
checkState(moduleSpecifier.isString());
info.loadFlags.add(JsArray.of("module", "es6"));
info.importedModules.add(moduleSpecifier.getString());
} else if (node.isExport()) {
info.loadFlags.add(JsArray.of("module", "es6"));
// export from
if (node.hasTwoChildren() && node.getLastChild().isString()) {
info.importedModules.add(node.getLastChild().getString());
}
}
}
}

/** JS function interface for reporting errors. */
@JsFunction
public interface Reporter {
Expand Down

0 comments on commit c654d8e

Please sign in to comment.