Merge pull request #77 from chriseth/structured_error

More structured errors.

src/analyzer/pil_analyzer.rs

@@ -5,7 +5,7 @@ use std::path::{Path, PathBuf};
 use crate::number::{abstract_to_degree, DegreeType};
 use crate::parser::ast;
 pub use crate::parser::ast::{BinaryOperator, UnaryOperator};
-use crate::{line_utils, parser};
+use crate::{parser, utils};
 
 use super::*;
 
@@ -100,7 +100,7 @@ impl PILContext {
         let old_line_starts = std::mem::take(&mut self.line_starts);
 
         // TOOD make this work for other line endings
-        self.line_starts = line_utils::compute_line_starts(contents);
+        self.line_starts = utils::compute_line_starts(contents);
         self.current_file = path.to_path_buf();
         let pil_file =
             parser::parse(Some(path.to_str().unwrap()), contents).unwrap_or_else(|err| {
@@ -181,7 +181,7 @@ impl PILContext {
     fn to_source_ref(&self, start: usize) -> SourceRef {
         let file = self.current_file.file_name().unwrap().to_str().unwrap();
         SourceRef {
-            line: line_utils::offset_to_line(start, &self.line_starts),
+            line: utils::offset_to_line(start, &self.line_starts),
             file: file.to_string(),
         }
     }

src/commit_evaluator/eval_error.rs

@@ -0,0 +1,59 @@
+use std::fmt::{Display, Formatter, Result};
+
+#[derive(Clone, Debug)]
+pub enum EvalError {
+    /// Previous value of witness column not known when trying to derive a value in the next row.
+    PreviousValueUnknown(String),
+    Generic(String),
+    Multiple(Vec<EvalError>),
+}
+
+pub fn combine(left: EvalError, right: EvalError) -> EvalError {
+    match (left, right) {
+        (EvalError::Multiple(l), EvalError::Multiple(r)) => {
+            EvalError::Multiple(l.into_iter().chain(r).collect())
+        }
+        (m @ EvalError::Multiple(_), other) | (other, m @ EvalError::Multiple(_)) => {
+            combine(m, EvalError::Multiple(vec![other]))
+        }
+        (l, r) => EvalError::Multiple(vec![l, r]),
+    }
+}
+
+impl From<String> for EvalError {
+    fn from(value: String) -> Self {
+        EvalError::Generic(value)
+    }
+}
+
+impl Display for EvalError {
+    fn fmt(&self, f: &mut Formatter<'_>) -> Result {
+        match self {
+            EvalError::Generic(reason) => write!(f, "{reason}"),
+            EvalError::PreviousValueUnknown(names) => write!(
+                f,
+                "Previous value of the following column(s) is not (yet) known: {names}.",
+            ),
+            EvalError::Multiple(errors) => {
+                let (previous_unknown, mut others) = errors.iter().fold(
+                    (vec![], vec![]),
+                    |(mut previous_unknown, mut others), err| {
+                        if let EvalError::PreviousValueUnknown(n) = err {
+                            previous_unknown.push(n.clone());
+                        } else {
+                            others.push(format!("{err}"));
+                        }
+                        (previous_unknown, others)
+                    },
+                );
+                if !previous_unknown.is_empty() {
+                    others.push(format!(
+                        "{}",
+                        EvalError::PreviousValueUnknown(previous_unknown.join(", "))
+                    ));
+                }
+                write!(f, "{}", others.join("\n"))
+            }
+        }
+    }
+}

src/commit_evaluator/mod.rs

@@ -6,11 +6,15 @@ use crate::analyzer::{
 };
 // TODO should use finite field instead of abstract number
 use crate::number::{abstract_to_degree, is_zero, AbstractNumberType, DegreeType};
+use crate::utils::indent;
 
 mod affine_expression;
+mod eval_error;
 
 use affine_expression::AffineExpression;
 
+use self::eval_error::EvalError;
+
 /// Generates the committed polynomial values
 /// @returns the values (in source order) and the degree of the polynomials.
 pub fn generate<'a>(
@@ -69,7 +73,7 @@ impl<'a> WitnessColumn<'a> {
     }
 }
 
-type EvalResult = Result<Vec<(usize, AbstractNumberType)>, String>;
+type EvalResult = Result<Vec<(usize, AbstractNumberType)>, EvalError>;
 
 struct Evaluator<'a, QueryCallback>
 where
@@ -101,9 +105,6 @@ enum EvaluationRow {
     Specific(DegreeType),
 }
 
-// This could have more structure in the future.
-type EvalError = String;
-
 impl<'a, QueryCallback> Evaluator<'a, QueryCallback>
 where
     QueryCallback: FnMut(&str) -> Option<AbstractNumberType>,
@@ -160,7 +161,13 @@ where
                     IdentityKind::Plookup => self.process_plookup(identity),
                     _ => Ok(vec![]),
                 }
-                .map_err(|err| format!("No progress on {identity}:\n    {err}"));
+                .map_err(|err| {
+                    format!(
+                        "No progress on {identity}:\n{}",
+                        indent(&format!("{err}"), "    ")
+                    )
+                    .into()
+                });
                 if result.is_err() {
                     identity_failed = true;
                 }
@@ -228,7 +235,7 @@ where
     fn process_witness_query(
         &mut self,
         column: &&WitnessColumn,
-    ) -> Result<Vec<(usize, AbstractNumberType)>, String> {
+    ) -> Result<Vec<(usize, AbstractNumberType)>, EvalError> {
         let query = self.interpolate_query(column.query.unwrap())?;
         if let Some(value) = self.query_callback.as_mut().unwrap()(&query) {
             Ok(vec![(column.id, value)])
@@ -236,7 +243,8 @@ where
             Err(format!(
                 "No query answer for {} query: {query}.",
                 self.committed_names[column.id]
-            ))
+            )
+            .into())
         }
     }
 
@@ -282,9 +290,9 @@ where
                 None => {
                     let formatted = self.format_affine_expression(&evaluated);
                     Err(if evaluated.is_invalid() {
-                        format!("Constraint is invalid ({formatted} != 0).")
+                        format!("Constraint is invalid ({formatted} != 0).").into()
                     } else {
-                        format!("Could not solve expression {formatted} = 0.")
+                        format!("Could not solve expression {formatted} = 0.").into()
                     })
                 }
             }
@@ -293,7 +301,7 @@ where
 
     fn process_plookup(&mut self, identity: &Identity) -> EvalResult {
         if identity.left.selector.is_some() || identity.right.selector.is_some() {
-            return Err("Selectors not yet supported.".to_string());
+            return Err("Selectors not yet supported.".to_string().into());
         }
         let left = identity
             .left
@@ -314,7 +322,8 @@ where
             None => Err(format!(
                 "First expression needs to be constant but is not: {}.",
                 self.format_affine_expression(&v)
-            )),
+            )
+            .into()),
         })?;
 
         let right_key = identity.right.expressions.first().unwrap();
@@ -354,7 +363,7 @@ where
             }
         }
         if result.is_empty() {
-            Err(reasons.join(", "))
+            Err(reasons.into_iter().reduce(eval_error::combine).unwrap())
         } else {
             Ok(result)
         }
@@ -374,6 +383,7 @@ where
                         "Constant value required: {}",
                         self.format_affine_expression(&r)
                     )
+                    .into()
                 })
             })?;
 
@@ -391,9 +401,9 @@ where
                     // TODO somehow also add `l` and `r` to the error message.
                     let formatted = self.format_affine_expression(&evaluated);
                     Err(if evaluated.is_invalid() {
-                        format!("Constraint is invalid ({formatted} != 0).")
+                        format!("Constraint is invalid ({formatted} != 0).").into()
                     } else {
-                        format!("Could not solve expression {formatted} = 0.")
+                        format!("Could not solve expression {formatted} = 0.").into()
                     })
                 }
             },
@@ -410,7 +420,7 @@ where
                 }
             }
             Err(reason) => {
-                self.failure_reasons.push(reason);
+                self.failure_reasons.push(format!("{reason}"));
             }
         }
     }
@@ -441,9 +451,7 @@ where
                         self.current[*id]
                             .as_ref()
                             .map(|value| value.clone().into())
-                            .ok_or_else(|| {
-                                format!("Previous value of column {} not yet known.", poly.name)
-                            })
+                            .ok_or_else(|| EvalError::PreviousValueUnknown(poly.name.clone()))
                     } else if (poly.next && row == EvaluationRow::Current)
                         || (!poly.next && row == EvaluationRow::Next)
                     {
@@ -459,7 +467,8 @@ where
                         Err(format!(
                             "{}' references the next-next row when evaluating on the current row.",
                             self.committed_names[*id]
-                        ))
+                        )
+                        .into())
                     }
                 } else {
                     // Constant polynomial (or something else)
@@ -481,13 +490,19 @@ where
                 self.evaluate_binary_operation(left, op, right, row)
             }
             Expression::UnaryOperation(op, expr) => self.evaluate_unary_operation(op, expr, row),
-            Expression::Tuple(_) => Err("Tuple not implemented.".to_string()),
-            Expression::String(_) => Err("String not implemented.".to_string()),
+            Expression::Tuple(_) => Err("Tuple not implemented.".to_string().into()),
+            Expression::String(_) => Err("String not implemented.".to_string().into()),
             Expression::LocalVariableReference(_) => {
-                Err("Local variable references not implemented.".to_string())
+                Err("Local variable references not implemented."
+                    .to_string()
+                    .into())
+            }
+            Expression::PublicReference(_) => {
+                Err("Public references not implemented.".to_string().into())
+            }
+            Expression::FunctionCall(_, _) => {
+                Err("Function calls not implemented.".to_string().into())
             }
-            Expression::PublicReference(_) => Err("Public references not implemented.".to_string()),
-            Expression::FunctionCall(_, _) => Err("Function calls not implemented.".to_string()),
         }
     }
 
@@ -512,7 +527,8 @@ where
                             "Multiplication of two non-constants: ({}) * ({})",
                             self.format_affine_expression(&left),
                             self.format_affine_expression(&right)
-                        ))
+                        )
+                        .into())
                     }
                 }
                 BinaryOperator::Div => {
@@ -529,7 +545,8 @@ where
                             "Division of two non-constants: ({}) / ({})",
                             self.format_affine_expression(&left),
                             self.format_affine_expression(&right)
-                        ))
+                        )
+                        .into())
                     }
                 }
                 BinaryOperator::Pow => {
@@ -540,7 +557,8 @@ where
                             "Pow of two non-constants: ({}) ** ({})",
                             self.format_affine_expression(&left),
                             self.format_affine_expression(&right)
-                        ))
+                        )
+                        .into())
                     }
                 }
                 BinaryOperator::Mod
@@ -566,7 +584,7 @@ where
                 }
             },
             (Ok(_), Err(reason)) | (Err(reason), Ok(_)) => Err(reason),
-            (Err(r1), Err(r2)) => Err(format!("{r1}, {r2}")),
+            (Err(r1), Err(r2)) => Err(eval_error::combine(r1, r2)),
         }
     }
 

src/lib.rs

@@ -4,6 +4,6 @@ pub mod commit_evaluator;
 pub mod compiler;
 pub mod constant_evaluator;
 pub mod json_exporter;
-pub mod line_utils;
 pub mod number;
 pub mod parser;
+pub mod utils;

src/parser/display.rs

@@ -1,5 +1,7 @@
 use std::fmt::{Display, Formatter, Result};
 
+use crate::utils::quote;
+
 use super::ast::*;
 
 // TODO indentation
@@ -78,10 +80,6 @@ impl Display for Statement {
     }
 }
 
-fn quote(input: &str) -> String {
-    format!("\"{}\"", input.replace('\\', "\\\\").replace('"', "\\\""))
-}
-
 fn format_names(names: &[PolynomialName]) -> String {
     names
         .iter()

src/utils.rs

@@ -11,6 +11,18 @@ pub fn offset_to_line(offset: usize, line_starts: &[usize]) -> usize {
     }
 }
 
+pub fn quote(input: &str) -> String {
+    format!("\"{}\"", input.replace('\\', "\\\\").replace('"', "\\\""))
+}
+
+pub fn indent(input: &str, indentation: &str) -> String {
+    if input.is_empty() {
+        String::new()
+    } else {
+        indentation.to_string() + &input.replace('\n', &format!("\n{indentation}"))
+    }
+}
+
 #[cfg(test)]
 mod test {
     use super::{compute_line_starts, offset_to_line};