Remove unwraps from binary expression algorithm (#992)

src/lang/errors.ts

@@ -25,6 +25,13 @@ export class KCLLexicalError extends KCLError {
   }
 }
 
+export class KCLInternalError extends KCLError {
+  constructor(msg: string, sourceRanges: [number, number][]) {
+    super('internal', msg, sourceRanges)
+    Object.setPrototypeOf(this, KCLSyntaxError.prototype)
+  }
+}
+
 export class KCLSyntaxError extends KCLError {
   constructor(msg: string, sourceRanges: [number, number][]) {
     super('syntax', msg, sourceRanges)

src/wasm-lib/kcl/src/errors.rs

@@ -28,6 +28,8 @@ pub enum KclError {
     InvalidExpression(KclErrorDetails),
     #[error("engine: {0:?}")]
     Engine(KclErrorDetails),
+    #[error("internal error, please report to KittyCAD team: {0:?}")]
+    Internal(KclErrorDetails),
 }
 
 #[derive(Debug, Serialize, Deserialize, ts_rs::TS, Clone)]
@@ -42,21 +44,8 @@ pub struct KclErrorDetails {
 impl KclError {
     /// Get the error message, line and column from the error and input code.
     pub fn get_message_line_column(&self, input: &str) -> (String, Option<usize>, Option<usize>) {
-        let (type_, source_range, message) = match &self {
-            KclError::Lexical(e) => ("lexical", e.source_ranges.clone(), e.message.clone()),
-            KclError::Syntax(e) => ("syntax", e.source_ranges.clone(), e.message.clone()),
-            KclError::Semantic(e) => ("semantic", e.source_ranges.clone(), e.message.clone()),
-            KclError::Type(e) => ("type", e.source_ranges.clone(), e.message.clone()),
-            KclError::Unimplemented(e) => ("unimplemented", e.source_ranges.clone(), e.message.clone()),
-            KclError::Unexpected(e) => ("unexpected", e.source_ranges.clone(), e.message.clone()),
-            KclError::ValueAlreadyDefined(e) => ("value already defined", e.source_ranges.clone(), e.message.clone()),
-            KclError::UndefinedValue(e) => ("undefined value", e.source_ranges.clone(), e.message.clone()),
-            KclError::InvalidExpression(e) => ("invalid expression", e.source_ranges.clone(), e.message.clone()),
-            KclError::Engine(e) => ("engine", e.source_ranges.clone(), e.message.clone()),
-        };
-
         // Calculate the line and column of the error from the source range.
-        let (line, column) = if let Some(range) = source_range.first() {
+        let (line, column) = if let Some(range) = self.source_ranges().first() {
             let line = input[..range.0[0]].lines().count();
             let column = input[..range.0[0]].lines().last().map(|l| l.len()).unwrap_or_default();
 
@@ -65,7 +54,23 @@ impl KclError {
             (None, None)
         };
 
-        (format!("{}: {}", type_, message), line, column)
+        (format!("{}: {}", self.error_type(), self.message()), line, column)
+    }
+
+    pub fn error_type(&self) -> &'static str {
+        match self {
+            KclError::Lexical(_) => "lexical",
+            KclError::Syntax(_) => "syntax",
+            KclError::Semantic(_) => "semantic",
+            KclError::Type(_) => "type",
+            KclError::Unimplemented(_) => "unimplemented",
+            KclError::Unexpected(_) => "unexpected",
+            KclError::ValueAlreadyDefined(_) => "value already defined",
+            KclError::UndefinedValue(_) => "undefined value",
+            KclError::InvalidExpression(_) => "invalid expression",
+            KclError::Engine(_) => "engine",
+            KclError::Internal(_) => "internal",
+        }
     }
 
     pub fn source_ranges(&self) -> Vec<SourceRange> {
@@ -80,6 +85,7 @@ impl KclError {
             KclError::UndefinedValue(e) => e.source_ranges.clone(),
             KclError::InvalidExpression(e) => e.source_ranges.clone(),
             KclError::Engine(e) => e.source_ranges.clone(),
+            KclError::Internal(e) => e.source_ranges.clone(),
         }
     }
 
@@ -96,6 +102,7 @@ impl KclError {
             KclError::UndefinedValue(e) => &e.message,
             KclError::InvalidExpression(e) => &e.message,
             KclError::Engine(e) => &e.message,
+            KclError::Internal(e) => &e.message,
         }
     }
 

src/wasm-lib/kcl/src/parser/math.rs

@@ -1,20 +1,33 @@
-use crate::ast::types::{BinaryExpression, BinaryOperator, BinaryPart};
+use crate::{
+    ast::types::{BinaryExpression, BinaryOperator, BinaryPart},
+    errors::{KclError, KclErrorDetails},
+    executor::SourceRange,
+};
 
 /// Parses a list of tokens (in infix order, i.e. as the user typed them)
 /// into a binary expression tree.
-pub fn parse(infix_tokens: Vec<BinaryExpressionToken>) -> BinaryExpression {
+pub fn parse(infix_tokens: Vec<BinaryExpressionToken>) -> Result<BinaryExpression, KclError> {
     let rpn = postfix(infix_tokens);
     evaluate(rpn)
 }
 
 /// Parses a list of tokens (in postfix order) into a binary expression tree.
-fn evaluate(rpn: Vec<BinaryExpressionToken>) -> BinaryExpression {
+fn evaluate(rpn: Vec<BinaryExpressionToken>) -> Result<BinaryExpression, KclError> {
-    let mut operand_stack = Vec::new();
+    let source_ranges = source_range(&rpn);
+    let mut operand_stack: Vec<BinaryPart> = Vec::new();
+    let e = KclError::Internal(KclErrorDetails {
+        source_ranges,
+        message: "error parsing binary math expressions".to_owned(),
+    });
     for item in rpn {
         let expr = match item {
             BinaryExpressionToken::Operator(operator) => {
-                let right: BinaryPart = operand_stack.pop().unwrap();
+                let Some(right) = operand_stack.pop() else {
-                let left = operand_stack.pop().unwrap();
+                    return Err(e);
+                };
+                let Some(left) = operand_stack.pop() else {
+                    return Err(e);
+                };
                 BinaryPart::BinaryExpression(Box::new(BinaryExpression {
                     start: left.start(),
                     end: right.end(),
@@ -27,10 +40,26 @@ fn evaluate(rpn: Vec<BinaryExpressionToken>) -> BinaryExpression {
         };
         operand_stack.push(expr)
     }
-    if let BinaryPart::BinaryExpression(expr) = operand_stack.pop().unwrap() {
+    if let Some(BinaryPart::BinaryExpression(expr)) = operand_stack.pop() {
-        *expr
+        Ok(*expr)
     } else {
-        panic!("Last expression was not a binary expression")
+        // If this branch is used, the evaluation algorithm has a bug and must be fixed.
+        // This is a programmer error, not a user error.
+        Err(e)
+    }
+}
+
+fn source_range(tokens: &[BinaryExpressionToken]) -> Vec<SourceRange> {
+    let sources: Vec<_> = tokens
+        .iter()
+        .filter_map(|op| match op {
+            BinaryExpressionToken::Operator(_) => None,
+            BinaryExpressionToken::Operand(o) => Some((o.start(), o.end())),
+        })
+        .collect();
+    match (sources.first(), sources.last()) {
+        (Some((start, _)), Some((_, end))) => vec![SourceRange([*start, *end])],
+        _ => Vec::new(),
     }
 }
 
@@ -46,15 +75,16 @@ fn postfix(infix: Vec<BinaryExpressionToken>) -> Vec<BinaryExpressionToken> {
                 //     there is an operator o2 at the top of the operator stack which is not a left parenthesis,
                 //     and (o2 has greater precedence than o1 or (o1 and o2 have the same precedence and o1 is left-associative))
                 // )
-                while operator_stack
+                // pop o2 from the operator stack into the output queue
-                    .last()
+                while let Some(o2) = operator_stack.pop() {
-                    .map(|o2| {
+                    if (o2.precedence() > o1.precedence())
-                        (o2.precedence() > o1.precedence())
+                        || o1.precedence() == o2.precedence() && o1.associativity().is_left()
-                            || o1.precedence() == o2.precedence() && o1.associativity().is_left()
+                    {
-                    })
+                        output.push(BinaryExpressionToken::Operator(o2));
-                    .unwrap_or(false)
+                    } else {
-                {
+                        operator_stack.push(o2);
-                    output.push(BinaryExpressionToken::Operator(operator_stack.pop().unwrap()));
+                        break;
+                    }
                 }
                 operator_stack.push(o1);
             }
@@ -127,8 +157,7 @@ mod tests {
         ];
         for infix_input in tests {
             let rpn = postfix(infix_input);
-            let tree = evaluate(rpn);
+            let _tree = evaluate(rpn).unwrap();
-            dbg!(tree);
         }
     }
 }

src/wasm-lib/kcl/src/parser/parser_impl.rs

@@ -1039,7 +1039,8 @@ fn binary_expression(i: TokenSlice) -> PResult<BinaryExpression> {
 
     // Pass the token slice into the specialized math parser, for things like
     // precedence and converting infix operations to an AST.
-    Ok(super::math::parse(tokens))
+    let expr = super::math::parse(tokens).map_err(|e| ErrMode::Backtrack(e.into()))?;
+    Ok(expr)
 }
 
 fn binary_expr_in_parens(i: TokenSlice) -> PResult<BinaryExpression> {