Major rework of stack unwinding for exceptions

  * Clarify opcode names better
  * Re-arrange how catch arms are pushed and handled
  * Added new regression tests that verify
  * Overall a cleaner design
  * Fix logic in propagation around finally

crates/compiler/src/ast.rs

@@ -158,7 +158,7 @@ pub enum Expr {
         consequence: Box<Expr>,
         alternative: Box<Expr>,
     },
-    Catch {
+    TryCatch {
         trye: Box<Expr>,
         codes: CatchCodes,
         except: Option<Box<Expr>>,

crates/compiler/src/codegen.rs

@@ -294,17 +294,18 @@ impl CodegenState {
         Ok(())
     }
 
-    fn generate_codes(&mut self, codes: &CatchCodes) -> Result<(), CompileError> {
+    fn generate_codes(&mut self, codes: &CatchCodes) -> Result<usize, CompileError> {
         match codes {
             CatchCodes::Codes(codes) => {
                 self.generate_arg_list(codes)?;
+                Ok(codes.len())
             }
             CatchCodes::Any => {
                 self.emit(Op::ImmInt(0));
                 self.push_stack(1);
+                Ok(1)
             }
         }
-        Ok(())
     }
 
     fn generate_expr(&mut self, expr: &Expr) -> Result<(), CompileError> {
@@ -450,19 +451,19 @@ impl CodegenState {
                 self.generate_expr(alternative.as_ref())?;
                 self.commit_jump_label(end_label);
             }
-            Expr::Catch {
+            Expr::TryCatch {
                 codes,
                 except,
                 trye,
             } => {
-                self.generate_codes(codes)?;
                 let handler_label = self.make_jump_label(None);
-                self.emit(Op::PushLabel(handler_label));
-                self.emit(Op::Catch(handler_label));
+                self.generate_codes(codes)?;
+                self.emit(Op::PushCatchLabel(handler_label));
+                self.pop_stack(1)   /* codes, catch */;
+                self.emit(Op::TryCatch { handler_label });
                 self.generate_expr(trye.as_ref())?;
                 let end_label = self.make_jump_label(None);
                 self.emit(Op::EndCatch(end_label));
-                self.pop_stack(1)   /* codes, catch */;
                 self.commit_jump_label(handler_label);
 
                 /* After this label, we still have a value on the stack, but now,
@@ -631,20 +632,20 @@ impl CodegenState {
             }
             StmtNode::TryExcept { body, excepts } => {
                 let mut labels = vec![];
+                let num_excepts = excepts.len();
                 for ex in excepts {
                     self.generate_codes(&ex.codes)?;
                     let push_label = self.make_jump_label(None);
-                    self.emit(Op::PushLabel(push_label));
+                    self.emit(Op::PushCatchLabel(push_label));
                     labels.push(push_label);
                 }
-                let num_excepts = excepts.len();
+                self.pop_stack(num_excepts);
                 self.emit(Op::TryExcept { num_excepts });
                 for stmt in body {
                     self.generate_stmt(stmt)?;
                 }
                 let end_label = self.make_jump_label(None);
                 self.emit(Op::EndExcept(end_label));
-                self.pop_stack(num_excepts);
                 for (i, ex) in excepts.iter().enumerate() {
                     self.commit_jump_label(labels[i]);
                     self.push_stack(1);
@@ -664,7 +665,9 @@ impl CodegenState {
             }
             StmtNode::TryFinally { body, handler } => {
                 let handler_label = self.make_jump_label(None);
-                self.emit(Op::TryFinally(handler_label));
+                self.emit(Op::TryFinally {
+                    end_label: handler_label,
+                });
                 for stmt in body {
                     self.generate_stmt(stmt)?;
                 }
@@ -674,7 +677,7 @@ impl CodegenState {
                 for stmt in handler {
                     self.generate_stmt(stmt)?;
                 }
-                self.emit(Op::Continue);
+                self.emit(Op::FinallyContinue);
                 self.pop_stack(2);
             }
             StmtNode::Break { exit: None } => {

crates/compiler/src/codegen_tests.rs

@@ -774,15 +774,17 @@ mod tests {
         assert_eq!(
             *binary.main_vector.as_ref(),
             vec![
-                TryFinally(0.into()),
+                TryFinally {
+                    end_label: 0.into()
+                },
                 ImmInt(1),
                 Put(a),
                 Pop,
                 EndFinally,
                 ImmInt(2),
                 Put(a),
                 Pop,
-                Continue,
+                FinallyContinue,
                 Done
             ]
         );
@@ -826,10 +828,10 @@ mod tests {
             vec![
                 ImmErr(E_INVARG),
                 MakeSingletonList,
-                PushLabel(0.into()),
+                PushCatchLabel(0.into()),
                 ImmErr(E_PROPNF),
                 MakeSingletonList,
-                PushLabel(1.into()),
+                PushCatchLabel(1.into()),
                 TryExcept { num_excepts: 2 },
                 ImmInt(1),
                 Put(a),
@@ -881,8 +883,10 @@ mod tests {
                 MakeSingletonList,
                 ImmErr(E_PERM),
                 ListAddTail,
-                PushLabel(0.into()),
-                Catch(0.into()),
+                PushCatchLabel(0.into()),
+                TryCatch {
+                    handler_label: 0.into(),
+                },
                 Push(x),
                 ImmInt(1),
                 Add,
@@ -921,8 +925,10 @@ mod tests {
             *binary.main_vector.as_ref(),
             vec![
                 ImmInt(0),
-                PushLabel(0.into()),
-                Catch(0.into()),
+                PushCatchLabel(0.into()),
+                TryCatch {
+                    handler_label: 0.into(),
+                },
                 ImmErr(E_INVARG),
                 MakeSingletonList,
                 FuncCall {
@@ -1445,12 +1451,12 @@ mod tests {
             vec![
                 ImmErr(E_RANGE),
                 MakeSingletonList,
-                PushLabel(Label(0)),
+                PushCatchLabel(Label(0)),
                 TryExcept { num_excepts: 1 },
                 Imm(Label(0)),
                 ImmInt(2),
                 // Our offset is different because we don't count PushLabel in the stack.
-                Length(Offset(1)),
+                Length(Offset(0)),
                 RangeRef,
                 Return,
                 EndExcept(Label(1)),
@@ -1484,8 +1490,10 @@ mod tests {
             vec![
                 ImmErr(E_INVIND),
                 MakeSingletonList,
-                PushLabel(0.into()),
-                Catch(0.into()),
+                PushCatchLabel(0.into()),
+                TryCatch {
+                    handler_label: 0.into(),
+                },
                 Push(this),
                 EndCatch(1.into()),
                 ImmInt(1),

crates/compiler/src/decompile.rs

@@ -631,7 +631,7 @@ impl Decompile {
                 let e = self.pop_expr()?;
                 self.push_expr(Expr::Scatter(scatter_items, Box::new(e)));
             }
-            Op::PushLabel(_) => {
+            Op::PushCatchLabel(_) => {
                 // ignore and consume, we don't need it.
             }
             Op::TryExcept { num_excepts } => {
@@ -707,16 +707,18 @@ impl Decompile {
                     line_num,
                 ));
             }
-            Op::TryFinally(_label) => {
+            Op::TryFinally { end_label: _label } => {
                 // decompile body up until the EndFinally
                 let (body, _) =
                     self.decompile_statements_until_match(|_, op| matches!(op, Op::EndFinally))?;
-                let (handler, _) =
-                    self.decompile_statements_until_match(|_, op| matches!(op, Op::Continue))?;
+                let (handler, _) = self
+                    .decompile_statements_until_match(|_, op| matches!(op, Op::FinallyContinue))?;
                 self.statements
                     .push(Stmt::new(StmtNode::TryFinally { body, handler }, line_num));
             }
-            Op::Catch(label) => {
+            Op::TryCatch {
+                handler_label: label,
+            } => {
                 let codes_expr = self.pop_expr()?;
                 let catch_codes = match codes_expr {
                     Expr::Value(_) => CatchCodes::Any,
@@ -761,7 +763,7 @@ impl Decompile {
                         ));
                     }
                 };
-                self.push_expr(Expr::Catch {
+                self.push_expr(Expr::TryCatch {
                     trye: Box::new(try_expr),
                     codes: catch_codes,
                     except,
@@ -810,7 +812,7 @@ impl Decompile {
             Op::Jump { .. } | Op::PushTemp => {
                 unreachable!("should have been handled other decompilation branches")
             }
-            Op::EndCatch(_) | Op::Continue | Op::EndExcept(_) | Op::EndFinally => {
+            Op::EndCatch(_) | Op::FinallyContinue | Op::EndExcept(_) | Op::EndFinally => {
                 // Early exit; main logic is in TRY_FINALLY or CATCH etc case, above
                 // TODO: MOO has "return ptr - 2;"  -- doing something with the iteration, that
                 //   I may not be able to do with the current structure. See if I need to

crates/compiler/src/opcode.rs

@@ -23,9 +23,8 @@ pub enum Op {
     Add,
     And(Label),
     CallVerb,
-    Catch(Label),
     CheckListForSplice,
-    Continue,
+    FinallyContinue,
     Div,
     Done,
     Eif(Label),
@@ -70,7 +69,6 @@ pub enum Op {
     Pop,
     Push(Name),
     PushGetProp,
-    PushLabel(Label),
     PushRef,
     PushTemp,
     Put(Name),
@@ -83,8 +81,10 @@ pub enum Op {
     Return0,
     Scatter(Box<ScatterArgs>),
     Sub,
+    PushCatchLabel(Label),
+    TryCatch { handler_label: Label },
     TryExcept { num_excepts: usize },
-    TryFinally(Label),
+    TryFinally { end_label: Label },
     UnaryMinus,
     While(Label),
     WhileId { id: Name, end_label: Label },

crates/compiler/src/parse.rs

@@ -283,7 +283,7 @@ fn parse_expr(
                 let except = inner
                     .next()
                     .map(|e| Box::new(parse_expr(names.clone(), e.into_inner()).unwrap()));
-                Ok(Expr::Catch {
+                Ok(Expr::TryCatch {
                     trye: Box::new(try_expr),
                     codes: catch_codes,
                     except,
@@ -1895,7 +1895,7 @@ mod tests {
         assert_eq!(
             stripped_stmts(&parse.stmts),
             vec![StmtNode::Return(Some(Expr::List(vec![Normal(
-                Expr::Catch {
+                Expr::TryCatch {
                     trye: Box::new(Id(parse.names.find_name("x").unwrap())),
                     codes: CatchCodes::Codes(vec![varnf]),
                     except: Some(Box::new(Value(v_int(666)))),
@@ -1912,7 +1912,7 @@ mod tests {
 
         assert_eq!(
             stripped_stmts(&parse.stmts),
-            vec![StmtNode::Expr(Expr::Catch {
+            vec![StmtNode::Expr(Expr::TryCatch {
                 trye: Box::new(Call {
                     function: Symbol::mk("raise"),
                     args: vec![invarg]
@@ -1930,7 +1930,7 @@ mod tests {
 
         assert_eq!(
             stripped_stmts(&parse.stmts),
-            vec![StmtNode::Expr(Expr::Catch {
+            vec![StmtNode::Expr(Expr::TryCatch {
                 trye: Box::new(Verb {
                     location: Box::new(Prop {
                         location: Box::new(Value(v_obj(0))),

crates/compiler/src/unparse.rs

@@ -73,7 +73,7 @@ impl Expr {
             Expr::Pass { .. } => 1,
             Expr::Call { .. } => 1,
             Expr::Length => 1,
-            Expr::Catch { .. } => 1,
+            Expr::TryCatch { .. } => 1,
         };
         15 - cpp_ref_prep
     }
@@ -237,7 +237,7 @@ impl Unparse {
                 self.unparse_expr(consequence)?,
                 brace_if_lower_eq(alternative)
             )),
-            Expr::Catch {
+            Expr::TryCatch {
                 trye,
                 codes,
                 except,

crates/kernel/src/builtins/bf_server.rs

@@ -204,7 +204,7 @@ fn bf_connected_seconds(bf_args: &mut BfCallState<'_>) -> Result<BfRet, BfErr> {
         return Err(BfErr::Code(E_TYPE));
     };
     let Ok(connected_seconds) = bf_args.session.connected_seconds(*who) else {
-        return Err(BfErr::Code(E_ARGS));
+        return Err(BfErr::Code(E_INVARG));
     };
 
     Ok(Ret(v_int(connected_seconds as i64)))

crates/kernel/src/tasks/mod.rs

@@ -132,9 +132,9 @@ pub mod vm_test_utils {
     use crate::tasks::sessions::Session;
     use crate::tasks::vm_host::{VMHostResponse, VmHost};
     use crate::tasks::VerbCall;
-    use moor_values::tasks::UncaughtException;
+    use moor_values::tasks::Exception;
 
-    pub type ExecResult = Result<Var, UncaughtException>;
+    pub type ExecResult = Result<Var, Exception>;
 
     fn execute<F>(
         world_state: &mut dyn WorldState,
@@ -244,12 +244,12 @@ pub mod scheduler_test_utils {
 
     use crate::tasks::scheduler_client::SchedulerClient;
     use crate::tasks::sessions::Session;
+    use moor_values::tasks::Exception;
     use moor_values::tasks::SchedulerError::{CommandExecutionError, TaskAbortedException};
-    use moor_values::tasks::UncaughtException;
 
     use super::TaskHandle;
 
-    pub type ExecResult = Result<Var, UncaughtException>;
+    pub type ExecResult = Result<Var, Exception>;
 
     fn execute<F>(fun: F) -> Result<Var, SchedulerError>
     where
@@ -269,7 +269,7 @@ pub mod scheduler_test_utils {
         {
             // Some errors can be represented as a MOO `Var`; translate those to a `Var`, so that
             // `moot` tests can match against them.
-            Err(TaskAbortedException(UncaughtException { code, .. })) => Ok(code.into()),
+            Err(TaskAbortedException(Exception { code, .. })) => Ok(code.into()),
             Err(CommandExecutionError(CommandError::NoCommandMatch)) => Ok(E_VERBNF.into()),
             Err(err) => Err(err),
             Ok(var) => Ok(var),

crates/kernel/src/tasks/task_scheduler_client.rs

@@ -17,9 +17,7 @@ use std::time::Instant;
 use crossbeam_channel::Sender;
 
 use moor_values::model::Perms;
-use moor_values::tasks::{
-    AbortLimitReason, CommandError, NarrativeEvent, TaskId, UncaughtException,
-};
+use moor_values::tasks::{AbortLimitReason, CommandError, Exception, NarrativeEvent, TaskId};
 use moor_values::var::Objid;
 use moor_values::var::Symbol;
 use moor_values::var::Var;
@@ -74,7 +72,7 @@ impl TaskSchedulerClient {
     }
 
     /// Send a message to the scheduler that an exception was thrown while executing the verb.
-    pub fn exception(&self, exception: UncaughtException) {
+    pub fn exception(&self, exception: Exception) {
         self.scheduler_sender
             .send((self.task_id, TaskControlMsg::TaskException(exception)))
             .expect("Could not deliver client message -- scheduler shut down?");
@@ -222,7 +220,7 @@ pub enum TaskControlMsg {
     /// The verb to be executed was not found.
     TaskVerbNotFound(Objid, Symbol),
     /// An exception was thrown while executing the verb.
-    TaskException(UncaughtException),
+    TaskException(Exception),
     /// The task is requesting that it be forked.
     TaskRequestFork(Fork, oneshot::Sender<TaskId>),
     /// The task is letting us know it was cancelled.