Skip to content

Commit

Permalink
ErrorToFixMapper: Add fixes for missing nullability lint checks.
Browse files Browse the repository at this point in the history 
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=221652339
  • Loading branch information
@DylanDavidson @brad4d
DylanDavidson authored and brad4d committed Nov 17, 2018
1 parent 0a7267d commit 553fb9f
Show file tree
Hide file tree
Showing 2 changed files with 139 additions and 105 deletions.
20 changes: 17 additions & 3 deletions src/com/google/javascript/refactoring/ErrorToFixMapper.java
View file
Expand Up @@ -66,7 +66,9 @@ public static ImmutableList<SuggestedFix> getFixesForJsError(
} }
switch (error.getType().key) { switch (error.getType().key) {
case "JSC_IMPLICITLY_NULLABLE_JSDOC": 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: default:
return ImmutableList.of(); return ImmutableList.of();
} }
Expand Down Expand Up @@ -228,7 +230,7 @@ private static SuggestedFix getFixForReferenceToShortImportByLongName(
.build(); .build();
} }


private static ImmutableList<SuggestedFix> getFixesForImplicitlyNullableJsDoc( private static ImmutableList<SuggestedFix> getFixesForImplicitNullabilityErrors(
JSError error, AbstractCompiler compiler) { JSError error, AbstractCompiler compiler) {
SuggestedFix qmark = SuggestedFix qmark =
new SuggestedFix.Builder() new SuggestedFix.Builder()
Expand All @@ -242,7 +244,19 @@ private static ImmutableList<SuggestedFix> getFixesForImplicitlyNullableJsDoc(
.insertBefore(error.node, "!") .insertBefore(error.node, "!")
.setDescription("Make type non-nullable") .setDescription("Make type non-nullable")
.build(); .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) { private static SuggestedFix removeNode(JSError error, AbstractCompiler compiler) {
Expand Down
224 changes: 122 additions & 102 deletions test/com/google/javascript/refactoring/ErrorToFixMapperTest.java
View file
Expand Up @@ -18,6 +18,7 @@
import static com.google.common.collect.ObjectArrays.concat; import static com.google.common.collect.ObjectArrays.concat;
import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertThat;
import static com.google.javascript.jscomp.CheckLevel.ERROR; 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 static com.google.javascript.jscomp.CheckLevel.WARNING;


import com.google.common.base.Joiner; import com.google.common.base.Joiner;
Expand All @@ -29,7 +30,6 @@
import com.google.javascript.jscomp.JSError; import com.google.javascript.jscomp.JSError;
import com.google.javascript.jscomp.SourceFile; import com.google.javascript.jscomp.SourceFile;
import java.util.Collection; import java.util.Collection;
import java.util.List;
import org.junit.Before; import org.junit.Before;
import org.junit.Test; import org.junit.Test;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
Expand Down Expand Up @@ -93,94 +93,112 @@ public void testEmptyStatement3() {


@Test @Test
public void testImplicitNullability1() { public void testImplicitNullability1() {
String originalCode = "/** @type {Object} */ var o;"; options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, OFF);
compiler.compile( assertChanges(
ImmutableList.<SourceFile>of(), // Externs "/** @type {Object} */ var o;",
ImmutableList.of(SourceFile.fromCode("test", originalCode)), "/** @type {?Object} */ var o;",
options); "/** @type {!Object} */ var o;");
assertThat(compiler.getErrors()).isEmpty();
JSError[] warnings = compiler.getWarnings();
assertThat(warnings).hasLength(2);
JSError warning = warnings[0];
List<SuggestedFix> 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;");
} }


@Test @Test
public void testImplicitNullability2() { public void testImplicitNullability2() {
String originalCode = "/** @param {Object} o */ function f(o) {}"; options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, OFF);
compiler.compile( assertChanges(
ImmutableList.<SourceFile>of(), // Externs "/** @param {Object} o */ function f(o) {}",
ImmutableList.of(SourceFile.fromCode("test", originalCode)), "/** @param {?Object} o */ function f(o) {}",
options); "/** @param {!Object} o */ function f(o) {}");
assertThat(compiler.getErrors()).isEmpty();
JSError[] warnings = compiler.getWarnings();
assertThat(warnings).hasLength(2);
JSError warning = warnings[0];
List<SuggestedFix> 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) {}");
} }


@Test @Test
public void testImplicitNullability3() { public void testImplicitNullability3() {
options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, OFF);
String originalCode = LINE_JOINER.join( String originalCode = LINE_JOINER.join(
"/**", "/**",
" * Some non-ASCII characters: αβγδε", " * Some non-ASCII characters: αβγδε",
" * @param {Object} o", " * @param {Object} o",
" */", " */",
"function f(o) {}"); "function f(o) {}");
compiler.compile( String expected1 =
ImmutableList.<SourceFile>of(), // Externs LINE_JOINER.join(
ImmutableList.of(SourceFile.fromCode("test", originalCode)), "/**",
options); " * Some non-ASCII characters: αβγδε",
assertThat(compiler.getErrors()).isEmpty(); " * @param {?Object} o",
JSError[] warnings = compiler.getWarnings(); " */",
assertThat(warnings).hasLength(2); "function f(o) {}");
JSError warning = warnings[0]; String expected2 =
List<SuggestedFix> fixes = ErrorToFixMapper.getFixesForJsError(warning, compiler); LINE_JOINER.join(
assertThat(fixes).hasSize(2); "/**",

" * Some non-ASCII characters: αβγδε",
// First fix is to add "!" " * @param {!Object} o",
String newCode = ApplySuggestedFixes.applySuggestedFixesToCode( " */",
ImmutableList.of(fixes.get(0)), ImmutableMap.of("test", originalCode)).get("test"); "function f(o) {}");
String expected = LINE_JOINER.join( assertChanges(originalCode, expected1, expected2);
"/**", }
" * Some non-ASCII characters: αβγδε",
" * @param {?Object} o",
" */",
"function f(o) {}");
assertThat(newCode).isEqualTo(expected);


// Second fix is to add "?" @Test
newCode = ApplySuggestedFixes.applySuggestedFixesToCode( public void testMissingNullabilityModifier1() {
ImmutableList.of(fixes.get(1)), ImmutableMap.of("test", originalCode)).get("test"); options.setWarningLevel(DiagnosticGroups.ANALYZER_CHECKS, OFF);
expected = LINE_JOINER.join( assertChanges(
"/**", "/** @type {Object} */ var o;",
" * Some non-ASCII characters: αβγδε", "/** @type {!Object} */ var o;",
" * @param {!Object} o", "/** @type {?Object} */ var o;");
" */", }
"function f(o) {}");
assertThat(newCode).isEqualTo(expected); @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 @Test
Expand Down Expand Up @@ -940,7 +958,7 @@ public void testAddLhsToGoogRequire_jsdoc() {
"", "",
"goog.require('world.util.Animal');", "goog.require('world.util.Animal');",
"", "",
"/** @type {world.util.Animal} */", "/** @type {!world.util.Animal} */",
"var cat;")); "var cat;"));
} }


Expand Down Expand Up @@ -980,23 +998,6 @@ public void testSwitchToShorthand_JSDoc2() {


@Test @Test
public void testSwitchToShorthand_JSDoc3() { 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( assertChanges(
LINE_JOINER.join( LINE_JOINER.join(
"goog.module('m');", "goog.module('m');",
Expand All @@ -1013,7 +1014,7 @@ public void testSwitchToShorthand_JSDoc4() {
} }


@Test @Test
public void testSwitchToShorthand_JSDoc5() { public void testSwitchToShorthand_JSDoc4() {
assertChanges( assertChanges(
LINE_JOINER.join( LINE_JOINER.join(
"goog.module('m');", "goog.module('m');",
Expand All @@ -1030,53 +1031,53 @@ public void testSwitchToShorthand_JSDoc5() {
} }


@Test @Test
public void testSwitchToShorthand_JSDoc6() { public void testSwitchToShorthand_JSDoc5() {
assertChanges( assertChanges(
LINE_JOINER.join( LINE_JOINER.join(
"goog.module('m');", "goog.module('m');",
"var Animal = goog.require('world.util.Animal');", "var Animal = goog.require('world.util.Animal');",
"", "",
"/** @type {?Array<world.util.Animal>} */", "/** @type {?Array<!world.util.Animal>} */",
"var animals;"), "var animals;"),
LINE_JOINER.join( LINE_JOINER.join(
"goog.module('m');", "goog.module('m');",
"var Animal = goog.require('world.util.Animal');", "var Animal = goog.require('world.util.Animal');",
"", "",
"/** @type {?Array<Animal>} */", "/** @type {?Array<!Animal>} */",
"var animals;")); "var animals;"));
} }


@Test @Test
public void testSwitchToShorthand_JSDoc7() { public void testSwitchToShorthand_JSDoc6() {
assertChanges( assertChanges(
LINE_JOINER.join( LINE_JOINER.join(
"goog.module('m');", "goog.module('m');",
"var Animal = goog.require('world.util.Animal');", "var Animal = goog.require('world.util.Animal');",
"", "",
"/** @type {?Array<world.util.Animal.Turtle>} */", "/** @type {?Array<!world.util.Animal.Turtle>} */",
"var turtles;"), "var turtles;"),
LINE_JOINER.join( LINE_JOINER.join(
"goog.module('m');", "goog.module('m');",
"var Animal = goog.require('world.util.Animal');", "var Animal = goog.require('world.util.Animal');",
"", "",
"/** @type {?Array<Animal.Turtle>} */", "/** @type {?Array<!Animal.Turtle>} */",
"var turtles;")); "var turtles;"));
} }


@Test @Test
public void testSwitchToShorthand_JSDoc8() { public void testSwitchToShorthand_JSDoc7() {
assertChanges( assertChanges(
LINE_JOINER.join( LINE_JOINER.join(
"goog.module('m');", "goog.module('m');",
"var AnimalAltName = goog.require('world.util.Animal');", "var AnimalAltName = goog.require('world.util.Animal');",
"", "",
"/** @type {?Array<world.util.Animal.Turtle>} */", "/** @type {?Array<!world.util.Animal.Turtle>} */",
"var turtles;"), "var turtles;"),
LINE_JOINER.join( LINE_JOINER.join(
"goog.module('m');", "goog.module('m');",
"var AnimalAltName = goog.require('world.util.Animal');", "var AnimalAltName = goog.require('world.util.Animal');",
"", "",
"/** @type {?Array<AnimalAltName.Turtle>} */", "/** @type {?Array<!AnimalAltName.Turtle>} */",
"var turtles;")); "var turtles;"));
} }


Expand Down Expand Up @@ -1702,6 +1703,25 @@ private void assertChanges(String originalCode, String expectedCode) {
assertThat(newCode).isEqualTo(expectedCode); assertThat(newCode).isEqualTo(expectedCode);
} }


private void assertChanges(String originalCode, String... expectedFixes) {
compiler.compile(
ImmutableList.<SourceFile>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) { protected void assertNoChanges(String originalCode) {
compiler.compile( compiler.compile(
ImmutableList.<SourceFile>of(), // Externs ImmutableList.<SourceFile>of(), // Externs
Expand Down

0 comments on commit 553fb9f

Please sign in to comment.