Report errors in path canonicalizer (powdr-labs#1401)

importer/Cargo.toml

@@ -11,6 +11,7 @@ repository = { workspace = true }
 powdr-ast.workspace = true
 powdr-number.workspace = true
 powdr-parser.workspace = true
+powdr-parser-util.workspace = true
 
 pretty_assertions = "1.4.0"
 

importer/src/lib.rs

@@ -10,14 +10,16 @@ pub use module_loader::load_module_files;
 use path_canonicalizer::canonicalize_paths;
 use powdr_ast::parsed::asm::ASMProgram;
 use powdr_parser::parse_asm;
+use powdr_parser_util::{Error, SourceRef};
 use powdr_std::add_std;
 
 pub fn load_dependencies_and_resolve(
     path: Option<PathBuf>,
     module: ASMProgram,
-) -> Result<ASMProgram, String> {
+) -> Result<ASMProgram, Error> {
     load_module_files(path, module)
         .and_then(add_std)
+        .map_err(|e| SourceRef::default().with_error(e))
         .and_then(canonicalize_paths)
 }
 

importer/src/path_canonicalizer.rs

@@ -16,12 +16,14 @@ use powdr_ast::parsed::{
     visitor::{Children, ExpressionVisitable},
     ArrayLiteral, BinaryOperation, BlockExpression, EnumDeclaration, EnumVariant, Expression,
     FunctionCall, IndexAccess, LambdaExpression, LetStatementInsideBlock, MatchArm,
-    MatchExpression, Pattern, PilStatement, StatementInsideBlock, TypedExpression, UnaryOperation,
+    MatchExpression, Pattern, PilStatement, SourceReference, StatementInsideBlock, TypedExpression,
+    UnaryOperation,
 };
+use powdr_parser_util::{Error, SourceRef};
 
 /// Changes all symbol references (symbol paths) from relative paths
 /// to absolute paths, and removes all import statements.
-pub fn canonicalize_paths(program: ASMProgram) -> Result<ASMProgram, String> {
+pub fn canonicalize_paths(program: ASMProgram) -> Result<ASMProgram, Error> {
     let paths = &generate_path_map(&program)?;
 
     let mut canonicalizer = Canonicalizer {
@@ -498,7 +500,7 @@ fn check_import(
     check_path(location.join(imported.path), state)
 }
 
-fn generate_path_map(program: &ASMProgram) -> Result<PathMap, String> {
+fn generate_path_map(program: &ASMProgram) -> Result<PathMap, Error> {
     // an empty state starting from this module
     let mut state = State {
         root: &program.main,
@@ -522,15 +524,19 @@ fn check_module(
     location: AbsoluteSymbolPath,
     module: &ASMModule,
     state: &mut State<'_>,
-) -> Result<(), String> {
-    module.symbol_definitions().try_fold(
-        BTreeSet::default(),
-        |mut acc, SymbolDefinition { name, .. }| {
-            acc.insert(name.clone())
-                .then_some(acc)
-                .ok_or(format!("Duplicate name `{name}` in module `{location}`"))
-        },
-    )?;
+) -> Result<(), Error> {
+    module
+        .symbol_definitions()
+        .try_fold(
+            BTreeSet::default(),
+            |mut acc, SymbolDefinition { name, .. }| {
+                // TODO we should store source refs in symbol definitions.
+                acc.insert(name.clone())
+                    .then_some(acc)
+                    .ok_or(format!("Duplicate name `{name}` in module `{location}`"))
+            },
+        )
+        .map_err(|e| SourceRef::default().with_error(e))?;
 
     for SymbolDefinition { name, value } in module.symbol_definitions() {
         // start with the initial state
@@ -546,15 +552,18 @@ fn check_module(
                 };
                 check_module(location.with_part(name), m, state)?;
             }
-            SymbolValue::Import(s) => check_import(location.clone(), s.clone(), state)?,
+            SymbolValue::Import(s) => check_import(location.clone(), s.clone(), state)
+                .map_err(|e| SourceRef::default().with_error(e))?,
             SymbolValue::Expression(TypedExpression { e, type_scheme }) => {
                 if let Some(type_scheme) = type_scheme {
-                    check_type_scheme(&location, type_scheme, state, &Default::default())?;
+                    check_type_scheme(&location, type_scheme, state, &Default::default())
+                        .map_err(|err| e.source_reference().with_error(err))?;
                 }
                 check_expression(&location, e, state, &HashSet::default())?
             }
             SymbolValue::TypeDeclaration(enum_decl) => {
-                check_type_declaration(&location, enum_decl, state)?
+                check_type_declaration(&location, enum_decl, state)
+                    .map_err(|e| SourceRef::default().with_error(e))?
             }
         }
     }
@@ -570,30 +579,34 @@ fn check_machine(
     location: AbsoluteSymbolPath,
     m: &Machine,
     state: &mut State<'_>,
-) -> Result<(), String> {
+) -> Result<(), Error> {
     // we check the path in the context of the parent module
     let module_location = location.clone().parent();
 
     // Find all local variables.
     let mut local_variables = HashSet::<String>::default();
     for name in m.local_names() {
         if !local_variables.insert(name.clone()) {
-            return Err(format!("Duplicate name `{name}` in machine `{location}`"));
+            // TODO local_names could also return a source ref.
+            return Err(SourceRef::default()
+                .with_error(format!("Duplicate name `{name}` in machine `{location}`")));
         }
     }
     for statement in &m.statements {
         match statement {
-            MachineStatement::Submachine(_, path, _) => {
-                check_path(module_location.clone().join(path.clone()), state)?
+            MachineStatement::Submachine(source_ref, path, _) => {
+                check_path(module_location.clone().join(path.clone()), state)
+                    .map_err(|e| source_ref.with_error(e))?
             }
             MachineStatement::FunctionDeclaration(_, _, _, statements) => statements
                 .iter()
                 .flat_map(|s| s.children())
                 .flat_map(free_inputs_in_expression)
                 .try_for_each(|e| check_expression(&module_location, e, state, &local_variables))?,
             MachineStatement::Pil(_, statement) => {
-                if let PilStatement::LetStatement(_, _, Some(type_scheme), _) = statement {
-                    check_type_scheme(&module_location, type_scheme, state, &local_variables)?;
+                if let PilStatement::LetStatement(source_ref, _, Some(type_scheme), _) = statement {
+                    check_type_scheme(&module_location, type_scheme, state, &local_variables)
+                        .map_err(|e| source_ref.with_error(e))?;
                 }
                 statement.children().try_for_each(|e| {
                     check_expression(&module_location, e, state, &local_variables)
@@ -644,17 +657,18 @@ fn check_expression(
     e: &Expression,
     state: &mut State<'_>,
     local_variables: &HashSet<String>,
-) -> Result<(), String> {
+) -> Result<(), Error> {
     // We cannot use the visitor here because we need to change the local variables
     // inside lambda expressions.
     match e {
-        Expression::Reference(_, reference) => {
+        Expression::Reference(source_ref, reference) => {
             if let Some(name) = reference.try_to_identifier() {
                 if local_variables.contains(name) {
                     return Ok(());
                 }
             }
             check_path_try_prelude(location.clone(), reference.path.clone(), state)
+                .map_err(|e| source_ref.with_error(e))
         }
         Expression::PublicReference(_, _) | Expression::Number(_, _) | Expression::String(_, _) => {
             Ok(())
@@ -663,7 +677,7 @@ fn check_expression(
             check_expressions(location, items, state, local_variables)
         }
         Expression::LambdaExpression(
-            _,
+            source_ref,
             LambdaExpression {
                 kind: _,
                 params,
@@ -672,7 +686,9 @@ fn check_expression(
         ) => {
             // Add the local variables, ignore collisions.
             let mut local_variables = local_variables.clone();
-            local_variables.extend(check_patterns(location, params, state)?);
+            local_variables.extend(
+                check_patterns(location, params, state).map_err(|e| source_ref.with_error(e))?,
+            );
             check_expression(location, body, state, &local_variables)
         }
         Expression::BinaryOperation(
@@ -699,11 +715,14 @@ fn check_expression(
             check_expression(location, function, state, local_variables)?;
             check_expressions(location, arguments, state, local_variables)
         }
-        Expression::MatchExpression(_, MatchExpression { scrutinee, arms }) => {
+        Expression::MatchExpression(source_ref, MatchExpression { scrutinee, arms }) => {
             check_expression(location, scrutinee, state, local_variables)?;
             arms.iter().try_for_each(|MatchArm { pattern, value }| {
                 let mut local_variables = local_variables.clone();
-                local_variables.extend(check_pattern(location, pattern, state)?);
+                local_variables.extend(
+                    check_pattern(location, pattern, state)
+                        .map_err(|e| source_ref.with_error(e))?,
+                );
                 check_expression(location, value, state, &local_variables)
             })
         }
@@ -719,7 +738,7 @@ fn check_expression(
             check_expression(location, body, state, local_variables)?;
             check_expression(location, else_body, state, local_variables)
         }
-        Expression::BlockExpression(_, BlockExpression { statements, expr }) => {
+        Expression::BlockExpression(source_ref, BlockExpression { statements, expr }) => {
             let mut local_variables = local_variables.clone();
             for statement in statements {
                 match statement {
@@ -730,7 +749,11 @@ fn check_expression(
                         if let Some(value) = value {
                             check_expression(location, value, state, &local_variables)?;
                         }
-                        local_variables.extend(check_pattern(location, pattern, state)?);
+                        // TODO we need a much more fine-grained source ref here.
+                        local_variables.extend(
+                            check_pattern(location, pattern, state)
+                                .map_err(|e| source_ref.with_error(e))?,
+                        );
                     }
                     StatementInsideBlock::Expression(expr) => {
                         check_expression(location, expr, state, &local_variables)?;
@@ -747,7 +770,7 @@ fn check_expressions(
     expressions: &[Expression],
     state: &mut State<'_>,
     local_variables: &HashSet<String>,
-) -> Result<(), String> {
+) -> Result<(), Error> {
     expressions
         .iter()
         .try_for_each(|e| check_expression(location, e, state, local_variables))
@@ -849,8 +872,11 @@ fn check_type(
         }
         check_path_try_prelude(location.clone(), p.clone(), state)?
     }
-    ty.children()
-        .try_for_each(|e| check_expression(location, e, state, local_variables))
+    // TODO once the return type of this function changes to Error,
+    // we can keep the erorr here.
+    ty.children().try_for_each(|e| {
+        check_expression(location, e, state, local_variables).map_err(|e| e.message().to_string())
+    })
 }
 
 #[cfg(test)]
@@ -877,7 +903,7 @@ mod tests {
             })
             .map_err(|s| s.to_string());
 
-        assert_eq!(res, expected);
+        assert_eq!(res.map_err(|e| e.message().to_string()), expected);
     }
 
     #[test]

parser-util/src/lib.rs

@@ -71,6 +71,10 @@ impl Error {
         let mut writer = StandardStream::stderr(ColorChoice::Always);
         term::emit(&mut writer, &config, &files, &diagnostic).unwrap()
     }
+
+    pub fn message(&self) -> &str {
+        &self.message
+    }
 }
 
 pub fn handle_parse_error(

pipeline/src/pipeline.rs

@@ -607,7 +607,11 @@ impl<T: FieldElement> Pipeline<T> {
                 let (path, parsed) = self.artifact.parsed_asm_file.take().unwrap();
 
                 self.log("Loading dependencies and resolving references");
-                powdr_importer::load_dependencies_and_resolve(path, parsed).map_err(|e| vec![e])?
+                powdr_importer::load_dependencies_and_resolve(path, parsed).map_err(|e| {
+                    // TODO at some point, change the error type in Pipeline so that we can forward it here.
+                    e.output_to_stderr();
+                    vec![e.message().to_string()]
+                })?
             });
         }