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.

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

Avoid completing a non-nullable Completer with a nullable value #3328

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

srawlins
Copy link
Contributor

@srawlins srawlins commented Dec 8, 2023

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

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • 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.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@srawlins
Copy link
Contributor Author

Ping for a review; I believe the test failures are unrelated.

@srawlins
Copy link
Contributor Author

Is there any way to force merge past the existing breakage? I don't have the right authority, maybe?

@keyonghan
Copy link
Contributor

keyonghan commented Dec 14, 2023

Is there any way to force merge past the existing breakage? I don't have the right authority, maybe?

Unfortunately, cocoon is not allowed with a force merge.

Seems some issue with some package versions: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8761714141529130593/+/u/dashboard/stdout.

@srawlins
Copy link
Contributor Author

I can also disable the cocooon flutter customer tests until cocoon is back in a good state.

@CaseyHillers
Copy link
Contributor

@srawlins the issue is fixed at head (based on https://flutter-dashboard.appspot.com/#/build?repo=cocoon&branch=main). Can you rebase your branch?

@keyonghan can we enable the "update branch" GitHub UI for Cocoon?

@keyonghan
Copy link
Contributor

That should be helpful. Filed flutter/flutter#140167 to track.

@srawlins
Copy link
Contributor Author

Thanks everyone! Let's merge! 😁

@keyonghan keyonghan added the autosubmit Merge PR when tree becomes green via auto submit App. label Dec 14, 2023
@auto-submit auto-submit bot merged commit ea2ea7a into main Dec 14, 2023
3 checks passed
@srawlins srawlins deleted the nullable-type branch December 14, 2023 21:28
@keyonghan
Copy link
Contributor

Same as other repos, we use autosubmit label to auto land. Added the label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flutter repo completes a non-nullable Completer with a nullable value
3 participants