Refactor unify_report_error and error-related stuff to improve diagno…

…stics
pc2 · Nov 20, 2024 · 4c94f66 · 4c94f66
1 parent b0fd455
commit 4c94f66
Show file tree

Hide file tree

Showing 8 changed files with 217 additions and 111 deletions.
diff --git a/src/errors.rs b/src/errors.rs
@@ -85,7 +85,7 @@ pub struct ErrorCollector<'linker> {
     pub file: FileUUID,
     /// Only used for debugging, to see no invalid errors are produced
     file_len: usize,
-    files: &'linker ArenaAllocator<FileData, FileUUIDMarker>,
+    pub files: &'linker ArenaAllocator<FileData, FileUUIDMarker>,
 }
 
 impl<'linker> ErrorCollector<'linker> {
@@ -211,32 +211,20 @@ impl<'ec> ErrorReference<'ec> {
         self.info((span, self.err_collector.file), reason)
     }
     pub fn info_obj<Obj: FileKnowingErrorInfoObject>(&self, obj: &Obj) -> &Self {
-        let ((position, file), info) = obj.make_global_info(&self.err_collector.files);
-        self.existing_info(ErrorInfo {
-            position,
-            file,
-            info,
-        })
+        self.existing_info(obj.make_global_info(&self.err_collector.files))
     }
     pub fn info_obj_same_file<Obj: ErrorInfoObject>(&self, obj: &Obj) -> &Self {
-        let (position, info) = obj.make_info(&self.err_collector.files[self.err_collector.file]);
-        self.existing_info(ErrorInfo {
-            position,
-            file: self.err_collector.file,
-            info,
-        })
+        self.existing_info(obj.make_info(self.err_collector.file))
     }
     pub fn info_obj_different_file<Obj: ErrorInfoObject>(
         &self,
         obj: &Obj,
         file: FileUUID,
     ) -> &Self {
-        let (position, info) = obj.make_info(&self.err_collector.files[file]);
-        self.existing_info(ErrorInfo {
-            position,
-            file,
-            info,
-        })
+        self.existing_info(obj.make_info(file))
+    }
+    pub fn add_info_list(&self, mut info_list: Vec<ErrorInfo>) {
+        self.err_collector.error_store.borrow_mut().errors[self.pos].infos.append(&mut info_list);
     }
     pub fn suggest_replace<S: Into<String>>(&self, replace_span: Span, replace_with: S) -> &Self {
         self.info_same_file(
@@ -251,59 +239,83 @@ impl<'ec> ErrorReference<'ec> {
 
 /// This represents objects that can be given as info to an error in a straight-forward way.
 pub trait ErrorInfoObject {
-    fn make_info(&self, file_data: &FileData) -> (Span, String);
+    fn make_info(&self, file: FileUUID) -> ErrorInfo;
 }
 
 pub trait FileKnowingErrorInfoObject {
     fn make_global_info(
         &self,
         files: &ArenaAllocator<FileData, FileUUIDMarker>,
-    ) -> ((Span, FileUUID), String);
+    ) -> ErrorInfo;
 }
 
 // Trait implementations in the compiler
 
 impl ErrorInfoObject for Declaration {
-    fn make_info(&self, _file_data: &FileData) -> (Span, String) {
-        (self.decl_span, format!("'{}' declared here", &self.name))
+    fn make_info(&self, file: FileUUID) -> ErrorInfo {
+        ErrorInfo {
+            position: self.name_span,
+            file,
+            info: format!("'{}' declared here", &self.name),
+        }
     }
 }
 
 impl ErrorInfoObject for SubModuleInstance {
-    fn make_info(&self, _file_data: &FileData) -> (Span, String) {
-        if let Some((name, span)) = &self.name {
+    fn make_info(&self, file: FileUUID) -> ErrorInfo {
+        let (position, info) = if let Some((name, span)) = &self.name {
             (*span, format!("{name} declared here"))
         } else {
             (self.module_ref.name_span, "Used here".to_owned())
+        };
+        ErrorInfo {
+            position,
+            file,
+            info,
         }
     }
 }
 
 impl ErrorInfoObject for Instruction {
-    fn make_info(&self, file_data: &FileData) -> (Span, String) {
+    fn make_info(&self, file: FileUUID) -> ErrorInfo {
         match self {
-            Instruction::SubModule(sm) => sm.make_info(file_data),
-            Instruction::Declaration(decl) => decl.make_info(file_data),
+            Instruction::SubModule(sm) => sm.make_info(file),
+            Instruction::Declaration(decl) => decl.make_info(file),
             _ => unreachable!("At least there shouldn't be cases where we're referring to something other than SubModule or Declaration")
         }
     }
 }
 
 impl ErrorInfoObject for TemplateInput {
-    fn make_info(&self, _file_data: &FileData) -> (Span, String) {
-        (self.name_span, format!("{} declared here", self.name))
+    fn make_info(&self, file: FileUUID) -> ErrorInfo {
+        ErrorInfo {
+            position: self.name_span,
+            file,
+            info: format!("Parameter '{}' declared here", self.name),
+        }
+    }
+}
+
+impl ErrorInfoObject for Port {
+    fn make_info(&self, file: FileUUID) -> ErrorInfo {
+        ErrorInfo {
+            position: self.name_span,
+            file,
+            info: format!("Port '{}' declared here", &self.name),
+        }
     }
 }
 
 impl FileKnowingErrorInfoObject for LinkInfo {
     fn make_global_info(
         &self,
         _files: &ArenaAllocator<FileData, FileUUIDMarker>,
-    ) -> ((Span, FileUUID), String) {
-        (
-            (self.name_span, self.file),
-            format!("'{}' defined here", &self.name),
-        )
+    ) -> ErrorInfo {
+        ErrorInfo {
+            position: self.name_span,
+            file: self.file,
+            info: format!("'{}' defined here", &self.name),
+        }
     }
 }
 
@@ -312,37 +324,30 @@ impl FileKnowingErrorInfoObject for (&'_ Module, &'_ Interface) {
     fn make_global_info(
         &self,
         _files: &ArenaAllocator<FileData, FileUUIDMarker>,
-    ) -> ((Span, FileUUID), String) {
+    ) -> ErrorInfo {
         let (md, interface) = *self;
-        (
-            (interface.name_span, md.link_info.file),
-            format!("Interface '{}' defined here", &interface.name),
-        )
-    }
-}
-
-impl ErrorInfoObject for Port {
-    fn make_info(&self, _file_data: &FileData) -> (Span, String) {
-        (
-            self.name_span,
-            format!("Port '{}' declared here", &self.name),
-        )
+        ErrorInfo {
+            position: interface.name_span,
+            file: md.link_info.file,
+            info: format!("Interface '{}' defined here", &interface.name),
+        }
     }
 }
 
 impl FileKnowingErrorInfoObject for Module {
     fn make_global_info(
         &self,
         files: &ArenaAllocator<FileData, FileUUIDMarker>,
-    ) -> ((Span, FileUUID), String) {
-        let ports_str =
-            self.make_all_ports_info_string(&files[self.link_info.file].file_text, None);
-        (
-            (self.link_info.name_span, self.link_info.file),
-            format!(
+    ) -> ErrorInfo {
+        let ports_str = self.make_all_ports_info_string(&files[self.link_info.file].file_text, None);
+
+        ErrorInfo {
+            position: self.link_info.name_span,
+            file: self.link_info.file,
+            info: format!(
                 "Module '{}' defined here. {}",
                 &self.link_info.name, ports_str
             ),
-        )
+        }
     }
 }
diff --git a/src/flattening/typechecking.rs b/src/flattening/typechecking.rs
@@ -1,4 +1,5 @@
 use crate::alloc::ArenaAllocator;
+use crate::errors::{ErrorInfo, ErrorInfoObject, FileKnowingErrorInfoObject};
 use crate::prelude::*;
 use crate::typing::abstract_type::AbstractType;
 use crate::typing::template::TemplateInputKind;
@@ -101,22 +102,22 @@ impl<'l, 'errs> TypeCheckingContext<'l, 'errs> {
         }
     }
 
-    fn get_wire_ref_declaration_point(
+    fn get_wire_ref_info(
         &self,
         wire_ref_root: &WireReferenceRoot,
-    ) -> SpanFile {
+    ) -> ErrorInfo {
         match wire_ref_root {
             WireReferenceRoot::LocalDecl(id, _) => {
                 let decl_root = self.working_on.instructions[*id].unwrap_wire_declaration();
-                (decl_root.decl_span, self.errors.file)
+                decl_root.make_info(self.errors.file)
             }
             WireReferenceRoot::NamedConstant(cst) => {
                 let linker_cst = &self.globals[cst.id];
-                linker_cst.link_info.get_span_file()
+                linker_cst.link_info.make_global_info(&self.errors.files)
             }
             WireReferenceRoot::SubModulePort(port) => {
                 let (decl, file) = self.get_decl_of_module_port(port.port, port.submodule_decl);
-                (decl.decl_span, file)
+                decl.make_info(file)
             }
         }
     }
@@ -451,7 +452,6 @@ impl<'l, 'errs> TypeCheckingContext<'l, 'errs> {
 
                     let (decl, file) =
                         self.get_decl_of_module_port(port, fc.interface_reference.submodule_decl);
-                    let declared_here = (decl.decl_span, file);
 
                     // Typecheck the value with target type
                     let from_wire = self.working_on.instructions[*arg].unwrap_wire();
@@ -461,7 +461,9 @@ impl<'l, 'errs> TypeCheckingContext<'l, 'errs> {
                         &from_wire.typ,
                         &write_to_type,
                         from_wire.span,
-                        "function argument"
+                        || {
+                            ("function argument".to_string(), vec![decl.make_info(file)])
+                        }
                     );
                 }
             }
@@ -473,20 +475,19 @@ impl<'l, 'errs> TypeCheckingContext<'l, 'errs> {
                 self.typecheck_wire_reference(&conn.to, conn.to_span, &conn.to_type);
                 self.join_with_condition(&conn.to_type.domain, conn.to_span.debug());
 
-                let declared_here = self.get_wire_ref_declaration_point(&conn.to.root);
-
-                let write_context = match conn.write_modifiers {
-                    WriteModifiers::Connection {..} => "connection",
-                    WriteModifiers::Initial { initial_kw_span: _ } => "initial value"
-                };
-
-
                 from_wire.span.debug();
                 self.type_checker.typecheck_write_to(
                     &from_wire.typ,
                     &conn.to_type,
                     from_wire.span,
-                    write_context,
+                    || {
+                        let write_context = match conn.write_modifiers {
+                            WriteModifiers::Connection {..} => "connection",
+                            WriteModifiers::Initial { initial_kw_span: _ } => "initial value"
+                        };
+                        let declared_here = self.get_wire_ref_info(&conn.to.root);
+                        (write_context.to_string(), vec![declared_here])
+                    }
                 );
             }
         }
@@ -565,27 +566,31 @@ pub fn apply_types(
     }
 
     // Print all errors
-    for FailedUnification{mut found, mut expected, span, context} in type_checker.type_substitutor.extract_errors() {
+    for FailedUnification{mut found, mut expected, span, context, infos} in type_checker.type_substitutor.extract_errors() {
         // Not being able to fully substitute is not an issue. We just display partial types
         let _ = found.fully_substitute(&type_checker.type_substitutor);
         let _ = expected.fully_substitute(&type_checker.type_substitutor);
 
         let expected_name = expected.to_string(types, &type_checker.template_type_names);
         let found_name = found.to_string(types, &type_checker.template_type_names);
-        errors.error(span, format!("Typing Error: {context} expects a {expected_name} but was given a {found_name}"));
+        errors
+            .error(span, format!("Typing Error: {context} expects a {expected_name} but was given a {found_name}"))
+            .add_info_list(infos);
 
         assert!(
             expected_name != found_name,
             "{expected_name} != {found_name}"
         );
     }
-    for FailedUnification{mut found, mut expected, span, context} in type_checker.domain_substitutor.extract_errors() {
+    for FailedUnification{mut found, mut expected, span, context, infos} in type_checker.domain_substitutor.extract_errors() {
         assert!(found.fully_substitute(&type_checker.domain_substitutor));
         assert!(expected.fully_substitute(&type_checker.domain_substitutor));
 
         let expected_name = format!("{expected:?}");
         let found_name = format!("{found:?}");
-        errors.error(span, format!("Domain error: Attempting to combine domains {found_name} and {expected_name} in {context}"));
+        errors
+            .error(span, format!("Domain error: Attempting to combine domains {found_name} and {expected_name} in {context}"))
+            .add_info_list(infos);
 
         assert!(
             expected_name != found_name,

diff --git a/src/instantiation/concrete_typecheck.rs b/src/instantiation/concrete_typecheck.rs
@@ -1,6 +1,7 @@
 
 use std::ops::Deref;
 
+use crate::errors::ErrorInfoObject;
 use crate::flattening::{DeclarationPortInfo, WireReferenceRoot, WireSource, WrittenType};
 use crate::linker::LinkInfo;
 use crate::typing::template::{ConcreteTemplateArg, HowDoWeKnowTheTemplateArg};
@@ -116,15 +117,17 @@ impl<'fl, 'l> InstantiationContext<'fl, 'l> {
         }
 
         // Print all errors
-        for FailedUnification{mut found, mut expected, span, context} in self.type_substitutor.extract_errors() {
+        for FailedUnification{mut found, mut expected, span, context, infos} in self.type_substitutor.extract_errors() {
             // Not being able to fully substitute is not an issue. We just display partial types
             let _ = found.fully_substitute(&self.type_substitutor);
             let _ = expected.fully_substitute(&self.type_substitutor);
 
             let expected_name = expected.to_string(&self.linker.types);
             let found_name = found.to_string(&self.linker.types);
-            self.errors.error(span, format!("Typing Error: {context} expects a {expected_name} but was given a {found_name}"));
-
+            self.errors
+                .error(span, format!("Typing Error: {context} expects a {expected_name} but was given a {found_name}"))
+                .add_info_list(infos);
+
             assert!(
                 expected_name != found_name,
                 "{expected_name} != {found_name}"
@@ -281,7 +284,12 @@ impl DelayedConstraint<InstantiationContext<'_, '_>> for SubmoduleTypecheckConst
                     }
                     (Some(concrete_port), Some(connecting_wire)) => {
                         let wire = &context.wires[connecting_wire.maps_to_wire];
-                        context.type_substitutor.unify_report_error(&wire.typ, &concrete_port.typ, submod_instr.module_ref.get_total_span(), "submodule port typechecking")
+                        context.type_substitutor.unify_report_error(&wire.typ, &concrete_port.typ, submod_instr.module_ref.get_total_span(), || {
+                            let abstract_port = &sub_module.ports[port_id];
+                            let port_declared_here = abstract_port.make_info(sub_module.link_info.file);
+
+                            (format!("Port '{}'", abstract_port.name), vec![port_declared_here])
+                        });
                     }
                 }
             }