fix: prevent ambiguous call by calling signal handler with std::nullopt (#541)

Co-authored-by: Stanislav Angelovič <stanislav.angelovic@protonmail.com>

docs/using-sdbus-c++.md

@@ -611,10 +611,10 @@ We recommend that sdbus-c++ users prefer the convenience API to the lower level,
 
 > **_Note_:** By default, signal callback handlers are not invoked (i.e., the signal is silently dropped) if there is a signal signature mismatch. If you want to be informed of such situations, you can add `std::optional<sdbus::Error>` parameter to the beginning of your signal callback handler's parameter list. When sdbus-c++ invokes the handler, it will set this argument either to be empty (in normal cases), or to carry a corresponding `sdbus::Error` object (in case of deserialization failures, like type mismatches). An example of a handler with the signature (`int`) different from the real signal contents (`string`):
 > ```c++
->     void onConcatenated(std::optional<sdbus::Error> e, int wrongParameter)
+>     void onConcatenated(std::optional<sdbus::Error> err, int wrongParameter)
 >     {
->         assert(e.has_value());
->         assert(e->getMessage() == "Failed to deserialize a int32 value");
+>         assert(err.has_value());
+>         assert(err->getMessage() == "Failed to deserialize a int32 value");
 >     }
 > ```
 > Signature mismatch in signal handlers is probably the most common reason why signals are not received in the client, while we can see them on the bus with `dbus-monitor`. Use `std::optional<sdbus::Error>`-based callback variant and inspect the error to check if that's the cause of your problems.
@@ -1310,13 +1310,17 @@ sdbus-c++-xml2cpp can generate C++ code for client-side async methods. We just n
 </node>
 ```
 
-An asynchronous method can be generated as a callback-based method, `std::future`-based method, or C++20 awaitable-based method. This can optionally be customized through an additional `org.freedesktop.DBus.Method.Async.ClientImpl` annotation. Its supported values are `callback`, `future` and `awaitable`. The default behavior is callback-based method.
+An asynchronous method can be generated as a callback-based method, `std::future`-based method, or C++20 awaitable-based method. This can optionally be customized through an additional `org.freedesktop.DBus.Method.Async.ClientImpl` annotation. Its supported values are `callback`, `direct-callback`, `future` and `awaitable`. The default behavior is callback-based method.
 
 #### Generating callback-based async methods
 
 For each client-side async method, a corresponding `on<MethodName>Reply` pure virtual function, where `<MethodName>` is the capitalized D-Bus method name, is generated in the generated proxy class. This function is the callback invoked when the D-Bus method reply arrives, and must be provided a body by overriding it in the implementation class.
 
-So in the specific example above, the tool will generate a `Concatenator_proxy` class similar to one shown in a [dedicated section above](#concatenator-client-glueh), with the difference that it will also generate an additional `virtual void onConcatenateReply(std::optional<sdbus::Error> error, const std::string& concatenatedString);` method, which we shall override in the derived `ConcatenatorProxy`.
+So in the specific example above, the tool will generate a `Concatenator_proxy` class similar to one shown in a [dedicated section above](#concatenator-client-glueh), with the difference that it will also generate an additional `virtual void onConcatenateReply(const std::string& concatenatedString, std::optional<sdbus::Error> error);` method, which we shall override in the derived `ConcatenatorProxy`.
+
+#### Generating direct callback-based async methods
+
+An additional callback parameter is added to the function signature. The callback is called when the D-Bus method reply arrives. The callback can be any generic callable that takes the method output arguments (`const std::string&` in this example), followed by the parameter of type `std::optional<sdbus::Error> error`.
 
 #### Generating std::future-based async methods
 

include/sdbus-c++/ConvenienceApiClasses.inl

@@ -317,11 +317,11 @@ namespace sdbus {
                 {
                     reply >> args;
                 }
-                catch (const Error& e)
+                catch (const Error& err)
                 {
                     // Pass message deserialization exceptions to the client via callback error parameter,
                     // instead of propagating them up the message loop call stack.
-                    sdbus::apply(callback, e, args);
+                    sdbus::apply(callback, err, args);
                     return;
                 }
             }
@@ -453,16 +453,16 @@ namespace sdbus {
                 {
                     signal >> signalArgs;
                 }
-                catch (const Error& e)
+                catch (const Error& err)
                 {
                     // Pass message deserialization exceptions to the client via callback error parameter,
                     // instead of propagating them up the message loop call stack.
-                    sdbus::apply(callback, e, signalArgs);
+                    sdbus::apply(callback, err, signalArgs);
                     return;
                 }
 
                 // Invoke callback with no error and input arguments from the tuple.
-                sdbus::apply(callback, {}, signalArgs);
+                sdbus::apply(callback, std::nullopt, signalArgs);
             }
             else
             {

tests/integrationtests/DBusSignalsTests.cpp

@@ -114,6 +114,14 @@ TYPED_TEST(SdbusTestObject, EmitsSignalWithoutRegistrationSuccessfully)
     ASSERT_THAT(this->m_proxy->m_signatureFromSignal["platform"], Eq(sdbus::Signature{"av"}));
 }
 
+TYPED_TEST(SdbusTestObject, EmitsSignalWithErrorAndTypeMismatchSuccessfully)
+{
+    this->m_adaptor->emitSignalWithErrorAndTypeMismatch();
+
+    ASSERT_TRUE(waitUntil(this->m_proxy->m_gotSignalWithTypeMismatch));
+    ASSERT_TRUE(this->m_proxy->m_errorFromSignal.has_value());
+}
+
 TYPED_TEST(SdbusTestObject, CanAccessAssociatedSignalMessageInSignalHandler)
 {
     this->m_adaptor->emitSimpleSignal();

tests/integrationtests/TestAdaptor.cpp

@@ -311,6 +311,11 @@ void TestAdaptor::emitSignalWithoutRegistration(const sdbus::Struct<std::string,
     getObject().emitSignal("signalWithoutRegistration").onInterface(sdbus::test::INTERFACE_NAME).withArguments(strct);
 }
 
+void TestAdaptor::emitSignalWithErrorAndTypeMismatch()
+{
+    getObject().emitSignal("signalWithErrorAndTypeMismatch").onInterface(sdbus::test::INTERFACE_NAME);
+}
+
 std::string TestAdaptor::getExpectedXmlApiDescription() 
 {
     return

tests/integrationtests/TestAdaptor.h

@@ -106,6 +106,7 @@ protected:
 
 public:
     void emitSignalWithoutRegistration(const sdbus::Struct<std::string, sdbus::Struct<sdbus::Signature>>& strct);
+    void emitSignalWithErrorAndTypeMismatch();
     static std::string getExpectedXmlApiDescription() ;
 
 private:

tests/integrationtests/TestProxy.cpp

@@ -45,6 +45,7 @@ TestProxy::TestProxy(ServiceName destination, ObjectPath objectPath)
     : ProxyInterfaces(std::move(destination), std::move(objectPath))
 {
     getProxy().uponSignal("signalWithoutRegistration").onInterface(sdbus::test::INTERFACE_NAME).call([this](const sdbus::Struct<std::string, sdbus::Struct<sdbus::Signature>>& strct){ this->onSignalWithoutRegistration(strct); });
+    getProxy().uponSignal("signalWithErrorAndTypeMismatch").onInterface(sdbus::test::INTERFACE_NAME).call([this](std::optional<sdbus::Error> err, int wrongParameter){ this->onSignalWithErrorAndTypeMismatch(std::move(err), wrongParameter); });
 
     registerProxy();
 }
@@ -60,6 +61,7 @@ TestProxy::TestProxy(sdbus::IConnection& connection, ServiceName destination, Ob
     : ProxyInterfaces(connection, std::move(destination), std::move(objectPath))
 {
     getProxy().uponSignal("signalWithoutRegistration").onInterface(sdbus::test::INTERFACE_NAME).call([this](const sdbus::Struct<std::string, sdbus::Struct<sdbus::Signature>>& strct){ this->onSignalWithoutRegistration(strct); });
+    getProxy().uponSignal("signalWithErrorAndTypeMismatch").onInterface(sdbus::test::INTERFACE_NAME).call([this](std::optional<sdbus::Error> err, int wrongParameter){ this->onSignalWithErrorAndTypeMismatch(std::move(err), wrongParameter); });
 
     registerProxy();
 }
@@ -96,6 +98,12 @@ void TestProxy::onSignalWithoutRegistration(const sdbus::Struct<std::string, sdb
     m_gotSignalWithSignature = true;
 }
 
+void TestProxy::onSignalWithErrorAndTypeMismatch(std::optional<sdbus::Error> err, [[maybe_unused]] int wrongParameter)
+{
+    m_errorFromSignal = std::move(err);
+    m_gotSignalWithTypeMismatch = true;
+}
+
 void TestProxy::onDoOperationReply(uint32_t returnValue, std::optional<sdbus::Error> error) const
 {
     if (m_DoOperationClientSideAsyncReplyHandler)

tests/integrationtests/TestProxy.h

@@ -94,6 +94,7 @@ protected:
     void onSignalWithVariant(const sdbus::Variant& aVariant) override;
 
     void onSignalWithoutRegistration(const sdbus::Struct<std::string, sdbus::Struct<sdbus::Signature>>& strct);
+    void onSignalWithErrorAndTypeMismatch(std::optional<sdbus::Error> err, int wrongParameter);
     void onDoOperationReply(uint32_t returnValue, std::optional<sdbus::Error> error) const;
 
     // Signals of standard D-Bus interfaces
@@ -130,6 +131,8 @@ public:
     double m_variantFromSignal{};
     std::atomic<bool> m_gotSignalWithSignature{false};
     std::map<std::string, Signature> m_signatureFromSignal;
+    std::atomic<bool> m_gotSignalWithTypeMismatch{false};
+    std::optional<sdbus::Error> m_errorFromSignal;
 
     std::function<void(uint32_t res, std::optional<sdbus::Error> err)> m_DoOperationClientSideAsyncReplyHandler;
     std::function<void(const sdbus::InterfaceName&, const std::map<PropertyName, sdbus::Variant>&, const std::vector<PropertyName>&)> m_onPropertiesChangedHandler;

tools/xml2cpp-codegen/ProxyGenerator.cpp

@@ -43,7 +43,7 @@ using sdbuscpp::xml::Node;
 using sdbuscpp::xml::Nodes;
 
 // Possible implementation backends of async methods
-enum class AsyncImpl { Callback, Future, Awaitable };
+enum class AsyncImpl { Callback, Future, Awaitable, DirectCallback };
 
 /**
  * Generate proxy code - client glue
@@ -178,9 +178,16 @@ std::tuple<std::string, std::string> ProxyGenerator::processMethods(const Nodes&
             {
                 if (annotationName == "org.freedesktop.DBus.Method.Async"
                      && (annotationValue == "client" || annotationValue == "clientserver" || annotationValue == "client-server"))
-                    asyncImpl = AsyncImpl::Callback; // Default to callback
+                {
+                    if (not asyncImpl.has_value())
+                    {
+                        asyncImpl = AsyncImpl::Callback; // Default to callback
+                    }
+                }
                 else if (annotationName == "org.freedesktop.DBus.Method.Async.ClientImpl" && annotationValue == "callback")
                     asyncImpl = AsyncImpl::Callback;
+                else if (annotationName == "org.freedesktop.DBus.Method.Async.ClientImpl" && annotationValue == "direct-callback")
+                    asyncImpl = AsyncImpl::DirectCallback;
                 else if (annotationName == "org.freedesktop.DBus.Method.Async.ClientImpl" && (annotationValue == "future" || annotationValue == "std::future"))
                     asyncImpl = AsyncImpl::Future;
                 else if (annotationName == "org.freedesktop.DBus.Method.Async.ClientImpl" && (annotationValue == "awaitable" || annotationValue == "coroutine"))
@@ -236,8 +243,17 @@ std::tuple<std::string, std::string> ProxyGenerator::processMethods(const Nodes&
             realRetType = retType;
         }
 
-        definitionSS << tab << realRetType << " " << nameSafe << "(" << inArgTypeStr << ")" << endl
+        if (asyncImpl.has_value() && *asyncImpl == AsyncImpl::DirectCallback)
+        {
+            definitionSS << tab << "template <typename F>" << endl
+                << tab << realRetType << " " << nameSafe << "(" << inArgTypeStr << (not inArgTypeStr.empty() ? ", " : "") << "F&& callback" << ")" << endl
                 << tab << "{" << endl;
+        }
+        else
+        {
+            definitionSS << tab << realRetType << " " << nameSafe << "(" << inArgTypeStr << ")" << endl
+                    << tab << "{" << endl;
+        }
 
         if (!timeoutValue.empty())
         {
@@ -277,6 +293,10 @@ std::tuple<std::string, std::string> ProxyGenerator::processMethods(const Nodes&
             {
                 definitionSS << ".getResultAsAwaitable<" << retTypeBare << ">()";
             }
+            else if (*asyncImpl == AsyncImpl::DirectCallback)
+            {
+                definitionSS << ".uponReplyInvoke(std::forward<F>(callback))";
+            }
             else // Callback
             {
                 definitionSS << ".uponReplyInvoke([this](std::optional<sdbus::Error> error" << (outArgTypeStr.empty() ? "" : ", ") << outArgTypeStr << ")"
@@ -323,7 +343,7 @@ std::tuple<std::string, std::string> ProxyGenerator::processSignals(const Nodes&
                 ".call([this](" << argTypeStr << ")"
                 "{ this->on" << nameBigFirst << "(" << argStr << "); });" << endl;
 
-        declarationSS << tab << "virtual void on" << nameBigFirst << "(" << argTypeStr << ") = 0;" << endl;
+        declarationSS << tab << "virtual void on" << nameBigFirst << "(" << argTypeStr << ") {}" << endl;
     }
 
     return std::make_tuple(registrationSS.str(), declarationSS.str());
@@ -353,17 +373,31 @@ std::tuple<std::string, std::string> ProxyGenerator::processProperties(const Nod
             const auto annotationValue = annotation->get("value");
 
             if (annotationName == "org.freedesktop.DBus.Property.Get.Async" && annotationValue == "client") // Server-side not supported (may be in the future)
-                asyncImplGet = AsyncImpl::Callback; // Default to callback
+            {
+                if (not asyncImplGet.has_value())
+                {
+                    asyncImplGet = AsyncImpl::Callback; // Default to callback
+                }
+            }
             else if (annotationName == "org.freedesktop.DBus.Property.Get.Async.ClientImpl" && annotationValue == "callback")
                 asyncImplGet = AsyncImpl::Callback;
+            else if (annotationName == "org.freedesktop.DBus.Property.Get.Async.ClientImpl" && annotationValue == "direct-callback")
+                asyncImplGet = AsyncImpl::DirectCallback;
             else if (annotationName == "org.freedesktop.DBus.Property.Get.Async.ClientImpl" && (annotationValue == "future" || annotationValue == "std::future"))
                 asyncImplGet = AsyncImpl::Future;
             else if (annotationName == "org.freedesktop.DBus.Property.Get.Async.ClientImpl" && (annotationValue == "awaitable" || annotationValue == "coroutine"))
                 asyncImplGet = AsyncImpl::Awaitable;
             else if (annotationName == "org.freedesktop.DBus.Property.Set.Async" && annotationValue == "client") // Server-side not supported (may be in the future)
-                asyncImplSet = AsyncImpl::Callback; // Default to callback
+            {
+                if (not asyncImplSet.has_value())
+                {
+                    asyncImplSet = AsyncImpl::Callback; // Default to callback
+                }
+            }
             else if (annotationName == "org.freedesktop.DBus.Property.Set.Async.ClientImpl" && annotationValue == "callback")
                 asyncImplSet = AsyncImpl::Callback;
+            else if (annotationName == "org.freedesktop.DBus.Property.Set.Async.ClientImpl" && annotationValue == "direct-callback")
+                asyncImplSet = AsyncImpl::DirectCallback;
             else if (annotationName == "org.freedesktop.DBus.Property.Set.Async.ClientImpl" && (annotationValue == "future" || annotationValue == "std::future"))
                 asyncImplSet = AsyncImpl::Future;
             else if (annotationName == "org.freedesktop.DBus.Property.Set.Async.ClientImpl" && (annotationValue == "awaitable" || annotationValue == "coroutine"))
@@ -388,10 +422,20 @@ std::tuple<std::string, std::string> ProxyGenerator::processProperties(const Nod
                 realRetType = propertyType;
             }
 
-            propertySS << tab << realRetType << " " << propertyNameSafe << "()" << endl
-                    << tab << "{" << endl;
+            if (asyncImplGet.has_value() && asyncImplGet.value() == AsyncImpl::DirectCallback)
+            {
+                propertySS << tab << "template <typename F>" << endl
+                    << tab << realRetType << " " << propertyNameSafe << "(F&& callback)" << endl;
+            }
+            else
+            {
+                propertySS << tab << realRetType << " " << propertyNameSafe << "()" << endl;
+            }
+
+            propertySS << tab << "{" << endl;
             propertySS << tab << tab << "return m_proxy.getProperty" << (asyncImplGet.has_value() ? "Async" : "") << "(\"" << propertyName << "\")"
                             ".onInterface(INTERFACE_NAME)";
+
             if (!asyncImplGet.has_value())
             {
                 propertySS << ".get<" << realRetType << ">()";
@@ -409,6 +453,10 @@ std::tuple<std::string, std::string> ProxyGenerator::processProperties(const Nod
                 {
                     propertySS << ".getResultAsAwaitable()";
                 }
+                else if (*asyncImplGet == AsyncImpl::DirectCallback)
+                {
+                    propertySS << ".uponReplyInvoke(std::forward<F>(callback))";
+                }
                 else // Callback
                 {
                     propertySS << ".uponReplyInvoke([this](std::optional<sdbus::Error> error, const sdbus::Variant& value)"
@@ -442,8 +490,17 @@ std::tuple<std::string, std::string> ProxyGenerator::processProperties(const Nod
                 realRetType = "void";
             }
 
-            propertySS << tab << realRetType << " " << propertyNameSafe << "(" << propertyTypeArg << ")" << endl
-                       << tab << "{" << endl;
+            if (asyncImplSet.has_value() && asyncImplSet.value() == AsyncImpl::DirectCallback)
+            {
+                propertySS << tab << "template <typename F>" << endl
+                    << tab << realRetType << " " << propertyNameSafe << "(" << propertyTypeArg << (not propertyTypeArg.empty() ? ", " : "") << "F&& callback)" << endl;
+            }
+            else
+            {
+                propertySS << tab << realRetType << " " << propertyNameSafe << "(" << propertyTypeArg << ")" << endl;
+            }
+
+            propertySS << tab << "{" << endl;
             propertySS << tab << tab << (asyncImplSet.has_value() ? "return " : "") << "m_proxy.setProperty" << (asyncImplSet.has_value() ? "Async" : "")
                        << "(\"" << propertyName << "\")"
                             ".onInterface(INTERFACE_NAME)"
@@ -462,6 +519,10 @@ std::tuple<std::string, std::string> ProxyGenerator::processProperties(const Nod
                 {
                     propertySS << ".getResultAsAwaitable()";
                 }
+                else if (*asyncImplSet == AsyncImpl::DirectCallback)
+                {
+                    propertySS << ".uponReplyInvoke(std::forward<T>(callback))";
+                }
                 else // Callback
                 {
                     propertySS << ".uponReplyInvoke([this](std::optional<sdbus::Error> error)"