mirror of
https://github.com/logos-co/logos-cpp-sdk.git
synced 2026-08-31 01:31:10 +00:00
`? maybe: tstr` and `maybe: ?tstr` are the same declaration. logos-lidl's
docs/spec.md says so and requires them to produce byte-identical code, and
logos-lidl#7 added fieldIsOptional()/fieldValueType() precisely so no backend
re-derives the answer. The cdylib, client-stub and both Rust backends honour
that. The LEGACY consumer path — the one every real C++ module builds through,
`logos-cpp-generator --general-only --dep <name>=<lidl>` from
logos-plugin-qt's buildPlugin.nix — did not:
? maybe: tstr -> QString maybe{}; __m.value("maybe").toString()
maybe: ?tstr -> QVariant maybe{}; __m.value("maybe")
and on `--api-style lp`, `std::string maybe{}` vs `LogosMap maybe{}` — neither
of them std::optional. The flag spelling is the one production contracts use:
logos-chat-module writes all five of its optionals that way, so every one of
them landed on the branch that silently defaults. A `QString` has no empty
inhabitant at all; an absent `nickname` and an empty-string one were the same
value by the time the consumer saw them.
ROOT CAUSE. legacy/main.cpp's moduleRecordsToJson / moduleMethodsToJson /
moduleEventsToJson flatten every TypeExpr into a single Qt TYPE-NAME STRING.
Answering "is this optional" from a name means answering it from the verbatim
spelling, which is the one thing the accessors exist to stop.
WHAT THIS CHANGES. The record-field half of that boundary, and only it. A field
object now carries `optional` alongside `type`, where `type` is the VALUE type
(fieldValueType) and `optional` is true for either spelling (fieldIsOptional).
Both spellings arrive at the emitter as the same object, so they leave as the
same code. Per surface, matching the sibling backend that already serves it:
Qt — QVariant, as lidl_gen_client.cpp already emits. Qt has no optional
template; an invalid QVariant is its single empty inhabitant. Two-state,
untyped.
Lp — std::optional<T>, as lidl_gen_cdylib.cpp already emits, encoded by
logos-protocol's Codec<std::optional<T>>. Keeps the value type.
`?any` / `?{K:V}` / `?[any]` collapse onto the bare LogosMap/LogosList:
nlohmann::json already carries null, so wrapping it would give the slot
two empty spellings — three states, which the two-state rule forbids.
Encode omits the key when empty (a record field is a NAMED slot); decode treats
an absent key and an explicit null as the same state, so neither turns empty
into "" or 0. The round trip is canonicalising, as the spec requires.
The three functions move to legacy/lidl_to_json.{h,cpp}. Not cosmetic: the rule
is a property of frontend -> JSON -> emitter, and while they sat inside a TU
with main() no test could observe it. tests/generator/test_optional_spellings.cpp
now runs that composition end to end.
WHAT THIS DOES NOT CHANGE, and why. POSITIONAL slots — method parameters,
return types, event parameters — are still flattened to QVariant (Qt) /
LogosMap (Lp). They have no name to hang a flag on, so they only ever had the
type-kind spelling and there is no divergence there to fix; what they lose is
the value type. Closing that changes generated method SIGNATURES, i.e. a source
break for every existing call site, for a defect this commit is not about.
`OptionalSpellings.PositionalSlotsAreStillFlattened` pins the current behaviour
so closing it later is deliberate, and the generator still prints a `Note:`
naming every slot it flattens. Nesting, map key types and descriptions still do
not cross the boundary either — the flag is per-field, not a general widening.
VERIFIED BY RUNNING, each check shown to fail when the property does not hold:
- Two contracts identical but for the spelling, generated with `--dep` on both
`--api-style qt` and `--api-style lp`: byte-identical. The SAME comparison on
a generator built from this base without the fix reports a difference on
both styles.
- Harness sensitivity: two contracts differing only in `count: uint` ->
`count: int` are reported as different, so the compare is not vacuous.
- A contract with no optional field generates byte-identically to the
unpatched generator, both styles — and the same compare reports a difference
when given genuinely different output.
- The 5 new assertions FAIL on this base with only the extraction grafted in
(generator_lib.cpp pristine) and pass with the fix; the 3 control assertions
pass on both, which is what makes them controls.
- Generated wrappers compile on both surfaces, for the probe contracts and for
the real logos-chat-module and test_fullapi_ext_rust contracts.
- Emitted lp record codec exercised at runtime: empty omits the key, an
explicit null decodes to nullopt rather than "", uint64 above 2^63 survives,
bstr stays canonically tagged, and `{"maybe": null}` re-encodes omitted.
- `nix build .#tests` green (107 tests), `nix build .#cpp-generator` green.
logos-qt-sdk's qt-generator has the same defect in lidl_gen_qt_consumer.cpp
(record fields read `f.type` directly); it is a separate repo and not touched
here.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
253 lines
10 KiB
C++
253 lines
10 KiB
C++
// One declaration, two spellings, one binding.
|
|
//
|
|
// A LIDL record field may be marked optional two ways — the field flag
|
|
// (`? name: T`) and the type kind (`name: ?T`) — and logos-lidl's docs/spec.md
|
|
// binds them to ONE meaning: "They are identical in meaning and MUST produce
|
|
// byte-identical generated code."
|
|
//
|
|
// The legacy consumer path is the one every real C++ module builds through
|
|
// (`--dep <name>=<lidl>`), and it did not honour that. It flattened each
|
|
// TypeExpr into a single Qt type NAME before the emitter saw it, which answers
|
|
// the optionality question by reading the verbatim spelling: the flag form kept
|
|
// T (so `? maybe: tstr` became a bare `QString` that has no empty inhabitant at
|
|
// all and silently defaults to "") while the type form collapsed to QVariant.
|
|
// One contract, two bindings — and the flag form is the one production
|
|
// contracts use.
|
|
//
|
|
// These tests run the whole path, LIDL text -> JSON surface -> emitted code,
|
|
// because the invariant is a property of the composition: neither half can be
|
|
// asserted alone. They are written to fail loudly if the two spellings are ever
|
|
// reconciled by making BOTH of them wrong, which a pure equality assertion
|
|
// would happily accept — see OptionalityActuallySurvives*.
|
|
|
|
#include <gtest/gtest.h>
|
|
|
|
#include "generator_lib.h"
|
|
#include "lidl_to_json.h"
|
|
|
|
namespace {
|
|
|
|
// The same contract twice: flag spelling, then type-kind spelling. Every
|
|
// round-trippable shape, plus a record, a list of records, and the untyped
|
|
// carriers (`any`, `{tstr: any}`) that must NOT gain a second empty state.
|
|
const char* kFlagSpelling = R"LIDL(
|
|
module probe {
|
|
version "0.1.0"
|
|
type Inner {
|
|
x: int
|
|
}
|
|
type Rec {
|
|
always: tstr
|
|
? maybe: tstr
|
|
? count: uint
|
|
? blob: bstr
|
|
? nested: Inner
|
|
? items: [tstr]
|
|
? recs: [Inner]
|
|
? amap: {tstr: any}
|
|
? anyv: any
|
|
}
|
|
method get() -> Rec
|
|
event changed(r: Rec)
|
|
}
|
|
)LIDL";
|
|
|
|
const char* kTypeSpelling = R"LIDL(
|
|
module probe {
|
|
version "0.1.0"
|
|
type Inner {
|
|
x: int
|
|
}
|
|
type Rec {
|
|
always: tstr
|
|
maybe: ?tstr
|
|
count: ?uint
|
|
blob: ?bstr
|
|
nested: ?Inner
|
|
items: ?[tstr]
|
|
recs: ?[Inner]
|
|
amap: ?{tstr: any}
|
|
anyv: ?any
|
|
}
|
|
method get() -> Rec
|
|
event changed(r: Rec)
|
|
}
|
|
)LIDL";
|
|
|
|
// Same contract with nothing optional — the control that the change is inert
|
|
// for every contract that does not use the feature.
|
|
const char* kNoOptional = R"LIDL(
|
|
module probe {
|
|
version "0.1.0"
|
|
type Inner {
|
|
x: int
|
|
}
|
|
type Rec {
|
|
always: tstr
|
|
maybe: tstr
|
|
}
|
|
method get() -> Rec
|
|
event changed(r: Rec)
|
|
}
|
|
)LIDL";
|
|
|
|
ModuleDecl parseOrDie(const char* src)
|
|
{
|
|
LidlParseResult pr = lidlParse(QString::fromUtf8(src));
|
|
EXPECT_FALSE(pr.hasError()) << pr.error << " (line " << pr.errorLine << ")";
|
|
return pr.module;
|
|
}
|
|
|
|
struct Emitted { QString header; QString source; };
|
|
|
|
Emitted emitFor(const char* lidl, ApiStyle style)
|
|
{
|
|
const ModuleDecl mod = parseOrDie(lidl);
|
|
const QJsonArray methods = moduleMethodsToJson(mod);
|
|
const QJsonArray events = moduleEventsToJson(mod);
|
|
const QJsonArray records = moduleRecordsToJson(mod);
|
|
Emitted e;
|
|
e.header = makeHeader("probe", "Probe", methods, style, events, BindMode::Static, records);
|
|
e.source = makeSource("probe", "Probe", "probe_api.h", methods, style, events,
|
|
BindMode::Static, records);
|
|
return e;
|
|
}
|
|
|
|
} // namespace
|
|
|
|
// The invariant, stated where it is actually decided: the two spellings reach
|
|
// the emitter as the SAME object. Everything downstream follows from this, and
|
|
// a failure here localises the bug to the boundary rather than the emitter.
|
|
TEST(OptionalSpellings, BoundaryJsonIsIdentical)
|
|
{
|
|
const QJsonArray flag = moduleRecordsToJson(parseOrDie(kFlagSpelling));
|
|
const QJsonArray type = moduleRecordsToJson(parseOrDie(kTypeSpelling));
|
|
EXPECT_EQ(flag, type);
|
|
}
|
|
|
|
TEST(OptionalSpellings, QtOutputIsByteIdentical)
|
|
{
|
|
const Emitted flag = emitFor(kFlagSpelling, ApiStyle::Qt);
|
|
const Emitted type = emitFor(kTypeSpelling, ApiStyle::Qt);
|
|
EXPECT_EQ(flag.header, type.header);
|
|
EXPECT_EQ(flag.source, type.source);
|
|
}
|
|
|
|
TEST(OptionalSpellings, LpOutputIsByteIdentical)
|
|
{
|
|
const Emitted flag = emitFor(kFlagSpelling, ApiStyle::Lp);
|
|
const Emitted type = emitFor(kTypeSpelling, ApiStyle::Lp);
|
|
EXPECT_EQ(flag.header, type.header);
|
|
EXPECT_EQ(flag.source, type.source);
|
|
}
|
|
|
|
// Equality alone is satisfied by two identically WRONG outputs — e.g. by
|
|
// dropping optionality from both spellings. These pin what the agreed answer
|
|
// has to be.
|
|
//
|
|
// Qt: an invalid QVariant is the surface's single empty inhabitant, so `?T` is
|
|
// QVariant — two-state, untyped. Same answer the client-stub backend gives for
|
|
// the same surface (lidl_gen_client.cpp), deliberately.
|
|
TEST(OptionalSpellings, OptionalityActuallySurvivesOnQt)
|
|
{
|
|
for (const char* lidl : {kFlagSpelling, kTypeSpelling}) {
|
|
const Emitted e = emitFor(lidl, ApiStyle::Qt);
|
|
// The declared type is not the value type — a bare QString could not be
|
|
// empty at all.
|
|
EXPECT_TRUE(e.header.contains("QVariant maybe{};")) << e.header.toStdString();
|
|
EXPECT_TRUE(e.header.contains("QVariant count{};")) << e.header.toStdString();
|
|
EXPECT_FALSE(e.header.contains("QString maybe{};")) << e.header.toStdString();
|
|
// A record field is a NAMED slot: empty omits the key.
|
|
EXPECT_TRUE(e.source.contains("if (v.maybe.isValid()) __m.insert")) << e.source.toStdString();
|
|
// Absent and null are the same state on decode: no conversion, which
|
|
// would have turned empty into "".
|
|
EXPECT_TRUE(e.source.contains(
|
|
"__out.maybe = __m.value(QStringLiteral(\"maybe\"));")) << e.source.toStdString();
|
|
// Required fields keep their typed conversion.
|
|
EXPECT_TRUE(e.source.contains(
|
|
"__out.always = __m.value(QStringLiteral(\"always\")).toString();")) << e.source.toStdString();
|
|
}
|
|
}
|
|
|
|
// Lp: the std surface HAS an optional, so it keeps the value type. Same answer
|
|
// the cdylib backend gives, and the encoder that pairs with it is
|
|
// logos-protocol's Codec<std::optional<T>>.
|
|
TEST(OptionalSpellings, OptionalityActuallySurvivesOnLp)
|
|
{
|
|
for (const char* lidl : {kFlagSpelling, kTypeSpelling}) {
|
|
const Emitted e = emitFor(lidl, ApiStyle::Lp);
|
|
EXPECT_TRUE(e.header.contains("std::optional<std::string> maybe{};")) << e.header.toStdString();
|
|
EXPECT_TRUE(e.header.contains("std::optional<uint64_t> count{};")) << e.header.toStdString();
|
|
EXPECT_TRUE(e.header.contains("std::optional<std::vector<uint8_t>> blob{};")) << e.header.toStdString();
|
|
EXPECT_TRUE(e.header.contains("std::optional<Inner> nested{};")) << e.header.toStdString();
|
|
EXPECT_TRUE(e.header.contains("std::optional<std::vector<std::string>> items{};")) << e.header.toStdString();
|
|
EXPECT_TRUE(e.header.contains("std::optional<std::vector<Inner>> recs{};")) << e.header.toStdString();
|
|
EXPECT_TRUE(e.header.contains("#include <optional>\n")) << e.header.toStdString();
|
|
// Named slot: empty omits the key; the value is encoded exactly as the
|
|
// non-optional field would be (bytes still canonically tagged).
|
|
EXPECT_TRUE(e.source.contains(
|
|
"if (v.blob.has_value()) __j[\"blob\"] = logos::bytesToJson((*v.blob));")) << e.source.toStdString();
|
|
// Absent AND explicit null both leave it nullopt. A `contains`-only
|
|
// guard would decode null through the value conversion and turn empty
|
|
// into 0 / "".
|
|
EXPECT_TRUE(e.source.contains(
|
|
"if (w.contains(\"maybe\") && !w.at(\"maybe\").is_null())")) << e.source.toStdString();
|
|
}
|
|
}
|
|
|
|
// `?any`, `?{K:V}` and `?[any]` are carried as nlohmann::json, which ALREADY
|
|
// has null among its inhabitants. Wrapping those in std::optional would give
|
|
// them two distinct empty spellings — three states, which the two-state rule
|
|
// forbids. They collapse onto the bare alias instead. (The cdylib backend makes
|
|
// exactly this exception; the Qt surface needs none, because `any` is QVariant
|
|
// there either way.)
|
|
TEST(OptionalSpellings, UntypedJsonAliasesDoNotGainASecondEmptyState)
|
|
{
|
|
for (const char* lidl : {kFlagSpelling, kTypeSpelling}) {
|
|
const Emitted e = emitFor(lidl, ApiStyle::Lp);
|
|
EXPECT_TRUE(e.header.contains("LogosMap amap{};")) << e.header.toStdString();
|
|
EXPECT_TRUE(e.header.contains("LogosMap anyv{};")) << e.header.toStdString();
|
|
EXPECT_FALSE(e.header.contains("std::optional<LogosMap>")) << e.header.toStdString();
|
|
EXPECT_FALSE(e.header.contains("std::optional<LogosList>")) << e.header.toStdString();
|
|
}
|
|
}
|
|
|
|
// A contract that declares no optional must be untouched by all of the above —
|
|
// including the conditional `#include <optional>`, whose whole point is that it
|
|
// is conditional.
|
|
TEST(OptionalSpellings, ContractsWithoutOptionalsAreUnaffected)
|
|
{
|
|
const Emitted qt = emitFor(kNoOptional, ApiStyle::Qt);
|
|
EXPECT_TRUE(qt.header.contains("QString maybe{};")) << qt.header.toStdString();
|
|
EXPECT_TRUE(qt.source.contains(
|
|
"__out.maybe = __m.value(QStringLiteral(\"maybe\")).toString();")) << qt.source.toStdString();
|
|
EXPECT_FALSE(qt.source.contains("isValid()) __m.insert")) << qt.source.toStdString();
|
|
|
|
const Emitted lp = emitFor(kNoOptional, ApiStyle::Lp);
|
|
EXPECT_TRUE(lp.header.contains("std::string maybe{};")) << lp.header.toStdString();
|
|
EXPECT_FALSE(lp.header.contains("std::optional")) << lp.header.toStdString();
|
|
EXPECT_FALSE(lp.header.contains("#include <optional>")) << lp.header.toStdString();
|
|
EXPECT_FALSE(lp.source.contains("has_value()")) << lp.source.toStdString();
|
|
}
|
|
|
|
// A POSITIONAL slot (method parameter, return type, event parameter) has no
|
|
// name to hang a flag on, so it has only the type-kind spelling and there is no
|
|
// divergence to fix — but it is also still flattened to an untyped carrier,
|
|
// which is the part of the gap this change does NOT close. Pinned so the
|
|
// remaining gap is a documented fact rather than an assumption, and so that
|
|
// closing it later is a deliberate, visible change.
|
|
TEST(OptionalSpellings, PositionalSlotsAreStillFlattened)
|
|
{
|
|
const char* lidl = R"LIDL(
|
|
module probe {
|
|
version "0.1.0"
|
|
method echo(v: ?tstr) -> ?tstr
|
|
}
|
|
)LIDL";
|
|
const Emitted qt = emitFor(lidl, ApiStyle::Qt);
|
|
EXPECT_TRUE(qt.header.contains("QVariant echo(QVariant v")) << qt.header.toStdString();
|
|
const Emitted lp = emitFor(lidl, ApiStyle::Lp);
|
|
EXPECT_TRUE(lp.header.contains("LogosMap echo(const LogosMap& v")) << lp.header.toStdString();
|
|
EXPECT_FALSE(lp.header.contains("std::optional")) << lp.header.toStdString();
|
|
}
|