Prepare for `NULLABLE_ARGUMENT_TO_NON_NULL_TYPE` #121321

asashour · 2023-02-23T09:50:51Z

#121318

This PR modifies the code in order not to trigger the incoming diagnostic.

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 and followed the Flutter Style Guide, including Features we expect every widget to implement.
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.

flutter#121318

christopherfujino · 2023-02-23T17:21:55Z

packages/flutter_tools/lib/src/vmservice.dart

          && event.extensionRPC == extensionName) {
        isolateEvents.cancel();
-        extensionAdded.complete(event.isolate);
+        extensionAdded.complete(event.isolate!);


can we add && event.isolate != null to the if comparison on lines 935-936?

or, maybe another conditional here? i'm not sure what to do here, but also not confident we won't hit a null check error here. @bkonyi WDYT?

This should never be null for a ServiceExtensionAdded event.

From what I know, if there is a null passed during runtime, an exception would be thrown. That's the main motivation for dart-lang/sdk#51367

So, the previous code never sent a null.

We get a lot of TypeErrors at runtime :)

However, this SGTM per Ben's comment

christopherfujino · 2023-03-03T01:43:49Z

Per https://github.com/flutter/flutter/wiki/Tree-hygiene#tests, I think you'll either need to add tests or request an exemption (I think an exemption would be reasonable here, but it's ultimately not my call)

asashour · 2023-03-09T20:40:22Z

I am under the impression that the exemption is not needed, as long as the bot doesn't ask for tests.

Anyway, a test exemption was requested.

christopherfujino · 2023-03-09T20:43:34Z

I am under the impression that the exemption is not needed, as long as the bot doesn't ask for tests.

Anyway, a test exemption was requested.

Ahh, good point, you're right, my bad!

christopherfujino

LGTM

auto-submit · 2023-03-09T20:45:29Z

auto label is removed for flutter/flutter, pr: 121321, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

auto-submit · 2023-03-09T20:45:30Z

auto label is removed for flutter/flutter, pr: 121321, due to Validations Fail.

CaseyHillers · 2023-03-09T22:16:28Z

test-exempt: Analysis fix before landing the lint in Dart

andrewkolos

LGTM

christopherfujino · 2023-04-06T22:16:12Z

packages/flutter/lib/src/material/scaffold.dart

        _dismissedBottomSheets.add(bottomSheet);
      }
-      completer.complete();
+      completer.complete(null as T);


Wait a minute, is this a safe cast to make?

cc @goderbauer

This doesn't look right. Is it just hiding a typing error elsewhere?

Please have a look at this particular dart-lang/sdk#51367 (comment) and dart-lang/sdk#51367 (comment)

The current Flutter usage is actually null.

My understanding from reading those comments is that the old code is unsafe (and that there may soon be a warning about this). This change silences that warning, but the code still remains unsafe: There is no guarantee that T is actually nullable, so this may still throw at runtime.

So, instead of just making sure the warning doesn't show up, we should actually fix the unsafe code here.

auto-submit · 2023-04-06T22:17:07Z

auto label is removed for flutter/flutter, pr: 121321, due to This PR has not met approval requirements for merging. Changes were requested by {christopherfujino}, please make the needed changes and resubmit this PR.
You have project association CONTRIBUTOR and need 1 more review(s) in order to merge this PR.

Merge guidelines: You need at least one approved review if you are already a MEMBER or two member reviews if you are not a MEMBER before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

auto-submit · 2023-04-06T22:17:08Z

auto label is removed for flutter/flutter, pr: 121321, due to - This pull request has changes requested by @christopherfujino. Please resolve those before re-applying the label.

goderbauer · 2023-04-07T16:26:26Z

packages/flutter/lib/src/material/scaffold.dart

    final AnimationController controller = (transitionAnimationController ?? BottomSheet.createAnimationController(this))..forward();
    setState(() {
-      _currentBottomSheet = _buildBottomSheet<T>(
+      _currentBottomSheet = _buildBottomSheet(


This is now guaranteed to return a PersistentBottomSheetController. Will that not cause trouble for the cast to PersistentBottomSheetController<T> below in line 2464 if T is not void?

TBH, I am not sure what T is supposed to represent here and unfortunately, it is not documented. However, we likely have to figure that out to fix the typing here. From looking at this briefly it seems like passing anything but void for T will cause trouble...

Maybe you were supposed to be able to pass back a return value when closing the bottom sheet (and T was the type for that), but from the looks of it maybe that got never fully implemented?

Sorry for late response.

The issue is that showBottomSheet expects a type T, and then the completer must return that a value of that type (if it is not void).

Yes, it seems that having T was meant, but never used.

I would think to either change the signature of showBottomSheet to not have a type (I guess this is a breaking change), or find a way to send a value if T is actually passed.

christopherfujino · 2023-04-25T20:25:29Z

@asashour are you still interested in working on this PR?

christopherfujino · 2023-05-18T20:48:35Z

will wait one more week, pinged @asashour in the discord.

christopherfujino · 2023-07-25T22:59:01Z

Closing as stale, feel free to re-open if you choose to pick this up again @asashour

Prepare for NULLABLE_ARGUMENT_TO_NON_NULL_TYPE

537073b

flutter#121318

asashour mentioned this pull request Feb 23, 2023

Update codebase for an incoming NULLABLE_ARGUMENT_TO_NON_NULL_TYPE #121318

Open

3 tasks

christopherfujino reviewed Feb 23, 2023

View reviewed changes

christopherfujino approved these changes Mar 9, 2023

View reviewed changes

christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 9, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 9, 2023

christopherfujino requested a review from andrewkolos March 9, 2023 21:01

andrewkolos approved these changes Mar 12, 2023

View reviewed changes

Merge branch 'master' into 121318_nullable_diagnostic

abbe39b

christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 6, 2023

christopherfujino suggested changes Apr 6, 2023

View reviewed changes

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 6, 2023

asashour added 3 commits April 7, 2023 01:30

Merge branch 'master' into 121318_nullable_diagnostic

06c4d7b

Remove Completer.complete

4357679

Change Completer type to void

4db8a93

goderbauer reviewed Apr 7, 2023

View reviewed changes

christopherfujino added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label May 4, 2023

Merge branch 'master' into 121318_nullable_diagnostic

868e783

github-actions bot removed c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels May 25, 2023

christopherfujino closed this Jul 25, 2023

Prepare for NULLABLE_ARGUMENT_TO_NON_NULL_TYPE #121321

Prepare for NULLABLE_ARGUMENT_TO_NON_NULL_TYPE #121321

Conversation

asashour commented Feb 23, 2023

Pre-launch Checklist

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!

asashour Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino commented Mar 3, 2023

Uh oh!

asashour commented Mar 9, 2023

Uh oh!

christopherfujino commented Mar 9, 2023

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Mar 9, 2023

Uh oh!

auto-submit bot commented Mar 9, 2023

Uh oh!

CaseyHillers commented Mar 9, 2023

Uh oh!

andrewkolos left a comment

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!

auto-submit bot commented Apr 6, 2023

Uh oh!

auto-submit bot commented Apr 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asashour May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino commented Apr 25, 2023

Uh oh!

christopherfujino commented May 18, 2023

Uh oh!

christopherfujino commented Jul 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Prepare for `NULLABLE_ARGUMENT_TO_NON_NULL_TYPE` #121321

Prepare for `NULLABLE_ARGUMENT_TO_NON_NULL_TYPE` #121321

asashour Feb 23, 2023 •

edited

Loading

christopherfujino Feb 23, 2023 •

edited

Loading

asashour May 25, 2023 •

edited

Loading