fix: take handler for sol_lua_check as function reference

CHANGELOG.md

@@ -103,6 +103,7 @@
 - Dev: Update vcpkg baseline. (#6359)
 - Dev: Added an explicit `frozen` flag to `Message`. (#6367)
 - Dev: Stop sending `JOIN`/`PART` commands for channels starting with `/`. (#6376)
+- Dev: Error handlers for Sol check functions now take a function reference over an owning type. (#6393)
 
 ## 2.5.3
 

src/controllers/plugins/SolTypes.cpp

@@ -40,7 +40,7 @@ void logError(Plugin *plugin, QStringView context, const QString &msg)
 // NOLINTBEGIN(readability-named-parameter)
 // QString
 bool sol_lua_check(sol::types<QString>, lua_State *L, int index,
-                   std::function<sol::check_handler_type> handler,
+                   chatterino::FunctionRef<sol::check_handler_type> handler,
                    sol::stack::record &tracking)
 {
     return sol::stack::check<const char *>(L, index, handler, tracking);
@@ -60,7 +60,7 @@ int sol_lua_push(sol::types<QString>, lua_State *L, const QString &value)
 
 // QStringList
 bool sol_lua_check(sol::types<QStringList>, lua_State *L, int index,
-                   std::function<sol::check_handler_type> handler,
+                   chatterino::FunctionRef<sol::check_handler_type> handler,
                    sol::stack::record &tracking)
 {
     return sol::stack::check<sol::table>(L, index, handler, tracking);
@@ -92,7 +92,7 @@ int sol_lua_push(sol::types<QStringList>, lua_State *L,
 
 // QByteArray
 bool sol_lua_check(sol::types<QByteArray>, lua_State *L, int index,
-                   std::function<sol::check_handler_type> handler,
+                   chatterino::FunctionRef<sol::check_handler_type> handler,
                    sol::stack::record &tracking)
 {
     return sol::stack::check<const char *>(L, index, handler, tracking);
@@ -115,10 +115,11 @@ namespace chatterino::lua {
 
 // ThisPluginState
 
-bool sol_lua_check(sol::types<chatterino::lua::ThisPluginState>,
-                   lua_State * /*L*/, int /* index*/,
-                   std::function<sol::check_handler_type> /* handler*/,
-                   sol::stack::record & /*tracking*/)
+bool sol_lua_check(
+    sol::types<chatterino::lua::ThisPluginState>, lua_State * /*L*/,
+    int /* index*/,
+    chatterino::FunctionRef<sol::check_handler_type> /* handler*/,
+    sol::stack::record & /*tracking*/)
 {
     return true;
 }

src/controllers/plugins/SolTypes.hpp

@@ -1,5 +1,6 @@
 #pragma once
 #ifdef CHATTERINO_HAVE_PLUGINS
+#    include "util/FunctionRef.hpp"
 #    include "util/QMagicEnum.hpp"
 #    include "util/TypeName.hpp"
 
@@ -174,12 +175,13 @@ void loggedVoidCall(const auto &fn, QStringView context, Plugin *plugin,
 }
 
 // NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
-#    define SOL_STACK_FUNCTIONS(TYPE)                                      \
-        bool sol_lua_check(sol::types<TYPE>, lua_State *L, int index,      \
-                           std::function<sol::check_handler_type> handler, \
-                           sol::stack::record &tracking);                  \
-        TYPE sol_lua_get(sol::types<TYPE>, lua_State *L, int index,        \
-                         sol::stack::record &tracking);                    \
+#    define SOL_STACK_FUNCTIONS(TYPE)                                 \
+        bool sol_lua_check(                                           \
+            sol::types<TYPE>, lua_State *L, int index,                \
+            chatterino::FunctionRef<sol::check_handler_type> handler, \
+            sol::stack::record &tracking);                            \
+        TYPE sol_lua_get(sol::types<TYPE>, lua_State *L, int index,   \
+                         sol::stack::record &tracking);               \
         int sol_lua_push(sol::types<TYPE>, lua_State *L, const TYPE &value);
 
 SOL_STACK_FUNCTIONS(chatterino::lua::ThisPluginState)

src/util/FunctionRef.hpp

@@ -0,0 +1,82 @@
+#pragma once
+
+#include <concepts>
+#include <cstdint>
+#include <type_traits>
+#include <utility>
+
+namespace chatterino {
+
+/// A non-owning, type-erased reference to a callable. Callables can be lambdas,
+/// functions, or std::functions.
+///
+/// It is intended to be used for functions taking a callback that is called
+/// immediately (such as filter/map functions). Since this doesn't own the
+/// callable, it's not safe to store.
+///
+/// This is based on llvm::function_ref (updated for C++ 20).
+template <typename Fn>
+class FunctionRef;
+
+template <typename Ret, typename... Params>
+class FunctionRef<Ret(Params...)>
+{
+public:
+    FunctionRef() = default;
+    FunctionRef(std::nullptr_t)
+    {
+    }
+
+    template <typename Callable>
+    FunctionRef(Callable &&callable)  // NOLINT
+        requires
+        // This is not the copy-constructor.
+        (!std::same_as<std::remove_cvref_t<Callable>, FunctionRef>) &&
+            // Functor must be callable and return a suitable type.
+            (std::is_void_v<Ret> ||
+             std::convertible_to<
+                 decltype(std::declval<Callable>()(std::declval<Params>()...)),
+                 Ret>)
+        : callback(callTrampoline<std::remove_reference_t<Callable>>)
+        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
+        , callable(reinterpret_cast<uintptr_t>(&callable))
+    {
+    }
+
+    Ret operator()(Params... params) const
+    {
+        return this->callback(this->callable, std::forward<Params>(params)...);
+    }
+
+    explicit operator bool() const
+    {
+        return this->callback != nullptr;
+    }
+
+    bool operator==(const FunctionRef &other) const
+    {
+        return this->callback == other.callback &&
+               this->callable == other.callable;
+    }
+
+    bool operator!=(const FunctionRef &other) const = default;
+
+private:
+    // same signature as callTrampoline
+    using Callback = Ret(uintptr_t, Params...);
+
+    /// Pointer to the call trampoline that, given the callable, calls the target function.
+    Callback *callback = nullptr;
+    /// Pointer to the actual function
+    uintptr_t callable = 0;
+
+    template <typename Callable>
+    static Ret callTrampoline(uintptr_t callable, Params... params)
+    {
+        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
+        return (*reinterpret_cast<Callable *>(callable))(
+            std::forward<Params>(params)...);
+    }
+};
+
+}  // namespace chatterino

tests/CMakeLists.txt

@@ -58,6 +58,7 @@ set(test_SOURCES
     ${CMAKE_CURRENT_LIST_DIR}/src/ImageUploader.cpp
     ${CMAKE_CURRENT_LIST_DIR}/src/TwitchChannel.cpp
     ${CMAKE_CURRENT_LIST_DIR}/src/TwitchUserColor.cpp
+    ${CMAKE_CURRENT_LIST_DIR}/src/FunctionRef.cpp
 
     ${CMAKE_CURRENT_LIST_DIR}/src/lib/Snapshot.cpp
     ${CMAKE_CURRENT_LIST_DIR}/src/lib/Snapshot.hpp

tests/src/FunctionRef.cpp

@@ -0,0 +1,171 @@
+#include "util/FunctionRef.hpp"
+
+#include "Test.hpp"
+
+#include <memory>
+
+using namespace chatterino;
+
+namespace {
+
+size_t myFunc(const std::string &s)
+{
+    return s.size();
+}
+
+std::string myFunc2(int i)
+{
+    return std::to_string(i);
+}
+
+std::string refCaller(FunctionRef<std::string(int)> fn, int i)
+{
+    return fn(i);
+}
+
+}  // namespace
+
+TEST(FunctionRef, lambda)
+{
+    static int i = 0;
+    auto empty = []() {
+        i++;
+    };
+
+    FunctionRef<void()>{empty}();
+    ASSERT_EQ(i, 1);
+
+    auto unique = std::make_unique<int>(0);
+    int *up = unique.get();
+    auto noncopyable = [p{std::move(unique)}]() {
+        *p += 1;
+    };
+    FunctionRef<void()>{noncopyable}();
+    ASSERT_EQ(*up, 1);
+    {
+        FunctionRef<void()> f{std::move(noncopyable)};
+        f();
+        ASSERT_EQ(*up, 2);
+    }
+
+    auto identity = [](auto &&it) {
+        return std::forward<decltype(it)>(it);
+    };
+
+    ASSERT_EQ(FunctionRef<int(int)>{identity}(1), 1);
+    ASSERT_EQ(
+        FunctionRef<std::string(std::string)>{identity}(std::string{"foo"}),
+        "foo");
+
+    ASSERT_EQ(refCaller(
+                  [](int i) {
+                      return std::to_string(i);
+                  },
+                  42),
+              "42");
+
+    ASSERT_EQ(refCaller(
+                  [p{std::make_unique<int>(1)}](int i) {
+                      return std::to_string(i + *p);
+                  },
+                  42),
+              "43");
+}
+
+TEST(FunctionRef, pointer)
+{
+    ASSERT_EQ(FunctionRef<int(std::string)>{myFunc}("foo"), 3);
+    ASSERT_EQ(FunctionRef<std::string(int)>{myFunc2}(2), "2");
+    ASSERT_EQ(refCaller(myFunc2, 42), "42");
+}
+
+TEST(FunctionRef, stdFunction)
+{
+    int i = 0;
+    FunctionRef<void()>{std::function{[&] {
+        i++;
+    }}}();
+    ASSERT_EQ(i, 1);
+    ASSERT_EQ(FunctionRef<size_t(std::string)>{std::function{myFunc}}("foo"),
+              3);
+}
+
+TEST(FunctionRef, operatorEq)
+{
+    int i = 0;
+    FunctionRef<void()> f1([] {});
+    FunctionRef<void()> f2([&] {
+        i++;
+    });
+    FunctionRef<void()> f3([=]() mutable {
+        i++;
+    });
+    ASSERT_EQ(f1, f1);
+    ASSERT_NE(f1, f2);
+    ASSERT_NE(f1, f3);
+    ASSERT_EQ(f2, f2);
+    ASSERT_NE(f2, f1);
+    ASSERT_NE(f2, f3);
+    ASSERT_EQ(f3, f3);
+    ASSERT_NE(f3, f1);
+    ASSERT_NE(f3, f2);
+
+    FunctionRef<size_t(std::string)> f4(myFunc);
+    FunctionRef<size_t(std::string)> f5(myFunc);
+    FunctionRef<size_t(std::string)> f6([](std::string /*s*/) {  // NOLINT
+        return 0;
+    });
+
+    ASSERT_EQ(f4, f4);
+    ASSERT_EQ(f4, f5);
+    ASSERT_EQ(f5, f4);
+    ASSERT_NE(f4, f6);
+    ASSERT_NE(f6, f4);
+    ASSERT_EQ(f6, f6);
+}
+
+TEST(FunctionRef, operatorBool)
+{
+    FunctionRef<void()> f0;
+    FunctionRef<void()> f1([] {});
+    FunctionRef<size_t(std::string)> f2(myFunc);
+
+    ASSERT_FALSE(f0);
+    ASSERT_TRUE(f1);
+    ASSERT_TRUE(f2);
+}
+
+TEST(FunctionRef, copyCtor)
+{
+    FunctionRef<void()> f0;
+    auto f0Copy = f0;
+    ASSERT_EQ(f0, f0Copy);
+    ASSERT_FALSE(f0);
+    ASSERT_FALSE(f0Copy);
+
+    int i = 0;
+    auto cb = [&] {
+        i++;
+    };
+    FunctionRef<void()> f1(cb);
+    auto f1Copy = f1;
+    ASSERT_EQ(f1, f1Copy);
+    f1();
+    ASSERT_EQ(i, 1);
+    f1Copy();
+    ASSERT_EQ(i, 2);
+
+    FunctionRef<size_t(std::string)> f2(myFunc);
+    auto f2Copy = f2;
+    ASSERT_EQ(f2, f2Copy);
+    ASSERT_EQ(f2("foobar"), f2Copy("foobar"));
+
+    static_assert(std::is_trivially_copyable_v<decltype(f0)>);
+    static_assert(std::is_trivially_copyable_v<decltype(f2)>);
+
+    static_assert(std::is_trivially_move_assignable_v<decltype(f0)>);
+    static_assert(std::is_trivially_move_assignable_v<decltype(f2)>);
+
+    static_assert(std::is_trivially_move_constructible_v<decltype(f0)>);
+    static_assert(std::is_trivially_move_constructible_v<decltype(f2)>);
+}