Skip to content

Replace role enums with attribute-bearing Role struct - #16

Merged
petlenz merged 4 commits into
mainfrom
role-refactor
May 22, 2026
Merged

Replace role enums with attribute-bearing Role struct#16
petlenz merged 4 commits into
mainfrom
role-refactor

Conversation

@petlenz

@petlenz petlenz commented May 21, 2026

Copy link
Copy Markdown
Member

Summary

Replace InputRole / OutputRole enums with a single attribute-bearing Role value type. Users can construct custom roles inline without library edits; backends switch on attributes (is_stateful, is_symmetric, …) rather than enum identity.

Closes the role-semantics item (ARCH-C2) in #14. Surfaces a Phase A asymmetry tracked separately in #15.

Why

The enum design forced a library edit (and a backend switch update) every time a user wanted a new semantic role. The MOOSE Phase A guard could only catch the literal InputRole::History — a user-defined custom history-like role would slip through unnoticed.

What changed

  • include/numsim_codegen/recipe.henum class InputRole and enum class OutputRole replaced by one struct Role { string_view name; bool is_stateful, is_driving, is_symmetric; optional<size_t> expected_rank; }. Name-based operator==. Predefined catalogue in namespace numsim::codegen::roles (Strain, StrainIncrement, DeformationGradient, Stress, ConsistentTangent, Temperature, History, Dissipation, Other). SymbolDecl::role and OutputDecl::role are now Role. add_* and find_*_by_role take Role.
  • include/numsim_codegen/targets/moose_material.h — Phase A guard becomes if (i.role.is_stateful), catching any user-defined stateful role rather than only the literal History enumerator. The input_role_doc() switch is gone; the validParams docstring uses i.role.name directly.
  • Tests + exampleInputRole::Strainroles::Strain, OutputRole::Stressroles::Stress. No semantic test changes.

User-facing ergonomics

// Predefined:
model.add_tensor_input("eps", 3, 2, roles::Strain);

// User-defined, no library edit needed:
auto phi = model.add_scalar_input("phi",
    Role{.name="phase_field", .is_driving=true, .expected_rank=0});

// Stateful custom role caught by the MOOSE Phase A guard automatically:
auto pl = model.add_tensor_input("plastic_strain", 3, 2,
    Role{.name="plastic_strain", .is_stateful=true});
// → throws "stateful role 'plastic_strain' on input 'plastic_strain' requires
//    the History machinery ... See the numsim-codegen Phase B roadmap."

Scope preserved

The Phase A guard inspects only inputs (matches pre-refactor behavior exactly). Stateful outputs are still silently accepted — tracked separately in #15 with the right fix to bundle alongside the negative-test infrastructure in #11.

Test plan

  • All 28 tests pass (ctest --output-on-failure)
  • Examples build (linear_elasticity, moose_linear_elastic)
  • Compile-check generated driver passes

@petlenz

petlenz commented May 21, 2026

Copy link
Copy Markdown
Member Author

Critical review findings

Self-review pulled in via specialist agent. Three real issues worth addressing before merge, two follow-ups.

Must fix in this PR

[1] High — no enforcement of the name-uniqueness contract. Role::operator== compares names only, but nothing in the code rejects a user-constructed Role{.name="strain", .is_driving=false}. Such a role compares equal to roles::Strain but carries wrong semantics; find_input_by_role(roles::Strain) would return it, backends would read the wrong attributes, silent miscompute. The PR comment says "don't redefine the canonical names with different attribute values" but it's load-bearing. Add a check in validate() (or in each add_*) that compares all attributes when a user role's name collides with a roles:: catalogue entry, and throws on mismatch.

[2] High — std::string_view UB on temporary std::string. Role::name is string_view. Role{.name = std::string("damage")} (or Role{.name = some_local_string} that goes out of scope) dangles. The compiler issues no warning. Either store std::string (Role is registered once per declaration, copy cost is negligible) or add Role(std::string &&) = delete to block the worst case. Recommended: switch to std::string — simpler and correct.

[5] Med — role.name injected unsanitized into a C++ string literal. `moose_material.h` emits "Coupled " << i.role.name inside a params.addRequiredParam<...>(..., "Coupled <name>") C++ literal. A user role name containing " or \ breaks the generated .C. Pre-existing for i.name / model.name(), but this PR newly exposes user-controlled role.name to the same vector. Add an escape_for_string_literal helper and apply it to all user-controlled strings landing in C++ literals. Cross-ref #14 → CORR-B4 (general identifier validation).

Follow-ups (don't block merge)

[4] Stateful-output asymmetry comment. The output loop in MooseMaterialTarget::emit() doesn't guard is_stateful outputs (tracked in #15). Add a one-line // NOTE: outputs not guarded — see #15 at the output-loop site, not just in the class doc.

[6] Test gap. No tests cover (a) custom user-defined Role round-tripping through the MOOSE backend, (b) emit() throwing for a non-roles::History stateful role, (c) find_*_by_role with a custom role. All three are the central new capabilities of this PR — a small RoleFlexibilityTest.cpp would close the gap.

Confirmed not issues

  • ODR for inline constexpr Role: safe (C++17 inline-var guarantee + name-based comparison works across TUs).
  • input_role_doc() removal: was strictly private static, no public exposure lost.

@petlenz

petlenz commented May 21, 2026

Copy link
Copy Markdown
Member Author

Addressed in fc938dd. All five points handled in the PR; tests went from 28 → 34.

[1] High — name-uniqueness contract enforced. Added ConstitutiveModel::validate_role_attributes(Role const &) private helper, called from every add_scalar_input / add_tensor_input / add_output. It compares attribute-by-attribute against the nine roles:: catalogue entries when names match and throws with a clear message naming the colliding constant. Fails at the offending add_* call (eager), not at emit time. Test: RoleFlexibility.RejectsNameCollisionWithCatalogueButMismatchedAttrs.

[2] High — Role::name now owns the string. Switched from std::string_view to std::string. The catalogue dropped inline constexpr for inline const (`std::string` isn't a portable literal type — inline still gives single-definition across TUs). Test: RoleFlexibility.RoleNameSurvivesTemporaryStringSource constructs a Role from a local string that goes out of scope, then mutates the original to detect aliasing.

[5] Med — escape_for_cpp_literal helper added. Static private method on MooseMaterialTarget. Currently used at the one new injection vector this PR introduced (the validParams docstring). Pre-existing fields (i.name, p.doc, model.name()) have the same exposure but are out of scope here — cross-ref #14 → CORR-B4 for the general-identifier validator that should cover them.

[4] Med — output asymmetry NOTE. Added a multi-line NOTE comment co-located with the input loop in MooseMaterialTarget::emit(), explicitly pointing at issue #15 with instructions for the future Phase B work ("mirror the pattern below").

[6] Med — new test file tests/RoleFlexibilityTest.cpp. Six tests covering: custom role round-trip through MOOSE backend, stateful-throw for non-roles::History role, find_input_by_role with custom role, name-collision attribute mismatch, exact catalogue rebuild accepted, and the lifetime test mentioned above.

Confirmed-not-issues already had no action items.

Build: 34/34 tests pass on local-cas configuration.

Note for #26: this push includes header changes (escape helper) that #26 rebase will need to carry into src/targets/moose_material.cpp since #26 moves the methods out of the header. Will handle in the #26 update.

@petlenz

petlenz commented May 21, 2026

Copy link
Copy Markdown
Member Author

Second-pass fixes in a787c69.

[3] Critical — newline/CR/NUL escape — fixed. `escape_for_cpp_literal` now handles `"`, `\`, `\n`, `\r`, `\0` via a switch. Added `RoleNameWithSpecialCharsIsEscapedInGeneratedSource` test that constructs a role with embedded quote, newline, and backslash; asserts the escaped form appears in the generated source and the raw forms don't.

[2]/[6d] Med — dynamic-init order — documented. Added a multi-line NOTE on the `roles::` namespace explaining that calling `add_*` from another TU's static-init phase is unsafe (catalogue globals dynamically initialised). Documenting rather than enforcing — moving to constinit-friendly types would require dropping `std::string`, which is the core of the [2] fix.

[6b] Nit — double-copy in add_* — fixed. Pushed into the `*_cache` first (copy), then `std::move` into `m_symbols`. Saves one `SymbolDecl` copy per declaration (one `std::string` for the name plus another for the role name).

[5] Test style/coverage — not changed. The `try/catch` pattern is internally consistent across the exception-checking tests; gmock matchers would be cleaner but require linking gmock which the test target currently doesn't. Lifetime test mechanism reviewed and is structurally sound (mutating `local_name` after the call would surface a string_view bug since the test reads `found->role.name` afterward).

37/37 tests pass.

@petlenz

petlenz commented May 21, 2026

Copy link
Copy Markdown
Member Author

Third-pass nit fixed in 8898972: \0 escape comment now correctly says "embeds a null byte in the string value, which silently truncates downstream C-string APIs" (was "silently truncates the literal at the null byte", which was imprecise — the escape produces a null byte inside the string, it doesn't truncate the literal itself). Logic unchanged, tests still 35/35.

@petlenz
petlenz merged commit 7eeff4f into main May 22, 2026
4 checks passed
@petlenz
petlenz deleted the role-refactor branch May 22, 2026 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant