Rewrite require and luaurc in lune-std #247

FlowGnarly · 2024-08-25T18:32:57Z

Highlights of this PR

doesn't mutably borrow the app container for the entire require process (Fixes Creating a new DataModel and setting it as a global called game errors #236, Fixes Require in metamethod causes a panic #262)
allows custom libraries that impl the StandardLibrary trait to be added to the require envrionment (Fixes Customizing globals without having to fork lune [lune-std] #197)
```
 context::RequireContext::inject_std(lua, "custom", CustomStandardLibrary::Timer)?;
```
```
 local timer = require("@custom/timer")
```

the luaurc parser has better error handling and doesnt fail silently when it fails to parse a .luaurc file (Fixes unable to find .luaurc if .luaurc contains invalid JSON syntax #210)

Failed to parse /path/to/.luaurc
ParserError: trailing comma at line 16 column 1
[Stack Begin]
        Script '[C]'
        Script '__mlua_async_poll', Line 4
        Script 'REPL', Line 1
[Stack End]

API Changes

for the existing crates that use lune-std nothing has to be changed, since this pr only changes the internal code and adds 2 new items which are totally optional and can be accessed by other crates, which are:

pub trait StandardLibrary
where
    Self: Debug,
{
    fn name(&self) -> &'static str;
    fn module<'lua>(&self, lua: &'lua Lua) -> LuaResult<LuaMultiValue<'lua>>;
}

pub struct RequireContext {}

impl RequireContext {
    pub fn inject_std(
        lua: &Lua,
        alias: &'static str,
        std: impl StandardLibrary + 'static,
    ) -> Result<(), RequireError>;
}

filiptibell

Thank you for the PR! I have not yet made comments on any specific files because I wanted to bring a few general points up. First, the positives:

The improvements in error handling and the more strongly typed errors are great. I believe this to be the main highlight of this PR.
The new library trait is a good change for flexbility and I hope we can also use it in the future for package management or something similar.

And then, a couple negatives:

There are areas where this new implementation introduces TOCTOU race conditions, due to the heavy usage of fs::try_exists. It is very important that if a file read succeeds, we use the contents of the file at that specific moment in time.
Rewriting LuauRc fully and removing every field but the aliases one may have been a step too far. It would be nice if Lune could help the user in case they have an invalid config and tell them what is wrong with their fields other than just the aliases. We just needed better error propagation.

Finally, some nitpicks. Feel free to ignore them and wait for me to comment on the specific files and locations if you wish:

We can use the path utils from lune_utils::path instead of re-implementing new versions in lune-std.
In many cases the new error propagation, which is great, introduces allocation when not necessary. Try to use ok_or_else instead of ok_or when creating the error needs allocation.

FlowGnarly · 2024-10-17T10:33:22Z

Hey thanks for the feedback! I really appreciate it

There are areas where this new implementation introduces TOCTOU race conditions, due to the heavy usage of fs::try_exists. It is very important that if a file read succeeds, we use the contents of the file at that specific moment in time.

Ive changed quite alot of code to get around this, it should be good now 😅

Rewriting LuauRc fully and removing every field but the aliases one may have been a step too far. It would be nice if Lune could help the user in case they have an invalid config and tell them what is wrong with their fields other than just the aliases. We just needed better error propagation.

I had no idea why we we're de-serializing all those fields hence I didn't include them in the rewrite, I've bringed back all the fields into the struct so it should be good now (excluding the paths field, because I couldn't find it in any luau rfc, what's that about?)

We can use the path utils from lune_utils::path instead of re-implementing new versions in lune-std.

I didn't know we had all those functions in lune_utils lol, I've removed all the duplicate functions and replaced them with the lune_utils path functions

Try to use ok_or_else instead of ok_or when creating the error needs allocation.

Done!

FlowGnarly added 30 commits August 23, 2024 00:26

luaurc rewrite

ba2d720

resolve require path

3f709f1

StandardLibrary trait

2b7bb89

make standardlibrary object safe

12b87b3

require and cache impl for stds

370c7f8

rename storage to context

cbd7cb6

make cache arc mutex

16b3cde

make data struct ready for impl

62fd0d0

move fn to impl

27a98f9

require impl (no caching)

505ff97

move path functions to path.rs

13cff6f

luaurc error impl

2eb1670

impl custom aliases

a633afc

impl cache

fedaf9a

fixed not converting cached results

25b09b1

drop app container early

3d9bb89

document requirecontext

eaa1c4d

impl require error enum

edb3041

unlock mutex before using async

9414a9f

replace runtime err with require err

1708c05

create thread when requiring module

3b56b41

split require into functions

42873f6

track thread

14d0bcb

make StandardLibrary trait public

e47555f

document functions

ddcda99

make some functions private

35f2422

better error message

42f5688

fixed wrong error message

c6a1808

move path_to_alias to RequireAlias::from_path

a7f9c5f

make RequireContext::init public only to crate

9279b92

filiptibell requested changes Oct 17, 2024

View reviewed changes

FlowGnarly added 4 commits October 17, 2024 12:30

validate every field in luaurc files

7f07c9d

eliminate unnecessary checks for files

76da13d

fix typo

6ce4563

eliminate unnecessary checks for files in require

981d323

create channel before reading file

2a49830

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite require and luaurc in lune-std #247

Rewrite require and luaurc in lune-std #247

FlowGnarly commented Aug 25, 2024 •

edited

Loading

filiptibell left a comment

FlowGnarly commented Oct 17, 2024

Rewrite require and luaurc in lune-std #247

Are you sure you want to change the base?

Rewrite require and luaurc in lune-std #247

Conversation

FlowGnarly commented Aug 25, 2024 • edited Loading

Highlights of this PR

API Changes

filiptibell left a comment

Choose a reason for hiding this comment

FlowGnarly commented Oct 17, 2024

FlowGnarly commented Aug 25, 2024 •

edited

Loading