Skip to content

improve lua error handling #16

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.

Sign up for GitHub

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

Open
D-Berg wants to merge 1 commit into from
Open

improve lua error handling #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 24 additions & 18 deletions src/actions/build_package.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ pub fn build(io: Io, gpa: Allocator, arena: Allocator, env: *std.process.Environ
try lua.new(0);
defer lua.close();

lua_helpers.setupState(&lua);
try lua_helpers.setupState(&lua);

var state: Package.State = .empty;
const pkg_id = try Package.collect(io, arena, &state, packa_dir, args.package_name, &lua, true);
Expand Down Expand Up @@ -154,8 +154,11 @@ pub fn build(io: Io, gpa: Allocator, arena: Allocator, env: *std.process.Environ
.verbose = args.verbose,
};
{ // b.run = luaRun
lua.pushLightUserdata(@ptrCast(@alignCast(@constCast(&run_ctx))));
lua.pushCClosure(luaRun, 1);
try lua_helpers.pushChecked(
&lua,
luaRun,
@ptrCast(@alignCast(@constCast(&run_ctx))),
);
lua.setField(b, "run");
}

Expand All @@ -166,30 +169,33 @@ pub fn build(io: Io, gpa: Allocator, arena: Allocator, env: *std.process.Environ
.pkg_state = &state,
};
{
lua.pushLightUserdata(@ptrCast(@alignCast(@constCast(&dep_ctx))));
lua.pushCClosure(luaDep, 1);
try lua_helpers.pushChecked(
&lua,
luaDep,
@ptrCast(@alignCast(@constCast(&dep_ctx))),
);
lua.setField(b, "dep");
}

const join_ctx: PathJoinContext = .{ .gpa = gpa };
lua.pushLightUserdata(@ptrCast(@alignCast(@constCast(&join_ctx))));
lua.pushCClosure(luaPathJoin, 1);
try lua_helpers.pushChecked(
&lua,
luaPathJoin,
@ptrCast(@alignCast(@constCast(&join_ctx))),
);
lua.setField(b, "pathJoin");

{ // b.env = env;
lua.createTable(0, 3);
const env_table = lua.getTop();

lua.pushLightUserdata(@ptrCast(@alignCast(&build_env)));
lua.pushCClosure(luaEnvSet, 1);
try lua_helpers.pushChecked(&lua, luaEnvSet, @ptrCast(@alignCast(&build_env)));
lua.setField(env_table, "set");

lua.pushLightUserdata(@ptrCast(@alignCast(&build_env)));
lua.pushCClosure(luaEnvGet, 1);
try lua_helpers.pushChecked(&lua, luaEnvGet, @ptrCast(@alignCast(&build_env)));
lua.setField(env_table, "get");

lua.pushLightUserdata(@ptrCast(@alignCast(&build_env)));
lua.pushCClosure(luaEnvAppend, 1);
try lua_helpers.pushChecked(&lua, luaEnvAppend, @ptrCast(@alignCast(&build_env)));
lua.setField(env_table, "append");

lua.setField(b, "env");
Expand Down Expand Up @@ -291,7 +297,7 @@ fn luaEnvSet(state: ?*zlua.LuaState) callconv(.c) c_int {
}

const ud = lua.toUserdata(lua.upvalueIndex(1)) orelse {
lua.pushBoolean(false);
lua.pushNil();
_ = lua.pushLString("null userdata");
return 2;
};
Expand Down Expand Up @@ -321,13 +327,13 @@ fn luaEnvGet(state: ?*zlua.LuaState) callconv(.c) c_int {
}

const ud = lua.toUserdata(lua.upvalueIndex(1)) orelse {
lua.pushBoolean(false);
lua.pushNil();
_ = lua.pushLString("null userdata");
return 2;
};
const env_map: *std.process.Environ.Map = @ptrCast(@alignCast(ud));

const key = lua.toLString(1);
const key = lua.toString(1);
if (env_map.get(key)) |val| {
_ = lua.pushLString(val);
return 1;
Expand All @@ -346,7 +352,7 @@ fn luaEnvAppend(state: ?*zlua.LuaState) callconv(.c) c_int {
}

const ud = lua.toUserdata(lua.upvalueIndex(1)) orelse {
lua.pushBoolean(false);
lua.pushNil();
_ = lua.pushLString("null userdata");
return 2;
};
Expand Down Expand Up @@ -381,7 +387,7 @@ fn luaRun(state: ?*zlua.LuaState) callconv(.c) c_int {
const n_args: usize = @intCast(lua.getTop());
if (n_args < 1) {
lua.pushNil();
_ = lua.pushLString("run requires atleast 1 arg");
_ = lua.pushLString("run requires atleast 1 argument");

@coderabbitai coderabbitai Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Correct the error text.

Change "atleast" to "at least".

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/actions/build_package.zig` at line 390, Update the error message in the
Lua argument-validation logic to use the correctly spaced phrase “at least”
instead of “atleast,” preserving the rest of the message unchanged.

return 2;
}

Expand Down
2 changes: 1 addition & 1 deletion src/actions/info.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ pub fn info(io: Io, gpa: Allocator, package_name: []const u8) !void {
try lua.new(0);
defer lua.close();

lua_helpers.setupState(&lua);
try lua_helpers.setupState(&lua);

var state: Package.State = .empty;
defer state.deinit(gpa);
Expand Down
2 changes: 1 addition & 1 deletion src/actions/install.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ pub fn install(
var lua: zlua.State = .{ .gpa = gpa };
try lua.new(0);
defer lua.close();
lua_helpers.setupState(&lua);
try lua_helpers.setupState(&lua);

var state: Package.State = .empty;
defer state.deinit(gpa);
Expand Down
13 changes: 13 additions & 0 deletions src/error_wrapper.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---Wraps `native` functions so they can safely return
--- errors and defer with longjump after cleanup
---@param native fun(...: any): (any, string?)
---@return fun(...: any): any
return function(native)
return function(...)
local value, failure = native(...)
if failure ~= nil then
error(failure, 2)
Comment on lines +5 to +9

@coderabbitai coderabbitai Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Capture the Lua error function during wrapper setup.

A manifest can replace global error before a checked native function fails. The wrapper can then return normally after a failed native call. Capture error in a local before returning the wrapper.

Proposed fix 
+local lua_error = error
+
 return function(native)
     return function(...)
         local value, failure = native(...)
         if failure ~= nil then
-            error(failure, 2)
+            lua_error(failure, 2)
         end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return function(native)
return function(...)
local value, failure = native(...)
if failure ~= nil then
error(failure, 2)
local lua_error = error
return function(native)
return function(...)
local value, failure = native(...)
if failure ~= nil then
lua_error(failure, 2)
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/error_wrapper.lua` around lines 5 - 9, Update the outer wrapper setup in
the returned function around native so it captures the current global error
function in a local before creating and returning the inner wrapper. Use that
captured function in the failure branch of the inner wrapper instead of
resolving global error at call time, while preserving the existing level
argument and return behavior.

end
return value
end
end
51 changes: 50 additions & 1 deletion src/lua_helpers.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,13 @@ pub fn lua_pkg(state: ?*zlua.LuaState) callconv(.c) c_int {
return 1;
}

pub fn setupState(lua: *const zlua.State) void {
pub fn setupState(lua: *const zlua.State) !void {
lua.requiref("_G", zlua.Open.base, true);

try lua.loadBuffer(@embedFile("error_wrapper.lua"), "@packa_lua_error_wrapper");
try lua.pcall(0, 1, 0);
lua.setField(zlua.REGISTRYINDEX, "packa.checked");

lua.setGlobal("load");
lua.pushNil();
lua.setGlobal("loadfile");
Expand All @@ -25,3 +29,48 @@ pub fn setupState(lua: *const zlua.State) void {
}));
lua.setGlobal("platform");
}

/// Wraps a native function with error_wrapper.lua
pub fn pushChecked(
lua: *const zlua.State,
native: zlua.CFunction,
context: ?*anyopaque,
) !void {
std.debug.assert(lua.getField(zlua.REGISTRYINDEX, "packa.checked") == .function);

var upvalues: usize = 0;
if (context) |ctx| {
lua.pushLightUserdata(ctx);
upvalues += 1;
}
lua.pushCClosure(native, upvalues);
try lua.pcall(1, 1, 0);
}

const TestContext = struct {
cleaned: bool = false,
};

fn testFailure(state: ?*zlua.LuaState) callconv(.c) c_int {
const lua: zlua.State = .{ .inner = state.? };
const context: *TestContext = @ptrCast(@alignCast(lua.toUserdata(lua.upvalueIndex(1))));
defer context.cleaned = true;

lua.pushNil();
_ = lua.pushLString("native failure");
return 2;
}

test pushChecked {
var lua: zlua.State = .{ .gpa = std.testing.allocator };
try lua.new(0);
defer lua.close();

try setupState(&lua);

var context: TestContext = .{};
try pushChecked(&lua, testFailure, &context);
try std.testing.expectError(zlua.Error.Run, lua.pcall(0, 0, 0));
try std.testing.expect(context.cleaned);
try std.testing.expectEqualStrings("native failure", lua.toLString(-1));
}