Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Mar 1, 2024

Conversation

nat-goodspeed
Copy link
Contributor

Push throwing Lua errors down into LLRequireResolver::findModule() and
findModuleImpl() so their callers don't have to handle the error case. That
eliminates finishrequire().

require() itself now only retrieves (and pops) the passed module name and
calls LLRequireResolver::resolveRequire() to do the actual work.

resolveRequire() is now void. It only instantiates LLRequireResolver and calls
its findModule() method.

findModule() is now also void. It's guaranteed to either push the loaded Lua
module or throw a Lua error. In particular, when findModuleImpl() cannot find
the specified module, findModule() throws an error. That replaces
ModuleStatus::NotFound.

Since std::filesystem::path::append() aka operator/() detects when its right
operand is absolute and, in that case, discards the left operand, we no longer
need resolveAndStoreDefaultPaths(): we can just invoke that operation inline.

When findModule() pushes _MODULES on the Lua stack, it uses LuaRemover (below)
to ensure that _MODULES is removed again no matter how findModules() exits.

findModuleImpl() now accepts the candidate pathname as its argument. That
eliminates mAbsolutePath.

findModuleImpl() now returns only bool: true means the module was found and
loaded and pushed on the Lua stack, false means not found and nothing was
pushed; no return means an error was reported.

Push running a newly found module's source file down into findModuleImpl().
That eliminates the distinction between Cached and FileRead, which obviates
ModuleStatus: a true return means either "previously cached" or "we read it,
compiled it, loaded it and ran it." That also eliminates the need to store the
module's textual content in mSourceCode.

Similarly, once loading the module succeeds, findModuleImpl() caches it in
_MODULES right away. That eliminates ResolvedRequire since we need not pass
the full pathname of the found module (or its contents) back up through the
call chain.

Move require() code that runs the new module into private runModule()method,
called by findModuleImpl() in the not-cached case. runModule() is the only
remaining method that can push either a string error message or the desired
module, because of its funny stack manipulations. That means the check for a
string error message on the stack top can move down to findModuleImpl().

Add LuaRemover class to ensure that on exit from some particular C++ block,
the specified Lua stack entry will definitely be removed. This is different
from LuaPopper in that it engages lua_remove() rather than lua_pop().

Also ditch obsolete await_event() Lua entry point.

Disable copy assignment operator as well as copy constructor.

Use std::uncaught_exceptions() in destructor to report whether there's an
in-flight exception at block exit. Since that was the whole point of the
DEBUGIN / DEBUGEND macros, those become obsolete. Ditch them and their
existing invocations.
Push throwing Lua errors down into LLRequireResolver::findModule() and
findModuleImpl() so their callers don't have to handle the error case. That
eliminates finishrequire().

require() itself now only retrieves (and pops) the passed module name and
calls LLRequireResolver::resolveRequire() to do the actual work.

resolveRequire() is now void. It only instantiates LLRequireResolver and calls
its findModule().

findModule() is now also void. It's guaranteed to either push the loaded Lua
module or throw a Lua error. In particular, when findPathImpl() cannot find
the specified module, findModule() throws an error. That replaces
ModuleStatus::NotFound.

Since std::filesystem::path::append() aka operator/() detects when its right
operand is absolute and, in that case, discards the left operand, we no longer
need resolveAndStoreDefaultPaths(): we can just invoke that operation inline.

When findModule() pushes _MODULES on the Lua stack, it uses LuaRemover (below)
to ensure that _MODULES is removed again no matter how findModules() exits.

findModuleImpl() now accepts the candidate pathname as its argument. That
eliminates mAbsolutePath.

findModuleImpl() now returns only bool: true means the module was found and
loaded and pushed on the Lua stack, false means not found and nothing was
pushed; no return means an error was reported.

Push running a newly found module's source file down into findModuleImpl().
That eliminates the distinction between Cached and FileRead, which obviates
ModuleStatus: a bool return means either "previously cached" or "we read it,
compiled it, loaded it and ran it." That also eliminates the need to store the
module's textual content in mSourceCode.

Similarly, once loading the module succeeds, findModuleImpl() caches it in
_MODULES right away. That eliminates ResolvedRequire since we need not pass
the full pathname of the found module (or its contents) back up through the
call chain.

Move require() code that runs the new module into private runModule() method,
called by findModuleImpl() in the not-cached case. runModule() is the only
remaining method that can push either a string error message or the desired
module, because of its funny stack manipulations. That means the check for a
string error message on the stack top can move down to findModuleImpl().

Add LuaRemover class to ensure that on exit from some particular C++ block,
the specified Lua stack entry will definitely be removed. This is different
from LuaPopper in that it engages lua_remove() rather than lua_pop().

Also ditch obsolete await_event() Lua entry point.
* 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
Copy link
Contributor

@maxim-productengine maxim-productengine Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I like this addition for clearing the stack. _MODULES is removed much cleaner now.

Add Queue.lua from roblox.com documentation.
@nat-goodspeed nat-goodspeed merged commit de71c63 into release/luau-scripting Mar 1, 2024
6 checks passed
@nat-goodspeed nat-goodspeed deleted the require-tweaks branch March 1, 2024 13:55
@github-actions github-actions bot locked and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants