⏺ Done. Here's what was added to src/eval.rs: #25
No reviewers
Labels
No labels
bug
dependencies
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
rust
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
navicore/patch-rexx!25
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue/24"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
https://github.com/navicore/patch-rexx/issues/24
All 488 tests pass and clippy is clean. Zero breaking changes — the existing set_command_handler API is untouched.
Review: PR #25 — CommandHandlerWithEnv
The take/put-back borrow trick is sound and the general structure is clean. One correctness bug and one minor robustness note.
Bug:
command_handler_with_envreturningNonesilently bypassescommand_handler(eval.rs:524-534)The
if/else ifstructure makes the two handler branches mutually exclusive. Whencommand_handler_with_envisSomebut its handler returnsNone,custom_rcisNoneand execution falls straight to shell —command_handleris never consulted.Both the PR description ("then falls back to the existing
command_handler") and the doc comment onset_command_handler_with_env("This handler is tried before the basiccommand_handler") promise three-level priority:but the implementation delivers:
The fix is to separate the two checks: after running the env-aware handler, if it returned
None, consultcommand_handlerbefore falling through to shell.Minor: panic in the handler permanently clears
command_handler_with_env(eval.rs:524-527)After
take(), if the handler panics, the field staysNonefor the rest of the evaluator lifetime. This matters only if the embedding application usescatch_unwindaround the evaluator, in which case it silently loses the handler on the first panicking invocation.No tests for the new API
There are no tests exercising
set_command_handler_with_env— neither the happy path (returnsSome(rc)) nor the fallthrough path (returnsNone). A test that sets both handlers with the env-aware one returningNonewould have caught the fallthrough bug above.The existing
set_command_handlerpath and all surrounding trap logic (ERROR/FAILURE conditions) are unchanged and look correct.PR Review —
CommandHandlerWithEnvThe overall design is clean and the borrow-conflict workaround (
take/put-back) is the right approach. Three issues worth addressing:1. Take/put-back silently drops handler on panic —
src/eval.rs:525-528If the closure panics,
command_handler_with_envstaysNonefor every subsequent call on thisEvaluator. The field is then indistinguishable from "no handler was registered," so subsequent REXX commands silently fall through to shell execution rather than routing through the custom handler. An embedding application that wrapseval.exec()incatch_unwindwill hit this.The doc comment says handlers must not panic, but that is a runtime contract with no type-system enforcement. At minimum, add a comment at the
takesite explaining the invariant: "if the handler panics the slot is intentionally left empty so the evaluator cannot be called back into a poisoned closure." Alternatively, ascopeguardor a thincatch_unwindwrapper would make the handler robust against misbehaving closures.2. Handler can corrupt PREVIOUS ADDRESS tracking —
src/eval.rs:510-514vssrc/env.rs:194-196set_addressalways rotates the address ring:In the
TemporaryADDRESS dispatch path:REXX requires PREVIOUS to be "XEDIT" (the temporary environment, per the comment at line 512), but it becomes "EXTRA" instead. Since
CommandHandlerWithEnvis the first API that grants handlers&mut Environment, this corruption is only possible through the new type. Theset_command_handler_with_envdoc comment should explicitly state that handlers must not callenv.set_address().3. No test for ERROR trap through the new handler path —
src/eval.rs:558-569The three new tests all assert
ExecSignal::Normal. The trap-firing branch at lines 558–569 is never exercised through the env-aware handler. Without a test that setsSIGNAL ON ERRORbefore issuing a command handled by the env-aware handler with nonzero rc, a future refactor to the trap-firing logic has no coverage for this new path.PR #25 Review —
CommandHandlerWithEnvOverall: the borrow-splitting strategy (take/put-back) is the correct solution, the test coverage is appreciated, and the fallthrough priority is correct. Four issues below, two of which are correctness bugs.
Bug 1 — Test demonstrates wrong storage layer for stem variables (
src/eval.rs:2367)The test handler calls:
env.setstores the value inscope.varsunder key"CURLINE.1". However, REXX code that readsCURLINE.1as a compound variable goes througheval_expr → Expr::Compound → env.get_compound("CURLINE", "1"), which searchesscope.stems, notscope.vars. These are entirely separate storage structures.The assertion on line 2373 passes because
env.get("CURLINE.1")also searchesscope.vars— so the test is self-consistent but does not exercise the stated use case. If actual REXX code in the program didSAY CURLINE.1, it would get back the unset default"CURLINE.1"(the variable's own name), not"Hello from XEDIT".For the documented use case ("refreshing EXTRACT stem variables"), the handler must call:
The docstring on
CommandHandlerWithEnv(line 185) andset_command_handler_with_env(line 972) both cite this EXTRACT-stem pattern as the motivating example, so the bug is in the shipped example, not just the test assertion.Bug 2 — Negative rc from custom handler fires ERROR trap instead of FAILURE trap (
src/eval.rs:562)This fires ERROR for any non-zero rc, including negative values. ANSI X3.274-1996 §7.3.3 distinguishes:
If
command_handler_with_envreturnsSome(-3)(e.g., to signal "command not found" in the host environment), the ERROR trap fires when FAILURE should fire instead.fire_failure_trapis currently only reached when spawningshitself fails, not for negative codes from handlers.This is a pre-existing bug for
command_handleras well, but the new handler is the right place to fix both: split the post-handler check intorc < 0 → fire_failure_trapandrc > 0 → fire ERROR trap. There is also no test for SIGNAL ON FAILURE behavior from a custom handler returning a negative rc.Design concern —
set_addressrestriction is advisory-only (src/eval.rs:976)The docstring says handlers must not call
Environment::set_address, butset_addressis a public method and there is no enforcement. A handler that callsenv.set_address("SOMETHING")will silently corrupt ADDRESS tracking.The specific breakage: in
exec_address → AddressAction::Temporary(lines ~509–514), the evaluator capturessaved_defaultbefore callingexec_host_command, and restores it after. If the handler callsset_addressinternally, the restoreself.env.set_address(&saved_default)will setaddress_previousto the wrong value, corrupting the ADDRESS ring for all subsequentADDRESSinstructions.Because the new handler accepts
&mut Environment(unlike the basiccommand_handlerwhich cannot touch the environment at all), this is a new footgun introduced by this PR. One option is to accept a wrapper type that exposes only theset/getvariable operations.Minor — Panic-safety comment is misleading (
src/eval.rs:525–527)There is no
catch_unwindhere. If the handler panics, the stack unwinds immediately throughexec_host_commandand beyond; the empty slot is never visible to any subsequent code path within the same call chain. "Poisoned closure" is also not a Rust concept (poisoning applies tostd::sync::Mutex).The slot being empty is a correct and useful side effect of using
take()(necessary to break the borrow conflict), but characterizing it as an intentional safety mechanism overstates what the code actually guarantees. The type-alias doc on line 188–191 correctly states "the panic will propagate through the evaluator" — the two comments should be reconciled.PLACEHOLDER
@claude why the hell did you dump all the code into a PR message?
//! REXX tree-walking evaluator — AST + Environment -> execution.
//!
//! Walks the AST produced by the parser, evaluating expressions using
//!
RexxValueandBigDecimalarithmetic, and executing instructions//! against an
Environment.use bigdecimal::{BigDecimal, Zero};
use std::collections::{HashMap, VecDeque};
use std::fmt;
use std::path::Path;
use std::str::FromStr;
use crate::ast::{
AddressAction, AssignTarget, BinOp, Clause, ClauseKind, Condition, ControlledLoop, DoBlock,
DoKind, Expr, NumericFormSetting, NumericSetting, ParseSource, ParseTemplate, Program,
SignalAction, TailElement, TemplateElement, UnaryOp,
};
use crate::env::{EnvVars, Environment};
use crate::error::{RexxDiagnostic, RexxError, RexxResult};
use crate::lexer::Lexer;
use crate::parser::Parser;
use crate::value::{NumericSettings, RexxValue};
/// Maximum nesting depth for INTERPRET to prevent stack overflow.
const MAX_INTERPRET_DEPTH: usize = 100;
/// REXX trace levels, ordered from least to most verbose.
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
enum TraceLevel {
Off,
Normal,
Failure,
Errors,
Commands,
Labels,
Results,
Intermediates,
All,
}
impl TraceLevel {
/// Parse a single-letter or full-word trace level (case-insensitive).
fn parse(s: &str) -> Option {
let upper = s.trim().to_uppercase();
match upper.as_str() {
"O" | "OFF" => Some(Self::Off),
"N" | "NORMAL" => Some(Self::Normal),
"F" | "FAILURE" => Some(Self::Failure),
"E" | "ERRORS" => Some(Self::Errors),
"C" | "COMMANDS" => Some(Self::Commands),
"L" | "LABELS" => Some(Self::Labels),
"R" | "RESULTS" => Some(Self::Results),
"I" | "INTERMEDIATES" => Some(Self::Intermediates),
"A" | "ALL" => Some(Self::All),
_ => None,
}
}
}
/// Combined trace setting: level + interactive flag.
#[derive(Debug, Clone)]
struct TraceSetting {
level: TraceLevel,
interactive: bool,
}
/// Result of parsing a trace setting string.
enum TraceAction {
/// "?" alone — toggle interactive mode, keep current level.
ToggleInteractive,
/// A concrete setting (e.g., "R", "?R", "OFF").
Set(TraceSetting),
}
impl TraceAction {
/// Parse a trace setting string like "R", "?R", "?", "OFF", "?Results".
fn parse(s: &str) -> Option {
let trimmed = s.trim();
if trimmed.is_empty() {
return None;
}
if trimmed == "?" {
return Some(Self::ToggleInteractive);
}
if let Some(rest) = trimmed.strip_prefix('?') {
let level = TraceLevel::parse(rest)?;
Some(Self::Set(TraceSetting {
level,
interactive: true,
}))
} else {
let level = TraceLevel::parse(trimmed)?;
Some(Self::Set(TraceSetting {
level,
interactive: false,
}))
}
}
}
impl Default for TraceSetting {
fn default() -> Self {
Self {
level: TraceLevel::Normal,
interactive: false,
}
}
}
impl fmt::Display for TraceSetting {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
if self.interactive {
write!(f, "?{}", self.level.letter())
} else {
write!(f, "{}", self.level.letter())
}
}
}
/// Signal returned by clause/block execution for control flow.
pub enum ExecSignal {
Normal,
Leave(Option),
Iterate(Option),
Exit(Option),
Return(Option),
/// SIGNAL transfers control to a label, abandoning all blocks.
Signal(String),
}
/// Pending EXIT state — distinguishes "no pending exit" from "exit with/without value".
enum PendingExit {
None,
WithValue(Option),
}
impl PendingExit {
/// If an EXIT is pending, take the value and reset to
None./// Returns the exit value wrapped in
ExecSignal::Exitfor immediate propagation.fn take_signal(&mut self) -> Option {
match std::mem::replace(self, PendingExit::None) {
PendingExit::None => Option::None,
PendingExit::WithValue(v) => Some(ExecSignal::Exit(v)),
}
}
}
/// A custom command handler for ADDRESS environments.
///
/// Receives
(address_environment, command_string)and returnsSome(rc)if/// it handled the command, or
Noneto fall through to default shell execution.///
/// # Panics
///
/// The handler must not panic. If it does, the panic will propagate through
/// the evaluator. Handlers should handle all error cases internally and
/// return appropriate return codes.
type CommandHandler = Box<dyn FnMut(&str, &str) -> Option>;
/// A custom command handler that also receives an [
EnvVars] handle for/// reading and writing REXX variables.
///
/// Receives
(address_environment, command_string, vars)and returnsSome(rc)/// if it handled the command, or
Noneto fall through to default shell execution.///
/// This variant allows embedding applications to inspect and update REXX variables
/// (e.g., refreshing EXTRACT stem variables) during command execution. The
/// [
EnvVars] wrapper intentionally restricts access to variable operations only —/// ADDRESS routing and PROCEDURE scoping are not exposed.
///
/// # Panics
///
/// The handler must not panic. If it does, the panic will propagate through
/// the evaluator. Handlers should handle all error cases internally and
/// return appropriate return codes.
type CommandHandlerWithEnv = Box<dyn FnMut(&str, &str, &mut EnvVars<'_>) -> Option>;
pub struct Evaluator<'a> {
env: &'a mut Environment,
program: &'a Program,
settings: NumericSettings,
labels: HashMap<String, usize>,
arg_stack: Vec<Vec>,
pending_exit: PendingExit,
/// Active condition traps: condition → target label name.
traps: HashMap<Condition, String>,
/// Pending signal from a trap (e.g., NOVALUE). Fires after clause completes.
pending_signal: Option,
/// Current INTERPRET nesting depth (for recursion limit).
interpret_depth: usize,
/// Current external function call nesting depth (for recursion limit).
external_depth: usize,
/// Current TRACE setting (level + interactive flag).
trace_setting: TraceSetting,
/// External data queue for PUSH/QUEUE/PULL.
queue: VecDeque,
/// Optional custom command handler for ADDRESS environments.
/// Called as
handler(address_environment, command_string)and returns///
Some(rc)to handle the command orNoneto fall through to shell execution.command_handler: Option,
/// Optional custom command handler with
&mut Environmentaccess./// Tried before
command_handler; allows embedding applications to/// inspect and update REXX variables during command execution.
command_handler_with_env: Option,
}
impl<'a> Evaluator<'a> {
pub fn new(env: &'a mut Environment, program: &'a Program) -> Self {
let labels = Self::build_labels(program);
Self {
env,
program,
settings: NumericSettings::default(),
labels,
arg_stack: Vec::new(),
pending_exit: PendingExit::None,
traps: HashMap::new(),
pending_signal: None,
interpret_depth: 0,
external_depth: 0,
trace_setting: TraceSetting::default(),
queue: VecDeque::new(),
command_handler: None,
command_handler_with_env: None,
}
}
}
/// Convert a
RexxValueto a boolean, requiring it to be "0" or "1".fn to_logical(val: &RexxValue) -> RexxResult {
match val.as_str().trim() {
"0" => Ok(false),
"1" => Ok(true),
_ => Err(RexxDiagnostic::new(RexxError::InvalidLogicalValue)
.with_detail(format!("'{}' is not 0 or 1", val.as_str()))),
}
}
/// Produce a REXX boolean value: "1" for true, "0" for false.
fn bool_val(b: bool) -> RexxValue {
RexxValue::new(if b { "1" } else { "0" })
}
/// REXX normal comparison: strip leading/trailing blanks from both sides,
/// then if both are valid numbers, compare numerically;
/// otherwise pad the shorter with blanks and compare character-by-character.
fn normal_compare(left: &RexxValue, right: &RexxValue) -> std::cmp::Ordering {
let ls = left.as_str().trim();
let rs = right.as_str().trim();
}
/// REXX integer division: divide and truncate toward zero.
fn trunc_div(a: &BigDecimal, b: &BigDecimal) -> BigDecimal {
let quotient = a / b;
// RoundingMode::Down truncates toward zero (not toward negative infinity).
// Using with_scale_round avoids string-based truncation that breaks on
// scientific notation from BigDecimal::to_string().
quotient.with_scale_round(0, bigdecimal::RoundingMode::Down)
}
/// Compute base ** exp for
BigDecimalwith integer exponent.fn pow_bigdecimal(base: &BigDecimal, exp: i64) -> BigDecimal {
if exp == 0 {
return BigDecimal::from(1);
}
if exp < 0 {
let pos_result = pow_bigdecimal(base, -exp);
return BigDecimal::from(1) / pos_result;
}
let mut result = BigDecimal::from(1);
let mut b = base.clone();
let mut e = exp;
// Exponentiation by squaring
while e > 0 {
if e & 1 == 1 {
result *= &b;
}
b = &b * &b;
e >>= 1;
}
result
}
#[cfg(test)]
mod tests {
use super::*;
use crate::lexer::Lexer;
use crate::parser::Parser;
}
PR #25 Review -- CommandHandlerWithEnv + FAILURE/ERROR split
1. Undocumented breaking change to the existing set_command_handler API
File: src/eval.rs:567-582
The PR description covers only the new CommandHandlerWithEnv feature, but the diff also changes the error-dispatching logic for all command handlers (including the pre-existing set_command_handler):
Before this PR: a handler returning -1 (or any negative value) fired the ERROR condition.
After this PR: negative rc fires FAILURE (or is silent if no FAILURE trap is set).
This is the correct ANSI X3.274-1996 section 7.3.3 behaviour, but it is a silent breaking change for any embedder whose REXX programs use SIGNAL ON ERROR to catch commands that return negative codes. Those programs must now use SIGNAL ON FAILURE instead. The PR description should call this out explicitly.
2. Stale doc comment on set_command_handler
File: src/eval.rs:961-971
The doc comment says nothing about what happens for negative vs positive rc. Now that sign matters (negative -> FAILURE, positive -> ERROR), both set_command_handler and set_command_handler_with_env (eval.rs:976-978) should document the rc sign semantics explicitly.
3. Missing test: negative rc with no FAILURE trap active
File: src/eval.rs:2441-2461
The PR tests that FAILURE fires when SIGNAL ON FAILURE is set and the handler returns -3. There is no test for the case where the handler returns a negative value and no SIGNAL ON FAILURE is active. Per the standard, execution should continue normally with RC set to the negative value. Without this test, a regression that halts or errors on negative rc would go undetected.
Expected test shape:
4. self.env.address() allocated twice when both handlers are present and the first returns None
File: src/eval.rs:534, 543
When the env-aware handler is set and returns None, address().to_string() runs twice for the same (unchanged) value -- once inside the env-aware handler block (line 534), once inside the basic handler block (line 543). Hoisting the addr binding before both if blocks eliminates the duplicate without any borrow-checker complication (the address cannot change while either handler runs).
5. CommandHandlerWithEnv type alias is not pub
File: src/eval.rs:195
This matches the existing non-pub CommandHandler at line 177, so it is internally consistent. However, EnvVars is now a public type that appears in the public interface of set_command_handler_with_env. External crates cannot name CommandHandlerWithEnv and must write the full concrete Box type at every call site. Making at least CommandHandlerWithEnv pub would improve embedder ergonomics.
What looks good