extend cases of unnecessary_null_checks/null_check_on_nullable_type_parameter #3258
Conversation
|
@pq is there something blocking here? |
| DartType? Function(DartType? type) mayHandleAwait = (type) => type; | ||
| if (parent is AwaitExpression) { | ||
| mayHandleAwait = | ||
| (type) => (type as ParameterizedType?)?.typeArguments.first; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
| return null; | ||
| } | ||
| return mayHandleAwait( | ||
| (staticType.returnType as ParameterizedType).typeArguments.first); |
There was a problem hiding this comment.
This and a few places below have the same problem. Consider moving the logic into a single _unwrapFuture method / function.
| if (staticType is! FunctionType) { | ||
| return null; | ||
| } | ||
| return (staticType.returnType as ParameterizedType).typeArguments.first; |
There was a problem hiding this comment.
Same here. Having a yield means that the return type is either Iterable<X> or Stream<X>. So a ParameterizedType.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
my bad, sorry and thanks
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? |
| return null; | ||
| } | ||
| if (withAwait || parentExpression.body.keyword?.lexeme == 'async') { | ||
| // return type is obligatorily a Future<X> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Maybe a host of negative tests would be good here?
| if (staticType is! FunctionType) { | ||
| return null; | ||
| } | ||
| return (staticType.returnType as ParameterizedType).typeArguments.first; |
There was a problem hiding this comment.
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.
I see only one additional lint fixed by flutter/flutter#99507 |
|
flutter/flutter#99507 is now merged. PTAL |
There was a problem hiding this comment.
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).
Sorry but I don't understand this example.
I added a test without any change and it seems to work correctly: typedef F16 = Future<int?>;
F16 f16(int? p) async => p!; // LINT |
…arameter (dart-lang/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
Description
This PR extends the cases where
unnecessary_null_checksandnull_check_on_nullable_type_parameterapply:yieldexpressionsawaitexpressionRelated to #3256
/cc @srawlins