Analyzer: report an error when `Future<nn-type>.value()` or `Completer<nn-type>.complete()` called with nullable value #53253

srawlins · 2023-08-17T15:53:00Z

In #45319 @simolus3 introduced an analyzer Warning that reported when a null value is (implicitly or explicitly) given as an argument to Future.value() or Completer.complete() and the type argument on the Future constructor or Completer instance is non-nullable.

At the end of the conversation he wrote:

I can change it to handle "nullable argument, non-nullable type" too but I'm not sure if it should be a default diagnostic if it can have false positives? In particular, it would warn about Future.value(x as dynamic) then.

I think your decision is sound. Later on, we can trigger on nullable arguments either in the same Hint, or with a new Hint (with a different message).

Thinking about this again, 2 years later, I think we should still report the "nullable argument" case (like int? x; Future<int>.value(x);), and I think we should report it as a default enabled Warning. With the following semantics, I think there are no false positives that diverge from the standard assignability rules, acting as if the parameter was typed FutureOr<T> rather than FutureOr<T>?:

Given a call to Future<T>.value (constructor) or Completer<T>.complete (method), where T is non-nullable:
- If the argument type is dynamic, do not report anything.
- If the argument type is potentially nullable, report a Warning.

The text was updated successfully, but these errors were encountered:

In these two cases, a nullable value is passed where a runtime error will occur if the value is null. Make runtime check explicit. Cleanup for #53253 Change-Id: Ia14bd345cbe20c50c75bdd2069f44449157bcc36 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/321423 Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Samuel Rawlins <[email protected]> Reviewed-by: Paul Berry <[email protected]>

…n-type>.completer. This is cleanup work required to start enforcing this with static analysis, as per dart-lang/sdk#53253. Real quick this issue is that this code is unsafe: ```dart void f(Completer<int> c, int? i) { Future<int>.value(i); // Ouch! c.complete(i); // Ouch! } ``` The typical fix is to add a null-assert (`!`), but sometimes a more appropriate or safer fix can be made.

…n-type>.completer (#6228) * Stop passing a nullable value to Future<nn-type>.value or Completer<nn-type>.completer. This is cleanup work required to start enforcing this with static analysis, as per dart-lang/sdk#53253. Real quick this issue is that this code is unsafe: ```dart void f(Completer<int> c, int? i) { Future<int>.value(i); // Ouch! c.complete(i); // Ouch! } ``` The typical fix is to add a null-assert (`!`), but sometimes a more appropriate or safer fix can be made. * Use non-nullable CpuSamples

…nn-type>.completer. This is cleanup work required to start enforcing this with static analysis, as per dart-lang/sdk#53253. Real quick this issue is that this code is unsafe: ```dart void f(Completer<int> c, int? i) { Future<int>.value(i); // Ouch! c.complete(i); // Ouch! } ``` This change should be a no-op. Adding this explicit null-assert ensures that any exception is thrown right at the null-assert.

This is cleanup work required to start enforcing this with static analysis, as per dart-lang/sdk#53253. Real quick this issue is that this code is unsafe: ```dart void f(Completer<int> c, int? i) { Future<int>.value(i); // Ouch! c.complete(i); // Ouch! } ```

This is cleanup work required to start enforcing this with static analysis, as per dart-lang/sdk#53253. Real quick this issue is that this code is unsafe: ```dart void f(Completer<int> c, int? i) { Future<int>.value(i); // Ouch! c.complete(i); // Ouch! } ``` - Thanks for your contribution! Please replace this text with a description of what this PR is changing or adding and why, list any relevant issues, and review the contribution guidelines below. --- <details> <summary>Contribution guidelines:</summary><br> - See our [contributor guide](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md) for general expectations for PRs. - Larger or significant changes should be discussed in an issue before creating a PR. - Contributions to our repos should follow the [Dart style guide](https://dart.dev/guides/language/effective-dart) and use `dart format`. - Most changes should add an entry to the changelog and may need to [rev the pubspec package version](https://github.com/dart-lang/sdk/wiki/External-Package-Maintenance#making-a-change). - Changes to packages require [corresponding tests](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md#Testing). Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback. </details>

…nn-type>.completer. This is cleanup work required to start enforcing this with static analysis, as per dart-lang/sdk#53253. Real quick this issue is that this code is unsafe: ```dart void f(Completer<int> c, int? i) { Future<int>.value(i); // Ouch! c.complete(i); // Ouch! } ``` This change should be a no-op. Adding this explicit null-assert ensures that any exception is thrown right at the null-assert.

This is cleanup work required to start enforcing this with static analysis, as per #53253. Real quick this issue is that this code is unsafe: ```dart void f(Completer<int> c, int? i) { Future<int>.value(i); // Ouch! c.complete(i); // Ouch! } ``` Change-Id: I91d7be0a16e62e78b530a70f115117d379259b9a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/324541 Reviewed-by: Liam Appelbe <[email protected]> Auto-Submit: Samuel Rawlins <[email protected]> Commit-Queue: Liam Appelbe <[email protected]>

This is cleanup work required to start enforcing this with static analysis, as per #53253. Real quick this issue is that this code is unsafe: ```dart void f(Completer<int> c, int? i) { Future<int>.value(i); // Ouch! c.complete(i); // Ouch! } ``` Change-Id: I12c4f0a92a2071fb47759d9626689e00cfa42f8d Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/324763 Reviewed-by: Konstantin Shcheglov <[email protected]> Auto-Submit: Samuel Rawlins <[email protected]> Commit-Queue: Samuel Rawlins <[email protected]> Commit-Queue: Konstantin Shcheglov <[email protected]>

This is cleanup work required to start enforcing this with static analysis, as per #53253. Real quick this issue is that this code is unsafe: ```dart void f(Completer<int> c, int? i) { Future<int>.value(i); // Ouch! c.complete(i); // Ouch! } ``` Change-Id: Ia4d83719c425601b24e8ae6e305c88c95cba8b20 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/324640 Auto-Submit: Samuel Rawlins <[email protected]> Commit-Queue: Liam Appelbe <[email protected]> Reviewed-by: Liam Appelbe <[email protected]>

This is cleanup work required to start enforcing this with static analysis, as per dart-lang/sdk#53253. Real quick this issue is that this code is unsafe: ```dart void f(Completer<int> c, int? i) { Future<int>.value(i); // Ouch! c.complete(i); // Ouch! } ```

There are two spots where a nullable value is passed to a non-nullable Future.value call, or a non-nullable Completer.complete call. In the interest of not making any functional change, I'm just ignoring this new code. ``` warning - lib/src/service/object.dart:870:29 - 'Future<Isolate>.value' shouldn't be called with an argument of nullable type 'Isolate?'. Try passing a non-null argument. - nullable_argument_to_non_null_type warning - lib/src/service/object.dart:1559:29 - 'Future<Class>.value' shouldn't be called with an argument of nullable type 'Class?'. Try passing a non-null argument. - nullable_argument_to_non_null_type ``` Related to #53253 TEST=N/A Change-Id: I091990975a1f2b927e007564b6ca0658c6a34c20 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/324762 Reviewed-by: Ben Konyi <[email protected]> Auto-Submit: Samuel Rawlins <[email protected]> Commit-Queue: Samuel Rawlins <[email protected]>

This reverts commit d3124b8. Reason for revert: unknown code for now. Original change's description: > observatory: Ignore new static Warning > > There are two spots where a nullable value is passed to a non-nullable > Future.value call, or a non-nullable Completer.complete call. In the > interest of not making any functional change, I'm just ignoring this > new code. > > ``` > warning - lib/src/service/object.dart:870:29 - 'Future<Isolate>.value' shouldn't be called with an argument of nullable type 'Isolate?'. Try passing a non-null argument. - nullable_argument_to_non_null_type > warning - lib/src/service/object.dart:1559:29 - 'Future<Class>.value' shouldn't be called with an argument of nullable type 'Class?'. Try passing a non-null argument. - nullable_argument_to_non_null_type > ``` > > Related to #53253 > > TEST=N/A > > Change-Id: I091990975a1f2b927e007564b6ca0658c6a34c20 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/324762 > Reviewed-by: Ben Konyi <[email protected]> > Auto-Submit: Samuel Rawlins <[email protected]> > Commit-Queue: Samuel Rawlins <[email protected]> Change-Id: Id5d033ca8601ea2f18aa9d2c8f8aeee8a2ceaf0d No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/326725 Commit-Queue: Alexander Thomas <[email protected]> Auto-Submit: Samuel Rawlins <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]> Reviewed-by: Alexander Thomas <[email protected]> Bot-Commit: Rubber Stamper <[email protected]> Reviewed-by: Samuel Rawlins <[email protected]>

…#136776) The code with out the null assertion is legal as per the type signature, but will throw a runtime exception if the nullable value is null. To make this exception more explicit, the value must be null-checked before completing the completer with the value. The analyzer will soon enforce such checks. See dart-lang/sdk#53253. Fixes #136775

…#137359) Also ignore one case where it is hard to fix. The code with out the null assertion is legal as per the type signature, but will throw a runtime exception if the nullable value is null. To make this exception more explicit, the value must be null-checked before completing the completer with the value. The analyzer will soon enforce such checks. See dart-lang/sdk#53253. Fixes #137294 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

Implicitly passing a nullable value to a Completer of a non-nullable type can lead to surprising null-asserting exceptions. See dart-lang/sdk#53253 for more details. In this PR, we add an explicit null-assertion, which makes this call in-line with the general concepts of null safety. Fixes flutter/flutter#137294

The code with out the null assertion is legal as per the type signature, but will throw a runtime exception if the nullable value is null. To make this exception more explicit, the value must be null-checked before completing the completer with the value. The analyzer will soon enforce such checks. See dart-lang/sdk#53253. This PR is behaviorally a no-op. Fixes flutter/flutter#136775

…#49053) The code with out the null assertion is legal as per the type signature, but will throw a runtime exception if the nullable value is null. To make this exception more explicit, the value must be null-checked before completing the completer with the value. The analyzer will soon enforce such checks. See dart-lang/sdk#53253. This PR is behaviorally a no-op. Fixes flutter/flutter#136775 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

This was referenced Aug 18, 2023

fix: Stop passing nullable values to Future<nn-type>.value firebase/flutterfire#11495

Closed

Stop passing a nullable value to Completer<nn-type>.completer. ryanheise/just_audio#1043

Merged

srawlins mentioned this issue Aug 21, 2023

Stop passing a nullable value to Future<nn-type>.value or Completer<nn-type>.completer flutter/devtools#6228

Merged

7 tasks

This was referenced Aug 23, 2023

Analyzer and CFE disagree on inferred Future.value type with nullable argument dart-lang/language#3304

Closed

CFE: Correct inference of Future.value type, without a context type #53371

Open

srawlins mentioned this issue Aug 31, 2023

Avoid passing a nullable value to Future<nn-type>.value or Completer<nn-type>.completer. dart-lang/webdev#2205

Merged

1 task

srawlins mentioned this issue Aug 31, 2023

Avoid passing a nullable value to Future<nn-type>.value dart-lang/build#3567

Merged

1 task

srawlins mentioned this issue Aug 31, 2023

Avoid passing a nullable value to Completer<nn-type>.complete dart-lang/http#1015

Merged

1 task

srawlins mentioned this issue Sep 25, 2023

Use type argument for Future of nullable value in test dart-lang/source_gen#681

Merged

1 task

This was referenced Oct 18, 2023

FlutterVmService completes a non-nullable Completer with a nullable value flutter/flutter#136775

Closed

Null-assert the value given to a Completer expecting a non-null value flutter/flutter#136776

Merged

srawlins mentioned this issue Dec 14, 2023

Null-assert the value given to a Completer expecting a non-null value flutter/engine#49053

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Analyzer: report an error when `Future<nn-type>.value()` or `Completer<nn-type>.complete()` called with nullable value #53253

Analyzer: report an error when `Future<nn-type>.value()` or `Completer<nn-type>.complete()` called with nullable value #53253

srawlins commented Aug 17, 2023

Analyzer: report an error when Future<nn-type>.value() or Completer<nn-type>.complete() called with nullable value #53253

Analyzer: report an error when Future<nn-type>.value() or Completer<nn-type>.complete() called with nullable value #53253

Comments

srawlins commented Aug 17, 2023

Analyzer: report an error when `Future<nn-type>.value()` or `Completer<nn-type>.complete()` called with nullable value #53253

Analyzer: report an error when `Future<nn-type>.value()` or `Completer<nn-type>.complete()` called with nullable value #53253