From 5df0fa2dad776bc4c574cfcb9d8b9f557fdd86fc Mon Sep 17 00:00:00 2001 From: tjgq Date: Fri, 7 Sep 2018 19:44:13 -0700 Subject: [PATCH] Automated g4 rollback of changelist 212058481. *** Reason for rollback *** The new linter will incorrectly warn on an unqualified template parameter, e.g. /** @constructor @template T */ function Foo() {} /** @param {T} x */ Foo.prototype.bar = function(x) {}; will warn that the T in @param needs a ! or ?. Rolling back to prevent a bad linter release. *** Original change description *** Extend the CheckRedundantNullabilityModifier linter pass to also check for missing nullability modifiers according to the style guide. *** ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=212075073 --- .../javascript/jscomp/DefaultPassConfig.java | 4 +- .../javascript/jscomp/DiagnosticGroups.java | 18 +- .../javascript/jscomp/LintPassConfig.java | 4 +- .../lint/CheckNullabilityModifiers.java | 164 -------------- .../CheckRedundantNullabilityModifier.java | 118 ++++++++++ .../lint/CheckNullabilityModifiersTest.java | 207 ------------------ ...CheckRedundantNullabilityModifierTest.java | 148 +++++++++++++ .../refactoring/ErrorToFixMapperTest.java | 6 +- .../navigational_xss_sinks_test_in.js | 2 +- .../navigational_xss_sinks_test_out.js | 2 +- .../testdata/set_location_href_test_in.js | 4 +- .../testdata/set_location_href_test_out.js | 4 +- .../testdata/set_window_location_test_in.js | 2 +- .../testdata/set_window_location_test_out.js | 2 +- 14 files changed, 290 insertions(+), 395 deletions(-) delete mode 100644 src/com/google/javascript/jscomp/lint/CheckNullabilityModifiers.java create mode 100644 src/com/google/javascript/jscomp/lint/CheckRedundantNullabilityModifier.java delete mode 100644 test/com/google/javascript/jscomp/lint/CheckNullabilityModifiersTest.java create mode 100644 test/com/google/javascript/jscomp/lint/CheckRedundantNullabilityModifierTest.java diff --git a/src/com/google/javascript/jscomp/DefaultPassConfig.java b/src/com/google/javascript/jscomp/DefaultPassConfig.java index 336de8a8aaa..cae6270e27d 100644 --- a/src/com/google/javascript/jscomp/DefaultPassConfig.java +++ b/src/com/google/javascript/jscomp/DefaultPassConfig.java @@ -51,10 +51,10 @@ import com.google.javascript.jscomp.lint.CheckJSDocStyle; import com.google.javascript.jscomp.lint.CheckMissingSemicolon; import com.google.javascript.jscomp.lint.CheckNoMutatedEs6Exports; -import com.google.javascript.jscomp.lint.CheckNullabilityModifiers; import com.google.javascript.jscomp.lint.CheckNullableReturn; import com.google.javascript.jscomp.lint.CheckPrimitiveAsObject; import com.google.javascript.jscomp.lint.CheckPrototypeProperties; +import com.google.javascript.jscomp.lint.CheckRedundantNullabilityModifier; import com.google.javascript.jscomp.lint.CheckRequiresAndProvidesSorted; import com.google.javascript.jscomp.lint.CheckUnusedLabels; import com.google.javascript.jscomp.lint.CheckUselessBlocks; @@ -1971,9 +1971,9 @@ protected HotSwapCompilerPass create(AbstractCompiler compiler) { .add(new CheckInterfaces(compiler)) .add(new CheckJSDocStyle(compiler)) .add(new CheckMissingSemicolon(compiler)) - .add(new CheckNullabilityModifiers(compiler)) .add(new CheckPrimitiveAsObject(compiler)) .add(new CheckPrototypeProperties(compiler)) + .add(new CheckRedundantNullabilityModifier(compiler)) .add(new CheckUnusedLabels(compiler)) .add(new CheckUselessBlocks(compiler)); return combineChecks(compiler, callbacks.build()); diff --git a/src/com/google/javascript/jscomp/DiagnosticGroups.java b/src/com/google/javascript/jscomp/DiagnosticGroups.java index f092d8f313d..21a97e2ba33 100644 --- a/src/com/google/javascript/jscomp/DiagnosticGroups.java +++ b/src/com/google/javascript/jscomp/DiagnosticGroups.java @@ -31,10 +31,10 @@ import com.google.javascript.jscomp.lint.CheckJSDocStyle; import com.google.javascript.jscomp.lint.CheckMissingSemicolon; import com.google.javascript.jscomp.lint.CheckNoMutatedEs6Exports; -import com.google.javascript.jscomp.lint.CheckNullabilityModifiers; import com.google.javascript.jscomp.lint.CheckNullableReturn; import com.google.javascript.jscomp.lint.CheckPrimitiveAsObject; import com.google.javascript.jscomp.lint.CheckPrototypeProperties; +import com.google.javascript.jscomp.lint.CheckRedundantNullabilityModifier; import com.google.javascript.jscomp.lint.CheckRequiresAndProvidesSorted; import com.google.javascript.jscomp.lint.CheckUnusedLabels; import com.google.javascript.jscomp.lint.CheckUselessBlocks; @@ -580,11 +580,10 @@ public DiagnosticGroup forName(String name) { CheckInterfaces.INTERFACE_FUNCTION_NOT_EMPTY, CheckInterfaces.INTERFACE_SHOULD_NOT_TAKE_ARGS, CheckMissingSemicolon.MISSING_SEMICOLON, - CheckNullabilityModifiers.MISSING_NULLABILITY_MODIFIER_JSDOC, - CheckNullabilityModifiers.REDUNDANT_NULLABILITY_MODIFIER_JSDOC, CheckPrimitiveAsObject.NEW_PRIMITIVE_OBJECT, CheckPrimitiveAsObject.PRIMITIVE_OBJECT_DECLARATION, CheckPrototypeProperties.ILLEGAL_PROTOTYPE_MEMBER, + CheckRedundantNullabilityModifier.REDUNDANT_NULLABILITY_MODIFIER_JSDOC, CheckRequiresAndProvidesSorted.DUPLICATE_REQUIRE, CheckRequiresAndProvidesSorted.REQUIRES_NOT_SORTED, CheckRequiresAndProvidesSorted.PROVIDES_NOT_SORTED, @@ -660,8 +659,7 @@ public DiagnosticGroup forName(String name) { // the file level is by using a whitelist file. @GwtIncompatible("Conformance") public static final DiagnosticGroup CONFORMANCE_VIOLATIONS = - DiagnosticGroups.registerGroup( - "conformanceViolations", + DiagnosticGroups.registerGroup("conformanceViolations", CheckConformance.CONFORMANCE_VIOLATION, CheckConformance.CONFORMANCE_POSSIBLE_VIOLATION); @@ -672,14 +670,16 @@ public DiagnosticGroup forName(String name) { public static final DiagnosticGroup MISSING_POLYFILL = DiagnosticGroups.registerGroup( - "missingPolyfill", RewritePolyfills.INSUFFICIENT_OUTPUT_VERSION_ERROR); + "missingPolyfill", + RewritePolyfills.INSUFFICIENT_OUTPUT_VERSION_ERROR); // For internal use only, so there are no constants for these groups. static { - DiagnosticGroups.registerGroup( - "invalidProvide", ProcessClosurePrimitives.INVALID_PROVIDE_ERROR); + DiagnosticGroups.registerGroup("invalidProvide", + ProcessClosurePrimitives.INVALID_PROVIDE_ERROR); - DiagnosticGroups.registerGroup("es6Typed", RhinoErrorReporter.MISPLACED_TYPE_SYNTAX); + DiagnosticGroups.registerGroup("es6Typed", + RhinoErrorReporter.MISPLACED_TYPE_SYNTAX); DiagnosticGroups.registerDeprecatedGroup("duplicateZipContents"); } diff --git a/src/com/google/javascript/jscomp/LintPassConfig.java b/src/com/google/javascript/jscomp/LintPassConfig.java index 1829a919f66..92d1389af1b 100644 --- a/src/com/google/javascript/jscomp/LintPassConfig.java +++ b/src/com/google/javascript/jscomp/LintPassConfig.java @@ -22,9 +22,9 @@ import com.google.javascript.jscomp.lint.CheckInterfaces; import com.google.javascript.jscomp.lint.CheckJSDocStyle; import com.google.javascript.jscomp.lint.CheckMissingSemicolon; -import com.google.javascript.jscomp.lint.CheckNullabilityModifiers; import com.google.javascript.jscomp.lint.CheckPrimitiveAsObject; import com.google.javascript.jscomp.lint.CheckPrototypeProperties; +import com.google.javascript.jscomp.lint.CheckRedundantNullabilityModifier; import com.google.javascript.jscomp.lint.CheckRequiresAndProvidesSorted; import com.google.javascript.jscomp.lint.CheckUnusedLabels; import com.google.javascript.jscomp.lint.CheckUselessBlocks; @@ -73,7 +73,7 @@ protected CompilerPass create(AbstractCompiler compiler) { new CheckSuper(compiler), new CheckPrimitiveAsObject(compiler), new ClosureCheckModule(compiler), - new CheckNullabilityModifiers(compiler), + new CheckRedundantNullabilityModifier(compiler), new CheckRequiresAndProvidesSorted(compiler), new CheckSideEffects( compiler, /* report */ true, /* protectSideEffectFreeCode */ false), diff --git a/src/com/google/javascript/jscomp/lint/CheckNullabilityModifiers.java b/src/com/google/javascript/jscomp/lint/CheckNullabilityModifiers.java deleted file mode 100644 index ca35653b64a..00000000000 --- a/src/com/google/javascript/jscomp/lint/CheckNullabilityModifiers.java +++ /dev/null @@ -1,164 +0,0 @@ -/* - * Copyright 2018 The Closure Compiler Authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package com.google.javascript.jscomp.lint; - -import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableSet; -import com.google.javascript.jscomp.AbstractCompiler; -import com.google.javascript.jscomp.CompilerPass; -import com.google.javascript.jscomp.DiagnosticType; -import com.google.javascript.jscomp.NodeTraversal; -import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; -import com.google.javascript.jscomp.NodeUtil; -import com.google.javascript.rhino.JSDocInfo; -import com.google.javascript.rhino.JSTypeExpression; -import com.google.javascript.rhino.Node; -import com.google.javascript.rhino.Token; - -/** - * Checks for missing or redundant nullability modifiers. - * - *

Primitive and literal types should not be preceded by a `!` modifier. Reference types must be - * preceded by a `?` or `!` modifier. - */ -public class CheckNullabilityModifiers extends AbstractPostOrderCallback implements CompilerPass { - - /** The diagnostic for a missing nullability modifier. */ - public static final DiagnosticType MISSING_NULLABILITY_MODIFIER_JSDOC = - DiagnosticType.disabled( - "JSC_MISSING_NULLABILITY_MODIFIER_JSDOC", - "{0} is a reference type with no nullability modifier, " - + "which is disallowed by the style guide.\n" - + "Please add a '!' to make it explicitly non-nullable, " - + "or a '?' to make it explicitly nullable."); - - /** The diagnostic for a redundant nullability modifier. */ - public static final DiagnosticType REDUNDANT_NULLABILITY_MODIFIER_JSDOC = - DiagnosticType.disabled( - "JSC_REDUNDANT_NULLABILITY_MODIFIER_JSDOC", - "{0} is a non-reference type which is already non-nullable.\n" - + "Please remove the redundant '!', which is disallowed by the style guide."); - - private static final ImmutableSet PRIMITIVE_TYPE_NAMES = - ImmutableSet.of("boolean", "number", "string", "symbol", "undefined", "null"); - - private final AbstractCompiler compiler; - - public CheckNullabilityModifiers(AbstractCompiler compiler) { - this.compiler = compiler; - } - - @Override - public void process(Node externs, Node root) { - NodeTraversal.traverseRoots(compiler, this, externs, root); - } - - @Override - public void visit(final NodeTraversal t, final Node n, final Node p) { - final JSDocInfo info = n.getJSDocInfo(); - if (info == null) { - return; - } - if (info.hasType()) { - checkTypeExpression(t, info.getType(), false); - } - for (String param : info.getParameterNames()) { - if (info.hasParameterType(param)) { - checkTypeExpression(t, info.getParameterType(param), false); - } - } - if (info.hasReturnType()) { - checkTypeExpression(t, info.getReturnType(), false); - } - if (info.hasEnumParameterType()) { - // JSDocInfoParser wraps the @enum type in an artificial '!' if it's not a primitive type. - // Thus a missing modifier warning can never trigger, but a redundant modifier warning can. - checkTypeExpression(t, info.getEnumParameterType(), false); - } - if (info.hasTypedefType()) { - checkTypeExpression(t, info.getTypedefType(), false); - } - if (info.hasThisType()) { - // JSDocInfoParser wraps the @this type in an artificial '!'. - // Thus a missing modifier warning can never trigger, but a top-level '!' should be ignored if - // it precedes a primitive or literal type; otherwise it will trigger a spurious redundant - // modifier warning. - checkTypeExpression(t, info.getThisType(), true); - } - for (JSTypeExpression expr : info.getThrownTypes()) { - checkTypeExpression(t, expr, false); - } - // JSDocInfoParser enforces the @extends and @implements types to be unqualified in the source - // code, so we don't need to check them. - } - - private static void checkTypeExpression( - final NodeTraversal t, JSTypeExpression expr, boolean hasArtificialTopLevelBang) { - final Node root = expr.getRoot(); - NodeUtil.visitPreOrder( - root, - node -> { - Node parent = node.getParent(); - - boolean isPrimitiveOrLiteral = - isPrimitiveType(node) || isFunctionLiteral(node) || isRecordLiteral(node); - boolean isReference = isReferenceType(node); - - // Whether the node is preceded by a '!' or '?'. - boolean hasBang = parent != null && parent.getToken() == Token.BANG; - boolean hasQmark = parent != null && parent.getToken() == Token.QMARK; - - // Whether the node is preceded by a '!' that wasn't artificially added. - boolean hasNonArtificialBang = hasBang && !(hasArtificialTopLevelBang && parent == root); - - // Whether the node is the type in function(new:T) or function(this:T). - boolean isNewOrThis = parent != null && (parent.isNew() || parent.isThis()); - - if (isReference && !hasBang && !hasQmark && !isNewOrThis) { - t.report(node, MISSING_NULLABILITY_MODIFIER_JSDOC, getReportedTypeName(node)); - } else if (isPrimitiveOrLiteral && hasNonArtificialBang) { - t.report(parent, REDUNDANT_NULLABILITY_MODIFIER_JSDOC, getReportedTypeName(node)); - } - }); - } - - private static boolean isPrimitiveType(Node node) { - return node.isString() && PRIMITIVE_TYPE_NAMES.contains(node.getString()); - } - - private static boolean isReferenceType(Node node) { - return node.isString() && !PRIMITIVE_TYPE_NAMES.contains(node.getString()); - } - - private static boolean isFunctionLiteral(Node node) { - return node.isFunction(); - } - - private static boolean isRecordLiteral(Node node) { - return node.getToken() == Token.LC; - } - - private static String getReportedTypeName(Node node) { - if (isFunctionLiteral(node)) { - return "Function"; - } - if (isRecordLiteral(node)) { - return "Record literal"; - } - Preconditions.checkState(isPrimitiveType(node) || isReferenceType(node)); - return node.getString(); - } -} diff --git a/src/com/google/javascript/jscomp/lint/CheckRedundantNullabilityModifier.java b/src/com/google/javascript/jscomp/lint/CheckRedundantNullabilityModifier.java new file mode 100644 index 00000000000..0f2b77d1942 --- /dev/null +++ b/src/com/google/javascript/jscomp/lint/CheckRedundantNullabilityModifier.java @@ -0,0 +1,118 @@ +/* + * Copyright 2018 The Closure Compiler Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.google.javascript.jscomp.lint; + +import com.google.common.collect.ImmutableSet; +import com.google.javascript.jscomp.AbstractCompiler; +import com.google.javascript.jscomp.CompilerPass; +import com.google.javascript.jscomp.DiagnosticType; +import com.google.javascript.jscomp.NodeTraversal; +import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; +import com.google.javascript.jscomp.NodeUtil; +import com.google.javascript.rhino.JSDocInfo; +import com.google.javascript.rhino.JSTypeExpression; +import com.google.javascript.rhino.Node; +import com.google.javascript.rhino.Token; + +/** + * Checks for redundant nullability modifiers. + * + *

A nullability modifier '!' is redundant if it precedes a value type or a record literal. + */ +public class CheckRedundantNullabilityModifier extends AbstractPostOrderCallback + implements CompilerPass { + + /** The diagnostic for a redundant nullability modifier. */ + public static final DiagnosticType REDUNDANT_NULLABILITY_MODIFIER_JSDOC = + DiagnosticType.warning( + "JSC_REDUNDANT_NULLABILITY_MODIFIER_JSDOC", + "Type is already not null. Please remove the redundant '!'."); + + private static final ImmutableSet PRIMITIVE_TYPE_NAMES = + ImmutableSet.of("boolean", "number", "string", "symbol", "undefined"); + + private final AbstractCompiler compiler; + + public CheckRedundantNullabilityModifier(AbstractCompiler compiler) { + this.compiler = compiler; + } + + @Override + public void process(Node externs, Node root) { + NodeTraversal.traverseRoots(compiler, this, externs, root); + } + + @Override + public void visit(final NodeTraversal t, final Node n, final Node p) { + final JSDocInfo info = n.getJSDocInfo(); + if (info == null) { + return; + } + if (info.hasType()) { + checkTypeExpression(t, info.getType(), false); + } + for (String param : info.getParameterNames()) { + if (info.hasParameterType(param)) { + checkTypeExpression(t, info.getParameterType(param), false); + } + } + if (info.hasReturnType()) { + checkTypeExpression(t, info.getReturnType(), false); + } + if (info.hasEnumParameterType()) { + checkTypeExpression(t, info.getEnumParameterType(), false); + } + if (info.hasTypedefType()) { + checkTypeExpression(t, info.getTypedefType(), false); + } + // JSDocInfoParser automatically prepends a '!' to the type associated with a @this annotation. + // It should be ignored since it doesn't actually exist in the source file. + if (info.hasThisType()) { + checkTypeExpression(t, info.getThisType(), true); + } + // We don't need to check @extends and @implements annotations since they never refer to a + // primitive type. + // TODO(tjgq): Should we check @throws annotations? The Google style guide forbids throwing a + // primitive type, so the warning would be somewhat misguided. + } + + private static void checkTypeExpression( + final NodeTraversal t, JSTypeExpression expr, boolean isTopLevelBangAllowed) { + NodeUtil.visitPreOrder( + expr.getRoot(), + node -> { + Node parent = node.getParent(); + Node grandparent = parent != null ? parent.getParent() : null; + + boolean hasModifier = parent != null && parent.getToken() == Token.BANG; + boolean isModifierRedundant = + isPrimitiveType(node) || node.isFunction() || isRecordLiteral(node); + boolean isRedundantModifierAllowed = isTopLevelBangAllowed && grandparent == null; + + if (hasModifier && isModifierRedundant && !isRedundantModifierAllowed) { + t.report(parent, REDUNDANT_NULLABILITY_MODIFIER_JSDOC); + } + }); + } + + private static boolean isPrimitiveType(Node node) { + return node.isString() && PRIMITIVE_TYPE_NAMES.contains(node.getString()); + } + + private static boolean isRecordLiteral(Node node) { + return node.getToken() == Token.LC; + } +} diff --git a/test/com/google/javascript/jscomp/lint/CheckNullabilityModifiersTest.java b/test/com/google/javascript/jscomp/lint/CheckNullabilityModifiersTest.java deleted file mode 100644 index b397b3876eb..00000000000 --- a/test/com/google/javascript/jscomp/lint/CheckNullabilityModifiersTest.java +++ /dev/null @@ -1,207 +0,0 @@ -/* - * Copyright 2018 The Closure Compiler Authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package com.google.javascript.jscomp.lint; - -import com.google.javascript.jscomp.CheckLevel; -import com.google.javascript.jscomp.Compiler; -import com.google.javascript.jscomp.CompilerOptions; -import com.google.javascript.jscomp.CompilerPass; -import com.google.javascript.jscomp.CompilerTestCase; -import com.google.javascript.jscomp.DiagnosticGroups; - -/** Unit tests for {@link CheckNullabilityModifiers}. */ -public final class CheckNullabilityModifiersTest extends CompilerTestCase { - - @Override - protected CompilerPass getProcessor(Compiler compiler) { - return new CheckNullabilityModifiers(compiler); - } - - @Override - protected CompilerOptions getOptions(CompilerOptions options) { - super.getOptions(options); - options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, CheckLevel.WARNING); - return options; - } - - public void testPrimitiveType() { - checkRedundantWarning("/** @type {!boolean} */ var x;"); - checkRedundantWarning("/** @type {!number} */ var x;"); - checkRedundantWarning("/** @type {!string} */ var x;"); - checkRedundantWarning("/** @type {!symbol} */ var x;"); - checkRedundantWarning("/** @type {!undefined} */ var x;"); - - checkNoWarning("/** @type {?boolean} */ var x;"); - checkNoWarning("/** @type {boolean} */ var x;"); - } - - public void testReferenceType() { - checkMissingWarning("/** @type {Object} */ var x;"); - checkMissingWarning("/** @type {Function} */ var x;"); - checkMissingWarning("/** @type {Symbol} */ var x;"); - - checkNoWarning("/** @type {?Object} */ var x;"); - checkNoWarning("/** @type {!Object} */ var x;"); - } - - public void testRecordType() { - checkRedundantWarning("/** @type {!{foo: string}} */ var x;"); - checkRedundantWarning("/** @type {{foo: !string}} */ var x;"); - - checkMissingWarning("/** @type {{foo: Object}} */ var x;"); - - checkNoWarning("/** @type {?{foo: string}} */ var x;"); - checkNoWarning("/** @type {{foo: string}} */ var x;"); - checkNoWarning("/** @type {{foo: ?string}} */ var x;"); - checkNoWarning("/** @type {{foo: !Object}} */ var x;"); - checkNoWarning("/** @type {{foo: ?Object}} */ var x;"); - } - - public void testFunctionType() { - checkRedundantWarning("/** @type {!function()} */ function f(){}"); - checkRedundantWarning("/** @type {function(!string)} */ function f(x){}"); - checkRedundantWarning("/** @type {function(): !string} */ function f(x){}"); - - checkMissingWarning("/** @type {function(Object)} */ function f(x){}"); - checkMissingWarning("/** @type {function(): Object} */ function f(x){}"); - - checkNoWarning("/** @type {function()} */ function f(){}"); - checkNoWarning("/** @type {function(?string): ?string} */ function f(x){}"); - checkNoWarning("/** @type {function(string): string} */ function f(x){}"); - checkNoWarning("/** @type {function(!Object): ?Object} */ function f(x){}"); - checkNoWarning("/** @type {function(new:Object)} */ function f(){}"); - checkNoWarning("/** @type {function(this:Object)} */ function f(){}"); - } - - public void testUnionType() { - checkRedundantWarning("/** @type {!Object|!string} */ var x;"); - - checkMissingWarning("/** @type {Object|string} */ var x;"); - - checkNoWarning("/** @type {?Object|string} */ var x;"); - checkNoWarning("/** @type {!Object|string} */ var x;"); - } - - public void testEnumType() { - checkRedundantWarning("/** @enum {!boolean} */ var x;"); - checkRedundantWarning("/** @enum {!number} */ var x;"); - checkRedundantWarning("/** @enum {!string} */ var x;"); - checkRedundantWarning("/** @enum {!symbol} */ var x;"); - - checkNoWarning("/** @enum {?string} */ var x;"); - checkNoWarning("/** @enum {string} */ var x;"); - checkNoWarning("/** @enum {Object} */ var x;"); - checkNoWarning("/** @enum {?Object} */ var x;"); - checkNoWarning("/** @enum {!Object} */ var x;"); - } - - public void testTemplateType() { - checkRedundantWarning("/** @type {!Array} */ var x;"); - - checkMissingWarning("/** @type {Array} */ var x;"); - checkMissingWarning("/** @type {!Array} */ var x;"); - - checkNoWarning("/** @type {!Array} */ var x;"); - checkNoWarning("/** @type {!Array} */ var x;"); - checkNoWarning("/** @type {!Array} */ var x;"); - checkNoWarning("/** @type {!Array} */ var x;"); - } - - public void testParamType() { - checkRedundantWarning("/** @param {!string} x */ function f(x){}"); - - checkMissingWarning("/** @param {Object} x */ function f(x){}"); - - checkNoWarning("/** @param {?string} x */ function f(x){}"); - checkNoWarning("/** @param {string} x */ function f(x){}"); - checkNoWarning("/** @param {?Object} x */ function f(x){}"); - checkNoWarning("/** @param {!Object} x */ function f(x){}"); - } - - public void testParamMissingType() { - checkNoWarning("/** @param x */ function f(x){}"); - } - - public void testReturnType() { - checkRedundantWarning("/** @return {!string} */ function f(){}"); - - checkMissingWarning("/** @return {Object} */ function f(){}"); - - checkNoWarning("/** @return {?string} */ function f(){}"); - checkNoWarning("/** @return {string} */ function f(){}"); - checkNoWarning("/** @return {?Object} */ function f(){}"); - checkNoWarning("/** @return {!Object} */ function f(){}"); - } - - public void testTypedefType() { - checkRedundantWarning("/** @typedef {!string} */ var x;"); - - checkMissingWarning("/** @typedef {Object} */ var x;"); - - checkNoWarning("/** @typedef {?string} */ var x;"); - checkNoWarning("/** @typedef {string} */ var x;"); - checkNoWarning("/** @typedef {?Object} */ var x;"); - checkNoWarning("/** @typedef {!Object} */ var x;"); - } - - public void testThisType() { - checkRedundantWarning("/** @this {!string} */ function f(){}"); - - checkNoWarning("/** @this {?string} */ function f(){}"); - checkNoWarning("/** @this {string} */ function f(){}"); - checkNoWarning("/** @this {Object} */ function f(){}"); - checkNoWarning("/** @this {?Object} */ function f(){}"); - checkNoWarning("/** @this {!Object} */ function f(){}"); - } - - public void testBaseType() { - checkNoWarning("/** @extends {Object} */ function f(){}"); - checkNoWarning("/** @implements {Object} */ function f(){}"); - } - - public void testThrowsType() { - // TODO(tjgq): The style guide forbids throwing anything other than Error subclasses, so an - // @throws should never contain a primitive type. Should we suppress the warning in this case? - checkRedundantWarning("/** @throws {!string} */ function f(){}"); - - checkMissingWarning("/** @throws {Object} */ function f(){}"); - - checkNoWarning("/** @throws {string} */ function f(){}"); - checkNoWarning("/** @throws {?string} */ function f(){}"); - checkNoWarning("/** @throws {?Object} */ function f(){}"); - checkNoWarning("/** @throws {!Object} */ function f(){}"); - } - - public void testEndPosition() { - checkRedundantWarning("/** @type {string!} */ var x;"); - - checkNoWarning("/** @type {string?} */ var x;"); - checkNoWarning("/** @type {Object!} */ var x;"); - checkNoWarning("/** @type {Object?} */ var x;"); - } - - private void checkNoWarning(String js) { - testSame(js); - } - - private void checkMissingWarning(String js) { - testWarning(js, CheckNullabilityModifiers.MISSING_NULLABILITY_MODIFIER_JSDOC); - } - - private void checkRedundantWarning(String js) { - testWarning(js, CheckNullabilityModifiers.REDUNDANT_NULLABILITY_MODIFIER_JSDOC); - } -} diff --git a/test/com/google/javascript/jscomp/lint/CheckRedundantNullabilityModifierTest.java b/test/com/google/javascript/jscomp/lint/CheckRedundantNullabilityModifierTest.java new file mode 100644 index 00000000000..9c4812f985a --- /dev/null +++ b/test/com/google/javascript/jscomp/lint/CheckRedundantNullabilityModifierTest.java @@ -0,0 +1,148 @@ +/* + * Copyright 2018 The Closure Compiler Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.google.javascript.jscomp.lint; + +import com.google.javascript.jscomp.CheckLevel; +import com.google.javascript.jscomp.Compiler; +import com.google.javascript.jscomp.CompilerOptions; +import com.google.javascript.jscomp.CompilerPass; +import com.google.javascript.jscomp.CompilerTestCase; +import com.google.javascript.jscomp.DiagnosticGroups; + +/** Unit tests for {@link CheckRedundantNullabilityModifier}. */ +public final class CheckRedundantNullabilityModifierTest extends CompilerTestCase { + @Override + protected CompilerOptions getOptions(CompilerOptions options) { + options.setWarningLevel(DiagnosticGroups.ANALYZER_CHECKS, CheckLevel.WARNING); + return options; + } + + @Override + protected CompilerPass getProcessor(Compiler compiler) { + return new CheckRedundantNullabilityModifier(compiler); + } + + public void testPrimitiveType() { + checkWarning("/** @type {!boolean} */ var x;"); + checkWarning("/** @type {!number} */ var x;"); + checkWarning("/** @type {!string} */ var x;"); + checkWarning("/** @type {!symbol} */ var x;"); + checkWarning("/** @type {!undefined} */ var x;"); + checkWarning("/** @type {!function()} */ function f(){}"); + + checkNoWarning("/** @type {?boolean} */ var x;"); + checkNoWarning("/** @type {boolean} */ var x;"); + } + + public void testReferenceType() { + checkNoWarning("/** @type {!Object} */ var x;"); + checkNoWarning("/** @type {!Function} */ var x;"); + checkNoWarning("/** @type {!Symbol} */ var x;"); + } + + public void testRecordLiteral() { + checkWarning("/** @type {!{foo: string, bar: number}} */ var x;"); + + checkNoWarning("/** @type {?{foo: string, bar: number}} */ var x;"); + checkNoWarning("/** @type {{foo: string, bar: number}} */ var x;"); + } + + public void testFunctionArgs() { + checkWarning( + "/** @type {function(!string)} */ function f(x){}"); + checkWarning( + "/** @type {function(): !string} */ function f(x){ return ''; }"); + + checkNoWarning("/** @type {function(?string): ?string} */ function f(x){ return ''; }"); + checkNoWarning("/** @type {function(string): string} */ function f(x){ return ''; }"); + } + + public void testUnionType() { + checkWarning("/** @type {Object|!string} */ var x;"); + + checkNoWarning("/** @type {Object|string} */ var x;"); + } + + public void testEnumType() { + checkWarning("/** @enum {!string} */ var o = {foo: 'foo'};"); + checkWarning("/** @enum {!number} */ var o = {foo: 42};"); + checkWarning("/** @enum {!boolean} */ var o = {foo: true};"); + checkWarning("/** @enum {!symbol} */ var o = {foo: Symbol('foo')};"); + + checkNoWarning("/** @enum {string} */ var o = {foo: 'foo'};"); + checkNoWarning("/** @enum {number} */ var o = {foo: 42};"); + checkNoWarning("/** @enum {boolean} */ var o = {foo: true};"); + checkNoWarning("/** @enum {symbol} */ var o = {foo: Symbol('foo')};"); + checkNoWarning("/** @enum {!Object} */ var o = {foo: {}};"); + } + + public void testTemplateType() { + checkWarning("/** @type {Array} */ var x;"); + + checkNoWarning("/** @type {Array} */ var x;"); + checkNoWarning("/** @type {Array} */ var x;"); + } + + public void testParamType() { + checkWarning("/** @param {!string} x */ function f(x){}"); + + checkNoWarning("/** @param {string} x */ function f(x){}"); + } + + public void testParamMissingType() { + checkNoWarning("/** @param x */ function f(x){}"); + } + + public void testReturnType() { + checkWarning("/** @return {!string} */ function f(){ return ''; }"); + + checkNoWarning("/** @return {string} */ function f(){ return ''; }"); + } + + public void testTypedefType() { + checkWarning("/** @typedef {!string} */ var x;"); + checkWarning("/** @typedef {Array} */ var x;"); + + checkNoWarning("/** @typedef {string} */ var x;"); + checkNoWarning("/** @typedef {Array} */ var x;"); + } + + public void testThisType() { + checkWarning("/** @this {!string} */ function f(){}"); + checkWarning("/** @this {!{foo: string}} */ function f(){}"); + + checkNoWarning("/** @this {string} */ function f(){}"); + checkNoWarning("/** @this {{foo: string}} */ function f(){}"); + } + + public void testEndPosition() { + checkWarning("/** @type {boolean!} */ var x;"); + } + + private void checkNoWarning(String js) { + testSame(js); + } + + private void checkWarning(String js) { + testWarning(js, CheckRedundantNullabilityModifier.REDUNDANT_NULLABILITY_MODIFIER_JSDOC); + } + + @Override + protected void setUp() throws Exception { + super.setUp(); + enableTypeCheck(); + } +} diff --git a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java index fb54d75a32e..6c31fe5b296 100644 --- a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java +++ b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java @@ -100,7 +100,7 @@ public void testImplicitNullability1() { options); assertThat(compiler.getErrors()).isEmpty(); JSError[] warnings = compiler.getWarnings(); - assertThat(warnings).hasLength(2); + assertThat(warnings).hasLength(1); JSError warning = warnings[0]; List fixes = ErrorToFixMapper.getFixesForJsError(warning, compiler); assertThat(fixes).hasSize(2); @@ -125,7 +125,7 @@ public void testImplicitNullability2() { options); assertThat(compiler.getErrors()).isEmpty(); JSError[] warnings = compiler.getWarnings(); - assertThat(warnings).hasLength(2); + assertThat(warnings).hasLength(1); JSError warning = warnings[0]; List fixes = ErrorToFixMapper.getFixesForJsError(warning, compiler); assertThat(fixes).hasSize(2); @@ -155,7 +155,7 @@ public void testImplicitNullability3() { options); assertThat(compiler.getErrors()).isEmpty(); JSError[] warnings = compiler.getWarnings(); - assertThat(warnings).hasLength(2); + assertThat(warnings).hasLength(1); JSError warning = warnings[0]; List fixes = ErrorToFixMapper.getFixesForJsError(warning, compiler); assertThat(fixes).hasSize(2); diff --git a/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_in.js b/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_in.js index 1a70e96a541..e6a9add173b 100644 --- a/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_in.js +++ b/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_in.js @@ -142,7 +142,7 @@ refactoring_testcase.unknown_location = function(target, val) { }; /** - * @param {!Object} target The target. + * @param {Object} target The target. * @param {string} val The value. */ refactoring_testcase.object_location = function(target, val) { diff --git a/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_out.js b/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_out.js index b2e0b7931b5..98ffc1d5e99 100644 --- a/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_out.js +++ b/test/com/google/javascript/refactoring/examples/testdata/navigational_xss_sinks_test_out.js @@ -144,7 +144,7 @@ refactoring_testcase.unknown_location = function(target, val) { }; /** - * @param {!Object} target The target. + * @param {Object} target The target. * @param {string} val The value. */ refactoring_testcase.object_location = function(target, val) { diff --git a/test/com/google/javascript/refactoring/examples/testdata/set_location_href_test_in.js b/test/com/google/javascript/refactoring/examples/testdata/set_location_href_test_in.js index b82ff8bcb46..61915ad408b 100644 --- a/test/com/google/javascript/refactoring/examples/testdata/set_location_href_test_in.js +++ b/test/com/google/javascript/refactoring/examples/testdata/set_location_href_test_in.js @@ -37,7 +37,7 @@ refactoring_testcase.location_href_string_literal = function(target) { }; /** - * @param {!Location|!Element} target The target. + * @param {Location|Element} target The target. * @param {string} val The value. */ refactoring_testcase.union_type_href = function(target, val) { @@ -46,7 +46,7 @@ refactoring_testcase.union_type_href = function(target, val) { }; /** - * @param {!Window|!Element} target The target. + * @param {Window|Element} target The target. * @param {string} val The value. */ refactoring_testcase.union_type_location = function(target, val) { diff --git a/test/com/google/javascript/refactoring/examples/testdata/set_location_href_test_out.js b/test/com/google/javascript/refactoring/examples/testdata/set_location_href_test_out.js index fd035698849..621a78c8e89 100644 --- a/test/com/google/javascript/refactoring/examples/testdata/set_location_href_test_out.js +++ b/test/com/google/javascript/refactoring/examples/testdata/set_location_href_test_out.js @@ -38,7 +38,7 @@ refactoring_testcase.location_href_string_literal = function(target) { }; /** - * @param {!Location|!Element} target The target. + * @param {Location|Element} target The target. * @param {string} val The value. */ refactoring_testcase.union_type_href = function(target, val) { @@ -47,7 +47,7 @@ refactoring_testcase.union_type_href = function(target, val) { }; /** - * @param {!Window|!Element} target The target. + * @param {Window|Element} target The target. * @param {string} val The value. */ refactoring_testcase.union_type_location = function(target, val) { diff --git a/test/com/google/javascript/refactoring/examples/testdata/set_window_location_test_in.js b/test/com/google/javascript/refactoring/examples/testdata/set_window_location_test_in.js index 38925261a45..6d770791616 100644 --- a/test/com/google/javascript/refactoring/examples/testdata/set_window_location_test_in.js +++ b/test/com/google/javascript/refactoring/examples/testdata/set_window_location_test_in.js @@ -28,7 +28,7 @@ refactoring_testcase.test_window_location = function(target, val) { }; /** - * @param {!Window|!Element} target The target. + * @param {Window|Element} target The target. * @param {string} val The value. */ refactoring_testcase.union_type_location = function(target, val) { diff --git a/test/com/google/javascript/refactoring/examples/testdata/set_window_location_test_out.js b/test/com/google/javascript/refactoring/examples/testdata/set_window_location_test_out.js index 948c750db9f..f7c2f2f1c5c 100644 --- a/test/com/google/javascript/refactoring/examples/testdata/set_window_location_test_out.js +++ b/test/com/google/javascript/refactoring/examples/testdata/set_window_location_test_out.js @@ -29,7 +29,7 @@ refactoring_testcase.test_window_location = function(target, val) { }; /** - * @param {!Window|!Element} target The target. + * @param {Window|Element} target The target. * @param {string} val The value. */ refactoring_testcase.union_type_location = function(target, val) {