Skip to content

Commit

Permalink
Warn for missing requires for getprops that are not callees.
Browse files Browse the repository at this point in the history 
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=141523131
  • Loading branch information
@blickly
tbreisacher authored and blickly committed Dec 9, 2016
1 parent 5409f15 commit 9620505
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 47 deletions.
91 changes: 64 additions & 27 deletions src/com/google/javascript/jscomp/CheckRequiresForConstructors.java
View file
Expand Up @@ -91,14 +91,13 @@ static enum Mode {
static final DiagnosticType MISSING_REQUIRE_WARNING =
DiagnosticType.disabled(
"JSC_MISSING_REQUIRE_WARNING", "missing require: ''{0}''");

static final DiagnosticType MISSING_REQUIRE_FOR_GOOG_SCOPE =
DiagnosticType.disabled(
"JSC_MISSING_REQUIRE_FOR_GOOG_SCOPE", "missing require: ''{0}''");

static final DiagnosticType MISSING_REQUIRE_CALL_WARNING =
DiagnosticType.disabled(
"JSC_MISSING_REQUIRE_CALL_WARNING", "missing require: ''{0}''");
// TODO(tbreisacher): Remove this and just use MISSING_REQUIRE_WARNING.
static final DiagnosticType MISSING_REQUIRE_STRICT_WARNING =
DiagnosticType.disabled("JSC_MISSING_REQUIRE_STRICT_WARNING", "missing require: ''{0}''");

public static final DiagnosticType EXTRA_REQUIRE_WARNING = DiagnosticType.disabled(
"JSC_EXTRA_REQUIRE_WARNING", "extra require: ''{0}''");
Expand Down Expand Up @@ -208,20 +207,20 @@ public void visit(NodeTraversal t, Node n, Node parent) {
}
break;
case NAME:
if (!NodeUtil.isLValue(n)) {
visitQualifiedName(n);
if (!NodeUtil.isLValue(n) && !parent.isGetProp()) {
visitQualifiedName(t, n, parent);
}
break;
case GETPROP:
// If parent is a GETPROP, they will handle the weak usages.
if (!parent.isGetProp() && n.isQualifiedName()) {
visitQualifiedName(n);
visitQualifiedName(t, n, parent);
}
break;
case STRING_KEY:
if (parent.isObjectLit() && !n.hasChildren()) {
// Object literal shorthand. This is a usage of the name.
visitQualifiedName(n);
visitQualifiedName(t, n, parent);
}
break;
case CALL:
Expand Down Expand Up @@ -322,12 +321,16 @@ private void visitScriptNode(NodeTraversal t) {
if (node.isCall()) {
String defaultName = parentNamespace != null ? parentNamespace : namespace;
String nameToReport = Iterables.getFirst(classNames, defaultName);
compiler.report(t.makeError(node, MISSING_REQUIRE_CALL_WARNING, nameToReport));
compiler.report(t.makeError(node, MISSING_REQUIRE_STRICT_WARNING, nameToReport));
} else if (node.getParent().isName()
&& node.getParent().getGrandparent() == googScopeBlock) {
compiler.report(t.makeError(node, MISSING_REQUIRE_FOR_GOOG_SCOPE, namespace));
} else {
compiler.report(t.makeError(node, MISSING_REQUIRE_WARNING, namespace));
if (node.isGetProp() && !node.getParent().isClass()) {
compiler.report(t.makeError(node, MISSING_REQUIRE_STRICT_WARNING, namespace));
} else {
compiler.report(t.makeError(node, MISSING_REQUIRE_WARNING, namespace));
}
}
namespaces.add(namespace);
}
Expand Down Expand Up @@ -375,6 +378,11 @@ private void visitRequire(String localName, Node node) {
if (!requires.containsKey(localName)) {
requires.put(localName, node);
}

// For goog.require('example.Outer.Inner'), add example.Outer as well.
for (String className : getClassNames(localName)) {
requires.put(className, node);
}
}

private void visitImportNode(Node importNode) {
Expand All @@ -397,23 +405,27 @@ private void maybeAddClosurizedNamespace(String requiredName) {
}
}

private void visitGoogRequire(String namespace, Node googRequireCall, Node parent) {
maybeAddClosurizedNamespace(namespace);
if (parent.isName()) {
visitRequire(parent.getString(), googRequireCall);
} else if (parent.isDestructuringLhs() && parent.getFirstChild().isObjectPattern()) {
for (Node stringKey : parent.getFirstChild().children()) {
if (stringKey.hasChildren()) {
visitRequire(stringKey.getFirstChild().getString(), stringKey.getFirstChild());
} else {
visitRequire(stringKey.getString(), stringKey);
}
}
} else {
visitRequire(namespace, googRequireCall);
}
}

private void visitCallNode(NodeTraversal t, Node call, Node parent) {
String required = extractNamespaceIfRequire(call);
if (required != null) {
maybeAddClosurizedNamespace(required);
if (parent.isName()) {
visitRequire(parent.getString(), call);
} else if (parent.isDestructuringLhs() && parent.getFirstChild().isObjectPattern()) {
for (Node stringKey : parent.getFirstChild().children()) {
if (stringKey.hasChildren()) {
visitRequire(stringKey.getFirstChild().getString(), stringKey.getFirstChild());
} else {
visitRequire(stringKey.getString(), stringKey);
}
}
} else {
visitRequire(required, call);
}
visitGoogRequire(required, call, parent);
return;
}
String provided = extractNamespaceIfProvide(call);
Expand Down Expand Up @@ -441,13 +453,30 @@ private void visitCallNode(NodeTraversal t, Node call, Node parent) {
Node root = NodeUtil.getRootOfQualifiedName(callee);
if (root.isName()) {
Var var = t.getScope().getVar(root.getString());
if (var == null || (!var.isExtern() && !var.isLocal())) {
if (var == null || (!var.isExtern() && var.isGlobal())) {
usages.put(callee.getQualifiedName(), call);
}
}
}
}

private void addUsageOfOutermostClassName(Node qname, NodeTraversal t) {
Node root = NodeUtil.getRootOfQualifiedName(qname);
if (!root.isName()) {
return;
}

for (Node n = root.getParent(); n.isGetProp(); n = n.getParent()) {
if (isClassName(n.getLastChild().getString())) {
Var var = t.getScope().getVar(root.getString());
if (var == null || (var.isGlobal() && !var.isExtern())) {
usages.put(n.getQualifiedName(), n);
return;
}
}
}
}

private void addWeakUsagesOfAllPrefixes(String qualifiedName) {
// For "foo.bar.baz.qux" add weak usages for "foo.bar.baz.qux", "foo.bar.baz",
// "foo.bar", and "foo" because those might all be goog.provide'd in different files,
Expand All @@ -459,10 +488,16 @@ private void addWeakUsagesOfAllPrefixes(String qualifiedName) {
weakUsages.add(qualifiedName);
}

private void visitQualifiedName(Node n) {
private void visitQualifiedName(NodeTraversal t, Node n, Node parent) {
Preconditions.checkState(n.isName() || n.isGetProp() || n.isStringKey(), n);
String qualifiedName = n.isStringKey() ? n.getString() : n.getQualifiedName();
addWeakUsagesOfAllPrefixes(qualifiedName);
if (mode != Mode.SINGLE_FILE) { // TODO(tbreisacher): Fix violations and remove this check.
return;
}
if (!n.isStringKey() && !NodeUtil.isLhsOfAssign(n) && !parent.isExprResult()) {
addUsageOfOutermostClassName(n, t);
}
}

private void visitNewNode(NodeTraversal t, Node newNode) {
Expand Down Expand Up @@ -537,7 +572,9 @@ private void visitClassNode(NodeTraversal t, Node classNode) {
if (var != null && (var.isLocal() || var.isExtern())) {
// "require" not needed for these
} else {
usages.put(extendClass.getQualifiedName(), extendClass);
List<String> classNames = getClassNames(extendClass.getQualifiedName());
String outermostClassName = Iterables.getFirst(classNames, extendClass.getQualifiedName());
usages.put(outermostClassName, extendClass);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/DiagnosticGroups.java
View file
Expand Up @@ -478,7 +478,7 @@ public DiagnosticGroup forName(String name) {
DiagnosticGroups.registerGroup("strictMissingRequire",
CheckRequiresForConstructors.MISSING_REQUIRE_WARNING,
CheckRequiresForConstructors.MISSING_REQUIRE_FOR_GOOG_SCOPE,
CheckRequiresForConstructors.MISSING_REQUIRE_CALL_WARNING);
CheckRequiresForConstructors.MISSING_REQUIRE_STRICT_WARNING);

public static final DiagnosticGroup STRICT_REQUIRES =
DiagnosticGroups.registerGroup("legacyGoogScopeRequire",
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/refactoring/ErrorToFixMapper.java
View file
Expand Up @@ -93,7 +93,7 @@ public static SuggestedFix getFixForJsError(JSError error, AbstractCompiler comp
case "JSC_INVALID_SUPER_CALL_WITH_SUGGESTION":
return getFixForInvalidSuper(error, compiler);
case "JSC_MISSING_REQUIRE_WARNING":
case "JSC_MISSING_REQUIRE_CALL_WARNING":
case "JSC_MISSING_REQUIRE_STRICT_WARNING":
return getFixForMissingRequire(error, compiler);
case "JSC_DUPLICATE_REQUIRE":
return getFixForDuplicateRequire(error, compiler);
Expand Down
72 changes: 54 additions & 18 deletions test/com/google/javascript/jscomp/MissingRequireTest.java
View file
Expand Up @@ -17,8 +17,8 @@
package com.google.javascript.jscomp;

import static com.google.common.truth.Truth.assertThat;
import static com.google.javascript.jscomp.CheckRequiresForConstructors.MISSING_REQUIRE_CALL_WARNING;
import static com.google.javascript.jscomp.CheckRequiresForConstructors.MISSING_REQUIRE_FOR_GOOG_SCOPE;
import static com.google.javascript.jscomp.CheckRequiresForConstructors.MISSING_REQUIRE_STRICT_WARNING;
import static com.google.javascript.jscomp.CheckRequiresForConstructors.MISSING_REQUIRE_WARNING;

import com.google.common.collect.ImmutableList;
Expand All @@ -31,6 +31,12 @@
*/

public final class MissingRequireTest extends Es6CompilerTestCase {
private CheckRequiresForConstructors.Mode mode;

public void setUp() {
mode = CheckRequiresForConstructors.Mode.FULL_COMPILE;
}

@Override
protected CompilerOptions getOptions(CompilerOptions options) {
options.setWarningLevel(DiagnosticGroups.STRICT_MISSING_REQUIRE, CheckLevel.WARNING);
Expand All @@ -39,12 +45,11 @@ protected CompilerOptions getOptions(CompilerOptions options) {

@Override
protected CompilerPass getProcessor(Compiler compiler) {
return new CheckRequiresForConstructors(compiler,
CheckRequiresForConstructors.Mode.FULL_COMPILE);
return new CheckRequiresForConstructors(compiler, mode);
}

private void testMissingRequireCall(String js, String warningText) {
test(js, js, null, MISSING_REQUIRE_CALL_WARNING, warningText);
private void testMissingRequireStrict(String js, String warningText) {
test(js, js, null, MISSING_REQUIRE_STRICT_WARNING, warningText);
}

private void testMissingRequire(String js, String warningText) {
Expand Down Expand Up @@ -138,12 +143,16 @@ public void testPassWithInnerClassInExtends() {
}

public void testPassEs6ClassExtends() {
String js =
testSameEs6(
LINE_JOINER.join(
"goog.require('goog.foo.Bar');",
"",
"class SubClass extends goog.foo.Bar.Inner {}");
testSameEs6(js);
"class SubClass extends goog.foo.Bar.Inner {}"));
testSameEs6(
LINE_JOINER.join(
"goog.require('goog.foo.Bar');",
"",
"class SubClass extends goog.foo.Bar {}"));
}

public void testPassPolymer() {
Expand Down Expand Up @@ -180,7 +189,7 @@ public void testPassGoogDefineClass_noRewriting() {
}

public void testWarnGoogModule_noRewriting() {
testMissingRequireCall(
testMissingRequireStrict(
LINE_JOINER.join(
"goog.module('example');",
"",
Expand Down Expand Up @@ -289,7 +298,7 @@ public void testGoogModuleGet() {

public void testDirectCall() {
String js = "foo.bar.baz();";
testMissingRequireCall(js, "missing require: 'foo.bar'");
testMissingRequireStrict(js, "missing require: 'foo.bar'");

List<SourceFile> externs = ImmutableList.of(SourceFile.fromCode("externs",
"var foo;"));
Expand All @@ -301,7 +310,7 @@ public void testDirectCall() {

public void testDotCall() {
String js = "foo.bar.baz.call();";
testMissingRequireCall(js, "missing require: 'foo.bar'");
testMissingRequireStrict(js, "missing require: 'foo.bar'");

List<SourceFile> externs = ImmutableList.of(SourceFile.fromCode("externs",
"var foo;"));
Expand All @@ -313,7 +322,7 @@ public void testDotCall() {

public void testDotApply() {
String js = "foo.bar.baz.call();";
testMissingRequireCall(js, "missing require: 'foo.bar'");
testMissingRequireStrict(js, "missing require: 'foo.bar'");

List<SourceFile> externs = ImmutableList.of(SourceFile.fromCode("externs",
"var foo;"));
Expand Down Expand Up @@ -357,33 +366,33 @@ public void testGoogLocale() {
}

public void testGoogArray() {
testMissingRequireCall(
testMissingRequireStrict(
"goog.array.forEach(arr, fn);",
"missing require: 'goog.array'");
}

public void testGoogDom() {
testMissingRequireCall(
testMissingRequireStrict(
"goog.dom.getElement('x');",
"missing require: 'goog.dom'");
}

public void testLongNameNoClasses() {
testMissingRequireCall(
testMissingRequireStrict(
"example.of.a.long.qualified.name(arr, fn);",
"missing require: 'example.of.a.long.qualified'");
}

// Occasionally people use namespaces that start with a capital letter, so this
// check thinks it's a class name. Predictably, we don't handle this well.
public void testClassNameAtStart() {
testMissingRequireCall(
testMissingRequireStrict(
"Example.of.a.namespace.that.looks.like.a.class.name(arr, fn);",
"missing require: 'Example'");
}

public void testGoogTimerCallOnce() {
testMissingRequireCall(
testMissingRequireStrict(
"goog.Timer.callOnce(goog.nullFunction, 0);",
"missing require: 'goog.Timer'");
}
Expand All @@ -397,7 +406,7 @@ public void testGoogTimer() {
public void testFailEs6ClassExtends() {
setAcceptedLanguage(LanguageMode.ECMASCRIPT_NEXT);
String js = "var goog = {}; class SubClass extends goog.foo.Bar.Inner {}";
String warning = "missing require: 'goog.foo.Bar.Inner'";
String warning = "missing require: 'goog.foo.Bar'";
testMissingRequire(js, warning);
}

Expand Down Expand Up @@ -425,6 +434,33 @@ public void testEs6ClassExtendsSomethingInExternsWithNS() {
test(externs, js, js, null, null, null);
}

public void testFailConstant() {
mode = CheckRequiresForConstructors.Mode.SINGLE_FILE;
testMissingRequireStrict(
"goog.require('example.Class'); alert(example.Constants.FOO);",
"missing require: 'example.Constants'");
testMissingRequireStrict(
"goog.require('example.Class'); alert(example.Outer.Inner.FOO);",
"missing require: 'example.Outer'");
}

public void testPassConstant() {
testSame("goog.require('example.Constants'); alert(example.Constants.FOO);");
testSame("goog.require('example.Outer'); alert(example.Outer.Inner.FOO);");
}

public void testPassLHSFromProvide() {
testSame("goog.provide('example.foo.Outer.Inner'); example.foo.Outer.Inner = {};");
}

public void testPassTypedef() {
testSame("/** @typedef {string|number} */\nexample.TypeDef;");
}

public void testPassConstantFromExterns() {
test("var example;", "alert(example.Constants.FOO);", (String) null, null, null);
}

public void testFailWithNestedNewNodes() {
String js = "goog.require('goog.foo.Bar'); var str = new goog.foo.Bar(new goog.foo.Baz('5'));";
String warning = "missing require: 'goog.foo.Baz'";
Expand Down

0 comments on commit 9620505

Please sign in to comment.