Merge pull request #905 from secondlife/require-tweaks

 Refactor `require()` to make it easier to reason about Lua stack usage.

9 files changed, 252 insertions, 209 deletions

diff --git a/indra/llcommon/lua_function.cpp b/indra/llcommon/lua_function.cpp
index a5f1f582d9..78abb8ba7e 100644
--- a/indra/llcommon/lua_function.cpp
+++ b/indra/llcommon/lua_function.cpp
@@ -789,7 +789,8 @@ std::ostream& operator<<(std::ostream& out, const lua_what& self)
 *****************************************************************************/
 std::ostream& operator<<(std::ostream& out, const lua_stack& self)
 {
-    const char* sep = "stack: [";
+    out << "stack: [";
+    const char* sep = "";
     for (int index = 1; index <= lua_gettop(self.L); ++index)
     {
         out << sep << lua_what(self.L, index);
diff --git a/indra/llcommon/lua_function.h b/indra/llcommon/lua_function.h
index 08a2353d29..07848e38af 100644
--- a/indra/llcommon/lua_function.h
+++ b/indra/llcommon/lua_function.h
@@ -17,6 +17,7 @@
 #include "luau/luaconf.h"
 #include "luau/lualib.h"
 #include "stringize.h"
+#include <exception>                // std::uncaught_exceptions()
 #include <memory>                   // std::shared_ptr
 #include <utility>                  // std::pair
 
@@ -216,4 +217,40 @@ private:
     lua_State* L;
 };
 
+// adapted from indra/test/debug.h
+// can't generalize Debug::operator() target because it's a variadic template
+class LuaLog
+{
+public:
+    template <typename... ARGS>
+    LuaLog(lua_State* L, ARGS&&... args):
+        L(L),
+        mBlock(stringize(std::forward<ARGS>(args)...))
+    {
+        (*this)("entry ", lua_stack(L));
+    }
+
+    // non-copyable
+    LuaLog(const LuaLog&) = delete;
+    LuaLog& operator=(const LuaLog&) = delete;
+
+    ~LuaLog()
+    {
+        auto exceptional{ std::uncaught_exceptions()? "exceptional " : "" };
+        (*this)(exceptional, "exit ", lua_stack(L));
+    }
+
+    template <typename... ARGS>
+    void operator()(ARGS&&... args)
+    {
+        LL_INFOS("Lua") << mBlock << ' ';
+        stream_to(LL_CONT, std::forward<ARGS>(args)...);
+        LL_ENDL;
+    }
+
+private:
+    lua_State* L;
+    const std::string mBlock;
+};
+
 #endif /* ! defined(LL_LUA_FUNCTION_H) */
diff --git a/indra/llcommon/tests/lleventcoro_test.cpp b/indra/llcommon/tests/lleventcoro_test.cpp
index c7a958da49..6ff895c1c1 100644
--- a/indra/llcommon/tests/lleventcoro_test.cpp
+++ b/indra/llcommon/tests/lleventcoro_test.cpp
@@ -113,30 +113,27 @@ namespace tut
 
     void test_data::explicit_wait(boost::shared_ptr<LLCoros::Promise<std::string>>& cbp)
     {
-        DEBUGIN
-        {
-            mSync.bump();
-            // The point of this test is to verify / illustrate suspending a
-            // coroutine for something other than an LLEventPump. In other
-            // words, this shows how to adapt to any async operation that
-            // provides a callback-style notification (and prove that it
-            // works).
+        DEBUG;
+        mSync.bump();
+        // The point of this test is to verify / illustrate suspending a
+        // coroutine for something other than an LLEventPump. In other
+        // words, this shows how to adapt to any async operation that
+        // provides a callback-style notification (and prove that it
+        // works).
 
-            // Perhaps we would send a request to a remote server and arrange
-            // for cbp->set_value() to be called on response.
-            // For test purposes, instead of handing 'callback' (or an
-            // adapter) off to some I/O subsystem, we'll just pass it back to
-            // our caller.
-            cbp = boost::make_shared<LLCoros::Promise<std::string>>();
-            LLCoros::Future<std::string> future = LLCoros::getFuture(*cbp);
+        // Perhaps we would send a request to a remote server and arrange
+        // for cbp->set_value() to be called on response.
+        // For test purposes, instead of handing 'callback' (or an
+        // adapter) off to some I/O subsystem, we'll just pass it back to
+        // our caller.
+        cbp = boost::make_shared<LLCoros::Promise<std::string>>();
+        LLCoros::Future<std::string> future = LLCoros::getFuture(*cbp);
 
-            // calling get() on the future causes us to suspend
-            debug("about to suspend");
-            stringdata = future.get();
-            mSync.bump();
-            ensure_equals("Got it", stringdata, "received");
-        }
-        DEBUGEND
+        // calling get() on the future causes us to suspend
+        debug("about to suspend");
+        stringdata = future.get();
+        mSync.bump();
+        ensure_equals("Got it", stringdata, "received");
     }
 
     template<> template<>
@@ -163,13 +160,9 @@ namespace tut
 
     void test_data::waitForEventOn1()
     {
-        DEBUGIN
-        {
-            mSync.bump();
-            result = suspendUntilEventOn("source");
-            mSync.bump();
-        }
-        DEBUGEND
+        mSync.bump();
+        result = suspendUntilEventOn("source");
+        mSync.bump();
     }
 
     template<> template<>
@@ -189,15 +182,11 @@ namespace tut
 
     void test_data::coroPump()
     {
-        DEBUGIN
-        {
-            mSync.bump();
-            LLCoroEventPump waiter;
-            replyName = waiter.getName();
-            result = waiter.suspend();
-            mSync.bump();
-        }
-        DEBUGEND
+        mSync.bump();
+        LLCoroEventPump waiter;
+        replyName = waiter.getName();
+        result = waiter.suspend();
+        mSync.bump();
     }
 
     template<> template<>
@@ -217,16 +206,12 @@ namespace tut
 
     void test_data::postAndWait1()
     {
-        DEBUGIN
-        {
-            mSync.bump();
-            result = postAndSuspend(LLSDMap("value", 17),       // request event
-                                 immediateAPI.getPump(),     // requestPump
-                                 "reply1",                   // replyPump
-                                 "reply");                   // request["reply"] = name
-            mSync.bump();
-        }
-        DEBUGEND
+        mSync.bump();
+        result = postAndSuspend(LLSDMap("value", 17),       // request event
+                                immediateAPI.getPump(),     // requestPump
+                                "reply1",                   // replyPump
+                                "reply");                   // request["reply"] = name
+        mSync.bump();
     }
 
     template<> template<>
@@ -240,15 +225,11 @@ namespace tut
 
     void test_data::coroPumpPost()
     {
-        DEBUGIN
-        {
-            mSync.bump();
-            LLCoroEventPump waiter;
-            result = waiter.postAndSuspend(LLSDMap("value", 17),
-                                        immediateAPI.getPump(), "reply");
-            mSync.bump();
-        }
-        DEBUGEND
+        mSync.bump();
+        LLCoroEventPump waiter;
+        result = waiter.postAndSuspend(LLSDMap("value", 17),
+                                       immediateAPI.getPump(), "reply");
+        mSync.bump();
     }
 
     template<> template<>
diff --git a/indra/newview/llluamanager.cpp b/indra/newview/llluamanager.cpp
index 0c39f6c8df..cd663021a1 100644
--- a/indra/newview/llluamanager.cpp
+++ b/indra/newview/llluamanager.cpp
@@ -195,30 +195,6 @@ lua_function(get_event_next,
     return 2;
 }
 
-lua_function(await_event,
-             "await_event(pumpname [, timeout [, value to return if timeout (default nil)]]):\n"
-             "pause the running Lua chunk until the next event on the named LLEventPump")
-{
-    auto pumpname{ lua_tostdstring(L, 1) };
-    LLSD result;
-    if (lua_gettop(L) > 1)
-    {
-        auto timeout{ lua_tonumber(L, 2) };
-        // with no 3rd argument, should be LLSD()
-        auto dftval{ lua_tollsd(L, 3) };
-        lua_settop(L, 0);
-        result = llcoro::suspendUntilEventOnWithTimeout(pumpname, timeout, dftval);
-    }
-    else
-    {
-        // no timeout
-        lua_pop(L, 1);
-        result = llcoro::suspendUntilEventOn(pumpname);
-    }
-    lua_pushllsd(L, result);
-    return 1;
-}
-
 void LLLUAmanager::runScriptFile(const std::string& filename, script_finished_fn cb)
 {
     // A script_finished_fn is used to initialize the LuaState.
@@ -361,14 +337,36 @@ std::string read_file(const std::string &name)
 
     if (in_file.is_open())
     {
-        std::string text {std::istreambuf_iterator<char>(in_file), {}};
-        return text;
+        return std::string{std::istreambuf_iterator<char>(in_file), {}};
     }
 
-    return std::string();
+    return {};
 }
 
-LLRequireResolver::LLRequireResolver(lua_State *L, const std::string& path) : mPathToResolve(path), L(L)
+lua_function(require, "require(module_name) : load module_name.lua from known places")
+{
+    std::string name = lua_tostdstring(L, 1);
+    lua_pop(L, 1);
+
+    // resolveRequire() does not return in case of error.
+    LLRequireResolver::resolveRequire(L, name);
+
+    // resolveRequire() returned the newly-loaded module on the stack top.
+    // Return it.
+    return 1;
+}
+
+// push loaded module or throw Lua error
+void LLRequireResolver::resolveRequire(lua_State *L, std::string path)
+{
+    LLRequireResolver resolver(L, std::move(path));
+    // findModule() pushes the loaded module or throws a Lua error.
+    resolver.findModule();
+}
+
+LLRequireResolver::LLRequireResolver(lua_State *L, const std::string& path) :
+    mPathToResolve(path),
+    L(L)
 {
     //Luau lua_Debug and lua_getinfo() are different compared to default Lua:
     //see https://github.com/luau-lang/luau/blob/80928acb92d1e4b6db16bada6d21b1fb6fa66265/VM/include/lua.h
@@ -383,31 +381,59 @@ LLRequireResolver::LLRequireResolver(lua_State *L, const std::string& path) : mP
         luaL_argerrorL(L, 1, "cannot require a full path");
 }
 
-[[nodiscard]] LLRequireResolver::ResolvedRequire LLRequireResolver::resolveRequire(lua_State *L, std::string path)
+/**
+ * Remove a particular stack index on exit from enclosing scope.
+ * If you pass a negative index (meaning relative to the current stack top),
+ * converts to an absolute index. The point of LuaRemover is to remove the
+ * entry at the specified index regardless of subsequent pushes to the stack.
+ */
+class LuaRemover
 {
-    LLRequireResolver resolver(L, std::move(path));
-    ModuleStatus status = resolver.findModule();
-    if (status != ModuleStatus::FileRead)
-        return ResolvedRequire {status};
-    else
-        return ResolvedRequire {status, resolver.mAbsolutePath, resolver.mSourceCode};
-}
+public:
+    LuaRemover(lua_State* L, int index):
+        mState(L),
+        mIndex(lua_absindex(L, index))
+    {}
+    LuaRemover(const LuaRemover&) = delete;
+    LuaRemover& operator=(const LuaRemover&) = delete;
+    ~LuaRemover()
+    {
+        lua_remove(mState, mIndex);
+    }
+
+private:
+    lua_State* mState;
+    int mIndex;
+};
 
-LLRequireResolver::ModuleStatus LLRequireResolver::findModule()
+// push the loaded module or throw a Lua error
+void LLRequireResolver::findModule()
 {
-    resolveAndStoreDefaultPaths();
+    // If mPathToResolve is absolute, this replaces mSourceChunkname.parent_path.
+    auto absolutePath = (std::filesystem::path((mSourceChunkname)).parent_path() / mPathToResolve).u8string();
 
-    // Put _MODULES table on stack for checking and saving to the cache
+    // Push _MODULES table on stack for checking and saving to the cache
     luaL_findtable(L, LUA_REGISTRYINDEX, "_MODULES", 1);
+    // Remove that stack entry no matter how we exit
+    LuaRemover rm_MODULES(L, -1);
+
+    // Check if the module is already in _MODULES table, read from file
+    // otherwise.
+    // findModuleImpl() pushes module if found, nothing if not, may throw Lua
+    // error.
+    if (findModuleImpl(absolutePath))
+        return;
 
-    // Check if the module is already in _MODULES table, read from file otherwise
-    LLRequireResolver::ModuleStatus moduleStatus = findModuleImpl();
-
-    if (moduleStatus != LLRequireResolver::ModuleStatus::NotFound)
-        return moduleStatus;
+    // not already cached - prep error message just in case
+    auto fail{
+        [L=L, path=mPathToResolve]()
+        { luaL_error(L, "could not find require('%s')", path.data()); }};
 
     if (std::filesystem::path(mPathToResolve).is_absolute())
-        return moduleStatus;
+    {
+        // no point searching known directories for an absolute path
+        fail();
+    }
 
     std::vector<std::string> lib_paths {gDirUtilp->getExpandedFilename(LL_PATH_SCRIPTS, "lua")};
 
@@ -416,30 +442,31 @@ LLRequireResolver::ModuleStatus LLRequireResolver::findModule()
         std::string absolutePathOpt = (std::filesystem::path(path) / mPathToResolve).u8string();
 
         if (absolutePathOpt.empty())
-            luaL_errorL(L, "error requiring module");
-
-        mAbsolutePath = absolutePathOpt;
-
-        moduleStatus = findModuleImpl();
+            luaL_error(L, "error requiring module '%s'", mPathToResolve.data());
 
-        if (moduleStatus != LLRequireResolver::ModuleStatus::NotFound)
-            return moduleStatus;
+        if (findModuleImpl(absolutePathOpt))
+            return;
     }
 
-    return LLRequireResolver::ModuleStatus::NotFound;
+    // not found
+    fail();
 }
 
-LLRequireResolver::ModuleStatus LLRequireResolver::findModuleImpl()
+// expects _MODULES table on stack top (and leaves it there)
+// - if found, pushes loaded module and returns true
+// - not found, pushes nothing and returns false
+// - may throw Lua error
+bool LLRequireResolver::findModuleImpl(const std::string& absolutePath)
 {
-    std::string possibleSuffixedPaths[] = {mAbsolutePath + ".luau", mAbsolutePath + ".lua"};
+    std::string possibleSuffixedPaths[] = {absolutePath + ".luau", absolutePath + ".lua"};
 
-     for (auto suffixedPath : possibleSuffixedPaths)
+    for (const auto& suffixedPath : possibleSuffixedPaths)
     {
          // Check _MODULES cache for module
-        lua_getfield(L, -1, suffixedPath.c_str());
+        lua_getfield(L, -1, suffixedPath.data());
         if (!lua_isnil(L, -1))
         {
-            return ModuleStatus::Cached;
+            return true;
         }
         lua_pop(L, 1);
 
@@ -447,91 +474,73 @@ LLRequireResolver::ModuleStatus LLRequireResolver::findModuleImpl()
         std::string source = read_file(suffixedPath);
         if (!source.empty())
         {
-            mAbsolutePath = suffixedPath;
-            mSourceCode = source;
-            return ModuleStatus::FileRead;
-        }
-    }
+            // Try to run the loaded source. This will leave either a string
+            // error message or the module contents on the stack top.
+            runModule(suffixedPath, source);
 
-    return ModuleStatus::NotFound;
-}
+            // If the stack top is an error message string, raise it.
+            if (lua_isstring(L, -1))
+                lua_error(L);
 
-void LLRequireResolver::resolveAndStoreDefaultPaths()
-{
-    if (!std::filesystem::path(mPathToResolve).is_absolute())
-    {
-        mAbsolutePath = (std::filesystem::path((mSourceChunkname)).parent_path() / mPathToResolve).u8string();;
-    }
-    else
-    {
-        mAbsolutePath = mPathToResolve;
-    }
-}
+            // duplicate the new module: _MODULES newmodule newmodule
+            lua_pushvalue(L, -1);
+            // store _MODULES[found path] = newmodule
+            lua_setfield(L, -3, suffixedPath.data());
 
-static int finishrequire(lua_State *L)
-{
-    if (lua_isstring(L, -1))
-        lua_error(L);
+            return true;
+        }
+    }
 
-    return 1;
+    return false;
 }
 
-lua_function(require, "require(module_name) : module_name can be fullpath or just the name, in both cases without .lua")
+// push string error message or new module
+void LLRequireResolver::runModule(const std::string& desc, const std::string& code)
 {
-    std::string name = lua_tostdstring(L, 1);
-
-    LLRequireResolver::ResolvedRequire resolvedRequire = LLRequireResolver::resolveRequire(L, name);
-
-    if (resolvedRequire.status == LLRequireResolver::ModuleStatus::Cached) 
-    {
-        // remove _MODULES from stack
-        lua_remove(L, -2);
-        return finishrequire(L);
-    }
-    else if (resolvedRequire.status == LLRequireResolver::ModuleStatus::NotFound)
-        luaL_errorL(L, "error requiring module");
-
-    // module needs to run in a new thread, isolated from the rest
-    // note: we create ML on main thread so that it doesn't inherit environment of L
+    // Here we just loaded a new module 'code', need to run it and get its result.
+    // Module needs to run in a new thread, isolated from the rest.
+    // Note: we create ML on main thread so that it doesn't inherit environment of L.
     lua_State *GL = lua_mainthread(L);
     lua_State *ML = lua_newthread(GL);
+    // lua_newthread() pushed the new thread object on GL's stack. Move to L's.
     lua_xmove(GL, L, 1);
 
     // new thread needs to have the globals sandboxed
     luaL_sandboxthread(ML);
 
     {
-        if (lluau::loadstring(ML, resolvedRequire.absolutePath.c_str(), resolvedRequire.sourceCode.c_str()) == LUA_OK)
+        // If loadstring() returns (! LUA_OK) then there's an error message on
+        // the stack. If it returns LUA_OK then the newly-loaded module code
+        // is on the stack.
+        if (lluau::loadstring(ML, desc, code) == LUA_OK)
         {
+            // luau uses Lua 5.3's version of lua_resume():
+            // run the coroutine on ML, "from" L, passing no arguments.
             int status = lua_resume(ML, L, 0);
 
             if (status == LUA_OK)
             {
-                if (lua_gettop(ML) == LUA_OK)
-                    lua_pushstring(ML, "module must return a value");
+                if (lua_gettop(ML) == 0)
+                    lua_pushfstring(ML, "module %s must return a value", desc.data());
                 else if (!lua_istable(ML, -1) && !lua_isfunction(ML, -1))
-                    lua_pushstring(ML, "module must return a table or function");
+                    lua_pushfstring(ML, "module %s must return a table or function, not %s",
+                                    desc.data(), lua_typename(ML, lua_type(ML, -1)));
             }
             else if (status == LUA_YIELD)
             {
-                lua_pushstring(ML, "module can not yield");
+                lua_pushfstring(ML, "module %s can not yield", desc.data());
             }
             else if (!lua_isstring(ML, -1))
             {
-                lua_pushstring(ML, "unknown error while running module");
+                lua_pushfstring(ML, "unknown error while running module %s", desc.data());
             }
         }
     }
-    // there's now a return value on top of ML; L stack: _MODULES ML
+    // There's now a return value (string error message or module) on top of ML.
+    // Move return value to L's stack.
     lua_xmove(ML, L, 1);
-    lua_pushvalue(L, -1);
-    lua_setfield(L, -4, resolvedRequire.absolutePath.c_str());
-
-    //remove _MODULES from stack
-    lua_remove(L, -3);
-    // remove ML from stack
+    // remove ML from L's stack
     lua_remove(L, -2);
-    
-    // L stack: [module name] [result]
-    return finishrequire(L);
+//  // DON'T call lua_close(ML)! Since ML is only a thread of L, corrupts L too!    
+//  lua_close(ML);
 }
diff --git a/indra/newview/llluamanager.h b/indra/newview/llluamanager.h
index 43950ccee4..fb9f1b8141 100644
--- a/indra/newview/llluamanager.h
+++ b/indra/newview/llluamanager.h
@@ -87,24 +87,7 @@ public:
 class LLRequireResolver
 {
  public:
-    enum class ModuleStatus
-    {
-        Cached,
-        FileRead,
-        NotFound
-    };
-
-    struct ResolvedRequire
-    {
-        ModuleStatus status;
-        std::string absolutePath;
-        std::string sourceCode;
-    };
-
-    [[nodiscard]] ResolvedRequire static resolveRequire(lua_State *L, std::string path);
-
-    std::string mAbsolutePath;
-    std::string mSourceCode;
+    static void resolveRequire(lua_State *L, std::string path);
 
  private:
     std::string mPathToResolve;
@@ -112,10 +95,10 @@ class LLRequireResolver
 
     LLRequireResolver(lua_State *L, const std::string& path);
 
-    ModuleStatus findModule();
+    void findModule();
     lua_State *L;
 
-    void resolveAndStoreDefaultPaths();
-    ModuleStatus findModuleImpl();
+    bool findModuleImpl(const std::string& absolutePath);
+    void runModule(const std::string& desc, const std::string& code);
 };
 #endif
diff --git a/indra/newview/scripts/lua/Queue.lua b/indra/newview/scripts/lua/Queue.lua
new file mode 100644
index 0000000000..e178ad9969
--- /dev/null
+++ b/indra/newview/scripts/lua/Queue.lua
@@ -0,0 +1,40 @@
+-- from https://create.roblox.com/docs/luau/queues#implementing-queues
+
+local Queue = {}
+Queue.__index = Queue
+
+function Queue.new()
+	local self = setmetatable({}, Queue)
+
+	self._first = 0
+	self._last = -1
+	self._queue = {}
+
+	return self
+end
+
+-- Check if the queue is empty
+function Queue:IsEmpty()
+	return self._first > self._last
+end
+
+-- Add a value to the queue
+function Queue:Enqueue(value)
+	local last = self._last + 1
+	self._last = last
+	self._queue[last] = value
+end
+
+-- Remove a value from the queue
+function Queue:Dequeue()
+	local first = self._first
+	if self:IsEmpty() then
+		return nil
+	end
+	local value = self._queue[first]
+	self._queue[first] = nil
+	self._first = first + 1
+	return value
+end
+
+return Queue
diff --git a/indra/newview/scripts/lua/testmod.lua b/indra/newview/scripts/lua/testmod.lua
new file mode 100644
index 0000000000..60f7f80db1
--- /dev/null
+++ b/indra/newview/scripts/lua/testmod.lua
@@ -0,0 +1,2 @@
+print('loaded scripts/lua/testmod.lua')
+return function () return 'hello from scripts/lua/testmod.lua' end
diff --git a/indra/newview/viewer_manifest.py b/indra/newview/viewer_manifest.py
index 1fa4df1682..562c7166a2 100755
--- a/indra/newview/viewer_manifest.py
+++ b/indra/newview/viewer_manifest.py
@@ -166,6 +166,9 @@ class ViewerManifest(LLManifest):
                         self.path("*/*/*/*.js")
                         self.path("*/*/*.html")
 
+            with self.prefix(src_dst="scripts/lua"):
+                self.path("*.lua")
+
             #build_data.json.  Standard with exception handling is fine.  If we can't open a new file for writing, we have worse problems
             #platform is computed above with other arg parsing
             build_data_dict = {"Type":"viewer","Version":'.'.join(self.args['version']),
diff --git a/indra/test/debug.h b/indra/test/debug.h
index f92cce3008..3c4f3cabb4 100644
--- a/indra/test/debug.h
+++ b/indra/test/debug.h
@@ -31,6 +31,7 @@
 
 #include "print.h"
 #include "stringize.h"
+#include <exception>                // std::uncaught_exceptions()
 
 /*****************************************************************************
 *   Debugging stuff
@@ -65,10 +66,12 @@ public:
 
     // non-copyable
     Debug(const Debug&) = delete;
+    Debug& operator=(const Debug&) = delete;
 
     ~Debug()
     {
-        (*this)("exit");
+        auto exceptional{ std::uncaught_exceptions()? "exceptional " : "" };
+        (*this)(exceptional, "exit");
     }
 
     template <typename... ARGS>
@@ -90,20 +93,4 @@ private:
 // of the Debug block.
 #define DEBUG Debug debug(LL_PRETTY_FUNCTION)
 
-// These DEBUGIN/DEBUGEND macros are specifically for debugging output --
-// please don't assume you must use such for coroutines in general! They only
-// help to make control flow (as well as exception exits) explicit.
-#define DEBUGIN                                 \
-{                                               \
-    DEBUG;                                      \
-    try
-
-#define DEBUGEND                                \
-    catch (...)                                 \
-    {                                           \
-        debug("*** exceptional ");              \
-        throw;                                  \
-    }                                           \
-}
-
 #endif /* ! defined(LL_DEBUG_H) */