From 06a9a4c4f813873bb54c7f1239768eb1a9a2010c Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 3 Aug 2026 19:04:33 +0300 Subject: [PATCH 1/2] core: key registration macro identifiers on __LINE__, not type spelling BRIDGE_REGISTER_MODEL/BRIDGE_REGISTER_ACTION built their generated static registrar names by token-pasting the model/action type directly onto a fixed prefix (bridge_model_reg_##M, bridge_action_reg_##M##_##A). That only produces a valid identifier when both arguments are bare identifiers -- a namespace-qualified type pastes ':' characters into the token and fails to compile, with the error pointing at the macro expansion rather than the call site. Key the generated names on __LINE__ instead (via the standard two-level CAT indirection so it expands before pasting). There is exactly one registration per macro invocation, so __LINE__ is equally unique, and it no longer depends on how the type is written. Unqualified registrations keep compiling unchanged. Fixes #21 Signed-off-by: Yaraslau Tamashevich --- include/morph/core/registry.hpp | 17 +++++-- tests/CMakeLists.txt | 1 + tests/test_registration_qualified_types.cpp | 50 +++++++++++++++++++++ 3 files changed, 65 insertions(+), 3 deletions(-) create mode 100644 tests/test_registration_qualified_types.cpp diff --git a/include/morph/core/registry.hpp b/include/morph/core/registry.hpp index 7bc88d09..03c5b17f 100644 --- a/include/morph/core/registry.hpp +++ b/include/morph/core/registry.hpp @@ -381,6 +381,16 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio // NOLINTBEGIN(bugprone-macro-parentheses) +// Keying the generated registrar names on `__LINE__` (rather than on the type spelling) +// keeps them valid identifiers regardless of how `M`/`A` are written. A namespace-qualified +// or template type (e.g. `app::models::Report`) would otherwise be pasted directly into the +// identifier, producing invalid tokens like `bridge_model_reg_app::models::Report`. There is +// exactly one registration per macro invocation, so `__LINE__` is equally unique. The extra +// indirection (`_CAT` calling `_CAT_`) is required so `__LINE__` is expanded to its numeric +// value before the paste, rather than being pasted as the literal text "__LINE__". +#define BRIDGE_DETAIL_CAT_(a, b) a##b +#define BRIDGE_DETAIL_CAT(a, b) BRIDGE_DETAIL_CAT_(a, b) + /// @brief Registers model type @p M with the string id @p NAME. /// /// Specialises `morph::model::ModelTraits` and registers a factory with the @@ -394,7 +404,8 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio static constexpr std::string_view typeId() noexcept { return NAME; } \ }; \ namespace { \ - [[maybe_unused]] const bool bridge_model_reg_##M = morph::model::detail::registerModelOnce(NAME); \ + [[maybe_unused]] const bool BRIDGE_DETAIL_CAT(bridge_model_reg_, __LINE__) = \ + morph::model::detail::registerModelOnce(NAME); \ } /// @brief Registers action type @p A (for model @p M) with the string id @p NAME. @@ -475,9 +486,9 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio } \ }; \ namespace { \ - [[maybe_unused]] const bool bridge_action_reg_##M##_##A = \ + [[maybe_unused]] const bool BRIDGE_DETAIL_CAT(bridge_action_reg_, __LINE__) = \ morph::model::detail::registerActionOnce(morph::model::ModelTraits::typeId(), NAME); \ - [[maybe_unused]] const bool bridge_action_exec_reg_##M##_##A = \ + [[maybe_unused]] const bool BRIDGE_DETAIL_CAT(bridge_action_exec_reg_, __LINE__) = \ morph::model::detail::registerActionExecutorOnce(morph::model::ModelTraits::typeId(), NAME); \ } /// @endcond diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 6ccaa781..70f115df 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -18,6 +18,7 @@ add_executable(morph_tests test_observability.cpp test_backend_extra.cpp test_registry_extra.cpp + test_registration_qualified_types.cpp test_bridge_local.cpp test_bridge_remote.cpp test_bridge_execute_json.cpp diff --git a/tests/test_registration_qualified_types.cpp b/tests/test_registration_qualified_types.cpp new file mode 100644 index 00000000..3bf728b2 --- /dev/null +++ b/tests/test_registration_qualified_types.cpp @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Regression test for issue #21: BRIDGE_REGISTER_MODEL / BRIDGE_REGISTER_ACTION built the name +// of their generated static registrar by token-pasting the model/action type onto a fixed +// prefix (`bridge_model_reg_##M`, `bridge_action_reg_##M##_##A`). That only produces a valid +// identifier when both arguments are bare identifiers -- a namespace-qualified type pastes ':' +// characters into the token and fails to compile. This file registers a namespace-qualified +// model and action exactly as the issue describes; it must compile (and the runtime dispatch +// below must succeed) for the fix to be verified. + +#include +#include + +#include +#include +#include + +namespace issue21::models { + +struct ReportCreateResult { + bool ok = false; +}; + +struct ReportCreate { + int id = 0; +}; + +class Report { +public: + ReportCreateResult execute(const ReportCreate& action) { return ReportCreateResult{.ok = action.id > 0}; } +}; + +} // namespace issue21::models + +// Namespace-qualified model and action -- this is exactly the case that used to fail to +// compile (the generated identifiers `bridge_model_reg_issue21::models::Report` and +// `bridge_action_reg_issue21::models::Report_issue21::models::ReportCreate` are not valid +// tokens). +BRIDGE_REGISTER_MODEL(issue21::models::Report, "Issue21_Report") +BRIDGE_REGISTER_ACTION(issue21::models::Report, issue21::models::ReportCreate, "Issue21_Create") + +TEST_CASE("BRIDGE_REGISTER_MODEL/BRIDGE_REGISTER_ACTION compile and register namespace-qualified types", + "[registry][issue21]") { + auto holder = morph::model::detail::ModelRegistryFactory::instance().create("Issue21_Report"); + REQUIRE(holder != nullptr); + + auto result = morph::model::detail::ActionDispatcher::instance().dispatch("Issue21_Report", "Issue21_Create", + *holder, R"({"id":5})"); + REQUIRE(result == R"({"ok":true})"); +} From 90ce3e205b6f0506637c90ad6189fd89cc7c9d9b Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 3 Aug 2026 21:57:52 +0300 Subject: [PATCH 2/2] core: fix cross-file registrar-name collision from __LINE__ keying BRIDGE_REGISTER_MODEL/BRIDGE_REGISTER_ACTION keyed their generated anonymous-namespace registrar variables on __LINE__ to avoid pasting a namespace-qualified or template type into the identifier (issue #21). __LINE__ is only unique within a single physical file, though: two different headers that each invoke one of these macros on the same line number produce the same generated identifier once both are transitively #include'd into one translation unit, which is a hard redefinition error since C++ unnamed namespaces are per-TU, not per-file. This is exactly what broke the WASM demo build in CI (multiple bank model headers, each registering on the same line number, pulled into one autogen TU). Switch the key from __LINE__ to __COUNTER__, which increments monotonically across the whole translation unit regardless of which file expands it, so it cannot collide this way. Add tests/test_registration_same_line.cpp (with companion headers issue21_same_line_{a,b}.hpp, whose BRIDGE_REGISTER_MODEL invocations are deliberately pinned to the same physical line number) as a regression test; confirmed it fails with the redefinition error under __LINE__ and passes under __COUNTER__. Updated docs/spec/core/registry.md with the naming-scheme rationale. Signed-off-by: Yaraslau Tamashevich --- docs/spec/core/registry.md | 14 ++++++++++++++ include/morph/core/registry.hpp | 21 +++++++++++++-------- tests/CMakeLists.txt | 1 + tests/issue21_same_line_a.hpp | 19 +++++++++++++++++++ tests/issue21_same_line_b.hpp | 19 +++++++++++++++++++ tests/test_registration_same_line.cpp | 26 ++++++++++++++++++++++++++ 6 files changed, 92 insertions(+), 8 deletions(-) create mode 100644 tests/issue21_same_line_a.hpp create mode 100644 tests/issue21_same_line_b.hpp create mode 100644 tests/test_registration_same_line.cpp diff --git a/docs/spec/core/registry.md b/docs/spec/core/registry.md index 99c4378b..9a234fa1 100644 --- a/docs/spec/core/registry.md +++ b/docs/spec/core/registry.md @@ -469,6 +469,20 @@ variables). ## Registration macros +Both macros below name their generated anonymous-namespace variable by pasting +a fixed prefix onto `__COUNTER__` (via the two-level `BRIDGE_DETAIL_CAT`/ +`BRIDGE_DETAIL_CAT_` indirection needed to force macro expansion before the +paste), not onto the spelling of `M`/`A`. Pasting the type directly (the +original approach) breaks for namespace-qualified or template types — +`app::models::Report` pastes `::` into the identifier. `__LINE__` was tried as +a replacement key but is only unique *within a single physical file*; two +different headers that each invoke one of these macros on the same line +number produce the same identifier once both are transitively `#include`d +into one translation unit, which is a hard redefinition error because C++ +unnamed namespaces are per-TU, not per-file. `__COUNTER__` increments +monotonically across the whole translation unit regardless of which file +expands it, so it cannot collide this way. + ### `BRIDGE_REGISTER_MODEL(M, NAME)` Specialises `ModelTraits` and registers a factory at static-init time. diff --git a/include/morph/core/registry.hpp b/include/morph/core/registry.hpp index 03c5b17f..ff0775d2 100644 --- a/include/morph/core/registry.hpp +++ b/include/morph/core/registry.hpp @@ -381,13 +381,18 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio // NOLINTBEGIN(bugprone-macro-parentheses) -// Keying the generated registrar names on `__LINE__` (rather than on the type spelling) +// Keying the generated registrar names on `__COUNTER__` (rather than on the type spelling) // keeps them valid identifiers regardless of how `M`/`A` are written. A namespace-qualified // or template type (e.g. `app::models::Report`) would otherwise be pasted directly into the -// identifier, producing invalid tokens like `bridge_model_reg_app::models::Report`. There is -// exactly one registration per macro invocation, so `__LINE__` is equally unique. The extra -// indirection (`_CAT` calling `_CAT_`) is required so `__LINE__` is expanded to its numeric -// value before the paste, rather than being pasted as the literal text "__LINE__". +// identifier, producing invalid tokens like `bridge_model_reg_app::models::Report`. +// `__LINE__` was tried first, but it is only unique *within a single physical file*: two +// headers that each invoke this macro on the same line number (e.g. `card_model.hpp:38` and +// `payment_model.hpp:38`) produce the same identifier once both are `#include`d into one +// translation unit, and since C++ unnamed namespaces are per-TU (not per-file), that is a +// hard redefinition error. `__COUNTER__` increments monotonically for the entire translation +// unit regardless of which file expands it, so it cannot collide this way. The extra +// indirection (`_CAT` calling `_CAT_`) is required so `__COUNTER__` is expanded to its numeric +// value before the paste, rather than being pasted as the literal text "__COUNTER__". #define BRIDGE_DETAIL_CAT_(a, b) a##b #define BRIDGE_DETAIL_CAT(a, b) BRIDGE_DETAIL_CAT_(a, b) @@ -404,7 +409,7 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio static constexpr std::string_view typeId() noexcept { return NAME; } \ }; \ namespace { \ - [[maybe_unused]] const bool BRIDGE_DETAIL_CAT(bridge_model_reg_, __LINE__) = \ + [[maybe_unused]] const bool BRIDGE_DETAIL_CAT(bridge_model_reg_, __COUNTER__) = \ morph::model::detail::registerModelOnce(NAME); \ } @@ -486,9 +491,9 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio } \ }; \ namespace { \ - [[maybe_unused]] const bool BRIDGE_DETAIL_CAT(bridge_action_reg_, __LINE__) = \ + [[maybe_unused]] const bool BRIDGE_DETAIL_CAT(bridge_action_reg_, __COUNTER__) = \ morph::model::detail::registerActionOnce(morph::model::ModelTraits::typeId(), NAME); \ - [[maybe_unused]] const bool BRIDGE_DETAIL_CAT(bridge_action_exec_reg_, __LINE__) = \ + [[maybe_unused]] const bool BRIDGE_DETAIL_CAT(bridge_action_exec_reg_, __COUNTER__) = \ morph::model::detail::registerActionExecutorOnce(morph::model::ModelTraits::typeId(), NAME); \ } /// @endcond diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 70f115df..7fe1ac70 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -19,6 +19,7 @@ add_executable(morph_tests test_backend_extra.cpp test_registry_extra.cpp test_registration_qualified_types.cpp + test_registration_same_line.cpp test_bridge_local.cpp test_bridge_remote.cpp test_bridge_execute_json.cpp diff --git a/tests/issue21_same_line_a.hpp b/tests/issue21_same_line_a.hpp new file mode 100644 index 00000000..1dde49f6 --- /dev/null +++ b/tests/issue21_same_line_a.hpp @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Companion header for test_registration_same_line.cpp -- see that file for the scenario this +// reproduces. The BRIDGE_REGISTER_MODEL invocation below must stay on the exact same physical +// line number as the one in issue21_same_line_b.hpp for the two headers to exercise the bug. + +#pragma once + +#include + +namespace issue21::sameline { + +struct WidgetA { + int execute(int x) { return x + 1; } +}; + +} // namespace issue21::sameline + +BRIDGE_REGISTER_MODEL(issue21::sameline::WidgetA, "Issue21_SameLine_WidgetA") diff --git a/tests/issue21_same_line_b.hpp b/tests/issue21_same_line_b.hpp new file mode 100644 index 00000000..88a0d2c3 --- /dev/null +++ b/tests/issue21_same_line_b.hpp @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Companion header for test_registration_same_line.cpp -- see that file for the scenario this +// reproduces. The BRIDGE_REGISTER_MODEL invocation below must stay on the exact same physical +// line number as the one in issue21_same_line_a.hpp for the two headers to exercise the bug. + +#pragma once + +#include + +namespace issue21::sameline { + +struct WidgetB { + int execute(int x) { return x + 2; } +}; + +} // namespace issue21::sameline + +BRIDGE_REGISTER_MODEL(issue21::sameline::WidgetB, "Issue21_SameLine_WidgetB") diff --git a/tests/test_registration_same_line.cpp b/tests/test_registration_same_line.cpp new file mode 100644 index 00000000..1eeb4f9f --- /dev/null +++ b/tests/test_registration_same_line.cpp @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Regression test for issue #21 (follow-up): keying the generated registrar variable name on +// `__LINE__` fixed the original namespace-qualified-type bug but introduced a new one -- +// `__LINE__` is only unique within a single physical file, so two different headers that each +// invoke BRIDGE_REGISTER_MODEL/BRIDGE_REGISTER_ACTION on the same line number produce the same +// generated identifier once both are `#include`d into one translation unit. Since C++ unnamed +// namespaces are per-translation-unit (not per-file), that is a hard redefinition error -- this +// is exactly what broke the WASM demo build (multiple bank model headers, each registering on +// the same line number, all pulled into one autogen TU). issue21_same_line_a.hpp and +// issue21_same_line_b.hpp each invoke BRIDGE_REGISTER_MODEL on line 19; this file must compile +// (and both models must be independently registered) for the fix (`__COUNTER__` instead of +// `__LINE__`) to be verified. + +#include "issue21_same_line_a.hpp" +#include "issue21_same_line_b.hpp" + +#include + +TEST_CASE("BRIDGE_REGISTER_MODEL on the same line number in two different headers does not collide", + "[registry][issue21]") { + auto holderA = morph::model::detail::ModelRegistryFactory::instance().create("Issue21_SameLine_WidgetA"); + auto holderB = morph::model::detail::ModelRegistryFactory::instance().create("Issue21_SameLine_WidgetB"); + REQUIRE(holderA != nullptr); + REQUIRE(holderB != nullptr); +}