From 8cb612085047ca6fe35ac308aa6606aaf2c1907e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 7 Jul 2026 19:48:44 +0000 Subject: [PATCH 1/3] Reject polymorphic reflectable records and document the layout constraint (#21) Member offsets are derived by OffsetFromStart (reinterprets the leading bytes of a pointer-to-member as a byte offset) and DEFINE_MEMBER (offsetof(struct REFLECTABLE, R)). Both are only valid for simple, non-inherited, non-polymorphic structs; a record that violates this (e.g. given a virtual function or a base class) computes wrong offsets silently - data corruption, not a compile or runtime error. Add static_assert(!std::is_polymorphic::value, ...) right after the macro-generated struct definition in include/reflection.h. This is the portable, always-safe guard: unlike std::is_standard_layout, it doesn't depend on whether std::wstring/TimePoint happen to be standard-layout on a given standard library (verified true on libstdc++ here; unverified on libc++/MSVC), so it can't fail to compile for a currently-valid record on any supported platform. Verified directly: a record with a FUNC(virtual ...) declaration - the realistic way a user would introduce a vtable into a REFLECTABLE struct - now fails to compile with a clear message, where previously offsetof would have only emitted a -Winvalid-offsetof warning and silently computed nonsense. Also verified (Linux/GCC/libstdc++, C++11 and C++20) that is_standard_layout does currently hold for wstring, TimePoint, and all four representative test records, matching the issue's measurement - but since that's unverified on libc++ (macOS) and MSVC (Windows) and this environment can't build for those toolchains, the stronger is_standard_layout assert is deferred to a separate, easily-isolated follow-up commit so CI can decide per-platform rather than guessing. Document the constraint in README.md ("Defining records") and with a comment block next to the REFLECTABLE struct definition. Add tests/reflection_test.cc with static_assert-based compile-time checks (plus a mirroring runtime test) that Person/Pet/Company/DatetimeContainer - one representative of each storage class - satisfy !is_polymorphic, to guard against regression. Full suite passes locally on Ubuntu/GCC in both C++11 and C++20 (75/75 tests); this is itself part of the proof the guard doesn't reject any currently-valid record. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK --- README.md | 8 ++++++ include/reflection.h | 15 ++++++++++++ tests/reflection_test.cc | 53 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+) create mode 100644 tests/reflection_test.cc diff --git a/README.md b/README.md index c450e47..cfca1bc 100644 --- a/README.md +++ b/README.md @@ -135,6 +135,14 @@ Supported field macros: | `sqlite_reflection::TimePoint` | `MEMBER_DATETIME(name)` | | member function declaration | `FUNC(signature)` | +**Layout constraint.** Reflectable records must be simple, standard-layout structs: no base +classes, no virtual functions, no virtual/multiple inheritance. Member access is computed from +`offsetof`/pointer-to-member byte offsets, which are only well-defined for such types; a struct +outside these bounds is rejected at compile time via a `static_assert` if it's polymorphic (has a +vtable), but other standard-layout violations are not otherwise detectable across all supported +compilers and would silently compute wrong member offsets instead of failing to compile. Stick to +plain data members declared through the `MEMBER_*` macros and you're always within these bounds. + Make sure each reflected record header is included by your program before `Database::Initialize()` is called. During initialization, the library creates one table for each registered record type if that table does not already exist. ## Opening and closing the database diff --git a/include/reflection.h b/include/reflection.h index 6b046de..022fcc1 100644 --- a/include/reflection.h +++ b/include/reflection.h @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -153,6 +154,11 @@ REFLECTION_EXPORT char* GetMemberAddress(void* p, const Reflection& record, size #pragma warning(push) #pragma warning(disable : 4002) // "too many actual parameters for macro 'MEMBER'" +/// Reflectable records must be simple, standard-layout structs: no base classes, no virtual +/// functions, no virtual inheritance. Member access is computed via offsetof/pointer-to-member +/// byte offsets (see OffsetFromStart and DEFINE_MEMBER above), which only give correct answers +/// for such types; a struct outside these bounds gets WRONG member offsets silently (data +/// corruption, not a compile or runtime error) unless caught by the static_assert below. struct REFLECTABLE_DLL_EXPORT REFLECTABLE { // member declaration according to the order given in source code #define MEMBER_DECLARE(L, R) L R; @@ -188,6 +194,15 @@ struct REFLECTABLE_DLL_EXPORT REFLECTABLE { #undef FUNC }; +// Reject the realistic footgun that actually breaks OffsetFromStart's pointer-to-member byte +// hack and offsetof's standard-layout requirement: giving a record a vtable via a virtual +// function or virtual/multiple inheritance. This does not depend on standard-library string +// layout (a struct with wstring members is never polymorphic on its own), so it holds on every +// supported compiler/platform. +static_assert(!std::is_polymorphic::value, + "sqlite-reflection: reflectable records must not be polymorphic " + "(no virtual functions or virtual/multiple inheritance)."); + /// Provide a static registration function for each reflectable struct static std::string CAT(Register, REFLECTABLE)() { std::string type_id = typeid(REFLECTABLE).name(); diff --git a/tests/reflection_test.cc b/tests/reflection_test.cc new file mode 100644 index 0000000..256d3be --- /dev/null +++ b/tests/reflection_test.cc @@ -0,0 +1,53 @@ +// MIT License +// +// Copyright (c) 2026 Ioannis Kaliakatsos +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +#include "reflection.h" + +#include + +#include + +#include "company.h" +#include "datetime_container.h" +#include "person.h" +#include "pet.h" + +using namespace sqlite_reflection; + +// Reflectable records must not be polymorphic (see include/reflection.h's static_assert next to +// the REFLECTABLE struct definition). These compile-time checks pin that guarantee for every +// storage class the test records exercise (TEXT/wstring, INT, REAL, BOOL, DATETIME/TimePoint), +// so a regression that reintroduces a virtual function/inheritance into the macro-generated +// struct fails the build here rather than silently corrupting member offsets at runtime. +static_assert(!std::is_polymorphic::value, "Person must not be polymorphic"); +static_assert(!std::is_polymorphic::value, "Pet must not be polymorphic"); +static_assert(!std::is_polymorphic::value, "Company must not be polymorphic"); +static_assert(!std::is_polymorphic::value, "DatetimeContainer must not be polymorphic"); + +// Mirrors the static_asserts above as ordinary runtime expectations, so the guarantee is also +// visible in normal test output rather than only enforced silently at compile time. +TEST(ReflectionTest, ReflectableRecordsAreNotPolymorphic) { + EXPECT_FALSE(std::is_polymorphic::value); + EXPECT_FALSE(std::is_polymorphic::value); + EXPECT_FALSE(std::is_polymorphic::value); + EXPECT_FALSE(std::is_polymorphic::value); +} From d4e4a67ba5591f7bf3f3abd0333cb123ab47a103 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 7 Jul 2026 19:50:58 +0000 Subject: [PATCH 2/3] Attempt the stronger is_standard_layout guard, isolated for CI (#21) Adds static_assert(std::is_standard_layout::value, ...) immediately after the polymorphic guard, as its own commit so a CI failure on any platform isolates cleanly to this one static_assert. is_standard_layout on REFLECTABLE is only true if std::wstring and TimePoint both happen to be standard-layout on the active standard library, which is implementation-defined. Verified true here on Ubuntu/GCC/libstdc++ in both C++11 and C++20 (matching the issue's own measurement), for all four representative test records (Person, Pet, Company, DatetimeContainer). NOT verified in this environment on libc++ (macOS) or MSVC (Windows) - this sandbox can only build for Linux/GCC. If CI is green on macOS and Windows (both C++11 and C++20) for this commit, #21 is fully closed. If it fails on either, revert just this commit, keeping the polymorphic guard from the prior commit, and note here that the standard-layout assert is blocked pending #25 (moving the text representation off std::wstring). Also adds a mirroring ReflectionTest.ReflectableRecordsAreStandardLayout runtime test alongside the compile-time check. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK --- include/reflection.h | 9 +++++++++ tests/reflection_test.cc | 11 +++++++++++ 2 files changed, 20 insertions(+) diff --git a/include/reflection.h b/include/reflection.h index 022fcc1..7c2200e 100644 --- a/include/reflection.h +++ b/include/reflection.h @@ -203,6 +203,15 @@ static_assert(!std::is_polymorphic::value, "sqlite-reflection: reflectable records must not be polymorphic " "(no virtual functions or virtual/multiple inheritance)."); +// Stronger guard, attempted separately from the polymorphic check above: is_standard_layout is +// only true for REFLECTABLE if std::wstring and sqlite_reflection::TimePoint both happen to be +// standard-layout on this standard library, which is implementation-defined and unverified here +// on libc++ (macOS) / MSVC (Windows) - verified true on libstdc++ (Linux). If this line fails to +// compile on any CI platform, that failure isolates to exactly this static_assert; remove it and +// keep only the polymorphic guard above (see #21 and #25). +static_assert(std::is_standard_layout::value, + "sqlite-reflection: reflectable records must be standard-layout."); + /// Provide a static registration function for each reflectable struct static std::string CAT(Register, REFLECTABLE)() { std::string type_id = typeid(REFLECTABLE).name(); diff --git a/tests/reflection_test.cc b/tests/reflection_test.cc index 256d3be..2efd250 100644 --- a/tests/reflection_test.cc +++ b/tests/reflection_test.cc @@ -51,3 +51,14 @@ TEST(ReflectionTest, ReflectableRecordsAreNotPolymorphic) { EXPECT_FALSE(std::is_polymorphic::value); EXPECT_FALSE(std::is_polymorphic::value); } + +// Mirrors the stronger is_standard_layout static_assert in include/reflection.h (attempted +// separately from the polymorphic guard, since it's only verified on libstdc++/Linux so far - +// see #21). If that assert is ever removed because a CI platform can't satisfy it, this test +// should be removed alongside it rather than weakened to keep passing. +TEST(ReflectionTest, ReflectableRecordsAreStandardLayout) { + EXPECT_TRUE(std::is_standard_layout::value); + EXPECT_TRUE(std::is_standard_layout::value); + EXPECT_TRUE(std::is_standard_layout::value); + EXPECT_TRUE(std::is_standard_layout::value); +} From 4ebcfd774adb472b5a31a8fe177c794295b8b677 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 7 Jul 2026 19:56:13 +0000 Subject: [PATCH 3/3] Revert the is_standard_layout guard: fails on MSVC (#21) CI confirms static_assert(std::is_standard_layout::value, ...) fails to compile on Windows Latest MSVC, in both C++11 and C++20, for every existing test record - std::wstring and/or sqlite_reflection::TimePoint are not standard-layout under MSVC's standard library. It compiled cleanly on Ubuntu/GCC/libstdc++ and macOS/Clang/libc++ (both C++11 and C++20). This confirms the exact risk the issue called out and the reason this guard was committed separately from the polymorphic one: is_standard_layout on a record is implementation-defined (true only if every member type, including wstring/TimePoint, is itself standard-layout), so it cannot be portably enforced today without breaking a real, currently-valid record on a supported compiler. Revert the assert and its mirroring test, leaving a comment recording what was tried and why, and keep only the portable static_assert(!std::is_polymorphic::value, ...) from the prior commit. #21 is closed by the polymorphic guard; the standard-layout strengthening is deferred pending #25 (moving the text representation off std::wstring). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK --- include/reflection.h | 16 ++++++++-------- tests/reflection_test.cc | 17 ++++++----------- 2 files changed, 14 insertions(+), 19 deletions(-) diff --git a/include/reflection.h b/include/reflection.h index 7c2200e..cf12e24 100644 --- a/include/reflection.h +++ b/include/reflection.h @@ -203,14 +203,14 @@ static_assert(!std::is_polymorphic::value, "sqlite-reflection: reflectable records must not be polymorphic " "(no virtual functions or virtual/multiple inheritance)."); -// Stronger guard, attempted separately from the polymorphic check above: is_standard_layout is -// only true for REFLECTABLE if std::wstring and sqlite_reflection::TimePoint both happen to be -// standard-layout on this standard library, which is implementation-defined and unverified here -// on libc++ (macOS) / MSVC (Windows) - verified true on libstdc++ (Linux). If this line fails to -// compile on any CI platform, that failure isolates to exactly this static_assert; remove it and -// keep only the polymorphic guard above (see #21 and #25). -static_assert(std::is_standard_layout::value, - "sqlite-reflection: reflectable records must be standard-layout."); +// A stronger static_assert(std::is_standard_layout::value, ...) was attempted here +// and confirmed via CI to fail to compile on MSVC (Windows), in both C++11 and C++20, for the +// existing test records - std::wstring and/or sqlite_reflection::TimePoint are not +// standard-layout on that standard library. It compiled fine on GCC/libstdc++ (Linux) and +// Clang/libc++ (macOS). Since is_standard_layout is implementation-defined and this project +// supports MSVC, that guard is not enforceable portably today; it's deferred pending #25 (moving +// the text representation off std::wstring). The polymorphic guard above remains the enforced, +// portable constraint. /// Provide a static registration function for each reflectable struct static std::string CAT(Register, REFLECTABLE)() { diff --git a/tests/reflection_test.cc b/tests/reflection_test.cc index 2efd250..55ff139 100644 --- a/tests/reflection_test.cc +++ b/tests/reflection_test.cc @@ -45,20 +45,15 @@ static_assert(!std::is_polymorphic::value, "DatetimeContainer // Mirrors the static_asserts above as ordinary runtime expectations, so the guarantee is also // visible in normal test output rather than only enforced silently at compile time. +// +// A stronger ReflectableRecordsAreStandardLayout test (mirroring +// static_assert(std::is_standard_layout::value, ...)) was attempted alongside this +// one and confirmed via CI to fail on MSVC (Windows), in both C++11 and C++20 - std::wstring +// and/or TimePoint are not standard-layout on that standard library - so it was removed; see the +// comment in include/reflection.h next to the REFLECTABLE struct definition. TEST(ReflectionTest, ReflectableRecordsAreNotPolymorphic) { EXPECT_FALSE(std::is_polymorphic::value); EXPECT_FALSE(std::is_polymorphic::value); EXPECT_FALSE(std::is_polymorphic::value); EXPECT_FALSE(std::is_polymorphic::value); } - -// Mirrors the stronger is_standard_layout static_assert in include/reflection.h (attempted -// separately from the polymorphic guard, since it's only verified on libstdc++/Linux so far - -// see #21). If that assert is ever removed because a CI platform can't satisfy it, this test -// should be removed alongside it rather than weakened to keep passing. -TEST(ReflectionTest, ReflectableRecordsAreStandardLayout) { - EXPECT_TRUE(std::is_standard_layout::value); - EXPECT_TRUE(std::is_standard_layout::value); - EXPECT_TRUE(std::is_standard_layout::value); - EXPECT_TRUE(std::is_standard_layout::value); -}