Skip to content

Commit

Permalink
Add more specific warning when implicity nullable types are explicitl…
Browse files Browse the repository at this point in the history
…y set to null.

This will allow the ErrorToFixMapper to have special case handling for this, to
just only add '?' (and not consider '!'). I also plan to run this over all of
the Docs code base (and maybe more) to try to minimize implicit nullability usage.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=221443852
  • Loading branch information
DylanDavidson authored and brad4d committed Nov 14, 2018
1 parent 21aa01f commit 0a501b3
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 3 deletions.
1 change: 1 addition & 0 deletions src/com/google/javascript/jscomp/DiagnosticGroups.java
Expand Up @@ -585,6 +585,7 @@ public DiagnosticGroup forName(String name) {
CheckInterfaces.INTERFACE_SHOULD_NOT_TAKE_ARGS,
CheckMissingSemicolon.MISSING_SEMICOLON,
CheckNullabilityModifiers.MISSING_NULLABILITY_MODIFIER_JSDOC,
CheckNullabilityModifiers.NULL_MISSING_NULLABILITY_MODIFIER_JSDOC,
CheckNullabilityModifiers.REDUNDANT_NULLABILITY_MODIFIER_JSDOC,
CheckPrimitiveAsObject.NEW_PRIMITIVE_OBJECT,
CheckPrimitiveAsObject.PRIMITIVE_OBJECT_DECLARATION,
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/NodeUtil.java
Expand Up @@ -3277,7 +3277,7 @@ static boolean isFunctionBind(Node expr) {
* @param parent Parent of the node
* @return True if n is the left hand of an assign
*/
static boolean isNameDeclOrSimpleAssignLhs(Node n, Node parent) {
public static boolean isNameDeclOrSimpleAssignLhs(Node n, Node parent) {
return
(parent.isAssign() && parent.getFirstChild() == n) || NodeUtil.isNameDeclaration(parent);
}
Expand Down
Expand Up @@ -46,6 +46,13 @@ public class CheckNullabilityModifiers extends AbstractPostOrderCallback impleme
+ "Please add a '!' to make it explicitly non-nullable, "
+ "or a '?' to make it explicitly nullable.");

/** The diagnostic for a missing nullability modifier, where the value is clearly nullable. */
public static final DiagnosticType NULL_MISSING_NULLABILITY_MODIFIER_JSDOC =
DiagnosticType.disabled(
"JSC_NULL_MISSING_NULLABILITY_MODIFIER_JSDOC",
"{0} is a reference type with no nullability modifier that is explicitly set to null.\n"
+ "Add a '?' to make it explicitly nullable.");

/** The diagnostic for a redundant nullability modifier. */
public static final DiagnosticType REDUNDANT_NULLABILITY_MODIFIER_JSDOC =
DiagnosticType.disabled(
Expand All @@ -62,6 +69,7 @@ public class CheckNullabilityModifiers extends AbstractPostOrderCallback impleme
// Store the candidate warnings and template types found while traversing a single script node.
private final HashSet<Node> redundantCandidates = new HashSet<>();
private final HashSet<Node> missingCandidates = new HashSet<>();
private final HashSet<Node> nullMissingCandidates = new HashSet<>();
private final HashSet<String> templateTypeNames = new HashSet<>();

public CheckNullabilityModifiers(AbstractCompiler compiler) {
Expand All @@ -79,7 +87,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
if (info != null) {
templateTypeNames.addAll(info.getTemplateTypeNames());
if (info.hasType()) {
visitTypeExpression(info.getType(), false);
handleHasType(info.getType(), n);
}
for (String param : info.getParameterNames()) {
if (info.hasParameterType(param)) {
Expand Down Expand Up @@ -116,17 +124,35 @@ public void visit(NodeTraversal t, Node n, Node parent) {
report(t);
redundantCandidates.clear();
missingCandidates.clear();
nullMissingCandidates.clear();
templateTypeNames.clear();
return;
}
}

private void handleHasType(JSTypeExpression expr, Node n) {
// Check if the type is explicitly set to null, if so use NULL_MISSING_NULLABILITY diagnostic.
if (NodeUtil.isNameDeclOrSimpleAssignLhs(n.getFirstChild(), n)) {
Node rValue = NodeUtil.getRValueOfLValue(n.getFirstChild());
if (rValue != null && rValue.isNull()) {
visitTypeExpression(expr, false, rValue);
return;
}
}
visitTypeExpression(expr, false);
}

private void report(NodeTraversal t) {
for (Node n : missingCandidates) {
if (shouldReport(n)) {
t.report(n, MISSING_NULLABILITY_MODIFIER_JSDOC, getReportedTypeName(n));
}
}
for (Node n : nullMissingCandidates) {
if (shouldReport(n)) {
t.report(n, NULL_MISSING_NULLABILITY_MODIFIER_JSDOC, getReportedTypeName(n));
}
}
for (Node n : redundantCandidates) {
if (shouldReport(n)) {
// Report on parent so that modifier is also highlighted.
Expand All @@ -146,6 +172,11 @@ private boolean shouldReport(Node n) {
}

private void visitTypeExpression(JSTypeExpression expr, boolean hasArtificialTopLevelBang) {
visitTypeExpression(expr, hasArtificialTopLevelBang, /* rValue= */ null);
}

private void visitTypeExpression(
JSTypeExpression expr, boolean hasArtificialTopLevelBang, Node rValue) {
Node root = expr.getRoot();
NodeUtil.visitPreOrder(
root,
Expand All @@ -170,7 +201,11 @@ private void visitTypeExpression(JSTypeExpression expr, boolean hasArtificialTop
boolean isTypeOfType = parent != null && parent.isTypeOf();

if (isReference && !hasBang && !hasQmark && !isNewOrThis && !isTypeOfType) {
missingCandidates.add(node);
if (rValue != null && rValue.isNull()) {
nullMissingCandidates.add(node);
} else {
missingCandidates.add(node);
}
} else if (isPrimitiveOrLiteral && hasNonArtificialBang) {
redundantCandidates.add(node);
}
Expand Down
Expand Up @@ -235,6 +235,20 @@ public void testMultipleFiles() {
"/** @param {T} x */ function g(x){}");
}

@Test
public void testSetToNull() {
checkNullMissingWarning("/** @type {Object} */ var x = null;");
checkNullMissingWarning(
"/** @constructor */ function C() {} /** @private {Object} */ C.prop = null;");
checkNullMissingWarning(
"/** @constructor */ function C() { /** @private {Object} */ this.foo = null; }");

checkNoWarning("/** @type {?Object} */ var x = null;");
checkNoWarning("/** @constructor */ function C() {} /** @private {?Symbol} */ C.prop = null;");
checkNoWarning(
"/** @constructor */ function C() { /** @private {?Object} */ this.foo = null; }");
}

private void checkNoWarning(String... js) {
testSame(js);
}
Expand All @@ -243,6 +257,10 @@ private void checkMissingWarning(String... js) {
testWarning(js, CheckNullabilityModifiers.MISSING_NULLABILITY_MODIFIER_JSDOC);
}

private void checkNullMissingWarning(String... js) {
testWarning(js, CheckNullabilityModifiers.NULL_MISSING_NULLABILITY_MODIFIER_JSDOC);
}

private void checkRedundantWarning(String... js) {
testWarning(js, CheckNullabilityModifiers.REDUNDANT_NULLABILITY_MODIFIER_JSDOC);
}
Expand Down

0 comments on commit 0a501b3

Please sign in to comment.