From 553fb9f5b965c686e1c3b4a3e06fdc06346ae288 Mon Sep 17 00:00:00 2001 From: dylandavidson Date: Thu, 15 Nov 2018 10:57:19 -0800 Subject: [PATCH] ErrorToFixMapper: Add fixes for missing nullability lint checks. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=221652339 --- .../refactoring/ErrorToFixMapper.java | 20 +- .../refactoring/ErrorToFixMapperTest.java | 224 ++++++++++-------- 2 files changed, 139 insertions(+), 105 deletions(-) diff --git a/src/com/google/javascript/refactoring/ErrorToFixMapper.java b/src/com/google/javascript/refactoring/ErrorToFixMapper.java index 59096711b99..105f3023831 100644 --- a/src/com/google/javascript/refactoring/ErrorToFixMapper.java +++ b/src/com/google/javascript/refactoring/ErrorToFixMapper.java @@ -66,7 +66,9 @@ public static ImmutableList getFixesForJsError( } switch (error.getType().key) { case "JSC_IMPLICITLY_NULLABLE_JSDOC": - return getFixesForImplicitlyNullableJsDoc(error, compiler); + case "JSC_MISSING_NULLABILITY_MODIFIER_JSDOC": + case "JSC_NULL_MISSING_NULLABILITY_MODIFIER_JSDOC": + return getFixesForImplicitNullabilityErrors(error, compiler); default: return ImmutableList.of(); } @@ -228,7 +230,7 @@ private static SuggestedFix getFixForReferenceToShortImportByLongName( .build(); } - private static ImmutableList getFixesForImplicitlyNullableJsDoc( + private static ImmutableList getFixesForImplicitNullabilityErrors( JSError error, AbstractCompiler compiler) { SuggestedFix qmark = new SuggestedFix.Builder() @@ -242,7 +244,19 @@ private static ImmutableList getFixesForImplicitlyNullableJsDoc( .insertBefore(error.node, "!") .setDescription("Make type non-nullable") .build(); - return ImmutableList.of(qmark, bang); + switch (error.getType().key) { + case "JSC_NULL_MISSING_NULLABILITY_MODIFIER_JSDOC": + // When initializer was null, we can be confident about nullability + return ImmutableList.of(qmark); + case "JSC_MISSING_NULLABILITY_MODIFIER_JSDOC": + // Otherwise, the linter should assume ! is preferred over ?. + return ImmutableList.of(bang, qmark); + case "JSC_IMPLICITLY_NULLABLE_JSDOC": + // The type-based check prefers ? over ! since it only warns for names that are nullable. + return ImmutableList.of(qmark, bang); + default: + throw new IllegalArgumentException("Unexpected JSError Type: " + error.getType().key); + } } private static SuggestedFix removeNode(JSError error, AbstractCompiler compiler) { diff --git a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java index fb54d75a32e..c7494c3d9dc 100644 --- a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java +++ b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java @@ -18,6 +18,7 @@ import static com.google.common.collect.ObjectArrays.concat; import static com.google.common.truth.Truth.assertThat; import static com.google.javascript.jscomp.CheckLevel.ERROR; +import static com.google.javascript.jscomp.CheckLevel.OFF; import static com.google.javascript.jscomp.CheckLevel.WARNING; import com.google.common.base.Joiner; @@ -29,7 +30,6 @@ import com.google.javascript.jscomp.JSError; import com.google.javascript.jscomp.SourceFile; import java.util.Collection; -import java.util.List; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -93,94 +93,112 @@ public void testEmptyStatement3() { @Test public void testImplicitNullability1() { - String originalCode = "/** @type {Object} */ var o;"; - compiler.compile( - ImmutableList.of(), // Externs - ImmutableList.of(SourceFile.fromCode("test", originalCode)), - options); - assertThat(compiler.getErrors()).isEmpty(); - JSError[] warnings = compiler.getWarnings(); - assertThat(warnings).hasLength(2); - JSError warning = warnings[0]; - List fixes = ErrorToFixMapper.getFixesForJsError(warning, compiler); - assertThat(fixes).hasSize(2); - - // First fix is to add "!" - String newCode = ApplySuggestedFixes.applySuggestedFixesToCode( - ImmutableList.of(fixes.get(0)), ImmutableMap.of("test", originalCode)).get("test"); - assertThat(newCode).isEqualTo("/** @type {?Object} */ var o;"); - - // Second fix is to add "?" - newCode = ApplySuggestedFixes.applySuggestedFixesToCode( - ImmutableList.of(fixes.get(1)), ImmutableMap.of("test", originalCode)).get("test"); - assertThat(newCode).isEqualTo("/** @type {!Object} */ var o;"); + options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, OFF); + assertChanges( + "/** @type {Object} */ var o;", + "/** @type {?Object} */ var o;", + "/** @type {!Object} */ var o;"); } @Test public void testImplicitNullability2() { - String originalCode = "/** @param {Object} o */ function f(o) {}"; - compiler.compile( - ImmutableList.of(), // Externs - ImmutableList.of(SourceFile.fromCode("test", originalCode)), - options); - assertThat(compiler.getErrors()).isEmpty(); - JSError[] warnings = compiler.getWarnings(); - assertThat(warnings).hasLength(2); - JSError warning = warnings[0]; - List fixes = ErrorToFixMapper.getFixesForJsError(warning, compiler); - assertThat(fixes).hasSize(2); - - // First fix is to add "!" - String newCode = ApplySuggestedFixes.applySuggestedFixesToCode( - ImmutableList.of(fixes.get(0)), ImmutableMap.of("test", originalCode)).get("test"); - assertThat(newCode).isEqualTo("/** @param {?Object} o */ function f(o) {}"); - - // Second fix is to add "?" - newCode = ApplySuggestedFixes.applySuggestedFixesToCode( - ImmutableList.of(fixes.get(1)), ImmutableMap.of("test", originalCode)).get("test"); - assertThat(newCode).isEqualTo("/** @param {!Object} o */ function f(o) {}"); + options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, OFF); + assertChanges( + "/** @param {Object} o */ function f(o) {}", + "/** @param {?Object} o */ function f(o) {}", + "/** @param {!Object} o */ function f(o) {}"); } @Test public void testImplicitNullability3() { + options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, OFF); String originalCode = LINE_JOINER.join( "/**", " * Some non-ASCII characters: αβγδε", " * @param {Object} o", " */", "function f(o) {}"); - compiler.compile( - ImmutableList.of(), // Externs - ImmutableList.of(SourceFile.fromCode("test", originalCode)), - options); - assertThat(compiler.getErrors()).isEmpty(); - JSError[] warnings = compiler.getWarnings(); - assertThat(warnings).hasLength(2); - JSError warning = warnings[0]; - List fixes = ErrorToFixMapper.getFixesForJsError(warning, compiler); - assertThat(fixes).hasSize(2); - - // First fix is to add "!" - String newCode = ApplySuggestedFixes.applySuggestedFixesToCode( - ImmutableList.of(fixes.get(0)), ImmutableMap.of("test", originalCode)).get("test"); - String expected = LINE_JOINER.join( - "/**", - " * Some non-ASCII characters: αβγδε", - " * @param {?Object} o", - " */", - "function f(o) {}"); - assertThat(newCode).isEqualTo(expected); + String expected1 = + LINE_JOINER.join( + "/**", + " * Some non-ASCII characters: αβγδε", + " * @param {?Object} o", + " */", + "function f(o) {}"); + String expected2 = + LINE_JOINER.join( + "/**", + " * Some non-ASCII characters: αβγδε", + " * @param {!Object} o", + " */", + "function f(o) {}"); + assertChanges(originalCode, expected1, expected2); + } - // Second fix is to add "?" - newCode = ApplySuggestedFixes.applySuggestedFixesToCode( - ImmutableList.of(fixes.get(1)), ImmutableMap.of("test", originalCode)).get("test"); - expected = LINE_JOINER.join( - "/**", - " * Some non-ASCII characters: αβγδε", - " * @param {!Object} o", - " */", - "function f(o) {}"); - assertThat(newCode).isEqualTo(expected); + @Test + public void testMissingNullabilityModifier1() { + options.setWarningLevel(DiagnosticGroups.ANALYZER_CHECKS, OFF); + assertChanges( + "/** @type {Object} */ var o;", + "/** @type {!Object} */ var o;", + "/** @type {?Object} */ var o;"); + } + + @Test + public void testMissingNullabilityModifier2() { + options.setWarningLevel(DiagnosticGroups.ANALYZER_CHECKS, OFF); + assertChanges( + "/** @param {Object} o */ function f(o) {}", + "/** @param {!Object} o */ function f(o) {}", + "/** @param {?Object} o */ function f(o) {}"); + } + + @Test + public void testMissingNullabilityModifier3() { + options.setWarningLevel(DiagnosticGroups.ANALYZER_CHECKS, OFF); + String originalCode = + LINE_JOINER.join( + "/**", + " * Some non-ASCII characters: αβγδε", + " * @param {Object} o", + " */", + "function f(o) {}"); + String expected1 = + LINE_JOINER.join( + "/**", + " * Some non-ASCII characters: αβγδε", + " * @param {!Object} o", + " */", + "function f(o) {}"); + String expected2 = + LINE_JOINER.join( + "/**", + " * Some non-ASCII characters: αβγδε", + " * @param {?Object} o", + " */", + "function f(o) {}"); + assertChanges(originalCode, expected1, expected2); + } + + @Test + public void testNullMissingNullabilityModifier1() { + options.setWarningLevel(DiagnosticGroups.ANALYZER_CHECKS, OFF); + assertChanges("/** @type {Object} */ var x = null;", "/** @type {?Object} */ var x = null;"); + } + + @Test + public void testNullMissingNullabilityModifier2() { + options.setWarningLevel(DiagnosticGroups.ANALYZER_CHECKS, OFF); + assertNoChanges("/** @type {?Object} */ var x = null;"); + } + + @Test + public void testMissingNullabilityModifier_nonNullValue() { + options.setWarningLevel(DiagnosticGroups.ANALYZER_CHECKS, OFF); + assertChanges( + "/** @type {Object} */ var o = {};", + "/** @type {!Object} */ var o = {};", + "/** @type {?Object} */ var o = {};"); } @Test @@ -940,7 +958,7 @@ public void testAddLhsToGoogRequire_jsdoc() { "", "goog.require('world.util.Animal');", "", - "/** @type {world.util.Animal} */", + "/** @type {!world.util.Animal} */", "var cat;")); } @@ -980,23 +998,6 @@ public void testSwitchToShorthand_JSDoc2() { @Test public void testSwitchToShorthand_JSDoc3() { - assertChanges( - LINE_JOINER.join( - "goog.module('m');", - "var Animal = goog.require('world.util.Animal');", - "", - "/** @type {world.util.Animal} */", - "var animal;"), - LINE_JOINER.join( - "goog.module('m');", - "var Animal = goog.require('world.util.Animal');", - "", - "/** @type {Animal} */", - "var animal;")); - } - - @Test - public void testSwitchToShorthand_JSDoc4() { assertChanges( LINE_JOINER.join( "goog.module('m');", @@ -1013,7 +1014,7 @@ public void testSwitchToShorthand_JSDoc4() { } @Test - public void testSwitchToShorthand_JSDoc5() { + public void testSwitchToShorthand_JSDoc4() { assertChanges( LINE_JOINER.join( "goog.module('m');", @@ -1030,53 +1031,53 @@ public void testSwitchToShorthand_JSDoc5() { } @Test - public void testSwitchToShorthand_JSDoc6() { + public void testSwitchToShorthand_JSDoc5() { assertChanges( LINE_JOINER.join( "goog.module('m');", "var Animal = goog.require('world.util.Animal');", "", - "/** @type {?Array} */", + "/** @type {?Array} */", "var animals;"), LINE_JOINER.join( "goog.module('m');", "var Animal = goog.require('world.util.Animal');", "", - "/** @type {?Array} */", + "/** @type {?Array} */", "var animals;")); } @Test - public void testSwitchToShorthand_JSDoc7() { + public void testSwitchToShorthand_JSDoc6() { assertChanges( LINE_JOINER.join( "goog.module('m');", "var Animal = goog.require('world.util.Animal');", "", - "/** @type {?Array} */", + "/** @type {?Array} */", "var turtles;"), LINE_JOINER.join( "goog.module('m');", "var Animal = goog.require('world.util.Animal');", "", - "/** @type {?Array} */", + "/** @type {?Array} */", "var turtles;")); } @Test - public void testSwitchToShorthand_JSDoc8() { + public void testSwitchToShorthand_JSDoc7() { assertChanges( LINE_JOINER.join( "goog.module('m');", "var AnimalAltName = goog.require('world.util.Animal');", "", - "/** @type {?Array} */", + "/** @type {?Array} */", "var turtles;"), LINE_JOINER.join( "goog.module('m');", "var AnimalAltName = goog.require('world.util.Animal');", "", - "/** @type {?Array} */", + "/** @type {?Array} */", "var turtles;")); } @@ -1702,6 +1703,25 @@ private void assertChanges(String originalCode, String expectedCode) { assertThat(newCode).isEqualTo(expectedCode); } + private void assertChanges(String originalCode, String... expectedFixes) { + compiler.compile( + ImmutableList.of(), // Externs + ImmutableList.of(SourceFile.fromCode("test", originalCode)), + options); + JSError[] warningsAndErrors = + concat(compiler.getWarnings(), compiler.getErrors(), JSError.class); + assertThat(warningsAndErrors).named("warnings/errors").isNotEmpty(); + SuggestedFix[] fixes = errorManager.getAllFixes().toArray(new SuggestedFix[0]); + assertThat(fixes).named("fixes").hasLength(expectedFixes.length); + for (int i = 0; i < fixes.length; i++) { + String newCode = + ApplySuggestedFixes.applySuggestedFixesToCode( + ImmutableList.of(fixes[i]), ImmutableMap.of("test", originalCode)) + .get("test"); + assertThat(newCode).isEqualTo(expectedFixes[i]); + } + } + protected void assertNoChanges(String originalCode) { compiler.compile( ImmutableList.of(), // Externs