From 96205056e79a366bc4d89e3ae2c94a747bc49066 Mon Sep 17 00:00:00 2001 From: tbreisacher Date: Thu, 8 Dec 2016 22:19:48 -0800 Subject: [PATCH] Warn for missing requires for getprops that are not callees. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=141523131 --- .../jscomp/CheckRequiresForConstructors.java | 91 +++++++++++++------ .../javascript/jscomp/DiagnosticGroups.java | 2 +- .../refactoring/ErrorToFixMapper.java | 2 +- .../javascript/jscomp/MissingRequireTest.java | 72 +++++++++++---- 4 files changed, 120 insertions(+), 47 deletions(-) diff --git a/src/com/google/javascript/jscomp/CheckRequiresForConstructors.java b/src/com/google/javascript/jscomp/CheckRequiresForConstructors.java index 4afd2b315b1..0742658cd24 100644 --- a/src/com/google/javascript/jscomp/CheckRequiresForConstructors.java +++ b/src/com/google/javascript/jscomp/CheckRequiresForConstructors.java @@ -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}''"); @@ -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: @@ -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); } @@ -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) { @@ -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); @@ -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, @@ -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) { @@ -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 classNames = getClassNames(extendClass.getQualifiedName()); + String outermostClassName = Iterables.getFirst(classNames, extendClass.getQualifiedName()); + usages.put(outermostClassName, extendClass); } } } diff --git a/src/com/google/javascript/jscomp/DiagnosticGroups.java b/src/com/google/javascript/jscomp/DiagnosticGroups.java index a5f2b88d7d6..2ae51dc8b94 100644 --- a/src/com/google/javascript/jscomp/DiagnosticGroups.java +++ b/src/com/google/javascript/jscomp/DiagnosticGroups.java @@ -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", diff --git a/src/com/google/javascript/refactoring/ErrorToFixMapper.java b/src/com/google/javascript/refactoring/ErrorToFixMapper.java index b9d84996987..3aba5442e52 100644 --- a/src/com/google/javascript/refactoring/ErrorToFixMapper.java +++ b/src/com/google/javascript/refactoring/ErrorToFixMapper.java @@ -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); diff --git a/test/com/google/javascript/jscomp/MissingRequireTest.java b/test/com/google/javascript/jscomp/MissingRequireTest.java index 18a5ef38b7f..c74edb8667a 100644 --- a/test/com/google/javascript/jscomp/MissingRequireTest.java +++ b/test/com/google/javascript/jscomp/MissingRequireTest.java @@ -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; @@ -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); @@ -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) { @@ -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() { @@ -180,7 +189,7 @@ public void testPassGoogDefineClass_noRewriting() { } public void testWarnGoogModule_noRewriting() { - testMissingRequireCall( + testMissingRequireStrict( LINE_JOINER.join( "goog.module('example');", "", @@ -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 externs = ImmutableList.of(SourceFile.fromCode("externs", "var foo;")); @@ -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 externs = ImmutableList.of(SourceFile.fromCode("externs", "var foo;")); @@ -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 externs = ImmutableList.of(SourceFile.fromCode("externs", "var foo;")); @@ -357,19 +366,19 @@ 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'"); } @@ -377,13 +386,13 @@ public void testLongNameNoClasses() { // 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'"); } @@ -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); } @@ -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'";