Fix the double forwarding bug in function fusion

src/PowerPlant.hpp

@@ -311,24 +311,6 @@ class PowerPlant {
         FusionFunction::call(*this, data, std::forward<Arguments>(args)...);
     }
 
-    template <template <typename> class First, template <typename> class... Remainder, typename... Arguments>
-    void emit_shared(Arguments&&... args) {
-
-        using Functions      = std::tuple<First<void>, Remainder<void>...>;
-        using ArgumentPack   = decltype(std::forward_as_tuple(*this, std::forward<Arguments>(args)...));
-        using CallerArgs     = std::tuple<>;
-        using FusionFunction = util::FunctionFusion<Functions, ArgumentPack, EmitCaller, CallerArgs, 1>;
-
-        // Provide a check to make sure they are passing us the right stuff
-        static_assert(
-            FusionFunction::value,
-            "There was an error with the arguments for the emit function, Check that your scope and arguments "
-            "match what you are trying to do.");
-
-        // Fuse our emit handlers and call the fused function
-        FusionFunction::call(*this, std::forward<Arguments>(args)...);
-    }
-
     /// Our TaskScheduler that handles distributing task to the pool threads
     threading::scheduler::Scheduler scheduler;
     /// Our vector of Reactors, will get destructed when this vector is

src/Reactor.hpp

@@ -135,9 +135,9 @@ namespace dsl {
             template <typename WatchdogGroup, typename RuntimeType>
             struct WatchdogServicer;
             template <typename WatchdogGroup, typename RuntimeType>
-            WatchdogServicer<WatchdogGroup, RuntimeType> ServiceWatchdog(RuntimeType&& data);
+            std::unique_ptr<WatchdogServicer<WatchdogGroup, RuntimeType>> ServiceWatchdog(RuntimeType&& data);
             template <typename WatchdogGroup>
-            WatchdogServicer<WatchdogGroup, void> ServiceWatchdog();
+            std::unique_ptr<WatchdogServicer<WatchdogGroup, void>> ServiceWatchdog();
         }  // namespace emit
     }  // namespace word
 }  // namespace dsl
@@ -272,10 +272,7 @@ class Reactor {
 
     /// @copydoc dsl::word::emit::ServiceWatchdog
     template <typename WatchdogGroup, typename... Arguments>
-    auto ServiceWatchdog(Arguments&&... args)
-        // THIS IS VERY IMPORTANT, the return type must be dependent on the function call
-        // otherwise it won't check it's valid in SFINAE
-        -> decltype(dsl::word::emit::ServiceWatchdog<WatchdogGroup>(std::forward<Arguments>(args)...)) {
+    auto ServiceWatchdog(Arguments&&... args) {
         return dsl::word::emit::ServiceWatchdog<WatchdogGroup>(std::forward<Arguments>(args)...);
     }
 
@@ -368,7 +365,7 @@ class Reactor {
                 util::CallbackGenerator<DSL, Function>(std::forward<Function>(callback)));
 
             // Get our tuple from binding our reaction
-            auto tuple = DSL::bind(reaction, std::get<Index>(args)...);
+            auto tuple = DSL::bind(reaction, std::move(std::get<Index>(args))...);
 
             auto handle = threading::ReactionHandle(reaction);
             reactor.reaction_handles.push_back(handle);
@@ -379,7 +376,8 @@ class Reactor {
         }
 
     public:
-        Binder(Reactor& r, Arguments&&... args) : reactor(r), args(args...) {}
+        template <typename... Args>
+        Binder(Reactor& r, Args&&... args) : reactor(r), args(std::forward<Args>(args)...) {}
 
         template <typename Label, typename Function>
         auto then(Label&& label, Function&& callback) {

src/dsl/word/emit/Watchdog.hpp

@@ -125,8 +125,8 @@ namespace dsl {
              * @return A WatchdogServicer object which will update the service time of the specified watchdog
              */
             template <typename WatchdogGroup, typename RuntimeType>
-            WatchdogServicer<WatchdogGroup, RuntimeType> ServiceWatchdog(RuntimeType&& data) {
-                return WatchdogServicer<WatchdogGroup, RuntimeType>(std::forward<RuntimeType>(data));
+            std::unique_ptr<WatchdogServicer<WatchdogGroup, RuntimeType>> ServiceWatchdog(RuntimeType&& data) {
+                return std::make_unique<WatchdogServicer<WatchdogGroup, RuntimeType>>(std::forward<RuntimeType>(data));
             }
 
             /**
@@ -137,8 +137,8 @@ namespace dsl {
              * @return WatchdogServicer<WatchdogGroup, void>
              */
             template <typename WatchdogGroup>
-            WatchdogServicer<WatchdogGroup, void> ServiceWatchdog() {
-                return WatchdogServicer<WatchdogGroup, void>();
+            std::unique_ptr<WatchdogServicer<WatchdogGroup, void>> ServiceWatchdog() {
+                return std::make_unique<WatchdogServicer<WatchdogGroup, void>>();
             }
 
             /**
@@ -158,9 +158,9 @@ namespace dsl {
 
                 template <typename WatchdogGroup, typename RuntimeType>
                 static void emit(const PowerPlant& /*powerplant*/,
-                                 WatchdogServicer<WatchdogGroup, RuntimeType>& servicer) {
+                                 const std::shared_ptr<WatchdogServicer<WatchdogGroup, RuntimeType>>& servicer) {
                     // Update our service time
-                    servicer.service();
+                    servicer->service();
                 }
             };
 

src/util/FunctionFusion.hpp

@@ -20,8 +20,8 @@
  * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
  */
 
-#ifndef NUCLEAR_UTIL_FUNCTIONFUSION_HPP
-#define NUCLEAR_UTIL_FUNCTIONFUSION_HPP
+#ifndef NUCLEAR_UTIL_FUNCTION_FUSION_HPP
+#define NUCLEAR_UTIL_FUNCTION_FUSION_HPP
 
 #include "Sequence.hpp"
 #include "tuplify.hpp"
@@ -51,8 +51,16 @@ namespace util {
     auto apply_function_fusion_call(const std::tuple<Arguments...>& args,
                                     const Sequence<Shared...>& /*shared*/,
                                     const Sequence<Selected...>& /*selected*/)
-        -> decltype(Function::call(std::get<Shared>(args)..., std::get<Selected>(args)...)) {
-        return Function::call(std::get<Shared>(args)..., std::get<Selected>(args)...);
+        -> decltype(Function::call(
+            std::get<Shared>(args)...,
+            static_cast<std::tuple_element_t<Selected, std::tuple<Arguments...>>>(std::get<Selected>(args))...)) {
+
+        return Function::call(
+            // By not moving/forwarding the shared arguments they will always end up as lvalues even if they were
+            // rvalues originally. That allows them to be used in multiple function calls without the first one being
+            // able to steal them via a move constructor etc
+            std::get<Shared>(args)...,
+            static_cast<std::tuple_element_t<Selected, std::tuple<Arguments...>>>(std::get<Selected>(args))...);
     }
 
     /**
@@ -138,13 +146,14 @@ namespace util {
          *
          * @return the result of calling this specific function
          */
-        template <typename Function, int Start, int End>
+        template <typename Function, int Start, int End, typename... Args>
         // It is forwarded as a tuple
-        // NOLINTNEXTLINE(cppcoreguidelines-rvalue-reference-param-not-moved)
-        static auto call_one(const Sequence<Start, End>& /*e*/, Arguments&&... args)
-            -> decltype(apply_function_fusion_call<Function, Shared, Start, End>(std::forward_as_tuple(args...))) {
+        static auto call_one(const Sequence<Start, End>& /*e*/, Args&&... args)
+            -> decltype(apply_function_fusion_call<Function, Shared, Start, End>(
+                std::forward_as_tuple(std::forward<Args>(args)...))) {
 
-            return apply_function_fusion_call<Function, Shared, Start, End>(std::forward_as_tuple(args...));
+            return apply_function_fusion_call<Function, Shared, Start, End>(
+                std::forward_as_tuple(std::forward<Args>(args)...));
         }
 
         /**
@@ -177,12 +186,14 @@ namespace util {
                                        NextStep::call(std::forward<Args>(args)...))) {
 
             // Call each on a separate line to preserve order of execution
-            // TODO(thouliston) this is a legitimate bug, fix it in a future PR and add a test to ensure it doesn't
-            // regress
-            // NOLINTNEXTLINE(bugprone-use-after-move)
-            auto current = tuplify(call_one<CurrentFunction>(CurrentRange(), std::forward<Args>(args)...));
+            // The std::forwards here are fine even though they are passed to multiple functions.
+            // As part of function fusion before, all of the shared arguments are enforced to be lvalues and only the
+            // selected arguments will be potentially rvalues. These rvalues are then only passed to a single target
+            // function, even if they are forwarded multiple times to the functions which select the right arguments.
+            // As shown in the moving test, casting to an rvalue, but then not using it won't change the result.
             // NOLINTNEXTLINE(bugprone-use-after-move)
-            auto remainder = NextStep::call(std::forward<Args>(args)...);
+            auto current   = tuplify(call_one<CurrentFunction>(CurrentRange(), std::forward<Args>(args)...));
+            auto remainder = NextStep::call(std::forward<Args>(args)...);  // NOLINT(bugprone-use-after-move)
 
             return std::tuple_cat(std::move(current), std::move(remainder));
         }
@@ -365,4 +376,4 @@ namespace util {
 }  // namespace util
 }  // namespace NUClear
 
-#endif  // NUCLEAR_UTIL_FUNCTIONFUSION_HPP
+#endif  // NUCLEAR_UTIL_FUNCTION_FUSION_HPP

src/util/Sequence.hpp

@@ -23,6 +23,8 @@
 #ifndef NUCLEAR_UTIL_SEQUENCE_HPP
 #define NUCLEAR_UTIL_SEQUENCE_HPP
 
+#include <type_traits>
+
 namespace NUClear {
 namespace util {
 

tests/tests/util/FunctionFusion.cpp

@@ -0,0 +1,172 @@
+/*
+ * MIT License
+ *
+ * Copyright (c) 2022 NUClear Contributors
+ *
+ * This file is part of the NUClear codebase.
+ * See https://github.com/Fastcode/NUClear for further info.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated
+ * documentation files (the "Software"), to deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
+ * WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include "util/FunctionFusion.hpp"
+
+#include <catch2/catch_test_macros.hpp>
+#include <tuple>
+#include <utility>
+#include <vector>
+
+
+// There are several linting rules from clang-tidy that will trigger in this code.
+// However as the point of these tests is to show what the behaviour is in these situations they are suppressed for this
+// file
+// NOLINTBEGIN(bugprone-use-after-move)
+
+namespace {  // Make everything here internal linkage
+
+/**
+ * This struct is used to test what is passed from FunctionFusion to the call function.
+ *
+ * It has four different versions of append to see which one is called
+ * The ones that take rvalue references will move the arguments into new temporaries to ensure the original arguments
+ * will be empty after the function runs.
+ */
+struct Appender {
+
+    /// rvalue/rvalue
+    static std::vector<char> append(std::vector<char>&& x, std::vector<char>&& y) {
+        const std::vector<char> x1 = std::move(x);
+        const std::vector<char> y1 = std::move(y);
+
+        std::vector<char> out;
+        out.push_back('r');
+        out.push_back('r');
+        out.insert(out.end(), x1.begin(), x1.end());  // HERE
+        out.insert(out.end(), y1.begin(), y1.end());
+
+        return out;
+    }
+
+    /// rvalue/lvalue
+    static std::vector<char> append(std::vector<char>&& x, const std::vector<char>& y) {
+        const std::vector<char> x1 = std::move(x);
+        const auto& y1             = y;
+
+        std::vector<char> out;
+        out.push_back('r');
+        out.push_back('l');
+        out.insert(out.end(), x1.begin(), x1.end());
+        out.insert(out.end(), y1.begin(), y1.end());
+
+        return out;
+    }
+
+    /// lvalue/rvalue
+    static std::vector<char> append(const std::vector<char>& x, std::vector<char>&& y) {
+        const auto& x1             = x;
+        const std::vector<char> y1 = std::move(y);
+
+        std::vector<char> out;
+        out.push_back('l');
+        out.push_back('r');
+        out.insert(out.end(), x1.begin(), x1.end());  // HERE
+        out.insert(out.end(), y1.begin(), y1.end());
+
+        return out;
+    }
+
+    /// lvalue/lvalue
+    static std::vector<char> append(const std::vector<char>& x, const std::vector<char>& y) {
+        const auto& x1 = x;
+        const auto& y1 = y;
+
+        std::vector<char> out;
+        out.push_back('l');
+        out.push_back('l');
+        out.insert(out.end(), x.begin(), x.end());
+        out.insert(out.end(), y.begin(), y.end());
+
+        return out;
+    }
+};
+
+template <typename T>
+struct AppendCaller {
+    template <typename... Args>
+    static auto call(Args&&... args) -> decltype(T::append(std::forward<Args>(args)...)) {
+        return T::append(std::forward<Args>(args)...);
+    }
+};
+
+template <int Shared, typename... Args>
+auto do_fusion(Args&&... args) {
+    return NUClear::util::FunctionFusion<std::tuple<Appender, Appender>,
+                                         decltype(std::forward_as_tuple(std::forward<Args>(args)...)),
+                                         AppendCaller,
+                                         std::tuple<>,
+                                         Shared>::call(std::forward<Args>(args)...);
+}
+
+}  // namespace
+
+SCENARIO("Shared arguments to FunctionFusion are not moved", "[util][FunctionFusion][shared][move]") {
+
+    WHEN("calling append with 1 shared and 1 selected arguments") {
+        std::vector<char> shared({'s'});
+        std::vector<char> arg1({'1'});
+        std::vector<char> arg2({'2'});
+
+        auto result = do_fusion<1>(std::move(shared), std::move(arg1), std::move(arg2));
+
+        const auto& r1 = std::get<0>(result);
+        const auto& r2 = std::get<1>(result);
+
+        THEN("the results are correct") {
+            CHECK(r1 == std::vector<char>({'l', 'r', 's', '1'}));
+            CHECK(r2 == std::vector<char>({'l', 'r', 's', '2'}));
+        }
+        THEN("the shared argument is not moved") {
+            CHECK(shared == std::vector<char>({'s'}));
+        }
+        THEN("the selected arguments are moved") {
+            CHECK(arg1.empty());
+            CHECK(arg2.empty());
+        }
+    }
+
+    WHEN("calling append with 0 shared and 2 selected arguments") {
+        std::vector<char> arg1({'1'});
+        std::vector<char> arg2({'2'});
+        std::vector<char> arg3({'3'});
+        std::vector<char> arg4({'4'});
+
+        auto result = do_fusion<0>(std::move(arg1), std::move(arg2), std::move(arg3), std::move(arg4));
+
+        const auto& r1 = std::get<0>(result);
+        const auto& r2 = std::get<1>(result);
+
+        THEN("the results are correct") {
+            CHECK(r1 == std::vector<char>({'r', 'r', '1', '2'}));
+            CHECK(r2 == std::vector<char>({'r', 'r', '3', '4'}));
+        }
+        THEN("the selected arguments are moved") {
+            CHECK(arg1.empty());
+            CHECK(arg2.empty());
+            CHECK(arg3.empty());
+            CHECK(arg4.empty());
+        }
+    }
+}
+
+// NOLINTEND(bugprone-use-after-move)