Skip to content

Make MultiChildRenderObjectWidget and its descendants const - #88359

Closed
spkersten wants to merge 1 commit into
flutter:masterfrom
spkersten:const-multi-child-render-object-widget
Closed

Make MultiChildRenderObjectWidget and its descendants const#88359
spkersten wants to merge 1 commit into
flutter:masterfrom
spkersten:const-multi-child-render-object-widget

Conversation

@spkersten

@spkersten spkersten commented Aug 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Relates to dart-lang/sdk#29276

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 feature I am adding, or Hixie said the 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-dashboard flutter-dashboard Bot added p: cupertino_ui cupertino_ui package in flutter/packages p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Aug 17, 2021
@google-cla google-cla Bot added the cla: yes label Aug 17, 2021
@spkersten
spkersten force-pushed the const-multi-child-render-object-widget branch from 396e46c to de7a34e Compare August 17, 2021 15:54
spkersten
spkersten commented Aug 17, 2021
View reviewed changes
Comment thread packages/flutter/test/widgets/list_body_test.dart Outdated

@spkersten spkersten Aug 17, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would have expected that const can on line 32, but that results in:

error: Evaluation of this constant expression throws an exception. (const_eval_throws_exception at [flutter] test/widgets/list_body_test.dart:49)

@spkersten
spkersten force-pushed the const-multi-child-render-object-widget branch from de7a34e to e96c309 Compare August 17, 2021 16:06
@flutter-dashboard flutter-dashboard Bot added a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) c: contributor-productivity Team-specific productivity, code health, technical debt. labels Aug 17, 2021
@spkersten
spkersten force-pushed the const-multi-child-render-object-widget branch from e96c309 to bba6858 Compare August 17, 2021 16:19
@goderbauer
goderbauer self-requested a review August 19, 2021 18:48
@spkersten

spkersten commented Sep 9, 2021

Copy link
Copy Markdown
Contributor Author

@goderbauer Since the PR has been open for a while, is this waiting for anything from me? (Besides the merge conflict.)

@goderbauer

goderbauer commented Sep 9, 2021

Copy link
Copy Markdown
Member

Sorry for the delay.

Is the reasoning here that we can remove the check because in an NNBD world List<Widget> children can never contain null? We've left these kind of null checks in the code base for now because not all apps have migrated to NNBD yet and they still need these asserts for proper error reporting.

@flutter-dashboard

flutter-dashboard Bot commented Oct 14, 2021

Copy link
Copy Markdown

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@spkersten spkersten closed this Oct 14, 2021
@nt4f04uNd nt4f04uNd mentioned this pull request Oct 21, 2021
Closed
@dnfield dnfield mentioned this pull request Oct 28, 2021
Closed
@gustav3d

gustav3d commented Nov 18, 2021

Copy link
Copy Markdown

Sorry for the delay.

Is the reasoning here that we can remove the check because in an NNBD world List<Widget> children can never contain null? We've left these kind of null checks in the code base for now because not all apps have migrated to NNBD yet and they still need these asserts for proper error reporting.

For how long will this be the case ?.
That some people dont fix their codebase to be NNBD but still expect to function in the otherwise evolving flutter versions is to prevent flutter itself to be pure NNBD.
Why not simple dictate that after version X, its pure NNBD, people who refusee to adapt are stuck on earlier version.

@darshankawar darshankawar mentioned this pull request Dec 14, 2021
Closed
@albertodev01

albertodev01 commented May 3, 2022

Copy link
Copy Markdown
Contributor

Is there any update on this? Having const available for rows, columns, stacks, containers and much more would be a huge lift

@albertodev01 albertodev01 mentioned this pull request Jul 24, 2022
Closed
@albertodev01

albertodev01 commented Jan 23, 2023

Copy link
Copy Markdown
Contributor

Done! See #118837

@gustav3d

gustav3d commented Jan 23, 2023

Copy link
Copy Markdown

Thanks ! today is a good day :)

@goderbauer

goderbauer commented Jan 23, 2023

Copy link
Copy Markdown
Member

It'll still requires a little more work before it is truly done, but we're on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goderbauer goderbauer Awaiting requested review from goderbauer

Assignees

No one assigned

Labels

a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. p: cupertino_ui cupertino_ui package in flutter/packages p: material_ui material_ui package in flutter/packages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@spkersten @goderbauer @gustav3d @albertodev01