Skip to content

Constrain in_place_type construction of sum and choice on the argument list#296

Merged
Bronek merged 3 commits into
mainfrom
bronek/P2/in_place_type_constraints
Jul 13, 2026
Merged

Constrain in_place_type construction of sum and choice on the argument list#296
Bronek merged 3 commits into
mainfrom
bronek/P2/in_place_type_constraints

Conversation

@Bronek

@Bronek Bronek commented Jul 13, 2026

Copy link
Copy Markdown
Member

Three interdependent fixes to the question "can this element be constructed": the brace-initializability trait is introduced for sum and choice, adopted and renamed for pack, and pack's reference elements are bound to match.

Constrain in_place_type construction of sum and choice on the argument list

The constructors checked only has_type<T>, so any argument list was reported viable and a bad one then failed to compile inside variadic_union, outside the immediate context. They now also require that the alternative can be constructed from the arguments, as pack::append already did; choice's constructor forwards to sum's and shared the gap.

The conjunct is brace-initializability, not is_constructible_v: the storage constructs the element as T{args...}, which elides braces for an aggregate (std::array<int, 3> is not is_constructible_v from three ints, yet its forwarding construction is supported and tested) and rejects narrowing.

as_sum gets the same conjunct on its in_place_type overload, and the guard as_pack already carried on its value lift - without it, a tag whose type is not default-constructible fell through to the value overload and was wrapped as an alternative.

Ask of a pack element the same question its initialization answers

append constrained on is_constructible_v while _append brace-initializes the element, so an aggregate could not be appended element-wise (std::array<int, 3> is not is_constructible_v from three ints) though sum constructs exactly that, and a narrowing argument list was admitted for the body to reject. Both now use the constraint introduced for sum, which asks about the initialization actually performed.

That constraint grows a reference leg in the process: a reference element is bound, not brace-initialized, and T{...} for a reference T is not a portable question - gcc rejects it, clang accepts - so it must be spelled as the binding it is. Renamed _brace_constructible to _initializable accordingly.

Bind a reference pack element instead of brace-initializing it

_append built every element as T{FWD(args)...}. For a reference T that is a bind ([expr.type.conv]/1.3 defines it as direct-initializing an invented variable of type T, and [dcl.init.list]/3.9 initializes a reference from a single element whose type is reference-related to it), but gcc reads the braced functional-cast form as materializing a temporary and rejects the binding - while accepting the equivalent declaration. So appending an lvalue, which deduces a reference element, did not compile on gcc at all.

It went unnoticed because every test of such an append only took decltype(...) of the call, never instantiating the body. The new tests evaluate it, and assert the element binds - address identity, and mutation observed through the pack - with a constexpr twin.

Closes #284
Closes #287


Stack: P2 of 13 — the release-0.1 bugfix queue, merging bottom-up into main. Based on #295 and to be retargeted to main once it merges; the diff shows only this PR's commits. Every stack boundary was validated with a gcc build and the full test suite on top of main, and the aggregate branch ran the full CI matrix green (#289).

@augmentcode

augmentcode Bot commented Jul 13, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: This PR makes std::in_place_type-based construction for sum and choice fail/participate in overload resolution based on whether the selected alternative can actually be initialized from the provided arguments.

Changes:

  • Introduces fn::detail::_initializable to model the library’s brace-initialization semantics (aggregate brace elision + narrowing rejection) and reference binding behavior.
  • Applies _initializable to sum/choice in_place_type constructors and to as_sum(in_place_type, ...).
  • Switches pack::append(in_place_type, ...) and pack_impl::_append constraints from is_constructible_v to _initializable to match the actual initialization performed.
  • Adds detail::_make_element so reference elements are bound via a cast-expression on GCC (avoiding brace-form binding pitfalls).
  • Adds targeted tests covering constraint behavior, aggregate element-wise initialization, and runtime/constexpr reference binding.

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

@Bronek Bronek changed the base branch from bronek/P1/pack_append_partial_spec to main July 13, 2026 11:04
Bronek added 3 commits July 13, 2026 12:10
…rgument list

The constructors checked only `has_type<T>`, so any argument list was reported
viable and a bad one then failed to compile inside `variadic_union`, outside the
immediate context. They now also require that the alternative can be constructed
from the arguments, as `pack::append` already did; `choice`'s constructor
forwards to `sum`'s and shared the gap.

The conjunct is brace-initializability, not `is_constructible_v`: the storage
constructs the element as `T{args...}`, which elides braces for an aggregate
(`std::array<int, 3>` is not `is_constructible_v` from three ints, yet its
forwarding construction is supported and tested) and rejects narrowing.

`as_sum` gets the same conjunct on its `in_place_type` overload, and the guard
`as_pack` already carried on its value lift - without it, a tag whose type is not
default-constructible fell through to the value overload and was wrapped as an
alternative.

Closes #284

Assisted-by: Claude:claude-opus-4-8[1m]
`append` constrained on `is_constructible_v` while `_append` brace-initializes the
element, so an aggregate could not be appended element-wise (`std::array<int, 3>`
is not `is_constructible_v` from three ints) though `sum` constructs exactly that,
and a narrowing argument list was admitted for the body to reject. Both now use the
constraint introduced for `sum`, which asks about the initialization actually
performed.

That constraint grows a reference leg in the process: a reference element is bound,
not brace-initialized, and `T{...}` for a reference `T` is not a portable question -
gcc rejects it, clang accepts - so it must be spelled as the binding it is. Renamed
`_brace_constructible` to `_initializable` accordingly.

Assisted-by: Claude:claude-opus-4-8[1m]
`_append` built every element as `T{FWD(args)...}`. For a reference `T` that is a
bind ([expr.type.conv]/1.3 defines it as direct-initializing an invented variable
of type T, and [dcl.init.list]/3.9 initializes a reference from a single element
whose type is reference-related to it), but gcc reads the braced functional-cast
form as materializing a temporary and rejects the binding - while accepting the
equivalent declaration. So appending an lvalue, which deduces a reference element,
did not compile on gcc at all.

It went unnoticed because every test of such an append only took `decltype(...)` of
the call, never instantiating the body. The new tests evaluate it, and assert the
element binds - address identity, and mutation observed through the pack - with a
constexpr twin.

Closes #287

Assisted-by: Claude:claude-opus-4-8[1m]
@Bronek Bronek force-pushed the bronek/P2/in_place_type_constraints branch from ad37ed3 to a8019a3 Compare July 13, 2026 11:10
@Bronek

Bronek commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

augment review

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
66.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed. 1 suggestion posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread include/fn/detail/traits.hpp
@Bronek

Bronek commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

augment review

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

@Bronek Bronek merged commit cc59add into main Jul 13, 2026
63 of 64 checks passed
Bronek added a commit that referenced this pull request Jul 13, 2026
Take #296's corrected expectations for sum, choice and pack. The
in_place_type constraints flip #284's tripwires, and the initializability
conjunct brings probes this suite could not state before: aggregate
brace-init, narrowing rejection, and reference elements bound rather than
brace-initialized. Main's new blocks land as SECTIONs, keeping the suite
free of the BDD macros. The #280 noexcept tripwires stay - that issue is
still open.

Assisted-by: Claude:claude-opus-4-8[1m]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant