fix: an error that crosses except or finally keeps the line it was raised at

Author: dylan-sutton-chavez

src/parser/control.rs

@@ -540,6 +540,8 @@ impl<'src, I: Iterator<Item = Token>> Parser<'src, I> {
                 break;
             }
 
+            // The header runs code of its own, so an error in its type reports its line.
+            self.mark_stmt();
             let mut as_name: Option<String> = None;
             if matches!(self.peek(), Some(TokenType::Colon)) {
                 had_bare = true;

src/parser/stmt.rs

@@ -23,12 +23,16 @@ enum TupleElem {
 
 impl<'src, I: Iterator<Item = Token>> Parser<'src, I> {
 
-    /* Statement dispatch, returns true if a value is left on the stack for caller to PopTop. */
-    pub(super) fn stmt(&mut self) -> bool {
-        // Record ip->source offset before any emit so `resolve()` can map ip to source.
+    /* Records ip->source offset before any emit, so `resolve()` maps what follows to the next token. */
+    pub(super) fn mark_stmt(&mut self) {
         let ip = self.chunk.instructions.len() as u32;
         let pos = self.tokens.peek().map(|t| t.start as u32).unwrap_or(self.last_end as u32);
         self.chunk.stmt_pos.push((ip, pos));
+    }
+
+    /* Statement dispatch, returns true if a value is left on the stack for caller to PopTop. */
+    pub(super) fn stmt(&mut self) -> bool {
+        self.mark_stmt();
 
         match self.peek() {
             Some(TokenType::If) => {

src/value/frames.rs

@@ -72,7 +72,8 @@ pub enum Unwind {
     Return(Val),
     // break/continue, run `remaining` more cleanups, then resume at `target`.
     Goto { target: usize, remaining: u16 },
-    Reraise(VmErr),
+    // An exception passing through, with the byte offset it was raised at.
+    Reraise(VmErr, Option<u32>),
 }
 
 /* Saved stack/iter/with/unwind depths for unwinding to a handler. Stored on the active Coroutine so `try`/`except` survives yields. */

src/vm/dispatch.rs

@@ -228,7 +228,7 @@ impl<'a> VM<'a> {
                             self.call_stack.clear();
                             match self.next_cleanup_handler(exc_base) {
                                 Some(h) => {
-                                    self.unwind_stack.push(Unwind::Reraise(e));
+                                    self.unwind_stack.push(Unwind::Reraise(e, None));
                                     ip = h;
                                     continue;
                                 }
@@ -260,20 +260,22 @@ impl<'a> VM<'a> {
                                 let msg_val = alloc(&mut self.heap, HeapObj::Str(e.message()))?;
                                 alloc(&mut self.heap, HeapObj::ExcInstance(msg, alloc::vec![msg_val]))?
                             };
-                            self.error_byte_pos = None;
+                            // Kept so raising the exception again reports where it was first raised.
+                            let at = self.error_byte_pos.take();
                             // Drop reasons from finally bodies this exception unwinds past.
                             self.unwind_stack.truncate(frame.unwind_depth);
                             match frame.kind {
                                 BlockKind::Except => {
                                     // Record the handled exc so a bare `raise` in the handler can re-raise it.
                                     self.handling_exc = Some(exc);
+                                    self.handling_pos = at;
                                     self.push(exc);
                                     ip = frame.handler_ip;
                                 }
                                 // finally/with run their cleanup, then re-raise via EndFinally.
                                 BlockKind::Finally => {
                                     self.pending.exc_val = Some(exc);
-                                    self.unwind_stack.push(Unwind::Reraise(e));
+                                    self.unwind_stack.push(Unwind::Reraise(e, at));
                                     ip = frame.handler_ip;
                                 }
                             }
@@ -699,7 +701,7 @@ impl<'a> VM<'a> {
                 let cm = self.with_stack.pop().ok_or(cold_runtime("WithExit without matching WithEnter"))?;
                 let (exit_fn, class) = self.with_dunder(cm, "__exit__")?;
                 // Reraise selects `__exit__(type, exc, None)`, other exits pass three Nones.
-                let (exc_type, exc) = if matches!(self.unwind_stack.last(), Some(Unwind::Reraise(_))) {
+                let (exc_type, exc) = if matches!(self.unwind_stack.last(), Some(Unwind::Reraise(..))) {
                     let exc = self.pending.exc_val.unwrap_or(Val::none());
                     let exc_name = self.exc_type_name(exc);
                     (self.heap.alloc(HeapObj::Type(exc_name))?, exc)
@@ -716,7 +718,7 @@ impl<'a> VM<'a> {
             OpCode::WithJudge => {
                 let r = self.pop()?;
                 // A truthy `__exit__` suppresses the exception, but never a cancel.
-                if !self.cancelling && matches!(self.unwind_stack.last(), Some(Unwind::Reraise(_))) && self.truthy(r) {
+                if !self.cancelling && matches!(self.unwind_stack.last(), Some(Unwind::Reraise(..))) && self.truthy(r) {
                     // Suppress, turn the re-raise into a normal exit and drop the exc identity.
                     if let Some(top) = self.unwind_stack.last_mut() { *top = Unwind::Normal; }
                     self.pending.exc_val = None;
@@ -742,7 +744,10 @@ impl<'a> VM<'a> {
                             *ip = target;
                         }
                     }
-                    Some(Unwind::Reraise(e)) => return Err(e),
+                    Some(Unwind::Reraise(e, at)) => {
+                        self.error_byte_pos = at;
+                        return Err(e);
+                    }
                 }
             }
             // break/continue across N finally/with blocks, the following Jump completes the transfer.

src/vm/mod.rs

@@ -120,6 +120,8 @@ pub struct VM<'a> {
     pub(crate) unwind_stack: Vec<types::Unwind>,
     /* Exception currently being handled in an except block, a bare `raise` re-raises it. */
     pub(crate) handling_exc: Option<Val>,
+    // Where the handled exception was raised, so re-raising it reports that line.
+    pub(crate) handling_pos: Option<u32>,
     pub(crate) functions: Vec<&'a (Vec<String>, SSAChunk, u16, u16)>,
     // (chunk_ptr, global fn ids), linear scan over a tiny list avoids HashMap monomorphization.
     pub(crate) fn_index: Vec<(*const SSAChunk, Vec<u32>)>,
@@ -269,6 +271,7 @@ impl<'a> VM<'a> {
             exception_stack: Vec::new(),
             unwind_stack: Vec::new(),
             handling_exc: None,
+            handling_pos: None,
             error_byte_pos: None,
             module_table: HashMap::default(),
             fn_module: Vec::new(),
@@ -527,6 +530,7 @@ impl<'a> VM<'a> {
         self.pending_sync_frames.clear();
         self.executing_coros.clear();
         self.handling_exc = None;
+        self.handling_pos = None;
         self.cancelling = false;
         self.resume_raise = None;
         self.yielded = false;

src/vm/opcodes/stack.rs

@@ -163,11 +163,16 @@ impl<'a> VM<'a> {
                     };
                     let name = self.exc_type_name(exc);
                     self.pending.exc_val = Some(exc);
+                    self.error_byte_pos = self.handling_pos;
                     return Err(VmErr::Raised(name));
                 }
                 // RaiseFrom emits both `expr` then `from expr`, the topmost value is the cause, but the exception to raise is the LHS.
                 if op == OpCode::RaiseFrom { let _cause = self.pop()?; }
                 let mut exc = self.pop()?;
+                // The exception being handled raised again keeps the line it was first raised at.
+                if Some(exc) == self.handling_exc {
+                    self.error_byte_pos = self.handling_pos;
+                }
                 // A class deriving from an exception raises an instance of itself made with no arguments.
                 if matches!(self.heap.try_get(exc), Some(HeapObj::Class(..))) && self.exc_base(exc).is_some() {
                     self.push(exc);

src/vm/snapshot.rs

@@ -249,7 +249,7 @@ codec!(enum Unwind, put_unwind, get_unwind {
     0 Normal,
     1 Return(v: val),
     2 Goto { target: usz, remaining: u16 },
-    3 Reraise(e: (put_vm_err, get_vm_err)),
+    3 Reraise(e: (put_vm_err, get_vm_err), at: opt_u32),
 });
 
 codec!(enum WaitKind, put_wait_kind, get_wait_kind {
@@ -504,6 +504,7 @@ vm_state! {
     exception_stack: [put_exc_frame, get_exc_frame],
     unwind_stack: [put_unwind, get_unwind],
     handling_exc: opt_val,
+    handling_pos: opt_u32,
     pending_sync_frames: [put_sync_frame, get_sync_frame],
     pending_exec_exc_base: opt_usz,
     pending: (put_pending, get_pending),

tests/cases/vm.json

@@ -5199,5 +5199,15 @@
   {"src": "x = 3.25\nprint(id(x) == id(x), id(None) == id(None), id(True) != id(1), id(0) != id(False))", "output": ["True True True True"]},
   {"src": "class C:\n    pass\nobjs = [C() for _ in range(5)]\nprint(len({id(o) for o in objs}), all((id(p) == id(q)) == (p is q) for p in objs for q in objs))", "output": ["5 True"]},
   {"src": "n = float('nan')\nm = float('nan')\nprint(id(n) == id(n), id(n) != id(m), (id(n) == id(m)) == (n is m))", "output": ["True True True"]},
-  {"src": "big = 10 ** 30\nneg = -10 ** 30\nprint(id(big) != id(neg), (id(big) == id(big)), id(big) >= 0, id(neg) >= 0)", "output": ["True True True True"]}
+  {"src": "big = 10 ** 30\nneg = -10 ** 30\nprint(id(big) != id(neg), (id(big) == id(big)), id(big) >= 0, id(neg) >= 0)", "output": ["True True True True"]},
+  {"src": "try:\n    x = 1 / 0\nexcept NoExiste:\n    pass\n", "output": [], "error": "NameError: name 'NoExiste' is not defined", "error_line": 3},
+  {"src": "try:\n    x = 1 / 0\nfinally:\n    pass\n", "output": [], "error": "ZeroDivisionError: division by zero", "error_line": 2},
+  {"src": "try:\n    x = 1 / 0\nexcept KeyError:\n    pass\n", "output": [], "error": "ZeroDivisionError: division by zero", "error_line": 2},
+  {"src": "try:\n    x = 1 / 0\nexcept ZeroDivisionError:\n    raise\n", "output": [], "error": "ZeroDivisionError", "error_line": 2},
+  {"src": "try:\n    x = 1 / 0\nexcept ZeroDivisionError as e:\n    raise e\n", "output": [], "error": "ZeroDivisionError: division by zero", "error_line": 2},
+  {"src": "try:\n    x = 1 / 0\nfinally:\n    int('x')\n", "output": [], "error": "ValueError", "error_line": 4},
+  {"src": "try:\n    x = 0\nexcept ValueError:\n    pass\nelse:\n    y = 1 / x\n", "output": [], "error": "ZeroDivisionError: division by zero", "error_line": 6},
+  {"src": "try:\n    x = 1 / 0\nexcept ValueError:\n    pass\nexcept Nope as e:\n    pass\n", "output": [], "error": "NameError: name 'Nope' is not defined", "error_line": 5},
+  {"src": "class CM:\n    def __enter__(self):\n        return self\n    def __exit__(self, t, v, tb):\n        return False\nwith CM():\n    x = 1 / 0\n", "output": [], "error": "ZeroDivisionError: division by zero", "error_line": 7},
+  {"src": "def f():\n    return 1 / 0\ntry:\n    f()\nfinally:\n    pass\n", "output": [], "error": "ZeroDivisionError: division by zero", "error_line": 2}
 ]

tests/snapshot.rs

@@ -42,6 +42,9 @@ mod test {
         // Present means send() reaches a scheduler, this corpus only needs it accepted.
         #[serde(default)]
         sends: Option<Vec<(String, String)>>,
+        // The line the vm suite checks an error at, which a replay leaves alone.
+        #[serde(default, rename = "error_line")]
+        _error_line: Option<usize>,
     }
 
     fn accept_send(_group: &str, _body: &str) -> bool {

tests/vm.rs

@@ -24,6 +24,9 @@ mod test {
         // Present installs a scheduler hook, the (group, body) pairs send() handed over.
         #[serde(default)]
         sends: Option<Vec<(String, String)>>,
+        // The line the error points at, counted from 1.
+        #[serde(default)]
+        error_line: Option<usize>,
     }
 
     std::thread_local! {
@@ -127,9 +130,15 @@ mod test {
                     }
                 }
                 Err(e) => match &case.error {
-                    Some(expected) => if !e.to_string().contains(expected.as_str()) {
-                        failures.push(format!("ERR {:?}\n   got '{}'\n   want '{}'", case.src, e, expected));
-                    },
+                    Some(expected) => {
+                        if !e.to_string().contains(expected.as_str()) {
+                            failures.push(format!("ERR {:?}\n   got '{}'\n   want '{}'", case.src, e, expected));
+                        }
+                        let line = vm.error_pos().map(|at| case.src[..at.min(case.src.len())].matches('\n').count() + 1);
+                        if let Some(want) = case.error_line && line != Some(want) {
+                            failures.push(format!("LINE {:?}\n   got {:?}\n   want {}", case.src, line, want));
+                        }
+                    }
                     None => failures.push(format!("RAISED {:?}: {}", case.src, e)),
                 }
             }