8000 Mark `IconData` `final` and `@mustBeConst` by dcharkes · Pull Request #181345 · flutter/flutter · GitHub
[go: up one dir, main page]
Skip to content

Mark IconData final and @mustBeConst#18134 8000 5

Draft
dcharkes wants to merge 1 commit intomasterfrom
icon-data-class-modifier
Draft

Mark IconData final and @mustBeConst#181345
dcharkes wants to merge 1 commit intomasterfrom
icon-data-class-modifier

Conversation

@dcharkes
Copy link
Contributor
@dcharkes dcharkes commented Jan 22, 2026 •
edited
Loading

Proof of concept for:

See the issue descriptions for the motivation.

cc @Piinks

It seems we have a non-const use in the Remote Flutter Widgets:

https://github.com/flutter/packages/blob/d0c7d1fad77331724882614345137221c77d5758/packages/rfw/lib/src/flutter/argument_decoders.dart#L801-L811

The package itself makes no mention of it not work with the Flutter Icon Tree Shaker. If it's a know issue that the icon tree shaker is not supported with package:rfw we can add // ignore: non_const_argument_for_const_parameter. (flutter/packages#10858)

2026-03-10 update, no push-back on the breaking changes. Migrations done / in progress downstream:

@flutter-dashboard
Copy link
flutter-dashboard bot commented Jan 22, 2026

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jan 22, 2026
gemini-code-assist[bot]
gemini-code-assist bot reviewed Jan 22, 2026
View reviewed changes
Copy link
Contributor
@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request makes IconData a final class and adds @mustBeConst to its constructor parameters, which is a great improvement for enforcing immutability and enabling compiler optimizations. My review includes a suggestion to apply the @mustBeConst annotation to all constructor parameters for completeness and consistency.

@dcharkes dcharkes marked this pull request as draft January 22, 2026 21:06
@flutter-dashboard
Copy link
flutter-dashboard bot commented Jan 22, 2026

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

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.

dcharkes added a commit to flutter/packages that referenced this pull request Jan 22, 2026
@dcharkes
[rfw] Workaround IconData parameters must be const
501f4cc 
A patch for:

* flutter/flutter#181345
@dcharkes dcharkes mentioned this pull request Jan 22, 2026
Draft
justinmc
justinmc reviewed Jan 22, 2026
View reviewed changes
Copy link
Contributor
@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

Seems like a win to me. I want to be sure that Material and Cupertino don't have any breaking usages of IconData though (doesn't look like it to me if I grep through the codebase).

I haven't worked on the rfw codebase, but taking a look it seems like it won't be possible for it to support icon tree shaking. That makes sense for a package that can execute code at runtime that is unknown at compile time, so I expect users of rfw will understand.

@loic-sharma
Copy link
Member
loic-sharma commented Jan 22, 2026 •
edited
Loading

FYI, some folks use the enum Foo implements IconData pattern, which I believe will be broken if we make IconData final. Some examples:

  1. https://github.com/FaFre/WebLibre/blob/82ea06d0ebd9fe72438dcbab63a2234b9314a2dd/app/lib/presentation/icons/tor_icons.dart#L22
  2. https://github.com/PiotrRogulski/advent_of_code/blob/acf963c651a4b83bdd14fc660fa96b35f85d6b9a/lib/design_system/icons.dart#L7
  3. Icon tree shaking ignores IconData implementation getters #126261

This comment explains why folks use this pattern: #126261 (comment)

One of the major if not the biggest value from keeping such values as an enum for me is the values property provided by the language itself. I use values in widgetbook; keeping such list manually is error-prone

However, it seems that this improvement is nice enough to warrant breaking breaking this pattern.

@dcharkes
Copy link
Contributor Author
dcharkes commented Jan 23, 2026

One of the major if not the biggest value from keeping such values as an enum for me is the values property provided by the language itself. I use values in widgetbook; keeping such list manually is error-prone

@Albert221 You rely on subtyping IconData to get an automated .values. Could you elaborate on how .values is used in the code? My understanding of the tree-shaking mechanism is if .values is used, then all the codePoints are reachable, which means you'll never get any code points tree-shaken from the fonts.

@Albert221
Copy link
Contributor
Albert221 commented Jan 23, 2026 •
edited
Loading

@dcharkes Hey! That is correct, they're not tree-shaken at all if I use .values. But I use it in my Widgetbook instance, where I want my developers and designers to have access to the whole library of available icons. The widgetbook is a separate package, so it doesn't hurt me. In the mobile app itself, I don't use .values at all for icons to have the tree-shaking :)

Setup we use for many projects and that many clients use in their codebases usually is:

  • app_package (depends on app_design_system)
  • app_design_system my custom icon datas
  • design_system_widgetbook (depends on app_design_system) where devs/designers can preview all available components, colors, icons, etc.

@dcharkes
Copy link
Contributor Author
dcharkes commented Jan 23, 2026 •
edited
Loading

Thanks for the reply @Albert221 🙏

Okay, then the tree-shaking is WAI in your use case. I suppose asking you to migrate away from enum ... implements IconData is asking you to break all your users? Would it be possible to migrate to the same structure as the other icon sets, an unrelated class around it with static const members and a code-genned(?) values const.

@Albert221
Copy link
Contributor
Albert221 commented Jan 23, 2026

Not ideal but possible, yes. If that's for the sake of making Flutter better for everyone, go for it

@dcharkes
Copy link
Contributor Author
dcharkes commented Jan 23, 2026

Not ideal but possible, yes. If that's for the sake of making Flutter better for everyone, go for it

🙏 👍 !

Okay, when we do this as a breaking change, then older versions of WidgetBook will stop working in a newer Flutter version. Please let me know when you have migrated, and if the migration didn't caused breaking change issues on your side (looking at the code I don't think so, but I might be wrong). I'll wait with landing anything on master for a while (and need to file a breaking change announcement as well) to ensure no one breaks.

@dcharkes dcharkes mentioned this pull request Jan 28, 2026
Draft
This was referenced Mar 10, 2026
Merged
Open
@dcharkes dcharkes force-pushed the icon-data-class-modifier branch from 6c1c25c to 63b8163 Compare March 10, 2026 08:43
dcharkes added a commit to flutter/packages that referenced this pull request Mar 11, 2026
@dcharkes
[rfw] Opt out of icon tree shaking (#11216)
a6542ce 
We are introducing the `@mustBeConst` annotation on `IconData`
parameters that must be const in order for the icon tree shaker to work:

* flutter/flutter#181344
* flutter/flutter#181345

RFW does not support tree-shaking.

This PR adds the lint ignores with an explicit comment that the lint
ignore is intentional.
@dcharkes dcharkes force-pushed the icon-data-class-modifier branch from 63b8163 to 84a1207 Compare March 12, 2026 15:42
@dcharkes dcharkes added the CICD Run CI/CD label Mar 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinmc justinmc justinmc left review comments

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

CICD Run CI/CD framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dcharkes @loic-sharma @Albert221 @justinmc
0