Make ErrorStore a separate type, allowing more freedom & debugging in…

… ErrorCollector

src/compiler_top.rs

@@ -3,7 +3,7 @@ use std::rc::Rc;
 use tree_sitter::Parser;
 
 use crate::{
-    debug::SpanDebugger ,errors::ErrorCollector, file_position::FileText, flattening::{flatten_all_modules, initialization::gather_initial_file_data, typechecking::typecheck_all_modules, Module}, instantiation::InstantiatedModule, linker::{FileData, FileUUID, Linker, ModuleUUID}
+    debug::SpanDebugger, errors::ErrorStore, file_position::FileText, flattening::{flatten_all_modules, initialization::gather_initial_file_data, typechecking::typecheck_all_modules, Module}, instantiation::InstantiatedModule, linker::{FileData, FileUUID, Linker, ModuleUUID}
 };
 
 pub fn add_file(text : String, linker : &mut Linker) -> FileUUID {
@@ -13,16 +13,18 @@ pub fn add_file(text : String, linker : &mut Linker) -> FileUUID {
 
     let file_id = linker.files.reserve();
     linker.files.alloc_reservation(file_id, FileData{
-        parsing_errors : ErrorCollector::new(file_id, text.len()),
+        parsing_errors : ErrorStore::new(),
         file_text : FileText::new(text),
         tree,
         associated_values : Vec::new()
     });
 
-    let mut builder = linker.get_file_builder(file_id);
-    let mut span_debugger = SpanDebugger::new("gather_initial_file_data in add_file", builder.file_text);
-    gather_initial_file_data(&mut builder);    
-    span_debugger.defuse();
+    linker.with_file_builder(file_id, |builder| {
+        let mut span_debugger = SpanDebugger::new("gather_initial_file_data in add_file", builder.file_text);
+        gather_initial_file_data(builder);
+        span_debugger.defuse();
+    });
+
     file_id
 }
 
@@ -33,21 +35,23 @@ pub fn update_file(text : String, file_id : FileUUID, linker : &mut Linker) {
     parser.set_language(&tree_sitter_sus::language()).unwrap();
     let tree = parser.parse(&text, None).unwrap();
 
-    file_data.parsing_errors.reset(text.len());
+    file_data.parsing_errors = ErrorStore::new();
     file_data.file_text = FileText::new(text);
     file_data.tree = tree;
 
-    let mut builder = linker.get_file_builder(file_id);
-    let mut span_debugger = SpanDebugger::new("gather_initial_file_data in update_file (temporary fix)", builder.file_text);
-    gather_initial_file_data(&mut builder);
-    span_debugger.defuse();
+    linker.with_file_builder(file_id, |builder| {
+        let mut span_debugger = SpanDebugger::new("gather_initial_file_data in update_file", builder.file_text);
+        gather_initial_file_data(builder);
+        span_debugger.defuse();
+    });
 }
 
 pub fn recompile_all(linker : &mut Linker) {
     // First reset all modules back to post-gather_initial_file_data
     for (_, md) in &mut linker.modules {
         let Module { link_info, module_ports:_, instructions, instantiations } = md;
         link_info.reset_to(link_info.after_initial_parse_cp);
+        link_info.after_flatten_cp = None;
         instructions.clear();
         instantiations.clear_instances()
     }

src/dev_aid/lsp.rs

@@ -9,7 +9,7 @@ use lsp_types::notification::Notification;
 use crate::{
     arena_alloc::ArenaVector,
     compiler_top::{add_file, recompile_all, update_file},
-    errors::{CompileError, ErrorCollector, ErrorLevel},
+    errors::{CompileError, ErrorLevel},
     file_position::{FileText, LineCol, Span},
     flattening::{FlatID, IdentifierType, Instruction, Module, WireInstance, WireReference, WireReferenceRoot, WireSource},
     instantiation::{SubModuleOrWire, CALCULATE_LATENCY_LATER},
@@ -215,7 +215,7 @@ fn cvt_span_to_lsp_range(ch_sp : Span, file_text : &FileText) -> lsp_types::Rang
 }
 
 // Requires that token_positions.len() == tokens.len() + 1 to include EOF token
-fn convert_diagnostic(err : CompileError, main_file_text : &FileText, linker : &Linker, uris : &ArenaVector<Url, FileUUIDMarker>) -> Diagnostic {
+fn convert_diagnostic(err : &CompileError, main_file_text : &FileText, linker : &Linker, uris : &ArenaVector<Url, FileUUIDMarker>) -> Diagnostic {
     assert!(main_file_text.is_span_valid(err.position), "bad error: {}", err.reason);
     let error_pos = cvt_span_to_lsp_range(err.position, main_file_text);
 
@@ -224,39 +224,16 @@ fn convert_diagnostic(err : CompileError, main_file_text : &FileText, linker : &
         ErrorLevel::Warning => DiagnosticSeverity::WARNING,
     };
     let mut related_info = Vec::new();
-    for info in err.infos {
+    for info in &err.infos {
         let info_file_text = &linker.files[info.file].file_text;
         let file_name = uris[info.file].to_string();
         let info_span = info.position;
         assert!(info_file_text.is_span_valid(info_span), "bad info in {file_name}:\n{}; in err: {}.\nSpan is {info_span}, but file length is {}", info.info, err.reason, info_file_text.len());
         let info_pos = cvt_span_to_lsp_range(info_span, info_file_text);
         let location = Location{uri : uris[info.file].clone(), range : info_pos};
-        related_info.push(DiagnosticRelatedInformation { location, message: info.info });
+        related_info.push(DiagnosticRelatedInformation { location, message: info.info.clone() });
     }
-    Diagnostic::new(error_pos, Some(severity), None, None, err.reason, Some(related_info), None)
-}
-
-// Requires that token_positions.len() == tokens.len() + 1 to include EOF token
-fn send_errors_warnings(connection: &Connection, errors : ErrorCollector, main_file_text : &FileText, linker : &Linker, uris : &ArenaVector<Url, FileUUIDMarker>) -> Result<(), Box<dyn Error + Sync + Send>> {
-    let mut diag_vec : Vec<Diagnostic> = Vec::new();
-    let (err_vec, file) = errors.get();
-    for err in err_vec {
-        diag_vec.push(convert_diagnostic(err, main_file_text, linker, uris));
-    }
-
-    let params = &PublishDiagnosticsParams{
-        uri: uris[file].clone(),
-        diagnostics: diag_vec,
-        version: None
-    };
-    let params_json = serde_json::to_value(params)?;
-
-    connection.sender.send(Message::Notification(lsp_server::Notification{
-        method: PublishDiagnostics::METHOD.to_owned(),
-        params: params_json
-    }))?;
-
-    Ok(())
+    Diagnostic::new(error_pos, Some(severity), None, None, err.reason.clone(), Some(related_info), None)
 }
 
 
@@ -397,11 +374,25 @@ fn get_hover_info<'l>(file_cache : &'l LoadedFileCache, text_pos : &lsp_types::T
 }
 
 fn push_all_errors(connection: &Connection, file_cache : &LoadedFileCache) -> Result<(), Box<dyn Error + Sync + Send>> {
-    for (uuid, file_data) in &file_cache.linker.files {
-        let errors = file_cache.linker.get_all_errors_in_file(uuid);
-
-        // println!("Errors: {:?}", &errors);
-        send_errors_warnings(&connection, errors, &file_data.file_text, &file_cache.linker, &file_cache.uris)?;
+    for (file_id, file_data) in &file_cache.linker.files {
+        let mut diag_vec : Vec<Diagnostic> = Vec::new();
+
+        file_cache.linker.for_all_errors_in_file(file_id, |err| {
+            diag_vec.push(convert_diagnostic(err, &file_data.file_text, &file_cache.linker, &file_cache.uris));
+        });
+
+        let params = &PublishDiagnosticsParams{
+            uri: file_cache.uris[file_id].clone(),
+            diagnostics: diag_vec,
+            version: None
+        };
+        let params_json = serde_json::to_value(params)?;
+
+        connection.sender.send(Message::Notification(lsp_server::Notification{
+            method: PublishDiagnostics::METHOD.to_owned(),
+            params: params_json
+        }))?;
+
     }
     Ok(())
 }

src/dev_aid/syntax_highlighting.rs

@@ -160,11 +160,9 @@ impl Cache<FileUUID> for ArenaVector<(PathBuf, Source<String>), FileUUIDMarker>
 
 pub fn print_all_errors(linker : &Linker, paths_arena : &mut ArenaVector<(PathBuf, Source), FileUUIDMarker>) {
     for (file_uuid, f) in &linker.files {
-        let errors = linker.get_all_errors_in_file(file_uuid);
-
-        for err in errors.get().0 {
-            pretty_print_error(&err, f.parsing_errors.file, linker, paths_arena);
-        }
+        let errors = linker.for_all_errors_in_file(file_uuid, |err| {
+            pretty_print_error(err, file_uuid, linker, paths_arena);
+        });
     }
 }
 

src/errors.rs

@@ -1,8 +1,8 @@
 
 
-use std::cell::{RefCell, Cell};
+use std::cell::RefCell;
 
-use crate::{file_position::Span, linker::{checkpoint::ErrorCheckpoint, FileUUID}};
+use crate::{arena_alloc::ArenaAllocator, file_position::Span, linker::{checkpoint::ErrorCheckpoint, FileData, FileUUID, FileUUIDMarker}};
 
 #[derive(Debug,Clone,PartialEq,Eq)]
 pub enum ErrorLevel {
@@ -29,40 +29,76 @@ pub fn error_info<S : Into<String>>(position : Span, file : FileUUID, reason : S
     ErrorInfo{position, file, info : reason.into()}
 }
 
-// Class that collects and manages errors and warnings
-// Implemented such that it can be shared immutably. This makes many operations to do with parsing easier
-// It doesn't allow indexing, so no immutable references to contents can exist
+/// Stores all errors gathered within a context for reporting to the user. 
+/// 
+/// Only editable by converting to a ErrorCollector using [ErrorStore::take_for_editing]
 #[derive(Debug,Clone)]
-pub struct ErrorCollector {
-    errors : RefCell<Vec<CompileError>>,
-    pub did_error : Cell<bool>,
-    pub file : FileUUID,
-    file_len : usize, // Only used for debugging, to see no invalid errors are produced
+pub struct ErrorStore {
+    errors : Vec<CompileError>,
+    pub did_error : bool
 }
 
-#[allow(dead_code)]
-impl ErrorCollector {
-    pub fn new(file : FileUUID, file_len : usize) -> Self {
-        Self{errors : RefCell::new(Vec::new()), file, file_len, did_error : Cell::new(false)}
+impl ErrorStore {
+    pub fn new() -> ErrorStore {
+        ErrorStore{
+            errors : Vec::new(),
+            did_error : false
+        }
     }
-    pub fn reset(&mut self, file_len : usize) {
-        self.file_len = file_len;
-        self.errors.get_mut().clear();
+
+    pub fn take_for_editing<'linker>(&mut self, file : FileUUID, files : &'linker ArenaAllocator<FileData, FileUUIDMarker>) -> ErrorCollector<'linker> {
+        let error_store = RefCell::new(std::mem::replace(self, ErrorStore::new()));
+        ErrorCollector { error_store, file, file_len : files[file].file_text.len(), files }
     }
+
+    pub fn checkpoint(&self) -> ErrorCheckpoint {
+        ErrorCheckpoint(self.errors.len(), self.did_error)
+    }
+
     pub fn reset_to(&mut self, checkpoint : ErrorCheckpoint) {
-        self.errors.borrow_mut().truncate(checkpoint.0);
-        self.did_error.set(checkpoint.1);
+        self.errors.truncate(checkpoint.0);
+        self.did_error = checkpoint.1;
     }
-    pub fn checkpoint(&self) -> ErrorCheckpoint {
-        ErrorCheckpoint(self.errors.borrow().len(), self.did_error.get())
+
+    pub fn is_untouched(&self) -> bool {
+        self.errors.is_empty()
     }
+}
 
-    pub fn new_for_same_file_clean_did_error(&self) -> Self {
-        Self{errors : RefCell::new(Vec::new()), file : self.file, file_len : self.file_len, did_error : Cell::new(false)}
+impl<'e> IntoIterator for &'e ErrorStore {
+    type Item = &'e CompileError;
+
+    type IntoIter = std::slice::Iter<'e, CompileError>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        self.errors.iter()
     }
+}
 
-    pub fn new_for_same_file_inherit_did_error(&self) -> Self {
-        Self{errors : RefCell::new(Vec::new()), file : self.file, file_len : self.file_len, did_error : self.did_error.clone()}
+
+
+/// Class that collects and manages errors and warnings
+/// 
+/// Implemented such that it can be shared immutably. 
+/// This allows use in immutable contexts, because reporting errors isn't really changing the context
+#[derive(Clone)]
+pub struct ErrorCollector<'linker> {
+    error_store : RefCell<ErrorStore>,
+    /// Main file of this collector. Makes creating errors easier
+    pub file : FileUUID,
+    /// Only used for debugging, to see no invalid errors are produced
+    file_len : usize,
+    files : &'linker ArenaAllocator<FileData, FileUUIDMarker>
+}
+
+impl<'linker> ErrorCollector<'linker> {
+    pub fn new_empty(file : FileUUID, files : &'linker ArenaAllocator<FileData, FileUUIDMarker>) -> Self {
+        Self{error_store : RefCell::new(ErrorStore::new()), file, file_len : files[file].file_text.len(), files}
+    }
+
+    /// Turn the collector back into a [ErrorStore]
+    pub fn into_storage(self) -> ErrorStore {
+        self.error_store.into_inner()
     }
 
     fn assert_span_good(&self, span : Span) {
@@ -73,22 +109,19 @@ impl ErrorCollector {
     fn push_diagnostic(&self, diagnostic : CompileError) {
         self.assert_span_good(diagnostic.position);
         for info in &diagnostic.infos {
-            // Can only verify for diagnostics within this file, but that should be good enough to catch bugs
-            if info.file == self.file {
-                self.assert_span_good(info.position);
-            }
+            assert!(info.position.into_range().end <= self.files[info.file].file_text.len());
         }
-        self.errors.borrow_mut().push(diagnostic);
+        let mut store = self.error_store.borrow_mut();
+        store.did_error |= diagnostic.level == ErrorLevel::Error;
+        store.errors.push(diagnostic);
     }
 
     pub fn error_basic<S : Into<String>>(&self, position : Span, reason : S) {
         self.push_diagnostic(CompileError{position, reason : reason.into(), infos : Vec::new(), level : ErrorLevel::Error});
-        self.did_error.set(true);
     }
 
     pub fn error_with_info<S : Into<String>>(&self, position : Span, reason : S, infos : Vec<ErrorInfo>) {
         self.push_diagnostic(CompileError{position, reason : reason.into(), infos : infos, level : ErrorLevel::Error});
-        self.did_error.set(true);
     }
 
     pub fn warn_basic<S : Into<String>>(&self, position : Span, reason : S) {
@@ -99,20 +132,7 @@ impl ErrorCollector {
         self.push_diagnostic(CompileError{position, reason : reason.into(), infos : infos, level : ErrorLevel::Warning});
     }
 
-    pub fn get(self) -> (Vec<CompileError>, FileUUID) {
-        (self.errors.into_inner(), self.file)
-    }
-
-    pub fn ingest(&self, source : &Self) {
-        assert!(self.file == source.file);
-        assert!(self.file_len == source.file_len);
-        self.errors.borrow_mut().extend_from_slice(&source.errors.borrow());
-    }
-
-    pub fn take(&mut self) -> ErrorCollector {
-        std::mem::replace(self, self.new_for_same_file_clean_did_error())
-    }
-    pub fn is_untouched(&self) -> bool {
-        self.errors.borrow().is_empty()
+    pub fn did_error(&self) -> bool {
+        self.error_store.borrow().did_error
     }
 }

src/flattening/initialization.rs

@@ -3,12 +3,7 @@ use sus_proc_macro::{field, kind};
 
 
 use crate::{
-    arena_alloc::{FlatAlloc, UUIDMarker, UUIDRange, UUID},
-    file_position::{FileText, Span},
-    flattening::Module,
-    instantiation::InstantiationList,
-    linker::{checkpoint::CheckPoint, FileBuilder, LinkInfo, ResolvedGlobals},
-    parser::Cursor
+    arena_alloc::{FlatAlloc, UUIDMarker, UUIDRange, UUID}, errors::ErrorCollector, file_position::{FileText, Span}, flattening::Module, instantiation::InstantiationList, linker::{checkpoint::CheckPoint, FileBuilder, LinkInfo, ResolvedGlobals}, parser::Cursor
 };
 
 use super::{FlatID, IdentifierType};
@@ -80,7 +75,7 @@ pub fn gather_initial_file_data(builder : &mut FileBuilder) {
         let (kind, span) = cursor.kind_span();
         match kind {
             kind!("module") => {
-                let parsing_errors = builder.other_parsing_errors.new_for_same_file_clean_did_error();
+                let parsing_errors = ErrorCollector::new_empty(builder.file_id, builder.files);
                 cursor.report_all_decendant_errors(&parsing_errors);
                 cursor.go_down_no_check(|cursor| {
                     let name_span = cursor.field_span(field!("name"), kind!("identifier"));
@@ -109,7 +104,8 @@ pub fn gather_initial_file_data(builder : &mut FileBuilder) {
                     interfaces.alloc(Interface{func_call_inputs, func_call_outputs, ports_for_this_interface : ports.range_since(ports_start_at)});
 
                     let resolved_globals = ResolvedGlobals::empty();
-                    let after_initial_parse_cp = CheckPoint::checkpoint(&parsing_errors, &resolved_globals);
+                    let errors = parsing_errors.into_storage();
+                    let after_initial_parse_cp = CheckPoint::checkpoint(&errors, &resolved_globals);
 
                     let md = Module{
                         link_info: LinkInfo {
@@ -118,9 +114,10 @@ pub fn gather_initial_file_data(builder : &mut FileBuilder) {
                             name,
                             name_span,
                             span,
-                            errors : parsing_errors,
+                            errors,
                             resolved_globals,
-                            after_initial_parse_cp
+                            after_initial_parse_cp,
+                            after_flatten_cp : None
                         },
                         instructions : FlatAlloc::new(),
                         module_ports : ModulePorts{

src/flattening/mod.rs

@@ -475,7 +475,7 @@ struct FlatteningContext<'l, 'errs> {
     #[allow(dead_code)]
     constants : Resolver<'l, 'errs, ConstantUUIDMarker, NamedConstant>,
     name_resolver : NameResolver<'l, 'errs>,
-    errors : &'errs ErrorCollector,
+    errors : &'errs ErrorCollector<'l>,
 
     ports_to_visit : UUIDRange<PortIDMarker>,
 

src/flattening/typechecking.rs

@@ -37,7 +37,7 @@ struct TypeCheckingContext<'l, 'errs> {
     modules : InternalResolver<'l, 'errs, ModuleUUIDMarker, Module>,
     types : Resolver<'l, 'errs, TypeUUIDMarker, NamedType>,
     constants : Resolver<'l, 'errs, ConstantUUIDMarker, NamedConstant>,
-    errors : &'errs ErrorCollector,
+    errors : &'errs ErrorCollector<'l>,
 }
 
 impl<'l, 'errs> Deref for TypeCheckingContext<'l, 'errs> {