bug: Replace panic! with proper error handling in FFI runtime code #210

New issue

Closed

opened 2026-01-09 04:42:49 +00:00 by navicore · 1 comment

navicore commented

2026-01-09 04:42:49 +00:00

(Migrated from github.com)

Problem

Runtime FFI functions use panic!() for error conditions, which can crash the entire program instead of gracefully handling errors.

Example from `crates/runtime/src/string_ops.rs` (lines 27-55):

pub unsafe extern "C" fn patch_seq_string_split(stack: Stack) -> Stack {
    assert!(!stack.is_null(), "string_split: stack is empty");
    // ...
    match (str_val, delim_val) {
        (Value::String(s), Value::String(d)) => { /* ... */ }
        _ => panic!("string_split: expected two strings on stack"),  // CRASH!
    }
}

Impact

Critical: Panics across FFI boundary cause undefined behavior
Runtime errors crash the entire program instead of being catchable
Users have no way to recover from type errors at runtime
Poor debugging experience - stack traces may be corrupted

Affected Files

crates/runtime/src/string_ops.rs - Multiple panic! calls
crates/runtime/src/arithmetic.rs - Assert macros
crates/runtime/src/io.rs - Panic on I/O errors
Other runtime modules with extern "C" functions

Proposed Solution

Option 1: Error return codes

#[repr(C)]
pub struct RuntimeResult {
    stack: Stack,
    error_code: i32,  // 0 = success, non-zero = error
    error_msg: *const c_char,
}

pub unsafe extern "C" fn patch_seq_string_split(stack: Stack) -> RuntimeResult {
    if stack.is_null() {
        return RuntimeResult::error(1, "stack is empty");
    }
    // ...
}

Option 2: Thread-local error state

thread_local! {
    static LAST_ERROR: RefCell<Option<String>> = RefCell::new(None);
}

pub unsafe extern "C" fn patch_seq_get_last_error() -> *const c_char { /* ... */ }

Option 3: Catch panics at FFI boundary

pub unsafe extern "C" fn patch_seq_string_split(stack: Stack) -> Stack {
    std::panic::catch_unwind(|| {
        // existing logic
    }).unwrap_or_else(|_| {
        set_error("string_split panicked");
        stack  // return original stack unchanged
    })
}

Acceptance Criteria

No panic!() or assert!() in extern "C" functions
All runtime errors are catchable/recoverable
Error messages are accessible from calling code
Existing tests pass
Add tests for error conditions

Labels

bug, runtime, high-priority

## Problem Runtime FFI functions use `panic!()` for error conditions, which can crash the entire program instead of gracefully handling errors. ### Example from `crates/runtime/src/string_ops.rs` (lines 27-55): ```rust pub unsafe extern "C" fn patch_seq_string_split(stack: Stack) -> Stack { assert!(!stack.is_null(), "string_split: stack is empty"); // ... match (str_val, delim_val) { (Value::String(s), Value::String(d)) => { /* ... */ } _ => panic!("string_split: expected two strings on stack"), // CRASH! } } ``` ## Impact - **Critical**: Panics across FFI boundary cause undefined behavior - Runtime errors crash the entire program instead of being catchable - Users have no way to recover from type errors at runtime - Poor debugging experience - stack traces may be corrupted ## Affected Files - `crates/runtime/src/string_ops.rs` - Multiple panic! calls - `crates/runtime/src/arithmetic.rs` - Assert macros - `crates/runtime/src/io.rs` - Panic on I/O errors - Other runtime modules with `extern "C"` functions ## Proposed Solution ### Option 1: Error return codes ```rust #[repr(C)] pub struct RuntimeResult { stack: Stack, error_code: i32, // 0 = success, non-zero = error error_msg: *const c_char, } pub unsafe extern "C" fn patch_seq_string_split(stack: Stack) -> RuntimeResult { if stack.is_null() { return RuntimeResult::error(1, "stack is empty"); } // ... } ``` ### Option 2: Thread-local error state ```rust thread_local! { static LAST_ERROR: RefCell<Option<String>> = RefCell::new(None); } pub unsafe extern "C" fn patch_seq_get_last_error() -> *const c_char { /* ... */ } ``` ### Option 3: Catch panics at FFI boundary ```rust pub unsafe extern "C" fn patch_seq_string_split(stack: Stack) -> Stack { std::panic::catch_unwind(|| { // existing logic }).unwrap_or_else(|_| { set_error("string_split panicked"); stack // return original stack unchanged }) } ``` ## Acceptance Criteria - [ ] No `panic!()` or `assert!()` in `extern "C"` functions - [ ] All runtime errors are catchable/recoverable - [ ] Error messages are accessible from calling code - [ ] Existing tests pass - [ ] Add tests for error conditions ## Labels bug, runtime, high-priority

navicore commented

2026-01-10 15:43:34 +00:00

(Migrated from github.com)

https://github.com/navicore/patch-seq/pull/223