history_property: decouple instantiation from serializability via numsim_serialize ADL hook (fixes #16)

[petlenz]

What

history_property<T>::serialize/deserialize carried a static_assert(is_trivially_copyable_v<T>) inside the virtual bodies. Because a static_assert in a virtual is evaluated when the class specialization completes, history_property<T> was uninstantiable for any non-trivially-copyable T (e.g. tmech::tensor, which is copyable+assignable but has user-provided special members) even when serialize was never called — and it diverged clang-vs-gcc per [temp.inst]/11 (clang eagerly instantiates unused virtuals for the vtable; gcc may defer). This is numsim-core#16.

The fix replaces the static_assert with if constexpr dispatch:

• trivially-copyable T → raw bytes (unchanged fast path),
• non-trivial T with an ADL numsim_serialize/numsim_deserialize hook → call the hook,
• otherwise → throw std::runtime_error at call time only (never blocks instantiation).

Serializability is a precondition of the serialize operation, not of being held as history — so its absence is a runtime error, not a compile-time barrier.

Commits

1. The fix — if constexpr dispatch + the numsim_serialize/numsim_deserialize ADL customization point + has_numsim_serialize/has_numsim_deserialize detection traits.
2. Review-hardening (folded-in 3-lens review, see REVIEW-history-property.md) — closes silent-correctness holes the tests cannot catch by construction:
   • both-or-neither hook enforcement (a serialize-only type would write state it can never read back) — static_assert inside the taken if constexpr branch, so input_property<T>::wire instantiates history_property<T> triviality static_assert — breaks tmech tensors under clang-19 #16 stays fixed;
   • trivially-copyable-AND-hook ambiguity rejected loudly (the hook would otherwise be silently ignored);
   • stream-state checks on the raw read/write path (a truncated read no longer silently corrupts);
   • ODR contract documented (the hook must live in T's own header so every TU detects it identically);
   • void-return-constrained detection traits; #include <utility>; tightened test throw-message assertions; hardened clone() test.

Test plan

• Suite 62/62 under g++-14 and clang-18 (the new tests/history_property/ included).
• Both new compile-time guards sabotage-verified to fire with the intended diagnostics.
• history_property<tmech::tensor<double,3,2>> (explicit template class forcing all virtuals) compiles and runs under clang — the exact input_property<T>::wire instantiates history_property<T> triviality static_assert — breaks tmech tensors under clang-19 #16 failure, resolved.

Follow-up (not in this push)

A CI workflow (gcc-14 + clang-19 matrix) is ready locally but blocked on the pushing token's missing workflow scope; it will be added so the clang-specific regression is guarded automatically. Until then this repo has no CI, so the dual-compiler verification above was run by hand.

[petlenz]

Critical review — findings & resolutions

A 3-lens parallel review (architect-reviewer + cpp-pro + code-reviewer) was run on this branch and folded into commit 2 (REVIEW-history-property.md has the full write-up). The if constexpr rewrite itself was confirmed correct by both C++ lenses — discarded-branch semantics genuinely remove the #16 instantiation barrier. Everything below is about the contract the fix establishes; all three HIGHs are silent-correctness holes the test suite cannot catch by construction.

HIGH

A — ODR hazard (cpp-pro). has_numsim_serialize<T> is detected by ADL at the point of instantiation. If a non-trivial T's hook is visible in some TUs but not others, the trait — and the serialize() virtual body that branches on it — differ across TUs: a silent ODR violation where the linker picks one vtable slot. Latent today (the real T is double, no hook), but a footgun the moment a hook lands.
→ Solved: documented the contract loudly at the customization point — the hook must be declared in T's own header (an associated namespace of T) so every TU detects it identically. Cannot be machine-enforced; the doc is the mitigation.

B — Asymmetric hooks (architect). serialize and deserialize detect their hooks independently, so a type defining numsim_serialize but not numsim_deserialize would write state it can never read back — silent until call time, after the data is persisted.
→ Solved: static_assert(has_numsim_deserialize<T>::value, …) inside the taken hook branch of serialize() (and the mirror in deserialize()). It lives in an if constexpr branch, so a type with neither hook still compiles (takes the throwing else-branch) → #16 stays fixed; only a type with exactly one hook fails to build. Sabotage-verified: a serialize-only type now fails to compile with the both-or-neither message.

C — Dispatch precedence (architect + code-reviewer). is_trivially_copyable is checked before the hook, so a trivially-copyable T that provides a hook gets it silently ignored (raw bytes win) — exactly the POD-portability case a hook exists for.
→ Solved: static_assert(!has_numsim_serialize<T>::value, …) inside the trivially-copyable branch (reject loudly). For T = double the trait is false → passes; only the ambiguous combination errors. Sabotage-verified.

MED

D — No I/O stream-state check (architect). os.write/is.read failures were unchecked; a truncated read left m_old/m_new indeterminate, indistinguishable from success.
→ Solved: if (!os) / if (!is) after the raw read/write → throw.

E — No CI (code-reviewer, sharpened). The #16 regression is clang-specific (eager virtual instantiation per [temp.inst]/11); this repo has no .github/workflows, so nothing builds it under clang — local g++ is false-green.
→ Solved (pending push): a gcc-14 + clang-19 × {Debug,Release} workflow is committed locally; it's blocked on the pushing token's missing workflow scope and will be added so the regression is guarded automatically. In the meantime the suite was built + run under both compilers by hand (see test plan).

F — commit()/revert() are noexcept and now reachable for a T with throwing copy-assignment (e.g. a std::vector-backed T under allocation failure → std::terminate).
→ Accepted by proof. The intended value types are safe — scalars, and tmech::tensor whose operator=/copy-ctor are noexcept (verified, array-backed). A static_assert(is_nothrow_copy_assignable_v<T>) is not a valid fix: commit()/revert() are virtuals pulled into the vtable, so the assert would fire at class instantiation and re-block history_property<T> for throwing-assign T — reintroducing the #16 class of bug (confirmed empirically). The base contract is noexcept, which an override cannot widen. So it's a documented precondition on the value type; revisit only if a throwing-assign history value type is genuinely needed.

G — EXPECT_THROW(std::runtime_error) too loose (both throws share the base type; a wrong-path throw would satisfy it).
→ Solved: replaced with a helper matching the message substring (numsim_serialize / numsim_deserialize), no gmock dependency.

LOW

• H — Missing #include <utility> for std::declval (worked via transitive <type_traits>, non-conforming). → added.
• I — Detection traits checked well-formedness only → false-positive on a non-void numsim_serialize. → constrained to std::enable_if_t<std::is_void_v<…>>.
• J — Raw reinterpret_cast+sizeof is now the documented default — non-portable/un-versioned. → documented as host-native/same-build only, not a portable archive format.
• K — clone() test asserted only non-null + hook round-trip didn't pin that bytes were produced. → clone() now dynamic_casts, checks values + source-mutation independence; hook test asserts !os.str().empty().

Verification

• Suite 62/62 under g++-14 and clang-18 (history_property tests first init #1–CMake: switch to INTERFACE library with INTERFACE cxx_std_23 #4 included).
• Both new compile-time guards (B, C) sabotage-verified to fire with the intended messages.
• Real-type regression: history_property<tmech::tensor<double,3,2>> (explicit template class forcing all virtuals) compiles and runs (commit/revert/clone; serialize throws with no hook) under clang — the exact input_property<T>::wire instantiates history_property<T> triviality static_assert — breaks tmech tensors under clang-19 #16 failure, resolved.

[petlenz]

Inline anchors for the folded-in review findings (full summary in the PR comment + REVIEW-history-property.md).

[petlenz]

Finding A (HIGH) — ODR. has_numsim_serialize<T> resolves by ADL at the point of instantiation, and it feeds the serialize() virtual body. If the hook is visible in some TUs but not others, the trait result — and the virtual's compiled body — diverge across TUs: a silent ODR violation, linker picks one vtable slot. Resolution: documented (here) that the hook must be declared in T's own header so every TU detects it identically. Not machine-enforceable; the doc is the mitigation.

[petlenz]

Finding I (LOW). Original trait used std::void_t<decltype(...)>, which checks only well-formedness → a non-void numsim_serialize would be a false positive. Resolution: constrained to std::enable_if_t<std::is_void_v<decltype(...)>> so the hook's void return is part of detection.

[petlenz]

Finding C (HIGH) — silent precedence. is_trivially_copyable is checked before the hook, so a trivially-copyable T that also provides a hook would have it silently ignored (raw bytes win) — the exact POD-portability case a hook exists for. Resolution: this static_assert rejects the ambiguous combination loudly. For T = double the trait is false, so it only fires on the genuine ambiguity. Sabotage-verified.

[petlenz]

Finding D (MED). Raw os.write failure was unchecked. Resolution: throw on a failed write so a serialize error is as loud as a missing hook.

[petlenz]

Finding B (HIGH) — asymmetric hooks. serialize/deserialize detect hooks independently, so a serialize-only type could write state it can never read back. Resolution: this static_assert (inside the taken hook branch) enforces both-or-neither. Because it sits in an if constexpr branch, a type with neither hook still compiles (throws at call time) — so the #16 fix is preserved; only the exactly-one-hook case fails to build. Sabotage-verified.

[petlenz]

Finding D (MED). A truncated/failed is.read previously left m_old/m_new indeterminate, indistinguishable from success. Resolution: throw on a short/failed read so a corrupt restore fails loudly.

[petlenz]

Finding F (MED) — accepted by proof. commit()/revert() are noexcept and now reachable for a T with throwing copy-assignment (e.g. std::vector-backed under OOM) → std::terminate. A static_assert(is_nothrow_copy_assignable_v<T>) is not a valid fix: these are virtuals pulled into the vtable, so it would fire at class instantiation and re-block history_property<T> exactly like the original #16 static_assert (confirmed empirically). Base contract is noexcept (an override can't widen). The intended types — scalars, tmech::tensor (noexcept ops) — are safe, so this is documented as a value-type precondition.