From 7bc20f78abf60491bf6d5ab2fd21774bbcdaf372 Mon Sep 17 00:00:00 2001 From: petlenz Date: Tue, 2 Jun 2026 23:41:43 +0200 Subject: [PATCH 1/4] Phase 2.2 prep: state-var alignment invariant, declared tag, find_state_variable_by_name (#59) --- include/numsim_codegen/passes/pass.h | 15 +++ include/numsim_codegen/passes/pass_tags.h | 11 +++ .../passes/symbol_validation_pass.h | 3 +- include/numsim_codegen/recipe.h | 80 ++++++++++++++++ tests/StateVariableTest.cpp | 93 +++++++++++++++++++ 5 files changed, 201 insertions(+), 1 deletion(-) diff --git a/include/numsim_codegen/passes/pass.h b/include/numsim_codegen/passes/pass.h index 046080c..b2bc932 100644 --- a/include/numsim_codegen/passes/pass.h +++ b/include/numsim_codegen/passes/pass.h @@ -15,6 +15,7 @@ namespace numsim::codegen { struct SymbolDecl; +struct StateVariable; // Shared state for a single PassManager invocation. Passes read the // recipe via `model` (a RecipeView — const-only today, will gain a @@ -73,6 +74,20 @@ enum class LookupError { std::string const &name) noexcept -> std::expected; +// Convenience: resolve a name to its StateVariable record. Returns +// nullable pointer rather than `std::expected` per `docs/workflow.md` +// §6.1 — only one failure mode ("no state variable with this name") +// that the caller could distinguish. Linear scan over +// `pctx.model.state_variables()`; state-variable counts are small +// (single-digit typical, tens worst-case for multi-surface plasticity) +// so the scan is fine. Could be promoted to a populated map in +// PassContext if that ever changes. Issue #59 / REVIEW-pr-58.md m3. +// +// Definition lives in recipe.h where `StateVariable` is complete. +[[nodiscard]] inline auto find_state_variable_by_name( + PassContext const &pctx, std::string_view name) noexcept + -> StateVariable const *; + // Abstract base for a single codegen pass. // // Passes advertise their pre/postconditions as string tags. PassManager diff --git a/include/numsim_codegen/passes/pass_tags.h b/include/numsim_codegen/passes/pass_tags.h index f6844a1..e17a78c 100644 --- a/include/numsim_codegen/passes/pass_tags.h +++ b/include/numsim_codegen/passes/pass_tags.h @@ -36,6 +36,17 @@ namespace numsim::codegen::pass_tags { inline constexpr std::string_view symbols_declared = "symbols-declared"; inline constexpr std::string_view identifiers_valid = "identifiers-valid"; +// SymbolValidationPass postcondition for Phase 2.1+ state variables +// (issue #59 / REVIEW-pr-58.md m2). Phase 2.2's TimeIntegrationPass +// materially depends on state variables being declared (it lowers +// `Dt(α) → (α − α_old)/dt` by looking up the pair). The tag means +// "checked," not "non-empty" — SymbolValidationPass advertises it +// unconditionally after walking `state_variables()`. Recipes without +// internal state (pure elasticity) still satisfy the postcondition with +// an empty `state_variables()` span. +inline constexpr std::string_view state_variables_declared = + "state-variables-declared"; + // TensorSpaceConsistencyPass postcondition. Renamed from the original // `tensor-space-validated` per P6 — the original overpromised; Phase 2's // expression-level inference pass will use a separate tag. diff --git a/include/numsim_codegen/passes/symbol_validation_pass.h b/include/numsim_codegen/passes/symbol_validation_pass.h index 5724bfe..bc34c56 100644 --- a/include/numsim_codegen/passes/symbol_validation_pass.h +++ b/include/numsim_codegen/passes/symbol_validation_pass.h @@ -34,7 +34,8 @@ class SymbolValidationPass final : public Pass { } [[nodiscard]] auto postconditions() const -> std::vector override { - return {pass_tags::symbols_declared, pass_tags::identifiers_valid}; + return {pass_tags::symbols_declared, pass_tags::identifiers_valid, + pass_tags::state_variables_declared}; } void run(PassContext &pctx) override; // defined in recipe.h after class. diff --git a/include/numsim_codegen/recipe.h b/include/numsim_codegen/recipe.h index a4a7650..5ec169d 100644 --- a/include/numsim_codegen/recipe.h +++ b/include/numsim_codegen/recipe.h @@ -382,6 +382,7 @@ class ConstitutiveModel { // artifacts for the emit pipeline must run inside the same // emit_compute_function() invocation, not a separate validate() call. void validate() const { + validate_state_variable_symbol_alignment(); PassContext pctx{RecipeView{*this}, CodeGenContext{}, std::nullopt, {}}; PassManager pm; pm.emplace(); @@ -396,6 +397,7 @@ class ConstitutiveModel { // Future phases (TimeIntegrationPass, AlgorithmicTangentPass, …) plug // additional passes into this same pipeline. [[nodiscard]] auto emit_compute_function() const -> std::string { + validate_state_variable_symbol_alignment(); PassContext pctx{RecipeView{*this}, CodeGenContext{}, std::nullopt, {}}; PassManager pm; pm.emplace(); @@ -488,6 +490,73 @@ class ConstitutiveModel { } private: + // Phase 2.2 prep (issue #59 / REVIEW-pr-58.md m1): structural-integrity + // invariant linking `m_state_variables` and `m_symbols`. + // + // Today the two are dual sources of truth: `add_*_state_variable` + // writes to both atomically, so an externally-visible mismatch can't + // arise from the public API alone. The check guards against: + // (a) Phase 2.2+ mutating passes that synthesise / replace state + // variables and forget to keep the paired SymbolDecls aligned; + // (b) a future internal codepath constructing a StateVariable + // directly (bypassing the add methods); + // (c) post-construction mutation of m_symbols by a not-yet-existing + // refactor pass that reorders / removes entries. + // + // Runs from validate() and emit_compute_function() before the pass + // pipeline so downstream passes can rely on the invariant. Throws + // std::runtime_error naming the offending state variable + the + // specific mismatch — silent corruption is worse than a loud throw. + void validate_state_variable_symbol_alignment() const { + auto check = [&](std::size_t idx, std::string_view expected_name, + SymbolDecl::Category expected_cat, + StateVariable const &sv, char const *which) { + if (idx >= m_symbols.size()) { + throw std::runtime_error(std::format( + "ConstitutiveModel '{}': StateVariable '{}' carries {} index " + "{} which is out of bounds for m_symbols (size {}). State " + "variable / symbol vectors are out of sync.", + m_name, sv.name, which, idx, m_symbols.size())); + } + auto const &s = m_symbols[idx]; + if (s.name != expected_name) { + throw std::runtime_error(std::format( + "ConstitutiveModel '{}': StateVariable '{}' {} index points " + "to SymbolDecl named '{}'; expected '{}'.", + m_name, sv.name, which, s.name, expected_name)); + } + if (s.category != expected_cat) { + throw std::runtime_error(std::format( + "ConstitutiveModel '{}': StateVariable '{}' {} symbol has " + "wrong Category. State variable / symbol vectors are out of " + "sync.", + m_name, sv.name, which)); + } + if (s.kind != sv.kind) { + throw std::runtime_error(std::format( + "ConstitutiveModel '{}': StateVariable '{}' {} symbol has " + "Kind mismatched against the state-variable record.", + m_name, sv.name, which)); + } + if (sv.kind == SymbolDecl::Kind::Tensor && + (s.dim != sv.dim || s.rank != sv.rank)) { + throw std::runtime_error(std::format( + "ConstitutiveModel '{}': StateVariable '{}' {} symbol has " + "dim/rank ({}, {}) mismatched against the state-variable " + "record ({}, {}).", + m_name, sv.name, which, s.dim, s.rank, sv.dim, sv.rank)); + } + }; + for (auto const &sv : m_state_variables) { + check(sv.current_symbol_idx, sv.name, + SymbolDecl::Category::StateVariableCurrent, sv, "current"); + auto const old_name = + sv.name + std::string{state_variable_old_suffix}; + check(sv.old_symbol_idx, old_name, + SymbolDecl::Category::StateVariableOld, sv, "old"); + } + } + // Phase 2.1 (M1+M2 in REVIEW-pr-58.md): reject a duplicate symbol // name at add time, with a clear pipeline-misconfiguration message. // Two collision modes this catches: @@ -634,6 +703,17 @@ inline auto RecipeView::tensor_symbol_map() const -> TensorSymbolMap const & { return &decl; } +[[nodiscard]] inline auto find_state_variable_by_name( + PassContext const &pctx, std::string_view name) noexcept + -> StateVariable const * { + for (auto const &sv : pctx.model.state_variables()) { + if (sv.name == name) { + return &sv; + } + } + return nullptr; +} + // ─── Function-frame rendering (free functions) ─────────────────────── // // These render the surrounding function frame after the body statements diff --git a/tests/StateVariableTest.cpp b/tests/StateVariableTest.cpp index 882dab6..40844bc 100644 --- a/tests/StateVariableTest.cpp +++ b/tests/StateVariableTest.cpp @@ -316,4 +316,97 @@ TEST(StateVariable, StateVarUsableInOutputExpression) { << "_old value appears as const input param: " << src; } +// ─── Phase 2.2 prep (issue #59) ────────────────────────────────────── + +TEST(StateVariablePhase22Prep, AlignmentInvariantValidatesScalarAndTensor) { + // Item 1 / REVIEW-pr-58.md m1: validate() runs the + // validate_state_variable_symbol_alignment() invariant check before + // the pass pipeline. A well-formed recipe with both scalar and tensor + // state variables (interleaved with inputs + parameters so indices + // land at non-trivial offsets) must pass. + ConstitutiveModel model("M"); + auto K = model.add_parameter("K", 1.0); + (void)model.add_scalar_input("eps_v"); + auto alpha = model.add_scalar_state_variable( + "alpha", cas::make_expression(0.0)); + (void)K; + (void)alpha; + auto eps_p_init = cas::make_expression(3, 2); + (void)model.add_tensor_state_variable("eps_p", 3, 2, eps_p_init); + + EXPECT_NO_THROW(model.validate()); + + // Indices land where add_*_state_variable claims they do: + for (auto const &sv : model.state_variables()) { + auto const &cur = model.symbols()[sv.current_symbol_idx]; + auto const &old = model.symbols()[sv.old_symbol_idx]; + EXPECT_EQ(cur.name, sv.name); + EXPECT_EQ(old.name, sv.name + "_old"); + EXPECT_EQ(cur.kind, sv.kind); + EXPECT_EQ(old.kind, sv.kind); + EXPECT_EQ(cur.category, SymbolDecl::Category::StateVariableCurrent); + EXPECT_EQ(old.category, SymbolDecl::Category::StateVariableOld); + if (sv.kind == SymbolDecl::Kind::Tensor) { + EXPECT_EQ(cur.dim, sv.dim); + EXPECT_EQ(cur.rank, sv.rank); + EXPECT_EQ(old.dim, sv.dim); + EXPECT_EQ(old.rank, sv.rank); + } + } +} + +TEST(StateVariablePhase22Prep, SymbolValidationPassAdvertisesStateVarsTag) { + // Item 2 / REVIEW-pr-58.md m2: SymbolValidationPass advertises + // `state_variables_declared` unconditionally, including for recipes + // with zero state variables. Phase 2.2's TimeIntegrationPass will + // declare this as a precondition. + SymbolValidationPass pass; + auto const post = pass.postconditions(); + bool found = false; + for (auto const &tag : post) { + if (tag == pass_tags::state_variables_declared) { + found = true; + break; + } + } + EXPECT_TRUE(found) + << "SymbolValidationPass must advertise state_variables_declared"; + + // The tag must be satisfied even when the recipe has no state vars: + ConstitutiveModel pure_elastic("E"); + (void)pure_elastic.add_parameter("mu", 0.5); + EXPECT_NO_THROW(pure_elastic.validate()); +} + +TEST(StateVariablePhase22Prep, FindStateVariableByName) { + // Item 3 / REVIEW-pr-58.md m3: find_state_variable_by_name resolves + // a name to its StateVariable record. Returns nullptr on miss. + ConstitutiveModel model("M"); + (void)model.add_scalar_state_variable( + "alpha", cas::make_expression(0.0)); + auto eps_p_init = cas::make_expression(3, 2); + (void)model.add_tensor_state_variable("eps_p", 3, 2, eps_p_init); + + PassContext pctx{RecipeView{model}, CodeGenContext{}, std::nullopt, {}}; + + auto const *alpha_sv = find_state_variable_by_name(pctx, "alpha"); + ASSERT_NE(alpha_sv, nullptr); + EXPECT_EQ(alpha_sv->name, "alpha"); + EXPECT_EQ(alpha_sv->kind, SymbolDecl::Kind::Scalar); + + auto const *eps_p_sv = find_state_variable_by_name(pctx, "eps_p"); + ASSERT_NE(eps_p_sv, nullptr); + EXPECT_EQ(eps_p_sv->name, "eps_p"); + EXPECT_EQ(eps_p_sv->kind, SymbolDecl::Kind::Tensor); + + // The paired `_old` symbol is NOT itself a StateVariable record — it + // is a SymbolDecl whose owning StateVariable is named without the + // suffix. Looking up by the suffixed name must miss. + EXPECT_EQ(find_state_variable_by_name(pctx, "alpha_old"), nullptr); + + // Unrelated names miss. + EXPECT_EQ(find_state_variable_by_name(pctx, "nope"), nullptr); + EXPECT_EQ(find_state_variable_by_name(pctx, ""), nullptr); +} + } // namespace numsim::codegen From 16ac2b9e9fa2fc27a4ea8872251bcd73dc4f45e3 Mon Sep 17 00:00:00 2001 From: petlenz Date: Wed, 3 Jun 2026 20:56:03 +0200 Subject: [PATCH 2/4] Address PR #66 review: alignment in pass, conditional tag, enum-value msgs, 5 negative tests --- include/numsim_codegen/passes/pass_tags.h | 38 +++- .../passes/symbol_validation_pass.h | 20 +- include/numsim_codegen/recipe.h | 167 ++++++++------- tests/StateVariableTest.cpp | 195 ++++++++++++++++-- 4 files changed, 321 insertions(+), 99 deletions(-) diff --git a/include/numsim_codegen/passes/pass_tags.h b/include/numsim_codegen/passes/pass_tags.h index e17a78c..a36a287 100644 --- a/include/numsim_codegen/passes/pass_tags.h +++ b/include/numsim_codegen/passes/pass_tags.h @@ -36,16 +36,34 @@ namespace numsim::codegen::pass_tags { inline constexpr std::string_view symbols_declared = "symbols-declared"; inline constexpr std::string_view identifiers_valid = "identifiers-valid"; -// SymbolValidationPass postcondition for Phase 2.1+ state variables -// (issue #59 / REVIEW-pr-58.md m2). Phase 2.2's TimeIntegrationPass -// materially depends on state variables being declared (it lowers -// `Dt(α) → (α − α_old)/dt` by looking up the pair). The tag means -// "checked," not "non-empty" — SymbolValidationPass advertises it -// unconditionally after walking `state_variables()`. Recipes without -// internal state (pure elasticity) still satisfy the postcondition with -// an empty `state_variables()` span. -inline constexpr std::string_view state_variables_declared = - "state-variables-declared"; +// State-variable tags advertised by `SymbolValidationPass` (issue #59 / +// REVIEW-pr-58.md m2, refined by PR #66 review). +// +// Phase 2.2+ passes that touch state variables fall in one of two +// shapes, and the framework can serve them differently with two tags: +// +// * `state_variables_checked` — ALWAYS advertised. The +// SymbolValidationPass body has walked `state_variables()` and +// verified the alignment invariant with `symbols()` (via +// `verify_state_variable_symbol_alignment`). A pass that runs even +// on pure-elasticity recipes (e.g. one that emits no-op stubs for +// consistency) takes this as a precondition. +// +// * `state_variables_non_empty` — advertised IFF the recipe actually +// has at least one state variable. A pass whose body is a no-op on +// empty state vectors (TimeIntegrationPass, KuhnTuckerLoweringPass) +// takes this as a precondition; PassManager then automatically +// refuses to register it on a pure-elasticity recipe — surfacing +// the misconfiguration loudly rather than the pass silently doing +// nothing. +// +// The split keeps the framework's tag-tracking honest: a passcondition +// has the same name across every consumer, so a typo or rename surfaces +// at compile time, not in the form of a pass that quietly never fires. +inline constexpr std::string_view state_variables_checked = + "state-variables-checked"; +inline constexpr std::string_view state_variables_non_empty = + "state-variables-non-empty"; // TensorSpaceConsistencyPass postcondition. Renamed from the original // `tensor-space-validated` per P6 — the original overpromised; Phase 2's diff --git a/include/numsim_codegen/passes/symbol_validation_pass.h b/include/numsim_codegen/passes/symbol_validation_pass.h index bc34c56..651f1e1 100644 --- a/include/numsim_codegen/passes/symbol_validation_pass.h +++ b/include/numsim_codegen/passes/symbol_validation_pass.h @@ -34,8 +34,17 @@ class SymbolValidationPass final : public Pass { } [[nodiscard]] auto postconditions() const -> std::vector override { - return {pass_tags::symbols_declared, pass_tags::identifiers_valid, - pass_tags::state_variables_declared}; + // `state_variables_non_empty` is conditionally advertised — PR #66 + // review #6. `m_state_variables_non_empty` is set by `run()` to + // reflect the recipe's actual state-variable count; PassManager + // queries postconditions() AFTER run(), so the conditional is safe. + std::vector tags{pass_tags::symbols_declared, + pass_tags::identifiers_valid, + pass_tags::state_variables_checked}; + if (m_state_variables_non_empty) { + tags.push_back(pass_tags::state_variables_non_empty); + } + return tags; } void run(PassContext &pctx) override; // defined in recipe.h after class. @@ -112,6 +121,13 @@ class SymbolValidationPass final : public Pass { } return !is_cxx_keyword(s); } + +private: + // Runtime flag set by `run()` to drive the conditional + // `state_variables_non_empty` postcondition (declared above). + // Default-false so a pre-`run()` query reports the safe shape (no + // such query exists today, but defensive). + bool m_state_variables_non_empty = false; }; } // namespace numsim::codegen diff --git a/include/numsim_codegen/recipe.h b/include/numsim_codegen/recipe.h index 5ec169d..cf6c11a 100644 --- a/include/numsim_codegen/recipe.h +++ b/include/numsim_codegen/recipe.h @@ -382,7 +382,6 @@ class ConstitutiveModel { // artifacts for the emit pipeline must run inside the same // emit_compute_function() invocation, not a separate validate() call. void validate() const { - validate_state_variable_symbol_alignment(); PassContext pctx{RecipeView{*this}, CodeGenContext{}, std::nullopt, {}}; PassManager pm; pm.emplace(); @@ -397,7 +396,6 @@ class ConstitutiveModel { // Future phases (TimeIntegrationPass, AlgorithmicTangentPass, …) plug // additional passes into this same pipeline. [[nodiscard]] auto emit_compute_function() const -> std::string { - validate_state_variable_symbol_alignment(); PassContext pctx{RecipeView{*this}, CodeGenContext{}, std::nullopt, {}}; PassManager pm; pm.emplace(); @@ -489,74 +487,16 @@ class ConstitutiveModel { return m_tensor_symbols; } -private: - // Phase 2.2 prep (issue #59 / REVIEW-pr-58.md m1): structural-integrity - // invariant linking `m_state_variables` and `m_symbols`. - // - // Today the two are dual sources of truth: `add_*_state_variable` - // writes to both atomically, so an externally-visible mismatch can't - // arise from the public API alone. The check guards against: - // (a) Phase 2.2+ mutating passes that synthesise / replace state - // variables and forget to keep the paired SymbolDecls aligned; - // (b) a future internal codepath constructing a StateVariable - // directly (bypassing the add methods); - // (c) post-construction mutation of m_symbols by a not-yet-existing - // refactor pass that reorders / removes entries. - // - // Runs from validate() and emit_compute_function() before the pass - // pipeline so downstream passes can rely on the invariant. Throws - // std::runtime_error naming the offending state variable + the - // specific mismatch — silent corruption is worse than a loud throw. - void validate_state_variable_symbol_alignment() const { - auto check = [&](std::size_t idx, std::string_view expected_name, - SymbolDecl::Category expected_cat, - StateVariable const &sv, char const *which) { - if (idx >= m_symbols.size()) { - throw std::runtime_error(std::format( - "ConstitutiveModel '{}': StateVariable '{}' carries {} index " - "{} which is out of bounds for m_symbols (size {}). State " - "variable / symbol vectors are out of sync.", - m_name, sv.name, which, idx, m_symbols.size())); - } - auto const &s = m_symbols[idx]; - if (s.name != expected_name) { - throw std::runtime_error(std::format( - "ConstitutiveModel '{}': StateVariable '{}' {} index points " - "to SymbolDecl named '{}'; expected '{}'.", - m_name, sv.name, which, s.name, expected_name)); - } - if (s.category != expected_cat) { - throw std::runtime_error(std::format( - "ConstitutiveModel '{}': StateVariable '{}' {} symbol has " - "wrong Category. State variable / symbol vectors are out of " - "sync.", - m_name, sv.name, which)); - } - if (s.kind != sv.kind) { - throw std::runtime_error(std::format( - "ConstitutiveModel '{}': StateVariable '{}' {} symbol has " - "Kind mismatched against the state-variable record.", - m_name, sv.name, which)); - } - if (sv.kind == SymbolDecl::Kind::Tensor && - (s.dim != sv.dim || s.rank != sv.rank)) { - throw std::runtime_error(std::format( - "ConstitutiveModel '{}': StateVariable '{}' {} symbol has " - "dim/rank ({}, {}) mismatched against the state-variable " - "record ({}, {}).", - m_name, sv.name, which, s.dim, s.rank, sv.dim, sv.rank)); - } - }; - for (auto const &sv : m_state_variables) { - check(sv.current_symbol_idx, sv.name, - SymbolDecl::Category::StateVariableCurrent, sv, "current"); - auto const old_name = - sv.name + std::string{state_variable_old_suffix}; - check(sv.old_symbol_idx, old_name, - SymbolDecl::Category::StateVariableOld, sv, "old"); - } - } + // Test-only accessor: grants negative-path tests in `tests/` controlled + // write access to `m_symbols` and `m_state_variables` so they can + // simulate the corruption shapes that Phase 2.2+ mutating passes might + // produce (issue #59 review finding #1). The friend is declared but + // never defined inside `numsim::codegen`; tests provide the only + // definition. Keeping the surface narrow — a single friend struct — + // means production code paths cannot accidentally rely on it. + friend struct testing_internal_state_variable_alignment; +private: // Phase 2.1 (M1+M2 in REVIEW-pr-58.md): reject a duplicate symbol // name at add time, with a clear pipeline-misconfiguration message. // Two collision modes this catches: @@ -714,6 +654,83 @@ inline auto RecipeView::tensor_symbol_map() const -> TensorSymbolMap const & { return nullptr; } +// Phase 2.2 prep (issue #59 / REVIEW-pr-58.md m1 + PR #66 review): +// verify the structural alignment between `model.state_variables()` and +// `model.symbols()`. Throws `std::runtime_error` naming the specific +// mismatch — silent corruption is worse than a loud throw. +// +// Today no public `ConstitutiveModel` API can produce a violation +// because `add_*_state_variable` writes both vectors atomically. The +// check earns its keep when Phase 2.2+ mutating passes start touching +// `m_state_variables` / `m_symbols` independently — at that point this +// function lives inside `SymbolValidationPass`, so mutators can re-run +// the pass (or a derived pass that calls this) to re-verify between +// stages. Run from the pass framework rather than `validate()` directly +// so the invariant cannot be bypassed by a future code path that runs +// passes without invoking `validate()`. +// +// TODO(phase-2.2): when the first mutating pass lands, the +// `testing_internal_state_variable_alignment` friend struct in +// `tests/StateVariableTest.cpp` exercises the failure paths; that +// scaffolding will become the production negative-test corpus. +inline void verify_state_variable_symbol_alignment(RecipeView model) { + auto check = [&](std::size_t idx, std::string_view expected_name, + SymbolDecl::Category expected_cat, + StateVariable const &sv, char const *which) { + auto const symbols = model.symbols(); + if (idx >= symbols.size()) { + throw std::runtime_error(std::format( + "ConstitutiveModel '{}': StateVariable '{}' carries {} index " + "{} which is out of bounds for symbols() (size {}). State " + "variable / symbol vectors are out of sync.", + model.name(), sv.name, which, idx, symbols.size())); + } + auto const &s = symbols[idx]; + if (s.name != expected_name) { + throw std::runtime_error(std::format( + "ConstitutiveModel '{}': StateVariable '{}' {} index points " + "to SymbolDecl named '{}'; expected '{}' (derived from " + "StateVariable.name). The symbols() entry has been renamed or " + "the indices have shifted.", + model.name(), sv.name, which, s.name, expected_name)); + } + if (s.category != expected_cat) { + throw std::runtime_error(std::format( + "ConstitutiveModel '{}': StateVariable '{}' {} symbol has " + "wrong Category (expected={}, got={}). State variable / symbol " + "vectors are out of sync.", + model.name(), sv.name, which, + static_cast(expected_cat), + static_cast(s.category))); + } + if (s.kind != sv.kind) { + throw std::runtime_error(std::format( + "ConstitutiveModel '{}': StateVariable '{}' {} symbol has Kind " + "mismatched against the state-variable record (expected={}, " + "got={}).", + model.name(), sv.name, which, + static_cast(sv.kind), + static_cast(s.kind))); + } + if (sv.kind == SymbolDecl::Kind::Tensor && + (s.dim != sv.dim || s.rank != sv.rank)) { + throw std::runtime_error(std::format( + "ConstitutiveModel '{}': StateVariable '{}' {} symbol has " + "dim/rank ({}, {}) mismatched against the state-variable " + "record ({}, {}).", + model.name(), sv.name, which, s.dim, s.rank, sv.dim, sv.rank)); + } + }; + for (auto const &sv : model.state_variables()) { + check(sv.current_symbol_idx, sv.name, + SymbolDecl::Category::StateVariableCurrent, sv, "current"); + auto const old_name = + std::string{sv.name} + std::string{state_variable_old_suffix}; + check(sv.old_symbol_idx, old_name, + SymbolDecl::Category::StateVariableOld, sv, "old"); + } +} + // ─── Function-frame rendering (free functions) ─────────────────────── // // These render the surrounding function frame after the body statements @@ -925,6 +942,18 @@ inline void SymbolValidationPass::run(PassContext &pctx) { "before referencing a symbol in an output expression."; throw std::runtime_error(msg); } + + // (3) State-variable alignment invariant (issue #59 / PR #66 review). + // Throws if any StateVariable's `current_symbol_idx` / `old_symbol_idx` + // points at a mismatched SymbolDecl. Today's public API can't produce + // a violation, but running the check here means Phase 2.2+ mutating + // passes that touch state variables can re-run SymbolValidationPass + // to re-verify between stages — the invariant isn't bypassable by + // running passes outside `ConstitutiveModel::validate()`. + verify_state_variable_symbol_alignment(model); + + // Drive the conditional `state_variables_non_empty` postcondition. + m_state_variables_non_empty = !model.state_variables().empty(); } inline void TensorSpaceConsistencyPass::run(PassContext &pctx) { diff --git a/tests/StateVariableTest.cpp b/tests/StateVariableTest.cpp index 40844bc..c2934a9 100644 --- a/tests/StateVariableTest.cpp +++ b/tests/StateVariableTest.cpp @@ -355,27 +355,48 @@ TEST(StateVariablePhase22Prep, AlignmentInvariantValidatesScalarAndTensor) { } } -TEST(StateVariablePhase22Prep, SymbolValidationPassAdvertisesStateVarsTag) { - // Item 2 / REVIEW-pr-58.md m2: SymbolValidationPass advertises - // `state_variables_declared` unconditionally, including for recipes - // with zero state variables. Phase 2.2's TimeIntegrationPass will - // declare this as a precondition. - SymbolValidationPass pass; - auto const post = pass.postconditions(); - bool found = false; - for (auto const &tag : post) { - if (tag == pass_tags::state_variables_declared) { - found = true; - break; +TEST(StateVariablePhase22Prep, SymbolValidationPassAdvertisesStateVarTags) { + // PR #66 review: tag split. SymbolValidationPass advertises + // `state_variables_checked` unconditionally and + // `state_variables_non_empty` only when the recipe actually has + // state variables. PassManager queries postconditions() AFTER run(), + // so the conditional advertisement is observable through it. + auto has = [](auto const &v, std::string_view t) { + for (auto const &x : v) { + if (x == t) return true; } + return false; + }; + + // Pure-elasticity recipe: only the always-on tag advertised. + { + ConstitutiveModel pure_elastic("E"); + (void)pure_elastic.add_parameter("mu", 0.5); + PassContext pctx{RecipeView{pure_elastic}, CodeGenContext{}, + std::nullopt, {}}; + SymbolValidationPass pass; + pass.run(pctx); + auto const post = pass.postconditions(); + EXPECT_TRUE(has(post, pass_tags::state_variables_checked)); + EXPECT_FALSE(has(post, pass_tags::state_variables_non_empty)) + << "non_empty tag must NOT fire for state-var-free recipes"; + // validate() itself must still succeed: + EXPECT_NO_THROW(pure_elastic.validate()); } - EXPECT_TRUE(found) - << "SymbolValidationPass must advertise state_variables_declared"; - // The tag must be satisfied even when the recipe has no state vars: - ConstitutiveModel pure_elastic("E"); - (void)pure_elastic.add_parameter("mu", 0.5); - EXPECT_NO_THROW(pure_elastic.validate()); + // Recipe with at least one state variable: both tags advertised. + { + ConstitutiveModel hardening("H"); + (void)hardening.add_scalar_state_variable( + "alpha", cas::make_expression(0.0)); + PassContext pctx{RecipeView{hardening}, CodeGenContext{}, + std::nullopt, {}}; + SymbolValidationPass pass; + pass.run(pctx); + auto const post = pass.postconditions(); + EXPECT_TRUE(has(post, pass_tags::state_variables_checked)); + EXPECT_TRUE(has(post, pass_tags::state_variables_non_empty)); + } } TEST(StateVariablePhase22Prep, FindStateVariableByName) { @@ -409,4 +430,142 @@ TEST(StateVariablePhase22Prep, FindStateVariableByName) { EXPECT_EQ(find_state_variable_by_name(pctx, ""), nullptr); } +// ─── Friend access for negative-path tests (PR #66 review #1) ──────── +// +// `verify_state_variable_symbol_alignment` has 5 distinct throw branches. +// The public ConstitutiveModel API cannot violate the invariant today, +// so the only way to verify each branch fires correctly is to mutate +// the internals directly. `testing_internal_state_variable_alignment` +// is declared `friend struct` inside ConstitutiveModel; its definition +// lives only here, in the test TU. Production code cannot reach it. +// +// When Phase 2.2 mutating passes land, these injection sites become the +// production negative-test corpus — they prove the alignment check +// catches each kind of corruption a mutator might produce. + +} // namespace numsim::codegen + +namespace numsim::codegen { + +struct testing_internal_state_variable_alignment { + // Clobber the name of the SymbolDecl pointed at by a state variable's + // current/old index. Simulates a mutating pass renaming a symbol + // without keeping the pair in sync. + static void poison_symbol_name(ConstitutiveModel &m, std::size_t idx, + std::string new_name) { + m.m_symbols[idx].name = std::move(new_name); + } + // Clobber the category of a symbol. + static void poison_symbol_category(ConstitutiveModel &m, std::size_t idx, + SymbolDecl::Category new_cat) { + m.m_symbols[idx].category = new_cat; + } + // Clobber the kind of a symbol. + static void poison_symbol_kind(ConstitutiveModel &m, std::size_t idx, + SymbolDecl::Kind new_kind) { + m.m_symbols[idx].kind = new_kind; + } + // Clobber the dim of a tensor symbol. + static void poison_symbol_dim(ConstitutiveModel &m, std::size_t idx, + std::size_t new_dim) { + m.m_symbols[idx].dim = new_dim; + } + // Push the current_symbol_idx of a state variable past the end of + // m_symbols to simulate a vector-shrink that left stale indices. + static void poison_state_var_current_idx(ConstitutiveModel &m, + std::size_t sv_idx, + std::size_t new_idx) { + m.m_state_variables[sv_idx].current_symbol_idx = new_idx; + } +}; + +TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionOutOfBoundsIdx) { + ConstitutiveModel model("M"); + (void)model.add_scalar_state_variable( + "alpha", cas::make_expression(0.0)); + testing_internal_state_variable_alignment::poison_state_var_current_idx( + model, 0, 999); + EXPECT_THROW(model.validate(), std::runtime_error); + try { + model.validate(); + } catch (std::runtime_error const &e) { + std::string const what = e.what(); + EXPECT_NE(what.find("out of bounds"), std::string::npos) << what; + EXPECT_NE(what.find("alpha"), std::string::npos) << what; + } +} + +TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionRenamedSymbol) { + ConstitutiveModel model("M"); + auto h = model.add_scalar_state_variable( + "alpha", cas::make_expression(0.0)); + (void)h; + auto const cur_idx = model.state_variables()[0].current_symbol_idx; + testing_internal_state_variable_alignment::poison_symbol_name( + model, cur_idx, "beta"); + EXPECT_THROW(model.validate(), std::runtime_error); + try { + model.validate(); + } catch (std::runtime_error const &e) { + std::string const what = e.what(); + EXPECT_NE(what.find("named 'beta'"), std::string::npos) << what; + EXPECT_NE(what.find("expected 'alpha'"), std::string::npos) << what; + } +} + +TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionWrongCategory) { + ConstitutiveModel model("M"); + (void)model.add_scalar_state_variable( + "alpha", cas::make_expression(0.0)); + auto const cur_idx = model.state_variables()[0].current_symbol_idx; + testing_internal_state_variable_alignment::poison_symbol_category( + model, cur_idx, SymbolDecl::Category::Input); + EXPECT_THROW(model.validate(), std::runtime_error); + try { + model.validate(); + } catch (std::runtime_error const &e) { + std::string const what = e.what(); + // The message must print both observed and expected enum values + // (PR #66 review #4): + EXPECT_NE(what.find("expected="), std::string::npos) << what; + EXPECT_NE(what.find("got="), std::string::npos) << what; + EXPECT_NE(what.find("wrong Category"), std::string::npos) << what; + } +} + +TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionWrongKind) { + ConstitutiveModel model("M"); + (void)model.add_scalar_state_variable( + "alpha", cas::make_expression(0.0)); + auto const cur_idx = model.state_variables()[0].current_symbol_idx; + testing_internal_state_variable_alignment::poison_symbol_kind( + model, cur_idx, SymbolDecl::Kind::Tensor); + EXPECT_THROW(model.validate(), std::runtime_error); + try { + model.validate(); + } catch (std::runtime_error const &e) { + std::string const what = e.what(); + EXPECT_NE(what.find("Kind mismatched"), std::string::npos) << what; + EXPECT_NE(what.find("expected="), std::string::npos) << what; + EXPECT_NE(what.find("got="), std::string::npos) << what; + } +} + +TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionWrongDim) { + ConstitutiveModel model("M"); + auto eps_p_init = cas::make_expression(3, 2); + (void)model.add_tensor_state_variable("eps_p", 3, 2, eps_p_init); + auto const cur_idx = model.state_variables()[0].current_symbol_idx; + testing_internal_state_variable_alignment::poison_symbol_dim( + model, cur_idx, 2); + EXPECT_THROW(model.validate(), std::runtime_error); + try { + model.validate(); + } catch (std::runtime_error const &e) { + std::string const what = e.what(); + EXPECT_NE(what.find("dim/rank"), std::string::npos) << what; + EXPECT_NE(what.find("eps_p"), std::string::npos) << what; + } +} + } // namespace numsim::codegen From a278feee204e4ef808d33852e4298987960906c0 Mon Sep 17 00:00:00 2001 From: petlenz Date: Wed, 3 Jun 2026 21:11:27 +0200 Subject: [PATCH 3/4] Address PR #66 round-2 review: reset flag on run() entry, fix docs, tighten 5 negative tests --- docs/workflow.md | 5 +- include/numsim_codegen/passes/pass.h | 20 +++++ include/numsim_codegen/passes/pass_tags.h | 9 ++- include/numsim_codegen/recipe.h | 80 +++++++++++++++----- tests/StateVariableTest.cpp | 92 ++++++++++++++++------- 5 files changed, 156 insertions(+), 50 deletions(-) diff --git a/docs/workflow.md b/docs/workflow.md index 262ab15..2fa9cf0 100644 --- a/docs/workflow.md +++ b/docs/workflow.md @@ -66,12 +66,13 @@ sequenceDiagram PM->>SVP: run(pctx) SVP->>SVP: walk symbols, check identifier validity
(ASCII range + C++ keyword reject) SVP->>SVP: walk outputs, check no undeclared symbols - SVP-->>PM: postconditions:
{symbols-declared, identifiers-valid} + SVP->>SVP: verify_state_variable_symbol_alignment
(Phase 2.1+ pairing invariant) + SVP-->>PM: postconditions:
{symbols-declared, identifiers-valid,
state-variables-checked,
state-variables-non-empty (iff non-empty)} PM->>TSP: run(pctx) TSP->>TSP: cross-check Role.is_symmetric vs
tensor_space.perm (Skew = conflict) TSP->>TSP: cross-check Role.expected_rank vs
tensor.rank() - TSP-->>PM: postconditions:
{tensor-space-validated} + TSP-->>PM: postconditions:
{tensor-space-declarations-checked} PM->>CEP: run(pctx) Note over CEP: Cycle-break via
unique_ptr indirection diff --git a/include/numsim_codegen/passes/pass.h b/include/numsim_codegen/passes/pass.h index b2bc932..37afd85 100644 --- a/include/numsim_codegen/passes/pass.h +++ b/include/numsim_codegen/passes/pass.h @@ -103,10 +103,30 @@ class Pass { public: virtual ~Pass() = default; [[nodiscard]] virtual auto name() const -> std::string_view = 0; + + // Tags this pass requires to be satisfied before `run()` is called. + // Stable across the pass's lifetime — `preconditions()` is queried by + // `PassManager::run()` *before* `run()`, so the value must not depend + // on per-call state. [[nodiscard]] virtual auto preconditions() const -> std::vector { return {}; } + + // Tags this pass advertises as satisfied after `run()` returns + // successfully. **Lifecycle contract (PR #66 round-2 review #11):** + // `PassManager::run()` queries `postconditions()` AFTER each call to + // `run()`, never before. Implementations are therefore free to return + // values that depend on per-call state set during `run()` (e.g. + // SymbolValidationPass uses this to conditionally advertise + // `state_variables_non_empty` based on the recipe). A pre-`run()` + // query is well-defined but may return a *subset* of what the + // post-`run()` query would — implementations should treat pre-`run()` + // as the safe-default shape. + // + // Implementations that store per-call state for this purpose must + // RESET that state at the *start* of `run()`, not the end, so a + // throwing `run()` leaves the pass in a coherent observable state. [[nodiscard]] virtual auto postconditions() const -> std::vector { return {}; diff --git a/include/numsim_codegen/passes/pass_tags.h b/include/numsim_codegen/passes/pass_tags.h index a36a287..1959bd9 100644 --- a/include/numsim_codegen/passes/pass_tags.h +++ b/include/numsim_codegen/passes/pass_tags.h @@ -52,10 +52,13 @@ inline constexpr std::string_view identifiers_valid = "identifiers-valid"; // * `state_variables_non_empty` — advertised IFF the recipe actually // has at least one state variable. A pass whose body is a no-op on // empty state vectors (TimeIntegrationPass, KuhnTuckerLoweringPass) -// takes this as a precondition; PassManager then automatically -// refuses to register it on a pure-elasticity recipe — surfacing +// takes this as a precondition; on a pure-elasticity recipe +// PassManager then fails at `run()` with a clear +// `"pass X requires precondition state-variables-non-empty but no +// earlier pass advertised that postcondition"` message — surfacing // the misconfiguration loudly rather than the pass silently doing -// nothing. +// nothing. (PassManager checks preconditions at `run()` time, not +// at registration; see `pass_manager.h:39-46`.) // // The split keeps the framework's tag-tracking honest: a passcondition // has the same name across every consumer, so a typo or rename surfaces diff --git a/include/numsim_codegen/recipe.h b/include/numsim_codegen/recipe.h index cf6c11a..4e4204c 100644 --- a/include/numsim_codegen/recipe.h +++ b/include/numsim_codegen/recipe.h @@ -190,6 +190,14 @@ struct OutputDecl { Role role = roles::Other; }; +// Forward-declared in a sub-namespace so `ConstitutiveModel`'s +// `friend struct testing_detail::state_variable_alignment_access;` +// resolves. Definition lives only in the test TU (see +// `tests/StateVariableTest.cpp`). PR #66 round-2 review #5. +namespace testing_detail { +struct state_variable_alignment_access; +} // namespace testing_detail + // The constitutive-model registry. Holds declared inputs, parameters, // outputs, and their semantic role tags. Target-agnostic — the same // recipe can be emitted as standalone C++, MOOSE Material, Abaqus UMAT, @@ -487,16 +495,28 @@ class ConstitutiveModel { return m_tensor_symbols; } - // Test-only accessor: grants negative-path tests in `tests/` controlled - // write access to `m_symbols` and `m_state_variables` so they can - // simulate the corruption shapes that Phase 2.2+ mutating passes might - // produce (issue #59 review finding #1). The friend is declared but - // never defined inside `numsim::codegen`; tests provide the only - // definition. Keeping the surface narrow — a single friend struct — - // means production code paths cannot accidentally rely on it. - friend struct testing_internal_state_variable_alignment; - private: + // ─── Test-only friends ────────────────────────────────────────── + // + // PR #66 round-2 review #5: grants negative-path tests in `tests/` + // controlled write access to `m_symbols` and `m_state_variables` so + // they can simulate the corruption shapes Phase 2.2+ mutating passes + // might produce. Placed under the `private:` label (rather than the + // public block) so a reader scanning the class layout doesn't mistake + // the line for a public-API affordance. + // + // Lives in the `testing_detail::` sub-namespace — both as a marker + // ("if you're writing production code and naming this, you've taken + // a wrong turn") and to keep the friend's reachable surface narrow + // from production-namespace TUs. + // + // The struct is declared here but never defined inside + // `numsim::codegen`; tests provide the only definition (see + // `tests/StateVariableTest.cpp`). Keeping the surface to a single + // friend struct means a single ODR slot in the test binary; if a + // future test TU needs the same access, factor the definition to a + // shared `tests/internal/` header rather than redefining it. + friend struct testing_detail::state_variable_alignment_access; // Phase 2.1 (M1+M2 in REVIEW-pr-58.md): reject a duplicate symbol // name at add time, with a clear pipeline-misconfiguration message. // Two collision modes this catches: @@ -663,16 +683,25 @@ inline auto RecipeView::tensor_symbol_map() const -> TensorSymbolMap const & { // because `add_*_state_variable` writes both vectors atomically. The // check earns its keep when Phase 2.2+ mutating passes start touching // `m_state_variables` / `m_symbols` independently — at that point this -// function lives inside `SymbolValidationPass`, so mutators can re-run -// the pass (or a derived pass that calls this) to re-verify between -// stages. Run from the pass framework rather than `validate()` directly -// so the invariant cannot be bypassed by a future code path that runs -// passes without invoking `validate()`. +// function lives inside `SymbolValidationPass`, so any pipeline that +// registers `SymbolValidationPass` re-verifies the invariant on every +// run. Phase 2.2 mutating passes can re-run the pass (or a derived +// `StructuralIntegrityPass` that calls this) to re-check between +// mutations. +// +// **Bypass caveat:** a caller that constructs a `PassManager` and +// omits `SymbolValidationPass` skips the check entirely. The framework +// has no enforcement against that today; the convention is that any +// pipeline that emits code or mutates state variables registers +// `SymbolValidationPass` first. `ConstitutiveModel::validate()` and +// `::emit_compute_function()` both do so. // -// TODO(phase-2.2): when the first mutating pass lands, the -// `testing_internal_state_variable_alignment` friend struct in -// `tests/StateVariableTest.cpp` exercises the failure paths; that -// scaffolding will become the production negative-test corpus. +// TODO(phase-2.2): this scaffolding fault-injects the assertion arms +// (see the friend-poison tests in `tests/StateVariableTest.cpp`). When +// the first mutating pass lands, mutator-driven integration tests +// (apply mutator → assert throws) will be added alongside this corpus, +// not replace it — the fault-injection harness covers the assertion +// arms; the integration tests cover the mutator-to-arm wiring. inline void verify_state_variable_symbol_alignment(RecipeView model) { auto check = [&](std::size_t idx, std::string_view expected_name, SymbolDecl::Category expected_cat, @@ -847,6 +876,21 @@ inline auto render_compute_function( inline void SymbolValidationPass::run(PassContext &pctx) { auto const &model = pctx.model; + // Reset the conditional-postcondition flag on entry — PR #66 round-2 + // review #2. Without this, three failure modes leak prior state: + // (a) `verify_state_variable_symbol_alignment` below throws → the + // end-of-run assignment never executes → the flag retains its + // previous value; + // (b) the same pass instance is reused across two recipes with + // different state-variable counts → mid-run state advertised + // between calls reflects the prior recipe; + // (c) a hypothetical pre-`run()` query (no caller today, but the + // framework contract is implicit) sees stale state. + // Resetting up-front guarantees `m_state_variables_non_empty` always + // reflects the *current* run; the real value is computed after + // validation succeeds at the bottom of this function. + m_state_variables_non_empty = false; + // P4 (post-fixup C1): build the name → index lookup once. Indices // survive any future `m_symbols.push_back()` reallocation, whereas // the original SymbolDecl pointers would silently dangle the first diff --git a/tests/StateVariableTest.cpp b/tests/StateVariableTest.cpp index c2934a9..2aa83d8 100644 --- a/tests/StateVariableTest.cpp +++ b/tests/StateVariableTest.cpp @@ -368,6 +368,18 @@ TEST(StateVariablePhase22Prep, SymbolValidationPassAdvertisesStateVarTags) { return false; }; + // PR #66 round-2 #6: pin the pre-`run()` contract. A pristine pass + // instance reports the safe (no non_empty) shape. Without this test + // a future refactor that lazy-inits in `run()` could silently regress + // the safe default. + { + SymbolValidationPass pristine; + auto const pre_run = pristine.postconditions(); + EXPECT_TRUE(has(pre_run, pass_tags::state_variables_checked)); + EXPECT_FALSE(has(pre_run, pass_tags::state_variables_non_empty)) + << "non_empty tag must NOT fire before run()"; + } + // Pure-elasticity recipe: only the always-on tag advertised. { ConstitutiveModel pure_elastic("E"); @@ -430,24 +442,25 @@ TEST(StateVariablePhase22Prep, FindStateVariableByName) { EXPECT_EQ(find_state_variable_by_name(pctx, ""), nullptr); } -// ─── Friend access for negative-path tests (PR #66 review #1) ──────── +// ─── Friend access for negative-path tests (PR #66 review #1, #5) ──── // // `verify_state_variable_symbol_alignment` has 5 distinct throw branches. // The public ConstitutiveModel API cannot violate the invariant today, // so the only way to verify each branch fires correctly is to mutate -// the internals directly. `testing_internal_state_variable_alignment` +// the internals directly. `testing_detail::state_variable_alignment_access` // is declared `friend struct` inside ConstitutiveModel; its definition -// lives only here, in the test TU. Production code cannot reach it. +// lives only here in the test TU. // -// When Phase 2.2 mutating passes land, these injection sites become the -// production negative-test corpus — they prove the alignment check -// catches each kind of corruption a mutator might produce. +// Lives in `numsim::codegen::testing_detail::` rather than the production +// `numsim::codegen::` namespace — both as a marker ("if you're writing +// production code and naming this, you've taken a wrong turn") and to +// keep the friend's reachable surface narrow from production TUs. } // namespace numsim::codegen -namespace numsim::codegen { +namespace numsim::codegen::testing_detail { -struct testing_internal_state_variable_alignment { +struct state_variable_alignment_access { // Clobber the name of the SymbolDecl pointed at by a state variable's // current/old index. Simulates a mutating pass renaming a symbol // without keeping the pair in sync. @@ -479,37 +492,52 @@ struct testing_internal_state_variable_alignment { } }; +} // namespace numsim::codegen::testing_detail + +namespace numsim::codegen { + +// Shorthand for the negative-path tests below. +using poison = testing_detail::state_variable_alignment_access; + TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionOutOfBoundsIdx) { + // PR #66 round-2 #7: single try/catch + FAIL() — silent test-pass on + // non-throw is impossible. Negative-substring asserts pin which arm + // fires (round-2 #8). ConstitutiveModel model("M"); (void)model.add_scalar_state_variable( "alpha", cas::make_expression(0.0)); - testing_internal_state_variable_alignment::poison_state_var_current_idx( - model, 0, 999); - EXPECT_THROW(model.validate(), std::runtime_error); + poison::poison_state_var_current_idx(model, 0, 999); try { model.validate(); + FAIL() << "expected std::runtime_error from alignment OOB arm"; } catch (std::runtime_error const &e) { std::string const what = e.what(); EXPECT_NE(what.find("out of bounds"), std::string::npos) << what; EXPECT_NE(what.find("alpha"), std::string::npos) << what; + // Pin: no other arm should have fired. + EXPECT_EQ(what.find("wrong Category"), std::string::npos) << what; + EXPECT_EQ(what.find("Kind mismatched"), std::string::npos) << what; + EXPECT_EQ(what.find("dim/rank"), std::string::npos) << what; } } TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionRenamedSymbol) { ConstitutiveModel model("M"); - auto h = model.add_scalar_state_variable( + (void)model.add_scalar_state_variable( "alpha", cas::make_expression(0.0)); - (void)h; auto const cur_idx = model.state_variables()[0].current_symbol_idx; - testing_internal_state_variable_alignment::poison_symbol_name( - model, cur_idx, "beta"); - EXPECT_THROW(model.validate(), std::runtime_error); + poison::poison_symbol_name(model, cur_idx, "beta"); try { model.validate(); + FAIL() << "expected std::runtime_error from alignment name arm"; } catch (std::runtime_error const &e) { std::string const what = e.what(); EXPECT_NE(what.find("named 'beta'"), std::string::npos) << what; EXPECT_NE(what.find("expected 'alpha'"), std::string::npos) << what; + EXPECT_EQ(what.find("wrong Category"), std::string::npos) << what; + EXPECT_EQ(what.find("Kind mismatched"), std::string::npos) << what; + EXPECT_EQ(what.find("dim/rank"), std::string::npos) << what; + EXPECT_EQ(what.find("out of bounds"), std::string::npos) << what; } } @@ -518,18 +546,21 @@ TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionWrongCategory) { (void)model.add_scalar_state_variable( "alpha", cas::make_expression(0.0)); auto const cur_idx = model.state_variables()[0].current_symbol_idx; - testing_internal_state_variable_alignment::poison_symbol_category( - model, cur_idx, SymbolDecl::Category::Input); - EXPECT_THROW(model.validate(), std::runtime_error); + poison::poison_symbol_category(model, cur_idx, + SymbolDecl::Category::Input); try { model.validate(); + FAIL() << "expected std::runtime_error from alignment category arm"; } catch (std::runtime_error const &e) { std::string const what = e.what(); // The message must print both observed and expected enum values - // (PR #66 review #4): + // (PR #66 round-1 #4). + EXPECT_NE(what.find("wrong Category"), std::string::npos) << what; EXPECT_NE(what.find("expected="), std::string::npos) << what; EXPECT_NE(what.find("got="), std::string::npos) << what; - EXPECT_NE(what.find("wrong Category"), std::string::npos) << what; + EXPECT_EQ(what.find("Kind mismatched"), std::string::npos) << what; + EXPECT_EQ(what.find("dim/rank"), std::string::npos) << what; + EXPECT_EQ(what.find("out of bounds"), std::string::npos) << what; } } @@ -538,16 +569,21 @@ TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionWrongKind) { (void)model.add_scalar_state_variable( "alpha", cas::make_expression(0.0)); auto const cur_idx = model.state_variables()[0].current_symbol_idx; - testing_internal_state_variable_alignment::poison_symbol_kind( - model, cur_idx, SymbolDecl::Kind::Tensor); - EXPECT_THROW(model.validate(), std::runtime_error); + poison::poison_symbol_kind(model, cur_idx, SymbolDecl::Kind::Tensor); try { model.validate(); + FAIL() << "expected std::runtime_error from alignment kind arm"; } catch (std::runtime_error const &e) { std::string const what = e.what(); EXPECT_NE(what.find("Kind mismatched"), std::string::npos) << what; EXPECT_NE(what.find("expected="), std::string::npos) << what; EXPECT_NE(what.find("got="), std::string::npos) << what; + // Pin: must be the kind arm, not the dim arm — the dim arm is + // gated on `sv.kind == Tensor` (record side, still Scalar here); + // a future reorder that swapped gating could silently still pass. + EXPECT_EQ(what.find("dim/rank"), std::string::npos) << what; + EXPECT_EQ(what.find("wrong Category"), std::string::npos) << what; + EXPECT_EQ(what.find("out of bounds"), std::string::npos) << what; } } @@ -556,15 +592,17 @@ TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionWrongDim) { auto eps_p_init = cas::make_expression(3, 2); (void)model.add_tensor_state_variable("eps_p", 3, 2, eps_p_init); auto const cur_idx = model.state_variables()[0].current_symbol_idx; - testing_internal_state_variable_alignment::poison_symbol_dim( - model, cur_idx, 2); - EXPECT_THROW(model.validate(), std::runtime_error); + poison::poison_symbol_dim(model, cur_idx, 2); try { model.validate(); + FAIL() << "expected std::runtime_error from alignment dim arm"; } catch (std::runtime_error const &e) { std::string const what = e.what(); EXPECT_NE(what.find("dim/rank"), std::string::npos) << what; EXPECT_NE(what.find("eps_p"), std::string::npos) << what; + EXPECT_EQ(what.find("wrong Category"), std::string::npos) << what; + EXPECT_EQ(what.find("Kind mismatched"), std::string::npos) << what; + EXPECT_EQ(what.find("out of bounds"), std::string::npos) << what; } } From de973522c95eb5efe36c4184c245f02b3a7678a9 Mon Sep 17 00:00:00 2001 From: petlenz Date: Wed, 3 Jun 2026 22:56:06 +0200 Subject: [PATCH 4/4] Address PR #66 round-3 review: codify state-var precondition, factor friend header, same-instance reuse test --- docs/workflow.md | 4 +- .../numsim_codegen/passes/code_emit_pass.h | 16 ++- include/numsim_codegen/passes/pass.h | 24 ++-- include/numsim_codegen/recipe.h | 17 ++- tests/StateVariableTest.cpp | 136 ++++++++++-------- .../state_variable_alignment_access.h | 68 +++++++++ 6 files changed, 188 insertions(+), 77 deletions(-) create mode 100644 tests/internal/state_variable_alignment_access.h diff --git a/docs/workflow.md b/docs/workflow.md index 2fa9cf0..abf1862 100644 --- a/docs/workflow.md +++ b/docs/workflow.md @@ -64,10 +64,12 @@ sequenceDiagram Model->>PM: run(pctx) PM->>SVP: run(pctx) + SVP->>SVP: reset m_state_variables_non_empty = false
(round-3 safe-on-throw guarantee) SVP->>SVP: walk symbols, check identifier validity
(ASCII range + C++ keyword reject) SVP->>SVP: walk outputs, check no undeclared symbols SVP->>SVP: verify_state_variable_symbol_alignment
(Phase 2.1+ pairing invariant) - SVP-->>PM: postconditions:
{symbols-declared, identifiers-valid,
state-variables-checked,
state-variables-non-empty (iff non-empty)} + SVP->>SVP: set m_state_variables_non_empty
= !state_variables().empty() + SVP-->>PM: postconditions:
{symbols-declared, identifiers-valid,
state-variables-checked,
state-variables-non-empty (iff non-empty)} PM->>TSP: run(pctx) TSP->>TSP: cross-check Role.is_symmetric vs
tensor_space.perm (Skew = conflict) diff --git a/include/numsim_codegen/passes/code_emit_pass.h b/include/numsim_codegen/passes/code_emit_pass.h index 0743a3a..2cfcb49 100644 --- a/include/numsim_codegen/passes/code_emit_pass.h +++ b/include/numsim_codegen/passes/code_emit_pass.h @@ -21,8 +21,17 @@ namespace numsim::codegen { // // Preconditions: `pass_tags::symbols_declared` + // `pass_tags::identifiers_valid` + `pass_tags::tensor_space_declarations_checked` -// (i.e. SymbolValidationPass + TensorSpaceConsistencyPass must have run -// first). If you add a pass that transforms expressions (e.g. a future +// + `pass_tags::state_variables_checked` (i.e. SymbolValidationPass + +// TensorSpaceConsistencyPass must have run first). The state-var tag +// (PR #66 round-3 review #3) codifies the bypass-caveat from +// `verify_state_variable_symbol_alignment` in the precondition graph: +// SymbolValidationPass advertises it unconditionally, so any emit +// pipeline transitively requires SVP — a custom pipeline that stubs +// the identifier tags via a no-op pass and skips SVP would now fail +// at `run()` rather than silently emit code with an unverified +// state-var alignment. +// +// If you add a pass that transforms expressions (e.g. a future // TimeIntegrationPass), register it AFTER the validators but BEFORE // CodeEmitPass. class CodeEmitPass final : public Pass { @@ -33,7 +42,8 @@ class CodeEmitPass final : public Pass { [[nodiscard]] auto preconditions() const -> std::vector override { return {pass_tags::symbols_declared, pass_tags::identifiers_valid, - pass_tags::tensor_space_declarations_checked}; + pass_tags::tensor_space_declarations_checked, + pass_tags::state_variables_checked}; } [[nodiscard]] auto postconditions() const -> std::vector override { diff --git a/include/numsim_codegen/passes/pass.h b/include/numsim_codegen/passes/pass.h index 37afd85..08d9fae 100644 --- a/include/numsim_codegen/passes/pass.h +++ b/include/numsim_codegen/passes/pass.h @@ -114,19 +114,23 @@ class Pass { } // Tags this pass advertises as satisfied after `run()` returns - // successfully. **Lifecycle contract (PR #66 round-2 review #11):** + // successfully. **Lifecycle (PR #66 round-2 review #11, round-3 #2):** // `PassManager::run()` queries `postconditions()` AFTER each call to // `run()`, never before. Implementations are therefore free to return - // values that depend on per-call state set during `run()` (e.g. - // SymbolValidationPass uses this to conditionally advertise - // `state_variables_non_empty` based on the recipe). A pre-`run()` - // query is well-defined but may return a *subset* of what the - // post-`run()` query would — implementations should treat pre-`run()` - // as the safe-default shape. + // values that depend on per-call state set during `run()`. // - // Implementations that store per-call state for this purpose must - // RESET that state at the *start* of `run()`, not the end, so a - // throwing `run()` leaves the pass in a coherent observable state. + // Most passes (`TensorSpaceConsistencyPass`, `CodeEmitPass`) return + // literal initialiser lists from `postconditions()` and don't need to + // think about lifecycle. `SymbolValidationPass` uses the pattern to + // conditionally advertise `state_variables_non_empty` based on the + // recipe — see its `run()` body for the canonical shape. + // + // **Guidance for passes that DO store per-call postcondition state:** + // reset that state at the *start* of `run()`, not the end, so a + // throwing `run()` leaves the pass in a coherent observable state and + // a pre-`run()` query reports the safe-default shape. This is + // convention, not a framework-enforced contract — PassManager has no + // way to check it. [[nodiscard]] virtual auto postconditions() const -> std::vector { return {}; diff --git a/include/numsim_codegen/recipe.h b/include/numsim_codegen/recipe.h index 4e4204c..52a1ab4 100644 --- a/include/numsim_codegen/recipe.h +++ b/include/numsim_codegen/recipe.h @@ -703,10 +703,14 @@ inline auto RecipeView::tensor_symbol_map() const -> TensorSymbolMap const & { // not replace it — the fault-injection harness covers the assertion // arms; the integration tests cover the mutator-to-arm wiring. inline void verify_state_variable_symbol_alignment(RecipeView model) { + // Hoisted out of the lambda body (PR #66 round-3 #6): the span is + // identical across all `check()` invocations within a single call + // — fetching once makes that explicit and removes a redundant copy + // per state variable. + auto const symbols = model.symbols(); auto check = [&](std::size_t idx, std::string_view expected_name, SymbolDecl::Category expected_cat, StateVariable const &sv, char const *which) { - auto const symbols = model.symbols(); if (idx >= symbols.size()) { throw std::runtime_error(std::format( "ConstitutiveModel '{}': StateVariable '{}' carries {} index " @@ -877,15 +881,16 @@ inline void SymbolValidationPass::run(PassContext &pctx) { auto const &model = pctx.model; // Reset the conditional-postcondition flag on entry — PR #66 round-2 - // review #2. Without this, three failure modes leak prior state: + // review #2. Two failure modes leak prior state without this: // (a) `verify_state_variable_symbol_alignment` below throws → the // end-of-run assignment never executes → the flag retains its // previous value; // (b) the same pass instance is reused across two recipes with - // different state-variable counts → mid-run state advertised - // between calls reflects the prior recipe; - // (c) a hypothetical pre-`run()` query (no caller today, but the - // framework contract is implicit) sees stale state. + // different state-variable counts → state advertised between + // calls reflects the prior recipe. + // (A pre-`run()` query on a pristine pass instance is already covered + // by the default-initialiser at `symbol_validation_pass.h:130` — the + // reset-at-entry here does not need to address that case.) // Resetting up-front guarantees `m_state_variables_non_empty` always // reflects the *current* run; the real value is computed after // validation succeeds at the bottom of this function. diff --git a/tests/StateVariableTest.cpp b/tests/StateVariableTest.cpp index 2aa83d8..579bf4c 100644 --- a/tests/StateVariableTest.cpp +++ b/tests/StateVariableTest.cpp @@ -15,9 +15,14 @@ // flow through codegen. This file only exercises the IR layer. #include +#include #include #include +// Shared friend-access header for negative-path tests — see header +// comment for the ODR rationale (PR #66 round-3 review #7). +#include "internal/state_variable_alignment_access.h" + #include #include #include @@ -409,6 +414,61 @@ TEST(StateVariablePhase22Prep, SymbolValidationPassAdvertisesStateVarTags) { EXPECT_TRUE(has(post, pass_tags::state_variables_checked)); EXPECT_TRUE(has(post, pass_tags::state_variables_non_empty)); } + + // Same-instance reuse: PR #66 round-3 #5. Mode (b) of the reset + // comment in `SymbolValidationPass::run` ("same instance reused + // across recipes with different state-var counts") is exercised + // here. Without the reset-at-entry the second `run()` would leave + // the non-empty tag advertised from the first run. + { + SymbolValidationPass reused; + + ConstitutiveModel with_sv("S"); + (void)with_sv.add_scalar_state_variable( + "alpha", cas::make_expression(0.0)); + ConstitutiveModel pure("P"); + (void)pure.add_parameter("k", 1.0); + + // First run: state-var recipe → both tags advertised. + { + PassContext pctx1{RecipeView{with_sv}, CodeGenContext{}, + std::nullopt, {}}; + reused.run(pctx1); + auto const post1 = reused.postconditions(); + EXPECT_TRUE(has(post1, pass_tags::state_variables_checked)); + EXPECT_TRUE(has(post1, pass_tags::state_variables_non_empty)); + } + + // Second run on the SAME instance: pure-elasticity recipe → + // non-empty tag must be cleared. + { + PassContext pctx2{RecipeView{pure}, CodeGenContext{}, + std::nullopt, {}}; + reused.run(pctx2); + auto const post2 = reused.postconditions(); + EXPECT_TRUE(has(post2, pass_tags::state_variables_checked)); + EXPECT_FALSE(has(post2, pass_tags::state_variables_non_empty)) + << "reset-at-entry must clear stale non-empty advert " + "when the same pass instance is reused across recipes " + "with different state-var counts (mode b)"; + } + + // Reverse order on the SAME instance for symmetry — pure first, + // then state-var. Catches a hypothetical "lazy-init that only + // initialises once" regression. + { + PassContext pctx3{RecipeView{pure}, CodeGenContext{}, + std::nullopt, {}}; + reused.run(pctx3); + EXPECT_FALSE(has(reused.postconditions(), + pass_tags::state_variables_non_empty)); + PassContext pctx4{RecipeView{with_sv}, CodeGenContext{}, + std::nullopt, {}}; + reused.run(pctx4); + EXPECT_TRUE(has(reused.postconditions(), + pass_tags::state_variables_non_empty)); + } + } } TEST(StateVariablePhase22Prep, FindStateVariableByName) { @@ -442,62 +502,24 @@ TEST(StateVariablePhase22Prep, FindStateVariableByName) { EXPECT_EQ(find_state_variable_by_name(pctx, ""), nullptr); } -// ─── Friend access for negative-path tests (PR #66 review #1, #5) ──── +// ─── Negative-path tests for the alignment invariant ──────────────── // // `verify_state_variable_symbol_alignment` has 5 distinct throw branches. // The public ConstitutiveModel API cannot violate the invariant today, // so the only way to verify each branch fires correctly is to mutate -// the internals directly. `testing_detail::state_variable_alignment_access` -// is declared `friend struct` inside ConstitutiveModel; its definition -// lives only here in the test TU. -// -// Lives in `numsim::codegen::testing_detail::` rather than the production -// `numsim::codegen::` namespace — both as a marker ("if you're writing -// production code and naming this, you've taken a wrong turn") and to -// keep the friend's reachable surface narrow from production TUs. - -} // namespace numsim::codegen - -namespace numsim::codegen::testing_detail { - -struct state_variable_alignment_access { - // Clobber the name of the SymbolDecl pointed at by a state variable's - // current/old index. Simulates a mutating pass renaming a symbol - // without keeping the pair in sync. - static void poison_symbol_name(ConstitutiveModel &m, std::size_t idx, - std::string new_name) { - m.m_symbols[idx].name = std::move(new_name); - } - // Clobber the category of a symbol. - static void poison_symbol_category(ConstitutiveModel &m, std::size_t idx, - SymbolDecl::Category new_cat) { - m.m_symbols[idx].category = new_cat; - } - // Clobber the kind of a symbol. - static void poison_symbol_kind(ConstitutiveModel &m, std::size_t idx, - SymbolDecl::Kind new_kind) { - m.m_symbols[idx].kind = new_kind; - } - // Clobber the dim of a tensor symbol. - static void poison_symbol_dim(ConstitutiveModel &m, std::size_t idx, - std::size_t new_dim) { - m.m_symbols[idx].dim = new_dim; - } - // Push the current_symbol_idx of a state variable past the end of - // m_symbols to simulate a vector-shrink that left stale indices. - static void poison_state_var_current_idx(ConstitutiveModel &m, - std::size_t sv_idx, - std::size_t new_idx) { - m.m_state_variables[sv_idx].current_symbol_idx = new_idx; - } -}; - -} // namespace numsim::codegen::testing_detail - -namespace numsim::codegen { - -// Shorthand for the negative-path tests below. -using poison = testing_detail::state_variable_alignment_access; +// the internals directly via the `testing_detail::state_variable_alignment_access` +// friend struct. Its definition lives in `tests/internal/` (PR #66 +// round-3 #7) so future test TUs that need the same access include the +// shared header rather than redefining the struct — making the ODR +// contract mechanical, not social. + +namespace { +// Anonymous-namespace alias (PR #66 round-3 #8) so a future test TU +// using `using poison = ...` for a different access struct doesn't +// quietly mean different things across TUs. +using sv_align_poison = + testing_detail::state_variable_alignment_access; +} // namespace TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionOutOfBoundsIdx) { // PR #66 round-2 #7: single try/catch + FAIL() — silent test-pass on @@ -506,7 +528,7 @@ TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionOutOfBoundsIdx) { ConstitutiveModel model("M"); (void)model.add_scalar_state_variable( "alpha", cas::make_expression(0.0)); - poison::poison_state_var_current_idx(model, 0, 999); + sv_align_poison::poison_state_var_current_idx(model, 0, 999); try { model.validate(); FAIL() << "expected std::runtime_error from alignment OOB arm"; @@ -526,7 +548,7 @@ TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionRenamedSymbol) { (void)model.add_scalar_state_variable( "alpha", cas::make_expression(0.0)); auto const cur_idx = model.state_variables()[0].current_symbol_idx; - poison::poison_symbol_name(model, cur_idx, "beta"); + sv_align_poison::poison_symbol_name(model, cur_idx, "beta"); try { model.validate(); FAIL() << "expected std::runtime_error from alignment name arm"; @@ -546,7 +568,7 @@ TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionWrongCategory) { (void)model.add_scalar_state_variable( "alpha", cas::make_expression(0.0)); auto const cur_idx = model.state_variables()[0].current_symbol_idx; - poison::poison_symbol_category(model, cur_idx, + sv_align_poison::poison_symbol_category(model, cur_idx, SymbolDecl::Category::Input); try { model.validate(); @@ -569,7 +591,7 @@ TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionWrongKind) { (void)model.add_scalar_state_variable( "alpha", cas::make_expression(0.0)); auto const cur_idx = model.state_variables()[0].current_symbol_idx; - poison::poison_symbol_kind(model, cur_idx, SymbolDecl::Kind::Tensor); + sv_align_poison::poison_symbol_kind(model, cur_idx, SymbolDecl::Kind::Tensor); try { model.validate(); FAIL() << "expected std::runtime_error from alignment kind arm"; @@ -592,7 +614,7 @@ TEST(StateVariablePhase22Prep, AlignmentDetectsCorruptionWrongDim) { auto eps_p_init = cas::make_expression(3, 2); (void)model.add_tensor_state_variable("eps_p", 3, 2, eps_p_init); auto const cur_idx = model.state_variables()[0].current_symbol_idx; - poison::poison_symbol_dim(model, cur_idx, 2); + sv_align_poison::poison_symbol_dim(model, cur_idx, 2); try { model.validate(); FAIL() << "expected std::runtime_error from alignment dim arm"; diff --git a/tests/internal/state_variable_alignment_access.h b/tests/internal/state_variable_alignment_access.h new file mode 100644 index 0000000..f1f9449 --- /dev/null +++ b/tests/internal/state_variable_alignment_access.h @@ -0,0 +1,68 @@ +#ifndef NUMSIM_CODEGEN_TESTS_INTERNAL_STATE_VARIABLE_ALIGNMENT_ACCESS_H +#define NUMSIM_CODEGEN_TESTS_INTERNAL_STATE_VARIABLE_ALIGNMENT_ACCESS_H + +// Test-only friend access to `ConstitutiveModel`'s private members for +// negative-path tests of the StateVariable ↔ SymbolDecl alignment +// invariant. PR #66 round-3 review #7. +// +// **Why this header exists.** `ConstitutiveModel` grants `friend` to +// `numsim::codegen::testing_detail::state_variable_alignment_access`. +// If two test TUs each defined that struct independently — even with +// the same name and same body — the program would be ODR-violating +// ([basic.def.odr]/12: at most one definition of a non-inline class). +// Factoring the definition here gives the test binary a single ODR +// slot; future test TUs that need the same access `#include` this +// header rather than redefining the struct. +// +// Methods are `static` so the struct itself is never instantiated; +// each method is `inline` so multiple TU includes link cleanly. +// Production code paths cannot accidentally rely on this — the header +// lives under `tests/internal/` and isn't on the public include path. + +#include + +#include +#include +#include + +namespace numsim::codegen::testing_detail { + +struct state_variable_alignment_access { + // Clobber the name of the SymbolDecl pointed at by a state variable's + // current/old index. Simulates a mutating pass renaming a symbol + // without keeping the pair in sync. + static inline void poison_symbol_name(ConstitutiveModel &m, + std::size_t idx, + std::string new_name) { + m.m_symbols[idx].name = std::move(new_name); + } + // Clobber the category of a symbol. + static inline void poison_symbol_category(ConstitutiveModel &m, + std::size_t idx, + SymbolDecl::Category new_cat) { + m.m_symbols[idx].category = new_cat; + } + // Clobber the kind of a symbol. + static inline void poison_symbol_kind(ConstitutiveModel &m, + std::size_t idx, + SymbolDecl::Kind new_kind) { + m.m_symbols[idx].kind = new_kind; + } + // Clobber the dim of a tensor symbol. + static inline void poison_symbol_dim(ConstitutiveModel &m, + std::size_t idx, + std::size_t new_dim) { + m.m_symbols[idx].dim = new_dim; + } + // Push the current_symbol_idx of a state variable past the end of + // m_symbols to simulate a vector-shrink that left stale indices. + static inline void poison_state_var_current_idx(ConstitutiveModel &m, + std::size_t sv_idx, + std::size_t new_idx) { + m.m_state_variables[sv_idx].current_symbol_idx = new_idx; + } +}; + +} // namespace numsim::codegen::testing_detail + +#endif // NUMSIM_CODEGEN_TESTS_INTERNAL_STATE_VARIABLE_ALIGNMENT_ACCESS_H