Skip to content
This repository was archived by the owner on Nov 20, 2024. It is now read-only.

add null_check_on_nullable_type_parameter #2245

Merged
merged 3 commits into from
Sep 22, 2020

Conversation

a14n
Copy link
Contributor

@a14n a14n commented Sep 18, 2020

Description

Don't use null check on a potentially nullable type parameter.
BAD:

T run<T>(T callback()) {
  T? result;
   (() { result = callback(); })();
  return result!;
}

GOOD:

T run<T>(T callback()) {
  T? result;
   (() { result = callback(); })();
  return result as T;
}

Fixes dart-lang/sdk#58231

In this PR I also extended unnecessary_null_checks to return cases.

if (node.operator.type != TokenType.BANG) return;

final expectedType = getExpectedType(node);
if (context.typeSystem.isPotentiallyNullable(node.operand.staticType) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this quite captures the original request. The original request specified an operand whose type is explicitly nullable (T?), where that type is a TypeParameterType that is potentially nullable. In particular, I think the test case m1<T>(T p) => p!; should not create a lint because in this case the operator isn't being used to remove nullability from the type (which would be more obvious if the method had an explicit return type of T).

It might be worthwhile asking on the issue whether that test case ought to produce a lint. It's possible that it was an oversight that the original description doesn't match that case.

final expectedType = getExpectedType(node);
if (context.typeSystem.isPotentiallyNullable(node.operand.staticType) &&
expectedType != null &&
context.typeSystem.isPotentiallyNullable(expectedType)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't validate that the expected type is T. In particular, I think the following would produce a lint when it's outside the original request:

R m<P, R>(P p) => p!;

It might be worth asking on the issue to make suer we really don't want to catch the case above.

@@ -95,3 +75,33 @@ class _Visitor extends SimpleAstVisitor<void> {
}
}
}

DartType getExpectedType(PostfixExpression node) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving this to one of the utility libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put this function here because it is really related to the lints (that is where null check checks happen). If we move it in utility library we should rename it to clearly indicate that it not only returns the expected type but it also only works where null_check_on_nullable_type_parameter and unnecessary_null_checks should apply.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm. It just feels like a code smell to have code being shared between lints that isn't in a utility library that's intended to be shared.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, is it ok to let this function here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't impact the correctness of the lint, so I suppose so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think I addressed all your comments and this PR is ready for another look :)


// test w/ `pub run test -N null_check_on_nullable_type_parameter`

m1<T>(T p) => p!; // LINT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these methods should have an explicit return type. As written, the return type is dynamic, which is potentially nullable, so the last part of the condition isn't actually being tested.

T m4<T extends Object?>(T? p) => p!; // LINT
T m5<T extends dynamic>(T? p) => p!; // LINT
T m6<T>(T p) => p!.a; // OK
T m7<T>(T p) => p!.m(); // OK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worthwhile to test cascades (p!..m()).

T m7<T>(T p) => p!.m(); // OK

T m10<T>(T? p) { return p!; } // LINT
T m20<T>(T? p) { T t = p!; } // LINT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also catch assignments such as

T m30<T>(T? p) {
  T t;
  t = p!;
}

or even

class C<T> {
  T t;
  m(T? p) {
    t = p!;
  }
}

if (node.operator.type != TokenType.BANG) return;

final expectedType = getExpectedType(node);
if (node.operand.staticType is TypeParameterType &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider assigning node.operand.staticType to a local variable (such as operandType). I think this condition would be easier to read.

@pq
Copy link
Contributor

pq commented Sep 22, 2020

This looks great.

The pana failure is distracting but I think ignorable.

image

I'm guessing this is a version skew issue since dartfmt is clean for me and also in our checks on travis.

/fyi @jonasfj

@pq pq merged commit 7b7bdb1 into dart-archive:master Sep 22, 2020
@a14n a14n deleted the null_check_on_nullable_type_parameter branch September 22, 2020 17:08
@devoncarew
Copy link
Contributor

We're seeing a (low volume) NPE from this lint internally, on or around line https://github.com/dart-lang/linter/blob/master/lib/src/rules/null_check_on_nullable_type_parameter.dart#L81:

dart-lang/sdk#58255

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

Consider a lint warning on uses of the null check operator on variables of type T?
5 participants