From f3369faca4e06ac9f89895b11fdcc202f86f00c2 Mon Sep 17 00:00:00 2001 From: Dario Lipicar Date: Mon, 3 Aug 2026 10:19:08 -0300 Subject: [PATCH] feat(generator): async callers can see the error, sync callers can set a deadline (#132) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two consumer surfaces had complementary holes: sync : T foo(params…, logos::CallError* err = nullptr) error yes, timeout NO async: void fooAsync(params…, cb, Timeout = Timeout()) timeout yes, error NO so an async caller could not tell a failed remote call from a provider that legitimately returned 0 / "" / false — the exact ambiguity the sync path's CallError* was added to resolve — and a sync caller could not say how long it was willing to wait, even though the transport overload the generator already calls takes both. Both fixes are additive: T foo(params…, logos::CallError* err = nullptr, Timeout timeout = Timeout()); void fooAsync(params…, std::function cb, Timeout timeout = Timeout()); // unchanged void fooAsyncResult(params…, std::function)> cb, Timeout timeout = Timeout()); // new logos::AsyncResult (new, Qt-free, cpp/logos_async_result.h) is {value, error} plus ok(); AsyncResult carries only the error so every fooAsyncResult has the same callback shape. The name is distinct rather than an overload because std::function)> next to std::function is ambiguous for a generic lambda. Applied to both emitters that produce this surface — legacy/generator_lib.cpp (the module-builder path) and experimental/lidl_gen_client.cpp (`--lidl --module-only`, from a published contract) — since a consumer can reach either for the same contract. The Qt-free (ApiStyle::Lp) surface gets the sync timeout (spelled `int timeout_ms`; `Timeout` lives behind a Qt header) but NOT fooAsyncResult: logos-protocol's lp_invoke_async hard-codes `cb(1, …)`, so an AsyncResult there would report ok() on a failed call. Measured, not assumed. See the note in makeHeaderLp. Co-authored-by: Claude Opus 5 --- README.md | 47 ++++ cpp-generator/docs/project.md | 30 +- .../experimental/lidl_gen_client.cpp | 116 ++++++-- cpp-generator/legacy/generator_lib.cpp | 264 ++++++++++++------ cpp/CMakeLists.txt | 1 + cpp/logos_async_result.h | 78 ++++++ .../cpp-sdk-generator-roundtrip.test.yaml | 26 +- nix/include.nix | 2 +- tests/experimental/test_lidl_gen_client.cpp | 66 +++++ tests/generator/CMakeLists.txt | 1 + tests/generator/test_async_result.cpp | 201 +++++++++++++ tests/generator/test_make_header.cpp | 8 +- tests/generator/test_make_source.cpp | 8 +- tests/generator/test_records.cpp | 2 +- 14 files changed, 716 insertions(+), 134 deletions(-) create mode 100644 cpp/logos_async_result.h create mode 100644 tests/generator/test_async_result.cpp diff --git a/README.md b/README.md index 3003cc2..a40b1ca 100644 --- a/README.md +++ b/README.md @@ -203,6 +203,53 @@ logos-cpp-generator --metadata metadata.json --general-only --output-dir ./gener This approach gives you fine-grained control over which modules to include and allows rebuilding just the umbrella headers without regenerating all module wrappers. +#### The three call surfaces on a generated wrapper + +Every LIDL `method foo(...) -> T` produces three entry points: + +```cpp +// 1. sync — optional error out-channel, optional deadline. Both trailing and +// defaulted, so `dep.foo(a, b)` and `dep.foo(a, b, &err)` are unchanged. +T foo(params…, logos::CallError* err = nullptr, Timeout timeout = Timeout()); + +// 2. async, value only — the historical form, unchanged. +void fooAsync(params…, std::function cb, Timeout timeout = Timeout()); + +// 3. async, value + error. +void fooAsyncResult(params…, std::function)> cb, + Timeout timeout = Timeout()); +``` + +Use (3) whenever a default-constructed `T` is also a legal success value — which +is almost always. `fooAsync` hands the callback a bare `T`, so a failed call and +a provider that genuinely returned `0` / `""` / `false` are indistinguishable; +that is exactly the ambiguity the sync form's `CallError*` exists to resolve. + +```cpp +dep.balanceAsyncResult(account, [](logos::AsyncResult r) { + if (!r.ok()) { // r.error is {code, message, origin} + qWarning() << "balance failed:" << r.error.code.c_str(); + return; + } + use(r.value); // now known to be a real answer +}); +``` + +`logos::AsyncResult` (`logos_async_result.h`) is `{ T value; CallError error; }` +plus `ok()`; `AsyncResult` carries only the error, so a `void`-returning +method has the same callback shape as every other one. + +The name is deliberately distinct rather than an overload of `fooAsync`: two +overloads differing only in `std::function` vs +`std::function)>` are ambiguous for a generic lambda +(`[](auto v){…}`), which would break existing call sites. + +**Qt-free (`--api-style lp`) wrappers** spell the deadline `int timeout_ms = 0` +(`<= 0` selects the protocol default) because `Timeout` lives in a Qt header, +and they do **not** yet get `fooAsyncResult` — logos-protocol's +`lp_invoke_async` does not report the call error to its callback, so an +`AsyncResult` there would report success on a failed call. + ### Universal modules: LogosModuleContext Universal (codegen-driven) modules — those built from a `package_xxx_impl.h` header rather than a handcrafted `QObject` plugin — don't see the raw `LogosAPI` at all. Instead, the codegen-generated provider 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 c246b54..3819f6a 100644 --- a/cpp-generator/docs/project.md +++ b/cpp-generator/docs/project.md @@ -329,16 +329,22 @@ Fixture files in `tests/experimental/fixtures/`: single-`_bytes`-field record is refused under both spellings. It used to read `f.type`, which refused `? _bytes: tstr` and let `_bytes: ?tstr` through — the same declaration, two answers. `?bstr` is unaffected either way: the tag lives in the value, not the slot. -- **A provider REJECTION reaches an async consumer callback only as a log line.** A - provider that refuses a call answers the canonical - `{"code":"dispatch_failed", "message":…, "origin":…}` object as its RESULT, not as a - transport error, and the Qt return table would convert it like any other value — +- **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 + 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 and folds it into the `logos::CallError` out-parameter the sync wrapper - already carries, so `mod.echoUintList(v, &err)` can tell a rejection from an empty - return. The generated `…Async` overload has no such channel — its callback is - `std::function`, and adding an error parameter would change the generated - public surface (which logos-qt-sdk's `qt-generator --backend consumer` veneer mirrors - 1:1) — so an async rejection is reported with `qWarning` and the callback still - receives the default-converted value. Giving async an error channel is an API change, - not a code-generation fix. + detects it (`logosDispatchRejection`, emitted once per wrapper) and folds it into the + error channel of every surface that HAS one: + - **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. + + The historical **`…Async`** overload is the one exception: its callback is + `std::function`, and adding an error parameter would change a generated public + surface (which logos-qt-sdk's `qt-generator --backend consumer` veneer mirrors 1:1). It + is left untouched, so there an async rejection is reported with `qWarning` and the + callback still receives the default-converted value. `…AsyncResult` exists precisely + because giving async an error channel was an API addition rather than a code-generation + fix — a caller that needs to SEE the rejection uses it. diff --git a/cpp-generator/experimental/lidl_gen_client.cpp b/cpp-generator/experimental/lidl_gen_client.cpp index 9a9c94a..280d890 100644 --- a/cpp-generator/experimental/lidl_gen_client.cpp +++ b/cpp-generator/experimental/lidl_gen_client.cpp @@ -264,6 +264,7 @@ QString lidlMakeHeader(const ModuleDecl& module, BindMode bindMode) s << "#include \"logos_api.h\"\n"; s << "#include \"logos_api_client.h\"\n"; s << "#include \"logos_call_error.h\"\n"; + s << "#include \"logos_async_result.h\"\n"; s << "#include \"logos_object.h\"\n\n"; emitRecords(s, module); @@ -288,19 +289,38 @@ QString lidlMakeHeader(const ModuleDecl& module, BindMode bindMode) if (i + 1 < md.params.size()) s << ", "; } // Optional error out-channel: pass a logos::CallError* to distinguish - // a failed remote call from a legitimately default-valued result. + // a failed remote call from a legitimately default-valued result — + // followed by an optional Timeout. Both trailing and defaulted, so + // existing call sites (including ones passing `&err` positionally) + // compile unchanged. Mirrors the legacy emitter in + // legacy/generator_lib.cpp; the two must agree, since a consumer can + // reach either (this one from a published `.lidl`, that one through the + // module builder) for the same contract. if (!md.params.empty()) s << ", "; - s << "logos::CallError* err = nullptr);\n"; + s << "logos::CallError* err = nullptr, Timeout timeout = Timeout());\n"; + + auto emitAsyncParams = [&]() { + for (int i = 0; i < md.params.size(); ++i) { + emitParam(s, lidlTypeToQt(md.params[i].type), md.params[i].name); + if (i + 1 < md.params.size()) s << ", "; + } + if (!md.params.empty()) s << ", "; + }; + QString asyncCb = (ret == "void") ? QString("std::function") : QString("std::function"; s << " void " << md.name << "Async("; - for (int i = 0; i < md.params.size(); ++i) { - emitParam(s, lidlTypeToQt(md.params[i].type), md.params[i].name); - if (i + 1 < md.params.size()) s << ", "; - } - if (!md.params.empty()) s << ", "; + emitAsyncParams(); s << asyncCb << " callback, Timeout timeout = Timeout());\n"; + + // Result-carrying async entry point. Distinct name, not an overload: + // std::function)> alongside std::function + // is ambiguous for a generic lambda. + s << " void " << md.name << "AsyncResult("; + emitAsyncParams(); + s << "std::function)> callback" + << ", Timeout timeout = Timeout());\n"; } s << "\nprivate:\n"; @@ -384,7 +404,7 @@ QString lidlMakeSource(const ModuleDecl& module, BindMode bindMode) if (i + 1 < nParams) s << ", "; } if (nParams > 0) s << ", "; - s << "logos::CallError* err) {\n"; + s << "logos::CallError* err, Timeout timeout) {\n"; // Call through the err-out overload: with a logos::CallError* the // caller can distinguish a failed remote call from a legitimately @@ -404,7 +424,9 @@ QString lidlMakeSource(const ModuleDecl& module, BindMode bindMode) s << qtArgExpr(md.params[i].type, qs(md.params[i].name)); if (i + 1 < nParams) s << ", "; } - s << "), Timeout(), &_err);\n"; + // The caller's deadline, not a hard-coded default: this is the overload + // that carries BOTH the deadline and the error out-channel. + s << "), timeout, &_err);\n"; s << " if (err) *err = _err;\n"; s << " else if (!_err.ok()) qWarning() << \"" << className << "::" << md.name << ": remote call failed:\" << QString::fromStdString(_err.message);\n"; @@ -413,31 +435,63 @@ QString lidlMakeSource(const ModuleDecl& module, BindMode bindMode) s << " " << returnConversionFor(md.returnType, ret) << "\n"; s << "}\n\n"; - s << "void " << className << "::" << md.name << "Async("; - for (int i = 0; i < nParams; ++i) { - emitParam(s, lidlTypeToQt(md.params[i].type), md.params[i].name); - if (i + 1 < nParams) s << ", "; - } - if (nParams > 0) s << ", "; - s << "std::function callback, Timeout timeout) {\n"; - s << " if (!callback) return;\n"; + // Shared between the two async entry points so they cannot drift in how + // they marshal args or decode the reply. + auto emitAsyncParams = [&]() { + for (int i = 0; i < nParams; ++i) { + emitParam(s, lidlTypeToQt(md.params[i].type), md.params[i].name); + if (i + 1 < nParams) s << ", "; + } + if (nParams > 0) s << ", "; + }; // Same one-element-per-arg packing as the sync path (see above): a // QVariantList-typed arg must not be spread across the args list. - s << " m_client->invokeRemoteMethodAsync(" << targetExpr << ", \"" << md.name << "\", packVariantList("; - for (int i = 0; i < nParams; ++i) { - s << qtArgExpr(md.params[i].type, qs(md.params[i].name)); - if (i + 1 < nParams) s << ", "; - } - s << ")"; + auto emitAsyncArgs = [&]() { + s << "packVariantList("; + for (int i = 0; i < nParams; ++i) { + s << qtArgExpr(md.params[i].type, qs(md.params[i].name)); + if (i + 1 < nParams) s << ", "; + } + s << ")"; + }; + // The QVariant -> typed-return expression, given the QVariant's name. + auto asyncDecodeExpr = [&](const QString& var) -> QString { + if (ret == "void") return QString(); + if (ret == "QVariant") return var; + return var + ".isValid() ? " + asyncReturnConversionFor(md.returnType, ret) + + " : " + asyncDefaultVal(ret); + }; + + s << "void " << className << "::" << md.name << "Async("; + emitAsyncParams(); + s << "std::function callback, Timeout timeout) {\n"; + s << " if (!callback) return;\n"; + s << " m_client->invokeRemoteMethodAsync(" << targetExpr << ", \"" << md.name << "\", "; + emitAsyncArgs(); + // ONE-argument lambda -> LogosAPIClient::AsyncResultCallback, i.e. the + // historical value-only transport overload. s << ", [callback](QVariant v) {\n"; - if (ret == "void") { - s << " callback();\n"; - } else if (ret == "QVariant") { - s << " callback(v);\n"; - } else { - s << " callback(v.isValid() ? " << asyncReturnConversionFor(md.returnType, ret) - << " : " << asyncDefaultVal(ret) << ");\n"; - } + if (ret == "void") s << " callback();\n"; + else s << " callback(" << asyncDecodeExpr("v") << ");\n"; + s << " }, timeout);\n"; + s << "}\n\n"; + + // Result-carrying async: a TWO-argument lambda, so it binds to the + // transport's CallError-aware AsyncResultErrorCallback overload. The + // value on failure is exactly what `Async` would have delivered; + // what changes is that the callback can now tell. + s << "void " << className << "::" << md.name << "AsyncResult("; + emitAsyncParams(); + s << "std::function)> callback, Timeout timeout) {\n"; + s << " if (!callback) return;\n"; + s << " m_client->invokeRemoteMethodAsync(" << targetExpr << ", \"" << md.name << "\", "; + emitAsyncArgs(); + s << ", [callback](QVariant v, const logos::CallError& _err) {\n"; + s << " logos::AsyncResult<" << ret << "> _r;\n"; + s << " _r.error = _err;\n"; + if (ret == "void") s << " (void)v;\n"; + else s << " _r.value = " << asyncDecodeExpr("v") << ";\n"; + s << " callback(_r);\n"; s << " }, timeout);\n"; s << "}\n\n"; } diff --git a/cpp-generator/legacy/generator_lib.cpp b/cpp-generator/legacy/generator_lib.cpp index e34ecc7..88853fa 100644 --- a/cpp-generator/legacy/generator_lib.cpp +++ b/cpp-generator/legacy/generator_lib.cpp @@ -589,6 +589,7 @@ QString makeHeader(const QString& moduleName, const QString& className, const QJ s << "#include \"logos_api.h\"\n"; s << "#include \"logos_api_client.h\"\n"; s << "#include \"logos_call_error.h\"\n"; + s << "#include \"logos_async_result.h\"\n"; s << "#include \"logos_object.h\"\n\n"; s << "class " << className << " {\n"; s << "public:\n"; @@ -662,25 +663,56 @@ QString makeHeader(const QString& moduleName, const QString& className, const QJ // Optional error out-channel: pass a logos::CallError* to distinguish // a failed remote call from a legitimately default-valued result. // Existing call sites compile unchanged. + // + // ...and an optional Timeout AFTER it, so the sync surface can say how + // long it is willing to wait. Appending (rather than inserting next to + // the value args, where the async overload carries it) keeps every + // existing call site source-compatible, including the ones that already + // pass `&err` positionally. The transport has taken both since it grew + // the error channel — logos_api_client.h's + // `invokeRemoteMethod(obj, method, args, Timeout, CallError*)` — and the + // generated body simply hard-coded `Timeout()` there. if (!params.isEmpty()) s << ", "; - s << "logos::CallError* err = nullptr);\n"; + s << "logos::CallError* err = nullptr, Timeout timeout = Timeout());\n"; + + // Param list shared by both async entry points. + auto emitAsyncParams = [&]() { + for (int i = 0; i < params.size(); ++i) { + QJsonObject p = params.at(i).toObject(); + QString qtPt = p.value("type").toString(); + QString pt = paramTypeFor(qtPt, apiStyle, rs); + QString pn = p.value("name").toString(); + bool byRef = byRefFor(qtPt, pt, apiStyle, rs); + if (byRef) s << "const " << pt << "& " << pn; + else s << pt << " " << pn; + if (i + 1 < params.size()) s << ", "; + } + if (params.size() > 0) s << ", "; + }; + // Async overload: same params + callback + optional Timeout QString asyncCallbackType = (ret == "void") ? QString("std::function") : QString("std::function"; s << " void " << name << "Async("; - for (int i = 0; i < params.size(); ++i) { - QJsonObject p = params.at(i).toObject(); - QString qtPt = p.value("type").toString(); - QString pt = paramTypeFor(qtPt, apiStyle, rs); - QString pn = p.value("name").toString(); - bool byRef = byRefFor(qtPt, pt, apiStyle, rs); - if (byRef) s << "const " << pt << "& " << pn; - else s << pt << " " << pn; - if (i + 1 < params.size()) s << ", "; - } - if (params.size() > 0) s << ", "; + emitAsyncParams(); s << asyncCallbackType << " callback, Timeout timeout = Timeout());\n"; + + // Result-carrying async entry point. The plain `Async` above + // hands the callback a bare value, so a failed call is + // INDISTINGUISHABLE from a provider that legitimately returned + // 0 / "" / false — the exact ambiguity the sync `CallError*` exists to + // resolve. This one delivers logos::AsyncResult {value, error}. + // + // A DISTINCT NAME, not an overload of `Async`: two overloads + // differing only in std::function vs + // std::function)> are ambiguous for a generic + // lambda (`[](auto v){...}` is invocable with either), which would + // break existing call sites. A distinct name has zero resolution risk. + s << " void " << name << "AsyncResult("; + emitAsyncParams(); + s << "std::function)> callback" + << ", Timeout timeout = Timeout());\n"; } s << "\nprivate:\n"; // ensureReplica() is needed whenever the wrapper subscribes to events, @@ -928,7 +960,7 @@ QString makeSource(const QString& moduleName, const QString& className, const QS if (i + 1 < params.size()) s << ", "; } if (!params.isEmpty()) s << ", "; - s << "logos::CallError* err) {\n"; + s << "logos::CallError* err, Timeout timeout) {\n"; // Body: perform call through the err-out overload. When the caller // passes a logos::CallError* it can distinguish a failed remote call @@ -953,7 +985,11 @@ QString makeSource(const QString& moduleName, const QString& className, const QS s << "QVariant::fromValue(" << wireArg(params.at(i).toObject()) << ")"; if (i + 1 < params.size()) s << ", "; } - s << "}, Timeout(), &_err);\n"; + // `timeout` — the caller's, defaulted to Timeout() at the declaration — + // not a hard-coded Timeout(). This is the overload that carries BOTH + // the deadline and the error out-channel; the generator used to call it + // with the error and drop the deadline on the floor. + s << "}, timeout, &_err);\n"; // A provider REJECTION arrives as the result, not as a transport error. // Fold it into the same error channel BEFORE the return table converts // it, or the conversion erases it (a rejected `[uint]` call answered [] @@ -1003,44 +1039,41 @@ QString makeSource(const QString& moduleName, const QString& className, const QS } s << "}\n\n"; - // Async implementation - s << "void " << className << "::" << name << "Async("; - for (int i = 0; i < params.size(); ++i) { - bool byRef; - emitParam(params.at(i).toObject(), byRef); - if (i + 1 < params.size()) s << ", "; - } - if (params.size() > 0) s << ", "; - s << "std::function callback, Timeout timeout) {\n"; - s << " if (!callback) return;\n"; - s << " m_client->invokeRemoteMethodAsync(" << targetExpr << ", \"" << name << "\", "; - if (params.size() == 0) { - s << "QVariantList()"; - } else { - // Same one-element-per-arg wrapping as the sync path (see above): a - // QVariantList-typed arg must not be spread across the args list. - s << "QVariantList{"; + // Shared pieces of the two async entry points, so `Async` and + // `AsyncResult` cannot drift apart in how they marshal args or + // decode the reply. + auto emitAsyncParams = [&]() { for (int i = 0; i < params.size(); ++i) { - s << "QVariant::fromValue(" << wireArg(params.at(i).toObject()) << ")"; + bool byRef; + emitParam(params.at(i).toObject(), byRef); if (i + 1 < params.size()) s << ", "; } - s << "}"; - } - s << ", [callback](QVariant v) {\n"; - // The async callback carries the value only — there is no CallError - // parameter to fill, and adding one would change the generated public - // surface. A rejection is at least made visible in the module log - // instead of vanishing into the return conversion below. - s << " { logos::CallError _rej; if (logosDispatchRejection(v, _rej))\n"; - s << " qWarning() << \"" << className << "::" << name - << "Async: remote call failed:\" << QString::fromStdString(_rej.message); }\n"; - if (ret == "void") { - s << " (void)v; callback();\n"; - } else if (retIsRecord) { - // A record decodes field by field; an invalid QVariant yields a - // default-constructed struct, matching the scalar paths. - s << " callback(" << fromWireFor(qtRet, apiStyle, rs, "v", className + "::") << ");\n"; - } else { + if (params.size() > 0) s << ", "; + }; + auto emitAsyncArgs = [&]() { + if (params.size() == 0) { + s << "QVariantList()"; + } else { + // Same one-element-per-arg wrapping as the sync path (see above): a + // QVariantList-typed arg must not be spread across the args list. + s << "QVariantList{"; + for (int i = 0; i < params.size(); ++i) { + s << "QVariant::fromValue(" << wireArg(params.at(i).toObject()) << ")"; + if (i + 1 < params.size()) s << ", "; + } + s << "}"; + } + }; + // The QVariant -> typed-return expression, given the QVariant's name. + // Empty for a void return. + auto asyncDecodeExpr = [&](const QString& var) -> QString { + if (ret == "void") return QString(); + if (retIsRecord) { + // A record decodes field by field; an invalid QVariant yields a + // default-constructed struct, matching the scalar paths. + return fromWireFor(qtRet, apiStyle, rs, var, className + "::"); + } + if (ret == "QVariant") return var; QString defaultVal; if (ret == "bool") defaultVal = "false"; else if (ret == "int" || ret == "qlonglong" || ret == "qulonglong" @@ -1051,12 +1084,61 @@ QString makeSource(const QString& moduleName, const QString& className, const QS else if (ret == "QVariantList") defaultVal = "QVariantList()"; else if (ret == "QVariantMap") defaultVal = "QVariantMap()"; else defaultVal = ret + "{}"; - if (ret == "QVariant") { - s << " callback(v);\n"; - } else { - s << " callback(v.isValid() ? qvariant_cast<" << ret << ">(v) : " << defaultVal << ");\n"; - } - } + return var + ".isValid() ? qvariant_cast<" + ret + ">(" + var + ") : " + defaultVal; + }; + + // Async implementation + s << "void " << className << "::" << name << "Async("; + emitAsyncParams(); + s << "std::function callback, Timeout timeout) {\n"; + s << " if (!callback) return;\n"; + s << " m_client->invokeRemoteMethodAsync(" << targetExpr << ", \"" << name << "\", "; + emitAsyncArgs(); + // A ONE-argument lambda: it is invocable only as + // LogosAPIClient::AsyncResultCallback, so this keeps binding to the + // historical value-only overload even though a CallError-aware one + // exists next to it. + s << ", [callback](QVariant v) {\n"; + // The value-only async callback has nowhere to put an error — there is + // no CallError parameter to fill, and adding one would change the + // historical public surface. A rejection is at least made visible in + // the module log instead of vanishing into the return conversion below. + // `AsyncResult` is the surface that can actually REPORT it. + s << " { logos::CallError _rej; if (logosDispatchRejection(v, _rej))\n"; + s << " qWarning() << \"" << className << "::" << name + << "Async: remote call failed:\" << QString::fromStdString(_rej.message); }\n"; + if (ret == "void") s << " (void)v; callback();\n"; + else s << " callback(" << asyncDecodeExpr("v") << ");\n"; + s << " }, timeout);\n"; + s << "}\n\n"; + + // Result-carrying async implementation. Routes to the transport's + // CallError-aware async overload (AsyncResultErrorCallback) — a TWO + // argument lambda, which is invocable only as that overload, so the + // pair above and below resolve unambiguously. + // + // On failure the value stays default-constructed exactly as + // `Async` would have delivered it; what changes is that the + // callback can now TELL, via r.error / r.ok(). + // + // That includes a provider REJECTION, which arrives as the RESULT and + // not as a transport error: it is folded into `_r.error` exactly as the + // sync path folds it into the caller's CallError. `Async` can only + // warn about one because its callback has no error slot; this one has, + // so a rejected call must NOT report ok() here. + s << "void " << className << "::" << name << "AsyncResult("; + emitAsyncParams(); + s << "std::function)> callback, Timeout timeout) {\n"; + s << " if (!callback) return;\n"; + s << " m_client->invokeRemoteMethodAsync(" << targetExpr << ", \"" << name << "\", "; + emitAsyncArgs(); + s << ", [callback](QVariant v, const logos::CallError& _err) {\n"; + s << " logos::AsyncResult<" << ret << "> _r;\n"; + s << " _r.error = _err;\n"; + s << " if (_r.error.ok()) logosDispatchRejection(v, _r.error);\n"; + if (ret == "void") s << " (void)v;\n"; + else s << " _r.value = " << asyncDecodeExpr("v") << ";\n"; + s << " callback(_r);\n"; s << " }, timeout);\n"; s << "}\n\n"; } @@ -1305,7 +1387,30 @@ QString makeHeaderLp(const QString& moduleName, const QString& className, const } if (!events.isEmpty()) s << "\n"; - // Methods: sync (with optional CallError out-param) + async overload. + // Methods: sync (with optional CallError out-param + timeout) + async + // overload. + // + // TIMEOUTS ARE SPELLED `int timeout_ms`, NOT `Timeout`, on this surface. + // `Timeout` lives in logos-protocol's logos_mode.h, which includes + // — naming it here would drag Qt into a translation unit whose whole reason + // for existing is not to have any. logos::LpClient already spells its + // deadlines `int timeout_ms` with the C ABI's rule (`<= 0` selects the + // protocol default), and this matches it. + // + // NO `AsyncResult` HERE — deliberately, for now. The Qt surface gets + // one because its transport reports the error + // (LogosAPIClient::AsyncResultErrorCallback). This surface's transport does + // NOT: logos-protocol's lp_invoke_async (cpp/logos_protocol.cpp) subscribes + // with the VALUE-ONLY invokeRemoteMethodAsync overload and unconditionally + // calls back `cb(1, json, ...)` — ok is hard-coded to 1 — even though + // lp_result_cb is documented as "ok == 0 → `json` is the canonical error + // object", and even though its own sync twin lp_invoke does return + // LP_ERR_UNAVAILABLE + makeErrorJson. So an AsyncResult emitted here would + // report ok() on a failed call to a module that is not loaded: an error + // channel that lies is worse than no error channel. (Measured, not assumed: + // a wrapper wired to it fires its callback with the default value and an + // EMPTY error code.) Once lp_invoke_async reports the error, emitting the + // AsyncResult twin here is the same few lines as above. for (const QJsonValue& v : methods) { const QJsonObject o = v.toObject(); if (!o.value("isInvokable").toBool()) continue; @@ -1313,31 +1418,29 @@ QString makeHeaderLp(const QString& moduleName, const QString& className, const const QString ret = returnTypeFor(o.value("returnType").toString(), ApiStyle::Lp, rs); const QJsonArray params = o.value("parameters").toArray(); + auto emitDeclParams = [&]() { + for (int i = 0; i < params.size(); ++i) { + const QJsonObject p = params.at(i).toObject(); + const QString qtPt = p.value("type").toString(); + const QString pt = paramTypeFor(qtPt, ApiStyle::Lp, rs); + if (byRefFor(qtPt, pt, ApiStyle::Lp, rs)) s << "const " << pt << "& " << p.value("name").toString(); + else s << pt << " " << p.value("name").toString(); + if (i + 1 < params.size()) s << ", "; + } + if (!params.isEmpty()) s << ", "; + }; + s << " " << ret << " " << name << "("; - for (int i = 0; i < params.size(); ++i) { - const QJsonObject p = params.at(i).toObject(); - const QString qtPt = p.value("type").toString(); - const QString pt = paramTypeFor(qtPt, ApiStyle::Lp, rs); - if (byRefFor(qtPt, pt, ApiStyle::Lp, rs)) s << "const " << pt << "& " << p.value("name").toString(); - else s << pt << " " << p.value("name").toString(); - if (i + 1 < params.size()) s << ", "; - } - if (!params.isEmpty()) s << ", "; - s << "logos::CallError* err = nullptr);\n"; + emitDeclParams(); + // Trailing, defaulted, and in that order — existing call sites, + // including ones already passing `&err` positionally, are unaffected. + s << "logos::CallError* err = nullptr, int timeout_ms = 0);\n"; const QString asyncCb = (ret == "void") ? QString("std::function") : QString("std::function"; s << " void " << name << "Async("; - for (int i = 0; i < params.size(); ++i) { - const QJsonObject p = params.at(i).toObject(); - const QString qtPt = p.value("type").toString(); - const QString pt = paramTypeFor(qtPt, ApiStyle::Lp, rs); - if (byRefFor(qtPt, pt, ApiStyle::Lp, rs)) s << "const " << pt << "& " << p.value("name").toString(); - else s << pt << " " << p.value("name").toString(); - if (i + 1 < params.size()) s << ", "; - } - if (!params.isEmpty()) s << ", "; + emitDeclParams(); s << asyncCb << " callback);\n"; } @@ -1430,16 +1533,18 @@ QString makeSourceLp(const QString& moduleName, const QString& className, const } }; - // Sync + // Sync — routes the caller's deadline to LpClient::invoke's + // `timeout_ms` parameter, which the generated body used to leave at its + // default (i.e. silently drop). s << retQual << " " << className << "::" << name << "("; emitParams(); if (!params.isEmpty()) s << ", "; - s << "logos::CallError* err) {\n"; + s << "logos::CallError* err, int timeout_ms) {\n"; emitArgsArray(); if (ret == "void") { - s << " " << clientExpr << ".invoke(\"" << name << "\", _args, err);\n"; + s << " " << clientExpr << ".invoke(\"" << name << "\", _args, err, timeout_ms);\n"; } else { - s << " nlohmann::json _r = " << clientExpr << ".invoke(\"" << name << "\", _args, err);\n"; + s << " nlohmann::json _r = " << clientExpr << ".invoke(\"" << name << "\", _args, err, timeout_ms);\n"; s << " return " << fromWireFor(qtRet, ApiStyle::Lp, rs, "_r", className + "::") << ";\n"; } s << "}\n\n"; @@ -1462,6 +1567,7 @@ QString makeSourceLp(const QString& moduleName, const QString& className, const } s << " });\n"; s << "}\n\n"; + // (No AsyncResult on this surface yet — see makeHeaderLp.) } return c; } diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index bb27ef2..ec7bd3e 100644 --- a/cpp/CMakeLists.txt +++ b/cpp/CMakeLists.txt @@ -65,5 +65,6 @@ install(FILES logos_json.h logos_result.h logos_lp_client.h + logos_async_result.h DESTINATION include ) diff --git a/cpp/logos_async_result.h b/cpp/logos_async_result.h new file mode 100644 index 0000000..546618c --- /dev/null +++ b/cpp/logos_async_result.h @@ -0,0 +1,78 @@ +#ifndef LOGOS_ASYNC_RESULT_H +#define LOGOS_ASYNC_RESULT_H + +// The async counterpart of logos::CallError's sync out-parameter. +// +// The sync generated wrapper takes an optional `logos::CallError*` precisely +// "to distinguish a failed remote call from a legitimately default-valued +// result". The async wrapper had no such channel: `fooAsync` hands the callback +// a bare T, and a failed call is indistinguishable from a provider that +// legitimately returned 0 / "" / false. AsyncResult is that missing channel +// — the value and the error travel together, so `fooAsyncResult`'s callback can +// check `.ok()` before trusting `.value`. +// +// This mirrors logos-rust-sdk, where BOTH surfaces already carry the error +// (lidl-gen/src/rustgen.rs: `-> Result` on the sync wrapper and +// `FnOnce(Result)` on the async one). C++ was the outlier. +// +// Deliberately Qt-FREE, exactly like logos_call_error.h next to which it +// conceptually lives: it is named in the signatures of BOTH generated surfaces +// — the Qt-typed one (ApiStyle::Qt) and the Qt-free lp one (ApiStyle::Lp, used +// by cdylib/universal modules whose translation units must not see Qt). Only +// std types and logos::CallError may appear here. +// +// (It lives in logos-cpp-sdk rather than in logos-protocol's +// logos_call_error.h only because that is where the generator that emits it +// lives; ${LOGOS_CPP_SDK_ROOT}/include is on the include path of every module +// build — see logos-plugin-qt/cmake/LogosModule.cmake and its module-builder +// twin, which add it unconditionally.) + +#include "logos_call_error.h" + +namespace logos { + +/** + * @brief An async call's outcome: the decoded value plus the call error. + * + * `error.ok()` (surfaced as `ok()`) is the ONLY reliable success test — a + * failed call leaves `value` default-constructed, which for most return types + * is also a perfectly legal success value. + * + * dep.echoIntAsyncResult(7, [](logos::AsyncResult r) { + * if (!r.ok()) { qWarning() << r.error.code.c_str(); return; } + * use(r.value); + * }); + * + * Aggregate — `AsyncResult{v, err}` and designated-ish brace init both work. + */ +template +struct AsyncResult { + T value{}; + CallError error; + + bool ok() const { return error.ok(); } + explicit operator bool() const { return error.ok(); } +}; + +/** + * @brief The void specialization: an error channel with no value. + * + * A `void`-returning method could equally have been given a plain + * `std::function` callback. It is spelled + * AsyncResult instead so that EVERY generated `fooAsyncResult` takes + * `std::function)>` for its own return type R with + * no special case — forwarding/proxy code (and the generator itself) can write + * the callback type from the return type mechanically, and every call site + * reads `if (!r.ok())` regardless of what the method returns. + */ +template <> +struct AsyncResult { + CallError error; + + bool ok() const { return error.ok(); } + explicit operator bool() const { return error.ok(); } +}; + +} // namespace logos + +#endif // LOGOS_ASYNC_RESULT_H diff --git a/doctests/cpp-sdk-generator-roundtrip.test.yaml b/doctests/cpp-sdk-generator-roundtrip.test.yaml index 8666bbe..7f461c8 100644 --- a/doctests/cpp-sdk-generator-roundtrip.test.yaml +++ b/doctests/cpp-sdk-generator-roundtrip.test.yaml @@ -269,8 +269,12 @@ sections: text: | The consumer side. From the same contract, `--module-only` emits the typed wrapper a *consumer* compiles against to call `sensor_module`. Each LIDL - `method` becomes a synchronous caller plus an `…Async` variant, and each - `event` an `on(...)` subscription. The type mapping is the Qt caller style: + `method` becomes three entry points — a synchronous caller, an `…Async` + variant that hands the callback a bare value, and an `…AsyncResult` + variant that hands it a `logos::AsyncResult` (`{value, error}`) so a + failed call is distinguishable from a legitimately default-valued one — + and each `event` an `on(...)` subscription. The type mapping is the Qt + caller style: `float64`→`double`, `tstr`→`QString`, `int`→`qlonglong`, `uint`→ `qulonglong`, `bstr`→`QByteArray`, `[tstr]`→`QStringList`, other arrays→`QVariantList`, and `result`→`LogosResult`. @@ -300,6 +304,24 @@ sections: - "QByteArray firmware(const QByteArray& image" - "QStringList labels(const QVariantList& ids" - "LogosResult reset(const QString& id" + - title: "The three call surfaces per method" + text: | + One method, three entry points. The sync form takes an optional + `logos::CallError*` **and** an optional `Timeout` — both trailing and + defaulted, so `temperature()` and `temperature(&err)` still compile. + `temperatureAsync` delivers the value alone; `temperatureAsyncResult` + delivers `logos::AsyncResult`, whose `.error` tells a failed + call apart from a provider that legitimately returned `0.0`. The + result-carrying entry point has its own name rather than being an + overload: a `std::function)>` overload alongside + `std::function` is ambiguous for a generic `[](auto v){…}`. + run: "grep -E 'temperature' consumer/sensor_module_api.h" + code_block: | + grep -E 'temperature' consumer/sensor_module_api.h + expect_contains: + - "double temperature(logos::CallError* err = nullptr, Timeout timeout = Timeout());" + - "void temperatureAsync(std::function callback, Timeout timeout = Timeout());" + - "void temperatureAsyncResult(std::function)> callback, Timeout timeout = Timeout());" - title: "The full type system: composite types" step: true diff --git a/nix/include.nix b/nix/include.nix index c0b6774..21cbe53 100644 --- a/nix/include.nix +++ b/nix/include.nix @@ -28,7 +28,7 @@ pkgs.stdenv.mkDerivation { # include/cpp/, a single TU would pull logos_result.h through two # distinct realpaths and #pragma once could not dedup them # (redefinition of StdLogosResult). Ship every std header in BOTH roots. - for file in logos_module_context.h logos_json.h logos_result.h logos_lp_client.h; do + for file in logos_module_context.h logos_json.h logos_result.h logos_lp_client.h logos_async_result.h; do cp cpp/$file $out/include/cpp/ cp cpp/$file $out/include/ done diff --git a/tests/experimental/test_lidl_gen_client.cpp b/tests/experimental/test_lidl_gen_client.cpp index 9dcf4be..aeaa675 100644 --- a/tests/experimental/test_lidl_gen_client.cpp +++ b/tests/experimental/test_lidl_gen_client.cpp @@ -483,3 +483,69 @@ TEST(LidlGenClient, BytesTagCollisionIsRefusedThroughAnOptional) EXPECT_TRUE(error.contains("Sneaky")) << error.toStdString(); } } + +// --------------------------------------------------------------------------- +// Sync timeout + result-carrying async +// +// This emitter and legacy/generator_lib.cpp produce the SAME consumer surface +// for the same contract — one is reached from a published `.lidl`, the other +// through the module builder — so the two must agree. tests/generator/ +// test_async_result.cpp holds the legacy twin of these assertions. +// --------------------------------------------------------------------------- + +TEST(LidlGenClient, SyncTakesBothErrorAndTimeout) +{ + auto m = makeTestModule(); + QString h = lidlMakeHeader(m); + // Trailing and defaulted, err first — `createAccount(p)` and + // `createAccount(p, &err)` are both unaffected. + EXPECT_TRUE(h.contains("QString createAccount(const QString& passphrase, " + "logos::CallError* err = nullptr, Timeout timeout = Timeout());")); + EXPECT_TRUE(h.contains("QStringList listAccounts(logos::CallError* err = nullptr, " + "Timeout timeout = Timeout());")); +} + +TEST(LidlGenClient, SyncBodyForwardsTheCallersTimeout) +{ + auto m = makeTestModule(); + QString s = lidlMakeSource(m); + EXPECT_TRUE(s.contains("WalletModule::createAccount(const QString& passphrase, " + "logos::CallError* err, Timeout timeout)")); + EXPECT_TRUE(s.contains("), timeout, &_err);")); + EXPECT_FALSE(s.contains("), Timeout(), &_err);")); +} + +TEST(LidlGenClient, HeaderDeclaresTheResultCarryingAsyncEntryPoint) +{ + auto m = makeTestModule(); + QString h = lidlMakeHeader(m); + EXPECT_TRUE(h.contains("#include \"logos_async_result.h\"")); + EXPECT_TRUE(h.contains("void createAccountAsyncResult(const QString& passphrase, " + "std::function)> callback, " + "Timeout timeout = Timeout());")); + EXPECT_TRUE(h.contains("void listAccountsAsyncResult(" + "std::function)> callback, " + "Timeout timeout = Timeout());")); +} + +TEST(LidlGenClient, ResultCarryingAsyncRoutesToTheCallErrorAwareOverload) +{ + auto m = makeTestModule(); + QString s = lidlMakeSource(m); + // Two-argument lambda: only AsyncResultErrorCallback is invocable with it. + EXPECT_TRUE(s.contains("[callback](QVariant v, const logos::CallError& _err) {")); + EXPECT_TRUE(s.contains("logos::AsyncResult _r;")); + EXPECT_TRUE(s.contains("_r.error = _err;")); + EXPECT_TRUE(s.contains("callback(_r);")); +} + +TEST(LidlGenClient, ThePlainAsyncEntryPointIsUnchanged) +{ + auto m = makeTestModule(); + QString h = lidlMakeHeader(m); + EXPECT_TRUE(h.contains("void createAccountAsync(const QString& passphrase, " + "std::function callback, Timeout timeout = Timeout());")); + QString s = lidlMakeSource(m); + // Still a ONE-argument lambda -> still the value-only transport overload. + EXPECT_TRUE(s.contains("[callback](QVariant v) {")); +} diff --git a/tests/generator/CMakeLists.txt b/tests/generator/CMakeLists.txt index 1f6a000..0b953c3 100644 --- a/tests/generator/CMakeLists.txt +++ b/tests/generator/CMakeLists.txt @@ -18,6 +18,7 @@ add_executable(generator_tests test_make_umbrella.cpp test_parse_provider_header.cpp test_records.cpp + test_async_result.cpp test_optional_spellings.cpp ) diff --git a/tests/generator/test_async_result.cpp b/tests/generator/test_async_result.cpp new file mode 100644 index 0000000..60557ea --- /dev/null +++ b/tests/generator/test_async_result.cpp @@ -0,0 +1,201 @@ +// The two complementary gaps this suite pins down: +// +// sync had an error channel (logos::CallError*) but NO timeout; +// async had a timeout but NO error channel. +// +// After the change the sync wrapper takes both, and a distinctly-named +// `AsyncResult` delivers logos::AsyncResult {value, error}. Every +// assertion here fails on the pre-change generator. +#include +#include +#include +#include "generator_lib.h" + +namespace { + +QJsonObject method(const QString& name, const QString& ret, const QStringList& paramTypes = {}) +{ + QJsonObject m; + m["name"] = name; + m["returnType"] = ret; + m["isInvokable"] = true; + QJsonArray params; + for (int i = 0; i < paramTypes.size(); ++i) { + QJsonObject p; + p["type"] = paramTypes.at(i); + p["name"] = QString("p%1").arg(i); + params.append(p); + } + m["parameters"] = params; + return m; +} + +QJsonArray sampleMethods() +{ + QJsonArray a; + a.append(method("add", "int", {"int", "int"})); + a.append(method("reset", "void")); + a.append(method("name", "QString")); + return a; +} + +QString qtHeader() { return makeHeader("mod", "Mod", sampleMethods(), ApiStyle::Qt); } +QString qtSource() { return makeSource("mod", "Mod", "mod.h", sampleMethods(), ApiStyle::Qt); } +QString lpHeader() { return makeHeader("mod", "Mod", sampleMethods(), ApiStyle::Lp); } +QString lpSource() { return makeSource("mod", "Mod", "mod.h", sampleMethods(), ApiStyle::Lp); } + +} // namespace + +// ─── 1. Sync gains a timeout, appended AFTER the error out-param ──────────── + +TEST(SyncTimeout, QtDeclarationTakesBothErrorAndTimeout) +{ + const QString h = qtHeader(); + // Order matters: err first, timeout second, both defaulted — so a call site + // that already passes `&err` positionally is unaffected. + EXPECT_TRUE(h.contains("int add(int p0, int p1, logos::CallError* err = nullptr, Timeout timeout = Timeout());")); + EXPECT_TRUE(h.contains("void reset(logos::CallError* err = nullptr, Timeout timeout = Timeout());")); + EXPECT_TRUE(h.contains("QString name(logos::CallError* err = nullptr, Timeout timeout = Timeout());")); +} + +TEST(SyncTimeout, QtBodyForwardsTheCallersTimeout) +{ + const QString src = qtSource(); + EXPECT_TRUE(src.contains("Mod::add(int p0, int p1, logos::CallError* err, Timeout timeout)")); + // Routes to the transport overload that takes BOTH — logos_api_client.h's + // invokeRemoteMethod(obj, method, args, Timeout, logos::CallError*). + EXPECT_TRUE(src.contains("}, timeout, &_err);")); + // ...and no longer hard-codes a fresh default. + EXPECT_FALSE(src.contains("}, Timeout(), &_err);")); +} + +TEST(SyncTimeout, LpDeclarationTakesBothErrorAndTimeoutMs) +{ + // `Timeout` lives in logos_mode.h, which includes ; the Qt-free + // surface spells its deadline the way LpClient/the C ABI do. + const QString h = lpHeader(); + EXPECT_TRUE(h.contains("int64_t add(int64_t p0, int64_t p1, logos::CallError* err = nullptr, int timeout_ms = 0);")); + EXPECT_TRUE(h.contains("void reset(logos::CallError* err = nullptr, int timeout_ms = 0);")); + EXPECT_FALSE(h.contains("Timeout timeout")); +} + +TEST(SyncTimeout, LpBodyForwardsTimeoutMs) +{ + const QString src = lpSource(); + EXPECT_TRUE(src.contains("Mod::add(int64_t p0, int64_t p1, logos::CallError* err, int timeout_ms)")); + EXPECT_TRUE(src.contains("m_client.invoke(\"add\", _args, err, timeout_ms);")); + EXPECT_TRUE(src.contains("m_client.invoke(\"reset\", _args, err, timeout_ms);")); + EXPECT_FALSE(src.contains("_args, err);")); +} + +// ─── 2. Async gains a result-carrying entry point ─────────────────────────── + +TEST(AsyncResult, QtHeaderDeclaresTheDistinctlyNamedEntryPoint) +{ + const QString h = qtHeader(); + EXPECT_TRUE(h.contains("void addAsyncResult(int p0, int p1, " + "std::function)> callback, " + "Timeout timeout = Timeout());")); + EXPECT_TRUE(h.contains("void nameAsyncResult(std::function)> callback, " + "Timeout timeout = Timeout());")); +} + +TEST(AsyncResult, VoidReturnUsesTheVoidSpecializationNotABespokeCallback) +{ + // Uniform shape: std::function)> for every R, including + // void, so generated forwarding code never special-cases the return type. + const QString h = qtHeader(); + EXPECT_TRUE(h.contains("void resetAsyncResult(std::function)> callback, " + "Timeout timeout = Timeout());")); +} + +TEST(AsyncResult, QtHeaderIncludesTheAsyncResultHeader) +{ + EXPECT_TRUE(qtHeader().contains("#include \"logos_async_result.h\"")); +} + +TEST(AsyncResult, QtBodyRoutesToTheCallErrorAwareTransportOverload) +{ + const QString src = qtSource(); + // A TWO-argument lambda: only LogosAPIClient::AsyncResultErrorCallback is + // invocable with it, so this cannot silently bind to the value-only one. + EXPECT_TRUE(src.contains("[callback](QVariant v, const logos::CallError& _err) {")); + EXPECT_TRUE(src.contains("logos::AsyncResult _r;")); + EXPECT_TRUE(src.contains("_r.error = _err;")); + EXPECT_TRUE(src.contains("_r.value = v.isValid() ? qvariant_cast(v) : 0;")); + EXPECT_TRUE(src.contains("callback(_r);")); + // The void form carries the error and nothing else. + EXPECT_TRUE(src.contains("logos::AsyncResult _r;")); +} + +TEST(AsyncResult, TheValueDecodeIsSharedWithThePlainAsyncEntryPoint) +{ + // Same decode expression in both, so a failed call delivers exactly the + // value `Async` would have delivered — plus the error. + const QString src = qtSource(); + EXPECT_TRUE(src.contains("callback(v.isValid() ? qvariant_cast(v) : 0);")); + EXPECT_TRUE(src.contains("_r.value = v.isValid() ? qvariant_cast(v) : 0;")); +} + +// ─── 3. The existing async surface is untouched ───────────────────────────── + +TEST(AsyncResult, ThePlainAsyncEntryPointIsUnchanged) +{ + const QString h = qtHeader(); + EXPECT_TRUE(h.contains("void addAsync(int p0, int p1, std::function callback, " + "Timeout timeout = Timeout());")); + EXPECT_TRUE(h.contains("void resetAsync(std::function callback, Timeout timeout = Timeout());")); + + const QString src = qtSource(); + EXPECT_TRUE(src.contains("void Mod::addAsync(int p0, int p1, std::function callback, Timeout timeout)")); + // Still a ONE-argument lambda -> still the value-only transport overload. + EXPECT_TRUE(src.contains("[callback](QVariant v) {")); +} + +// ─── 4. The Qt-free surface deliberately has no AsyncResult yet ───────────── + +TEST(AsyncResult, LpSurfaceDoesNotEmitAsyncResultWhileTheCAbiCannotReportOne) +{ + // logos-protocol's lp_invoke_async hard-codes `cb(1, ...)`, so an + // AsyncResult here would report ok() on a failed call. See makeHeaderLp. + EXPECT_FALSE(lpHeader().contains("AsyncResult")); + EXPECT_FALSE(lpSource().contains("AsyncResult")); + // ...and the Lp async entry point keeps its exact shape. + EXPECT_TRUE(lpHeader().contains("void addAsync(int64_t p0, int64_t p1, std::function callback);")); +} + +// ─── 5. A REJECTION reaches the surface that can report it ────────────────── +// +// A provider that refuses a call answers the canonical +// {"code":"dispatch_failed", …} object as its RESULT, not as a transport error. +// The sync path folds it into the caller's CallError. `Async` can only +// warn — its callback takes the value alone. `AsyncResult` is the first +// async surface with an error slot, so it must fold it too; reporting ok() for +// a rejected call there would reintroduce, on the new surface, exactly the +// defect the sync fold removed. + +TEST(AsyncResult, RejectionIsFoldedIntoTheAsyncResultError) +{ + const QString src = qtSource(); + EXPECT_TRUE(src.contains("if (_r.error.ok()) logosDispatchRejection(v, _r.error);")); + // Folded BEFORE the value is decoded and before the callback runs, so the + // callback never sees an ok() AsyncResult for a rejected call. + const int fold = src.indexOf("if (_r.error.ok()) logosDispatchRejection(v, _r.error);"); + const int decode = src.indexOf("_r.value = v.isValid() ? qvariant_cast(v) : 0;"); + const int deliver = src.indexOf("callback(_r);"); + EXPECT_NE(fold, -1); + EXPECT_NE(decode, -1); + EXPECT_NE(deliver, -1); + EXPECT_LT(fold, decode); + EXPECT_LT(decode, deliver); +} + +TEST(AsyncResult, ThePlainAsyncEntryPointStillOnlyWarns) +{ + // Unchanged public surface -> nowhere to put an error -> the log is all + // there is. The warning names `Async`, never `AsyncResult`. + const QString src = qtSource(); + EXPECT_TRUE(src.contains("{ logos::CallError _rej; if (logosDispatchRejection(v, _rej))")); + EXPECT_TRUE(src.contains("Mod::addAsync: remote call failed:")); + EXPECT_FALSE(src.contains("Mod::addAsyncResult: remote call failed:")); +} diff --git a/tests/generator/test_make_header.cpp b/tests/generator/test_make_header.cpp index f9d39a5..ad91884 100644 --- a/tests/generator/test_make_header.cpp +++ b/tests/generator/test_make_header.cpp @@ -94,8 +94,8 @@ TEST(MakeHeaderTest, ContainsMethodDeclarations) QJsonArray methods = makeTestMethods(); QString h = makeHeader("test_mod", "TestMod", methods); - EXPECT_TRUE(h.contains("int add(int a, int b, logos::CallError* err = nullptr)")); - EXPECT_TRUE(h.contains("void reset(logos::CallError* err = nullptr)")); + EXPECT_TRUE(h.contains("int add(int a, int b, logos::CallError* err = nullptr, Timeout timeout = Timeout())")); + EXPECT_TRUE(h.contains("void reset(logos::CallError* err = nullptr, Timeout timeout = Timeout())")); // Non-invokable should not appear EXPECT_FALSE(h.contains("internal")); } @@ -197,7 +197,7 @@ TEST(MakeHeaderTest, QVariantListAsyncOverload) } QString h = makeHeader("mod", "Mod", methods); - EXPECT_TRUE(h.contains("QVariantList getItems(logos::CallError* err = nullptr)")); + EXPECT_TRUE(h.contains("QVariantList getItems(logos::CallError* err = nullptr, Timeout timeout = Timeout())")); EXPECT_TRUE(h.contains("getItemsAsync(")); EXPECT_TRUE(h.contains("std::function callback")); } @@ -215,7 +215,7 @@ TEST(MakeHeaderTest, QVariantMapAsyncOverload) } QString h = makeHeader("mod", "Mod", methods); - EXPECT_TRUE(h.contains("QVariantMap getData(logos::CallError* err = nullptr)")); + EXPECT_TRUE(h.contains("QVariantMap getData(logos::CallError* err = nullptr, Timeout timeout = Timeout())")); EXPECT_TRUE(h.contains("getDataAsync(")); EXPECT_TRUE(h.contains("std::function callback")); } diff --git a/tests/generator/test_make_source.cpp b/tests/generator/test_make_source.cpp index 10f5e33..0ff8572 100644 --- a/tests/generator/test_make_source.cpp +++ b/tests/generator/test_make_source.cpp @@ -44,7 +44,7 @@ TEST(MakeSourceTest, ZeroParams) QJsonArray methods; methods.append(makeMethod("doStuff", "int", 0)); QString src = makeSource("mod", "Mod", "mod.h", methods); - EXPECT_TRUE(src.contains("m_client->invokeRemoteMethod(\"mod\", \"doStuff\", QVariantList{}, Timeout(), &_err)")); + EXPECT_TRUE(src.contains("m_client->invokeRemoteMethod(\"mod\", \"doStuff\", QVariantList{}, timeout, &_err)")); EXPECT_TRUE(src.contains("return _result.toInt()")); } @@ -53,7 +53,7 @@ TEST(MakeSourceTest, OneParam) QJsonArray methods; methods.append(makeMethod("fn", "bool", 1)); QString src = makeSource("mod", "Mod", "mod.h", methods); - EXPECT_TRUE(src.contains("m_client->invokeRemoteMethod(\"mod\", \"fn\", QVariantList{QVariant::fromValue(p0)}, Timeout(), &_err)")); + EXPECT_TRUE(src.contains("m_client->invokeRemoteMethod(\"mod\", \"fn\", QVariantList{QVariant::fromValue(p0)}, timeout, &_err)")); EXPECT_TRUE(src.contains("return _result.toBool()")); } @@ -62,7 +62,7 @@ TEST(MakeSourceTest, TwoParams) QJsonArray methods; methods.append(makeMethod("fn", "void", 2)); QString src = makeSource("mod", "Mod", "mod.h", methods); - EXPECT_TRUE(src.contains("m_client->invokeRemoteMethod(\"mod\", \"fn\", QVariantList{QVariant::fromValue(p0), QVariant::fromValue(p1)}, Timeout(), &_err)")); + EXPECT_TRUE(src.contains("m_client->invokeRemoteMethod(\"mod\", \"fn\", QVariantList{QVariant::fromValue(p0), QVariant::fromValue(p1)}, timeout, &_err)")); } TEST(MakeSourceTest, ThreeParams) @@ -121,7 +121,7 @@ TEST(MakeSourceTest, ListArgWrappedAsOneElement) methods.append(m); QString src = makeSource("mod", "Mod", "mod.h", methods); - EXPECT_TRUE(src.contains("invokeRemoteMethod(\"mod\", \"echoList\", QVariantList{QVariant::fromValue(v)}, Timeout(), &_err)")); + EXPECT_TRUE(src.contains("invokeRemoteMethod(\"mod\", \"echoList\", QVariantList{QVariant::fromValue(v)}, timeout, &_err)")); EXPECT_TRUE(src.contains("invokeRemoteMethodAsync(\"mod\", \"echoList\", QVariantList{QVariant::fromValue(v)}")); // The bare (spreading) form must not appear. EXPECT_FALSE(src.contains("QVariantList{v}")); diff --git a/tests/generator/test_records.cpp b/tests/generator/test_records.cpp index 155c9b7..8ffbf78 100644 --- a/tests/generator/test_records.cpp +++ b/tests/generator/test_records.cpp @@ -88,7 +88,7 @@ TEST(Records, QtWrapperExposesTheStruct) EXPECT_TRUE(h.contains(" QList items{};")); EXPECT_TRUE(h.contains(" QMap tags{};")); - EXPECT_TRUE(h.contains("Status getStatus(logos::CallError* err = nullptr);")); + EXPECT_TRUE(h.contains("Status getStatus(logos::CallError* err = nullptr, Timeout timeout = Timeout());")); EXPECT_TRUE(h.contains("QString describeStatus(const Status& s,")); EXPECT_TRUE(h.contains("QList listStatuses("));