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

@@ -18,7 +18,7 @@ struct UniqueEngineTraits {

  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

@@ -18,7 +18,7 @@ struct UniqueEngineTraits {

  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.

chinmaygarde requested review from jason-simmons, gaaclarke and flar 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Define a concept for UniqueObjectTraits. #174905

Define a concept for UniqueObjectTraits. #174905

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

Uh oh!

gemini-code-assist bot Sep 3, 2025

Uh oh!

chinmaygarde Sep 3, 2025

Uh oh!

chinmaygarde Sep 3, 2025

Uh oh!

flar Sep 3, 2025

Uh oh!

chinmaygarde Sep 3, 2025

Uh oh!

gaaclarke left a comment

Uh oh!

chinmaygarde commented Sep 3, 2025

Uh oh!

flar Sep 3, 2025

Uh oh!

auto-submit bot commented Sep 3, 2025

Uh oh!

flar left a comment

Uh oh!

Uh oh!

		@@ -18,7 +18,7 @@ struct UniqueEngineTraits {

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

		static void Free(FlutterEngine& engine) {

Define a concept for UniqueObjectTraits. #174905

Are you sure you want to change the base?

Define a concept for UniqueObjectTraits. #174905

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!