`Feature`: implemented (almost) all `io.` and `file:` functions in stdlib #112

reloginn · 2025-05-08T18:24:21Z

All functions are based on and fully comply with the Lua 5.3 manual (https://www.lua.org/manual/5.3/).

Features

Implemented functions io.close, io.flush, io.input, io.lines, io.open, io.output, io.read, io.tmpfile, io.type, io.write, file:flush, file:lines, file:read, file:seek, file:write.
Added tests in io.lua for all new functions.
In Cargo.toml, the tempfile and either crates have been added. tempfile is used for io.tmpfile, and either is used as a helper in the IoFile type.

…rom stack

…io.tmpfile`

notxvilka · 2025-05-21T04:15:29Z

@Aeledfyr since you are reviewing all PRs, could you please review this one as well?

Aeledfyr · 2025-05-21T21:09:53Z

I can review this, but I'm only merging relatively small changes. Larger changes like this will have to wait until kyren is available to review as well, which will likely be a while from now.

I'll do a first-pass review of the larger PRs (and see if there are sub-parts that can be split out to merge sooner), but it may be a while before they can be fully merged.

Aeledfyr

All functions are based on and fully comply with the Lua 5.3 manual

Note that piccolo is generally targetting Lua 5.4 rather than 5.3, but there don't seem to be many differences in the io/file APIs.

I've gone through and done a first-pass review; there are a number of things that can be simplified, but the main things are to reuse the inner implementations directly, rather than going through Lua calls to use them (for the io functions that call file methods), and to try to simplify the input checking / error handling.

Aeledfyr · 2025-05-21T23:19:44Z

src/meta_ops.rs

+        Value::UserData(u) if u.downcast_static::<IoFile>().is_ok() => {
+            let idx = ctx.io_metatable().get_value(ctx, MetaMethod::Index);
+            if idx.is_nil() {
+                return Ok(MetaResult::Value(Value::Nil));
+            }
+            idx
+        }


This should be handled by the existing UserData code, by setting the metatable for the file UserData objects with UserData::set_metatable. (As such, the global io_metatable does not need to exist.)

Aeledfyr · 2025-05-21T23:22:21Z

src/stdlib/io.rs

+    thread_local! {
+        static IO_STATE: IoState = IoState::new();
+    }


This should be a reference counted object (using Rc) rather than a thread-local, so that this can support multiple instances of the Lua runtime without conflicts.

Aeledfyr · 2025-05-21T23:43:31Z

src/stdlib/io.rs

+        "open",
+        Callback::from_fn(&ctx, |ctx, _, mut stack| {
+            let (filename, mode) = stack.consume::<(String, Option<String>)>(ctx)?;
+            let filename = filename.to_str()?;


Non-utf8 filenames can be valid on some systems. Not supporting those is probably fine, but it should be noted here.

Aeledfyr · 2025-05-21T23:51:01Z

src/stdlib/io.rs

+            let mode = mode.map(|s| s.to_str()).unwrap_or(Ok("r"))?;
+
+            let mut file = OpenOptions::new();
+
+            let mode = mode.replace("b", "");
+
+            let file = match mode.as_str() {
+                "r" => file.read(true),
+                "w" => file.write(true).create(true).truncate(true),
+                "a" => file.write(true).create(true).append(true),
+                "r+" => file.read(true).write(true),
+                "w+" => file.read(true).write(true).create(true).truncate(true),
+                "a+" => file.read(true).write(true).create(true).append(true),
+                _ => return Err("invalid `mode`".into_value(ctx).into()),
+            };


A simpler approach to parsing mode would be to match on the byte string directly, and to handle the b variants (since other orderings, like br or rb+, are invalid):

let mode = mode.map(|s| s.as_bytes()).unwrap_or(b"r"); match mode { b"r" | b"rb" => /* ... */, b"r+" | b"r+b" => /* ... */, // ... _ => return Err(/* ... */), }

Aeledfyr · 2025-05-22T00:04:25Z

src/stdlib/io.rs

+                Value::String(filename) => {
+                    let io: Table = ctx.globals().get(ctx, "io")?;
+                    let open: Callback = io.get(ctx, "open")?;
+
+                    stack.replace(ctx, (filename, "r"));
+                    Ok(CallbackReturn::Call {
+                        function: open.into(),
+                        then: Some(BoxSequence::new(&ctx, InputOpenThen)),
+                    })
+                }


This can be simplified significantly by making the implementation of open a normal Rust closure---then, rather than having to call it through the globals table and callbacks/sequences, you can call it as a normal function.

let open = move |filename: &String, mode: &String| -> Result<IoFile, Error> { // ... }; io.set_field(ctx, "open", Callback::from_fn(&ctx, |ctx, _, mut stack| { let (filename, mode) = stack.consume::<(String, Option<String>)>(ctx)?; let result = open(&filename, &mode); match result { // ... } }); io.set_field(ctx, "input", Callback::from_fn(&ctx, |ctx, _, mut stack| { let file = stack.consume::<Option<Value>>(ctx)?; match file { Value::String(name) => { let file = open(&name, "r"); // ... }, // ... } });

Note that you might have to clone the open closure if it captures state (like the io metatable); let open_ref = open.clone() should work for that.

Aeledfyr · 2025-05-22T00:29:25Z

src/stdlib/io/file.rs

+            let mut buf = Vec::new();
+            let mut byte = [0u8; 1];
+
+            loop {
+                match seek_read(file, position, &mut byte) {
+                    Ok(0) => {
+                        if buf.is_empty() {
+                            return Ok(None);
+                        }
+                        break;
+                    }
+                    Ok(_) => {
+                        buf.push(byte[0]);
+                        if byte[0] == b'\n' {
+                            break;
+                        } else if byte[0] == b'\r' {
+                            let mut next_byte = [0u8; 1];
+                            match seek_read(file, position, &mut next_byte) {
+                                Ok(1) => {
+                                    if next_byte[0] == b'\n' {
+                                        *position += 1;
+                                        buf.push(next_byte[0]);
+                                    } else {
+                                        // Again, we can't put back what we read in a generic Read
+                                        // This byte will be part of the next line
+                                    }
+                                    break;
+                                }
+                                _ => break,
+                            }
+                        }
+                    }
+                    Err(err) => return Err(err.to_string().into_value(ctx).into()),
+                }
+            }


Lua's readline only treats \n as newlines, so this can just read until it finds a \n byte. I'd recommend using BufRead::read_until here instead, as it can handle it more efficiently than making a read for each byte.

Aeledfyr · 2025-05-22T00:29:55Z

src/stdlib/io/file.rs

+fn read_from_any<'gc, W: Write + Read + Seek>(
+    ctx: Context<'gc>,
+    file: &mut W,
+    format: &str,


It would be good to make the format an enum, so that this function doesn't need to deal with parsing the format specifier.

Aeledfyr · 2025-05-22T00:31:31Z

src/stdlib/io/file.rs

+    fn seek_read<'gc, W: Write + Read + Seek>(
+        file: &mut W,
+        position: &mut usize,
+        buf: &mut [u8],
+    ) -> io::Result<usize> {
+        file.seek(io::SeekFrom::Start(*position as u64))?;
+        match file.read(buf) {
+            Ok(n) => {
+                *position += n;
+                Ok(n)
+            }
+            Err(err) => Err(err),
+        }
+    }


The offset into the file shouldn't need to be manually tracked -- the files track those internally, and Rust already exposes the calls to change the offset through the Seek trait.

Aeledfyr · 2025-05-22T00:35:01Z

src/stdlib/io/file.rs

+            let s = std::string::String::from_utf8_lossy(&buf);
+            match s
+                .parse::<i64>()
+                .ok()
+                .map(Value::from)
+                .or_else(|| s.parse::<f64>().ok().map(Value::from))
+            {
+                Some(value) => Ok(Some(value)),
+                None => {
+                    *position = start_position;
+                    Ok(None)
+                }
+            }


This should use the existing number parsing logic from the lexer (like compiler::string_utils::{read_integer, read_float}) so that it supports Lua's number syntax.

Aeledfyr · 2025-05-22T00:40:46Z

src/stdlib/io/file.rs

+                    let mut read_bytes = self.read_position.borrow_mut();
+                    let mut stdin = io::stdin();
+                    let mut buf = Vec::new();
+                    stdin.read_exact(&mut buf)?;
+                    let mut cursor = Cursor::new(buf);
+                    read_from_any(ctx, &mut cursor, format, &mut read_bytes)


This approach won't work, since always reads a fixed amount of data (currently 0 bytes). I assume this was a work-around for stdin/stdout not supporting Seek, but read_from_any just needs to be changed to support non-seekable files.

Aeledfyr · 2025-05-22T21:04:18Z

Another high-level thing that needs to be considered is how to handle sandboxing this, and how to expose that to the user. The main things we'd need to achieve for reasonable sandboxing:

Allowing the host to limit access to stdin/stdout/stderr, and/or replace them with custom files/streams
Allowing the host to limit what files can be opened, and what operations can be done on them

One approach that might make this possible would be to have the user provide the system interface through a trait implementation -- something like this:

type FileRef = Rc<RefCell<dyn IoFile>>;
trait IoContext {
    fn open(&self, path: &[u8], mode: Mode) -> Result<FileRef, Error>;
    fn tempfile(&self) -> Result<FileRef, Error>;
    fn stdin(&self) -> Option<FileRef>;
    fn stdout(&self) -> Option<FileRef>;
    fn stderr(&self) -> Option<FileRef>;
}
// For files that only support some of the read/write/seek
// operations, make a placeholder impl that always errors.
trait IoFile: Read + Write + Seek {
    fn close(&mut self) -> Result<(), Error>;
    fn flush(&mut self) -> Result<(), Error>;
}

struct LuaStdin(std::io::Stdin);
impl Read for LuaStdin { /* pass through */ }
impl Write for LuaStdin { /* always error */ }
impl Seek for LuaStdin { /* always error */ }
impl IoFile for LuaStdin {
    fn close(&mut self) -> Result<(), Error> { Ok(()) }
    fn flush(&mut self) -> Result<(), Error> { Ok(()) }
}

With that, load_io could just take in an implementation of IoContext and rely on the user to provide the implementation / sandboxing.

rib · 2025-07-05T01:31:26Z

Another high-level thing that needs to be considered is how to handle sandboxing this, and how to expose that to the user. The main things we'd need to achieve for reasonable sandboxing:

Allowing the host to limit access to stdin/stdout/stderr, and/or replace them with custom files/streams

Allowing the host to limit what files can be opened, and what operations can be done on them

One approach that might make this possible would be to have the user provide the system interface through a trait implementation -- something like this:

Yeah, this suggestion to add some kind of trait abstraction around I/O or OS details sounds good, to avoid assuming it's safe/desirable to expose the host OS / filesystem to Lua code.

I've been playing with using piccolo recently and at least for my use case it's a feature that piccolo doesn't support the io APIs which allow code to interact with the host platform.

reloginn added 9 commits May 7, 2025 01:12

feature: implemented (almost) all functions in io module

f0d3a8e

fix: discrepancies in implementation

f14f527

fix: IoState is now static variable, bug fix for getting values f…

6cee501

…rom stack

feature: metatable support; refactor: split code into modules

4d0ceb3

fix: bug in read_with_format, missing implementation

2998d9f

fix: bug in read_from_any, added tests

e59fa03

fix: bug in file:seek, added tests for file:seek

21f72e3

refactor: rename read_state to read_position, added tests for `…

83e38f6

…io.tmpfile`

update COMPABILITY.md and README.md

fddbbf9

Aeledfyr requested changes May 22, 2025

View reviewed changes

reloginn marked this pull request as draft June 3, 2025 17:13

Merge branch 'kyren:master' into io

dd572fd

Feature: implemented (almost) all io.* and file:* functions in stdlib #112

Are you sure you want to change the base?

Feature: implemented (almost) all io.* and file:* functions in stdlib #112

Conversation

reloginn commented May 8, 2025

Features

Uh oh!

notxvilka commented May 21, 2025

Uh oh!

Aeledfyr commented May 21, 2025

Uh oh!

Aeledfyr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aeledfyr commented May 22, 2025

Uh oh!

rib commented Jul 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

`Feature`: implemented (almost) all `io.` and `file:` functions in stdlib #112

`Feature`: implemented (almost) all `io.` and `file:` functions in stdlib #112