Rework plugin commands to sol

Co-authored-by: Nerixyz <nerixdev@outlook.de>

src/controllers/plugins/LuaAPI.cpp

@@ -61,40 +61,6 @@ QDebug qdebugStreamForLogLevel(lua::api::LogLevel lvl)
 // luaL_error is a c-style vararg function, this makes clang-tidy not dislike it so much
 namespace chatterino::lua::api {
 
-int c2_register_command(lua_State *L)
-{
-    auto *pl = getApp()->getPlugins()->getPluginByStatePtr(L);
-    if (pl == nullptr)
-    {
-        luaL_error(L, "internal error: no plugin");
-        return 0;
-    }
-
-    QString name;
-    if (!lua::peek(L, &name, 1))
-    {
-        luaL_error(L, "cannot get command name (1st arg of register_command, "
-                      "expected a string)");
-        return 0;
-    }
-    if (lua_isnoneornil(L, 2))
-    {
-        luaL_error(L, "missing argument for register_command: function "
-                      "\"pointer\"");
-        return 0;
-    }
-
-    auto callbackSavedName = QString("c2commandcb-%1").arg(name);
-    lua_setfield(L, LUA_REGISTRYINDEX, callbackSavedName.toStdString().c_str());
-    auto ok = pl->registerCommand(name, callbackSavedName);
-
-    // delete both name and callback
-    lua_pop(L, 2);
-
-    lua::push(L, ok);
-    return 1;
-}
-
 int c2_register_callback(lua_State *L)
 {
     auto *pl = getApp()->getPlugins()->getPluginByStatePtr(L);

src/controllers/plugins/LuaAPI.hpp

@@ -3,8 +3,6 @@
 #ifdef CHATTERINO_HAVE_PLUGINS
 
 #    include <lua.h>
-#    include "controllers/plugins/LuaUtilities.hpp"
-
 #    include <QString>
 
 #    include <cassert>
@@ -93,7 +91,6 @@ struct CompletionEvent {
  * @lua@return boolean ok  Returns `true` if everything went ok, `false` if a command with this name exists.
  * @exposed c2.register_command
  */
-int c2_register_command(lua_State *L);
 
 /**
  * Registers a callback to be invoked when completions for a term are requested.

src/controllers/plugins/LuaUtilities.cpp

@@ -114,20 +114,6 @@ StackIdx push(lua_State *L, const std::string &str)
     return lua_gettop(L);
 }
 
-StackIdx push(lua_State *L, const CommandContext &ctx)
-{
-    StackGuard guard(L, 1);
-    auto outIdx = pushEmptyTable(L, 2);
-
-    push(L, ctx.words);
-    lua_setfield(L, outIdx, "words");
-
-    push(L, ctx.channel);
-    lua_setfield(L, outIdx, "channel");
-
-    return outIdx;
-}
-
 StackIdx push(lua_State *L, const bool &b)
 {
     lua_pushboolean(L, int(b));

src/controllers/plugins/LuaUtilities.hpp

@@ -60,7 +60,6 @@ StackIdx pushEmptyArray(lua_State *L, int countArray);
  */
 StackIdx pushEmptyTable(lua_State *L, int countProperties);
 
-StackIdx push(lua_State *L, const CommandContext &ctx);
 StackIdx push(lua_State *L, const QString &str);
 StackIdx push(lua_State *L, const std::string &str);
 StackIdx push(lua_State *L, const bool &b);

src/controllers/plugins/Plugin.cpp

@@ -13,6 +13,7 @@
 #    include <QJsonObject>
 #    include <QLoggingCategory>
 #    include <QUrl>
+#    include <sol/sol.hpp>
 
 #    include <algorithm>
 #    include <unordered_map>
@@ -188,7 +189,8 @@ PluginMeta::PluginMeta(const QJsonObject &obj)
     }
 }
 
-bool Plugin::registerCommand(const QString &name, const QString &functionName)
+bool Plugin::registerCommand(const QString &name,
+                             sol::protected_function function)
 {
     if (this->ownedCommands.find(name) != this->ownedCommands.end())
     {
@@ -200,7 +202,7 @@ bool Plugin::registerCommand(const QString &name, const QString &functionName)
     {
         return false;
     }
-    this->ownedCommands.insert({name, functionName});
+    this->ownedCommands.emplace(name, std::move(function));
     return true;
 }
 
@@ -227,6 +229,8 @@ Plugin::~Plugin()
     this->activeTimeouts.clear();
     if (this->state_ != nullptr)
     {
+        // clearing this after the state is gone is not safe to do
+        this->ownedCommands.clear();
         lua_close(this->state_);
     }
 }

src/controllers/plugins/Plugin.hpp

@@ -2,6 +2,7 @@
 
 #ifdef CHATTERINO_HAVE_PLUGINS
 #    include "Application.hpp"
+#    include "common/Common.hpp"
 #    include "common/network/NetworkCommon.hpp"
 #    include "controllers/plugins/LuaAPI.hpp"
 #    include "controllers/plugins/LuaUtilities.hpp"
@@ -11,6 +12,7 @@
 #    include <QString>
 #    include <QUrl>
 #    include <semver/semver.hpp>
+#    include <sol/forward.hpp>
 
 #    include <unordered_map>
 #    include <unordered_set>
@@ -75,13 +77,18 @@ public:
 
     ~Plugin();
 
+    Plugin(const Plugin &) = delete;
+    Plugin(Plugin &&) = delete;
+    Plugin &operator=(const Plugin &) = delete;
+    Plugin &operator=(Plugin &&) = delete;
+
     /**
      * @brief Perform all necessary tasks to bind a command name to this plugin
      * @param name name of the command to create
-     * @param functionName name of the function that should be called when the command is executed
+     * @param function the function that should be called when the command is executed
      * @return true if addition succeeded, false otherwise (for example because the command name is already taken)
      */
-    bool registerCommand(const QString &name, const QString &functionName);
+    bool registerCommand(const QString &name, sol::protected_function function);
 
     /**
      * @brief Get names of all commands belonging to this plugin
@@ -149,8 +156,8 @@ private:
 
     QString error_;
 
-    // maps command name -> function name
-    std::unordered_map<QString, QString> ownedCommands;
+    // maps command name -> function
+    std::unordered_map<QString, sol::protected_function> ownedCommands;
     std::vector<QTimer *> activeTimeouts;
     int lastTimerId = 0;
 

src/controllers/plugins/PluginController.cpp

@@ -13,6 +13,7 @@
 #    include "controllers/plugins/api/IOWrapper.hpp"
 #    include "controllers/plugins/LuaAPI.hpp"
 #    include "controllers/plugins/LuaUtilities.hpp"
+#    include "controllers/plugins/SolTypes.hpp"
 #    include "messages/MessageBuilder.hpp"
 #    include "singletons/Paths.hpp"
 #    include "singletons/Settings.hpp"
@@ -21,6 +22,7 @@
 #    include <lua.h>
 #    include <lualib.h>
 #    include <QJsonDocument>
+#    include <sol/sol.hpp>
 
 #    include <memory>
 #    include <utility>
@@ -111,9 +113,9 @@ bool PluginController::tryLoadFromDir(const QDir &pluginDir)
     return true;
 }
 
-void PluginController::openLibrariesFor(lua_State *L, const PluginMeta &meta,
-                                        const QDir &pluginDir)
+void PluginController::openLibrariesFor(Plugin *plugin, const QDir &pluginDir)
 {
+    auto *L = plugin->state_;
     lua::StackGuard guard(L);
     // Stuff to change, remove or hide behind a permission system:
     static const std::vector<luaL_Reg> loadedlibs = {
@@ -147,7 +149,6 @@ void PluginController::openLibrariesFor(lua_State *L, const PluginMeta &meta,
 
     // NOLINTNEXTLINE(*-avoid-c-arrays)
     static const luaL_Reg c2Lib[] = {
-        {"register_command", lua::api::c2_register_command},
         {"register_callback", lua::api::c2_register_callback},
         {"log", lua::api::c2_log},
         {"later", lua::api::c2_later},
@@ -294,6 +295,20 @@ void PluginController::openLibrariesFor(lua_State *L, const PluginMeta &meta,
 
     lua_pushnil(L);
     lua_setfield(L, LUA_REGISTRYINDEX, "_IO_output");
+
+    sol::state_view lua(L);
+    PluginController::initSol(lua, plugin);
+}
+
+// TODO: investigate if `plugin` can ever point to an invalid plugin,
+// especially in cases when the plugin is errored.
+void PluginController::initSol(sol::state_view &lua, Plugin *plugin)
+{
+    sol::table c2 = lua.globals()["c2"];
+    c2.set_function("register_command",
+                    [plugin](const QString &name, sol::protected_function cb) {
+                        return plugin->registerCommand(name, std::move(cb));
+                    });
 }
 
 void PluginController::load(const QFileInfo &index, const QDir &pluginDir,
@@ -312,7 +327,7 @@ void PluginController::load(const QFileInfo &index, const QDir &pluginDir,
                                  << " because safe mode is enabled.";
         return;
     }
-    PluginController::openLibrariesFor(l, meta, pluginDir);
+    PluginController::openLibrariesFor(temp, pluginDir);
 
     if (!PluginController::isPluginEnabled(pluginName) ||
         !getSettings()->pluginsEnabled)
@@ -343,16 +358,18 @@ bool PluginController::reload(const QString &id)
     {
         return false;
     }
-    if (it->second->state_ != nullptr)
-    {
-        lua_close(it->second->state_);
-        it->second->state_ = nullptr;
-    }
+
     for (const auto &[cmd, _] : it->second->ownedCommands)
     {
         getApp()->getCommands()->unregisterPluginCommand(cmd);
     }
     it->second->ownedCommands.clear();
+
+    if (it->second->state_ != nullptr)
+    {
+        lua_close(it->second->state_);
+        it->second->state_ = nullptr;
+    }
     QDir loadDir = it->second->loadDirectory_;
     this->plugins_.erase(id);
     this->tryLoadFromDir(loadDir);
@@ -367,27 +384,35 @@ QString PluginController::tryExecPluginCommand(const QString &commandName,
         if (auto it = plugin->ownedCommands.find(commandName);
             it != plugin->ownedCommands.end())
         {
-            const auto &funcName = it->second;
+            sol::state_view lua(plugin->state_);
+            sol::table args = lua.create_table_with(
+                "words", ctx.words,                           //
+                "channel", lua::api::ChannelRef(ctx.channel)  //
+            );
 
-            auto *L = plugin->state_;
-            lua_getfield(L, LUA_REGISTRYINDEX, funcName.toStdString().c_str());
-            lua::push(L, ctx);
-
-            auto res = lua_pcall(L, 1, 0, 0);
-            if (res != LUA_OK)
+            auto result =
+                lua::tryCall<std::optional<QString>>(it->second, args);
+            if (!result)
             {
-                ctx.channel->addSystemMessage("Lua error: " +
-                                              lua::humanErrorText(L, res));
-                return "";
+                ctx.channel->addSystemMessage(
+                    QStringView(
+                        u"Failed to evaluate command from plugin %1: %2")
+                        .arg(plugin->meta.name, result.error()));
+                return {};
             }
-            return "";
+
+            if (!*result)
+            {
+                return {};
+            }
+            return **result;
         }
     }
     qCCritical(chatterinoLua)
         << "Something's seriously up, no plugin owns command" << commandName
         << "yet a call to execute it came in";
     assert(false && "missing plugin command owner");
-    return "";
+    return {};
 }
 
 bool PluginController::isPluginEnabled(const QString &id)

src/controllers/plugins/PluginController.hpp

@@ -10,6 +10,7 @@
 #    include <QJsonArray>
 #    include <QJsonObject>
 #    include <QString>
+#    include <sol/forward.hpp>
 
 #    include <algorithm>
 #    include <map>
@@ -66,8 +67,10 @@ private:
               const PluginMeta &meta);
 
     // This function adds lua standard libraries into the state
-    static void openLibrariesFor(lua_State *L, const PluginMeta & /*meta*/,
-                                 const QDir &pluginDir);
+    static void openLibrariesFor(Plugin *plugin, const QDir &pluginDir);
+
+    static void initSol(sol::state_view &lua, Plugin *plugin);
+
     static void loadChatterinoLib(lua_State *l);
     bool tryLoadFromDir(const QDir &pluginDir);
     std::map<QString, std::unique_ptr<Plugin>> plugins_;