From 44b92b0480aedb8dc1e60b23718d43cab14cfec7 Mon Sep 17 00:00:00 2001 From: Dario Lipicar Date: Fri, 31 Jul 2026 11:19:04 -0300 Subject: [PATCH] =?UTF-8?q?feat(generator):=20no=20silent=20admissions=20?= =?UTF-8?q?=E2=80=94=20an=20unknown=20C++=20spelling=20is=20a=20build=20er?= =?UTF-8?q?ror=20(salvage=20of=20#112)=20(#127)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(parser): name nlohmann::json explicitly, ahead of killing the fallback `nlohmann::json` (and the `json` alias) has never had a branch in cppTypeToLidl. It reaches the opaque `any` the same way every unrecognised spelling does: the fallback at the bottom of the function. That is fine while the fallback is silent and wrong the moment it becomes an error, because `any` is the RIGHT answer here. test_fullapi_cpp declares `nlohmann::json echoAny(const nlohmann::json&)`, `bool fireAnyEvent(const nlohmann::json&)` and `logos_events: void anyEvent(const nlohmann::json&)`, and those three are the cross-language conformance chain's `any` cells — they must keep publishing `any`. So this lands first and on its own. It maps to the bare `any` primitive, which is exactly what the fallback already produced, making the change output-neutral: generating over every impl header and every .lidl in the workspace produces 764 byte-identical artifacts. That is what lets the later commit treat everything still reaching the fallback as a silent admission rather than a legitimate `any`. Co-Authored-By: Claude Opus 5 * feat(cdylib): a typed map decodes into the author's own container `{tstr: T}` has two C++ spellings — std::map and std::unordered_map — and logos_codec.h specializes Codec for both, because they are the same wire shape. The generated dispatch named one: lidlImpl().echoIntMap(logos::fromJson>(...)) fromJson returns a std::map, and a std::map does not convert to an unordered_map parameter. So the container the author picked decided whether generated code they never wrote compiles — with the diagnostic pointing at that generated line, not at their declaration. logos::JsonArg (logos-protocol, already on master) exists for exactly this: it instantiates its conversion operator with the parameter's own type, so the author's declaration drives the decode. The return side is the same problem mirrored, and `logos::toJson(result)` deduces instead of asserting. Restricted to Map because every other LIDL type has one C++ spelling here, and because JsonArg documents one target it cannot serve — std::optional, whose converting constructor out-ranks the proxy's conversion operator. The Optional branch returns before reaching this code. For a std::map author nothing changes: JsonArg instantiates the same Codec>::from at the same path, and toJson deduces the same T. Compile-checked against both spellings, including a bad element still failing with `expected string at arg0.k, got number`. Across the whole workspace the emitted delta is 5 methods, all in test_fullapi_ext. Co-Authored-By: Claude Opus 5 * feat(parser): an unknown C++ spelling is a build error, not a silent `any` cppTypeToLidl ended with `// Fallback: treat as opaque` -> `any`, and `any` is ADMITTED by every backend gate. So a spelling nobody had written a branch for was accepted in silence, published as `any`, and dispatched as a bare `lidlImpl().f(args.at(0))` — no logos::fromJson<>, no check. That is the one hole #113-#122 closed for every typed slot and left open for anything that reached `any`. An 11-method hostile probe was admitted wholesale: uint32_t, size_t, float and uint8_t all typed as `any`; std::set, std::vector> and a non-string-keyed map as `any`; std::vector as `[any]`. Now every spelling with no LIDL type is collected with the declaration that carries it, and parseImplHeader turns the list into a parse error naming the offending type and the fix: method 'send_generic_public_transaction': parameter 'instruction' declared `const std::vector&`, whose element `uint32_t` has no LIDL type. LIDL numbers are 64-bit only. Declare it `uint64_t` (LIDL `uint`). Widening is source-compatible for every caller; a narrow type on the wire is not, which is why LIDL has none. Numbers get that tailored hint (uint8_t its own — it means bytes here, and only as std::vector); sets, pairs/tuples, non-string map keys, list/deque/array, Qt types and pointers each get theirs; anything else gets the full table of recognised spellings. A hint that does not name a replacement just moves the guesswork, so all of them do. std::unordered_map joins std::map as a spelling of `{tstr: T}` — the codec has always handled both, and the previous commit made the generated dispatch bind whichever the author declared. Two slots are the exception and say so: a record FIELD and an event PARAMETER, where the generator writes the spelling out into code the author's own declaration has to match and can only pick one name. Three properties keep this from breaking things it should not: * The MAPPING is unchanged. cppTypeToLidl still returns `any` for an unsupported spelling; only a diagnostic is recorded. A diagnostic that is later withdrawn therefore leaves output byte-identical. * Diagnostics are withdrawn for declarations that never reach the contract — a helper struct dropped by keepOnlyReferencedRecords, a reserved lifecycle hook (onContextReady and friends), a struct with no parsed fields. Publishing is what makes a type a promise. * An empty spelling is not a C++ type, it is this line-based parser failing to find one. It keeps the old behaviour rather than reporting `''`. Also fixes a latent ordering bug found while threading the context through: metadata.json's event parameters were typed BEFORE the header was read, i.e. against whatever g_recordNames the previous module's parse had left behind. They are now read there and typed after scanForRecords, in the same position in module.events as before. Verified by generating over every impl header and every .lidl in the workspace: 31 derived contracts byte-identical, 368 consumer-umbrella files byte-identical under both --api-style qt and --api-style lp, 46 generated types headers byte-identical. Exactly two modules now fail, at exactly the four slots a prior scan identified as silent admissions. Co-Authored-By: Claude Opus 5 * feat(cdylib): a wrong argument count reports invalid_args The arity gate was `if (args.size() < N) return nullptr;`. The Qt glue turns a NULL reply into an empty QVariant, so "you passed 2 of 4 arguments" was indistinguishable from a method that legitimately returned nothing. logos-rust-sdk already ships the other half. src/args.rs::invalid_args is documented "Same code and message as the C++ generated glue" and pinned by a test named invalid_args_shape_matches_cpp — both of which were false: Rust answered a structured object and C++ answered NULL. Checked against the JSON that crate actually emits rather than against its comment, the two are now byte-identical: {"code":"invalid_args","message":"expected 4 arguments, got 2","origin":"my_module"} `expected` counts REQUIRED parameters in both, so a trailing optional does not change it. The guard is emitted only when the method has at least one required parameter. args.size() is unsigned, so `< 0` never fires: a zero-argument method carried a dead branch. The Rust generator skips it for the same reason, so the two now agree on when a check exists at all. Co-Authored-By: Claude Opus 5 * refactor(cdylib): delete the dead emitted base64 codec #117 replaced the codec the generator emitted into every module with logos-protocol's logos_codec.h, but left the base64 pair it had grown around: * lidlB64Idx + lidlBytesFromJson — 55 emitted lines with NO call site at all. Every byte parameter had already moved to logos::bytesFromJsonLenient. Confirmed across the whole workspace: in 48 generated export TUs, all 48 mentions of lidlBytesFromJson are its own definition line. * lidlB64UrlEncode + lidlBytesToJson — 34 emitted lines that are logos::bytesToJson rewritten, in a translation unit that already includes it through "_types.h". Scalar bstr slots now call logos::bytesToJson. Composite ones ([bstr], {tstr: bstr}, records) have gone through logos::Codec since #117, so this removes the last place a module carried its own copy of an encoder — the arrangement that once let the emitted and canonical halves disagree about padded base64, and that #117's own comment set out to end. hasBytesEventParam goes with it: it existed only to keep the emitted copy from sitting unused in the events sidecar of modules whose events carry no binary data, and the `namespace { }` block it gated is gone too. Co-Authored-By: Claude Opus 5 * docs(doctest): the bstr event marshal is logos::bytesToJson now Commit (D) deleted the dead emitted base64 codec, so the cdylib events sidecar calls logos::bytesToJson -- the one in logos_codec.h, included in the same translation unit -- instead of emitting its own lidlBytesToJson. The doctest still pinned the old spelling and failed on the new output: expected 'args.push_back(lidlBytesToJson(frame));' not found in output The prose around it is unchanged and still correct: the payload is still the canonical {"_bytes": ""} form, which is the property that assertion exists to guard (#99). Only the symbol moved. Verified against real generator output rather than by search-and-replace: built the branch generator, ran --backend cdylib over a sensor_module contract with a bstr event param, and read the emitted line: args.push_back(logos::bytesToJson(frame)); Checked the rest of the specs for other stale symbols (lidlStrdup, b64Url, hasBytesEventParam, the old 'return nullptr' arity gate) -- this was the only one. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- .../experimental/impl_header_parser.cpp | 369 ++++++++++++++++-- .../experimental/lidl_gen_cdylib.cpp | 178 ++++----- .../cpp-sdk-generator-roundtrip.test.yaml | 2 +- .../experimental/test_impl_header_parser.cpp | 241 ++++++++++++ tests/experimental/test_lidl_gen_cdylib.cpp | 128 +++++- 5 files changed, 756 insertions(+), 162 deletions(-) diff --git a/cpp-generator/experimental/impl_header_parser.cpp b/cpp-generator/experimental/impl_header_parser.cpp index d5210e3..0f98e13 100644 --- a/cpp-generator/experimental/impl_header_parser.cpp +++ b/cpp-generator/experimental/impl_header_parser.cpp @@ -51,13 +51,151 @@ static QSet g_recordNames; // (same reason g_recordNames is file-static); parseImplHeader drains it. static QStringList g_unmappableSpellings; -static TypeExpr cppTypeToLidl(const QString& raw) +// --------------------------------------------------------------------------- +// Spellings with NO LIDL type at all. +// +// These used to reach the `any` fallback at the bottom of cppTypeToLidl, and +// `any` is ADMITTED by every backend gate — so the declaration was accepted, the +// published contract said `any`, and the generated dispatch handed the raw +// nlohmann::json straight to the author's parameter. That either worked by luck +// through nlohmann's implicit conversions, threw at call time, or emitted a +// non-canonical wire value. Nothing said a word. +// +// Now every one of them is recorded here and parseImplHeader turns the list into +// a hard parse error naming the offending C++ type and the fix. +// +// cppTypeToLidl still RETURNS the historical `any` for these: the diagnostic and +// the mapping are separate, so a declaration whose diagnostic is later discarded +// (a helper struct that never reaches the contract, a reserved lifecycle hook) +// produces byte-identical output to before. +struct UnsupportedSpelling { + QString context; // "method 'foo': parameter 'bar'" + QString record; // non-empty when this came from a record field + QString declared; // the full spelling as written on the declaration + QString offending; // the spelling that has no LIDL type (may be nested) + QString hint; // what to write instead +}; +static QList g_unsupported; + +// Drop the qualifiers that are about how a value is PASSED rather than what it +// is: cppTypeToLidl normalizes with this, and the diagnostics compare against it +// so `const nlohmann::json&` and `nlohmann::json` are recognised as the same +// spelling instead of reading as a type nested inside itself. +static QString normalizeCppSpelling(const QString& raw) { - // Normalize: strip const, &, leading/trailing whitespace QString t = raw.trimmed(); t.remove(QRegularExpression("^const\\s+")); t.remove(QRegularExpression("\\s*&$")); - t = t.trimmed(); + return t.trimmed(); +} + +// Collapse whitespace so `unsigned long int` and `unsigned long int` are one +// key, and drop the redundant `int` from the multi-word integer spellings. +static QString normalizeNumericSpelling(QString t) +{ + t = t.simplified(); + static const QRegularExpression trailingInt("\\s+int$"); + if (t != "int" && t.contains(' ')) + t.remove(trailingInt); + return t; +} + +// What to write instead. Every hint names a spelling that IS in the contract, +// because "unsupported" without a replacement just moves the guesswork. +static QString unsupportedHint(const QString& t) +{ + static const QString kWidenNote = + "Widening is source-compatible for every caller; a narrow type on the " + "wire is not, which is why LIDL has none."; + + // uint8_t has exactly ONE meaning in this contract and it is not a number. + if (t == "uint8_t" || t == "std::uint8_t") + return "uint8_t means BYTES here, and only as `std::vector` " + "(LIDL `bstr`). For a small number declare `uint64_t` (LIDL " + "`uint`); for binary data declare `std::vector`."; + + const QString n = normalizeNumericSpelling(t); + static const QSet kUnsigned = { + "unsigned", "unsigned char", "unsigned short", "unsigned long", + "unsigned long long", "uint16_t", "uint32_t", "size_t", + "uintptr_t", "uintmax_t", + "std::uint16_t", "std::uint32_t", "std::size_t", "std::uintptr_t" + }; + static const QSet kSigned = { + "char", "signed char", "signed", "short", "int", "long", "long long", + "int8_t", "int16_t", "int32_t", "ssize_t", "ptrdiff_t", "intptr_t", + "intmax_t", "std::int8_t", "std::int16_t", "std::int32_t", + "std::ptrdiff_t", "std::intptr_t" + }; + static const QSet kFloating = { "float", "long double" }; + + if (kUnsigned.contains(n)) + return "LIDL numbers are 64-bit only. Declare it `uint64_t` (LIDL " + "`uint`). " + kWidenNote; + if (kSigned.contains(n)) + return "LIDL numbers are 64-bit only. Declare it `int64_t` (LIDL " + "`int`). " + kWidenNote; + if (kFloating.contains(n)) + return "LIDL has one floating type, `float64`. Declare it `double`."; + + if (t.startsWith("std::set<") || t.startsWith("std::unordered_set<") + || t.startsWith("std::multiset<")) + return "LIDL has no set type. Declare it `std::vector` (LIDL `[T]`); " + "uniqueness is not carried on the wire, so the module has to " + "enforce it either way."; + if (t.startsWith("std::pair<") || t.startsWith("std::tuple<")) + return "LIDL has no pair or tuple. Declare a `struct` in this header — " + "it becomes a contract `type` with named fields — or, for " + "key/value data, `std::map` (LIDL `{tstr: V}`). " + "A struct is usually the right answer: positional pairs have no " + "field names for a consumer in another language to bind to."; + if (t.startsWith("std::map<") || t.startsWith("std::unordered_map<") + || t.startsWith("std::multimap<")) + return "LIDL map keys are always `tstr`. Declare it " + "`std::map` / `std::unordered_map`, or a `[T]` of a struct carrying the key as a field."; + if (t.startsWith("std::list<") || t.startsWith("std::deque<") + || t.startsWith("std::array<") || t.startsWith("std::forward_list<")) + return "LIDL's sequence type is `[T]`, spelled `std::vector`. " + "Declare it that way."; + if (t.startsWith("Q")) + return "Qt types cannot appear in a universal impl header — the " + "module's own translation units are Qt-free, and Qt is confined " + "to the generated glue. Use the std spelling (`std::string`, " + "`std::vector`, `std::map`) or the untyped " + "`LogosMap` / `LogosList`."; + if (t.endsWith("*") || t.endsWith("&&")) + return "A pointer or rvalue reference has no wire form. Pass the value " + "(by value or `const T&`), or a `struct` declared in this " + "header."; + + return "The recognised spellings are: `bool`, `int64_t`, `uint64_t`, " + "`double`, `std::string`, `std::vector` (bytes), " + "`std::optional`, `std::vector`, `std::map`, " + "`std::unordered_map`, `LogosMap` / `LogosList` / " + "`nlohmann::json` (untyped JSON), `StdLogosResult` and `void` as " + "returns, plus any `struct` declared in this header. Rewrite the " + "declaration with one of them, or declare a struct for it."; +} + +// `context` names the declaration being typed ("method 'foo': parameter 'bar'") +// and `declared` the full spelling on it, so a nested offender reports both the +// element that has no LIDL type and the declaration that carries it. `record` is +// set only while typing a struct's fields, so a diagnostic can be withdrawn when +// the struct turns out never to reach the contract. +// +// `nameEmitted` marks the slots whose C++ spelling the generator WRITES OUT into +// code the author's own declaration has to match — a record field's codec, an +// event's generated body. In those the derived spelling is a constraint on the +// author; everywhere else the generated code only has to consume or produce a +// value, and can adapt to whatever the author declared. +static TypeExpr cppTypeToLidl(const QString& raw, const QString& context = QString(), + const QString& declared = QString(), + const QString& record = QString(), + bool nameEmitted = false) +{ + // Normalize: strip const, &, leading/trailing whitespace + QString t = normalizeCppSpelling(raw); // Primitives if (t == "bool") return { TypeExpr::Primitive, "bool", {} }; @@ -113,7 +251,7 @@ static TypeExpr cppTypeToLidl(const QString& raw) // [{tstr: int}]. Without it the element list above was exhaustive and // every other vector fell all the way through to the opaque `any`, // which then encoded a record as a LogosMap. - return { TypeExpr::Array, "", { cppTypeToLidl(inner) } }; + return { TypeExpr::Array, "", { cppTypeToLidl(inner, context, declared, record, nameEmitted) } }; } // Qt collection types — pass through directly (non-std-convertible) @@ -131,17 +269,60 @@ static TypeExpr cppTypeToLidl(const QString& raw) if (t == "LogosList") return { TypeExpr::Array, "", { {TypeExpr::Primitive, "any", {}} } }; + // The alias spelled out. LogosMap / LogosList ARE nlohmann::json, and real + // modules write the underlying name — test_fullapi_cpp's `echoAny` / + // `fireAnyEvent` / `anyEvent`, and both full_api interface headers, all + // declare `nlohmann::json`. It reached `any` ONLY through the fallback at + // the bottom of this function, so naming it here is a PREREQUISITE for + // turning that fallback into an error: without this branch the whole + // cross-language conformance chain stops building. + // + // Mapped to the bare `any` primitive rather than LogosMap's `{tstr: any}` / + // LogosList's `[any]`: `nlohmann::json` is an untyped value of ANY kind, not + // specifically an object or an array. That is the type the fallback already + // produced for it, so nothing about the published contract moves. + if (t == "nlohmann::json" || t == "json") + return { TypeExpr::Primitive, "any", {} }; + // StdLogosResult — pure C++ result type for universal impls. The generator // emits a StdLogosResult→Qt LogosResult conversion in the glue layer. if (t == "StdLogosResult") return { TypeExpr::Primitive, "result", {} }; - // std::map -> {tstr: T}. Absent before, so a typed map was - // unspellable header-first and fell through to `any`. - static QRegularExpression mapRe("^std::map\\s*<\\s*std::string\\s*,\\s*(.+)\\s*>$"); + // std::map / std::unordered_map -> {tstr: T}. Absent before, + // so a typed map was unspellable header-first and fell through to `any`. + // + // Both containers, because logos_codec.h specializes Codec for both and they + // are the same wire shape — a JSON object. Only the KEY is constrained: a + // non-`std::string` key falls through to the unsupported report below, since + // `{tstr: T}` is the only map LIDL has. + static QRegularExpression mapRe( + "^std::(?:unordered_)?map\\s*<\\s*std::string\\s*,\\s*(.+)\\s*>$"); QRegularExpressionMatch mm = mapRe.match(t); if (mm.hasMatch()) { - TypeExpr val = cppTypeToLidl(mm.captured(1).trimmed()); + // ...with one boundary. In a `nameEmitted` slot the generator WRITES the + // spelling out — a record field's codec says `Codec>` and + // an event's generated body repeats the parameter list the author + // declared. It has to pick one of the two container names there, and + // picking the wrong one is a compile error in code the author never + // wrote. Method parameters and returns have no such constraint: they go + // through logos::JsonArg / deduced logos::toJson, which instantiate with + // whatever the author declared. + if (t.startsWith("std::unordered_map") && nameEmitted && !context.isEmpty()) { + UnsupportedSpelling u; + u.context = context; + u.record = record; + u.declared = declared.isEmpty() ? t : declared; + u.offending = t; + u.hint = "`{tstr: T}` has two C++ spellings and this slot's spelling " + "is written into generated code your own declaration has to " + "match, so it can only be one of them: declare it " + "`std::map`. (A method parameter or return " + "may use either container — those are decoded and encoded " + "through your declared type, not a named one.)"; + g_unsupported.append(u); + } + TypeExpr val = cppTypeToLidl(mm.captured(1).trimmed(), context, declared, record, nameEmitted); return { TypeExpr::Map, "", { {TypeExpr::Primitive, "tstr", {}}, val } }; } @@ -157,7 +338,7 @@ static TypeExpr cppTypeToLidl(const QString& raw) static QRegularExpression optRe("^std::optional\\s*<\\s*(.+)\\s*>$"); QRegularExpressionMatch om = optRe.match(t); if (om.hasMatch()) { - TypeExpr inner = cppTypeToLidl(om.captured(1).trimmed()); + TypeExpr inner = cppTypeToLidl(om.captured(1).trimmed(), context, declared, record, nameEmitted); // std::optional> has NO LIDL type. // // `?T` is two-state, and optionality is idempotent under that rule — so @@ -183,7 +364,34 @@ static TypeExpr cppTypeToLidl(const QString& raw) if (g_recordNames.contains(t)) return { TypeExpr::Named, t.toStdString(), {} }; - // Fallback: treat as opaque + // NOTHING above matched: this spelling has no LIDL type. + // + // It used to return the opaque `any` right here, silently. `any` is admitted + // by every backend gate, so the declaration was accepted and the generated + // dispatch handed the raw nlohmann::json to the author's parameter with no + // `logos::fromJson<>` and no check — the one hole left open after #113-#122 + // closed it for every TYPED slot. A `std::vector` parameter + // published `[any]` and worked by accident; a + // `std::vector>` published `[any]` and + // shipped raw binary through a UTF-8 string. + // + // The return value is UNCHANGED (`any`) on purpose: mapping and diagnosis + // are separate concerns. A diagnostic that is later withdrawn — a helper + // struct that never reaches the contract, a reserved lifecycle hook — must + // leave the emitted output byte-identical to what it was. + // + // An empty spelling is not a C++ type at all, it is this line-based parser + // failing to find one (a macro, a member initialiser). Reporting "'' has no + // LIDL type" would be noise, so it keeps the old behaviour. + if (!t.isEmpty() && !context.isEmpty()) { + UnsupportedSpelling u; + u.context = context; + u.record = record; + u.declared = declared.isEmpty() ? t : declared; + u.offending = t; + u.hint = unsupportedHint(t); + g_unsupported.append(u); + } return { TypeExpr::Primitive, "any", {} }; } @@ -219,6 +427,9 @@ static std::vector scanForRecords(const QStringList& lines) TypeDecl td; td.name = om.captured(1).toStdString(); + // Withdraw this struct's diagnostics if it turns out to declare no + // fields at all — nothing is published, so nothing is misreported. + const int diagMark = g_unsupported.size(); for (int j = i + 1; j < lines.size(); ++j) { const QString body = lines.at(j).trimmed(); if (body.startsWith("};")) break; @@ -235,10 +446,15 @@ static std::vector scanForRecords(const QStringList& lines) if (!fm.hasMatch()) continue; FieldDecl fd; fd.name = fm.captured(2).toStdString(); - fd.type = cppTypeToLidl(fm.captured(1).trimmed()); + const QString spelling = fm.captured(1).trimmed(); + fd.type = cppTypeToLidl( + spelling, + QString("type '%1': field '%2'").arg(om.captured(1), fm.captured(2)), + spelling, om.captured(1), /*nameEmitted=*/true); td.fields.push_back(fd); } if (!td.fields.empty()) out.push_back(td); + else while (g_unsupported.size() > diagMark) g_unsupported.removeLast(); } return out; } @@ -296,7 +512,11 @@ static void keepOnlyReferencedRecords(ModuleDecl& module) // Parse a single method declaration line // --------------------------------------------------------------------------- -static bool parseMethodLine(const QString& line, MethodDecl& out) +// `kind` is "method" or "event" — it only labels the diagnostics an unsupported +// C++ spelling produces, so the report matches the section the declaration was +// written in rather than the function that happens to parse both. +static bool parseMethodLine(const QString& line, MethodDecl& out, + const QString& kind = "method") { // Find the parameter list: everything between the last '(' and ')' int parenOpen = -1; @@ -347,7 +567,9 @@ static bool parseMethodLine(const QString& line, MethodDecl& out) return false; out.name = methodName.toStdString(); QString retTypeStr = stripDeclarationSpecifiers(prefix.left(nameStart).trimmed()); - out.returnType = cppTypeToLidl(retTypeStr); + out.returnType = cppTypeToLidl( + retTypeStr, QString("%1 '%2': return type").arg(kind, methodName), retTypeStr, + QString(), /*nameEmitted=*/kind == "event"); // Flag methods whose impl returns LogosMap / LogosList so the generator // can emit nlohmann→Qt conversion code in the glue layer. out.jsonReturn = (retTypeStr == "LogosMap" || retTypeStr == "LogosList"); @@ -385,8 +607,13 @@ static bool parseMethodLine(const QString& line, MethodDecl& out) if (pNameStart >= pNameEnd) continue; ParamDecl pd; - pd.name = p.mid(pNameStart, pNameEnd - pNameStart).toStdString(); - pd.type = cppTypeToLidl(p.left(pNameStart)); + const QString pName = p.mid(pNameStart, pNameEnd - pNameStart); + const QString pSpelling = p.left(pNameStart).trimmed(); + pd.name = pName.toStdString(); + pd.type = cppTypeToLidl( + p.left(pNameStart), + QString("%1 '%2': parameter '%3'").arg(kind, methodName, pName), + pSpelling, QString(), /*nameEmitted=*/kind == "event"); out.params.push_back(pd); } } @@ -413,10 +640,15 @@ ImplParseResult parseImplHeader(const QString& headerPath, { ImplParseResult result; - // Both file-statics are per-parse state: one process generates for more than - // one module. Cleared here rather than next to their first use because the - // metadata's event params are typed before the header is even read. + // All three file-statics are per-parse state: one process generates for more + // than one module. g_recordNames is cleared HERE as well as beside + // scanForRecords, because a name left over from the previous module's header + // would otherwise be visible while this one's metadata events are typed. g_unmappableSpellings.clear(); + g_unsupported.clear(); + g_recordNames.clear(); + + QJsonArray metadataEvents; // --- Read metadata.json --- { @@ -440,24 +672,12 @@ ImplParseResult parseImplHeader(const QString& headerPath, for (const QString& depName : dependencyNames(deps)) result.module.depends.push_back(depName.toStdString()); - // Read events declared in metadata.json - QJsonArray events = obj.value("events").toArray(); - for (const QJsonValue& ev : events) { - QJsonObject evObj = ev.toObject(); - EventDecl ed; - ed.name = evObj.value("name").toString().toStdString(); - ed.description = evObj.value("description").toString().toStdString(); - QJsonArray params = evObj.value("params").toArray(); - for (const QJsonValue& pv : params) { - QJsonObject po = pv.toObject(); - ParamDecl pd; - pd.name = po.value("name").toString().toStdString(); - pd.type = cppTypeToLidl(po.value("type").toString()); - ed.params.push_back(pd); - } - if (!ed.name.empty()) - result.module.events.push_back(ed); - } + // Events declared in metadata.json. Only READ here — their parameter + // types are C++ spellings like any other, and typing them requires the + // record set, which does not exist until the header has been scanned. + // They used to be typed right here, against whatever g_recordNames the + // PREVIOUS module's parse left behind. + metadataEvents = obj.value("events").toArray(); } // --- Read and parse header --- @@ -529,6 +749,31 @@ ImplParseResult parseImplHeader(const QString& headerPath, g_recordNames.clear(); result.module.types = scanForRecords(lines); + // Now the record set exists, the metadata-declared events can be typed. They + // stay AHEAD of the header's `logos_events:` events, as they always were. + for (const QJsonValue& ev : metadataEvents) { + QJsonObject evObj = ev.toObject(); + EventDecl ed; + ed.name = evObj.value("name").toString().toStdString(); + ed.description = evObj.value("description").toString().toStdString(); + const QJsonArray params = evObj.value("params").toArray(); + for (const QJsonValue& pv : params) { + QJsonObject po = pv.toObject(); + ParamDecl pd; + const QString pName = po.value("name").toString(); + const QString pType = po.value("type").toString(); + pd.name = pName.toStdString(); + pd.type = cppTypeToLidl( + pType, + QString("event '%1': parameter '%2' (declared in metadata.json)") + .arg(evObj.value("name").toString(), pName), + pType, QString(), /*nameEmitted=*/true); + ed.params.push_back(pd); + } + if (!ed.name.empty()) + result.module.events.push_back(ed); + } + // State machine: find "class ", then collect declarations. // `InLogosEvents` is entered by the literal `logos_events:` token // (mirrors Qt's `signals:`) — methods declared there are parsed as @@ -688,7 +933,7 @@ ImplParseResult parseImplHeader(const QString& headerPath, if (line.endsWith(';')) { QString decl = line.left(line.size() - 1).trimmed(); MethodDecl md; - if (parseMethodLine(decl, md)) { + if (parseMethodLine(decl, md, "event")) { EventDecl ed; ed.name = md.name; ed.params = md.params; @@ -715,6 +960,10 @@ ImplParseResult parseImplHeader(const QString& headerPath, if (line.endsWith(';')) { QString decl = line.left(line.size() - 1).trimmed(); MethodDecl md; + // Withdraw the declaration's diagnostics if it turns out to be a + // reserved lifecycle hook: it is not part of the contract, so an + // unsupported spelling in it is not a contract defect. + const int diagMark = g_unsupported.size(); if (parseMethodLine(decl, md)) { // LogosModuleContext lifecycle hooks / context accessors are // framework plumbing, not part of the module's API contract. @@ -731,6 +980,9 @@ ImplParseResult parseImplHeader(const QString& headerPath, if (!reserved.contains(qs(md.name))) { md.description = joinDocLines(pendingDoc).toStdString(); result.module.methods.push_back(md); + } else { + while (g_unsupported.size() > diagMark) + g_unsupported.removeLast(); } } } @@ -745,6 +997,49 @@ done: // contract types. keepOnlyReferencedRecords(result.module); + // A C++ spelling with no LIDL type is a BUILD ERROR, not a silent `any`. + // + // Reported after keepOnlyReferencedRecords so a helper struct that never + // reaches the contract cannot fail the build: publishing is what makes a + // declaration's type a promise, and an internal struct promises nothing. + { + std::set published; + for (const TypeDecl& td : result.module.types) published.insert(td.name); + + QStringList reports; + QSet seen; + for (const UnsupportedSpelling& u : g_unsupported) { + if (!u.record.isEmpty() && !published.count(u.record.toStdString())) + continue; // struct dropped: not part of the contract + QString line = " " + u.context; + // "declared X, whose element Y" only when Y really is nested inside + // X — not when the two differ by a `const` and an `&`. + if (normalizeCppSpelling(u.declared) != u.offending) + line += QString(" is declared `%1`, whose element `%2` has no " + "LIDL type.\n ").arg(u.declared, u.offending); + else + line += QString(" is `%1`, which has no LIDL type.\n ") + .arg(u.offending); + line += u.hint; + if (seen.contains(line)) continue; + seen.insert(line); + reports << line; + } + if (!reports.isEmpty()) { + result.error = + headerPath + ": " + QString::number(reports.size()) + + (reports.size() == 1 ? " declaration uses" : " declarations use") + + " a C++ type that has no LIDL type.\n\n" + + reports.join("\n\n") + + "\n\nEach of these used to be published as the opaque `any`, with no " + "diagnostic. `any` is admitted by every backend gate, so the generated " + "dispatch handed the raw JSON straight to the parameter with no decode " + "and no check — the value either converted by luck, threw at call time, " + "or went onto the wire in a form no other language decodes.\n"; + return result; + } + } + if (!g_unmappableSpellings.isEmpty()) { g_unmappableSpellings.removeDuplicates(); err << "Warning: " << headerPath << ": " << g_unmappableSpellings.join(", ") diff --git a/cpp-generator/experimental/lidl_gen_cdylib.cpp b/cpp-generator/experimental/lidl_gen_cdylib.cpp index ae20ca3..11bf34c 100644 --- a/cpp-generator/experimental/lidl_gen_cdylib.cpp +++ b/cpp-generator/experimental/lidl_gen_cdylib.cpp @@ -142,6 +142,23 @@ QString jsonArgToStd(const TypeExpr& te, const QString& expr, const QString& pat const QString cpp = lidlTypeToStdCdylib(te, recs); if (cpp == "LogosMap" || cpp == "LogosList") return expr; // untyped JSON passes through, as it always has + // A TYPED map does not NAME its C++ type — it hands the compiler a proxy and + // lets the author's own declaration pick it. + // + // `{tstr: T}` has two C++ spellings, std::map and std::unordered_map, and + // logos_codec.h specializes Codec for both. Naming one of them here would + // silently make the other a compile error in generated code the author never + // wrote: `logos::fromJson>` returns a std::map, and a std::map + // does not convert to an unordered_map parameter. logos::JsonArg instantiates + // the conversion with the EXACT parameter type instead, so both spellings + // decode — through the same Codec, with the same path in the same error. + // + // Only maps: every other LIDL type has exactly one C++ spelling here, and + // JsonArg documents one type it cannot serve (std::optional, whose own + // converting constructor out-ranks the proxy's conversion operator) — the + // Optional branch above returns before reaching this line. + if (te.kind == TypeExpr::Map) + return "logos::JsonArg(" + expr + ", \"" + path + "\")"; return "logos::fromJson<" + cpp + ">(" + expr + ", \"" + path + "\")"; } @@ -164,12 +181,17 @@ QString stdReturnToJson(const MethodDecl& md, const QString& var, return var; // LogosMap / LogosList are nlohmann::json already } if (te.kind == TypeExpr::Primitive) { - if (te.name == "bstr") return "lidlBytesToJson(" + var + ")"; + if (te.name == "bstr") return "logos::bytesToJson(" + var + ")"; if (te.name == "any") return var; return "nlohmann::json(" + var + ")"; } if (cppRet == "LogosMap" || cppRet == "LogosList") return var; + // Same reason the map ARGUMENT does not name its type: `{tstr: T}` is both + // std::map and std::unordered_map, so let the return variable's own type be + // deduced rather than asserting one of them. + if (te.kind == TypeExpr::Map) + return "logos::toJson(" + var + ")"; // `nlohmann::json(v)` would serialize a vector as a plain number // array and a record not at all; the codec keeps bytes tagged at depth. return "logos::toJson<" + cppRet + ">(" + var + ")"; @@ -372,15 +394,6 @@ void emitRecordCodecs(QTextStream& s, const ModuleDecl& module, s << "}} // namespace logos::detail\n\n"; } -bool hasBytesEventParam(const ModuleDecl& module) -{ - for (const EventDecl& ed : module.events) - for (const ParamDecl& pd : ed.params) - if (pd.type.kind == TypeExpr::Primitive && pd.type.name == "bstr") - return true; - return false; -} - // The Qt spelling of what actually crosses the Qt boundary. // // NOT lidlTypeToQt: that answers the CONSUMER's question ("what type does the @@ -420,35 +433,23 @@ bool hasJsonEventParam(const ModuleDecl& module) return false; } -// The SCALAR tagged-bytes helpers. A `[bstr]` (and bytes at any deeper -// nesting) rides logos::Codec instead: its full specialization for -// std::vector beats the generic vector rule, so one mechanism covers -// [bstr], [[bstr]] and {tstr: [bstr]} alike. #111 emitted a dedicated depth-1 -// list codec here; the generic one subsumes it, and keeping both left an -// unused static in every module that mentioned [bstr]. -void emitBytesEncodeHelpers(QTextStream& s) -{ - s << "// Canonical tagged bytes form {\"_bytes\": base64url} (see logos_protocol.h)\n"; - s << "std::string lidlB64UrlEncode(const std::vector& bytes)\n{\n"; - s << " static const char* alpha = \"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_\";\n"; - s << " std::string out;\n"; - s << " size_t i = 0;\n"; - s << " while (i + 3 <= bytes.size()) {\n"; - s << " uint32_t n = (uint32_t(bytes[i]) << 16) | (uint32_t(bytes[i+1]) << 8) | uint32_t(bytes[i+2]);\n"; - s << " out += alpha[(n >> 18) & 0x3f]; out += alpha[(n >> 12) & 0x3f];\n"; - s << " out += alpha[(n >> 6) & 0x3f]; out += alpha[n & 0x3f];\n"; - s << " i += 3;\n }\n"; - s << " if (i < bytes.size()) {\n"; - s << " uint32_t n = uint32_t(bytes[i]) << 16;\n"; - s << " if (i + 1 < bytes.size()) n |= uint32_t(bytes[i+1]) << 8;\n"; - s << " out += alpha[(n >> 18) & 0x3f]; out += alpha[(n >> 12) & 0x3f];\n"; - s << " if (i + 1 < bytes.size()) out += alpha[(n >> 6) & 0x3f];\n"; - s << " }\n return out;\n}\n\n"; - - s << "nlohmann::json lidlBytesToJson(const std::vector& bytes)\n{\n"; - s << " return nlohmann::json{{\"_bytes\", lidlB64UrlEncode(bytes)}};\n}\n\n"; - -} +// The generated base64 codec is GONE — all of it. +// +// #117 replaced the emitted generic codec with logos-protocol's logos_codec.h, +// but left behind the base64 pair it had grown around: an encoder +// (lidlB64UrlEncode / lidlBytesToJson) and a decoder (lidlB64Idx / +// lidlBytesFromJson), ~89 emitted lines in every module's export TU. The decoder +// had no call site at all — every byte parameter had already moved to +// logos::bytesFromJsonLenient — and the encoder was a byte-for-byte reimplementation +// of logos::bytesToJson, which is included via in the very same +// translation unit. +// +// A second copy of an encoder is not free: this is the arrangement that let the +// emitted and canonical halves drift over padded base64 once already, and it is +// exactly the duplication #117's own comment set out to end. Scalar `bstr` slots +// now call logos::bytesToJson directly, which is what every composite slot +// (`[bstr]`, `{tstr: bstr}`, records) has been doing through logos::Codec since +// #117. void emitInterfaceJson(QTextStream& s, const ModuleDecl& module) { @@ -650,64 +651,6 @@ QString lidlMakeModuleImplExports(const ModuleDecl& module, s << " if (out) std::memcpy(out, str.data(), str.size() + 1);\n"; s << " return out;\n}\n\n"; - emitBytesEncodeHelpers(s); - - s << "int lidlB64Idx(char ch)\n{\n"; - s << " if (ch >= 'A' && ch <= 'Z') return ch - 'A';\n"; - s << " if (ch >= 'a' && ch <= 'z') return ch - 'a' + 26;\n"; - s << " if (ch >= '0' && ch <= '9') return ch - '0' + 52;\n"; - s << " if (ch == '-') return 62;\n if (ch == '_') return 63;\n return -1;\n}\n\n"; - - s << "std::vector lidlBytesFromJson(const nlohmann::json& j)\n{\n"; - s << " std::vector out;\n"; - s << " // Lenient bytes decode (matches the std path, where a QString or\n"; - s << " // QByteArray arg both became bytes): a caller may send the tagged\n"; - s << " // {\"_bytes\": base64url} form, a plain string (raw UTF-8 bytes), or\n"; - s << " // an array of byte values. Only the tagged form needs base64.\n"; - s << " if (j.is_string()) {\n"; - s << " const std::string s = j.get();\n"; - s << " out.assign(s.begin(), s.end());\n"; - s << " return out;\n"; - s << " }\n"; - s << " if (j.is_number()) {\n"; - s << " // A number arg becomes its decimal text as bytes — matches\n"; - s << " // Qt's QVariant(int)->QByteArray, so a caller (or the\n"; - s << " // logoscore CLI's type auto-detection) passing a bare number\n"; - s << " // to a bytes param behaves the same as the Qt path.\n"; - s << " const std::string s = j.dump();\n"; - s << " out.assign(s.begin(), s.end());\n"; - s << " return out;\n"; - s << " }\n"; - s << " if (j.is_array()) {\n"; - s << " for (const auto& e : j)\n"; - s << " if (e.is_number_integer() || e.is_number_unsigned())\n"; - s << " out.push_back(static_cast(e.get() & 0xff));\n"; - s << " return out;\n"; - s << " }\n"; - s << " if (!j.is_object() || j.size() != 1 || !j.contains(\"_bytes\") || !j[\"_bytes\"].is_string())\n"; - s << " return out;\n"; - s << " const std::string s64 = j[\"_bytes\"].get();\n"; - s << " size_t i = 0;\n"; - s << " while (i + 4 <= s64.size()) {\n"; - s << " int a = lidlB64Idx(s64[i]), b = lidlB64Idx(s64[i+1]), c2 = lidlB64Idx(s64[i+2]), d = lidlB64Idx(s64[i+3]);\n"; - s << " if (a < 0 || b < 0 || c2 < 0 || d < 0) return {};\n"; - s << " uint32_t n = (uint32_t(a) << 18) | (uint32_t(b) << 12) | (uint32_t(c2) << 6) | uint32_t(d);\n"; - s << " out.push_back((n >> 16) & 0xff); out.push_back((n >> 8) & 0xff); out.push_back(n & 0xff);\n"; - s << " i += 4;\n }\n"; - s << " size_t rem = s64.size() - i;\n"; - s << " if (rem == 2 || rem == 3) {\n"; - s << " int a = lidlB64Idx(s64[i]), b = lidlB64Idx(s64[i+1]);\n"; - s << " if (a < 0 || b < 0) return {};\n"; - s << " uint32_t n = (uint32_t(a) << 18) | (uint32_t(b) << 12);\n"; - s << " out.push_back((n >> 16) & 0xff);\n"; - s << " if (rem == 3) {\n"; - s << " int c2 = lidlB64Idx(s64[i+2]);\n"; - s << " if (c2 < 0) return {};\n"; - s << " n |= uint32_t(c2) << 6;\n"; - s << " out.push_back((n >> 8) & 0xff);\n"; - s << " }\n }\n return out;\n}\n\n"; - - s << "nlohmann::json lidlResultToJson(const StdLogosResult& r)\n{\n"; s << " nlohmann::json obj;\n"; s << " obj[\"success\"] = r.success;\n"; @@ -807,7 +750,32 @@ QString lidlMakeModuleImplExports(const ModuleDecl& module, for (size_t i = 0; i < md.params.size(); ++i) if (!paramIsOptional(md.params[i])) minArgs = i + 1; s << " if (m == \"" << md.name << "\") {\n"; - s << " if (args.size() < " << minArgs << ") return nullptr;\n"; + // A wrong argument COUNT is reported, not swallowed. + // + // This used to be `return nullptr`, and the Qt glue turns a NULL reply + // into an empty QVariant — indistinguishable from a method that + // legitimately returned nothing. "You passed 2 of 4 arguments" looked + // like a successful empty answer. + // + // The shape is the one logos-rust-sdk's args::invalid_args() already + // emits (src/args.rs), so a C++ and a Rust provider answer a malformed + // call identically — which is what that module's + // invalid_args_shape_matches_cpp test claims, and what was not true + // until now. Same three keys, same message text, same `origin`. + // + // Emitted only when the method has at least one REQUIRED parameter: + // `args.size() < 0` is unsigned-compared and always false, so a zero-arg + // method carried a dead branch (the Rust generator skips it for the same + // reason). + if (minArgs > 0) { + s << " if (args.size() < " << minArgs << ") {\n"; + s << " nlohmann::json err{{\"code\", \"invalid_args\"},\n"; + s << " {\"message\", \"expected " << minArgs + << " arguments, got \" + std::to_string(args.size())},\n"; + s << " {\"origin\", \"" << module.name << "\"}};\n"; + s << " return lidlStrdup(err.dump());\n"; + s << " }\n"; + } QString call = "lidlImpl()." + qs(md.name) + "("; for (size_t i = 0; i < md.params.size(); ++i) { const QString expr = (i < minArgs) @@ -913,13 +881,11 @@ QString lidlMakeEventsSourceCdylib(const ModuleDecl& module, s << "#include \n"; s << "\n"; - // Only the modules that actually emit binary event payloads need the bytes - // encoder; emitting it everywhere would leave it unused (and warned about). - if (hasBytesEventParam(module)) { - s << "namespace {\n\n"; - emitBytesEncodeHelpers(s); - s << "} // namespace\n\n"; - } + // No local bytes encoder any more, and so no hasBytesEventParam() gate for + // it either: a `bstr` event parameter calls logos::bytesToJson, which the + // pulled in by "_types.h" above already provides. + // The gate existed only to keep the emitted copy from sitting unused in + // modules whose events carry no binary data. for (const EventDecl& ed : module.events) { s << "void " << implClass << "::" << ed.name << "("; @@ -964,7 +930,7 @@ QString lidlMakeEventsSourceCdylib(const ModuleDecl& module, continue; } if (pd.type.kind == TypeExpr::Primitive && pd.type.name == "bstr") - s << " args.push_back(lidlBytesToJson(" << pd.name << "));\n"; + s << " args.push_back(logos::bytesToJson(" << pd.name << "));\n"; else s << " args.push_back(" << pd.name << ");\n"; } diff --git a/doctests/cpp-sdk-generator-roundtrip.test.yaml b/doctests/cpp-sdk-generator-roundtrip.test.yaml index 9b3168e..8666bbe 100644 --- a/doctests/cpp-sdk-generator-roundtrip.test.yaml +++ b/doctests/cpp-sdk-generator-roundtrip.test.yaml @@ -262,7 +262,7 @@ sections: - "void SensorModuleImpl::reading(uint64_t id, double value)" - "args.push_back(value);" - "void SensorModuleImpl::capture(uint64_t id, const std::vector& frame)" - - "args.push_back(lidlBytesToJson(frame));" + - "args.push_back(logos::bytesToJson(frame));" - title: "Flow 3 — LIDL → consumer header" step: true diff --git a/tests/experimental/test_impl_header_parser.cpp b/tests/experimental/test_impl_header_parser.cpp index f5588da..63857f2 100644 --- a/tests/experimental/test_impl_header_parser.cpp +++ b/tests/experimental/test_impl_header_parser.cpp @@ -689,3 +689,244 @@ TEST_F(ImplHeaderParserTest, NestedOptionalCollapsesAndIsReported) << errOutput.toStdString(); EXPECT_TRUE(errOutput.contains("no LIDL type")) << errOutput.toStdString(); } + +// A one-class header written to a temp dir and parsed, for the cases that are +// about a single declaration rather than a whole fixture module. +namespace { + +QString probeHeader(QTemporaryDir& dir, const QString& body) +{ + const QString hp = dir.filePath("probe_impl.h"); + QFile f(hp); + if (!f.open(QIODevice::WriteOnly | QIODevice::Text)) + return QString(); + f.write(("#pragma once\n" + "#include \n" + "#include \n" + "#include \n" + "#include \n" + "#include \n" + "#include \n" + "#include \n" + "#include \n" + "#include \n" + + body).toUtf8()); + return hp; +} + +} // namespace + +// `nlohmann::json` reaches `any` only through the fallback, and `any` is the +// RIGHT answer for it — test_fullapi_cpp's echoAny / fireAnyEvent / anyEvent +// are the conformance matrix's `any` cells. Naming it is what lets the fallback +// become an error without taking them out. +TEST_F(ImplHeaderParserTest, NlohmannJsonIsAnyByName) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString hp = probeHeader(dir, + "class ProbeImpl {\n" + "public:\n" + " nlohmann::json echoAny(const nlohmann::json& v);\n" + " bool fireAnyEvent(const nlohmann::json& v);\n" + "logos_events:\n" + " void anyEvent(const nlohmann::json& v);\n" + "};\n"); + auto r = parseImplHeader(hp, "ProbeImpl", + fixturesDir() + "/sample_metadata.json", err); + ASSERT_FALSE(r.hasError()) << r.error.toStdString(); + + ASSERT_EQ(r.module.methods.size(), 2u); + EXPECT_EQ(r.module.methods[0].returnType.name, "any"); + EXPECT_EQ(r.module.methods[0].params[0].type.name, "any"); + EXPECT_EQ(r.module.methods[1].params[0].type.name, "any"); + ASSERT_EQ(r.module.events.size(), 1u); + EXPECT_EQ(r.module.events[0].params[0].type.name, "any"); +} + +// --------------------------------------------------------------------------- +// A C++ spelling with no LIDL type is a BUILD ERROR, not a silent `any`. +// +// cppTypeToLidl used to end with `// Fallback: treat as opaque` -> `any`, and +// `any` is admitted by every backend gate. So an unrecognised spelling was +// accepted in silence, published as `any`, and dispatched as a raw +// `lidlImpl().f(args.at(0))` with no decode and no check. +// --------------------------------------------------------------------------- + +TEST_F(ImplHeaderParserTest, NarrowNumericIsRejectedWithTheWidening) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString hp = probeHeader(dir, + "class ProbeImpl {\n" + "public:\n" + " int64_t f(uint32_t depth);\n" + "};\n"); + ASSERT_FALSE(hp.isEmpty()); + + auto r = parseImplHeader(hp, "ProbeImpl", + fixturesDir() + "/sample_metadata.json", err); + ASSERT_TRUE(r.hasError()) << "uint32_t was admitted as `any`"; + // Names the declaration, the offending type, and what to write instead. + EXPECT_TRUE(r.error.contains("method 'f': parameter 'depth'")) << r.error.toStdString(); + EXPECT_TRUE(r.error.contains("`uint32_t`")) << r.error.toStdString(); + EXPECT_TRUE(r.error.contains("`uint64_t`")) << r.error.toStdString(); +} + +// The offender may be nested. Report BOTH: the element with no LIDL type, and +// the declaration that carries it. +TEST_F(ImplHeaderParserTest, NestedOffenderNamesTheDeclarationToo) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString hp = probeHeader(dir, + "class ProbeImpl {\n" + "public:\n" + " int64_t f(const std::vector& instruction);\n" + "};\n"); + auto r = parseImplHeader(hp, "ProbeImpl", + fixturesDir() + "/sample_metadata.json", err); + ASSERT_TRUE(r.hasError()); + EXPECT_TRUE(r.error.contains("const std::vector&")) << r.error.toStdString(); + EXPECT_TRUE(r.error.contains("`uint32_t`")) << r.error.toStdString(); +} + +// Each family gets a hint that names a replacement. A diagnostic without one +// just moves the guesswork. +TEST_F(ImplHeaderParserTest, EachUnsupportedFamilyNamesItsReplacement) +{ + struct Case { const char* decl; const char* mentions; }; + const Case cases[] = { + {"int64_t f(float v);", "`double`"}, + {"int64_t f(uint8_t v);", "std::vector"}, + {"int64_t f(size_t v);", "`uint64_t`"}, + {"int64_t f(const std::set& v);", "std::vector"}, + {"int64_t f(const std::pair& v);", "struct"}, + {"int64_t f(const std::map& v);", "`tstr`"}, + }; + for (const Case& c : cases) { + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString hp = probeHeader(dir, + QString("class ProbeImpl {\npublic:\n %1\n};\n").arg(c.decl)); + QString e; + QTextStream es(&e); + auto r = parseImplHeader(hp, "ProbeImpl", + fixturesDir() + "/sample_metadata.json", es); + ASSERT_TRUE(r.hasError()) << c.decl; + EXPECT_TRUE(r.error.contains(c.mentions)) + << c.decl << "\n" << r.error.toStdString(); + } +} + +// std::unordered_map is the second C++ spelling of `{tstr: T}`. +// logos_codec.h has always specialized Codec for it; the parser had not, so it +// published `any`. +TEST_F(ImplHeaderParserTest, UnorderedMapIsAStringKeyedMap) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString hp = probeHeader(dir, + "class ProbeImpl {\n" + "public:\n" + " int64_t f(const std::unordered_map& m);\n" + "};\n"); + auto r = parseImplHeader(hp, "ProbeImpl", + fixturesDir() + "/sample_metadata.json", err); + ASSERT_FALSE(r.hasError()) << r.error.toStdString(); + ASSERT_EQ(r.module.methods.size(), 1u); + const TypeExpr& t = r.module.methods[0].params[0].type; + EXPECT_EQ(t.kind, TypeExpr::Map); + ASSERT_EQ(t.elements.size(), 2u); + EXPECT_EQ(t.elements[0].name, "tstr"); + EXPECT_EQ(t.elements[1].name, "tstr"); +} + +// ...except in the two slots whose C++ spelling the generator WRITES OUT — a +// record field's codec and an event's generated body. There it has to pick one +// container name, and picking the wrong one is a compile error in code the +// author never wrote. Say so at the declaration instead. +TEST_F(ImplHeaderParserTest, UnorderedMapIsRejectedWhereTheSpellingIsEmitted) +{ + for (const char* body : { + "struct Rec {\n" + " std::unordered_map m;\n" + "};\n" + "class ProbeImpl {\npublic:\n Rec echo(const Rec& v);\n};\n", + + "class ProbeImpl {\n" + "public:\n" + " bool fire();\n" + "logos_events:\n" + " void changed(const std::unordered_map& m);\n" + "};\n"}) { + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString hp = probeHeader(dir, body); + QString e; + QTextStream es(&e); + auto r = parseImplHeader(hp, "ProbeImpl", + fixturesDir() + "/sample_metadata.json", es); + ASSERT_TRUE(r.hasError()) << body; + EXPECT_TRUE(r.error.contains("std::map")) << r.error.toStdString(); + } +} + +// A helper struct the API never mentions is dropped from the contract, so an +// unsupported spelling INSIDE it promises nothing and must not fail the build. +// Publishing is what makes a declaration's type a promise. +TEST_F(ImplHeaderParserTest, UnreferencedHelperStructDoesNotFailTheBuild) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString hp = probeHeader(dir, + "struct PendingAction {\n" + " uint32_t attempts;\n" + "};\n" + "class ProbeImpl {\n" + "public:\n" + " int64_t f(int64_t n);\n" + "};\n"); + auto r = parseImplHeader(hp, "ProbeImpl", + fixturesDir() + "/sample_metadata.json", err); + ASSERT_FALSE(r.hasError()) << r.error.toStdString(); + EXPECT_TRUE(r.module.types.empty()); + + // ...and the same struct DOES fail once a method publishes it. + QTemporaryDir dir2; + ASSERT_TRUE(dir2.isValid()); + const QString hp2 = probeHeader(dir2, + "struct PendingAction {\n" + " uint32_t attempts;\n" + "};\n" + "class ProbeImpl {\n" + "public:\n" + " PendingAction f(int64_t n);\n" + "};\n"); + QString e2; + QTextStream es2(&e2); + auto r2 = parseImplHeader(hp2, "ProbeImpl", + fixturesDir() + "/sample_metadata.json", es2); + ASSERT_TRUE(r2.hasError()); + EXPECT_TRUE(r2.error.contains("type 'PendingAction': field 'attempts'")) + << r2.error.toStdString(); +} + +// The reserved LogosModuleContext hooks are framework plumbing, not contract. +// They are dropped after parsing, so their spellings are not a contract defect. +TEST_F(ImplHeaderParserTest, ReservedHookSpellingsAreNotContractDefects) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString hp = probeHeader(dir, + "class ProbeImpl {\n" + "public:\n" + " void onContextReady(uint32_t generation);\n" + " int64_t f(int64_t n);\n" + "};\n"); + auto r = parseImplHeader(hp, "ProbeImpl", + fixturesDir() + "/sample_metadata.json", err); + ASSERT_FALSE(r.hasError()) << r.error.toStdString(); + ASSERT_EQ(r.module.methods.size(), 1u); + EXPECT_EQ(r.module.methods[0].name, "f"); +} diff --git a/tests/experimental/test_lidl_gen_cdylib.cpp b/tests/experimental/test_lidl_gen_cdylib.cpp index 35f36dd..db6c8ce 100644 --- a/tests/experimental/test_lidl_gen_cdylib.cpp +++ b/tests/experimental/test_lidl_gen_cdylib.cpp @@ -82,10 +82,12 @@ TEST(LidlGenCdylib, BinaryEventPayloadUsesCanonicalBytesEncoding) const QString source = eventsSourceFor(m); - // The real argument is serialized, through the canonical encoder... - EXPECT_TRUE(source.contains("args.push_back(lidlBytesToJson(payload));")); - EXPECT_TRUE(source.contains("std::string lidlB64UrlEncode")); - EXPECT_TRUE(source.contains("nlohmann::json lidlBytesToJson")); + // The real argument is serialized, through THE canonical encoder — the one + // in logos-protocol's logos_codec.h, reached via "_types.h". The + // sidecar used to emit its own base64 encoder beside this call. + EXPECT_TRUE(source.contains("args.push_back(logos::bytesToJson(payload));")); + EXPECT_FALSE(source.contains("lidlB64UrlEncode")); + EXPECT_FALSE(source.contains("lidlBytesToJson")); // ...and the empty tagged value is gone. EXPECT_FALSE(source.contains("nlohmann::json{{\"_bytes\", \"\"}}")); @@ -98,22 +100,33 @@ TEST(LidlGenCdylib, BinaryEventPayloadUsesCanonicalBytesEncoding) EXPECT_TRUE(source.contains("const std::vector& payload")); } -// The encoder is only needed by modules that actually emit binary payloads. -// Emitted unconditionally it is an unused static function in every other -// module's sidecar (-Wunused-function). -TEST(LidlGenCdylib, BytesEncoderOmittedWhenNoEventCarriesBytes) +// No module carries a local base64 codec any more — not the ones with binary +// events and not the ones without. The gate that used to decide which got one +// is gone with it. +TEST(LidlGenCdylib, NoModuleEmitsItsOwnBase64Codec) { - const ModuleDecl m = moduleWithEvent("fault", { + const ModuleDecl bytes = moduleWithEvent("messageReceived", { + param("payload", prim("bstr")), + }); + const ModuleDecl plain = moduleWithEvent("fault", { param("code", prim("int")), param("message", prim("tstr")), param("fatal", prim("bool")), }); - const QString source = eventsSourceFor(m); - - EXPECT_FALSE(source.contains("lidlB64UrlEncode")); - EXPECT_FALSE(source.contains("lidlBytesToJson")); - EXPECT_TRUE(source.contains("args.push_back(code);")); + for (const QString& source : {eventsSourceFor(bytes), eventsSourceFor(plain), + implSourceFor(bytes), implSourceFor(plain)}) { + EXPECT_FALSE(source.contains("lidlB64UrlEncode")) << source.toStdString(); + EXPECT_FALSE(source.contains("lidlB64Idx")) << source.toStdString(); + EXPECT_FALSE(source.contains("lidlBytesToJson")) << source.toStdString(); + // The decoder had no call site at all after #117 — emitted into every + // module and never once called. + EXPECT_FALSE(source.contains("lidlBytesFromJson")) << source.toStdString(); + EXPECT_FALSE(source.contains( + "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_")) + << source.toStdString(); + } + EXPECT_TRUE(eventsSourceFor(plain).contains("args.push_back(code);")); } // The sidecar is compiled into the module's Qt-free cdylib, so a JSON payload @@ -350,6 +363,16 @@ TypeExpr opt(const TypeExpr& inner) return {TypeExpr::Optional, "", {inner}}; } +TypeExpr arr(const TypeExpr& elem) +{ + return {TypeExpr::Array, "", {elem}}; +} + +TypeExpr map(const TypeExpr& key, const TypeExpr& value) +{ + return {TypeExpr::Map, "", {key, value}}; +} + FieldDecl field(const char* name, const TypeExpr& type) { FieldDecl f; @@ -473,7 +496,10 @@ TEST(LidlGenCdylib, OptionalArgumentMayBeAbsentOrNull) param("maybe", opt(prim("tstr")))})); const QString src = lidlMakeModuleImplExports(m, "OImpl", "o_impl.h"); - EXPECT_TRUE(src.contains("if (args.size() < 1) return nullptr;")) << src.toStdString(); + // The gate counts REQUIRED parameters — the same rule the Rust generator + // applies, so the two report the same `expected` for the same contract. + EXPECT_TRUE(src.contains("if (args.size() < 1) {")) << src.toStdString(); + EXPECT_TRUE(src.contains("\"expected 1 arguments, got \"")) << src.toStdString(); EXPECT_TRUE(src.contains("(args.size() > 1 ? args.at(1) : nlohmann::json())")) << src.toStdString(); EXPECT_TRUE(src.contains("logos::fromJson>")) @@ -483,8 +509,12 @@ TEST(LidlGenCdylib, OptionalArgumentMayBeAbsentOrNull) << src.toStdString(); } -// ...and a method with no optional parameter emits the gate it always did. -TEST(LidlGenCdylib, RequiredOnlyArityGateIsUnchanged) +// A wrong argument COUNT is reported, in the shape logos-rust-sdk's +// args::invalid_args() emits — same three keys, same message text, same origin. +// It used to `return nullptr`, which the Qt glue turns into an empty QVariant: +// "you passed 1 of 2 arguments" was indistinguishable from a successful empty +// answer. +TEST(LidlGenCdylib, WrongArgumentCountReportsInvalidArgs) { ModuleDecl m; m.name = "o_module"; @@ -492,10 +522,32 @@ TEST(LidlGenCdylib, RequiredOnlyArityGateIsUnchanged) {param("a", prim("tstr")), param("b", prim("tstr"))})); const QString src = lidlMakeModuleImplExports(m, "OImpl", "o_impl.h"); - EXPECT_TRUE(src.contains("if (args.size() < 2) return nullptr;")) << src.toStdString(); + EXPECT_TRUE(src.contains("if (args.size() < 2) {")) << src.toStdString(); + EXPECT_TRUE(src.contains("{\"code\", \"invalid_args\"}")) << src.toStdString(); + EXPECT_TRUE(src.contains( + "{\"message\", \"expected 2 arguments, got \" + std::to_string(args.size())}")) + << src.toStdString(); + EXPECT_TRUE(src.contains("{\"origin\", \"o_module\"}")) << src.toStdString(); + EXPECT_TRUE(src.contains("return lidlStrdup(err.dump());")) << src.toStdString(); + // The silent reply is gone from the arity path. + EXPECT_FALSE(src.contains("if (args.size() < 2) return nullptr;")) << src.toStdString(); EXPECT_FALSE(src.contains("args.size() > ")) << src.toStdString(); } +// `args.size()` is unsigned, so `< 0` never fires: a zero-argument method +// carried a dead branch. The Rust generator has always skipped it; now both do. +TEST(LidlGenCdylib, ZeroArgumentMethodEmitsNoArityGate) +{ + ModuleDecl m; + m.name = "o_module"; + m.methods.push_back(method("ping", prim("tstr"), {})); + + const QString src = lidlMakeModuleImplExports(m, "OImpl", "o_impl.h"); + EXPECT_FALSE(src.contains("args.size() < 0")) << src.toStdString(); + EXPECT_FALSE(src.contains("invalid_args")) << src.toStdString(); + EXPECT_TRUE(src.contains("lidlImpl().ping()")) << src.toStdString(); +} + // R4. Optional widens the accepted domain by exactly ONE inhabitant (empty); a // present value is still decoded as T. For `bstr` that has to be the LENIENT // decode a bare `bstr` argument gets, or the identical value would be accepted @@ -554,3 +606,43 @@ TEST(LidlGenCdylib, OptionalEventParamIsConstRefAndNullWhenEmpty) << src.toStdString(); EXPECT_TRUE(src.contains("#include ")) << src.toStdString(); } + +// `{tstr: T}` is the one LIDL type with two C++ spellings (std::map and +// std::unordered_map), and logos_codec.h specializes Codec for both. Naming one +// of them in the generated dispatch made the other a compile error in code the +// author never wrote, so the map slots hand the compiler a proxy / deduce +// instead and let the author's declaration pick. +TEST(LidlGenCdylib, TypedMapBindsTheAuthorsOwnContainer) +{ + ModuleDecl m; + m.name = "o_module"; + m.methods.push_back(method("echoIntMap", map(prim("tstr"), prim("int")), + {param("v", map(prim("tstr"), prim("int")))})); + + const QString src = lidlMakeModuleImplExports(m, "OImpl", "o_impl.h"); + EXPECT_TRUE(src.contains("logos::JsonArg(args.at(0), \"arg0\")")) << src.toStdString(); + EXPECT_TRUE(src.contains("logos::toJson(result)")) << src.toStdString(); + EXPECT_FALSE(src.contains("logos::fromJson>")) + << src.toStdString(); + EXPECT_FALSE(src.contains("logos::toJson>")) + << src.toStdString(); +} + +// ...and only maps. Every other type has one C++ spelling here, and JsonArg +// documents one target it cannot serve — std::optional, whose converting +// constructor out-ranks the proxy's conversion operator, so an empty optional +// would decode as a wrong-typed X and throw. +TEST(LidlGenCdylib, NonMapSlotsStillNameTheirType) +{ + ModuleDecl m; + m.name = "o_module"; + m.methods.push_back(method("f", prim("bool"), + {param("a", arr(prim("int"))), + param("b", opt(map(prim("tstr"), prim("tstr"))))})); + + const QString src = lidlMakeModuleImplExports(m, "OImpl", "o_impl.h"); + EXPECT_TRUE(src.contains("logos::fromJson>")) << src.toStdString(); + EXPECT_TRUE(src.contains("logos::fromJson>>")) + << src.toStdString(); + EXPECT_FALSE(src.contains("logos::JsonArg")) << src.toStdString(); +}