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 7bc88d09..ff0775d2 100644 --- a/include/morph/core/registry.hpp +++ b/include/morph/core/registry.hpp @@ -381,6 +381,21 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio // NOLINTBEGIN(bugprone-macro-parentheses) +// 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`. +// `__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) + /// @brief Registers model type @p M with the string id @p NAME. /// /// Specialises `morph::model::ModelTraits` and registers a factory with the @@ -394,7 +409,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_, __COUNTER__) = \ + morph::model::detail::registerModelOnce(NAME); \ } /// @brief Registers action type @p A (for model @p M) with the string id @p NAME. @@ -475,9 +491,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_, __COUNTER__) = \ 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_, __COUNTER__) = \ morph::model::detail::registerActionExecutorOnce(morph::model::ModelTraits::typeId(), NAME); \ } /// @endcond diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 6ccaa781..7fe1ac70 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -18,6 +18,8 @@ add_executable(morph_tests test_observability.cpp 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_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})"); +} 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); +}