extend cases of unnecessary_null_checks/null_check_on_nullable_type_parameter by a14n · Pull Request #3258 · dart-archive/linter

a14n · 2022-02-28T15:12:47Z

Description

This PR extends the cases where unnecessary_null_checks and null_check_on_nullable_type_parameter apply:

in yield expressions
in list/set/map literals
in await expression

…arameter

coveralls · 2022-02-28T16:38:22Z

Coverage increased (+0.07%) to 95.681% when pulling 545ba5c on a14n:extend-null-checks into 8c01f68 on dart-lang:master.

a14n · 2022-03-03T07:50:06Z

@pq is there something blocking here?

bwilkerson · 2022-03-03T15:56:06Z

+  DartType? Function(DartType? type) mayHandleAwait = (type) => type;
+  if (parent is AwaitExpression) {
+    mayHandleAwait =
+        (type) => (type as ParameterizedType?)?.typeArguments.first;


I might be misunderstanding the code, but if the goal of this is to unwrap Future and FutureOr, it will actually attempt unwrap any potentially parameterized type. (The class ParameterizedType only means "a kind of type that is allowed to have type parameters". It doesn't mean "a type that actually has type parameters".)

I think that this will happily unwrap List<int>, and will crash when type is something without type arguments, such as String.

Consider using isDartAsyncFuture and isDartAsyncFutureOr explicitly.

humm, there was indeed issue with await handling. The return await case is the only one that need a special processing because the target type is really a Future (unlike other case where the unwrap type is provided)

bwilkerson · 2022-03-03T15:58:19Z

+      return null;
+    }
+    return mayHandleAwait(
+        (staticType.returnType as ParameterizedType).typeArguments.first);


This and a few places below have the same problem. Consider moving the logic into a single _unwrapFuture method / function.

a14n · 2022-03-03T16:49:00Z

+    if (staticType is! FunctionType) {
+      return null;
+    }
+    return (staticType.returnType as ParameterizedType).typeArguments.first;


Same here. Having a yield means that the return type is either Iterable<X> or Stream<X>. So a ParameterizedType.

No, having a yield means that the return type is a supertype of one of those types. dynamic works just fine and it isn't a ParameterizedType.

my bad, sorry and thanks

pq · 2022-03-03T17:20:05Z

@pq is there something blocking here?

Sorry was tangled up elsewhere. The big thing is to make sure these lints aren't being used in places where new diagnostics would block an SDK roll. I don't see them used internally on a quick scan.

I do see them enabled in flutter... Do you know if this will flag new cases there?

bwilkerson · 2022-03-03T17:54:14Z

+      return null;
+    }
+    if (withAwait || parentExpression.body.keyword?.lexeme == 'async') {
+      //  return type is obligatorily a Future<X>


The return type could also be void, dynamic, or Never, so it isn't safe to just cast it to a ParameterizedType; doing so could cause an exception to be thrown.

Even when it's a ParameterizedType, you can't assume that it has type arguments. Every InterfaceType is a ParameterizedType, but not every InterfaceType has type arguments. So asking for the first type argument without first checking that the list isn't empty will result in an exception being thrown.

Maybe a host of negative tests would be good here?

bwilkerson · 2022-03-03T18:00:39Z

+    if (staticType is! FunctionType) {
+      return null;
+    }
+    return (staticType.returnType as ParameterizedType).typeArguments.first;


No, having a yield means that the return type is a supertype of one of those types. dynamic works just fine and it isn't a ParameterizedType.

a14n · 2022-03-03T20:07:07Z

I do see them enabled in flutter... Do you know if this will flag new cases there?

I see only one additional lint fixed by flutter/flutter#99507

a14n · 2022-03-14T13:08:34Z

flutter/flutter#99507 is now merged.

PTAL

bwilkerson

It would be good to have some tests for the cases that the code is guarding against in order to ensure that it is correct.

Also, it didn't occur to me last time, but do we need to worry about subclasses of the targeted types? For example:

class Source extends Iterable<int?> {}

Source f16(int? p) async => p!; // LINT?

What about type aliases:

typedef Source = Iterable<int?>;

If so, we would need to use DartType.asInstanceOf to find the actual value of the type argument (and wouldn't need the isDartCore* tests).

a14n · 2022-03-14T16:49:34Z

Also, it didn't occur to me last time, but do we need to worry about subclasses of the targeted types? For example:
class Source extends Iterable<int?> {}

Source f16(int? p) async => p!; // LINT?

Sorry but I don't understand this example.

What about type aliases:
typedef Source = Iterable<int?>;
If so, we would need to use DartType.asInstanceOf to find the actual value of the type argument (and wouldn't need the isDartCore* tests).

I added a test without any change and it seems to work correctly:

typedef F16 = Future<int?>;
F16 f16(int? p) async => p!; // LINT

bwilkerson

Sorry but I don't understand this example.

I didn't realize that wasn't valid. Sorry for the noise.

Still wouldn't hurt to have some negative tests for the cases we're explicitly guarding against.

…arameter (dart-archive/linter#3258) * extend cases of unnecessary_null_checks/null_check_on_nullable_type_parameter * fix bad await handling * add handling of async * address review comments * add test for typedef * add some negative tests

extend cases of unnecessary_null_checks/null_check_on_nullable_type_p…

5081ed4

…arameter

bwilkerson reviewed Mar 3, 2022

View reviewed changes

fix bad await handling

401eac1

a14n commented Mar 3, 2022

View reviewed changes

add handling of async

1e8b797

bwilkerson reviewed Mar 3, 2022

View reviewed changes

address review comments

1227225

a14n mentioned this pull request Mar 3, 2022

remove unnecessary null check flutter/flutter#99507

Merged

bwilkerson reviewed Mar 14, 2022

View reviewed changes

add test for typedef

2695e5a

bwilkerson approved these changes Mar 14, 2022

View reviewed changes

pq approved these changes Mar 15, 2022

View reviewed changes

add some negative tests

545ba5c

pq merged commit de38ee4 into dart-archive:master Mar 15, 2022

a14n deleted the extend-null-checks branch March 16, 2022 07:10

Uh oh!

Conversation

a14n commented Feb 28, 2022

Description

Uh oh!

coveralls commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a14n commented Mar 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pq commented Mar 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a14n commented Mar 3, 2022

Uh oh!

a14n commented Mar 14, 2022

Uh oh!

bwilkerson left a comment

Choose a reason for hiding this comment

Uh oh!

a14n commented Mar 14, 2022

Uh oh!

bwilkerson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

coveralls commented Feb 28, 2022 •

edited

Loading