Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

extend cases of unnecessary_null_checks/null_check_on_nullable_type_parameter #3258

Merged
merged 6 commits into from
Mar 15, 2022
Merged

extend cases of unnecessary_null_checks/null_check_on_nullable_type_parameter #3258

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5081ed4
extend cases of unnecessary_null_checks/null_check_on_nullable_type_p…
a14n Feb 28, 2022
401eac1
fix bad await handling
a14n Mar 3, 2022
1e8b797
add handling of async
a14n Mar 3, 2022
1227225
address review comments
a14n Mar 3, 2022
2695e5a
add test for typedef
a14n Mar 14, 2022
545ba5c
add some negative tests
a14n Mar 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 44 additions & 5 deletions lib/src/rules/unnecessary_null_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,36 @@ DartType? getExpectedType(PostfixExpression node) {
node.thisOrAncestorMatching((e) => e.parent is! ParenthesizedExpression);
var parent = realNode?.parent;

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

parent = parent.parent;
}

// in return value
if (parent is ReturnStatement || parent is ExpressionFunctionBody) {
var parentExpression = parent?.thisOrAncestorOfType<FunctionExpression>();
if (parentExpression == null) {
return null;
}
var staticType = parentExpression.staticType;
return staticType is FunctionType ? staticType.returnType : null;
return staticType is FunctionType
? mayHandleAwait(staticType.returnType)
: null;
}
// in yield value
if (parent is YieldStatement) {
var parentExpression = parent.thisOrAncestorOfType<FunctionExpression>();
if (parentExpression == null) {
return null;
}
var staticType = parentExpression.staticType;
if (staticType is! FunctionType) {
return null;
}
return mayHandleAwait(
(staticType.returnType as ParameterizedType).typeArguments.first);
Copy link
Member

Choose a reason for hiding this comment

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

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

}
// assignment
if (parent is AssignmentExpression &&
Expand All @@ -57,27 +79,44 @@ DartType? getExpectedType(PostfixExpression node) {
node.operand is! Identifier ||
(parent.leftHandSide as Identifier).name !=
(node.operand as Identifier).name)) {
return parent.writeType;
return mayHandleAwait(parent.writeType);
}
// in variable declaration
if (parent is VariableDeclaration) {
return parent.declaredElement?.type;
return mayHandleAwait(parent.declaredElement?.type);
}
// as right member of binary operator
if (parent is BinaryExpression && parent.rightOperand == realNode) {
var parentElement = parent.staticElement;
if (parentElement == null) {
return null;
}
return parentElement.parameters.first.type;
return mayHandleAwait(parentElement.parameters.first.type);
}
// as member of list
if (parent is ListLiteral) {
return mayHandleAwait(
(parent.staticType as ParameterizedType?)?.typeArguments.first);
}
// as member of set
if (parent is SetOrMapLiteral && parent.isSet) {
return mayHandleAwait(
(parent.staticType as ParameterizedType?)?.typeArguments.first);
}
// as member of map
if (parent is MapLiteralEntry) {
var typeParameters =
(parent.parent! as SetOrMapLiteral).staticType as ParameterizedType?;
return mayHandleAwait(
typeParameters?.typeArguments[parent.key == node ? 0 : 1]);
}
// as parameter of function
if (parent is NamedExpression) {
realNode = parent;
parent = parent.parent;
}
if (parent is ArgumentList && realNode is Expression) {
return realNode.staticParameterElement?.type;
return mayHandleAwait(realNode.staticParameterElement?.type);
}
return null;
}
Expand Down
8 changes: 8 additions & 0 deletions test_data/rules/experiments/nnbd/rules/null_check_on_nullable_type_parameter.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,14 @@ T m30<T>(T? p) {
t = p!; // LINT
return t;
}
Future<T> m40<T extends Object?>(T? p) async => await p!; // LINT
Future<List<T>> m41<T extends Object?>(T? p) async => await [p!]; // LINT
List<T> m50<T>(T? p) => [p!]; // LINT
Set<T> m60<T>(T? p) => {p!}; // LINT
Map<String, T> m71<T>(T? p) => {'': p!}; // LINT
Map<T, String> m72<T>(T? p) => {p!: ''}; // LINT
Iterable<T> m80<T>(T? p) sync* {yield p!;} // LINT
Stream<T> m90<T>(T? p) async* {yield p!;} // LINT
class C<T> {
late T t;
m(T? p) {
Expand Down
7 changes: 7 additions & 0 deletions test_data/rules/experiments/nnbd/rules/unnecessary_null_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,10 @@ f5(int? p) {
v1 ??= p!; // OK
}

Future<int?> f6(int? p) async => await p!; // LINT
List<int?> f7(int? p) => [p!]; // LINT
Set<int?> f8(int? p) => {p!}; // LINT
Map<int?, String> f9(int? p) => {p!: ''}; // LINT
Map<String, int?> f10(int? p) => {'': p!}; // LINT
Iterable<int?> f11(int? p) sync* {yield p!;} // LINT
Stream<int?> f12(int? p) async* {yield p!;} // LINT