Define a concept for UniqueObjectTraits. #174905

chinmaygarde · 2025-09-03T21:03:11Z

Earlier, it was just a comment. If you didn't do what the comment said, you'd get a mysterious compiler error when the template was instantiated at the point where the trait method was invoked. This would be extremely far away from where the template was instantiated and typically in fml/unique_object.h.

Now, the exact reason and where a fix would go is printed in the compiler error. For instance, if I delete the Free method in UniqueDirTraits, I get (among other output):

no member named 'Free' in 'fml::internal::os_unix::UniqueDirTraits'

flutter-dashboard · 2025-09-03T21:03:16Z

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.

chinmaygarde · 2025-09-03T21:03:56Z

There are other instances where concepts would have made compiler errors easier to reason about. But this one was by far my biggest bugbear and one I had run into numerous times.

gemini-code-assist

Code Review

This pull request introduces a C++20 concept, UniqueObjectTraits, to enforce the requirements for traits classes used with fml::UniqueObject. This is a great improvement as it will provide clearer compiler errors when the traits are not implemented correctly, as described in the pull request. The change in embedder_config_builder.h is a good fix that aligns with the new concept.

I have one suggestion to make the new concept even more robust by removing an implicit DefaultConstructible constraint on the object type T.

gemini-code-assist · 2025-09-03T21:04:58Z

engine/src/flutter/fml/unique_object.h

+concept UniqueObjectTraits = requires {
+  // |InvalidValue| should be fast and inline.
+  { Traits::InvalidValue() } -> std::same_as<T>;
+
+  // |IsValid| function should be fast and inline.
+  { Traits::IsValid(T{}) } -> std::same_as<bool>;
+
+  // |Free| function will not be called if value == InvalidValue()!
+  { Traits::Free(T{}) };
+};


The use of T{} in the requires expression for IsValid and Free introduces a requirement that T must be default-constructible. This might be too restrictive for a generic utility like UniqueObject, as some handle types T might not have a default constructor.

To make the concept more general and avoid this constraint, you can use std::declval<T>(). This allows checking the validity of the expressions without needing to construct an object of type T.

This change maintains the other constraints of the concept (like preventing Free from taking a non-const lvalue reference) while making it applicable to a wider range of types for T.

Suggested change

concept UniqueObjectTraits = requires {

// |InvalidValue| should be fast and inline.

{ Traits::InvalidValue() } -> std::same_as<T>;

// |IsValid| function should be fast and inline.

{ Traits::IsValid(T{}) } -> std::same_as<bool>;

// |Free| function will not be called if value == InvalidValue()!

{ Traits::Free(T{}) };

};

concept UniqueObjectTraits = requires {

// |InvalidValue| should be fast and inline.

{ Traits::InvalidValue() } -> std::same_as<T>;

// |IsValid| function should be fast and inline.

{ Traits::IsValid(std::declval<T>()) } -> std::same_as<bool>;

// |Free| function will not be called if value == InvalidValue()!

{ Traits::Free(std::declval<T>()) };

};

good bot gem gem

chinmaygarde · 2025-09-03T21:06:04Z

engine/src/flutter/shell/platform/embedder/tests/embedder_config_builder.h


  static bool IsValid(const FlutterEngine& value) { return value != nullptr; }

-  static void Free(FlutterEngine& engine) {


FlutterEngine is a pointer. This one was creating a reference to a pointer. The concept makes this check stricter. Though I am sure I could use std::convertible_to instead of std::same_as. But this one is an error so decided to keep the stricter version and fixup this callsite.

The same would be true of the IsValid method above this.

Why is that type defined as a pointer? Shouldn't we define the base type and make the pointers explicit at the point of use?

Hmm, you're right. IsValid could be static bool IsValid(FlutterEngine value) just fine. So why is Free an issue.

From my investigation, FlutterEngine& would make try to bind a non-const lvalue reference to a temporary of type pointer. According to the internet, this should be problematic. I am not sure why the current code works. It shouldn't right?

Instead of figuring out why the bad code is working, I'm going to vote for just fixing it.

gaaclarke

lgtm modulo gemini's suggestion to support non default constructible types.

chinmaygarde · 2025-09-03T21:15:36Z

Expect presub failures till #174891 lands. Will rebase on top.

Earlier, it was just a comment. If you didn't do what the comment said, you'd get a mysterious compiler error when the template was instantiated at the point where the trait method was invoked. This would be extremely far away from where the template was instantiated and typically in `fml/unique_object.h`. Now, the exact reason and where a fix would go is printed in the compiler error. For instance, if I delete the Free method in `UniqueDirTraits`, I get (among other output): ``` no member named 'Free' in 'fml::internal::os_unix::UniqueDirTraits' ```

flar · 2025-09-03T21:25:54Z

engine/src/flutter/shell/platform/embedder/tests/embedder_config_builder.h


  static bool IsValid(const FlutterEngine& value) { return value != nullptr; }

-  static void Free(FlutterEngine& engine) {


The same would be true of the IsValid method above this.

Why is that type defined as a pointer? Shouldn't we define the base type and make the pointers explicit at the point of use?

auto-submit · 2025-09-03T21:26:39Z

autosubmit label was removed for flutter/flutter/174905, because This PR has not met approval requirements for merging. Changes were requested by {flar}, please make the needed changes and resubmit this PR.
The PR author is a member of flutter-hackers and needs 0 more review(s) in order to merge this PR.

Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

flar

I've identified my concern. I'll approve and leave it to you to figure out what/whether to do about it.

…74905)

flutter/flutter@973320c...a082096 2025-09-09 [email protected] Roll Packages from 24588c6 to 2d651b2 (2 revisions) (flutter/flutter#175130) 2025-09-09 [email protected] Roll Skia from ab13fd19dd32 to 19ba56dde579 (2 revisions) (flutter/flutter#175127) 2025-09-09 [email protected] Roll Skia from 29a015f8712b to ab13fd19dd32 (2 revisions) (flutter/flutter#175108) 2025-09-09 [email protected] Roll Skia from 6a4613b83365 to 29a015f8712b (5 revisions) (flutter/flutter#175106) 2025-09-09 [email protected] Roll Dart SDK from 7b645442db0f to f446144fb7c9 (2 revisions) (flutter/flutter#175104) 2025-09-08 [email protected] Nav bar static components respect ambient MediaQueryData (flutter/flutter#174673) 2025-09-08 [email protected] fix typo in test documentation #2 (flutter/flutter#174707) 2025-09-08 [email protected] Update ImageReaderSurfaceProducer.MAX_IMAGES to include the maximum number of retained dequeued images (flutter/flutter#174971) 2025-09-08 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 5.0.0 to 6.0.1 in the all-github-actions group (flutter/flutter#175093) 2025-09-08 [email protected] Roll Skia from 25f00cb247f2 to 6a4613b83365 (3 revisions) (flutter/flutter#175087) 2025-09-08 [email protected] Roll Dart SDK from 83c6b6124380 to 7b645442db0f (1 revision) (flutter/flutter#175086) 2025-09-08 [email protected] Impeller: Convert GLProc name field and GLErrorToString to std::string_view (flutter/flutter#173771) 2025-09-08 [email protected] Depend on operator overload synthesis for three-way and equality comparisons. (flutter/flutter#174892) 2025-09-08 [email protected] Define a concept for UniqueObjectTraits. (flutter/flutter#174905) 2025-09-08 [email protected] Roll Skia from 0c2b0a00b7b5 to 25f00cb247f2 (1 revision) (flutter/flutter#175077) 2025-09-08 [email protected] Fix GitHub labeler platform-android typo (flutter/flutter#175076) 2025-09-08 [email protected] [Gradle 9] Removed `minSdkVersion` and only use `minSdk` (flutter/flutter#173892) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Earlier, it was just a comment. If you didn't do what the comment said, you'd get a mysterious compiler error when the template was instantiated at the point where the trait method was invoked. This would be extremely far away from where the template was instantiated and typically in `fml/unique_object.h`. Now, the exact reason and where a fix would go is printed in the compiler error. For instance, if I delete the Free method in `UniqueDirTraits`, I get (among other output): ``` no member named 'Free' in 'fml::internal::os_unix::UniqueDirTraits' ```

…r#9983) flutter/flutter@973320c...a082096 2025-09-09 [email protected] Roll Packages from 24588c6 to 2d651b2 (2 revisions) (flutter/flutter#175130) 2025-09-09 [email protected] Roll Skia from ab13fd19dd32 to 19ba56dde579 (2 revisions) (flutter/flutter#175127) 2025-09-09 [email protected] Roll Skia from 29a015f8712b to ab13fd19dd32 (2 revisions) (flutter/flutter#175108) 2025-09-09 [email protected] Roll Skia from 6a4613b83365 to 29a015f8712b (5 revisions) (flutter/flutter#175106) 2025-09-09 [email protected] Roll Dart SDK from 7b645442db0f to f446144fb7c9 (2 revisions) (flutter/flutter#175104) 2025-09-08 [email protected] Nav bar static components respect ambient MediaQueryData (flutter/flutter#174673) 2025-09-08 [email protected] fix typo in test documentation flutter#2 (flutter/flutter#174707) 2025-09-08 [email protected] Update ImageReaderSurfaceProducer.MAX_IMAGES to include the maximum number of retained dequeued images (flutter/flutter#174971) 2025-09-08 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 5.0.0 to 6.0.1 in the all-github-actions group (flutter/flutter#175093) 2025-09-08 [email protected] Roll Skia from 25f00cb247f2 to 6a4613b83365 (3 revisions) (flutter/flutter#175087) 2025-09-08 [email protected] Roll Dart SDK from 83c6b6124380 to 7b645442db0f (1 revision) (flutter/flutter#175086) 2025-09-08 [email protected] Impeller: Convert GLProc name field and GLErrorToString to std::string_view (flutter/flutter#173771) 2025-09-08 [email protected] Depend on operator overload synthesis for three-way and equality comparisons. (flutter/flutter#174892) 2025-09-08 [email protected] Define a concept for UniqueObjectTraits. (flutter/flutter#174905) 2025-09-08 [email protected] Roll Skia from 0c2b0a00b7b5 to 25f00cb247f2 (1 revision) (flutter/flutter#175077) 2025-09-08 [email protected] Fix GitHub labeler platform-android typo (flutter/flutter#175076) 2025-09-08 [email protected] [Gradle 9] Removed `minSdkVersion` and only use `minSdk` (flutter/flutter#173892) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…74905)

Earlier, it was just a comment. If you didn't do what the comment said, you'd get a mysterious compiler error when the template was instantiated at the point where the trait method was invoked. This would be extremely far away from where the template was instantiated and typically in `fml/unique_object.h`. Now, the exact reason and where a fix would go is printed in the compiler error. For instance, if I delete the Free method in `UniqueDirTraits`, I get (among other output): ``` no member named 'Free' in 'fml::internal::os_unix::UniqueDirTraits' ```

chinmaygarde requested review from flar, gaaclarke and jason-simmons September 3, 2025 21:03

github-actions bot added the engine flutter/engine related. See also e: labels. label Sep 3, 2025

gemini-code-assist bot reviewed Sep 3, 2025

View reviewed changes

chinmaygarde commented Sep 3, 2025

View reviewed changes

gaaclarke approved these changes Sep 3, 2025

View reviewed changes

chinmaygarde added 2 commits September 3, 2025 14:20

PR comments.

ab3cce1

chinmaygarde force-pushed the uniqueconcept branch from 63e1f7e to ab3cce1 Compare September 3, 2025 21:20

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 3, 2025

flar requested changes Sep 3, 2025

View reviewed changes

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 3, 2025

flar approved these changes Sep 3, 2025

View reviewed changes

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2025

auto-submit bot added this pull request to the merge queue Sep 8, 2025

Merged via the queue into flutter:master with commit f356a80 Sep 8, 2025
185 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2025

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2025

f356a8030 Define a concept for UniqueObjectTraits. (flutter/flutter#1…

3be6d23

…74905)

engine-flutter-autoroll mentioned this pull request Sep 9, 2025

Roll Flutter from 973320c0270b to 4277a18e139b (15 revisions) flutter/packages#9982

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2025

f356a8030 Define a concept for UniqueObjectTraits. (flutter/flutter#1…

9137fad

…74905)

engine-flutter-autoroll mentioned this pull request Sep 9, 2025

Roll Flutter from 973320c0270b to a0820964d59c (17 revisions) flutter/packages#9983

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2025

f356a8030 Define a concept for UniqueObjectTraits. (flutter/flutter#1…

d56d2d6

…74905)

engine-flutter-autoroll mentioned this pull request Sep 9, 2025

Roll Flutter from 973320c0270b to a0820964d59c (17 revisions) flutter/packages#9984

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2025

f356a8030 Define a concept for UniqueObjectTraits. (flutter/flutter#1…

2619cda

…74905)

engine-flutter-autoroll mentioned this pull request Sep 9, 2025

Roll Flutter from 973320c0270b to 53f8432312ed (18 revisions) flutter/packages#9985

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 12, 2025

f356a8030 Define a concept for UniqueObjectTraits. (flutter/flutter#1…

62d2869

…74905)


		static bool IsValid(const FlutterEngine& value) { return value != nullptr; }

		static void Free(FlutterEngine& engine) {

Define a concept for UniqueObjectTraits. #174905

Define a concept for UniqueObjectTraits. #174905

Uh oh!

Conversation

chinmaygarde commented Sep 3, 2025

Uh oh!

flutter-dashboard bot commented Sep 3, 2025

Uh oh!

chinmaygarde commented Sep 3, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

flar Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Sep 3, 2025

Uh oh!

flar Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Sep 3, 2025

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants