refactor: Not allowing 'zero' Position

At the cost of more verbose propagation of the current `Position`,
removing the `at_no_pos`, `patch_err_pos`, `with_err_no_pos`
and related methods.

The code should not be constructing "zero" Position objects,
hoping that they will be filled-in later.

Removed method `try_map` of `Positioned` as it was used only once.

Removed `ErrorEnvelope::NoPos` from interpreter

Enforcing that `Position` cannot have zero row or col.

Removed `Position::zero` method.

Author: ngeor

rusty_basic/src/error_envelope.rs

@@ -1,70 +1,36 @@
 use rusty_common::{HasPos, Position};
 
-//
-// ErrorEnvelope
-//
-
+/// ErrorEnvelope wraps an error with stacktrace information.
 #[derive(Clone, Debug, PartialEq, Eq)]
-pub enum ErrorEnvelope<T> {
-    NoPos(T),
-    Pos(T, Position),
-    Stacktrace(T, Vec<Position>),
-}
-
-impl<T> AsRef<T> for ErrorEnvelope<T> {
-    fn as_ref(&self) -> &T {
-        match self {
-            Self::NoPos(t) | Self::Pos(t, _) | Self::Stacktrace(t, _) => t,
-        }
-    }
-}
+pub struct ErrorEnvelope<T>(T, Vec<Position>);
 
 impl<T> ErrorEnvelope<T> {
-    pub fn into_err(self) -> T {
-        match self {
-            Self::NoPos(t) | Self::Pos(t, _) | Self::Stacktrace(t, _) => t,
-        }
+    pub fn new(err: T, pos: Position) -> Self {
+        Self(err, vec![pos])
     }
 
-    /// Patches the envelope with the given position.
-    /// If the object already has a position or a stacktrace,
-    /// it is returned as-is.
-    pub fn patch_pos(self, pos: Position) -> Self {
-        match self {
-            Self::NoPos(t) => Self::Pos(t, pos),
-            _ => self,
-        }
+    pub fn new_with_stacktrace(err: T, stacktrace: Vec<Position>) -> Self {
+        Self(err, stacktrace)
     }
 
-    pub fn patch_stacktrace(self, v_new: &mut Vec<Position>) -> Self {
-        let mut v_old: Vec<Position> = match &self {
-            Self::NoPos(_) => vec![],
-            Self::Pos(_, p) => vec![*p],
-            Self::Stacktrace(_, v) => v.clone(),
-        };
-        v_old.append(v_new);
-        let body = self.into_err();
-        if v_old.is_empty() {
-            Self::NoPos(body)
-        } else if v_old.len() == 1 {
-            Self::Pos(body, v_old.pop().unwrap())
+    pub fn new_draining_stacktrace(err: T, stacktrace: &mut Vec<Position>) -> Self {
+        debug_assert!(!stacktrace.is_empty());
+        if stacktrace.len() == 1 {
+            Self::new(err, stacktrace.pop().unwrap())
         } else {
-            Self::Stacktrace(body, v_old)
+            let new_stacktrace = stacktrace.drain(0..stacktrace.len()).collect();
+            Self(err, new_stacktrace)
         }
     }
-}
 
-//
-// result.with_err_no_pos()
-//
-
-pub trait WithErrNoPos<TResult> {
-    fn with_err_no_pos(self) -> TResult;
-}
+    pub fn err(&self) -> &T {
+        &self.0
+    }
 
-impl<T, E> WithErrNoPos<Result<T, ErrorEnvelope<E>>> for Result<T, E> {
-    fn with_err_no_pos(self) -> Result<T, ErrorEnvelope<E>> {
-        self.map_err(|e| ErrorEnvelope::NoPos(e))
+    pub fn appen_draining_stacktrace(self, stacktrace: &mut Vec<Position>) -> Self {
+        let Self(err, mut old_stacktrace) = self;
+        old_stacktrace.append(stacktrace);
+        Self(err, old_stacktrace)
     }
 }
 
@@ -78,20 +44,24 @@ pub trait WithErrAt<Pos, TResult> {
 
 impl<Pos: HasPos, T, E> WithErrAt<&Pos, Result<T, ErrorEnvelope<E>>> for Result<T, E> {
     fn with_err_at(self, p: &Pos) -> Result<T, ErrorEnvelope<E>> {
-        self.map_err(|e| ErrorEnvelope::Pos(e, p.pos()))
+        self.map_err(|e| ErrorEnvelope::new(e, p.pos()))
     }
 }
 
-//
-// result.patch_err_pos()
-//
+pub trait WithStacktrace<O = Self> {
+    /// Adds the given stacktrace to the error.
+    /// The given stacktrace is emptied, to avoid adding the same items twice.
+    fn with_stacktrace(self, stacktrace: &mut Vec<Position>) -> O;
+}
 
-pub trait PatchErrPos<Pos, TResult> {
-    fn patch_err_pos(self, p: Pos) -> TResult;
+impl<T> WithStacktrace for ErrorEnvelope<T> {
+    fn with_stacktrace(self, stacktrace: &mut Vec<Position>) -> Self {
+        self.appen_draining_stacktrace(stacktrace)
+    }
 }
 
-impl<Pos: HasPos, T, E> PatchErrPos<&Pos, Self> for Result<T, ErrorEnvelope<E>> {
-    fn patch_err_pos(self, p: &Pos) -> Self {
-        self.map_err(|e| e.patch_pos(p.pos()))
+impl<T, E> WithStacktrace<Result<T, ErrorEnvelope<E>>> for Result<T, E> {
+    fn with_stacktrace(self, stacktrace: &mut Vec<Position>) -> Result<T, ErrorEnvelope<E>> {
+        self.map_err(|err| ErrorEnvelope::new_draining_stacktrace(err, stacktrace))
     }
 }

rusty_basic/src/interpreter/built_ins/instr.rs

@@ -66,7 +66,7 @@ mod tests {
         assert_prints!(r#"PRINT INSTR("", "")"#, "0");
         assert_eq!(
             interpret_err(r#"PRINT INSTR(0, "oops", "zero")"#),
-            ErrorEnvelope::Pos(RuntimeError::IllegalFunctionCall, Position::new(1, 7))
+            ErrorEnvelope::new(RuntimeError::IllegalFunctionCall, Position::new(1, 7))
         );
     }
 }

rusty_basic/src/interpreter/built_ins/kill.rs

@@ -24,7 +24,7 @@ mod tests {
     fn test_kill_edge_cases() {
         assert_eq!(
             interpret_err(r#"KILL "KILL2.TXT""#),
-            ErrorEnvelope::Pos(RuntimeError::FileNotFound, Position::new(1, 1))
+            ErrorEnvelope::new(RuntimeError::FileNotFound, Position::new(1, 1))
         );
     }
 }

rusty_basic/src/interpreter/interpreter.rs

@@ -1,5 +1,5 @@
 use super::handlers::{cast, comparison, logical, math, registers, subprogram, var_path};
-use crate::error_envelope::{WithErrAt, WithErrNoPos};
+use crate::error_envelope::WithErrAt;
 use crate::instruction_generator::{Instruction, InstructionGeneratorResult, Path, PrinterType};
 use crate::interpreter::arguments::ArgumentInfo;
 use crate::interpreter::context::*;
@@ -17,7 +17,7 @@ use crate::interpreter::registers::{RegisterStack, Registers};
 use crate::interpreter::screen::{CrossTermScreen, Screen};
 use crate::interpreter::write_printer::WritePrinter;
 use crate::interpreter::Stdlib;
-use crate::{RuntimeError, RuntimeErrorPos};
+use crate::{RuntimeError, RuntimeErrorPos, WithStacktrace};
 use rusty_common::*;
 use rusty_linter::{HasUserDefinedTypes, QBNumberCast};
 use rusty_parser::UserDefinedTypes;
@@ -215,7 +215,7 @@ impl<TStdlib: Stdlib, TStdIn: Input, TStdOut: Printer, TLpt1: Printer, U: HasUse
                     }
                 },
                 Err(e) => {
-                    self.last_error_code = Some(e.as_ref().get_code());
+                    self.last_error_code = Some(e.err().get_code());
                     match ctx.error_handler {
                         ErrorHandler::Address(handler_address) => {
                             // store error address, so we can call RESUME and RESUME NEXT from within the error handler
@@ -227,7 +227,7 @@ impl<TStdlib: Stdlib, TStdIn: Input, TStdOut: Printer, TLpt1: Printer, U: HasUse
                             i = ctx.nearest_statement_finder.find_next(i);
                         }
                         ErrorHandler::None => {
-                            return Err(e.patch_stacktrace(&mut self.stacktrace));
+                            return Err(e.with_stacktrace(&mut self.stacktrace));
                         }
                     }
                 }
@@ -433,12 +433,14 @@ impl<TStdlib: Stdlib, TStdIn: Input, TStdOut: Printer, TLpt1: Printer, U: HasUse
                 subprogram::push_a_to_named_arg(self, param_name);
             }
             Instruction::BuiltInSub(s) => {
-                // note: not patching the error pos for built-ins because it's already in-place by Instruction::PushStack
-                super::built_ins::run_sub(s, self).with_err_no_pos()?;
+                // the stacktrace should be already populated by Instruction::PushStack
+                debug_assert!(!self.stacktrace.is_empty());
+                super::built_ins::run_sub(s, self).with_stacktrace(&mut self.stacktrace)?;
             }
             Instruction::BuiltInFunction(f) => {
-                // note: not patching the error pos for built-ins because it's already in-place by Instruction::PushStack
-                super::built_ins::run_function(f, self).with_err_no_pos()?;
+                // the stacktrace should be already populated by Instruction::PushStack
+                debug_assert!(!self.stacktrace.is_empty());
+                super::built_ins::run_function(f, self).with_stacktrace(&mut self.stacktrace)?;
             }
             Instruction::Label(_) => (), // no-op
             Instruction::Halt => {

rusty_basic/src/interpreter/test_utils.rs

@@ -246,7 +246,7 @@ macro_rules! assert_interpreter_err {
     ($program:expr, $expected_err:expr, $expected_row:expr, $expected_col:expr) => {
         assert_eq!(
             $crate::interpreter::test_utils::interpret_err($program),
-            $crate::ErrorEnvelope::Pos(
+            $crate::ErrorEnvelope::new(
                 $expected_err,
                 rusty_common::Position::new($expected_row, $expected_col)
             )

rusty_basic/src/interpreter/tests/loops.rs

@@ -75,7 +75,7 @@ fn test_for_loop_with_zero_step() {
     ";
     assert_eq!(
         interpret_err(input),
-        ErrorEnvelope::Pos(RuntimeError::ForLoopZeroStep, Position::new(2, 27))
+        ErrorEnvelope::new(RuntimeError::ForLoopZeroStep, Position::new(2, 27))
     );
 }
 

rusty_basic/src/interpreter/tests/on_error.rs

@@ -46,7 +46,7 @@ fn reset_error_handler() {
     let err = result.unwrap_err();
     assert_eq!(
         err,
-        RuntimeErrorPos::Pos(RuntimeError::DivisionByZero, Position::new(5, 13))
+        RuntimeErrorPos::new(RuntimeError::DivisionByZero, Position::new(5, 13))
     );
     assert_eq!(interpreter.stdout().output(), "oops");
 }

rusty_basic/src/interpreter/tests/sub_call.rs

@@ -106,7 +106,7 @@ fn test_stacktrace() {
     "#;
     assert_eq!(
         interpret_err(program),
-        ErrorEnvelope::Stacktrace(
+        ErrorEnvelope::new_with_stacktrace(
             RuntimeError::Other("Invalid expression. Must be name=value.".to_string()),
             vec![
                 Position::new(10, 13), // "inside" Environ

rusty_common/src/position.rs

@@ -7,6 +7,8 @@ pub struct Position {
 
 impl Position {
     pub fn new(row: u32, col: u32) -> Self {
+        debug_assert!(row > 0);
+        debug_assert!(col > 0);
         Self { row, col }
     }
 
@@ -23,10 +25,6 @@ impl Position {
         Self::new(1, 1)
     }
 
-    pub fn zero() -> Self {
-        Self::new(0, 0)
-    }
-
     pub fn row(&self) -> u32 {
         self.row
     }

rusty_common/src/positioned.rs

@@ -4,7 +4,6 @@ use crate::Position;
 /// within the file it was read from.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct Positioned<T> {
-    // TODO make fields private
     pub element: T,
     pub pos: Position,
 }
@@ -29,16 +28,6 @@ impl<T> Positioned<T> {
             pos,
         }
     }
-
-    /// Converts this instance to a result, given the specified function.
-    /// The function acts on the element of this instance and returns
-    /// the result. The error of the result is converted to a [Positioned] error.
-    pub fn try_map<F, U, E>(&self, op: F) -> Result<U, Positioned<E>>
-    where
-        F: FnOnce(&T) -> Result<U, E>,
-    {
-        (op)(&self.element).map_err(|e| e.at_pos(self.pos))
-    }
 }
 
 // AtPos
@@ -59,10 +48,6 @@ pub trait AtPos: Sized {
     fn at_start(self) -> Positioned<Self> {
         self.at_pos(Position::start())
     }
-
-    fn at_no_pos(self) -> Positioned<Self> {
-        self.at_pos(Position::zero())
-    }
 }
 
 impl<T> AtPos for T {}
@@ -94,7 +79,7 @@ impl<T> HasPos for Box<Positioned<T>> {
 }
 
 //
-// NoPosIter
+// NoPosIter TODO: the following are used only in tests
 //
 
 pub struct NoPosIter<'a, T: 'a, I>