diff --git a/README.md b/README.md index 560dd53..e3a7e6e 100644 --- a/README.md +++ b/README.md @@ -318,10 +318,16 @@ honest. Both `foo(…, &err)` and `fooAsyncResult` on this surface also fold a provider **rejection** into the error, matching the Qt path: a provider that ran and -refused answers `{"code": "dispatch_failed", …}` as its *result*, which the -return decode would otherwise erase into a default value. `fooAsync` still +refused answers `{"code": …, "message": …, "origin": …}` as its *result*, which +the return decode would otherwise erase into a default value. `fooAsync` still cannot report it — its callback has nowhere to put it. +`code` is matched against a closed set — `dispatch_failed`, `invalid_args`, +`unknown_method` — held in one place (`kRejectionCodes`, `generator_lib.cpp`) +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. + ### 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 555086d..3325b2c 100644 --- a/cpp-generator/docs/project.md +++ b/cpp-generator/docs/project.md @@ -347,7 +347,7 @@ Fixture files in `tests/experimental/fixtures/`: two answers. `?bstr` is unaffected either way: the tag lives in the value, not the slot. - **A provider REJECTION reaches `…Async`'s callback only as a log line** (but `…AsyncResult`'s callback gets it properly). A provider that refuses a call answers the - canonical `{"code":"dispatch_failed", "message":…, "origin":…}` object as its RESULT, not + canonical `{"code":…, "message":…, "origin":…}` object as its RESULT, not as a transport error, and the Qt return table would convert it like any other value — erasing it (`_result.toList()` on that map is `[]`). The Qt consumer emitter therefore detects it (`logosDispatchRejection`, emitted once per wrapper) and folds it into the @@ -355,7 +355,20 @@ Fixture files in `tests/experimental/fixtures/`: - **sync** — the `logos::CallError*` out-parameter, so `mod.echoUintList(v, &err)` can tell a rejection from an empty return; - **`…AsyncResult`** — `logos::AsyncResult::error`, so `r.ok()` is false and - `r.error.code == "dispatch_failed"` exactly as on the sync path. + `r.error.code` carries the provider's code exactly as on the sync path. + + `code` is matched against a **closed set** — `kRejectionCodes` in `generator_lib.cpp`, + the single source of truth both emitters build their condition from: + `dispatch_failed` (the provider refused the argument VALUES), `invalid_args` (wrong + argument COUNT) and `unknown_method`. It was the single literal `dispatch_failed` until + the arity code was found to be live and undetected — `experimental/lidl_gen_cdylib.cpp` + and logos-rust-sdk's `args::invalid_args` have both emitted `invalid_args` all along, + so a missing argument reached a typed consumer as a *successful* call returning a + three-key map. `unknown_method` is in the set before any provider emits it: widening a + detector is backwards-compatible on its own, whereas a new provider code shipped against + narrow detectors would arrive as data. The set stays CLOSED — a method may legitimately + return a three-string map, so matching the shape alone would let user data impersonate a + refusal. The historical **`…Async`** overload is the one exception: its callback is `std::function`, and adding an error parameter would change a generated public diff --git a/cpp-generator/generator_lib.cpp b/cpp-generator/generator_lib.cpp index ff9dbb0..8aa146a 100644 --- a/cpp-generator/generator_lib.cpp +++ b/cpp-generator/generator_lib.cpp @@ -799,13 +799,91 @@ QString makeHeader(const QString& moduleName, const QString& className, const QJ return h; } +// --------------------------------------------------------------------------- +// The provider REJECTION codes, as a CLOSED SET. +// +// This array is the single source of truth for BOTH detectors emitted below — +// the nlohmann::json one for the lp surface and the QVariant one for the Qt +// surface. They used to spell the literal out separately, which is exactly how +// two detectors drift apart; the condition text is now built from here, so a +// code added to this array reaches both emitters or neither. +// +// Why a closed set and not "any {code,message,origin} object": a method may +// legitimately RETURN a three-string map, and an `any` return certainly can. +// Matching the shape alone would let user data impersonate a refusal. The set +// is what keeps the in-band signal narrow. +// +// "dispatch_failed" — the provider ran and refused well-formed-looking +// arguments (a type it could not decode). +// "invalid_args" — wrong argument COUNT. Emitted today by the generated +// cdylib dispatch (experimental/lidl_gen_cdylib.cpp) and +// by logos-rust-sdk `args::invalid_args`, and until now +// detected by nobody: `logosctl call m isPositive` with +// the argument missing exited 0 with status "ok" and the +// refusal object as its RESULT. +// "unknown_method" — NOT emitted by any provider yet. Listed now on purpose. +// logos_protocol.h records that an unknown method is +// currently answered with a bare null, indistinguishable +// from a legitimate null return, and that closing it +// needs a provider-contract change across the SDKs. The +// detector has to be widened FIRST: widening is +// backwards-compatible on its own (nothing emits the code, +// so nothing changes), whereas a new provider code shipped +// against old detectors would arrive at consumers as DATA +// — the same silent-success bug, freshly minted. +// +// WHY THIS IS A PER-REPO CONSTANT AND NOT A SHARED ONE. There are five copies of +// this detector: the two emitted below, logos-qt-sdk's byte-identical +// lidl_gen_qt_consumer.cpp, logos-rust-sdk's args::as_dispatch_rejection, and +// logos-logoscore-cli's core_service/call_envelope.cpp. Two candidate shared +// homes were considered and both rejected FOR NOW: +// +// * a shared EMITTER in share/lidl-frontend (which already ships +// lidl_emit_common to logos-qt-generator, so the channel exists). It would +// collapse 2 of the 5 — not the QVariant twin, not Rust, not core_service — +// and it would make logos-qt-sdk's commit depend on this one plus a pin +// bump, turning four independently landable fixes into an ordered stack for +// no behavioural gain. +// * a runtime predicate in logos-protocol that the generated code CALLS. This +// is the principled end state, and it is how the analogous CONVERSION +// duplication was actually solved (logos_json_convert, reached through +// logos_qt_lp_bridge.h) rather than by sharing an emitter. It is a separate +// change because it converts a TEXT-level duplication into a BUILD-level +// version coupling: the emitted body is self-contained today, so a wrapper +// generated by any generator compiles against any logos-protocol a module +// happens to pin. Calling a protocol symbol ends that. +// +// So: copies stay, and each repo holds the vocabulary in ONE named place so a +// drift between them is visible rather than silent. Here that place is this +// array, and BOTH emitters below build their condition from it. +// --------------------------------------------------------------------------- +static const char* const kRejectionCodes[] = { + "dispatch_failed", "invalid_args", "unknown_method", +}; + +// `c != "a" && c != "b" && ...` over kRejectionCodes, with each literal passed +// through `wrap` (identity for std::string, QStringLiteral for QString). +static QString rejectionCodeMismatch(const QString& var, + QString (*wrap)(const char*), + const QString& joinIndent) +{ + QStringList terms; + for (const char* code : kRejectionCodes) + terms << var + " != " + wrap(code); + return terms.join("\n" + joinIndent + "&& "); +} + +static QString plainLiteral(const char* c) { return QString("\"") + c + "\""; } +static QString qtLiteral(const char* c) { return QString("QStringLiteral(\"") + c + "\")"; } + // The Qt consumer's rejection detector, emitted once per generated wrapper. // // A provider that REJECTS a call answers the canonical -// {"code":"dispatch_failed", "message":..., "origin":...} object as its RESULT, -// not as a transport error. Every provider flavour produces the same object -// (logos-qt-sdk `dispatchFailedVariant`, the generated cdylib dispatch, the Rust -// provider's `args::dispatch_failed`), and the Qt return table converts it like +// {"code":..., "message":..., "origin":...} object as its RESULT, with `code` +// drawn from kRejectionCodes above, not as a transport error. Every provider +// flavour produces the same object (logos-qt-sdk `dispatchFailedVariant`, the +// generated cdylib dispatch, logos-rust-sdk's `args::dispatch_failed` and +// `args::invalid_args`), and the Qt return table converts it like // any other value — which ERASES it: `_result.toList()` on a map is `[]`, // `.toString()` is "", `.toLongLong()` is 0. A caller then cannot tell "you sent // me the wrong thing" from "the provider returned nothing". @@ -819,9 +897,9 @@ QString makeHeader(const QString& moduleName, const QString& className, const QJ // several of these in ONE translation unit. Internal linkage handles the // separate-TU case; only the preprocessor handles this one. // The Qt-free twin of emitDispatchRejectionDetector, for the lp surface, whose -// results arrive as nlohmann::json rather than QVariant. Same exact match on -// the same three string fields and the same code, for the same reason: an `any` -// or map return carrying user data must never false-match. +// results arrive as nlohmann::json rather than QVariant. Same match on the same +// three string fields against the same closed code set, for the same reason: an +// `any` or map return carrying user data must never false-match. // // Guarded identically — the umbrella (`logos_sdk.cpp`) textually #includes every // generated `_api.cpp`, so a module with more than one dependency puts @@ -844,8 +922,9 @@ static void emitDispatchRejectionDetectorJson(QTextStream& s) s << " auto code = v.find(\"code\"), message = v.find(\"message\"), origin = v.find(\"origin\");\n"; s << " if (code == v.end() || message == v.end() || origin == v.end()) return false;\n"; s << " if (!code->is_string() || !message->is_string() || !origin->is_string()) return false;\n"; - s << " if (code->get() != \"dispatch_failed\") return false;\n"; - s << " out.code = code->get();\n"; + s << " const std::string _code = code->get();\n"; + s << " if (" << rejectionCodeMismatch("_code", plainLiteral, " ") << ") return false;\n"; + s << " out.code = _code;\n"; s << " out.message = message->get();\n"; s << " out.origin = origin->get();\n"; s << " return true;\n"; @@ -862,9 +941,10 @@ static void emitDispatchRejectionDetector(QTextStream& s) s << "// True when `v` is the canonical provider REJECTION object rather than a\n"; s << "// value; fills `out` with its {code, message, origin} on a match.\n"; s << "//\n"; - s << "// The match is exact — those three fields, all strings, and that code — for the\n"; - s << "// same reason logos_rpc_status.h's isUnauthorizedSentinel is exact: an `any` or\n"; - s << "// map return carrying user data must never false-match.\n"; + s << "// The match is narrow — those three fields, all strings, and a code from the\n"; + s << "// CLOSED SET above — for the same reason logos_rpc_status.h's\n"; + s << "// isUnauthorizedSentinel is exact: an `any` or map return carrying user data\n"; + s << "// must never false-match. Any other code stays DATA.\n"; s << "bool logosDispatchRejection(const QVariant& v, logos::CallError& out)\n"; s << "{\n"; s << " QVariantMap m;\n"; @@ -881,8 +961,9 @@ static void emitDispatchRejectionDetector(QTextStream& s) s << " if (code.userType() != QMetaType::QString\n"; s << " || message.userType() != QMetaType::QString\n"; s << " || origin.userType() != QMetaType::QString) return false;\n"; - s << " if (code.toString() != QStringLiteral(\"dispatch_failed\")) return false;\n"; - s << " out.code = code.toString().toStdString();\n"; + s << " const QString _code = code.toString();\n"; + s << " if (" << rejectionCodeMismatch("_code", qtLiteral, " ") << ") return false;\n"; + s << " out.code = _code.toStdString();\n"; s << " out.message = message.toString().toStdString();\n"; s << " out.origin = origin.toString().toStdString();\n"; s << " return true;\n"; diff --git a/tests/generator/test_async_result.cpp b/tests/generator/test_async_result.cpp index 3c8f9fa..c0ebc27 100644 --- a/tests/generator/test_async_result.cpp +++ b/tests/generator/test_async_result.cpp @@ -271,6 +271,46 @@ TEST(AsyncResult, LpDispatchRejectionDetectorIsEmittedOnceAndGuarded) EXPECT_TRUE(src.contains("#define LOGOS_GENERATED_DISPATCH_REJECTION_JSON")); } +// The two emitters in this repo (this nlohmann one and the QVariant one in +// test_make_source.cpp) are driven by ONE kRejectionCodes array in +// generator_lib.cpp, so they cannot drift. These assert the array reached the +// emitted text on this side too. +TEST(AsyncResult, LpDispatchRejectionDetectorMatchesTheClosedCodeSet) +{ + const QString src = lpSource(); + EXPECT_TRUE(src.contains( + " if (_code != \"dispatch_failed\"\n" + " && _code != \"invalid_args\"\n" + " && _code != \"unknown_method\") return false;\n")) + << src.toStdString(); + // "invalid_args" is the code that was LIVE and undetected: the cdylib + // dispatch (experimental/lidl_gen_cdylib.cpp) and logos-rust-sdk's + // args::invalid_args both answer an arity error with it, and a consumer + // decoded it as a three-key map. "unknown_method" is inert until providers + // emit it; it is here so the detector is ready before they do. +} + +TEST(AsyncResult, LpDispatchRejectionDetectorMatchesNoOtherCode) +{ + // The negatives, at the only level this text-emitting generator can pin + // them: the guards that make an unrecognised code, a 2- or 4-key object and + // a non-string value all stay DATA must still be there. Widening the code + // literal into a set must not have loosened the SHAPE. + const QString src = lpSource(); + const int begin = src.indexOf("bool logosDispatchRejectionJson("); + ASSERT_NE(begin, -1); + const QString body = src.mid(begin, src.indexOf("} // namespace", begin) - begin); + EXPECT_TRUE(body.contains("if (!v.is_object() || v.size() != 3) return false;")); + EXPECT_TRUE(body.contains( + "if (code == v.end() || message == v.end() || origin == v.end()) return false;")); + EXPECT_TRUE(body.contains( + "if (!code->is_string() || !message->is_string() || !origin->is_string()) return false;")); + // Exactly three comparisons, one per code — not a prefix test, not a + // "has a code key" shortcut. + EXPECT_EQ(body.count("_code != \""), 3) << body.toStdString(); + EXPECT_LT(body.indexOf("_code != \"unknown_method\""), body.indexOf("return true;")); +} + TEST(AsyncResult, LpContractWithNoInvokableMethodEmitsNoDetector) { // The detector is only reachable from a method body; emitting it anyway is diff --git a/tests/generator/test_make_source.cpp b/tests/generator/test_make_source.cpp index 325a11b..926151a 100644 --- a/tests/generator/test_make_source.cpp +++ b/tests/generator/test_make_source.cpp @@ -304,11 +304,18 @@ TEST(MakeSourceTest, NonInvokableSkipped) // ─── The provider REJECTION envelope on the return path ───────────────────── // // A provider that refuses a call answers the canonical -// {"code":"dispatch_failed", "message":…, "origin":…} object as its RESULT. The -// Qt return table converts it like any other value, which ERASES it — a rejected -// `[uint]` call answered `[]`, indistinguishable from "the provider returned -// nothing". These pin the consumer folding it into the CallError out-channel the -// wrapper already uses for a failed call. +// {"code":…, "message":…, "origin":…} object as its RESULT. The Qt return table +// converts it like any other value, which ERASES it — a rejected `[uint]` call +// answered `[]`, indistinguishable from "the provider returned nothing". These +// pin the consumer folding it into the CallError out-channel the wrapper already +// uses for a failed call. +// +// `code` is matched against a CLOSED SET (kRejectionCodes in generator_lib.cpp), +// not against the single literal "dispatch_failed" it once was. Providers have +// emitted "invalid_args" for an arity error all along and no detector matched +// it, so a missing argument reached a typed consumer as a successful call +// returning a map. "unknown_method" is in the set before anything emits it: a +// detector can be widened compatibly on its own, a provider code cannot. TEST(MakeSourceTest, QtEmitsRejectionDetector) { @@ -316,10 +323,39 @@ TEST(MakeSourceTest, QtEmitsRejectionDetector) methods.append(makeMethod("fn", "QVariantList", 1)); QString src = makeSource("mod", "Mod", "mod.h", methods); EXPECT_TRUE(src.contains("bool logosDispatchRejection(const QVariant& v, logos::CallError& out)")); - // Exact match only: an `any` / map return carrying user data must not - // false-match (same discipline as logos_rpc_status.h's sentinel). + // The guards that keep the widened match NARROW. Without them the set + // becomes an open shape match and any `any` / map return carrying user data + // false-matches (same discipline as logos_rpc_status.h's sentinel). EXPECT_TRUE(src.contains("if (m.size() != 3) return false;")); - EXPECT_TRUE(src.contains("if (code.toString() != QStringLiteral(\"dispatch_failed\")) return false;")); + EXPECT_TRUE(src.contains("if (code.userType() != QMetaType::QString")); + // The closed set, in full, as ONE chain of != — so an unrecognised code + // falls through to `return false` and stays DATA. + EXPECT_TRUE(src.contains( + " if (_code != QStringLiteral(\"dispatch_failed\")\n" + " && _code != QStringLiteral(\"invalid_args\")\n" + " && _code != QStringLiteral(\"unknown_method\")) return false;\n")) + << src.toStdString(); +} + +TEST(MakeSourceTest, QtRejectionDetectorMatchesNoOtherCode) +{ + // The negative that stops this becoming an open match by accident. The + // emitted body must compare `_code` against the three literals and nothing + // else — no `contains`, no prefix test, no "has a code key" shortcut. + QJsonArray methods; + methods.append(makeMethod("fn", "QVariantList", 1)); + const QString src = makeSource("mod", "Mod", "mod.h", methods); + const int begin = src.indexOf("bool logosDispatchRejection(const QVariant&"); + ASSERT_NE(begin, -1); + const QString body = src.mid(begin, src.indexOf("} // namespace", begin) - begin); + EXPECT_EQ(body.count("_code != QStringLiteral("), 3) << body.toStdString(); + // Exactly the three codes appear, and `return true` is reached only after + // every guard. + EXPECT_EQ(body.count("\"dispatch_failed\""), 1); + EXPECT_EQ(body.count("\"invalid_args\""), 1); + EXPECT_EQ(body.count("\"unknown_method\""), 1); + EXPECT_LT(body.indexOf("_code != QStringLiteral(\"unknown_method\")"), + body.indexOf("return true;")); } TEST(MakeSourceTest, QtSyncFoldsRejectionIntoCallError)