From 8137576335c823c89f2c4c69bc9d4977c87577bc Mon Sep 17 00:00:00 2001 From: Dario Gabriel Lipicar Date: Sat, 26 Sep 2026 22:40:06 -0300 Subject: [PATCH 1/4] fix(generator): spell a parameter named like the wrapper's own apart Every generated method appends its own `err` and deadline (`timeout_ms` on the lp surface, `timeout` on Qt), the async entry points a `callback`, and the bodies declare locals of their own. A contract parameter with one of those names was emitted verbatim: logos-rust-sdk's sdk_test_caller_module declares `timed_call(sleep_ms: int, timeout_ms: int)`, and its client failed with "redefinition of parameter 'timeout_ms'". A clashing parameter now gets `_` appended in C++ (cpp_identifier.h), never onto a name the contract already spells. Arguments are positional, so the wire is unchanged. The cdylib provider's event bodies get the same rule for `args` and `emitEventImpl_`. A contract without such names emits the same bytes. tests/generator now compiles what it generates (generated_code.h). Co-Authored-By: Claude Opus 5.5 --- README.md | 6 + cpp-generator/cpp_identifier.h | 33 ++++ cpp-generator/docs/project.md | 14 ++ .../experimental/lidl_gen_cdylib.cpp | 18 +- cpp-generator/generator_lib.cpp | 52 +++++- tests/generator/CMakeLists.txt | 22 +++ tests/generator/generated_code.h | 173 ++++++++++++++++++ tests/generator/test_reserved_param_names.cpp | 98 ++++++++++ 8 files changed, 404 insertions(+), 12 deletions(-) create mode 100644 cpp-generator/cpp_identifier.h create mode 100644 tests/generator/generated_code.h create mode 100644 tests/generator/test_reserved_param_names.cpp diff --git a/README.md b/README.md index ba1300d..35259f4 100644 --- a/README.md +++ b/README.md @@ -344,6 +344,12 @@ so the Qt and Qt-free emitters cannot drift. Anything else stays a value: a method may legitimately return a three-string map, and matching the shape alone would let user data impersonate a refusal. +**Contract names in C++.** A parameter named like one of the wrapper's own +(`err`, `timeout` / `timeout_ms`, `callback`) gets a trailing `_` in C++ only: +`method timed(sleep_ms: int, timeout_ms: int)` is +`timed(int64_t sleep_ms, int64_t timeout_ms_, logos::CallError* err = nullptr, int timeout_ms = 0)`. +The wire is unchanged. The rule is in `cpp-generator/docs/project.md` (*C++ names*). + ### Universal modules: LogosModuleContext Universal (codegen-driven) modules — those built from a plain `src/_impl.h` header rather than a handcrafted `QObject` plugin — don't see the raw `LogosAPI` at all. The contract is **derived from that header**: the module's ordinary public methods *are* its API, with no marker of any kind (there used to be a `LOGOS_METHOD` marker under `interface: "provider"`; both are gone). `metadata.json#codegen.impl_class` / `codegen.impl_header` name the class and the header when they differ from the defaults (`Impl` in `src/_impl.h`). Instead of a `LogosAPI`, the generated C-ABI export TU (`_module_impl.cpp`) populates a narrow `LogosModuleContext` base class with everything an impl typically needs: diff --git a/cpp-generator/cpp_identifier.h b/cpp-generator/cpp_identifier.h new file mode 100644 index 0000000..971d78b --- /dev/null +++ b/cpp-generator/cpp_identifier.h @@ -0,0 +1,33 @@ +#ifndef CPP_IDENTIFIER_H +#define CPP_IDENTIFIER_H + +// A LIDL name as a C++ identifier. The wire keeps the LIDL spelling; the C++ +// identifier gets `_` appended when it is a name the generated code declares +// in the same scope (`reserved`), until it is not and no other name of the +// scope spells it. A usable name never changes. + +#include +#include +#include + +// The C++ spellings of one scope's LIDL names, index-aligned with `names`. +inline QStringList lidlCppNames(const QStringList& names, const QSet& reserved = {}) +{ + auto clashes = [&](const QString& n) { return reserved.contains(n); }; + QSet taken; + for (const QString& n : names) + if (!clashes(n)) taken.insert(n); + QStringList out; + for (const QString& n : names) { + QString cpp = n; + if (clashes(cpp)) { + cpp += '_'; + while (clashes(cpp) || taken.contains(cpp)) cpp += '_'; + taken.insert(cpp); + } + out << cpp; + } + return out; +} + +#endif // CPP_IDENTIFIER_H diff --git a/cpp-generator/docs/project.md b/cpp-generator/docs/project.md index 31c3824..93d5b32 100644 --- a/cpp-generator/docs/project.md +++ b/cpp-generator/docs/project.md @@ -112,6 +112,19 @@ Per surface: | legacy consumer, **positional** slot (param, return, event param) | `QVariant` (Qt) / `LogosMap` (Lp) | still flattened — see Known Limitations | | header-first (`impl_header_parser`) | `std::optional` ↔ `?T` | `std::optional>` has no LIDL type; it collapses to `?T` and is reported on stderr | +### C++ names + +A LIDL name becomes a C++ identifier, but the wire keeps it as written: method names, +record field keys and event names are never renamed. Only the identifier changes, by one +rule (`cpp_identifier.h`): a name that clashes gets `_` appended until it no longer does +and no other name of its scope spells it. A name that does not clash never changes. + +A method **parameter** clashes with what the wrapper declares in the same scope: its own +`err` and deadline (`timeout_ms` on lp, `timeout` on Qt), the async `callback`, and the +locals and members its bodies use. So `method timed(sleep_ms: int, timeout_ms: int)` takes +`int64_t timeout_ms_` ahead of the wrapper's `int timeout_ms = 0`. A cdylib provider's +event bodies declare `args` and call `emitEventImpl_`, so its event parameters keep off both. + ### Client stubs (`lidl_gen_client.h/cpp`) - `lidlMakeHeader(ModuleDecl)` / `lidlMakeSource(ModuleDecl)` — typed `` client wrapper; each method (and its `…Async` twin) carries a Doxygen `///` comment generated from the method's `description` @@ -312,6 +325,7 @@ In `tests/generator/`, alongside the wrapper-emitter tests: | Test file | What it tests | |-----------|---------------| | `test_make_umbrella.cpp` | The `LogosModules` aggregate: both dependency forms on both API styles, that every member's type is included, dropped nameless entries, empty deps | +| `test_reserved_param_names.cpp` | Parameters named like the wrapper's own (`timeout_ms`, `err`, `callback`, …): spelled apart, and the lp wrapper and cdylib provider compile (`generated_code.h` runs this suite's compiler over the output) | Fixture files in `tests/experimental/fixtures/`: - `sample_impl.h` — module with all supported type variations diff --git a/cpp-generator/experimental/lidl_gen_cdylib.cpp b/cpp-generator/experimental/lidl_gen_cdylib.cpp index f97b2c4..0784c33 100644 --- a/cpp-generator/experimental/lidl_gen_cdylib.cpp +++ b/cpp-generator/experimental/lidl_gen_cdylib.cpp @@ -1,4 +1,5 @@ #include "lidl_gen_cdylib.h" +#include "cpp_identifier.h" #include "lidl_emit_common.h" #include @@ -1154,6 +1155,10 @@ QString lidlMakeEventsSourceCdylib(const ModuleDecl& module, // modules whose events carry no binary data. for (const EventDecl& ed : module.events) { + // The body declares `args` and calls `emitEventImpl_`; parameters keep off both. + QStringList pnames; + for (const ParamDecl& pd : ed.params) pnames << qs(pd.name); + pnames = lidlCppNames(pnames, {"args", "emitEventImpl_"}); s << "void " << implClass << "::" << ed.name << "("; for (int i = 0; i < ed.params.size(); ++i) { const QString stdType = lidlTypeToStdCdylib(ed.params[i].type, recsEv); @@ -1169,14 +1174,15 @@ QString lidlMakeEventsSourceCdylib(const ModuleDecl& module, || stdType.startsWith("std::optional") || isRecord(ed.params[i].type, recsEv) || stdType == "LogosMap" || stdType == "LogosList") - s << "const " << stdType << "& " << ed.params[i].name; + s << "const " << stdType << "& " << pnames.at(i); else - s << stdType << " " << ed.params[i].name; + s << stdType << " " << pnames.at(i); if (i + 1 < ed.params.size()) s << ", "; } s << ")\n{\n"; s << " nlohmann::json args = nlohmann::json::array();\n"; - for (const ParamDecl& pd : ed.params) { + for (int i = 0; i < ed.params.size(); ++i) { + const ParamDecl& pd = ed.params[i]; const QString evStd = lidlTypeToStdCdylib(pd.type, recsEv); // A record or a composite carrying bytes rides the generated codec, // exactly like a method return — otherwise an event payload would be @@ -1192,13 +1198,13 @@ QString lidlMakeEventsSourceCdylib(const ModuleDecl& module, || pd.type.kind == TypeExpr::Array || pd.type.kind == TypeExpr::Map || pd.type.kind == TypeExpr::Optional)) { s << " args.push_back(logos::toJson<" << evStd << ">(" - << pd.name << "));\n"; + << pnames.at(i) << "));\n"; continue; } if (pd.type.kind == TypeExpr::Primitive && pd.type.name == "bstr") - s << " args.push_back(logos::bytesToJson(" << pd.name << "));\n"; + s << " args.push_back(logos::bytesToJson(" << pnames.at(i) << "));\n"; else - s << " args.push_back(" << pd.name << ");\n"; + s << " args.push_back(" << pnames.at(i) << ");\n"; } s << " emitEventImpl_(\"" << ed.name << "\", &args);\n"; s << "}\n\n"; diff --git a/cpp-generator/generator_lib.cpp b/cpp-generator/generator_lib.cpp index 5a75d84..34b084d 100644 --- a/cpp-generator/generator_lib.cpp +++ b/cpp-generator/generator_lib.cpp @@ -1,5 +1,6 @@ #include "generator_lib.h" +#include "cpp_identifier.h" #include "metadata_dependencies.h" #include @@ -676,10 +677,46 @@ static void emitRecordConversions(QTextStream& s, const RecordSet& rs, ApiStyle } } -QString makeHeader(const QString& moduleName, const QString& className, const QJsonArray& methods, ApiStyle apiStyle, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) +// ─── Parameter names ───────────────────────────────────────────────────── +// +// A method's parameters share a scope with the wrapper's own trailing +// parameters and with the locals and members its bodies use, so a contract +// parameter named `timeout_ms` (or `err`, `callback`, ...) is spelled apart from +// them (cpp_identifier.h). Arguments are positional, so the wire never sees a +// parameter name and the methods are renamed once, up front. +static const QSet& methodScopeNames(ApiStyle style) +{ + static const QSet qt = {"err", "timeout", "callback", "_err", "_result", + "m_client", "m_moduleName"}; + static const QSet lp = {"err", "timeout_ms", "callback", "_args", "_err", "_r", + "m_client", "m_state"}; + return style == ApiStyle::Qt ? qt : lp; +} + +static QJsonArray withCppParamNames(QJsonArray methods, ApiStyle style) +{ + for (int i = 0; i < methods.size(); ++i) { + QJsonObject m = methods.at(i).toObject(); + QJsonArray params = m.value("parameters").toArray(); + QStringList names; + for (const QJsonValue& p : params) names << p.toObject().value("name").toString(); + const QStringList cpp = lidlCppNames(names, methodScopeNames(style)); + for (int j = 0; j < params.size(); ++j) { + QJsonObject p = params.at(j).toObject(); + p["name"] = cpp.at(j); + params.replace(j, p); + } + m["parameters"] = params; + methods.replace(i, m); + } + return methods; +} + +QString makeHeader(const QString& moduleName, const QString& className, const QJsonArray& contractMethods, ApiStyle apiStyle, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) { if (apiStyle == ApiStyle::Lp) - return makeHeaderLp(moduleName, className, methods, events, bindMode, records); + return makeHeaderLp(moduleName, className, contractMethods, events, bindMode, records); + const QJsonArray methods = withCppParamNames(contractMethods, apiStyle); const RecordSet rs = parseRecords(records); QString h; QTextStream s(&h); @@ -1010,10 +1047,11 @@ static void emitDispatchRejectionDetector(QTextStream& s) s << "#endif // LOGOS_GENERATED_DISPATCH_REJECTION\n\n"; } -QString makeSource(const QString& moduleName, const QString& className, const QString& headerBaseName, const QJsonArray& methods, ApiStyle apiStyle, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) +QString makeSource(const QString& moduleName, const QString& className, const QString& headerBaseName, const QJsonArray& contractMethods, ApiStyle apiStyle, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) { if (apiStyle == ApiStyle::Lp) - return makeSourceLp(moduleName, className, headerBaseName, methods, events, bindMode, records); + return makeSourceLp(moduleName, className, headerBaseName, contractMethods, events, bindMode, records); + const QJsonArray methods = withCppParamNames(contractMethods, apiStyle); const RecordSet rs = parseRecords(records); // The rejection detector is only reachable from a method body, so a // contract with no invokable method must not emit it (an unused function in @@ -1441,9 +1479,10 @@ static QString lpEventAccessorName(const QString& evName) return QString("on") + cap; } -QString makeHeaderLp(const QString& moduleName, const QString& className, const QJsonArray& methods, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) +QString makeHeaderLp(const QString& moduleName, const QString& className, const QJsonArray& contractMethods, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) { (void)moduleName; + const QJsonArray methods = withCppParamNames(contractMethods, ApiStyle::Lp); const RecordSet rs = parseRecords(records); QString h; QTextStream s(&h); @@ -1598,8 +1637,9 @@ QString makeHeaderLp(const QString& moduleName, const QString& className, const return h; } -QString makeSourceLp(const QString& moduleName, const QString& className, const QString& headerBaseName, const QJsonArray& methods, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) +QString makeSourceLp(const QString& moduleName, const QString& className, const QString& headerBaseName, const QJsonArray& contractMethods, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) { + const QJsonArray methods = withCppParamNames(contractMethods, ApiStyle::Lp); const RecordSet rs = parseRecords(records); QString c; QTextStream s(&c); diff --git a/tests/generator/CMakeLists.txt b/tests/generator/CMakeLists.txt index 4ac71f4..cf2d808 100644 --- a/tests/generator/CMakeLists.txt +++ b/tests/generator/CMakeLists.txt @@ -8,6 +8,8 @@ add_executable(generator_tests ${CMAKE_CURRENT_SOURCE_DIR}/../../cpp-generator/generator_lib.cpp ${CMAKE_CURRENT_SOURCE_DIR}/../../cpp-generator/lidl_to_json.cpp ${CMAKE_CURRENT_SOURCE_DIR}/../../cpp-generator/experimental/lidl_emit_common.cpp + # The provider emitter, so a name rule can be compiled on both sides. + ${CMAKE_CURRENT_SOURCE_DIR}/../../cpp-generator/experimental/lidl_gen_cdylib.cpp test_to_pascal_case.cpp test_normalize_type.cpp test_map_param_type.cpp @@ -23,6 +25,8 @@ add_executable(generator_tests # the per-module (not per-event) subscription state surface. test_lp_event_options.cpp test_optional_spellings.cpp + # Generate from a small contract and compile the result (generated_code.h). + test_reserved_param_names.cpp ) target_include_directories(generator_tests PRIVATE @@ -31,6 +35,24 @@ target_include_directories(generator_tests PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/../../cpp ) +# generated_code.h compiles emitted wrappers with this suite's own compiler, +# against the SDK, logos-protocol and nlohmann headers. +find_package(nlohmann_json REQUIRED) +if(EXISTS "${LOGOS_PROTOCOL_ROOT}/cpp/logos_protocol.h") + set(_protocol_include "${LOGOS_PROTOCOL_ROOT}/cpp") +elseif(EXISTS "${LOGOS_PROTOCOL_ROOT}/include/logos_protocol.h") + set(_protocol_include "${LOGOS_PROTOCOL_ROOT}/include") +else() + message(FATAL_ERROR "logos_protocol.h not found under LOGOS_PROTOCOL_ROOT=${LOGOS_PROTOCOL_ROOT}") +endif() +get_target_property(_json_include nlohmann_json::nlohmann_json INTERFACE_INCLUDE_DIRECTORIES) +string(JOIN "|" _compile_includes + "${CMAKE_CURRENT_SOURCE_DIR}/../../cpp" "${_protocol_include}" ${_json_include}) +target_compile_definitions(generator_tests PRIVATE + LOGOS_TEST_CXX="${CMAKE_CXX_COMPILER}" + LOGOS_TEST_INCLUDES="${_compile_includes}" +) + target_link_libraries(generator_tests PRIVATE GTest::gtest_main Qt${QT_VERSION_MAJOR}::Core diff --git a/tests/generator/generated_code.h b/tests/generator/generated_code.h new file mode 100644 index 0000000..ff05a98 --- /dev/null +++ b/tests/generator/generated_code.h @@ -0,0 +1,173 @@ +// Compiling what the emitters produce. The rest of this suite asserts on text; +// these helpers hand generated code to the compiler that builds the suite, with +// the SDK, logos-protocol and nlohmann include dirs CMake passes in. +#pragma once + +#include +#include +#include +#include +#include +#include +#include + +#include "generator_lib.h" +#include "lidl_gen_cdylib.h" +#include "lidl_to_json.h" + +namespace generated { + +struct Result { + bool ok = false; + QString log; +}; + +struct Wrapper { + QString header; + QString source; +}; + +// `_api.{h,cpp}` for `lidl`, as the umbrella's `--dep` path emits them. +inline Wrapper wrapperFor(const char* lidl, ApiStyle style, const QString& module = "probe", + const QString& className = "Probe") +{ + const LidlParseResult pr = lidlParse(QString::fromUtf8(lidl)); + if (pr.hasError()) return {"#error " + qs(pr.error), QString()}; + ModuleDecl mod = pr.module; + QString idErr; + lidlInjectIdentity(mod, &idErr); + const QJsonArray methods = moduleMethodsToJson(mod); + const QJsonArray events = moduleEventsToJson(mod); + const QJsonArray records = moduleRecordsToJson(mod); + return {makeHeader(module, className, methods, style, events, BindMode::Static, records), + makeSource(module, className, module + "_api.h", methods, style, events, + BindMode::Static, records)}; +} + +// A contract-first cdylib provider around the hand-written `implHeader` (class +// ProbeImpl), plus the empty lp umbrella its exports include. +inline QMap providerFor(const char* lidl, const QString& implHeader) +{ + ModuleDecl mod = lidlParse(QString::fromUtf8(lidl)).module; + const QString document = lidlSerialize(mod); + QString idErr; + lidlInjectIdentity(mod, &idErr); + const QString name = qs(mod.name); + QMap files{ + {name + "_impl.h", implHeader}, + {name + "_types.h", lidlMakeTypesHeaderCdylib(mod)}, + {name + "_module_impl.cpp", + lidlMakeModuleImplExports(mod, "ProbeImpl", name + "_impl.h", document)}, + {"logos_sdk.h", makeUmbrellaHeaderFromDeps({}, {}, ApiStyle::Lp, name)}, + }; + if (!mod.events.empty()) + files.insert(name + "_events_cdylib.cpp", + lidlMakeEventsSourceCdylib(mod, "ProbeImpl", name + "_impl.h")); + return files; +} + +inline Result runTool(const QString& program, const QStringList& args, const QString& dir) +{ + QProcess p; + p.setWorkingDirectory(dir); + p.setProcessChannelMode(QProcess::MergedChannels); + p.start(program, args); + Result r; + if (!p.waitForFinished(300000)) { + r.log = program + " did not finish: " + p.errorString(); + return r; + } + r.log = QString::fromUtf8(p.readAll()); + r.ok = p.exitStatus() == QProcess::NormalExit && p.exitCode() == 0; + return r; +} + +inline QStringList compilerFlags(const QString& std) +{ + QStringList flags{"-std=" + std, "-Wall", "-Wextra", "-I."}; + for (const QString& dir : QString(LOGOS_TEST_INCLUDES).split('|', Qt::SkipEmptyParts)) + flags << "-I" + dir; + return flags; +} + +inline bool writeFiles(const QTemporaryDir& dir, const QMap& files) +{ + for (auto it = files.cbegin(); it != files.cend(); ++it) { + QFile f(QDir(dir.path()).filePath(it.key())); + if (!f.open(QIODevice::WriteOnly | QIODevice::Truncate)) return false; + f.write(it.value().toUtf8()); + } + return true; +} + +// Whether `source`, one of `files`, compiles as `std`. +inline Result compiles(const QMap& files, const QString& source, + const QString& std = "c++17") +{ + QTemporaryDir dir; + if (!dir.isValid() || !writeFiles(dir, files)) return {false, "cannot write the sources"}; + return runTool(LOGOS_TEST_CXX, compilerFlags(std) << "-fsyntax-only" << source, dir.path()); +} + +// The lp_* C ABI an lp wrapper calls, stubbed so that every call answers its +// first argument: a value goes out through the generated encode and comes back +// through the generated decode. +inline QString lpEchoStub() +{ + return QStringLiteral(R"CPP(#include "logos_protocol.h" +#include +#include +#include +#include +namespace { +char* heapCopy(const std::string& s) +{ + char* out = static_cast(std::malloc(s.size() + 1)); + std::memcpy(out, s.c_str(), s.size() + 1); + return out; +} +std::string firstArg(const char* args) +{ + const nlohmann::json a = nlohmann::json::parse(args ? args : "[]"); + return a.empty() ? std::string("null") : a.at(0).dump(); +} +} +extern "C" { +lp_client* lp_client_create(const char*, const char*, const char*, const char*) { return reinterpret_cast(new int(0)); } +void lp_client_destroy(lp_client* c) { delete reinterpret_cast(c); } +int lp_invoke(lp_client*, const char*, const char* args, int, char** out, char** err) +{ + if (out) *out = heapCopy(firstArg(args)); + if (err) *err = nullptr; + return LP_OK; +} +int lp_invoke_async(lp_client*, const char*, const char* args, int, lp_result_cb cb, void* ud) +{ + cb(1, firstArg(args).c_str(), ud); + return LP_OK; +} +lp_subscription* lp_subscribe(lp_client*, const char*, lp_event_cb, void*) { return reinterpret_cast(new int(0)); } +void lp_unsubscribe(lp_subscription* s) { delete reinterpret_cast(s); } +void lp_string_free(char* s) { std::free(s); } +char* lp_get_methods(lp_client*) { return heapCopy("[]"); } +int lp_client_set_subscription_status_cb(lp_client*, lp_subscription_status_cb, void*) { return 1; } +unsigned long long lp_client_subscription_generation(lp_client*) { return 0; } +int lp_client_set_subscription_options(lp_client*, const char*) { return 1; } +int lp_client_rearm_subscriptions(lp_client*) { return 0; } +} +)CPP"); +} + +// Builds `sources` into one program and runs it: ok when it exits 0. +inline Result runs(const QMap& files, const QStringList& sources) +{ + QTemporaryDir dir; + if (!dir.isValid() || !writeFiles(dir, files)) return {false, "cannot write the sources"}; + const Result built = runTool(LOGOS_TEST_CXX, + compilerFlags("c++17") << sources << "-o" << "program", + dir.path()); + if (!built.ok) return {false, "build failed:\n" + built.log}; + return runTool(QDir(dir.path()).filePath("program"), {}, dir.path()); +} + +} // namespace generated diff --git a/tests/generator/test_reserved_param_names.cpp b/tests/generator/test_reserved_param_names.cpp new file mode 100644 index 0000000..79a6637 --- /dev/null +++ b/tests/generator/test_reserved_param_names.cpp @@ -0,0 +1,98 @@ +// A contract parameter named like one of the wrapper's own. Every method appends +// `err` and a deadline (`timeout_ms` on lp, `timeout` on Qt), the async ones a +// `callback`, and the bodies declare locals; logos-rust-sdk's +// sdk_test_caller_module has `timed_call(sleep_ms, timeout_ms)`, which emitted +// "redefinition of parameter 'timeout_ms'". The C++ name moves; the wire, which +// is positional, does not. + +#include + +#include "generated_code.h" + +namespace { + +const char* kContract = R"LIDL( +module probe { + version "1.0.0" + method timed(sleep_ms: int, timeout_ms: int) -> int + method every(err: tstr, callback: tstr, timeout: bool, _args: int, _err: int, _r: int, _result: int, m_client: tstr, m_state: tstr, m_moduleName: tstr) -> tstr + method taken(timeout_ms: int, timeout_ms_: int) -> int + method plain(timeoutMs: int, error: tstr) -> int + event fired(args: tstr, emitEventImpl_: int) +} +)LIDL"; + +} // namespace + +TEST(ReservedParamNames, LpWrapperSpellsThemApartAndCompiles) +{ + const generated::Wrapper w = generated::wrapperFor(kContract, ApiStyle::Lp); + EXPECT_TRUE(w.header.contains( + "int64_t timed(int64_t sleep_ms, int64_t timeout_ms_, logos::CallError* err = nullptr, " + "int timeout_ms = 0);")) << w.header.toStdString(); + // The argument is the contract's; the deadline is still the caller's. + EXPECT_TRUE(w.source.contains("_args.push_back(timeout_ms_);")) << w.source.toStdString(); + EXPECT_TRUE(w.source.contains("m_client.invoke(\"timed\", _args, &_err, timeout_ms);")) + << w.source.toStdString(); + EXPECT_TRUE(w.header.contains( + "std::string every(const std::string& err_, const std::string& callback_, bool timeout, " + "int64_t _args_, int64_t _err_, int64_t _r_, int64_t _result, " + "const std::string& m_client_, const std::string& m_state_, " + "const std::string& m_moduleName, ")) << w.header.toStdString(); + // Never onto a name the contract already spells. + EXPECT_TRUE(w.header.contains("int64_t taken(int64_t timeout_ms__, int64_t timeout_ms_, ")) + << w.header.toStdString(); + EXPECT_TRUE(w.header.contains("int64_t plain(int64_t timeoutMs, const std::string& error, ")) + << w.header.toStdString(); + + const generated::Result r = generated::compiles( + {{"probe_api.h", w.header}, {"probe_api.cpp", w.source}}, "probe_api.cpp"); + EXPECT_TRUE(r.ok) << r.log.toStdString(); +} + +// The Qt surface has its own names (`timeout`, `_result`, `m_moduleName`); its +// headers live in logos-plugin-qt, so only the spelling is checked here. +TEST(ReservedParamNames, QtWrapperSpellsThemApart) +{ + const generated::Wrapper w = generated::wrapperFor(kContract, ApiStyle::Qt); + EXPECT_TRUE(w.header.contains( + "QString every(const QString& err_, const QString& callback_, bool timeout_, " + "qlonglong _args, qlonglong _err_, qlonglong _r, qlonglong _result_, " + "const QString& m_client_, const QString& m_state, const QString& m_moduleName_, " + "logos::CallError* err = nullptr, Timeout timeout = Timeout());")) << w.header.toStdString(); + EXPECT_TRUE(w.source.contains("QVariant::fromValue(timeout_)")) << w.source.toStdString(); + EXPECT_TRUE(w.header.contains("qlonglong timed(qlonglong sleep_ms, qlonglong timeout_ms, ")) + << w.header.toStdString(); +} + +// The provider's event bodies declare `args` and call `emitEventImpl_`. +TEST(ReservedParamNames, ProviderEventBodiesCompile) +{ + const QMap files = generated::providerFor(kContract, R"CPP(#pragma once +#include +#include +#include "logos_module_context.h" +class ProbeImpl : public LogosModuleContext { +public: + int64_t timed(int64_t sleep_ms, int64_t timeout_ms) { return sleep_ms + timeout_ms; } + std::string every(const std::string&, const std::string&, bool, int64_t, int64_t, int64_t, + int64_t, const std::string&, const std::string&, const std::string&) + { return {}; } + int64_t taken(int64_t a, int64_t b) { return a + b; } + int64_t plain(int64_t a, const std::string&) { return a; } +logos_events: + void fired(const std::string& args, int64_t emitEventImpl_); +}; +)CPP"); + const QString events = files.value("probe_events_cdylib.cpp"); + EXPECT_TRUE(events.contains( + "void ProbeImpl::fired(const std::string& args_, int64_t emitEventImpl__)")) + << events.toStdString(); + EXPECT_TRUE(events.contains("args.push_back(args_);")) << events.toStdString(); + EXPECT_TRUE(events.contains("emitEventImpl_(\"fired\", &args);")) << events.toStdString(); + + for (const QString& tu : {QString("probe_events_cdylib.cpp"), QString("probe_module_impl.cpp")}) { + const generated::Result r = generated::compiles(files, tu); + EXPECT_TRUE(r.ok) << tu.toStdString() << "\n" << r.log.toStdString(); + } +} From b64ddb8593f0ba419afd21c14807bd2bb11797ee Mon Sep 17 00:00:00 2001 From: Dario Gabriel Lipicar Date: Sat, 26 Sep 2026 22:46:36 -0300 Subject: [PATCH 2/4] fix(generator): give a C++ keyword in a contract a C++ spelling LIDL admits any identifier as a name, so a contract may call a field `class` or a method `delete`. Every emitter wrote the name verbatim, and the output did not compile: logos-json-rpc-bridge-py's names.lidl, with fields `class` and `int`, failed in the lp client, the Qt wrapper and the cdylib provider. A name that is a C++ keyword (the C++20 list, alternative tokens included, so the output also builds as C++20) now gets `_` appended wherever it is a C++ identifier: records, fields, methods, parameters, event parameters, and the provider's method calls and event bodies. The wire keeps every name, and names derived from one keep it too (`delete` gives `delete_()`, `deleteAsync()`, `onClass()`). A contract-first provider's author writes the same spellings. A contract without keyword names emits the same bytes. Co-Authored-By: Claude Opus 5.5 --- README.md | 11 +- cpp-generator/cpp_identifier.h | 37 ++++- cpp-generator/docs/project.md | 13 +- .../experimental/lidl_gen_cdylib.cpp | 57 +++++-- cpp-generator/generator_lib.cpp | 141 +++++++++++------- tests/generator/CMakeLists.txt | 1 + tests/generator/test_keyword_names.cpp | 139 +++++++++++++++++ 7 files changed, 323 insertions(+), 76 deletions(-) create mode 100644 tests/generator/test_keyword_names.cpp diff --git a/README.md b/README.md index 35259f4..580086d 100644 --- a/README.md +++ b/README.md @@ -344,11 +344,12 @@ so the Qt and Qt-free emitters cannot drift. Anything else stays a value: a method may legitimately return a three-string map, and matching the shape alone would let user data impersonate a refusal. -**Contract names in C++.** A parameter named like one of the wrapper's own -(`err`, `timeout` / `timeout_ms`, `callback`) gets a trailing `_` in C++ only: -`method timed(sleep_ms: int, timeout_ms: int)` is -`timed(int64_t sleep_ms, int64_t timeout_ms_, logos::CallError* err = nullptr, int timeout_ms = 0)`. -The wire is unchanged. The rule is in `cpp-generator/docs/project.md` (*C++ names*). +**Contract names in C++.** A name that is a C++ keyword, or a parameter named +like one of the wrapper's own (`err`, `timeout` / `timeout_ms`, `callback`), +gets a trailing `_` in C++ only: `method delete(class: tstr, timeout_ms: int)` +is `delete_(const std::string& class_, int64_t timeout_ms_, logos::CallError* err = nullptr, int timeout_ms = 0)`, +still called `"delete"` on the wire, and its async twins stay `deleteAsync` / +`deleteAsyncResult`. The rule is in `cpp-generator/docs/project.md` (*C++ names*). ### Universal modules: LogosModuleContext diff --git a/cpp-generator/cpp_identifier.h b/cpp-generator/cpp_identifier.h index 971d78b..4a49805 100644 --- a/cpp-generator/cpp_identifier.h +++ b/cpp-generator/cpp_identifier.h @@ -2,18 +2,40 @@ #define CPP_IDENTIFIER_H // A LIDL name as a C++ identifier. The wire keeps the LIDL spelling; the C++ -// identifier gets `_` appended when it is a name the generated code declares -// in the same scope (`reserved`), until it is not and no other name of the -// scope spells it. A usable name never changes. +// identifier gets `_` appended when it is a C++ keyword or a name the generated +// code declares in the same scope (`reserved`), until it is neither and no +// other name of the scope spells it. A usable name never changes, and names +// derived from one (`Async`, `on`) keep the LIDL spelling. #include #include #include +// C++20, alternative tokens included: generated code must also build as C++20. +inline bool lidlIsCppKeyword(const QString& name) +{ + static const QSet keywords = { + "alignas", "alignof", "and", "and_eq", "asm", "auto", "bitand", "bitor", + "bool", "break", "case", "catch", "char", "char8_t", "char16_t", "char32_t", + "class", "compl", "concept", "const", "consteval", "constexpr", "constinit", + "const_cast", "continue", "co_await", "co_return", "co_yield", "decltype", + "default", "delete", "do", "double", "dynamic_cast", "else", "enum", + "explicit", "export", "extern", "false", "float", "for", "friend", "goto", + "if", "inline", "int", "long", "mutable", "namespace", "new", "noexcept", + "not", "not_eq", "nullptr", "operator", "or", "or_eq", "private", + "protected", "public", "register", "reinterpret_cast", "requires", "return", + "short", "signed", "sizeof", "static", "static_assert", "static_cast", + "struct", "switch", "template", "this", "thread_local", "throw", "true", + "try", "typedef", "typeid", "typename", "union", "unsigned", "using", + "virtual", "void", "volatile", "wchar_t", "while", "xor", "xor_eq", + }; + return keywords.contains(name); +} + // The C++ spellings of one scope's LIDL names, index-aligned with `names`. inline QStringList lidlCppNames(const QStringList& names, const QSet& reserved = {}) { - auto clashes = [&](const QString& n) { return reserved.contains(n); }; + auto clashes = [&](const QString& n) { return lidlIsCppKeyword(n) || reserved.contains(n); }; QSet taken; for (const QString& n : names) if (!clashes(n)) taken.insert(n); @@ -30,4 +52,11 @@ inline QStringList lidlCppNames(const QStringList& names, const QSet& r return out; } +// `name`'s spelling in the scope `scope` (which should contain it). +inline QString lidlCppName(const QString& name, const QStringList& scope) +{ + const int i = scope.indexOf(name); + return i < 0 ? lidlCppNames({name}).first() : lidlCppNames(scope).at(i); +} + #endif // CPP_IDENTIFIER_H diff --git a/cpp-generator/docs/project.md b/cpp-generator/docs/project.md index 93d5b32..04a7ad3 100644 --- a/cpp-generator/docs/project.md +++ b/cpp-generator/docs/project.md @@ -119,8 +119,16 @@ record field keys and event names are never renamed. Only the identifier changes rule (`cpp_identifier.h`): a name that clashes gets `_` appended until it no longer does and no other name of its scope spells it. A name that does not clash never changes. -A method **parameter** clashes with what the wrapper declares in the same scope: its own -`err` and deadline (`timeout_ms` on lp, `timeout` on Qt), the async `callback`, and the +A **C++ keyword** clashes everywhere (the C++20 list, alternative tokens such as `and` +included): a record, field, method, parameter or event named `class`, `delete` or +`default` is `class_`, `delete_`, `default_` in C++. Names derived from a LIDL name keep +it, so `method delete` gives `delete_()`, `deleteAsync()` and `deleteAsyncResult()`, and +`event class` gives `onClass()`. A contract-first cdylib provider's structs and impl class +are the author's, and use the same spellings: a field `class_`, a method `delete_()`, a +`logos_events:` entry `class_(...)`. + +A method **parameter** also clashes with what the wrapper declares in the same scope: its +own `err` and deadline (`timeout_ms` on lp, `timeout` on Qt), the async `callback`, and the locals and members its bodies use. So `method timed(sleep_ms: int, timeout_ms: int)` takes `int64_t timeout_ms_` ahead of the wrapper's `int timeout_ms = 0`. A cdylib provider's event bodies declare `args` and call `emitEventImpl_`, so its event parameters keep off both. @@ -326,6 +334,7 @@ In `tests/generator/`, alongside the wrapper-emitter tests: |-----------|---------------| | `test_make_umbrella.cpp` | The `LogosModules` aggregate: both dependency forms on both API styles, that every member's type is included, dropped nameless entries, empty deps | | `test_reserved_param_names.cpp` | Parameters named like the wrapper's own (`timeout_ms`, `err`, `callback`, …): spelled apart, and the lp wrapper and cdylib provider compile (`generated_code.h` runs this suite's compiler over the output) | +| `test_keyword_names.cpp` | Records, fields, methods, parameters and events named with C++ keywords: `_` in C++, the LIDL name on the wire; the lp wrapper and the provider compile as C++17 and C++20 | Fixture files in `tests/experimental/fixtures/`: - `sample_impl.h` — module with all supported type variations diff --git a/cpp-generator/experimental/lidl_gen_cdylib.cpp b/cpp-generator/experimental/lidl_gen_cdylib.cpp index 0784c33..2cea088 100644 --- a/cpp-generator/experimental/lidl_gen_cdylib.cpp +++ b/cpp-generator/experimental/lidl_gen_cdylib.cpp @@ -51,6 +51,31 @@ bool isRecord(const TypeExpr& te, const std::set& recs) return te.kind == TypeExpr::Named && recs.count(te.name) > 0; } +// The C++ name the author gives a record, a field, a method or an event +// (cpp_identifier.h): a keyword gains `_`. The wire keeps the LIDL name. +QString recordCppName(const std::string& name, const std::set& recs) +{ + QStringList scope; + for (const std::string& r : recs) scope << qs(r); + return lidlCppName(qs(name), scope); +} + +template +QStringList declCppNames(const Decls& decls) +{ + QStringList names; + for (const auto& d : decls) names << qs(d.name); + return lidlCppNames(names); +} + +template +QString declCppName(const Decls& decls, const std::string& name) +{ + QStringList scope; + for (const auto& d : decls) scope << qs(d.name); + return lidlCppName(qs(name), scope); +} + bool typeSupported(const TypeExpr& te, bool isReturn, const std::set& recs) { if (te.kind == TypeExpr::Primitive) { @@ -268,7 +293,7 @@ QString lidlTypeToStdCdylib(const TypeExpr& te, const std::set& rec // A declared record is its generated struct. if (isRecord(te, recs)) - return qs(te.name); + return recordCppName(te.name, recs); // Recurse, so [bstr] is std::vector> and {tstr: Blob} // is std::map. lidlTypeToStd() would answer QVariantList // / QVariantMap here — a Qt name in a Qt-FREE translation unit, which only @@ -374,13 +399,17 @@ void emitRecordCodecs(QTextStream& s, const ModuleDecl& module, s << "namespace logos { namespace detail {\n\n"; // One specialization per declared record. Field order follows the contract. for (const TypeDecl& t : module.types) { - const QString name = qs(t.name); + const QString name = recordCppName(t.name, recs); + // `fn` is the author's member, `key` the field's name on the wire. + const QStringList members = declCppNames(t.fields); s << "template <> struct Codec<::" << name << ", void> {\n"; s << " static nlohmann::json to(const " << name << "& v) {\n"; s << " nlohmann::json out = nlohmann::json::object();\n"; - for (const FieldDecl& f : t.fields) { + for (size_t i = 0; i < t.fields.size(); ++i) { + const FieldDecl& f = t.fields[i]; const QString ft = lidlFieldTypeCdylib(f, recs); - const QString fn = qs(f.name); + const QString fn = members.at(int(i)); + const QString key = qs(f.name); if (ft.startsWith("std::optional<")) { // ENCODE: a record field is a NAMED slot, so empty is spelled by // OMITTING the key — never by writing null. This is the half of @@ -394,10 +423,10 @@ void emitRecordCodecs(QTextStream& s, const ModuleDecl& module, // spellings mean the same state. const QString vt = lidlTypeToStdCdylib(fieldValueType(f), recs); s << " if (v." << fn << ".has_value())\n"; - s << " out[\"" << fn << "\"] = Codec<" << vt << ">::to(*v." + s << " out[\"" << key << "\"] = Codec<" << vt << ">::to(*v." << fn << ");\n"; } else { - s << " out[\"" << fn << "\"] = Codec<" << ft << ">::to(v." + s << " out[\"" << key << "\"] = Codec<" << ft << ">::to(v." << fn << ");\n"; } } @@ -405,9 +434,11 @@ void emitRecordCodecs(QTextStream& s, const ModuleDecl& module, s << " static " << name << " from(const nlohmann::json& j, const std::string& path) {\n"; s << " if (!j.is_object()) detail::typeError(path, \"object\", j);\n"; s << " " << name << " out;\n"; - for (const FieldDecl& f : t.fields) { + for (size_t i = 0; i < t.fields.size(); ++i) { + const FieldDecl& f = t.fields[i]; const QString ft = lidlFieldTypeCdylib(f, recs); - const QString fn = qs(f.name); + const QString fn = members.at(int(i)); + const QString key = qs(f.name); // A missing field is reported at its own path rather than // default-constructed: a record that silently loses a field is the // failure mode this whole layer exists to prevent. @@ -419,9 +450,9 @@ void emitRecordCodecs(QTextStream& s, const ModuleDecl& module, // in a required one Codec still rejects both. One expression, // both halves of the rule. s << " out." << fn << " = Codec<" << ft << ">::from(\n"; - s << " j.contains(\"" << fn << "\") ? j.at(\"" << fn + s << " j.contains(\"" << key << "\") ? j.at(\"" << key << "\") : nlohmann::json(),\n"; - s << " path + \"." << fn << "\");\n"; + s << " path + \"." << key << "\");\n"; } s << " return out;\n }\n};\n\n"; } @@ -626,7 +657,7 @@ QString lidlMakeTypesHeaderCdylib(const ModuleDecl& module) // declarations, so the codec below can name them in any order. if (!module.types.empty()) { for (const TypeDecl& t : module.types) - s << "struct " << qs(t.name) << ";\n"; + s << "struct " << recordCppName(t.name, recs) << ";\n"; s << "\n"; } @@ -880,7 +911,7 @@ QString lidlMakeModuleImplExports(const ModuleDecl& module, s << " }\n"; return; } - QString call = "lidlImpl()." + qs(md.name) + "("; + QString call = "lidlImpl()." + declCppName(module.methods, md.name) + "("; for (size_t i = 0; i < md.params.size(); ++i) { const QString expr = (i < minArgs) ? QString("args.at(%1)").arg(i) @@ -1159,7 +1190,7 @@ QString lidlMakeEventsSourceCdylib(const ModuleDecl& module, QStringList pnames; for (const ParamDecl& pd : ed.params) pnames << qs(pd.name); pnames = lidlCppNames(pnames, {"args", "emitEventImpl_"}); - s << "void " << implClass << "::" << ed.name << "("; + s << "void " << implClass << "::" << declCppName(module.events, ed.name) << "("; for (int i = 0; i < ed.params.size(); ++i) { const QString stdType = lidlTypeToStdCdylib(ed.params[i].type, recsEv); // Must match the author's declaration in the `logos_events:` block: diff --git a/cpp-generator/generator_lib.cpp b/cpp-generator/generator_lib.cpp index 34b084d..f847ab5 100644 --- a/cpp-generator/generator_lib.cpp +++ b/cpp-generator/generator_lib.cpp @@ -320,6 +320,22 @@ static bool isRecordName(const RecordSet& rs, const QString& name) return false; } +// C++ spellings (cpp_identifier.h): record names share the wrapper class's +// scope, a record's field names their own. The wire keeps the LIDL names. +static QString recordCppName(const RecordSet& rs, const QString& name) +{ + QStringList names; + for (const RecordDef& d : rs) names << d.name; + return lidlCppName(name, names); +} + +static QStringList fieldCppNames(const RecordDef& d) +{ + QStringList names; + for (const RecordField& f : d.fields) names << f.name; + return lidlCppNames(names); +} + // How a type name mentions a record, if at all. enum class RecordShape { None, Scalar, List, Map }; @@ -346,7 +362,7 @@ static QString recordCppType(const RecordSet& rs, const QString& t, ApiStyle sty { QString elem; const RecordShape shape = recordShape(rs, t, &elem); - const QString q = qual + elem; + const QString q = qual + recordCppName(rs, elem); switch (shape) { case RecordShape::None: return QString(); case RecordShape::Scalar: return q; @@ -560,9 +576,11 @@ static void emitRecordStructs(QTextStream& s, const RecordSet& rs, ApiStyle styl if (rs.isEmpty()) return; s << " // Record types declared by the contract.\n"; for (const RecordDef& d : rs) { - s << " struct " << d.name << " {\n"; - for (const RecordField& f : d.fields) - s << " " << fieldTypeFor(f, style, rs) << " " << f.name << "{};\n"; + const QStringList fieldNames = fieldCppNames(d); + s << " struct " << recordCppName(rs, d.name) << " {\n"; + for (int i = 0; i < d.fields.size(); ++i) + s << " " << fieldTypeFor(d.fields.at(i), style, rs) << " " << fieldNames.at(i) + << "{};\n"; s << " };\n"; } s << "\n"; @@ -579,20 +597,23 @@ static void emitRecordConversions(QTextStream& s, const RecordSet& rs, ApiStyle const QString qual = className + "::"; for (const RecordDef& d : rs) { - s << "static " << wire << " " << recToWireFn(d.name) - << "(const " << qual << d.name << "& v);\n"; - s << "static " << qual << d.name << " " << recFromWireFn(d.name) - << "(const " << wire << "& w);\n"; + const QString rec = qual + recordCppName(rs, d.name); + s << "static " << wire << " " << recToWireFn(d.name) << "(const " << rec << "& v);\n"; + s << "static " << rec << " " << recFromWireFn(d.name) << "(const " << wire << "& w);\n"; } s << "\n"; for (const RecordDef& d : rs) { + // Field names: `m` below is the member, `f.name` the key. + const QString rec = qual + recordCppName(rs, d.name); + const QStringList members = fieldCppNames(d); // Encode. - s << "static " << wire << " " << recToWireFn(d.name) - << "(const " << qual << d.name << "& v) {\n"; + s << "static " << wire << " " << recToWireFn(d.name) << "(const " << rec << "& v) {\n"; if (style == ApiStyle::Lp) { s << " nlohmann::json __j = nlohmann::json::object();\n"; - for (const RecordField& f : d.fields) { + for (int i = 0; i < d.fields.size(); ++i) { + const RecordField& f = d.fields.at(i); + const QString m = "v." + members.at(i); if (fieldIsWrappedOptional(f, style, rs)) { // A record field is a NAMED slot, so empty is spelled by // OMITTING the key — never by writing null. (A positional @@ -601,30 +622,32 @@ static void emitRecordConversions(QTextStream& s, const RecordSet& rs, ApiStyle // canonicalising, not identity: a peer that sent // `"f": null` gets the key back omitted, and both spellings // decode to the same single empty state. - s << " if (v." << f.name << ".has_value()) __j[\"" << f.name << "\"] = " - << toWireFor(f.type, style, rs, "(*v." + f.name + ")") << ";\n"; + s << " if (" << m << ".has_value()) __j[\"" << f.name << "\"] = " + << toWireFor(f.type, style, rs, "(*" + m + ")") << ";\n"; continue; } s << " __j[\"" << f.name << "\"] = " - << toWireFor(f.type, style, rs, "v." + f.name) << ";\n"; + << toWireFor(f.type, style, rs, m) << ";\n"; } s << " return __j;\n"; } else { s << " QVariantMap __m;\n"; - for (const RecordField& f : d.fields) { + for (int i = 0; i < d.fields.size(); ++i) { + const RecordField& f = d.fields.at(i); + const QString m = "v." + members.at(i); if (f.optional) { // Same named-slot rule on the Qt surface: an INVALID // QVariant is empty, and empty omits the key. Inserting it // would encode `"f": null`, which is the positional // spelling. - s << " if (v." << f.name << ".isValid()) __m.insert(QStringLiteral(\"" - << f.name << "\"), v." << f.name << ");\n"; + s << " if (" << m << ".isValid()) __m.insert(QStringLiteral(\"" + << f.name << "\"), " << m << ");\n"; continue; } // Qt's surface type IS the wire type for non-record fields, so // fromValue is what puts it in the map; records/containers // already produce a QVariant-compatible value. - const QString v = toWireFor(f.type, style, rs, "v." + f.name); + const QString v = toWireFor(f.type, style, rs, m); const bool isRec = recordShape(rs, f.type, nullptr) != RecordShape::None; s << " __m.insert(QStringLiteral(\"" << f.name << "\"), " << (isRec ? v : "QVariant::fromValue(" + v + ")") @@ -636,12 +659,13 @@ static void emitRecordConversions(QTextStream& s, const RecordSet& rs, ApiStyle // Decode. A missing / mistyped field keeps its default rather than // failing the whole call — same leniency the scalar paths use. - s << "static " << qual << d.name << " " << recFromWireFn(d.name) - << "(const " << wire << "& w) {\n"; - s << " " << qual << d.name << " __out;\n"; + s << "static " << rec << " " << recFromWireFn(d.name) << "(const " << wire << "& w) {\n"; + s << " " << rec << " __out;\n"; if (style == ApiStyle::Lp) { s << " if (!w.is_object()) return __out;\n"; - for (const RecordField& f : d.fields) { + for (int i = 0; i < d.fields.size(); ++i) { + const RecordField& f = d.fields.at(i); + const QString m = "__out." + members.at(i); const QString acc = "w.at(\"" + f.name + "\")"; if (fieldIsWrappedOptional(f, style, rs)) { // An absent key and an explicit null are the SAME state on @@ -649,26 +673,28 @@ static void emitRecordConversions(QTextStream& s, const RecordSet& rs, ApiStyle // only `contains` would decode `"f": null` through the // value conversion and turn empty into a VALUE (0, ""). s << " if (w.contains(\"" << f.name << "\") && !" << acc - << ".is_null()) __out." << f.name << " = " + << ".is_null()) " << m << " = " << fromWireFor(f.type, style, rs, acc, qual) << ";\n"; continue; } - s << " if (w.contains(\"" << f.name << "\")) __out." << f.name << " = " + s << " if (w.contains(\"" << f.name << "\")) " << m << " = " << fromWireFor(f.type, style, rs, acc, qual) << ";\n"; } } else { s << " const QVariantMap __m = w.toMap();\n"; - for (const RecordField& f : d.fields) { + for (int i = 0; i < d.fields.size(); ++i) { + const RecordField& f = d.fields.at(i); + const QString m = "__out." + members.at(i); const QString acc = "__m.value(QStringLiteral(\"" + f.name + "\"))"; if (f.optional) { // Absent and null both arrive as an INVALID QVariant — the // same state, as the contract requires. Converting (a // `.toString()` on an optional `tstr`) would have turned // empty into "", which is a value. - s << " __out." << f.name << " = " << acc << ";\n"; + s << " " << m << " = " << acc << ";\n"; continue; } - s << " __out." << f.name << " = " + s << " " << m << " = " << fromWireFor(f.type, style, rs, acc, qual) << ";\n"; } } @@ -677,13 +703,14 @@ static void emitRecordConversions(QTextStream& s, const RecordSet& rs, ApiStyle } } -// ─── Parameter names ───────────────────────────────────────────────────── +// ─── Method and event names ────────────────────────────────────────────── // // A method's parameters share a scope with the wrapper's own trailing // parameters and with the locals and members its bodies use, so a contract // parameter named `timeout_ms` (or `err`, `callback`, ...) is spelled apart from -// them (cpp_identifier.h). Arguments are positional, so the wire never sees a -// parameter name and the methods are renamed once, up front. +// them, as a C++ keyword is (cpp_identifier.h). Arguments are positional, so +// the wire never sees a parameter name: parameters are renamed once, up front. +// A method's own name is on the wire, so its C++ spelling rides beside it. static const QSet& methodScopeNames(ApiStyle style) { static const QSet qt = {"err", "timeout", "callback", "_err", "_result", @@ -693,30 +720,37 @@ static const QSet& methodScopeNames(ApiStyle style) return style == ApiStyle::Qt ? qt : lp; } -static QJsonArray withCppParamNames(QJsonArray methods, ApiStyle style) +// Each declaration gains "cppName"; its parameters (under `paramsKey`) are renamed. +static QJsonArray withCppNames(QJsonArray decls, const QString& paramsKey, + const QSet& reserved) { - for (int i = 0; i < methods.size(); ++i) { - QJsonObject m = methods.at(i).toObject(); - QJsonArray params = m.value("parameters").toArray(); + QStringList declNames; + for (const QJsonValue& d : decls) declNames << d.toObject().value("name").toString(); + const QStringList declCpp = lidlCppNames(declNames); + for (int i = 0; i < decls.size(); ++i) { + QJsonObject d = decls.at(i).toObject(); + d["cppName"] = declCpp.at(i); + QJsonArray params = d.value(paramsKey).toArray(); QStringList names; for (const QJsonValue& p : params) names << p.toObject().value("name").toString(); - const QStringList cpp = lidlCppNames(names, methodScopeNames(style)); + const QStringList cpp = lidlCppNames(names, reserved); for (int j = 0; j < params.size(); ++j) { QJsonObject p = params.at(j).toObject(); p["name"] = cpp.at(j); params.replace(j, p); } - m["parameters"] = params; - methods.replace(i, m); + d[paramsKey] = params; + decls.replace(i, d); } - return methods; + return decls; } -QString makeHeader(const QString& moduleName, const QString& className, const QJsonArray& contractMethods, ApiStyle apiStyle, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) +QString makeHeader(const QString& moduleName, const QString& className, const QJsonArray& contractMethods, ApiStyle apiStyle, const QJsonArray& contractEvents, BindMode bindMode, const QJsonArray& records) { if (apiStyle == ApiStyle::Lp) - return makeHeaderLp(moduleName, className, contractMethods, events, bindMode, records); - const QJsonArray methods = withCppParamNames(contractMethods, apiStyle); + return makeHeaderLp(moduleName, className, contractMethods, contractEvents, bindMode, records); + const QJsonArray methods = withCppNames(contractMethods, "parameters", methodScopeNames(apiStyle)); + const QJsonArray events = withCppNames(contractEvents, "params", {}); const RecordSet rs = parseRecords(records); QString h; QTextStream s(&h); @@ -792,7 +826,7 @@ QString makeHeader(const QString& moduleName, const QString& className, const QJ const QString name = o.value("name").toString(); const QString qtRet = o.value("returnType").toString(); const QString ret = returnTypeFor(qtRet, apiStyle, rs); - s << " " << ret << " " << name << "("; + s << " " << ret << " " << o.value("cppName").toString() << "("; QJsonArray params = o.value("parameters").toArray(); for (int i = 0; i < params.size(); ++i) { QJsonObject p = params.at(i).toObject(); @@ -1047,11 +1081,12 @@ static void emitDispatchRejectionDetector(QTextStream& s) s << "#endif // LOGOS_GENERATED_DISPATCH_REJECTION\n\n"; } -QString makeSource(const QString& moduleName, const QString& className, const QString& headerBaseName, const QJsonArray& contractMethods, ApiStyle apiStyle, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) +QString makeSource(const QString& moduleName, const QString& className, const QString& headerBaseName, const QJsonArray& contractMethods, ApiStyle apiStyle, const QJsonArray& contractEvents, BindMode bindMode, const QJsonArray& records) { if (apiStyle == ApiStyle::Lp) - return makeSourceLp(moduleName, className, headerBaseName, contractMethods, events, bindMode, records); - const QJsonArray methods = withCppParamNames(contractMethods, apiStyle); + return makeSourceLp(moduleName, className, headerBaseName, contractMethods, contractEvents, bindMode, records); + const QJsonArray methods = withCppNames(contractMethods, "parameters", methodScopeNames(apiStyle)); + const QJsonArray events = withCppNames(contractEvents, "params", {}); const RecordSet rs = parseRecords(records); // The rejection detector is only reachable from a method body, so a // contract with no invokable method must not emit it (an unused function in @@ -1210,7 +1245,7 @@ QString makeSource(const QString& moduleName, const QString& className, const QS }; // Signature - s << retQual << " " << className << "::" << name << "("; + s << retQual << " " << className << "::" << o.value("cppName").toString() << "("; for (int i = 0; i < params.size(); ++i) { bool byRef; emitParam(params.at(i).toObject(), byRef); @@ -1479,10 +1514,11 @@ static QString lpEventAccessorName(const QString& evName) return QString("on") + cap; } -QString makeHeaderLp(const QString& moduleName, const QString& className, const QJsonArray& contractMethods, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) +QString makeHeaderLp(const QString& moduleName, const QString& className, const QJsonArray& contractMethods, const QJsonArray& contractEvents, BindMode bindMode, const QJsonArray& records) { (void)moduleName; - const QJsonArray methods = withCppParamNames(contractMethods, ApiStyle::Lp); + const QJsonArray methods = withCppNames(contractMethods, "parameters", methodScopeNames(ApiStyle::Lp)); + const QJsonArray events = withCppNames(contractEvents, "params", {}); const RecordSet rs = parseRecords(records); QString h; QTextStream s(&h); @@ -1602,7 +1638,7 @@ QString makeHeaderLp(const QString& moduleName, const QString& className, const if (!params.isEmpty()) s << ", "; }; - s << " " << ret << " " << name << "("; + s << " " << ret << " " << o.value("cppName").toString() << "("; emitDeclParams(); // Trailing, defaulted, and in that order — existing call sites, // including ones already passing `&err` positionally, are unaffected. @@ -1637,9 +1673,10 @@ QString makeHeaderLp(const QString& moduleName, const QString& className, const return h; } -QString makeSourceLp(const QString& moduleName, const QString& className, const QString& headerBaseName, const QJsonArray& contractMethods, const QJsonArray& events, BindMode bindMode, const QJsonArray& records) +QString makeSourceLp(const QString& moduleName, const QString& className, const QString& headerBaseName, const QJsonArray& contractMethods, const QJsonArray& contractEvents, BindMode bindMode, const QJsonArray& records) { - const QJsonArray methods = withCppParamNames(contractMethods, ApiStyle::Lp); + const QJsonArray methods = withCppNames(contractMethods, "parameters", methodScopeNames(ApiStyle::Lp)); + const QJsonArray events = withCppNames(contractEvents, "params", {}); const RecordSet rs = parseRecords(records); QString c; QTextStream s(&c); @@ -1759,7 +1796,7 @@ QString makeSourceLp(const QString& moduleName, const QString& className, const // Sync — routes the caller's deadline to LpClient::invoke's // `timeout_ms` parameter, which the generated body used to leave at its // default (i.e. silently drop). - s << retQual << " " << className << "::" << name << "("; + s << retQual << " " << className << "::" << o.value("cppName").toString() << "("; emitParams(); if (!params.isEmpty()) s << ", "; s << "logos::CallError* err, int timeout_ms) {\n"; diff --git a/tests/generator/CMakeLists.txt b/tests/generator/CMakeLists.txt index cf2d808..637aaaa 100644 --- a/tests/generator/CMakeLists.txt +++ b/tests/generator/CMakeLists.txt @@ -27,6 +27,7 @@ add_executable(generator_tests test_optional_spellings.cpp # Generate from a small contract and compile the result (generated_code.h). test_reserved_param_names.cpp + test_keyword_names.cpp ) target_include_directories(generator_tests PRIVATE diff --git a/tests/generator/test_keyword_names.cpp b/tests/generator/test_keyword_names.cpp new file mode 100644 index 0000000..7310150 --- /dev/null +++ b/tests/generator/test_keyword_names.cpp @@ -0,0 +1,139 @@ +// A contract name that is a C++ keyword. LIDL admits any identifier, so a +// record, field, method, parameter or event may be called `class` or `delete`; +// those were emitted verbatim and did not compile. The C++ identifier gains a +// `_`; the wire keeps the name, and so do names derived from it (`deleteAsync`). + +#include + +#include "generated_code.h" + +namespace { + +const char* kContract = R"LIDL( +module probe { + version "1.0.0" + type operator { + new: tstr + ? default: int + namespace: [tstr] + requires: bool + } + type Holder { + op: operator + ops: [operator] + byName: {tstr: operator} + ? maybe: operator + } + method delete(class: tstr, register: int) -> operator + method delete_(x: int) -> int + method echo(h: Holder) -> Holder + method and(or: bool, not: bool) -> bool + method co_await(concept: int) -> int + event class(for: tstr, while: int) +} +)LIDL"; + +} // namespace + +TEST(KeywordNames, LpWrapperSpellsThemApartAndCompiles) +{ + const generated::Wrapper w = generated::wrapperFor(kContract, ApiStyle::Lp); + for (const char* decl : {"struct operator_ {", "std::string new_{};", + "std::optional default_{};", + "std::vector namespace_{};", "bool requires_{};", + "operator_ op{};", "std::vector ops{};", + "std::map byName{};", + "std::optional maybe{};"}) + EXPECT_TRUE(w.header.contains(decl)) << decl << "\n" << w.header.toStdString(); + // `delete_` is the contract's own method, so `delete` moves past it. + EXPECT_TRUE(w.header.contains("operator_ delete__(const std::string& class_, int64_t register_, ")) + << w.header.toStdString(); + EXPECT_TRUE(w.header.contains("int64_t delete_(int64_t x, ")) << w.header.toStdString(); + EXPECT_TRUE(w.header.contains("void deleteAsync(const std::string& class_, int64_t register_, ")) + << w.header.toStdString(); + EXPECT_TRUE(w.header.contains("bool and_(bool or_, bool not_, ")) << w.header.toStdString(); + EXPECT_TRUE(w.header.contains("int64_t co_await_(int64_t concept_, ")) << w.header.toStdString(); + EXPECT_TRUE(w.header.contains( + "logos::SubHandle onClass(std::function callback);")) + << w.header.toStdString(); + + // The wire keeps every name. + EXPECT_TRUE(w.source.contains("Probe::operator_ Probe::delete__(")) << w.source.toStdString(); + EXPECT_TRUE(w.source.contains("m_client.invoke(\"delete\", _args, &_err, timeout_ms);")) + << w.source.toStdString(); + EXPECT_TRUE(w.source.contains("__j[\"new\"] = v.new_;")) << w.source.toStdString(); + EXPECT_TRUE(w.source.contains("if (w.contains(\"requires\")) __out.requires_ = ")) + << w.source.toStdString(); + EXPECT_TRUE(w.source.contains("m_client.subscribe(\"class\", ")) << w.source.toStdString(); + + const QMap files{{"probe_api.h", w.header}, {"probe_api.cpp", w.source}}; + for (const char* std : {"c++17", "c++20"}) { + const generated::Result r = generated::compiles(files, "probe_api.cpp", std); + EXPECT_TRUE(r.ok) << std << "\n" << r.log.toStdString(); + } +} + +TEST(KeywordNames, QtWrapperSpellsThemApart) +{ + const generated::Wrapper w = generated::wrapperFor(kContract, ApiStyle::Qt); + EXPECT_TRUE(w.header.contains("struct operator_ {")) << w.header.toStdString(); + EXPECT_TRUE(w.header.contains("QString new_{};")) << w.header.toStdString(); + EXPECT_TRUE(w.header.contains("operator_ delete__(const QString& class_, qlonglong register_, ")) + << w.header.toStdString(); + EXPECT_TRUE(w.source.contains( + "m_client->invokeRemoteMethod(\"probe\", \"delete\", QVariantList{QVariant::fromValue(class_)")) + << w.source.toStdString(); + EXPECT_TRUE(w.source.contains("__m.insert(QStringLiteral(\"new\"), QVariant::fromValue(v.new_));")) + << w.source.toStdString(); + EXPECT_TRUE(w.source.contains("__out.new_ = __m.value(QStringLiteral(\"new\")).toString();")) + << w.source.toStdString(); +} + +// A contract-first provider's structs and impl class are the author's, and use +// the same spellings; the generated codec, dispatch and event bodies match them. +TEST(KeywordNames, ProviderCompilesAgainstTheSameSpellings) +{ + const QMap files = generated::providerFor(kContract, R"CPP(#pragma once +#include +#include +#include +#include +#include +#include "logos_module_context.h" +struct operator_ { + std::string new_; + std::optional default_; + std::vector namespace_; + bool requires_ = false; +}; +struct Holder { + operator_ op; + std::vector ops; + std::map byName; + std::optional maybe; +}; +class ProbeImpl : public LogosModuleContext { +public: + operator_ delete__(const std::string& class_, int64_t register_); + int64_t delete_(int64_t x); + Holder echo(const Holder& h); + bool and_(bool or_, bool not_); + int64_t co_await_(int64_t concept_); +logos_events: + void class_(const std::string& for_, int64_t while_); +}; +)CPP"); + EXPECT_TRUE(files.value("probe_types.h").contains("out[\"new\"] = Codec::to(v.new_);")) + << files.value("probe_types.h").toStdString(); + EXPECT_TRUE(files.value("probe_module_impl.cpp").contains("lidlImpl().delete__(")) + << files.value("probe_module_impl.cpp").toStdString(); + EXPECT_TRUE(files.value("probe_events_cdylib.cpp").contains( + "void ProbeImpl::class_(const std::string& for_, int64_t while_)")) + << files.value("probe_events_cdylib.cpp").toStdString(); + + for (const char* std : {"c++17", "c++20"}) + for (const QString& tu : {QString("probe_events_cdylib.cpp"), QString("probe_module_impl.cpp")}) { + const generated::Result r = generated::compiles(files, tu, std); + EXPECT_TRUE(r.ok) << std << " " << tu.toStdString() << "\n" << r.log.toStdString(); + } +} From eb2e753c68e0bf7c552447df5d72e7455365318b Mon Sep 17 00:00:00 2001 From: Dario Gabriel Lipicar Date: Sat, 26 Sep 2026 22:54:59 -0300 Subject: [PATCH 3/4] fix(generator): hold a recursive record through std::shared_ptr A record may name itself, and the consumer wrappers held every record field by value: `Node { ? parent: Node }` became std::optional inside Node (an incomplete type), `Loop { next: Loop }` a struct containing itself, and a record naming one declared after it an unknown type, in declaration order. logos-json-rpc-bridge-py's recursive.lidl failed on every one, on lp and Qt. Nothing recursed forever: the codec only follows the data it is given. A field whose record, held by value, would hold the record it belongs to is now a std::shared_ptr: empty is absent, on the wire as for an optional. By value means T, and ?T on lp; Qt spells an optional field QVariant, so only a required cycle gets a pointer there. std::vector / std::map / QList / QMap of an incomplete type are fine, so `[Node]` is unchanged. Structs follow the records they hold by value, and are declared first when one names a record defined after it. Every other contract emits the same bytes. The cdylib provider still spells such a field T / std::optional in its codec; documented as a known limitation. Co-Authored-By: Claude Opus 5.5 --- README.md | 6 + cpp-generator/docs/project.md | 19 +++ cpp-generator/generator_lib.cpp | 146 +++++++++++++++++- tests/generator/CMakeLists.txt | 1 + tests/generator/test_recursive_records.cpp | 166 +++++++++++++++++++++ 5 files changed, 334 insertions(+), 4 deletions(-) create mode 100644 tests/generator/test_recursive_records.cpp diff --git a/README.md b/README.md index 580086d..6127ee5 100644 --- a/README.md +++ b/README.md @@ -351,6 +351,12 @@ is `delete_(const std::string& class_, int64_t timeout_ms_, logos::CallError* er still called `"delete"` on the wire, and its async twins stay `deleteAsync` / `deleteAsyncResult`. The rule is in `cpp-generator/docs/project.md` (*C++ names*). +**Recursive records.** A record field that would hold its own record by value +(`type Node { ? parent: Node }`, or two records holding each other) is a +`std::shared_ptr` in C++: `node.parent->value`, empty when absent. Lists and +maps of a record (`children: [Node]`) stay `std::vector` / `std::map`. See +*Recursive records* in `cpp-generator/docs/project.md`. + ### Universal modules: LogosModuleContext Universal (codegen-driven) modules — those built from a plain `src/_impl.h` header rather than a handcrafted `QObject` plugin — don't see the raw `LogosAPI` at all. The contract is **derived from that header**: the module's ordinary public methods *are* its API, with no marker of any kind (there used to be a `LOGOS_METHOD` marker under `interface: "provider"`; both are gone). `metadata.json#codegen.impl_class` / `codegen.impl_header` name the class and the header when they differ from the defaults (`Impl` in `src/_impl.h`). Instead of a `LogosAPI`, the generated C-ABI export TU (`_module_impl.cpp`) populates a narrow `LogosModuleContext` base class with everything an impl typically needs: diff --git a/cpp-generator/docs/project.md b/cpp-generator/docs/project.md index 04a7ad3..24196b2 100644 --- a/cpp-generator/docs/project.md +++ b/cpp-generator/docs/project.md @@ -133,6 +133,19 @@ locals and members its bodies use. So `method timed(sleep_ms: int, timeout_ms: i `int64_t timeout_ms_` ahead of the wrapper's `int timeout_ms = 0`. A cdylib provider's event bodies declare `args` and call `emitEventImpl_`, so its event parameters keep off both. +### Recursive records + +`children: [Node]` inside `Node` is fine as it is: `std::vector` / `std::map` (and `QList` / +`QMap`) take an incomplete type. A record held **by value** cannot contain itself, though: +`? parent: Node` as `std::optional` inside `Node`, `next: Loop` inside `Loop`, or two +records holding each other. A field on such a cycle is a `std::shared_ptr` instead +(`std::shared_ptr parent`): empty is absent, and a copied record shares the pointee. +By value means `T` and, on lp, `?T`; on Qt an optional field is already a `QVariant`, so only +a required cycle gets a pointer there. The wire is unchanged, and an empty pointer omits its +key even for a required field (a provider refuses that call). Structs are emitted after the +records they hold by value, and all are declared first when one names a record defined +after it. A contract with neither emits what it did. + ### Client stubs (`lidl_gen_client.h/cpp`) - `lidlMakeHeader(ModuleDecl)` / `lidlMakeSource(ModuleDecl)` — typed `` client wrapper; each method (and its `…Async` twin) carries a Doxygen `///` comment generated from the method's `description` @@ -335,6 +348,7 @@ In `tests/generator/`, alongside the wrapper-emitter tests: | `test_make_umbrella.cpp` | The `LogosModules` aggregate: both dependency forms on both API styles, that every member's type is included, dropped nameless entries, empty deps | | `test_reserved_param_names.cpp` | Parameters named like the wrapper's own (`timeout_ms`, `err`, `callback`, …): spelled apart, and the lp wrapper and cdylib provider compile (`generated_code.h` runs this suite's compiler over the output) | | `test_keyword_names.cpp` | Records, fields, methods, parameters and events named with C++ keywords: `_` in C++, the LIDL name on the wire; the lp wrapper and the provider compile as C++17 and C++20 | +| `test_recursive_records.cpp` | Self-, mutually- and forward-referencing records: `std::shared_ptr` on by-value cycles, definition order, forward declarations; the lp wrapper is built and round-trips a tree through the generated codec over a stubbed `lp_*` ABI | Fixture files in `tests/experimental/fixtures/`: - `sample_impl.h` — module with all supported type variations @@ -374,6 +388,11 @@ Fixture files in `tests/experimental/fixtures/`: pins the current behaviour so closing the gap is a deliberate change. - **Nesting, map key types and descriptions still do not cross that boundary either** — the flag added for optionality is per-field, not a general widening. +- **A recursive record cannot yet be implemented by a contract-first cdylib provider.** Its + structs are the author's, and the generated codec (`emitRecordCodecs`) still spells a + by-value cycle `T` / `std::optional`, which no struct can hold. The consumers box it + (*Recursive records* above); the provider would need the same convention, and the + header-first parser a way to read `std::shared_ptr` back as `T` or `?T`. - `lidlRecordCollidesWithBytesTag` reads *through* an optional (via `fieldValueType`), so a single-`_bytes`-field record is refused under both spellings. It used to read `f.type`, which refused `? _bytes: tstr` and let `_bytes: ?tstr` through — the same declaration, diff --git a/cpp-generator/generator_lib.cpp b/cpp-generator/generator_lib.cpp index f847ab5..85e5c48 100644 --- a/cpp-generator/generator_lib.cpp +++ b/cpp-generator/generator_lib.cpp @@ -9,6 +9,8 @@ #include #include +#include + bool parseApiStyleFlag(const QStringList& args, ApiStyle& outStyle, QTextStream& err) { QString apiVal; @@ -548,6 +550,110 @@ static QString fieldTypeFor(const RecordField& f, ApiStyle style, const RecordSe return "std::optional<" + value + ">"; } +// ─── Recursive records ─────────────────────────────────────────────────── +// +// A struct cannot hold itself: `Node { ? parent: Node }` spelled +// std::optional inside Node is an incomplete type, and so is any cycle of +// records held by value (`Loop { next: Loop }`, or two records holding each +// other). A field on such a cycle is held through std::shared_ptr instead. +// std::vector / std::map / QList / QMap take an incomplete T, so `[Node]` stays. +// A struct is defined after those it holds by value, and all are declared first +// when one names a struct defined after it. + +// The record a field holds by value, or empty: `T`, and `?T` on the lp surface +// (the Qt one spells an optional field QVariant). +static QString valueFieldRecord(const RecordField& f, ApiStyle style, const RecordSet& rs) +{ + QString elem; + if (recordShape(rs, f.type, &elem) != RecordShape::Scalar) return QString(); + return (f.optional && style == ApiStyle::Qt) ? QString() : elem; +} + +// Whether record `from` holds record `to` by value, directly or through others. +static bool holdsByValue(const RecordSet& rs, ApiStyle style, const QString& from, const QString& to) +{ + QSet seen; + QStringList pending{from}; + while (!pending.isEmpty()) { + const QString cur = pending.takeLast(); + if (cur == to) return true; + if (seen.contains(cur)) continue; + seen.insert(cur); + for (const RecordDef& d : rs) { + if (d.name != cur) continue; + for (const RecordField& f : d.fields) { + const QString held = valueFieldRecord(f, style, rs); + if (!held.isEmpty()) pending << held; + } + } + } + return false; +} + +// A field whose record, held by value, would hold the record it belongs to. +static bool fieldIsBoxed(const RecordDef& owner, const RecordField& f, ApiStyle style, + const RecordSet& rs) +{ + const QString held = valueFieldRecord(f, style, rs); + return !held.isEmpty() && holdsByValue(rs, style, held, owner.name); +} + +static QString recordFieldCppType(const RecordDef& owner, const RecordField& f, ApiStyle style, + const RecordSet& rs) +{ + if (fieldIsBoxed(owner, f, style, rs)) + return "std::shared_ptr<" + recordCppType(rs, f.type, style, QString()) + ">"; + return fieldTypeFor(f, style, rs); +} + +static bool recordsBoxAny(const RecordSet& rs, ApiStyle style) +{ + for (const RecordDef& d : rs) + for (const RecordField& f : d.fields) + if (fieldIsBoxed(d, f, style, rs)) return true; + return false; +} + +// Declaration order, except that a struct follows every struct it holds by value. +// The fields left unboxed form no cycle, so this always terminates. +static QVector recordDefinitionOrder(const RecordSet& rs, ApiStyle style) +{ + QVector order; + QVector placed(rs.size(), false); + std::function place = [&](int i) { + if (placed[i]) return; + placed[i] = true; + for (const RecordField& f : rs[i].fields) { + const QString held = valueFieldRecord(f, style, rs); + if (held.isEmpty() || fieldIsBoxed(rs[i], f, style, rs)) continue; + for (int j = 0; j < rs.size(); ++j) + if (rs[j].name == held) { place(j); break; } + } + order << i; + }; + for (int i = 0; i < rs.size(); ++i) place(i); + return order; +} + +// Whether, in `order`, a struct's fields name a record defined after it. +static bool recordsNeedForwardDecls(const RecordSet& rs, ApiStyle style, const QVector& order) +{ + static const QRegularExpression identifier("[A-Za-z_][A-Za-z0-9_]*"); + QSet records, defined; + for (const RecordDef& d : rs) records << recordCppName(rs, d.name); + for (int i : order) { + defined << recordCppName(rs, rs[i].name); + for (const RecordField& f : rs[i].fields) { + auto words = identifier.globalMatch(recordFieldCppType(rs[i], f, style, rs)); + while (words.hasNext()) { + const QString word = words.next().captured(); + if (records.contains(word) && !defined.contains(word)) return true; + } + } + } + return false; +} + // True when some field actually materialises a std::optional on the Lp surface // — gates the generated `#include `, so a contract with no optional // (or one whose only optionals are untyped-JSON aliases) keeps its header @@ -556,7 +662,8 @@ static bool recordsUseStdOptional(const RecordSet& rs) { for (const RecordDef& d : rs) for (const RecordField& f : d.fields) - if (f.optional && !lpAliasIsAlreadyNullable(paramTypeFor(f.type, ApiStyle::Lp, rs))) + if (f.optional && !fieldIsBoxed(d, f, ApiStyle::Lp, rs) + && !lpAliasIsAlreadyNullable(paramTypeFor(f.type, ApiStyle::Lp, rs))) return true; return false; } @@ -575,12 +682,16 @@ static void emitRecordStructs(QTextStream& s, const RecordSet& rs, ApiStyle styl { if (rs.isEmpty()) return; s << " // Record types declared by the contract.\n"; - for (const RecordDef& d : rs) { + const QVector order = recordDefinitionOrder(rs, style); + if (recordsNeedForwardDecls(rs, style, order)) + for (const RecordDef& d : rs) s << " struct " << recordCppName(rs, d.name) << ";\n"; + for (int r : order) { + const RecordDef& d = rs.at(r); const QStringList fieldNames = fieldCppNames(d); s << " struct " << recordCppName(rs, d.name) << " {\n"; for (int i = 0; i < d.fields.size(); ++i) - s << " " << fieldTypeFor(d.fields.at(i), style, rs) << " " << fieldNames.at(i) - << "{};\n"; + s << " " << recordFieldCppType(d, d.fields.at(i), style, rs) << " " + << fieldNames.at(i) << "{};\n"; s << " };\n"; } s << "\n"; @@ -614,6 +725,12 @@ static void emitRecordConversions(QTextStream& s, const RecordSet& rs, ApiStyle for (int i = 0; i < d.fields.size(); ++i) { const RecordField& f = d.fields.at(i); const QString m = "v." + members.at(i); + if (fieldIsBoxed(d, f, style, rs)) { + // An empty box is absent, required or not: no default is built. + s << " if (" << m << ") __j[\"" << f.name << "\"] = " + << toWireFor(f.type, style, rs, "(*" + m + ")") << ";\n"; + continue; + } if (fieldIsWrappedOptional(f, style, rs)) { // A record field is a NAMED slot, so empty is spelled by // OMITTING the key — never by writing null. (A positional @@ -635,6 +752,11 @@ static void emitRecordConversions(QTextStream& s, const RecordSet& rs, ApiStyle for (int i = 0; i < d.fields.size(); ++i) { const RecordField& f = d.fields.at(i); const QString m = "v." + members.at(i); + if (fieldIsBoxed(d, f, style, rs)) { + s << " if (" << m << ") __m.insert(QStringLiteral(\"" << f.name << "\"), " + << toWireFor(f.type, style, rs, "(*" + m + ")") << ");\n"; + continue; + } if (f.optional) { // Same named-slot rule on the Qt surface: an INVALID // QVariant is empty, and empty omits the key. Inserting it @@ -667,6 +789,12 @@ static void emitRecordConversions(QTextStream& s, const RecordSet& rs, ApiStyle const RecordField& f = d.fields.at(i); const QString m = "__out." + members.at(i); const QString acc = "w.at(\"" + f.name + "\")"; + if (fieldIsBoxed(d, f, style, rs)) { + s << " if (w.contains(\"" << f.name << "\") && !" << acc << ".is_null()) " << m + << " = std::make_shared<" << recordCppType(rs, f.type, style, qual) << ">(" + << fromWireFor(f.type, style, rs, acc, qual) << ");\n"; + continue; + } if (fieldIsWrappedOptional(f, style, rs)) { // An absent key and an explicit null are the SAME state on // decode, so both must leave the field nullopt. Testing @@ -686,6 +814,12 @@ static void emitRecordConversions(QTextStream& s, const RecordSet& rs, ApiStyle const RecordField& f = d.fields.at(i); const QString m = "__out." + members.at(i); const QString acc = "__m.value(QStringLiteral(\"" + f.name + "\"))"; + if (fieldIsBoxed(d, f, style, rs)) { + s << " if (!" << acc << ".isNull()) " << m << " = std::make_shared<" + << recordCppType(rs, f.type, style, qual) << ">(" + << fromWireFor(f.type, style, rs, acc, qual) << ");\n"; + continue; + } if (f.optional) { // Absent and null both arrive as an INVALID QVariant — the // same state, as the contract requires. Converting (a @@ -763,6 +897,8 @@ QString makeHeader(const QString& moduleName, const QString& className, const QJ s << "#include \n"; s << "#include \n"; s << "#include \n"; + // std::shared_ptr, only for a recursive record. + if (recordsBoxAny(rs, apiStyle)) s << "#include \n"; s << "#include \"logos_types.h\"\n"; s << "#include \"logos_api.h\"\n"; s << "#include \"logos_api_client.h\"\n"; @@ -1530,6 +1666,8 @@ QString makeHeaderLp(const QString& moduleName, const QString& className, const // optional keeps its header byte-for-byte unchanged. if (recordsUseStdOptional(rs)) s << "#include \n"; s << "#include \n"; + // std::shared_ptr, only for a recursive record. + if (recordsBoxAny(rs, ApiStyle::Lp)) s << "#include \n"; s << "#include \n"; s << "#include \"logos_json.h\"\n"; s << "#include \"logos_result.h\"\n"; diff --git a/tests/generator/CMakeLists.txt b/tests/generator/CMakeLists.txt index 637aaaa..ea7b88f 100644 --- a/tests/generator/CMakeLists.txt +++ b/tests/generator/CMakeLists.txt @@ -28,6 +28,7 @@ add_executable(generator_tests # Generate from a small contract and compile the result (generated_code.h). test_reserved_param_names.cpp test_keyword_names.cpp + test_recursive_records.cpp ) target_include_directories(generator_tests PRIVATE diff --git a/tests/generator/test_recursive_records.cpp b/tests/generator/test_recursive_records.cpp new file mode 100644 index 0000000..f1d24b6 --- /dev/null +++ b/tests/generator/test_recursive_records.cpp @@ -0,0 +1,166 @@ +// Records that hold themselves. `? parent: Node` inside Node was spelled +// std::optional (an incomplete type), `next: Loop` a struct holding +// itself, and a record naming one declared after it an unknown type. A field on +// a by-value cycle is now a std::shared_ptr, and structs are ordered and, when +// needed, declared up front. The round trip runs through the generated codec. + +#include + +#include "generated_code.h" + +namespace { + +const char* kContract = R"LIDL( +module probe { + version "1.0.0" + type Node { + value: int + children: [Node] + byName: {tstr: Node} + ? parent: Node + } + type Loop { + next: Loop + } + type Pair { + left: Tree + right: ? Tree + } + type Tree { + pair: ? Pair + leaf: ? int + } + type Early { + late: Late + lates: [Late] + } + type Late { + n: int + } + method echoNode(n: Node) -> Node + method echoTree(t: Tree) -> Tree + method echoLoop(l: Loop) -> Loop + method echoEarly(e: Early) -> Early + event grown(node: Node, depth: uint) +} +)LIDL"; + +const char* kRoundTrip = R"CPP(#include "probe_api.h" +#include +#define CHECK(c) do { if (!(c)) { std::printf("FAILED line %d: %s\n", __LINE__, #c); return 1; } } while (0) +int main() +{ + Probe probe("probe_test"); + Probe::Node leaf; + leaf.value = 3; + Probe::Node child; + child.value = 2; + child.children.push_back(leaf); + Probe::Node root; + root.value = 1; + root.children.push_back(child); + root.byName["c"] = child; + root.parent = std::make_shared(); + root.parent->parent = std::make_shared(); + root.parent->parent->value = -1; + const Probe::Node n = probe.echoNode(root); + CHECK(n.value == 1 && n.children.size() == 1 && n.children[0].value == 2); + CHECK(n.children[0].children.size() == 1 && n.children[0].children[0].value == 3); + CHECK(n.byName.count("c") == 1 && n.byName.at("c").children.at(0).value == 3); + CHECK(n.parent && n.parent->value == 0 && n.parent->parent); + CHECK(n.parent->parent->value == -1 && !n.parent->parent->parent && !n.children[0].parent); + + Probe::Tree t; + t.pair = std::make_shared(); + t.pair->left = std::make_shared(); + t.pair->left->leaf = 7; + const Probe::Tree back = probe.echoTree(t); + CHECK(back.pair && back.pair->left && back.pair->left->leaf == 7); + CHECK(!back.pair->right && !back.leaf && !back.pair->left->pair); + + // An empty box encodes as absent: no default Loop is built, so no recursion. + CHECK(!probe.echoLoop(Probe::Loop{}).next); + + Probe::Early e; + e.late.n = 5; + e.lates.push_back(Probe::Late{6}); + const Probe::Early early = probe.echoEarly(e); + CHECK(early.late.n == 5 && early.lates.size() == 1 && early.lates[0].n == 6); + std::printf("ok\n"); + return 0; +} +)CPP"; + +} // namespace + +TEST(RecursiveRecords, LpWrapperBoxesTheCyclesAndRoundTrips) +{ + const generated::Wrapper w = generated::wrapperFor(kContract, ApiStyle::Lp); + for (const char* decl : {"struct Node;\n struct Loop;\n struct Pair;\n struct Tree;\n", + "std::vector children{};", "std::map byName{};", + "std::shared_ptr parent{};", "std::shared_ptr next{};", + "std::shared_ptr left{};", "std::shared_ptr right{};", + "std::shared_ptr pair{};", "std::optional leaf{};", + "#include \n"}) + EXPECT_TRUE(w.header.contains(decl)) << decl << "\n" << w.header.toStdString(); + // Late is held by value, so it is defined first. + EXPECT_LT(w.header.indexOf("struct Late {"), w.header.indexOf("struct Early {")) << w.header.toStdString(); + EXPECT_TRUE(w.source.contains("if (v.parent) __j[\"parent\"] = recToWire_Node((*v.parent));")) + << w.source.toStdString(); + EXPECT_TRUE(w.source.contains( + "if (w.contains(\"next\") && !w.at(\"next\").is_null()) __out.next = " + "std::make_shared(recFromWire_Loop(w.at(\"next\")));")) << w.source.toStdString(); + + const generated::Result r = generated::runs( + {{"probe_api.h", w.header}, {"probe_api.cpp", w.source}, {"main.cpp", kRoundTrip}, + {"lp_stub.cpp", generated::lpEchoStub()}}, + {"probe_api.cpp", "main.cpp", "lp_stub.cpp"}); + EXPECT_TRUE(r.ok) << r.log.toStdString(); + EXPECT_TRUE(r.log.contains("ok")) << r.log.toStdString(); +} + +// On Qt an optional field is a QVariant, so only a required cycle needs a box, +// and Pair/Tree is just an ordering. +TEST(RecursiveRecords, QtWrapperBoxesOnlyRequiredCycles) +{ + const generated::Wrapper w = generated::wrapperFor(kContract, ApiStyle::Qt); + for (const char* decl : {"QList children{};", "QMap byName{};", + "QVariant parent{};", "std::shared_ptr next{};", "Tree left{};", + "QVariant pair{};", "#include \n"}) + EXPECT_TRUE(w.header.contains(decl)) << decl << "\n" << w.header.toStdString(); + EXPECT_FALSE(w.header.contains("struct Node;")) << w.header.toStdString(); + EXPECT_LT(w.header.indexOf("struct Tree {"), w.header.indexOf("struct Pair {")) << w.header.toStdString(); + EXPECT_TRUE(w.source.contains( + "if (v.next) __m.insert(QStringLiteral(\"next\"), recToWire_Loop((*v.next)));")) + << w.source.toStdString(); + EXPECT_TRUE(w.source.contains( + "if (!__m.value(QStringLiteral(\"next\")).isNull()) __out.next = std::make_shared(")) + << w.source.toStdString(); +} + +// Records in declaration order that hold nothing recursively keep their exact +// historical shape: no forward declarations, no , no boxes. +TEST(RecursiveRecords, OtherRecordsAreUnaffected) +{ + const char* plain = R"LIDL( +module probe { + version "1.0.0" + type Late { + n: int + } + type Early { + late: Late + ? maybe: Late + lates: [Late] + } + method get() -> Early +} +)LIDL"; + for (ApiStyle style : {ApiStyle::Lp, ApiStyle::Qt}) { + const generated::Wrapper w = generated::wrapperFor(plain, style); + EXPECT_TRUE(w.header.contains(" // Record types declared by the contract.\n struct Late {")) + << w.header.toStdString(); + EXPECT_FALSE(w.header.contains("shared_ptr")) << w.header.toStdString(); + EXPECT_FALSE(w.header.contains("#include ")) << w.header.toStdString(); + } +} From 96420803f2d65b1c8c0b118f168b7e485fe4ef82 Mon Sep 17 00:00:00 2001 From: Dario Gabriel Lipicar Date: Sat, 26 Sep 2026 23:25:32 -0300 Subject: [PATCH 4/4] test(generator): let the compile probes find the SDK headers from ./result/bin CI runs the installed generator_tests after `nix build .#tests`, when the unpacked source dir the probes' include path named is gone, so every probe failed with "logos_json.h: No such file or directory". nix now passes the SDK headers as a store path; other builds keep the source dir. Co-Authored-By: Claude Opus 5.5 --- nix/tests.nix | 2 +- tests/generator/CMakeLists.txt | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/nix/tests.nix b/nix/tests.nix index 4b9d228..ced373e 100644 --- a/nix/tests.nix +++ b/nix/tests.nix @@ -20,7 +20,7 @@ pkgs.stdenv.mkDerivation { mkdir -p build-tests cd build-tests - cmake ../tests -GNinja -DLOGOS_PROTOCOL_ROOT=${logos-protocol} $cmakeFlags + cmake ../tests -GNinja -DLOGOS_PROTOCOL_ROOT=${logos-protocol} -DLOGOS_TEST_SDK_INCLUDE=${src}/cpp $cmakeFlags ninja cd .. diff --git a/tests/generator/CMakeLists.txt b/tests/generator/CMakeLists.txt index ea7b88f..7f833a9 100644 --- a/tests/generator/CMakeLists.txt +++ b/tests/generator/CMakeLists.txt @@ -48,8 +48,10 @@ else() message(FATAL_ERROR "logos_protocol.h not found under LOGOS_PROTOCOL_ROOT=${LOGOS_PROTOCOL_ROOT}") endif() get_target_property(_json_include nlohmann_json::nlohmann_json INTERFACE_INCLUDE_DIRECTORIES) +# nix passes a store path: CI runs ./result/bin/generator_tests, after the build dir is gone. +set(LOGOS_TEST_SDK_INCLUDE "${CMAKE_CURRENT_SOURCE_DIR}/../../cpp" CACHE PATH "SDK headers for the compile probes") string(JOIN "|" _compile_includes - "${CMAKE_CURRENT_SOURCE_DIR}/../../cpp" "${_protocol_include}" ${_json_include}) + "${LOGOS_TEST_SDK_INCLUDE}" "${_protocol_include}" ${_json_include}) target_compile_definitions(generator_tests PRIVATE LOGOS_TEST_CXX="${CMAKE_CXX_COMPILER}" LOGOS_TEST_INCLUDES="${_compile_includes}"