From d658782e0a171c32b879b578cde68d4f262cacac Mon Sep 17 00:00:00 2001 From: SpontanCombust <61706594+SpontanCombust@users.noreply.github.com> Date: Tue, 14 May 2024 21:01:35 +0200 Subject: [PATCH] Refactor: use `LocatedDiagnostic` instead of maps of `Diagnostic` --- crates/analysis/src/jobs/merge_symtabs.rs | 41 +++-- crates/analysis/src/jobs/scan_symbols.rs | 150 +++++++++++------- .../src/model/collections/symbol_table.rs | 12 +- crates/lsp/src/tasks/symbol_scan_tasks.rs | 21 ++- 4 files changed, 129 insertions(+), 95 deletions(-) diff --git a/crates/analysis/src/jobs/merge_symtabs.rs b/crates/analysis/src/jobs/merge_symtabs.rs index 498b60c6..bafc532a 100644 --- a/crates/analysis/src/jobs/merge_symtabs.rs +++ b/crates/analysis/src/jobs/merge_symtabs.rs @@ -1,31 +1,28 @@ -use std::collections::HashMap; -use abs_path::AbsPath; use witcherscript_diagnostics::*; use crate::model::collections::SymbolTable; -/// Key in the `diagnostics` map is a local path of the source file in the source tree pub fn merge_symbol_tables( target_symtab: &mut SymbolTable, source_symtab: SymbolTable, - diagnostics: &mut HashMap> + diagnostics: &mut Vec ) { - for (local_source_path, errors) in target_symtab.merge(source_symtab) { - let abs_source_path = target_symtab.script_root().join(local_source_path).unwrap(); - - let errors_as_diags = errors.into_iter() - .map(|err| Diagnostic { - range: err.incoming_location.label_range, - kind: DiagnosticKind::SymbolNameTaken { - name: err.occupied_path.components().last().unwrap().name.to_string(), - precursor_range: err.occupied_location.as_ref().map(|loc| loc.label_range), - precursor_file_path: err.occupied_location.as_ref().map(|loc| target_symtab.script_root().join(&loc.local_source_path).unwrap()) - }.into() - }); - - diagnostics - .entry(abs_source_path) - .or_default() - .extend(errors_as_diags); - } + diagnostics.extend( + target_symtab + .merge(source_symtab) + .into_iter() + .map(|err| { + LocatedDiagnostic { + path: err.incoming_location.abs_source_path.clone(), + diagnostic: Diagnostic { + range: err.incoming_location.label_range, + kind: DiagnosticKind::SymbolNameTaken { + name: err.occupied_path.components().last().unwrap().name.to_string(), + precursor_range: err.occupied_location.as_ref().map(|loc| loc.label_range), + precursor_file_path: err.occupied_location.as_ref().map(|loc| loc.abs_source_path.clone()) + } + } + } + }) + ); } \ No newline at end of file diff --git a/crates/analysis/src/jobs/scan_symbols.rs b/crates/analysis/src/jobs/scan_symbols.rs index 76bede5b..fe3098ce 100644 --- a/crates/analysis/src/jobs/scan_symbols.rs +++ b/crates/analysis/src/jobs/scan_symbols.rs @@ -17,21 +17,21 @@ pub fn scan_symbols( doc: &ScriptDocument, local_source_path: &Path, scripts_root: &AbsPath, - symtab: &mut SymbolTable -) -> Vec { + symtab: &mut SymbolTable, + diagnostics: &mut Vec +) { let mut visitor = SymbolScannerVisitor { symtab, doc, local_source_path, scripts_root, - diagnostics: Vec::new(), + diagnostics, current_path: SymbolPathBuf::empty(), current_param_ordinal: 0, current_var_ordinal: 0 }; script.visit_nodes(&mut visitor); - visitor.diagnostics } @@ -40,7 +40,7 @@ struct SymbolScannerVisitor<'a> { doc: &'a ScriptDocument, local_source_path: &'a Path, scripts_root: &'a AbsPath, - diagnostics: Vec, + diagnostics: &'a mut Vec, current_path: SymbolPathBuf, current_param_ordinal: usize, @@ -58,13 +58,16 @@ impl SymbolScannerVisitor<'_> { .map(|loc| (Some(self.scripts_root.join(loc.local_source_path).unwrap()), Some(loc.label_range))) .unwrap_or((None, None)); - self.diagnostics.push(Diagnostic { - range, - kind: DiagnosticKind::SymbolNameTaken { - name: err.occupied_path.components().last().unwrap().name.to_string(), - precursor_file_path, - precursor_range - }.into() + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range, + kind: DiagnosticKind::SymbolNameTaken { + name: err.occupied_path.components().last().unwrap().name.to_string(), + precursor_file_path, + precursor_range + } + } }); } @@ -78,9 +81,12 @@ impl SymbolScannerVisitor<'_> { fn check_type_from_identifier(&mut self, n: IdentifierNode) -> BasicTypeSymbolPath { let type_name = n.value(&self.doc); if type_name == ArrayTypeSymbol::TYPE_NAME { - self.diagnostics.push(Diagnostic { - range: Range::new(n.range().end, n.range().end), - kind: DiagnosticKind::MissingTypeArg + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range: n.range(), + kind: DiagnosticKind::MissingTypeArg + } }); } else { return BasicTypeSymbolPath::new(&type_name); @@ -100,9 +106,12 @@ impl SymbolScannerVisitor<'_> { } } else { // since only array type takes type argument, all other uses of type arg are invalid - self.diagnostics.push(Diagnostic { - range: n.type_arg().unwrap().range(), - kind: DiagnosticKind::UnnecessaryTypeArg + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range: n.type_arg().unwrap().range(), + kind: DiagnosticKind::UnnecessaryTypeArg + } }); return self.check_type_from_identifier(n.type_name()).into(); @@ -138,9 +147,12 @@ impl SyntaxNodeVisitor for SymbolScannerVisitor<'_> { for (spec, range) in n.specifiers().map(|specn| (specn.value(), specn.range())) { if !sym.specifiers.insert(spec) { - self.diagnostics.push(Diagnostic { - range, - kind: DiagnosticKind::RepeatedSpecifier + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range, + kind: DiagnosticKind::RepeatedSpecifier + } }); } } @@ -189,9 +201,12 @@ impl SyntaxNodeVisitor for SymbolScannerVisitor<'_> { for (spec, range) in n.specifiers().map(|specn| (specn.value(), specn.range())) { if !sym.specifiers.insert(spec) { - self.diagnostics.push(Diagnostic { - range, - kind: DiagnosticKind::RepeatedSpecifier + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range, + kind: DiagnosticKind::RepeatedSpecifier + } }); } } @@ -249,9 +264,12 @@ impl SyntaxNodeVisitor for SymbolScannerVisitor<'_> { for (spec, range) in n.specifiers().map(|specn| (specn.value(), specn.range())) { if !sym.specifiers.insert(spec) { - self.diagnostics.push(Diagnostic { - range, - kind: DiagnosticKind::RepeatedSpecifier + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range, + kind: DiagnosticKind::RepeatedSpecifier + } }); } } @@ -322,9 +340,12 @@ impl SyntaxNodeVisitor for SymbolScannerVisitor<'_> { for (spec, range) in n.specifiers().map(|specn| (specn.value(), specn.range())) { if !sym.specifiers.insert(spec) { - self.diagnostics.push(Diagnostic { - range, - kind: DiagnosticKind::RepeatedSpecifier + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range, + kind: DiagnosticKind::RepeatedSpecifier + } }); } } @@ -369,18 +390,24 @@ impl SyntaxNodeVisitor for SymbolScannerVisitor<'_> { for (spec, range) in n.specifiers().map(|specn| (specn.value(), specn.range())) { if matches!(spec, MemberFunctionSpecifier::AccessModifier(_)) { if found_access_modif_before { - self.diagnostics.push(Diagnostic { - range, - kind: DiagnosticKind::MultipleAccessModifiers - }) + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range, + kind: DiagnosticKind::MultipleAccessModifiers + } + }); } found_access_modif_before = true; } if !sym.specifiers.insert(spec) { - self.diagnostics.push(Diagnostic { - range, - kind: DiagnosticKind::RepeatedSpecifier + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range, + kind: DiagnosticKind::RepeatedSpecifier + } }); } } @@ -445,9 +472,12 @@ impl SyntaxNodeVisitor for SymbolScannerVisitor<'_> { let mut specifiers = SpecifierBitmask::new(); for (spec, range) in n.specifiers().map(|specn| (specn.value(), specn.range())) { if !specifiers.insert(spec) { - self.diagnostics.push(Diagnostic { - range, - kind: DiagnosticKind::RepeatedSpecifier + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range, + kind: DiagnosticKind::RepeatedSpecifier + } }); } } @@ -477,18 +507,24 @@ impl SyntaxNodeVisitor for SymbolScannerVisitor<'_> { for (spec, range) in n.specifiers().map(|specn| (specn.value(), specn.range())) { if matches!(spec, MemberVarSpecifier::AccessModifier(_)) { if found_access_modif_before { - self.diagnostics.push(Diagnostic { - range, - kind: DiagnosticKind::MultipleAccessModifiers - }) + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range, + kind: DiagnosticKind::MultipleAccessModifiers + } + }); } found_access_modif_before = true; } if !specifiers.insert(spec) { - self.diagnostics.push(Diagnostic { - range, - kind: DiagnosticKind::RepeatedSpecifier + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range, + kind: DiagnosticKind::RepeatedSpecifier + } }); } } @@ -523,18 +559,24 @@ impl SyntaxNodeVisitor for SymbolScannerVisitor<'_> { for (spec, range) in n.specifiers().map(|specn| (specn.value(), specn.range())) { if matches!(spec, AutobindSpecifier::AccessModifier(_)) { if found_access_modif_before { - self.diagnostics.push(Diagnostic { - range, - kind: DiagnosticKind::MultipleAccessModifiers - }) + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range, + kind: DiagnosticKind::MultipleAccessModifiers + } + }); } found_access_modif_before = true; } if !sym.specifiers.insert(spec) { - self.diagnostics.push(Diagnostic { - range, - kind: DiagnosticKind::RepeatedSpecifier + self.diagnostics.push(LocatedDiagnostic { + path: self.symtab.script_root().join(self.local_source_path).unwrap(), + diagnostic: Diagnostic { + range, + kind: DiagnosticKind::RepeatedSpecifier + } }); } } diff --git a/crates/analysis/src/model/collections/symbol_table.rs b/crates/analysis/src/model/collections/symbol_table.rs index 230d3459..d5504636 100644 --- a/crates/analysis/src/model/collections/symbol_table.rs +++ b/crates/analysis/src/model/collections/symbol_table.rs @@ -156,16 +156,14 @@ impl SymbolTable { - pub(crate) fn merge(&mut self, mut other: Self) -> HashMap> { - let mut errors = HashMap::new(); + pub(crate) fn merge(&mut self, mut other: Self) -> Vec { + let mut errors = Vec::new(); if other.is_empty() { return errors; } let mut root_children_sympaths = Vec::new(); for (file_path, sympath_roots) in other.source_path_assocs { - let mut file_errors = Vec::new(); - self.source_path_assocs.entry(file_path.clone()) .or_default(); @@ -174,7 +172,7 @@ impl SymbolTable { if let Some(occupying_variant) = self.symbols.get(root) { let occupying_sym = occupying_variant.as_dyn(); if !occupying_sym.path().has_missing() { - file_errors.push(MergeConflictError { + errors.push(MergeConflictError { occupied_path: occupying_sym.path().to_sympath_buf(), occupied_location: self.locate(root), incoming_location: SymbolLocation { @@ -220,7 +218,7 @@ impl SymbolTable { let occupying_sym = occupying_variant.as_dyn(); if !occupying_sym.path().has_missing() { - file_errors.push(MergeConflictError { + errors.push(MergeConflictError { occupied_path: occupying_sym.path().to_sympath_buf(), occupied_location: self.locate(&incoming_sympath), incoming_location: SymbolLocation { @@ -241,8 +239,6 @@ impl SymbolTable { root_children_sympaths.clear(); } - - errors.insert(file_path.clone(), file_errors); } errors diff --git a/crates/lsp/src/tasks/symbol_scan_tasks.rs b/crates/lsp/src/tasks/symbol_scan_tasks.rs index 61f59234..438791db 100644 --- a/crates/lsp/src/tasks/symbol_scan_tasks.rs +++ b/crates/lsp/src/tasks/symbol_scan_tasks.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, sync::{Arc, Mutex}}; +use std::sync::{Arc, Mutex}; use abs_path::AbsPath; use rayon::prelude::*; use tokio::{sync::oneshot, time::Instant}; @@ -19,6 +19,7 @@ impl Backend { for p in modified_source_paths.iter() { symtab.remove_for_source(p.local()); + self.reporter.clear_diagnostics(p.absolute(), DiagnosticDomain::SymbolAnalysis).await; } let worker_count = std::cmp::min(rayon::current_num_threads(), modified_source_paths.len()); @@ -57,16 +58,15 @@ impl Backend { let duration = Instant::now() - start; self.reporter.log_info(format!("Updated symbol table for content {} in {:.3}s", content_path, duration.as_secs_f32())).await; - for (file_path, diagnostics) in scanning_diagnostis { - self.reporter.clear_diagnostics(&file_path, DiagnosticDomain::SymbolAnalysis).await; - self.reporter.push_diagnostics(&file_path, diagnostics).await; + for loc_diag in scanning_diagnostis { + self.reporter.push_diagnostic(&loc_diag.path, loc_diag.diagnostic).await; } } } struct SymbolScanWorker { symtab: SymbolTable, - diagnostics: HashMap>, + diagnostics: Vec, job_provider: SymbolScanJobProvider, scripts: Arc } @@ -75,7 +75,7 @@ impl SymbolScanWorker { fn new(job_provider: SymbolScanJobProvider, scripts: Arc, scripts_root: Arc) -> Self { Self { symtab: SymbolTable::new(scripts_root), - diagnostics: HashMap::new(), + diagnostics: Vec::new(), job_provider, scripts } @@ -84,19 +84,18 @@ impl SymbolScanWorker { fn work(&mut self) { while let Some(job) = self.job_provider.poll() { let script_state = self.scripts.get(job.source_path.absolute()).unwrap(); - let diagnostics = jobs::scan_symbols( + jobs::scan_symbols( &script_state.script, &script_state.buffer, &job.source_path.local(), &job.source_path.script_root(), - &mut self.symtab + &mut self.symtab, + &mut self.diagnostics ); - - self.diagnostics.insert(job.source_path.into_absolute(), diagnostics); } } - fn finish(self) -> (SymbolTable, HashMap>) { + fn finish(self) -> (SymbolTable, Vec) { (self.symtab, self.diagnostics) } }