From c80e21fac9c4c71f030b1709a1cf503ac505a686 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 1 Dec 2025 10:54:15 +0100 Subject: [PATCH 01/34] Report ambiguous overrides Signed-off-by: Jonatan Waern --- CHANGELOG.md | 5 ++ src/actions/requests.rs | 57 ++++++++++++++------- src/analysis/mod.rs | 79 ++++++++++++++++++++---------- src/analysis/templating/methods.rs | 74 +++++++++++++++++++++++----- src/analysis/templating/objects.rs | 23 ++++----- 5 files changed, 173 insertions(+), 65 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ea9519d..be294697 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,11 @@ or footers - The DLS will no longer fail to parse files with unicode characters in cblock header or footers, or in multiline comments +- The DLS will now report when an ambiguous default call is made +- Improved how definitions and references from/to methods and default calls are resolved, + goto-definition on a default call will now only go to the method it will call, + and goto-references on a method definition will no longer go to default declarations + that cannot call it ## 0.9.17 - Fixed linter wrongly throwing an error on space after `defined` keyword diff --git a/src/actions/requests.rs b/src/actions/requests.rs index 20bf9ede..ba12c75a 100644 --- a/src/actions/requests.rs +++ b/src/actions/requests.rs @@ -227,8 +227,8 @@ fn fp_to_symbol_refs fn handle_default_remapping (ctx: &InitActionContext, - symbols: Vec, - fp: &ZeroFilePosition) -> HashSet + symbols: &[SymbolRef], + fp: &ZeroFilePosition) -> Option> { let analysis = ctx.analysis.lock().unwrap(); let refr_opt = analysis.reference_at_pos(fp); @@ -239,7 +239,7 @@ fn handle_default_remapping // If we are at a defaut reference, // remap symbol references to methods // to the correct decl site, leave others as-is - return symbols.into_iter() + return Some(symbols.iter() .flat_map(|d|'sym: { let sym = d.lock().unwrap(); if sym.kind == DMLSymbolKind::Method { @@ -248,13 +248,11 @@ fn handle_default_remapping break 'sym vec![*loc]; } } - sym.declarations.clone() - }).collect(); + vec![] + }).collect()); } } - symbols.into_iter() - .flat_map(|d|d.lock().unwrap().declarations.clone()) - .collect() + None } /// The result of a deglob action for a single wildcard import. @@ -617,8 +615,11 @@ impl RequestAction for GotoDeclaration { match fp_to_symbol_refs(&fp, &ctx, &mut limitations) { Ok(symbols) => { let unique_locations = handle_default_remapping(&ctx, - symbols, - &fp); + &symbols, + &fp) + .unwrap_or_else(||symbols.into_iter() + .flat_map(|d|d.lock().unwrap().declarations.clone()) + .collect()); let lsp_locations = unique_locations.into_iter() .map(|l|ls_util::dls_to_location(&l)) .collect(); @@ -676,10 +677,14 @@ impl RequestAction for GotoDefinition { let mut limitations = HashSet::new(); match fp_to_symbol_refs(&fp, &ctx, &mut limitations) { Ok(symbols) => { - let unique_locations: HashSet = - symbols.into_iter() - .flat_map(|d|d.lock().unwrap() - .definitions.clone()).collect(); + let unique_locations = handle_default_remapping(&ctx, + &symbols, + &fp) + .unwrap_or_else(||symbols.into_iter() + .flat_map(|d|d.lock().unwrap().definitions.clone()) + .collect() + ); + let lsp_locations: Vec<_> = unique_locations.into_iter() .map(|l|ls_util::dls_to_location(&l)) .collect(); @@ -699,6 +704,25 @@ impl RequestAction for GotoDefinition { } } +fn filter_out_unpointed_defaults(fp: &ZeroFilePosition, + symbols: &[SymbolRef]) + -> HashSet { + symbols + .iter() + .flat_map(|d|{ + let sym = d.lock().unwrap(); + let ignored_defaults = sym.default_mappings + .iter() + .filter(|(_, ref_loc)|!ref_loc.contains_pos(fp)) + .map(|(decl_loc, _)|*decl_loc) + .collect::>(); + sym.references.iter().filter(|loc| { + !ignored_defaults.contains(loc) + }).cloned().collect::>() + }) + .collect() +} + impl RequestAction for References { type Response = ResponseWithMessage>; @@ -737,10 +761,7 @@ impl RequestAction for References { let mut limitations = HashSet::new(); match fp_to_symbol_refs(&fp, &ctx, &mut limitations) { Ok(symbols) => { - let unique_locations: HashSet = - symbols.into_iter() - .flat_map(|d|d.lock().unwrap() - .references.clone()).collect(); + let unique_locations = filter_out_unpointed_defaults(&fp, &symbols); let lsp_locations: Vec<_> = unique_locations.into_iter() .map(|l|ls_util::dls_to_location(&l)) .collect(); diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index b60e1231..369a4065 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -26,8 +26,7 @@ use rayon::prelude::*; use crate::actions::SourcedDMLError; use crate::actions::analysis_storage::TimestampedStorage; -use crate::analysis::symbols::{SimpleSymbol, DMLSymbolKind, SymbolMaker, - SymbolContainer, StructureSymbol, SymbolSource}; +use crate::analysis::symbols::{DMLSymbolKind, SimpleSymbol, StructureSymbol, SymbolContainer, SymbolMaker, SymbolSource}; pub use crate::analysis::symbols::SymbolRef; use crate::analysis::reference::{Reference, GlobalReference, VariableReference, @@ -60,8 +59,7 @@ use crate::analysis::templating::objects::{make_device, DMLObject, use crate::analysis::templating::topology::{RankMaker, rank_templates, create_templates_traits}; -use crate::analysis::templating::methods::{MethodDeclaration, DMLMethodRef, - DMLMethodArg}; +use crate::analysis::templating::methods::{DMLMethodArg, DMLMethodRef, DefaultCallReference, MethodDeclaration}; use crate::analysis::templating::traits::{DMLTemplate, TemplateTraitInfo}; use crate::analysis::templating::types::DMLResolvedType; @@ -1320,11 +1318,56 @@ impl DeviceAnalysis { } } + fn handle_symbol_ref(symbol: &SymbolRef, + reference: &Reference, + report: &mut Vec) { + let ambiguous_desc: &'static str + = "Ambiguous default call, you may need to clarify the template ordering or use a template-qualified-method-implementation-call"; + + let mut sym = symbol.lock().unwrap(); + sym.references.insert(*reference.loc_span()); + if let Some(meth) = sym.source + .as_object() + .and_then(DMLObject::as_shallow) + .and_then(|s|s.variant.as_method()) { + if let Some(var) = reference.as_variable_ref() { + if var.reference.to_string().as_str() + == "default" { + if let Some(default_decl_ref) = meth.get_default() { + match default_decl_ref { + DefaultCallReference::Valid(default_decl) => + { + sym.default_mappings.insert( + *var.loc_span(), + *default_decl.location()); + }, + DefaultCallReference::Ambiguous(ambiguous_decls) => + report.push(DMLError { + span: *var.loc_span(), + description: ambiguous_desc.to_string(), + severity: Some(DiagnosticSeverity::ERROR), + related: ambiguous_decls + .iter() + .map(|d|(*d.location(), + format!("Possible candidate for default call{}", + d.get_disambiguation_name() + .map_or_else(||"".to_string(), + |n|format!(", can be explicitly called as 'this.{}'", n))) + )) + .collect(), + }), + } + } + } + } + } + } + #[allow(clippy::ptr_arg)] fn match_references_in_scope<'c>( &'c self, scope_chain: Vec<&'c dyn Scope>, - _report: &mut Vec, + report: &mut Vec, method_structure: &HashMap, reference_cache: &Mutex, status: &AliveStatus) { @@ -1332,8 +1375,9 @@ impl DeviceAnalysis { let context_chain: Vec = scope_chain .iter().map(|s|s.create_context()).collect(); // NOTE: chunk number is arbitrarily picked that benches well - current_scope.defined_references().par_chunks(25).for_each(|references|{ + report.extend(current_scope.defined_references().par_chunks(25).flat_map(|references|{ status.assert_alive(); + let mut local_reports = vec![]; for reference in references { debug!("In {:?}, Matching {:?}", context_chain, reference); let symbol_lookup = match &reference { @@ -1364,30 +1408,15 @@ impl DeviceAnalysis { // (not done here because of ownership issues) ReferenceMatches::Found(symbols) => for symbol in &symbols { - let mut sym = symbol.lock().unwrap(); - sym.references.insert(*reference.loc_span()); - if let Some(meth) = sym.source - .as_object() - .and_then(DMLObject::as_shallow) - .and_then(|s|s.variant.as_method()) { - if let Some(default_decl) = meth.get_default() { - if let Some(var) = reference.as_variable_ref() { - if var.reference.to_string().as_str() - == "default" { - sym.default_mappings.insert( - *var.loc_span(), - *default_decl.location()); - } - } - } - } + Self::handle_symbol_ref(symbol, reference, &mut local_reports); }, ReferenceMatches::WrongType(_) => //TODO: report mismatch, (), } } - }) + local_reports.into_par_iter() + }).collect::>()); } } @@ -1737,7 +1766,7 @@ fn add_new_symbol_from_shallow(maker: &SymbolMaker, param.declarations.iter() .map(|(_, def)|*def.loc_span()).collect()), DMLShallowObjectVariant::Method(method_ref) => - (vec![*method_ref.get_base().location()], + (method_ref.get_bases().iter().map(|b|*b.location()).collect(), method_ref.get_all_defs(), method_ref.get_all_decls()), DMLShallowObjectVariant::Constant(constant) => diff --git a/src/analysis/templating/methods.rs b/src/analysis/templating/methods.rs index b22b69e3..74076aa9 100644 --- a/src/analysis/templating/methods.rs +++ b/src/analysis/templating/methods.rs @@ -294,6 +294,16 @@ pub enum DMLMethodRef { } impl DMLMethodRef { + pub fn get_disambiguation_name(&self) -> Option { + match self { + DMLMethodRef::TraitMethod(trt, _) => + Some(format!("templates.{}.{}", + trt.name, + self.identity())), + DMLMethodRef::ConcreteMethod(_) => None, + } + } + pub fn get_decl(&self) -> &MethodDecl { match self { Self::TraitMethod(_, decl) => decl, @@ -301,11 +311,11 @@ impl DMLMethodRef { } } - pub fn get_default(&self) -> Option> { + pub fn get_default(&self) -> Option { match self { Self::TraitMethod(_, _) => None, Self::ConcreteMethod(decl) => decl.default_call - .as_ref().map(Arc::clone), + .as_ref().cloned(), } } @@ -329,19 +339,22 @@ impl DMLMethodRef { DMLMethodRef::ConcreteMethod(concmeth) => concmeth.get_all_defs(), } } - pub fn get_base(&self) -> MethodDecl { - match self { + // Invariant: Return is non-empty + pub fn get_bases(&self) -> Vec { + let to_return = match self { DMLMethodRef::TraitMethod(_, decl) => { - decl.clone() + vec![decl.clone()] }, DMLMethodRef::ConcreteMethod(meth) => { - if let Some(default) = &meth.default_call { - default.get_base() + if let Some(default_ref) = &meth.default_call { + default_ref.get_bases() } else { - meth.decl.clone() + vec![meth.decl.clone()] } } - } + }; + assert!(!to_return.is_empty()); + to_return } } @@ -414,14 +427,53 @@ impl DeclarationSpan for DMLMethodRef { DMLMethodRef::ConcreteMethod(decl) => decl.span(), } } - } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum DefaultCallReference { + Valid(Arc), + Ambiguous(Vec>), +} + +impl DefaultCallReference { + pub fn get_bases(&self) -> Vec { + match self { + DefaultCallReference::Valid(method) => method.get_bases(), + DefaultCallReference::Ambiguous(methods) => + methods.iter().flat_map(|m|m.get_bases()).collect(), + } + } + + pub fn get_all_decls(&self) -> Vec { + match self { + DefaultCallReference::Valid(method) => method.get_all_decls(), + DefaultCallReference::Ambiguous(methods) => + methods.iter().flat_map(|m|m.get_all_decls()).collect(), + } + } + + pub fn get_all_defs(&self) -> Vec { + match self { + DefaultCallReference::Valid(method) => method.get_all_defs(), + DefaultCallReference::Ambiguous(methods) => + methods.iter().flat_map(|m|m.get_all_defs()).collect(), + } + } + + pub fn as_valid(&self) -> Option<&Arc> { + match self { + DefaultCallReference::Valid(method) => Some(method), + DefaultCallReference::Ambiguous(_) => None, + } + } +} // This is roughly equivalent with a non-codegenned method in DMLC #[derive(Debug, Clone, PartialEq, Eq)] pub struct DMLConcreteMethod { pub decl: MethodDecl, // where the default call goes (None if invalid) - pub default_call: Option>, + pub default_call: Option, } impl DMLConcreteMethod { diff --git a/src/analysis/templating/objects.rs b/src/analysis/templating/objects.rs index f8a516ea..166d1497 100644 --- a/src/analysis/templating/objects.rs +++ b/src/analysis/templating/objects.rs @@ -28,9 +28,7 @@ use crate::analysis::structure::toplevel::{ExistCondition, ObjectDecl, use crate::analysis::symbols::{DMLSymbolKind}; use crate::analysis::templating::Declaration; use crate::analysis::templating::types::eval_type; -use crate::analysis::templating::methods::{DMLMethodRef, - MethodDecl, MethodDeclaration, - DMLConcreteMethod}; +use crate::analysis::templating::methods::{DMLConcreteMethod, DMLMethodRef, DefaultCallReference, MethodDecl, MethodDeclaration}; use crate::analysis::templating::topology::{InEachStruct, InferiorVariant, Rank, RankDesc, RankDescKind, RankMaker, topsort}; @@ -1891,13 +1889,16 @@ fn add_methods(obj: &mut DMLCompositeObject, continue; } let defaults = default_map.get(method).unwrap(); - let default = if defaults.len() == 1 { - let decl = defaults.iter().next().unwrap(); - trace!("Default call decl is {:?}", decl); - trace!("And the current map is {:?}", decl_to_method); - Some(decl_to_method.get(decl).unwrap()) - } else { - None + let default = match defaults.len() { + 1 => { + let decl = defaults.iter().next().unwrap(); + trace!("Default call decl is {:?}", decl); + trace!("And the current map is {:?}", decl_to_method); + Some(DefaultCallReference::Valid(Arc::clone(decl_to_method.get(decl).unwrap()))) + }, + 0 => None, + _ => Some(DefaultCallReference::Ambiguous( + defaults.iter().map(|d| Arc::clone(decl_to_method.get(d).unwrap())).collect())), }; trace!("Default call would be {:?}", default); for decl in default_map.get(method).unwrap() { @@ -1927,7 +1928,7 @@ fn add_methods(obj: &mut DMLCompositeObject, let new_method = Arc::new(DMLMethodRef::ConcreteMethod( DMLConcreteMethod { decl: (*method).clone(), - default_call: default.cloned(), + default_call: default, })); trace!("Inserted dependent methoddecl {:?}", new_method); decl_to_method.insert((*method).clone(), new_method); From fbf2505a44eea160925adbccd899d1fbf88152f7 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Wed, 14 Jan 2026 13:37:39 +0100 Subject: [PATCH 02/34] Refactor ReferenceMatches Allows better control of what is _semantically_ matched and what messages are reported to the user Signed-off-by: Jonatan Waern --- src/analysis/mod.rs | 133 +++++++++++++++++++++++++++++--------------- 1 file changed, 88 insertions(+), 45 deletions(-) diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index 369a4065..587bde8b 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -393,58 +393,98 @@ pub struct DeviceAnalysis { pub clientpath: PathBuf, } +#[derive(Debug, Clone, Eq, PartialEq)] +pub enum ReferenceMatchKind { + Found, + MismatchedFind, + NotFound, +} + #[derive(Debug, Clone)] -pub enum ReferenceMatches { - Found(HashSet), - WrongType(SymbolRef), - NotFound(HashSet), +pub struct ReferenceMatches { + pub kind: ReferenceMatchKind, + // How these references are interpreted depends on 'kind', + // for NotFound and MismatchedFind they are generally suggestions + // whereas for Found they are actual matches + pub references: HashSet, + // These will be reported as-is, regardless of kind + pub messages: Vec, } impl Default for ReferenceMatches { fn default() -> Self { - Self::NotFound(HashSet::default()) + ReferenceMatches { + kind: ReferenceMatchKind::NotFound, + references: HashSet::default(), + messages: vec![], + } } } impl ReferenceMatches { pub fn add_match(&mut self, reference: SymbolRef) { - if !matches!(self, Self::WrongType(_)) { - if let Self::Found(ref mut references) = self { - references.insert(reference); - } else { - *self = Self::Found(HashSet::from([reference])); - } + if self.kind != ReferenceMatchKind::MismatchedFind { + self.kind = ReferenceMatchKind::Found; + self.references.insert(reference); } } pub fn add_suggestion(&mut self, reference: SymbolRef) { - if let Self::NotFound(ref mut references) = self { - references.insert(reference); + if !matches!(self.kind, ReferenceMatchKind::Found + | ReferenceMatchKind::MismatchedFind) { + self.references.insert(reference); } } - pub fn set_wrongtype(&mut self, reference: SymbolRef) { - *self = Self::WrongType(reference); + pub fn set_mismatched(&mut self, reference: SymbolRef) { + self.kind = ReferenceMatchKind::MismatchedFind; + self.references.insert(reference); } pub fn new() -> Self { Self::default() } - pub fn has_matches(&self) -> bool { - matches!(self, Self::Found(_)) + pub fn as_matches(&self) -> Option> { + if self.kind == ReferenceMatchKind::Found { + Some(self.references.clone()) + } else { + None + } + } + + pub fn as_suggestions(&self) -> Option> { + if self.kind == ReferenceMatchKind::NotFound { + Some(self.references.clone()) + } else { + None + } + } + + pub fn as_mismatched(&self) -> Option> { + if self.kind == ReferenceMatchKind::MismatchedFind { + Some(self.references.clone()) + } else { + None + } } pub fn merge_with(&mut self, other: Self) { - match other { - Self::Found(syms) => for sym in syms { - self.add_match(sym); - }, - Self::NotFound(syms) => for sym in syms { - self.add_suggestion(sym); - }, - Self::WrongType(sym) => self.set_wrongtype(sym), + match other.kind { + ReferenceMatchKind::MismatchedFind => + for sym in other.references { + self.set_mismatched(sym); + }, + ReferenceMatchKind::Found => + for sym in other.references { + self.add_match(sym); + }, + ReferenceMatchKind::NotFound => + for sym in other.references { + self.add_suggestion(sym); + }, } + self.messages.extend(other.messages); } } @@ -1045,8 +1085,10 @@ impl DeviceAnalysis { let _: Vec<_> = objs.into_iter() .map(|o|self.lookup_def_in_obj(&o, sym.symbol, &mut refs)) .collect(); - if let ReferenceMatches::Found(syms) = refs { - syms.into_iter().collect() + // We can ignore messages from lookup defs here, as this lookup is + // live and reporting additional things from here makes no sense + if let Some(matches) = refs.as_matches() { + matches.into_iter().collect() } else { vec![] } @@ -1174,18 +1216,18 @@ impl DeviceAnalysis { subnode, method_structure, &mut intermediate_matches); - match intermediate_matches { - ReferenceMatches::Found(syms) => { - let wrapped_simple = NodeRef::Simple(simple.clone()); - for sym in syms { - self.resolve_noderef_in_symbol( - &sym, - &wrapped_simple, - method_structure, - ref_matches); - } - }, - other => ref_matches.merge_with(other), + + if let Some(syms) = intermediate_matches.as_matches() { + let wrapped_simple = NodeRef::Simple(simple.clone()); + for sym in syms { + self.resolve_noderef_in_symbol( + &sym, + &wrapped_simple, + method_structure, + ref_matches); + } + } else { + ref_matches.merge_with(intermediate_matches); } } } @@ -1308,7 +1350,7 @@ impl DeviceAnalysis { reference, method_structure, reference_matches); - if !reference_matches.has_matches() { + if reference_matches.as_matches().is_none() { let (_, new_chain) = context_chain.split_last().unwrap(); let sub_matches = self.find_target_for_reference(new_chain, reference, @@ -1390,8 +1432,8 @@ impl DeviceAnalysis { self.lookup_global_from_ref(glob), }; - match symbol_lookup { - ReferenceMatches::NotFound(_suggestions) => + match symbol_lookup.kind { + ReferenceMatchKind::NotFound => // TODO: report suggestions? // TODO: Uncomment reporting of errors here when // semantics are strong enough that they are rare @@ -1406,14 +1448,15 @@ impl DeviceAnalysis { // This maps symbols->references, this is later // used to create the inverse map // (not done here because of ownership issues) - ReferenceMatches::Found(symbols) => - for symbol in &symbols { + ReferenceMatchKind::Found => + for symbol in &symbol_lookup.references { Self::handle_symbol_ref(symbol, reference, &mut local_reports); }, - ReferenceMatches::WrongType(_) => + ReferenceMatchKind::MismatchedFind => //TODO: report mismatch, (), } + local_reports.extend(symbol_lookup.messages); } local_reports.into_par_iter() }).collect::>()); From 5dbcfa4fb065ed981befe3568f831ba181a0f48a Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Fri, 16 Jan 2026 11:48:16 +0100 Subject: [PATCH 03/34] Change debug level of some prints Signed-off-by: Jonatan Waern --- src/actions/analysis_queue.rs | 12 ++++++------ src/actions/analysis_storage.rs | 10 +++++----- src/actions/hover.rs | 2 +- src/actions/mod.rs | 10 +++++----- src/actions/requests.rs | 2 +- src/analysis/mod.rs | 6 +++--- src/analysis/parsing/parser.rs | 12 ++++++------ src/analysis/scope.rs | 14 +++++++------- src/analysis/templating/objects.rs | 8 ++++---- src/analysis/templating/topology.rs | 2 +- src/file_management.rs | 2 +- src/server/mod.rs | 20 ++++++++++---------- 12 files changed, 50 insertions(+), 50 deletions(-) diff --git a/src/actions/analysis_queue.rs b/src/actions/analysis_queue.rs index 2ffd15c5..c190bf74 100644 --- a/src/actions/analysis_queue.rs +++ b/src/actions/analysis_queue.rs @@ -257,7 +257,7 @@ impl AnalysisQueue { || !device_lock.is_empty() || !isolated_lock.is_empty(); if has_work { - debug!("Queue still has work ({:?}, {:?}, {:?})", + trace!("Queue still has work ({:?}, {:?}, {:?})", queue_lock, device_lock, isolated_lock); } has_work @@ -296,7 +296,7 @@ impl AnalysisQueue { for job in queue_lock.iter() { if let QueuedJob::IsolatedAnalysisJob(ijob) = job { if paths.contains(&ijob.path) { - debug!("Detected there is still isolated \ + trace!("Detected there is still isolated \ work in queue on {:?}", &ijob.path); return true; @@ -308,7 +308,7 @@ impl AnalysisQueue { let isolated_lock = self.isolated_tracker.lock().unwrap(); for (_, path) in isolated_lock.iter() { if paths.contains(path) { - debug!("Detected there is still isolated \ + trace!("Detected there is still isolated \ work in-flight on {:?}", path); return true; @@ -326,7 +326,7 @@ impl AnalysisQueue { for job in queue_lock.iter() { if let QueuedJob::DeviceAnalysisJob(ijob) = job { if paths.contains(&ijob.root.path) { - debug!("Detected there is still device \ + trace!("Detected there is still device \ work in queue on {:?}", &ijob.root.path); return true; @@ -338,7 +338,7 @@ impl AnalysisQueue { let device_lock = self.device_tracker.lock().unwrap(); for (path, _) in device_lock.values() { if paths.contains(path) { - debug!("Detected there is still device \ + trace!("Detected there is still device \ work in-flight on {:?}", path); return true; @@ -613,7 +613,7 @@ impl LinterJob { self.file.clone())).ok(); }, Err(e) => { - trace!("Failed to create isolated linter analysis: {}", e); + debug!("Failed to create isolated linter analysis: {}", e); } } } diff --git a/src/actions/analysis_storage.rs b/src/actions/analysis_storage.rs index 34853b04..cbb3d84b 100644 --- a/src/actions/analysis_storage.rs +++ b/src/actions/analysis_storage.rs @@ -306,7 +306,7 @@ impl AnalysisStorage { }; contexts.insert(None); - debug!("Full contexts for {:?} are {:?}", path, contexts); + trace!("Full contexts for {:?} are {:?}", path, contexts); if self.get_isolated_analysis(path).map_or( false, |a|a.is_device_file()) { @@ -489,7 +489,7 @@ impl AnalysisStorage { pub fn update_analysis(&mut self, resolver: &PathResolver) { let mut device_analysis = vec![]; let mut results_holder = vec![]; - debug!("Updating stored analysises"); + trace!("Updating stored analysises"); for r in self.results.try_iter() { results_holder.push(r); } @@ -566,7 +566,7 @@ impl AnalysisStorage { |i|!timestamp_is_newer(timestamp, i.timestamp)) .unwrap_or(false)) { - debug!("was not invalidated by recent \ + trace!("was not invalidated by recent \ isolated analysis"); self.device_analysis.insert(canon_path, TimestampedStorage { @@ -628,7 +628,7 @@ impl AnalysisStorage { fn update_last_use(&self, path: &CanonPath) { if let Some(mut_lock) = self.last_use.get(path) { let now = SystemTime::now(); - debug!("Updated last-use of {} to {:?}", path.as_str(), now); + trace!("Updated last-use of {} to {:?}", path.as_str(), now); *mut_lock.lock().unwrap() = now; } } @@ -677,7 +677,7 @@ impl AnalysisStorage { if let Some(device_timestamp) = self.device_analysis.get(path) .map(|device|device.timestamp) { - debug!("Timestamp is {:?}", device_timestamp); + trace!("Timestamp is {:?}", device_timestamp); for dependee_path in self.all_dependencies(path, Some(path)) { // NOTE: This means that calling this function with a missing // isolated analysis will not tell you the device needs to be diff --git a/src/actions/hover.rs b/src/actions/hover.rs index 7fddb367..e149bbd0 100644 --- a/src/actions/hover.rs +++ b/src/actions/hover.rs @@ -24,6 +24,6 @@ pub fn tooltip( let hover_span = ctx.convert_pos_to_span(hover_file_path, params.position); // TODO: sort out how to handle hovers, and what info they should provide let contents = vec![]; - debug!("tooltip: contents.len: {}", contents.len()); + trace!("tooltip: contents.len: {}", contents.len()); Ok(Tooltip { contents, range: hover_span.range }) } diff --git a/src/actions/mod.rs b/src/actions/mod.rs index 1791e727..ecc24539 100644 --- a/src/actions/mod.rs +++ b/src/actions/mod.rs @@ -1183,10 +1183,10 @@ impl InitActionContext { response: sender, }; if self.check_wait(&wait) { - debug!("Wait {:?} was immediately completed", wait); + trace!("Wait {:?} was immediately completed", wait); Ok(self.check_wait_satisfied(&wait)) } else { - debug!("Wait {:?} needs to wait", wait); + trace!("Wait {:?} needs to wait", wait); self.add_state_wait(wait); loop { match receiver.recv() { @@ -1198,7 +1198,7 @@ impl InitActionContext { } fn check_wait(&self, wait: &AnalysisStateWaitDefinition) -> bool { - debug!("Checking wait {:?}", wait); + trace!("Checking wait {:?}", wait); let mut wait_done = true; let analysis = self.analysis.lock().unwrap(); let queue = &self.analysis_queue; @@ -1258,9 +1258,9 @@ impl InitActionContext { } if wait_done { - debug!("{:?} was done", wait); + trace!("{:?} was done", wait); } else { - debug!("{:?} not done", wait); + trace!("{:?} not done", wait); } wait_done } diff --git a/src/actions/requests.rs b/src/actions/requests.rs index ba12c75a..d3520450 100644 --- a/src/actions/requests.rs +++ b/src/actions/requests.rs @@ -558,7 +558,7 @@ impl RequestAction for GotoImplementation { let lsp_locations: Vec<_> = unique_locations.into_iter() .map(|l|ls_util::dls_to_location(&l)) .collect(); - trace!("Requested implementations are {:?}", lsp_locations); + debug!("Requested implementations are {:?}", lsp_locations); Ok(response_maybe_with_limitations( // NOTE: this ends up being the client-path, which is // actually what we want diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index 587bde8b..6c562c90 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -1421,7 +1421,7 @@ impl DeviceAnalysis { status.assert_alive(); let mut local_reports = vec![]; for reference in references { - debug!("In {:?}, Matching {:?}", context_chain, reference); + trace!("In {:?}, Matching {:?}", context_chain, reference); let symbol_lookup = match &reference { Reference::Variable(var) => self.find_target_for_reference( context_chain.as_slice(), @@ -1689,7 +1689,7 @@ fn objects_to_symbols(maker: &SymbolMaker, for obj in objects.values() { let new_symbol: SymbolRef = new_symbol_from_object(maker, obj); - debug!("Comp obj symbol is: {:?}", new_symbol); + debug!("Created comp obj symbol: {:?}", new_symbol); storage.object_symbols.insert(obj.key, new_symbol); for subobj in obj.components.values() { // Non-shallow objects will be handled by the iteration @@ -1826,7 +1826,7 @@ fn add_new_symbol_from_shallow(maker: &SymbolMaker, vec![*hook.loc_span()], vec![*hook.loc_span()]), }; - debug!("Made symbol for {:?}", shallow); + debug!("Made shallow symbol for {:?}", shallow); let new_sym = symbol_ref!( maker, *shallow.location(), diff --git a/src/analysis/parsing/parser.rs b/src/analysis/parsing/parser.rs index db64bfbd..466bb756 100644 --- a/src/analysis/parsing/parser.rs +++ b/src/analysis/parsing/parser.rs @@ -2,7 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 and MIT use logos::Lexer; use regex::Regex; -use log::debug; +use log::trace; use lazy_static::lazy_static; use crate::span::{Range, ZeroIndexed, Position}; @@ -129,13 +129,13 @@ impl ParseContext { let peek = fileparser.peek(); let (endpos, endtok) = match &peek { Some(tok) => { - debug!("Ended context at {:?}: got {}, expected {}", + trace!("Ended context at {:?}: got {}, expected {}", tok.range.end(), tok.kind.description(), description); (Some(tok.range.start()), Some(*tok)) }, None => { - debug!("Ended context at {:?}: got EOF, expected {}", + trace!("Ended context at {:?}: got EOF, expected {}", fileparser.get_position(), description); (Some(fileparser.get_position()), None) }, @@ -193,7 +193,7 @@ impl ParseContext { } else { // If nobody could've handled this token, // skip it and retry - debug!("Skipped token {} at {:?}, expected {}", + trace!("Skipped token {} at {:?}, expected {}", token.kind.description(), token.range, description); fileparser.skip(description); @@ -241,7 +241,7 @@ impl ParseContext { } else { // If nobody could've handled this token, // skip it and retry - debug!("Skipped token {} at {:?}, expected {}", + trace!("Skipped token {} at {:?}, expected {}", token.kind.description(), token.range, description); fileparser.skip(description); @@ -278,7 +278,7 @@ impl ParseContext { } else { // If nobody could've handled this token, // skip it and retry - debug!("Skipped token {} at {:?}, expected {}", + trace!("Skipped token {} at {:?}, expected {}", token.kind.description(), token.range, description); fileparser.skip(description); diff --git a/src/analysis/scope.rs b/src/analysis/scope.rs index 63fdd164..e82e4eb4 100644 --- a/src/analysis/scope.rs +++ b/src/analysis/scope.rs @@ -8,7 +8,7 @@ use crate::analysis::reference::{Reference}; use crate::analysis::structure::objects::Method; -use log::{debug}; +use log::{debug, trace}; pub trait Scope : DeclarationSpan + Named { // Need this to create default mappings @@ -24,17 +24,17 @@ pub trait Scope : DeclarationSpan + Named { fn reference_at_pos_inside(&self, pos: ZeroPosition) -> Option<&Reference> { self.defined_references().iter() - .find(|r|{debug!("Considering {:?}", r); + .find(|r|{trace!("Considering {:?}", r); r.loc_span().range.contains_pos(pos)}) } fn reference_at_pos(&self, pos: &ZeroFilePosition) -> Option<&Reference> { - debug!("Searching for references inside {:?}", self.create_context()); + trace!("Searching for references inside {:?}", self.create_context()); if let Some(our_ref) = self.reference_at_pos_inside(pos.position) { - debug!("Got {:?}", our_ref); + trace!("Got {:?}", our_ref); Some(our_ref) } else { - debug!("Recursed"); + trace!("Recursed"); for scope in &self.defined_scopes() { if scope.span().contains_pos(pos) { return scope.reference_at_pos(pos); @@ -219,7 +219,7 @@ impl <'t> ContextedSymbol<'t> { impl SymbolContext { pub fn lookup_symbol<'t>(&'t self, pos: &ZeroFilePosition) -> Option> { - debug!("Starting lookup in {:?}", self.get_name()); + debug!("Starting file-position lookup in {:?}", self.get_name()); let mut acc = vec![]; self.lookup_symbol_aux(pos, &mut acc).map( |sub|ContextedSymbol { @@ -234,7 +234,7 @@ impl SymbolContext { -> Option<&'t SimpleSymbol> { acc.push(&self.context); self.subsymbols.iter() - .find(|sub|{debug!("Considering {:?}, contains pos? {:?}", + .find(|sub|{trace!("Considering {:?}, contains pos? {:?}", sub, sub.contains_pos(pos)); sub.contains_pos(pos)}) diff --git a/src/analysis/templating/objects.rs b/src/analysis/templating/objects.rs index 166d1497..1a4235b5 100644 --- a/src/analysis/templating/objects.rs +++ b/src/analysis/templating/objects.rs @@ -330,7 +330,7 @@ pub fn make_device<'t>(path: &str, container, report); let device_obj = container.get(obj_key).unwrap(); - debug!("Device components are: {:?}", device_obj.components); + trace!("Device components are: {:?}", device_obj.components); export_invariants(toplevel, device_obj, container, report); device_obj } @@ -1945,7 +1945,7 @@ fn check_trait_overrides(obj: &DMLCompositeObject, container: &StructureContainer, report: &mut Vec) { debug!("Checking traits overrides on {:?}", obj.identity); - debug!("all symbols are: {:?}", + trace!("all symbols are: {:?}", obj.components.keys().collect::>()); fn report_collision(name: &str, srcloc: ZeroSpan, @@ -2188,7 +2188,7 @@ pub fn make_object(loc: ZeroSpan, add_template_specs(&mut obj_specs, &each_stmts); add_template_ineachs(&obj_specs, &mut each_stmts); - debug!("Has specs at {:?}", obj_specs.iter().map(|rc|rc.loc) + trace!("Has specs at {:?}", obj_specs.iter().map(|rc|rc.loc) .collect::>()); trace!("Complete specs are: {:?}", @@ -2206,7 +2206,7 @@ pub fn make_object(loc: ZeroSpan, saveds, sessions, methods, hooks, subobjs) = collect_symbols(¶meters, &obj_specs, report); - debug!("All local symbols are: {:?}", + trace!("All local symbols are: {:?}", symbols.keys().map(|k|k.as_str()).collect::>()); let new_obj_key = create_object_instance(Some(loc), diff --git a/src/analysis/templating/topology.rs b/src/analysis/templating/topology.rs index 484d152b..fa4de98d 100644 --- a/src/analysis/templating/topology.rs +++ b/src/analysis/templating/topology.rs @@ -309,7 +309,7 @@ pub fn dependencies<'t>(statements: &'t StatementSpec, InferiorVariant::Import(imp) => { let name = imp_map.get(&imp.obj) .map_or_else(||imp.obj.imported_name(), |s|s.as_str()); - debug!("Mapped {:?} to {:?}", imp.obj, name); + debug!("Mapped import {:?} to {:?}", imp.obj, name); inferior.insert(name, InferiorVariant::Import(imp)); if imp.cond == ExistCondition::Always { diff --git a/src/file_management.rs b/src/file_management.rs index 672218d5..cd2de599 100644 --- a/src/file_management.rs +++ b/src/file_management.rs @@ -138,7 +138,7 @@ impl PathResolver { fn try_path(path: PathBuf) -> Option { if let Ok(info) = fs::metadata(&path) { if info.is_file() { - debug!("Resolved {:?} to {:?}", path, + trace!("Resolved {:?} to {:?}", path, // Fairly sure this can never fail fs::canonicalize(&path).unwrap()); return CanonPath::from_path_buf(path); diff --git a/src/server/mod.rs b/src/server/mod.rs index c4243d63..582a9efd 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -330,7 +330,7 @@ impl LsService { let output = client_reader_output; let send = client_reader_send; loop { - debug!("Awaiting message"); + trace!("Awaiting message"); let msg_string = match msg_reader.read_message() { Some(m) => m, None => { @@ -346,11 +346,11 @@ impl LsService { trace!("Read a message `{}`", msg_string); match RawMessageOrResponse::try_parse(&msg_string) { Ok(RawMessageOrResponse::Message(rm)) => { - debug!("Parsed a message: {}", rm.method); + trace!("Parsed a message: {}", rm.method); send.send(ServerToHandle::ClientMessage(rm)).ok(); }, Ok(RawMessageOrResponse::Response(rr)) => { - debug!("Parsed a response: {}", rr.id); + trace!("Parsed a response: {}", rr.id); send.send(ServerToHandle::ClientResponse(rr)).ok(); }, Err(e) => { @@ -394,7 +394,7 @@ impl LsService { ServerToHandle::ExitCode(code) => return code, ServerToHandle::IsolatedAnalysisDone(path, context, requests) => { - debug!("Received isolated analysis of {:?}", path); + trace!("Received isolated analysis of {:?}", path); if let ActionContext::Init(ctx) = &mut self.ctx { // hack where we try to activate a device context // as early as we possibly can, unless device context @@ -445,14 +445,14 @@ impl LsService { } }, ServerToHandle::DeviceAnalysisDone(path) => { - debug!("Received device analysis of {:?}", path); + trace!("Received device analysis of {:?}", path); if let ActionContext::Init(ctx) = &mut self.ctx { ctx.report_errors(&self.output); ctx.check_state_waits(); } }, ServerToHandle::LinterDone(path) => { - debug!("Received linter analysis of {:?}", path); + trace!("Received linter analysis of {:?}", path); if let ActionContext::Init(ctx) = &mut self.ctx { ctx.report_errors(&self.output); } @@ -460,7 +460,7 @@ impl LsService { ServerToHandle::AnalysisRequest(importpath, context, source) => { if let ActionContext::Init(ctx) = &mut self.ctx { if !ctx.config.lock().unwrap().to_owned().suppress_imports { - debug!("Analysing imported file {}", + trace!("Analysing imported file {}", &importpath.to_str().unwrap()); ctx.isolated_analyze( &importpath, Some(source), context, &self.output); @@ -479,14 +479,14 @@ impl LsService { blocking_requests: $($br_action: ty),*; requests: $($request: ty),*; ) => { - debug!("Handling `{}`", $method); + trace!("Handling `{}`", $method); match $method.as_str() { $( <$n_action as LSPNotification>::METHOD => { let notification: Notification<$n_action> = msg.parse_as_notification()?; if let Ok(mut ctx) = self.ctx.inited() { - debug!("Notified: {}", $method); + trace!("Notified: {}", $method); if notification.dispatch(&mut ctx, self.output.clone()).is_err() { debug!("Error handling notification: {:?}", msg); } @@ -531,7 +531,7 @@ impl LsService { <$request as LSPRequest>::METHOD => { let request: Request<$request> = msg.parse_as_request()?; if let Ok(ctx) = self.ctx.inited() { - debug!("Unblockingly Requested: {}", $method); + trace!("Unblockingly Requested: {}", $method); self.dispatcher.dispatch(request, ctx); } else { From dc1c3ba4895548013d012687701d2f5ae8ba07f6 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 12 Jan 2026 13:55:18 +0100 Subject: [PATCH 04/34] Rework symbol methodology for methods Signed-off-by: Jonatan Waern --- src/actions/requests.rs | 54 ++---- src/analysis/mod.rs | 267 ++++++++++++++++++++--------- src/analysis/symbols.rs | 52 ++++-- src/analysis/templating/methods.rs | 8 + 4 files changed, 246 insertions(+), 135 deletions(-) diff --git a/src/actions/requests.rs b/src/actions/requests.rs index d3520450..0af33264 100644 --- a/src/actions/requests.rs +++ b/src/actions/requests.rs @@ -222,39 +222,10 @@ fn fp_to_symbol_refs } }, } + debug!("Resolved to symbols: {:?}", definitions); Ok(definitions) } -fn handle_default_remapping - (ctx: &InitActionContext, - symbols: &[SymbolRef], - fp: &ZeroFilePosition) -> Option> -{ - let analysis = ctx.analysis.lock().unwrap(); - let refr_opt = analysis.reference_at_pos(fp); - // NOTE: Because the call to this is preceded by a symbol lookup, - // it is probably safe to discard an error here - if let Ok(Some(refr)) = refr_opt { - if refr.to_string().as_str() == "default" { - // If we are at a defaut reference, - // remap symbol references to methods - // to the correct decl site, leave others as-is - return Some(symbols.iter() - .flat_map(|d|'sym: { - let sym = d.lock().unwrap(); - if sym.kind == DMLSymbolKind::Method { - if let Some(loc) = sym.default_mappings - .get(refr.loc_span()) { - break 'sym vec![*loc]; - } - } - vec![] - }).collect()); - } - } - None -} - /// The result of a deglob action for a single wildcard import. /// /// The `location` is the position of the wildcard. @@ -614,12 +585,9 @@ impl RequestAction for GotoDeclaration { let mut limitations = HashSet::new(); match fp_to_symbol_refs(&fp, &ctx, &mut limitations) { Ok(symbols) => { - let unique_locations = handle_default_remapping(&ctx, - &symbols, - &fp) - .unwrap_or_else(||symbols.into_iter() + let unique_locations = symbols.into_iter() .flat_map(|d|d.lock().unwrap().declarations.clone()) - .collect()); + .collect::>(); let lsp_locations = unique_locations.into_iter() .map(|l|ls_util::dls_to_location(&l)) .collect(); @@ -677,13 +645,9 @@ impl RequestAction for GotoDefinition { let mut limitations = HashSet::new(); match fp_to_symbol_refs(&fp, &ctx, &mut limitations) { Ok(symbols) => { - let unique_locations = handle_default_remapping(&ctx, - &symbols, - &fp) - .unwrap_or_else(||symbols.into_iter() + let unique_locations = symbols.into_iter() .flat_map(|d|d.lock().unwrap().definitions.clone()) - .collect() - ); + .collect::>(); let lsp_locations: Vec<_> = unique_locations.into_iter() .map(|l|ls_util::dls_to_location(&l)) @@ -704,7 +668,7 @@ impl RequestAction for GotoDefinition { } } -fn filter_out_unpointed_defaults(fp: &ZeroFilePosition, +/* fn filter_out_unpointed_defaults(fp: &ZeroFilePosition, symbols: &[SymbolRef]) -> HashSet { symbols @@ -721,7 +685,7 @@ fn filter_out_unpointed_defaults(fp: &ZeroFilePosition, }).cloned().collect::>() }) .collect() -} +} */ impl RequestAction for References { type Response = ResponseWithMessage>; @@ -761,7 +725,9 @@ impl RequestAction for References { let mut limitations = HashSet::new(); match fp_to_symbol_refs(&fp, &ctx, &mut limitations) { Ok(symbols) => { - let unique_locations = filter_out_unpointed_defaults(&fp, &symbols); + let unique_locations = symbols.into_iter() + .flat_map(|d|d.lock().unwrap().references.clone()) + .collect::>(); let lsp_locations: Vec<_> = unique_locations.into_iter() .map(|l|ls_util::dls_to_location(&l)) .collect(); diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index 6c562c90..c1502575 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -356,7 +356,9 @@ pub struct SymbolStorage { pub param_symbols: HashMap<(ZeroSpan, String), HashMap>, pub object_symbols: HashMap, - pub method_symbols: HashMap, + // This is doubly-indexed, by decl location and then + // by parent object key + pub method_symbols: HashMap>, // constants, sessions, saveds, hooks, method args pub variable_symbols: HashMap, } @@ -366,7 +368,7 @@ impl SymbolStorage { self.template_symbols.values() .chain(self.param_symbols.values().flat_map(|h|h.values())) .chain(self.object_symbols.values()) - .chain(self.method_symbols.values()) + .chain(self.method_symbols.values().flat_map(|h|h.values())) .chain(self.variable_symbols.values()) } } @@ -469,6 +471,10 @@ impl ReferenceMatches { } } + pub fn add_message(&mut self, message: DMLError) { + self.messages.push(message); + } + pub fn merge_with(&mut self, other: Self) { match other.kind { ReferenceMatchKind::MismatchedFind => @@ -840,7 +846,11 @@ impl DeviceAnalysis { DMLResolvedObject::ShallowObject(shallow) => match &shallow.variant { DMLShallowObjectVariant::Method(m) => - self.symbol_info.method_symbols.get(m.location()), + // For resolutions, a methods symbols is just the symbol + // of the method we resolved to + self.symbol_info.method_symbols + .get(m.location()) + .and_then(|m|m.get(&parent.key)), DMLShallowObjectVariant::Session(s) | DMLShallowObjectVariant::Saved(s) => self.symbol_info.variable_symbols.get(s.loc_span()), @@ -1117,7 +1127,10 @@ impl DeviceAnalysis { self.resolve_noderef_in_obj(&obj_copy, node, method_structure, - ref_matches) + ref_matches); + }, + SymbolSource::Method(key, method) => { + self.resolve_noderef_in_method(key, method, node, method_structure, ref_matches); }, // TODO: Cannot be resolved without constant folding SymbolSource::MethodArg(_method, _name) => (), @@ -1129,8 +1142,20 @@ impl DeviceAnalysis { } } + fn get_method_symbol(&self, + method: &Arc, + parent_obj_key: &StructureKey) + -> Option<&SymbolRef> { + let to_ret = self.symbol_info.method_symbols.get(method.location()) + .and_then(|m|m.get(parent_obj_key)); + if to_ret.is_none() { + internal_error!("Missing method symbol for method ref {:?}", method); + } + to_ret + } + fn resolve_simple_noderef_in_method<'c>(&'c self, - obj: &DMLShallowObject, + parent_key: &StructureKey, meth: &Arc, node: &DMLString, _type_hint: Option<()>, @@ -1148,18 +1173,45 @@ impl DeviceAnalysis { "this" => ref_matches.add_match(Arc::clone( self.symbol_info.object_symbols.get( - &obj.parent).unwrap())), + parent_key).unwrap())), "default" => - // NOTE: This is part of the hack that maps default - // references in methods to the corret method decl. - // Here, we simply check if the method has any - // default call, and if so map the reference to the - // method symbol - if meth.get_default().is_some() { - if let Some(s) = self.symbol_info.method_symbols - .get(obj.location()) { - ref_matches.add_match(Arc::clone(s)) + // NOTE: Here we match a default ref to the symbol of the method decl + // that we directly overrode. Meaning that further lookups on that symbol + // will be specialized for that overriding chain + if let Some(defref) = meth.get_default() { + match defref { + DefaultCallReference::Valid(refr) => { + if let Some(s) = self.get_method_symbol(&refr, parent_key) { + ref_matches.add_match(Arc::clone(s)); + } + }, + DefaultCallReference::Ambiguous(refs) => { + // TODO/NOTE: These are currently unused, but since we want to mark a mismatch anyway may as well + // pass all these references in + for reference in &refs { + if let Some(s) = self.get_method_symbol(reference, parent_key) { + ref_matches.set_mismatched(Arc::clone(s)); + } + } + let ambiguous_desc: &'static str + = "Ambiguous default call, you may need to clarify the template ordering or use a template-qualified-method-implementation-call"; + // TODO: curren + ref_matches.add_message(DMLError { + span: *node.span(), + description: ambiguous_desc.to_string(), + severity: Some(DiagnosticSeverity::ERROR), + related: refs + .iter() + .map(|d|(*d.location(), + format!("Possible candidate for default call{}", + d.get_disambiguation_name() + .map_or_else(||"".to_string(), + |n|format!(", can be explicitly called as 'this.{}'", n))) + )) + .collect(), + }); } + } } else { // fairly sure 'default' cannot be a // reference otherwise @@ -1184,6 +1236,43 @@ impl DeviceAnalysis { } } + fn resolve_noderef_in_method<'c>(&'c self, + parent_key: &StructureKey, + method: &Arc, + node: &NodeRef, + method_structure: &HashMap, + ref_matches: &mut ReferenceMatches) { + match node { + NodeRef::Simple(simple) => { + self.resolve_simple_noderef_in_method(parent_key, method, simple, + None, + method_structure, + ref_matches); + }, + NodeRef::Sub(subnode, simple, _) => { + let mut intermediate_matches = ReferenceMatches::new(); + self.resolve_noderef_in_method(parent_key, + method, + subnode, + method_structure, + &mut intermediate_matches); + if let Some(syms) = intermediate_matches.as_matches() { + let wrapped_simple = NodeRef::Simple(simple.clone()); + for sym in syms { + self.resolve_noderef_in_symbol( + &sym, + &wrapped_simple, + method_structure, + ref_matches); + } + } else { + ref_matches.merge_with(intermediate_matches); + } + } + } + } + fn resolve_noderef_in_obj<'c>(&'c self, obj: &DMLObject, node: &NodeRef, @@ -1199,7 +1288,9 @@ impl DeviceAnalysis { .. }) = resolvedobj { - self.resolve_simple_noderef_in_method(shallow, m, simple, + self.resolve_simple_noderef_in_method(&shallow.parent, + m, + simple, None, method_structure, ref_matches); @@ -1361,48 +1452,9 @@ impl DeviceAnalysis { } fn handle_symbol_ref(symbol: &SymbolRef, - reference: &Reference, - report: &mut Vec) { - let ambiguous_desc: &'static str - = "Ambiguous default call, you may need to clarify the template ordering or use a template-qualified-method-implementation-call"; - + reference: &Reference) { let mut sym = symbol.lock().unwrap(); sym.references.insert(*reference.loc_span()); - if let Some(meth) = sym.source - .as_object() - .and_then(DMLObject::as_shallow) - .and_then(|s|s.variant.as_method()) { - if let Some(var) = reference.as_variable_ref() { - if var.reference.to_string().as_str() - == "default" { - if let Some(default_decl_ref) = meth.get_default() { - match default_decl_ref { - DefaultCallReference::Valid(default_decl) => - { - sym.default_mappings.insert( - *var.loc_span(), - *default_decl.location()); - }, - DefaultCallReference::Ambiguous(ambiguous_decls) => - report.push(DMLError { - span: *var.loc_span(), - description: ambiguous_desc.to_string(), - severity: Some(DiagnosticSeverity::ERROR), - related: ambiguous_decls - .iter() - .map(|d|(*d.location(), - format!("Possible candidate for default call{}", - d.get_disambiguation_name() - .map_or_else(||"".to_string(), - |n|format!(", can be explicitly called as 'this.{}'", n))) - )) - .collect(), - }), - } - } - } - } - } } #[allow(clippy::ptr_arg)] @@ -1450,7 +1502,7 @@ impl DeviceAnalysis { // (not done here because of ownership issues) ReferenceMatchKind::Found => for symbol in &symbol_lookup.references { - Self::handle_symbol_ref(symbol, reference, &mut local_reports); + Self::handle_symbol_ref(symbol, reference); }, ReferenceMatchKind::MismatchedFind => //TODO: report mismatch, @@ -1748,6 +1800,7 @@ fn new_symbol_from_object(maker: &SymbolMaker, SymbolSource::DMLObject(DMLObject::CompObject(object.key)), definitions = all_decl_defs.clone(), declarations = all_decl_defs.clone(), + // TODO: this does not follow from the new definition of bases bases = all_decl_defs) } @@ -1765,14 +1818,13 @@ fn new_symbol_from_arg(maker: &SymbolMaker, bases = bases, definitions = definitions, declarations = declarations - ) } fn log_non_same_insert(map: &mut HashMap, key: K, val: SymbolRef) -> bool -where K: std::hash::Hash + Eq + Clone, +where K: std::hash::Hash + Eq + Clone + std::fmt::Debug, { // NOTE: We should not need to do these comparisons, when // object symbol creation is properly guided by structural AST @@ -1786,14 +1838,49 @@ where K: std::hash::Hash + Eq + Clone, if !old.lock().unwrap().equivalent( &map.get(&key).unwrap().lock().unwrap()) { internal_error!( - "Overwrote previous symbol {:?} with non-similar symbol {:?}", - old, map.get(&key)); + "Overwrote previous symbol {:?} at {:?} with non-similar symbol {:?}", + old, key, map.get(&key)); return true; } } false } +// The current strategy for method symbols is: +// Create a symbol for each level of overriding for each object where the method +// is actualized. We then end up with many symbols for the same decl location, +// and leave it to requests to collect the aggregate information at the point +fn add_new_symbol_from_method(maker: &SymbolMaker, parent_obj_key: &StructureKey, method_ref: &Arc, errors: &mut Vec, storage: &mut SymbolStorage, method_structure: &mut HashMap) { + let (bases, definitions, declarations) = ( + method_ref.get_bases().iter().map(|b|*b.location()).collect(), + vec![*method_ref.get_decl().location()], + vec![*method_ref.get_decl().location()], + ); + debug!("Made symbol for method {:?}", method_ref); + let new_sym = symbol_ref!( + maker, + *method_ref.location(), + DMLSymbolKind::Method, + SymbolSource::Method(*parent_obj_key, Arc::clone(method_ref)), + bases = bases, + definitions = definitions, + declarations = declarations); + let insert_at = storage.method_symbols.entry(*method_ref.location()) + .or_default(); + if !log_non_same_insert(insert_at, *parent_obj_key, new_sym) { + for arg in method_ref.args() { + let new_argsymbol = new_symbol_from_arg(maker, method_ref, arg); + log_non_same_insert(&mut storage.variable_symbols, *arg.loc_span(), new_argsymbol); + } + add_method_scope_symbols(maker, method_ref, method_structure, storage, errors); + if let Some(defaults) = method_ref.get_default() { + for default in defaults.flat_refs() { + add_new_symbol_from_method(maker, parent_obj_key, default, errors, storage, method_structure); + } + } + } +} + #[allow(clippy::ptr_arg)] fn add_new_symbol_from_shallow(maker: &SymbolMaker, shallow: &DMLShallowObject, @@ -1809,9 +1896,7 @@ fn add_new_symbol_from_shallow(maker: &SymbolMaker, param.declarations.iter() .map(|(_, def)|*def.loc_span()).collect()), DMLShallowObjectVariant::Method(method_ref) => - (method_ref.get_bases().iter().map(|b|*b.location()).collect(), - method_ref.get_all_defs(), - method_ref.get_all_decls()), + return add_new_symbol_from_method(maker, &shallow.parent, method_ref, errors, storage, method_structure), DMLShallowObjectVariant::Constant(constant) => (vec![*constant.loc_span()], vec![*constant.loc_span()], @@ -1848,21 +1933,9 @@ fn add_new_symbol_from_shallow(maker: &SymbolMaker, shallow.parent, new_sym); }, - DMLShallowObjectVariant::Method(method_ref) => - if !log_non_same_insert(&mut storage.method_symbols, - *shallow.location(), - new_sym) { - for arg in method_ref.args() { - let new_argsymbol = new_symbol_from_arg(maker, - method_ref, - arg); - log_non_same_insert(&mut storage.variable_symbols, - *arg.loc_span(), - new_argsymbol); - } - add_method_scope_symbols(maker, method_ref, method_structure, - storage, errors); - }, + DMLShallowObjectVariant::Method(method_ref) => { + internal_error!("Unreachable method_ref case reached, ignored. ({:?})", method_ref); + }, DMLShallowObjectVariant::Constant(_) | DMLShallowObjectVariant::Session(_) | DMLShallowObjectVariant::Saved(_) | @@ -2303,6 +2376,10 @@ impl DeviceAnalysis { &container, &mut errors, &mut method_structure); + // This needs to be done after all symbols are created, because method + // symbol order is not correlted to the object iteration order + bind_method_implementations(&mut symbol_info.method_symbols); + status.assert_alive(); // TODO: how do we store type info? extend_with_templates(&maker, &mut symbol_info, &tt_info); @@ -2407,3 +2484,35 @@ pub fn from_device_and_bases<'a>(_device: &'a IsolatedAnalysis, toplevels.iter().flat_map( |tl|&tl.templates).collect() } + +fn bind_method_implementations(method_symbols: &mut HashMap>) { + debug!("Bind method implementations"); + for method_symbol in method_symbols.values().flat_map(|m| m.values()) { + debug!("Binding for {}", method_symbol.lock().unwrap().medium_debug_info()); + // Cloning the arc is not strictly necessary, but avoiding holding the lock is good practice + let method = match &method_symbol.lock().unwrap().source { + SymbolSource::Method(_, methref) => Arc::clone(methref), + _ => { + internal_error!("Method symbol {:?} did not have method symbol source", method_symbol); + continue; + } + }; + let default_decls = method.get_default().into_iter() + .flat_map(|d|d.flat_refs().into_iter().cloned().collect::>()) + .collect::>(); + debug!("Default decls are {:?}", default_decls); + let parent_symbols = default_decls.into_iter() + .filter_map(|d| + if let Some(parent_syms) = method_symbols.get(d.location()) { + Some(parent_syms) + } else { + internal_error!("Method symbol {:?} did not have method symbol source", d); + None + }) + .flat_map(|ps|ps.values()); + for parent in parent_symbols { + debug!("Inserted into parent {}", parent.lock().unwrap().medium_debug_info()); + parent.lock().unwrap().implementations.insert(*method.location()); + } + } +} diff --git a/src/analysis/symbols.rs b/src/analysis/symbols.rs index 487faf4f..d74a5720 100644 --- a/src/analysis/symbols.rs +++ b/src/analysis/symbols.rs @@ -2,7 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 and MIT use std::sync::Arc; -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use std::sync::Mutex; use crate::analysis::{Named, LocationSpan}; @@ -11,7 +11,7 @@ use crate::analysis::parsing::tree::ZeroSpan; use crate::analysis::structure::objects::{CompObjectKind}; use crate::analysis::structure::expressions::DMLString; -use crate::analysis::templating::objects::DMLObject; +use crate::analysis::templating::objects::{DMLObject, DMLNamedMember, StructureKey}; use crate::analysis::templating::methods::{DMLMethodRef}; use crate::analysis::templating::types::DMLResolvedType; use crate::analysis::templating::traits::DMLTemplate; @@ -112,6 +112,10 @@ impl SimpleSymbol { #[derive(Debug, Clone, Eq, PartialEq)] pub enum SymbolSource { DMLObject(DMLObject), + // (key of containing object, method ref) + // TODO: same principle for shared methods? likely not, there is + // no object for the topmost method + Method(StructureKey, Arc), MethodArg(Arc, DMLString), MethodLocal(Arc, DMLString), // TODO: RC this if it's expensive @@ -153,6 +157,17 @@ impl SymbolSource { (_, _) => self == other } } + + pub fn maybe_name(&self) -> Option<&str> { + match self { + SymbolSource::DMLObject(obj) => obj.as_shallow().map(|o| o.identity()), + SymbolSource::Method(_, methref) => Some(methref.identity()), + SymbolSource::MethodArg(_, name) => Some(&name.val), + SymbolSource::MethodLocal(_, name) => Some(&name.val), + SymbolSource::Type(_) => None, + SymbolSource::Template(templ) => Some(&templ.name), + } + } } // Intentionally omitted implementation of clone, should not be done @@ -162,22 +177,24 @@ pub struct Symbol { pub id: SymbolID, pub loc: ZeroSpan, pub kind: DMLSymbolKind, + // Bases are the topmost declarations of this symbol + pub bases: Vec, + // Definitions are the sites which define at least part of this symbol pub definitions: Vec, + // Declarations are the sites which declare this symbol pub declarations: Vec, + // References are all locations that may refer to this symbol pub references: HashSet, // NOTE: The meaning of 'implementation' varies with symbol kind - // For methods and interfaces, this is straightforward - // For templates, it will give all declarations for all objects that - // directly or indirectly implement the template + // - For interfaces this is straightforward + // - For templates it will give all direct instantiations of the template + // - For methods, it gives all methods that directly override this method, + // note that when presented to the user, we give both direct and indirect overrides // TODO: For objects, we could give the locations of in-eachs that affect it // there is no way to go to these specs without finding definitions of // some param or method set in them - pub implementations: Vec, - pub bases: Vec, + pub implementations: HashSet, pub source: SymbolSource, - // Used for method symbols only, maps default references - // to the method_decl they should resolve to - pub default_mappings: HashMap, // TODO: RC or box this if it's expensive pub typed: Option, } @@ -286,10 +303,21 @@ impl Symbol { definitions: Vec::default(), declarations: Vec::default(), references: HashSet::default(), - implementations: Vec::default(), + implementations: HashSet::default(), bases: Vec::default(), - default_mappings: HashMap::default(), typed: None, } } + + // Prints appropriate info about a method without clogging the output + // with minutiae for symbols that may contain lots of sub-data (methods) + pub fn medium_debug_info(&self) -> String { + format!( + "Symbol({}, {:?} at loc: {:?})", + self.source.maybe_name() + .map_or(format!("id={}", self.id), + |name| format!("{} (id={})", name, self.id)), + self.kind, + self.loc) + } } diff --git a/src/analysis/templating/methods.rs b/src/analysis/templating/methods.rs index 74076aa9..a3e3c884 100644 --- a/src/analysis/templating/methods.rs +++ b/src/analysis/templating/methods.rs @@ -466,6 +466,14 @@ impl DefaultCallReference { DefaultCallReference::Ambiguous(_) => None, } } + + pub fn flat_refs(&self) -> Vec<&Arc> { + match self { + DefaultCallReference::Valid(method) => vec![method], + DefaultCallReference::Ambiguous(methods) => + methods.iter().collect() + } + } } // This is roughly equivalent with a non-codegenned method in DMLC From a0566de1784006bcbc0c4ca90b8b0c3bde00ee38 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Fri, 16 Jan 2026 14:35:44 +0100 Subject: [PATCH 05/34] Rework how fp-based symbol lookup works Ditch the structural lookup, just iterate over all symbols and grab the ones that match Signed-off-by: Jonatan Waern --- src/actions/analysis_storage.rs | 2 +- src/actions/requests.rs | 38 +++++++++++++++++---------------- src/analysis/mod.rs | 20 +++++++++++++++++ src/span/mod.rs | 2 +- 4 files changed, 42 insertions(+), 20 deletions(-) diff --git a/src/actions/analysis_storage.rs b/src/actions/analysis_storage.rs index cbb3d84b..bafbc501 100644 --- a/src/actions/analysis_storage.rs +++ b/src/actions/analysis_storage.rs @@ -16,7 +16,7 @@ use std::time::{Duration, SystemTime}; use crate::actions::ContextDefinition; use crate::analysis::scope::{ContextedSymbol, ContextKey}; use crate::analysis::structure::objects::Import; -use crate::analysis::{IsolatedAnalysis, DeviceAnalysis, DMLError}; +use crate::analysis::{DMLError, DeviceAnalysis, IsolatedAnalysis}; use crate::lsp_data::*; use crate::analysis::parsing::tree::{ZeroSpan, ZeroFilePosition}; diff --git a/src/actions/requests.rs b/src/actions/requests.rs index 0af33264..386e33cb 100644 --- a/src/actions/requests.rs +++ b/src/actions/requests.rs @@ -149,39 +149,41 @@ fn fp_to_symbol_refs // returns different information for "no symbols found" and // "no info at pos" debug!("Looking up symbols/references at {:?}", fp); - let (context_sym, reference) = (analysis.context_symbol_at_pos(fp)?, - analysis.reference_at_pos(fp)?); - debug!("Got {:?} and {:?}", context_sym, reference); + let reference = analysis.reference_at_pos(fp)?; + debug!("Got reference {:?}", reference); + let canon_path = CanonPath::from_path_buf(fp.path()) .ok_or(AnalysisLookupError::NoFile)?; + + let analysises = analysis.all_device_analysises_containing_file(&canon_path); + if analysises.is_empty() { + return Err(AnalysisLookupError::NoDeviceAnalysis); + } + let context_sym = analysis.context_symbol_at_pos(fp)?; + let symbols = context_sym.map(|cs| { + analysises.into_iter().flat_map(|a|a.lookup_symbols(&cs, relevant_limitations)).collect::>() + }); + if let Some(syms) = &symbols { + debug!("Got symbols {:?}", syms.iter().map(|s|s.lock().unwrap().medium_debug_info()).collect::>()); + } + // Rather than holding the lock throughout the request, clone // the filter let filter = Some(ctx.device_active_contexts.lock().unwrap().clone()); let mut definitions = vec![]; - let analysises = analysis.all_device_analysises_containing_file( - &canon_path); - if analysises.is_empty() { - return Err(AnalysisLookupError::NoDeviceAnalysis); - } - match (context_sym, reference) { + + match (symbols, reference) { (None, None) => { debug!("No symbol or reference at point"); return Ok(vec![]); }, - (Some(sym), refer) => { + (Some(syms), refer) => { if refer.is_some() { error!("Obtained both symbol and reference at {:?}\ (reference is {:?}), defaulted to symbol", &fp, refer); } - for device in analysis.filtered_device_analysises_containing_file( - &canon_path, - filter.as_ref()) { - definitions.extend( - device.lookup_symbols_by_contexted_symbol( - &sym, relevant_limitations) - .into_iter()); - } + definitions.extend(syms); }, (None, Some(refr)) => { debug!("Mapping {:?} to symbols", refr.loc_span()); diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index c1502575..8c635d42 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -611,6 +611,26 @@ pub fn isolated_template_limitation(template_name: &str) -> DLSLimitation { } impl DeviceAnalysis { + pub fn lookup_symbols<'t>(&self, context_sym: &ContextedSymbol<'t>, limitations: &mut HashSet) -> Vec { + trace!("Looking up symbols at {:?}", context_sym); + + // Special handling for methods, since each method decl has its + // own symbol + if context_sym.kind() == DMLSymbolKind::Method { + let mut result = vec![]; + for sym in self.symbol_info.method_symbols.values().flat_map(|m|m.values()) { + let sym_lock = sym.lock().unwrap(); + if sym_lock.declarations.iter().any( + |decl|decl == context_sym.loc_span()) { + trace!("Found symbol {:?} at for {:?}", sym_lock, context_sym.loc_span()); + result.push(Arc::clone(sym)); + } + } + return result; + } + self.lookup_symbols_by_contexted_symbol(context_sym, limitations) + } + pub fn get_device_obj(&self) -> &DMLObject { &self.device_obj } diff --git a/src/span/mod.rs b/src/span/mod.rs index 18a979f4..f2b4eefc 100644 --- a/src/span/mod.rs +++ b/src/span/mod.rs @@ -528,7 +528,7 @@ impl Span { impl Span { pub fn contains_pos(&self, pos: &FilePosition) -> bool { - self.range.contains_pos(pos.position) + self.file == pos.file && self.range.contains_pos(pos.position) } } From 1a0f5b5f611df01767c7878bdbc18a29cc644867 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Fri, 16 Jan 2026 15:54:14 +0100 Subject: [PATCH 06/34] Rework how abstract methods and template methods work Signed-off-by: Jonatan Waern --- src/analysis/mod.rs | 22 ++-- src/analysis/structure/objects.rs | 6 ++ src/analysis/templating/methods.rs | 145 +++++++++++-------------- src/analysis/templating/objects.rs | 131 +++++++++++------------ src/analysis/templating/traits.rs | 163 +++++++++++++++++++++++------ 5 files changed, 267 insertions(+), 200 deletions(-) diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index 8c635d42..a4c026c7 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -1201,14 +1201,14 @@ impl DeviceAnalysis { if let Some(defref) = meth.get_default() { match defref { DefaultCallReference::Valid(refr) => { - if let Some(s) = self.get_method_symbol(&refr, parent_key) { + if let Some(s) = self.get_method_symbol(refr, parent_key) { ref_matches.add_match(Arc::clone(s)); } }, DefaultCallReference::Ambiguous(refs) => { // TODO/NOTE: These are currently unused, but since we want to mark a mismatch anyway may as well // pass all these references in - for reference in &refs { + for reference in refs { if let Some(s) = self.get_method_symbol(reference, parent_key) { ref_matches.set_mismatched(Arc::clone(s)); } @@ -1231,6 +1231,9 @@ impl DeviceAnalysis { .collect(), }); } + _ => { + // 'default' when overriding nothing is an error, similar to the else-case below + } } } else { // fairly sure 'default' cannot be a @@ -1978,15 +1981,14 @@ fn add_method_scope_symbols(maker: &SymbolMaker, symbols: HashMap::default(), sub_ranges: vec![], }; - if !matches!(&*method.get_decl().body, StatementKind::Compound(_)) { - error!("Internal Error: Method body was not a compound statement"); + if matches!(&*method.get_decl().body, StatementKind::Compound(_)) { + add_new_method_scope_symbols(maker, + method, + &method.get_decl().body, + errors, + storage, + &mut entry); } - add_new_method_scope_symbols(maker, - method, - &method.get_decl().body, - errors, - storage, - &mut entry); if !entry.is_empty() { method_structure.insert( *method.get_decl().location(), diff --git a/src/analysis/structure/objects.rs b/src/analysis/structure/objects.rs index b69be6a0..6e34f51a 100644 --- a/src/analysis/structure/objects.rs +++ b/src/analysis/structure/objects.rs @@ -1014,6 +1014,12 @@ impl StructureSymbol for Method { } } +impl MaybeAbstract for Method { + fn is_abstract(&self) -> bool { + matches!(self.body.as_ref(), StatementKind::Empty(_)) + } +} + impl Scope for Method { fn create_context(&self) -> ContextKey { ContextKey::Method(SimpleSymbol::make(self, self.kind())) diff --git a/src/analysis/templating/methods.rs b/src/analysis/templating/methods.rs index a3e3c884..8c467dd2 100644 --- a/src/analysis/templating/methods.rs +++ b/src/analysis/templating/methods.rs @@ -163,6 +163,7 @@ impl DeclarationSpan for MethodDecl { pub trait MethodDeclaration : DMLNamedMember + MaybeAbstract { fn is_shared(&self) -> bool; + fn is_default(&self) -> bool; fn throws(&self) -> bool; fn fully_typed(&self) -> bool { for arg in self.args() { @@ -175,12 +176,27 @@ pub trait MethodDeclaration : DMLNamedMember + MaybeAbstract { fn args(&self) -> &Vec; fn returns(&self) -> &Vec; + // TODO: Let this take multiple overridden methods, as there are allowed cases + // of that and we can provide better (fewer, more compact) reports, bundling + // together similar ones fn check_override(&self, overridden: &T, report: &mut Vec) where T : MethodDeclaration, { + if self.is_shared() && !overridden.is_shared() { + report.push(DMLError { + span: *self.location(), + description: + "Shared method cannot override non-shared method".to_string(), + related: vec![( + *overridden.location(), + "Overridden method declared here".to_string())], + severity: Some(DiagnosticSeverity::ERROR), + }); + } + if self.throws() && !overridden.throws() { report.push(DMLError { span: *self.location(), @@ -257,6 +273,10 @@ impl MethodDeclaration for MethodDecl { matches!(self.modifier, MethodModifier::Shared) } + fn is_default(&self) -> bool { + self.default + } + fn throws(&self) -> bool { self.throws } @@ -287,71 +307,41 @@ impl MethodDecl { } } +// Note: A method can have a template ref even if not shared, it just means +// it was declared top-level within a template #[derive(Clone, Debug, PartialEq, Eq)] -pub enum DMLMethodRef { - TraitMethod(Arc, MethodDecl), - ConcreteMethod(DMLConcreteMethod), +pub struct DMLMethodRef { + pub template_ref: Option>, + pub concrete_decl: DMLConcreteMethod, } impl DMLMethodRef { pub fn get_disambiguation_name(&self) -> Option { - match self { - DMLMethodRef::TraitMethod(trt, _) => - Some(format!("templates.{}.{}", - trt.name, - self.identity())), - DMLMethodRef::ConcreteMethod(_) => None, - } + self.template_ref.as_ref().map( + |trt|format!("templates.{}.{}", + trt.name, self.identity())) } pub fn get_decl(&self) -> &MethodDecl { - match self { - Self::TraitMethod(_, decl) => decl, - Self::ConcreteMethod(conc) => &conc.decl, - } + &self.concrete_decl.decl } - pub fn get_default(&self) -> Option { - match self { - Self::TraitMethod(_, _) => None, - Self::ConcreteMethod(decl) => decl.default_call - .as_ref().cloned(), - } + pub fn get_default(&self) -> Option<&DefaultCallReference> { + self.concrete_decl.default_call.as_ref() } pub fn get_all_defs(&self) -> Vec { - match self { - DMLMethodRef::TraitMethod(_, decl) => if !decl.is_abstract() { - vec![*decl.location()] - } else { - vec![] - }, - DMLMethodRef::ConcreteMethod(concmeth) => concmeth.get_all_defs(), - } + self.concrete_decl.get_all_defs() } pub fn get_all_decls(&self) -> Vec { - match self { - DMLMethodRef::TraitMethod(_, decl) => if decl.is_abstract() { - vec![*decl.location()] - } else { - vec![] - }, - DMLMethodRef::ConcreteMethod(concmeth) => concmeth.get_all_defs(), - } + self.concrete_decl.get_all_decls() } // Invariant: Return is non-empty pub fn get_bases(&self) -> Vec { - let to_return = match self { - DMLMethodRef::TraitMethod(_, decl) => { - vec![decl.clone()] - }, - DMLMethodRef::ConcreteMethod(meth) => { - if let Some(default_ref) = &meth.default_call { - default_ref.get_bases() - } else { - vec![meth.decl.clone()] - } - } + let to_return = if let Some(default_refs) = &self.concrete_decl.default_call { + default_refs.flat_refs().into_iter().flat_map(|m|m.get_bases()).collect() + } else { + vec![self.concrete_decl.decl.clone()] }; assert!(!to_return.is_empty()); to_return @@ -360,77 +350,55 @@ impl DMLMethodRef { impl MaybeAbstract for DMLMethodRef { fn is_abstract(&self) -> bool { - match self { - DMLMethodRef::TraitMethod(_, decl) => decl.is_abstract(), - DMLMethodRef::ConcreteMethod(decl) => decl.is_abstract(), - } + self.concrete_decl.is_abstract() } } impl MethodDeclaration for DMLMethodRef { fn is_shared(&self) -> bool { - match self { - DMLMethodRef::TraitMethod(_, decl) => decl.is_shared(), - DMLMethodRef::ConcreteMethod(decl) => decl.is_shared(), - } + self.concrete_decl.is_shared() + } + + fn is_default(&self) -> bool { + self.concrete_decl.is_default() } fn throws(&self) -> bool { - match self { - DMLMethodRef::TraitMethod(_, decl) => decl.throws(), - DMLMethodRef::ConcreteMethod(decl) => decl.throws(), - } + self.concrete_decl.throws() } fn args(&self) -> &Vec { - match self { - DMLMethodRef::TraitMethod(_, decl) => decl.args(), - DMLMethodRef::ConcreteMethod(decl) => decl.args(), - } + self.concrete_decl.args() } fn returns(&self) -> &Vec { - match self { - DMLMethodRef::TraitMethod(_, decl) => decl.returns(), - DMLMethodRef::ConcreteMethod(decl) => decl.returns(), - } + self.concrete_decl.returns() } } impl DMLNamedMember for DMLMethodRef { fn identity(&self) -> &str { - match self { - DMLMethodRef::TraitMethod(_, decl) => decl.identity(), - DMLMethodRef::ConcreteMethod(decl) => decl.identity(), - } + self.concrete_decl.identity() } fn kind_name(&self) -> &'static str { - match self { - DMLMethodRef::TraitMethod(_, _) => "shared method", - DMLMethodRef::ConcreteMethod(_) => "method", - } + self.concrete_decl.kind_name() } fn location(&self) -> &ZeroSpan { - match self { - DMLMethodRef::TraitMethod(_, decl) => decl.location(), - DMLMethodRef::ConcreteMethod(decl) => decl.location(), - } + self.concrete_decl.location() } } impl DeclarationSpan for DMLMethodRef { fn span(&self) -> &ZeroSpan { - match self { - DMLMethodRef::TraitMethod(_, decl) => decl.span(), - DMLMethodRef::ConcreteMethod(decl) => decl.span(), - } + self.concrete_decl.span() } } #[derive(Debug, Clone, PartialEq, Eq)] pub enum DefaultCallReference { + Abstract(MethodDecl), Valid(Arc), Ambiguous(Vec>), } @@ -438,6 +406,7 @@ pub enum DefaultCallReference { impl DefaultCallReference { pub fn get_bases(&self) -> Vec { match self { + DefaultCallReference::Abstract(method) => vec![method.clone()], DefaultCallReference::Valid(method) => method.get_bases(), DefaultCallReference::Ambiguous(methods) => methods.iter().flat_map(|m|m.get_bases()).collect(), @@ -446,6 +415,7 @@ impl DefaultCallReference { pub fn get_all_decls(&self) -> Vec { match self { + DefaultCallReference::Abstract(method) => vec![*method.location()], DefaultCallReference::Valid(method) => method.get_all_decls(), DefaultCallReference::Ambiguous(methods) => methods.iter().flat_map(|m|m.get_all_decls()).collect(), @@ -454,6 +424,7 @@ impl DefaultCallReference { pub fn get_all_defs(&self) -> Vec { match self { + DefaultCallReference::Abstract(_) => vec![], DefaultCallReference::Valid(method) => method.get_all_defs(), DefaultCallReference::Ambiguous(methods) => methods.iter().flat_map(|m|m.get_all_defs()).collect(), @@ -464,11 +435,13 @@ impl DefaultCallReference { match self { DefaultCallReference::Valid(method) => Some(method), DefaultCallReference::Ambiguous(_) => None, + DefaultCallReference::Abstract(_) => None, } } pub fn flat_refs(&self) -> Vec<&Arc> { match self { + DefaultCallReference::Abstract(_) => vec![], DefaultCallReference::Valid(method) => vec![method], DefaultCallReference::Ambiguous(methods) => methods.iter().collect() @@ -480,7 +453,7 @@ impl DefaultCallReference { #[derive(Debug, Clone, PartialEq, Eq)] pub struct DMLConcreteMethod { pub decl: MethodDecl, - // where the default call goes (None if invalid) + // where the default call goes (None if nothing was overriding) pub default_call: Option, } @@ -524,6 +497,10 @@ impl MethodDeclaration for DMLConcreteMethod { self.decl.is_shared() } + fn is_default(&self) -> bool { + self.decl.default + } + fn throws(&self) -> bool { self.decl.throws() } diff --git a/src/analysis/templating/objects.rs b/src/analysis/templating/objects.rs index 1a4235b5..9f29ef52 100644 --- a/src/analysis/templating/objects.rs +++ b/src/analysis/templating/objects.rs @@ -1769,23 +1769,18 @@ fn merge_composite_subobjs<'c>(parent_each_stmts: &InEachSpec, fn sort_method_decls<'t>(decls: &[(&Rank, &'t MethodDecl)]) -> (HashMap<&'t MethodDecl, HashSet<&'t MethodDecl>>, Vec<&'t MethodDecl>) { trace!("Sorting method declarations: {:?}", decls); - let mut rank_to_method: HashMap<&Rank, &MethodDecl> - = HashMap::default(); - let mut conflicting_decls: HashMap<&MethodDecl, Vec<&MethodDecl>> + let mut rank_to_method: HashMap<&Rank, (Vec<&MethodDecl>, Vec<&MethodDecl>)> = HashMap::default(); + for (rank, decl) in decls { - if let Some(conflict) = rank_to_method.get(rank) { - // Conflicting declarations within one block - conflicting_decls.entry(conflict) - .and_modify(|e|e.push(decl)) - .or_insert_with(||vec![decl]); + let (ref mut defs, ref mut decls) = rank_to_method.entry(rank).or_default(); + if decl.is_abstract() { + decls.push(*decl); } else { - rank_to_method.insert(rank, *decl); + defs.push(*decl); } } - trace!("Conflicting declarations are: {:?}", conflicting_decls); - let ranks: HashSet<&Rank> = rank_to_method.keys() .cloned().collect(); @@ -1810,19 +1805,31 @@ fn sort_method_decls<'t>(decls: &[(&Rank, &'t MethodDecl)]) -> trace!("Minimal ancestors are {:?}", minimal_ancestry); - // TODO: there is an error that is missed here, - // see L463 of traits.py in dmlc source - // (or, if it has moved, the EAMBINH report in the - // sort_method_implementations function in the same - // file) - - let method_map: HashMap<&'t MethodDecl, HashSet<&'t MethodDecl>> - = rank_to_method.iter() - .map(|(r, m)|(*m, - minimal_ancestry[r].iter() - .map(|subrank|&rank_to_method[*subrank]) - .cloned().collect())) - .collect(); + // Map method declaration to those that they directly override + let mut method_map: HashMap<&'t MethodDecl, HashSet<&'t MethodDecl>> + = HashMap::default(); + for (r, (defs, decls)) in &rank_to_method { + for def in defs { + // Ensure the entry exists for later unwraps + method_map.entry(def).or_default(); + for subrank in &minimal_ancestry[r] { + let (subdefs, subdecls) = rank_to_method.get(subrank).unwrap(); + if subdefs.is_empty() { + method_map.get_mut(def).unwrap() + .extend(subdecls.clone()); + } else { + method_map.get_mut(def).unwrap() + .extend(subdefs.clone()); + } + } + method_map.get_mut(def).unwrap() + .extend(decls.iter().cloned()); + } + for decl in decls { + method_map.entry(decl).or_default(); + } + } + trace!("Default map is {:?}", method_map); let method_order = topsort(&method_map).unwrap_sorted(); trace!("Method order is {:?}", method_order); @@ -1831,10 +1838,10 @@ fn sort_method_decls<'t>(decls: &[(&Rank, &'t MethodDecl)]) -> fn add_methods(obj: &mut DMLCompositeObject, methods: MethodMapping, - method_map: &HashMap>, + name_to_trait_map: &HashMap>, report: &mut Vec) { debug!("Adding methods to {}", obj.identity()); - trace!("Shared methods are {:?}", method_map); + for (name, (used, regular_decls)) in methods { if !used { trace!("Skipped {} name, declaration unused", name); @@ -1844,12 +1851,11 @@ fn add_methods(obj: &mut DMLCompositeObject, name, regular_decls); let mut all_decls: Vec<(&Rank, &MethodDecl)> = regular_decls.iter().map(|(a, b)|(a, b)).collect(); - // TODO: If we obtain a shared method that is not in a trait - // does this mean it was declared in a non-shared context, - // and if so, do we need to report it? - if let Some(trait_impl) = method_map.get(&name) { - if let Some(trait_decl) = trait_impl.get_method(&name) { - all_decls.push((&trait_impl.rank, trait_decl)); + // Note: Sorting and overrides between methods is sorted (hah) already + // find the topmost relevant method decl + if let Some(trait_ref) = name_to_trait_map.get(&name) { + if let Some(trait_decl) = trait_ref.get_method(&name) { + all_decls.push((&trait_ref.rank, trait_decl)); } } trace!("All possible declarations are: {:?}", all_decls); @@ -1858,50 +1864,26 @@ fn add_methods(obj: &mut DMLCompositeObject, let mut decl_to_method: HashMap> = HashMap::default(); for method in method_order.iter().rev() { - debug!("Handling overrides of {:?}", method); - // Guaranteed, every val in method order is a key in default_map - let default_decls = default_map.get(method).unwrap(); - if method.is_shared() { - trace!("Was a shared method, deferred"); - let conflicting_decls : Vec<&MethodDecl> = default_decls.iter() - .filter(|d|!d.is_shared()).cloned().collect(); - if !conflicting_decls.is_empty() { - report.push(DMLError { - span: method.name.span, - description: - "Shared method cannot override non-shared method" - .to_string(), - related: conflicting_decls.iter().map( - |d|(d.name.span, - "Overrides this non-shared method".to_string())) - .collect(), - severity: Some(DiagnosticSeverity::ERROR), - }); - } - // The actual shared method "codegenning" is done later - // since it is not stored with the containing object - let new_method = Arc::new( - DMLMethodRef::TraitMethod( - Arc::clone(method_map.get(method.identity()).unwrap()), - (*method).clone())); - trace!("Inserted dependent methoddecl {:?}", new_method); - decl_to_method.insert((*method).clone(), new_method); - continue; - } + trace!("Handling overrides of {:?}", method); + // Guaranteed by topsort let defaults = default_map.get(method).unwrap(); let default = match defaults.len() { 1 => { let decl = defaults.iter().next().unwrap(); trace!("Default call decl is {:?}", decl); trace!("And the current map is {:?}", decl_to_method); - Some(DefaultCallReference::Valid(Arc::clone(decl_to_method.get(decl).unwrap()))) + if let Some(m) = decl_to_method.get(decl) { + Some(DefaultCallReference::Valid(Arc::clone(m))) + } else { + Some(DefaultCallReference::Abstract((*decl).clone())) + } }, 0 => None, _ => Some(DefaultCallReference::Ambiguous( defaults.iter().map(|d| Arc::clone(decl_to_method.get(d).unwrap())).collect())), }; trace!("Default call would be {:?}", default); - for decl in default_map.get(method).unwrap() { + for decl in defaults { // TODO: I suspect we could improve this error message in cases // where several similar incorrect overrides are made over one // method @@ -1920,16 +1902,21 @@ fn add_methods(obj: &mut DMLCompositeObject, }); } - if !decl.is_shared() { - method.check_override(*decl, report); - } + method.check_override(*decl, report); } - let new_method = Arc::new(DMLMethodRef::ConcreteMethod( - DMLConcreteMethod { - decl: (*method).clone(), - default_call: default, - })); + let template_ref = name_to_trait_map.get(&name) + .map(Arc::clone); + + let new_method = Arc::new( + DMLMethodRef { + template_ref, + concrete_decl: DMLConcreteMethod { + decl: (*method).clone(), + default_call: default, + }, + } + ); trace!("Inserted dependent methoddecl {:?}", new_method); decl_to_method.insert((*method).clone(), new_method); } diff --git a/src/analysis/templating/traits.rs b/src/analysis/templating/traits.rs index d28b01bb..8eba464f 100644 --- a/src/analysis/templating/traits.rs +++ b/src/analysis/templating/traits.rs @@ -7,14 +7,14 @@ use std::iter; use std::sync::Arc; use crate::analysis::parsing::tree::ZeroSpan; -use crate::analysis::structure; use crate::analysis::structure::objects::{MaybeAbstract}; use crate::analysis::structure::expressions::DMLString; use crate::analysis::structure::toplevel::{ObjectDecl, ExistCondition}; use crate::analysis::DMLError; -use crate::analysis::templating::methods::{MethodDecl, MethodDeclaration}; +use crate::analysis::templating::methods::{DMLConcreteMethod, DMLMethodRef, DefaultCallReference, MethodDecl, MethodDeclaration}; use crate::analysis::templating::objects::{CoarseObjectKind, + DMLNamedMember, DMLCoarseObjectKind, ObjectSpec, Spec}; use crate::analysis::templating::Declaration; @@ -42,14 +42,18 @@ pub struct DMLTrait { pub ancestors: HashMap>, // This is the final specification of a trait in analysis. So // lock in the objects here and move them to owned - // for further analysis later. Not that these are + // for further analysis later. Note that these are // _not_ objectdecls as conditional declarations are not // part of template type (this is IMO weird but what can you do) pub sessions: HashMap, pub saveds: HashMap, pub params: HashMap, - pub methods: HashMap, + + pub abstract_methods: HashMap, + // These are now concrete method declarations, since we need to be able + // to handle them as such for purposes of TQMIC resolutions or default calls + pub methods: HashMap, // Maps a symbol name to the ancestor implementing it // does NOT include symbols defined in this trait @@ -67,23 +71,36 @@ pub enum TraitMemberKind<'t> { Method(&'t MethodDecl), } -impl <'t> TraitMemberKind<'t> { - pub fn location(&self) -> &ZeroSpan { +impl <'t> DMLNamedMember for TraitMemberKind<'t> { + fn location(&self) -> &'t ZeroSpan { match self { TraitMemberKind::Session(d) | TraitMemberKind::Saved(d) | TraitMemberKind::Parameter(d) => &d.name.span, - TraitMemberKind::Method(m) => &m.name.span, + TraitMemberKind::Method(m) => m.location(), } } - pub fn name(&self) -> &str { + + fn identity(&self) -> &'t str { match self { TraitMemberKind::Session(d) | TraitMemberKind::Saved(d) | TraitMemberKind::Parameter(d) => &d.name.val, - TraitMemberKind::Method(m) => &m.name.val, + TraitMemberKind::Method(m) => m.identity() + } + } + + fn kind_name(&self) -> &'static str { + match self { + TraitMemberKind::Session(_) => "session", + TraitMemberKind::Saved(_) => "saved", + TraitMemberKind::Parameter(_) => "parameter", + TraitMemberKind::Method(_) => "method", } } +} + +impl <'t> TraitMemberKind<'t> { pub fn as_method(&self) -> Option<&'t MethodDecl> { match self { TraitMemberKind::Method(m) => Some(m), @@ -134,6 +151,7 @@ pub fn get_impls(me: &Arc) -> HashMap> { .chain(me.saveds.keys()) .chain(me.params.keys()) .chain(me.methods.keys()) + .chain(me.abstract_methods.keys()) .map(|name|(name.to_string(), Arc::clone(me))) .chain(me.ancestor_map.iter().map(|(n, rc)|(n.clone(), Arc::clone(rc)))) .collect() @@ -148,6 +166,18 @@ pub enum ImplementRelation { } impl DMLTrait { + pub fn get_defined_method<'t, T>(&'t self, method_name: &T) + -> Option<&'t DMLConcreteMethod> + where T: Borrow + ?Sized { + self.methods.get(method_name.borrow()) + } + + pub fn get_method<'t, T>(&'t self, method_name: &T) + -> Option<&'t MethodDecl> + where T: Borrow + ?Sized { + self.get_member(method_name)?.as_method() + } + pub fn implement_relation(&self, other: &T) -> ImplementRelation where T: Borrow { @@ -168,12 +198,6 @@ impl DMLTrait { } } - pub fn get_method<'t, T>(&'t self, method_name: &T) - -> Option<&'t MethodDecl> - where T: Borrow + ?Sized { - self.get_member(method_name)?.as_method() - } - // Lookup a definiton or declaration of a member in THIS trait, does not // examine ancestors pub fn get_member<'t, T>(&'t self, member_name: &T) @@ -194,6 +218,10 @@ impl DMLTrait { return Some(TraitMemberKind::Parameter(param)); } if let Some(method) = self.methods.get(member_name.borrow()) { + trace!("Got as {:?}", method); + return Some(TraitMemberKind::Method(&method.decl)); + } + if let Some(method) = self.abstract_methods.get(member_name.borrow()) { trace!("Got as {:?}", method); return Some(TraitMemberKind::Method(method)); } @@ -251,6 +279,7 @@ impl DMLTrait { sessions: HashMap::default(), saveds: HashMap::default(), params: HashMap::default(), + abstract_methods: HashMap::default(), methods: HashMap::default(), ancestor_map: HashMap::default(), reserved_symbols: HashMap::default(), @@ -376,26 +405,71 @@ impl DMLTrait { trace!("params are: {:?}", params); // Map methods into non-colliding method declarations - let methods: HashMap = spec.methods.iter() + let abstract_methods: HashMap = spec.methods.iter() .filter_map(discard_conditional) - .filter(|method|method.modifier == - structure::objects::MethodModifier::Shared) + .filter(|method|method.is_abstract()) .filter_map(|m| { let new_m = MethodDecl::from_content(&m, report); if maybe_report_collision( &mut used_names, - new_m.name.val.clone(), - new_m.name.span, + new_m.identity().to_string(), + *new_m.location(), report) { Some(new_m) } else { None }}).map(|m|(m.name.val.clone(), m)).collect(); - trace!("methods are: {:?}", methods); - - // Check that our overrides are sound - for meth in methods.values() { - if let Some(ancestor) = impl_map.get(&meth.name.val) { + trace!("abstract methods are: {:?}", abstract_methods); + + let mut defined_methods: HashMap = HashMap::default(); + for method in spec.methods.iter() + .filter_map(discard_conditional) + .filter(|method|!method.is_abstract()) { + let new_m = MethodDecl::from_content(&method, report); + // special case, methods are allowed to conflict + // with abstract methods + // NOTE: Right now this means we'd report the collision + // with non-method members from abstract decl only + // when the definition also collides, which is probably fine + if abstract_methods.contains_key(new_m.name.val.as_str()) + || (maybe_report_collision( + &mut used_names, + new_m.identity().to_string(), + *new_m.location(), + report)) { + // Need one more check here, if we have multiple + // non-shared decls and one or more shared decls, + // we'd miss the extra non-shared ones otherwise + if defined_methods.contains_key(new_m.name.val.as_str()) { + report.push(DMLError { + span: *new_m.location(), + description: + format!("Name collision on '{}'", + new_m.identity()), + severity: Some(DiagnosticSeverity::ERROR), + related: vec![ + (*defined_methods[new_m.identity()].location(), + "Previously defined here".to_string())], + }); + } else { + defined_methods.insert( + new_m.identity().to_string(), new_m.clone()); + } + } + } + + trace!("defined methods are: {:?}", defined_methods); + + let mut methods: HashMap = HashMap::default(); + + // Check that our overrides are sound and construct our concrete methods + // NOTE: Override order is already checked in merge_impl_maps, so we can use + // impl_map to get the next in order + for meth in defined_methods.values().chain(abstract_methods.values()) { + // TODO: Fairly sure this does not handle ambiguous overrides correctly. + // Odds are: they are reported correctly from merge_impl_maps, but + // we do not collect all of them here + let default_call = if let Some(ancestor) = impl_map.get(&meth.name.val) { if let Some(coll) = ancestor.get_member( meth.name.val.as_str()) { match coll { @@ -408,24 +482,30 @@ impl DMLTrait { override another method".into(), severity: Some(DiagnosticSeverity::ERROR), related: vec![ - (undermeth.name.span, + (*undermeth.location(), "Overridden definition here".into())], }); } - if !undermeth.default && !undermeth.is_abstract() { + if !undermeth.is_default() && !undermeth.is_abstract() { report.push(DMLError { span: meth.name.span, description: "This method attempts to override \ a non-default method".into(), related: vec![ - (undermeth.name.span, + (*undermeth.location(), "Attempted to override this \ declaration".into())], severity: Some(DiagnosticSeverity::ERROR), }); } meth.check_override(undermeth, report); + ancestor.get_defined_method(meth.identity()) + .map(|dmeth|DefaultCallReference::Valid( + Arc::new(DMLMethodRef { + template_ref: Some(Arc::clone(ancestor)), + concrete_decl: dmeth.clone(), + }))) }, TraitMemberKind::Saved(invalid) | TraitMemberKind::Session(invalid) | @@ -440,15 +520,29 @@ impl DMLTrait { (invalid.name.span, "Previously defined here".into())], }); + None } } } else { - error!("Invalid internal state: trait {} was indicated \ - as defining {} by {}, but didn't", - ancestor.name, - meth.name.val, - name); + internal_error!("trait {} was indicated \ + as defining {} by {}, but didn't", + ancestor.name, + meth.name.val, + name); + None } + } else { + None + }; + if !meth.is_abstract() { + methods.insert( + meth.name.val.clone(), + DMLConcreteMethod { + // TODO: This clone is slightly inefficient + decl: meth.clone(), + default_call, + } + ); } } @@ -490,6 +584,7 @@ impl DMLTrait { sessions, saveds, params, + abstract_methods, methods, ancestor_map: impl_map, reserved_symbols, @@ -593,7 +688,7 @@ where related: traits.iter().map( |t|{ let decl = t.get_method(ambiguous_name).unwrap(); - (decl.name.span, + (*decl.location(), format!("Conflicting definition here in {}", t.name)) }).collect(), From ccee9d59140dfe97734e4f9e72332fdcf38eeaa9 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 19 Jan 2026 13:32:17 +0100 Subject: [PATCH 07/34] Clarify and ensure behavior of goto- operations Signed-off-by: Jonatan Waern --- USAGE.md | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/USAGE.md b/USAGE.md index 0d397092..160c3273 100644 --- a/USAGE.md +++ b/USAGE.md @@ -12,6 +12,78 @@ details consult the documentation of the client. As this file is currently a work in progress, relevant details may be missing or incomplete. +## Clarification of semantics of Symbol Lookups +DML is at its core a _declarative_ language centered around a Device, +and declarations are merged together according to the hierarchical +structure. Thus, the meaning of `goto-definiton`, `goto-declaration`, +`goto-implementations`, and `goto-references` may not be similar to how +they behave in other languages. +Here is a clarification of what each operation means for each object +kind in DML. It is worth noting that unless otherwise specified, a goto- +operation on a reference is equivalent to the same operation on each symbol +that it could refer to. + +*NOTE:* Due to limitations in the DLS, a matching from references to +declarations is not always possible. `goto-references` may provide incomplete +info and `goto-definition` and similar on a reference may fail in some cases. +See issues [#13](https://github.com/intel/dml-language-server/issues/13), +[#23](https://github.com/intel/dml-language-server/issues/23), +[#26](https://github.com/intel/dml-language-server/issues/26), +[#30](https://github.com/intel/dml-language-server/issues/30), and +[#65](https://github.com/intel/dml-language-server/issues/65). + +### On Composite Objects (banks, registers, implements, etc.) +`goto-declaration` and `goto-definition` on an composite object are equivalent. +They will pointreturn the locations of all object declarations that may be +merged with the one at the name. + +`goto-implementations` on objects is currently unused. + +`goto-references` will go to any location where the object is referred to +directly. + +### On Methods +`goto-declaration` on a method will find the most-overridden declaration +for this method, this will usually be a 'default' or abstract declaration. +TODO: this is wrong currently + +`goto-definition` on a method reference will find all defintions of that method +that could be called from that reference. Note that this will NOT point to +method declarations that are entirely overridden. `goto-definition` on a method +name is a no-op and will just point back to that same method. +TODO: does not work correctly on default in overridden method body + +`goto-implementations` on a method will find all method declarations that +could override it. +TODO: only finds next-level + +`goto-references` will go to any location where the method is referred to +directly(including call sites). + +### On Parameters +`goto-declaration` on a parameter will find the most-overridden declaration of +this parameter, usually a 'default' or abstract declaration. + +`goto-definition` on a parameter will find all definitions that are used within +a method. + +`goto-implementations` on a parameter is currently unused. + +`goto-references` will go to any location where the parameter is referred to +directly. + +### On Templates +`goto-declaration` and `goto-definition` are equivalent, and will both give the +template declaration site. + +`goto-implementations` will give each location where the template is directly +instantiated. +TODO: does not actually work + +`goto-references` will go to any location where the template is referred to, +including direct instantiation sites (so this is a super-set of +goto-implementations`) + ## In-Line Linting Configuration It may be desireable to control linting on a per-file basis, rather than relying on the linting configuration. This can be done with in-line From 6ef965b3669058dd5a229b59367ec6ac72543661 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Wed, 21 Jan 2026 13:08:33 +0100 Subject: [PATCH 08/34] Refactor and move semantic lookup methods Signed-off-by: Jonatan Waern --- USAGE.md | 1 - src/actions/analysis_storage.rs | 26 +-- src/actions/mod.rs | 1 + src/actions/requests.rs | 171 ++------------- src/actions/semantic_lookup.rs | 357 ++++++++++++++++++++++++++++++++ src/analysis/mod.rs | 67 +++--- src/analysis/symbols.rs | 7 + 7 files changed, 413 insertions(+), 217 deletions(-) create mode 100644 src/actions/semantic_lookup.rs diff --git a/USAGE.md b/USAGE.md index 160c3273..ac0ceaf8 100644 --- a/USAGE.md +++ b/USAGE.md @@ -55,7 +55,6 @@ TODO: does not work correctly on default in overridden method body `goto-implementations` on a method will find all method declarations that could override it. -TODO: only finds next-level `goto-references` will go to any location where the method is referred to directly(including call sites). diff --git a/src/actions/analysis_storage.rs b/src/actions/analysis_storage.rs index bafbc501..5f5b6edf 100644 --- a/src/actions/analysis_storage.rs +++ b/src/actions/analysis_storage.rs @@ -2,7 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 and MIT //! Stores currently completed analysis. -use log::{debug, trace, info}; +use log::{debug, info, trace}; use crossbeam::channel; @@ -14,13 +14,12 @@ use std::sync::Mutex; use std::time::{Duration, SystemTime}; use crate::actions::ContextDefinition; -use crate::analysis::scope::{ContextedSymbol, ContextKey}; +use crate::analysis::scope::ContextKey; use crate::analysis::structure::objects::Import; use crate::analysis::{DMLError, DeviceAnalysis, IsolatedAnalysis}; use crate::lsp_data::*; use crate::analysis::parsing::tree::{ZeroSpan, ZeroFilePosition}; -use crate::analysis::reference::Reference; use crate::server::ServerToHandle; use crate::lint::LinterAnalysis; @@ -211,27 +210,6 @@ impl AnalysisStorage { } } - pub fn context_symbol_at_pos<'t>(&'t self, pos: &ZeroFilePosition) -> - Result>, AnalysisLookupError> { - let canon_path = CanonPath::from_path_buf(pos.path()) - .ok_or(AnalysisLookupError::NoFile)?; - let analysis = self.get_isolated_analysis(&canon_path)?; - let mut context = analysis.lookup_context_symbol(pos); - // Patch out leading 'device' context, unneeded - if let Some(ref mut ic) = context { - ic.remove_head_context(); - } - Ok(context) - } - - pub fn reference_at_pos(&self, pos: &ZeroFilePosition) -> - Result, AnalysisLookupError> { - let canon_path = CanonPath::from_path_buf(pos.path()) - .ok_or(AnalysisLookupError::NoFile)?; - let analysis = self.get_isolated_analysis(&canon_path)?; - Ok(analysis.lookup_reference(pos)) - } - pub fn first_context_at_pos(&self, pos: &ZeroFilePosition) -> Result, AnalysisLookupError> { let canon_path = CanonPath::from_path_buf(pos.path()) diff --git a/src/actions/mod.rs b/src/actions/mod.rs index ecc24539..ed91df54 100644 --- a/src/actions/mod.rs +++ b/src/actions/mod.rs @@ -92,6 +92,7 @@ pub mod notifications; pub mod requests; pub mod progress; pub mod work_pool; +pub mod semantic_lookup; /// Persistent context shared across all requests and notifications. pub enum ActionContext { diff --git a/src/actions/requests.rs b/src/actions/requests.rs index 386e33cb..b4e2f651 100644 --- a/src/actions/requests.rs +++ b/src/actions/requests.rs @@ -15,10 +15,8 @@ use crate::actions::{AnalysisProgressKind, AnalysisWaitKind, ContextDefinition, InitActionContext, rpc_error_code}; use crate::actions::notifications::ContextDefinitionKindParam; -use crate::analysis::{Named, DeclarationSpan, LocationSpan, DLSLimitation, - SymbolRef, ZeroFilePosition, ZeroSpan, - isolated_template_limitation}; -use crate::analysis::reference::ReferenceKind; +use crate::actions::semantic_lookup::{DLSLimitation, declarations_at_fp, definitions_at_fp, implementations_at_fp, references_at_fp}; +use crate::analysis::{Named, DeclarationSpan, LocationSpan}; use crate::analysis::symbols::SimpleSymbol; use crate::config::Config; @@ -127,107 +125,6 @@ where } } - -pub fn type_semantic_limitation() -> DLSLimitation { - DLSLimitation { - issue_num: 65, - description: "The DLS does not currently support semantic analysis of \ - types, including reference finding".to_string(), - } -} - -// TODO: This function is getting bloated, refactor into several smaller ones -fn fp_to_symbol_refs - (fp: &ZeroFilePosition, - ctx: &InitActionContext, - relevant_limitations: &mut HashSet) - -> Result, AnalysisLookupError> -{ - let analysis = ctx.analysis.lock().unwrap(); - // This step-by-step approach could be folded into analysis_storage, - // but I keep it as separate here so that we could, perhaps, - // returns different information for "no symbols found" and - // "no info at pos" - debug!("Looking up symbols/references at {:?}", fp); - let reference = analysis.reference_at_pos(fp)?; - debug!("Got reference {:?}", reference); - - let canon_path = CanonPath::from_path_buf(fp.path()) - .ok_or(AnalysisLookupError::NoFile)?; - - let analysises = analysis.all_device_analysises_containing_file(&canon_path); - if analysises.is_empty() { - return Err(AnalysisLookupError::NoDeviceAnalysis); - } - let context_sym = analysis.context_symbol_at_pos(fp)?; - let symbols = context_sym.map(|cs| { - analysises.into_iter().flat_map(|a|a.lookup_symbols(&cs, relevant_limitations)).collect::>() - }); - if let Some(syms) = &symbols { - debug!("Got symbols {:?}", syms.iter().map(|s|s.lock().unwrap().medium_debug_info()).collect::>()); - } - - // Rather than holding the lock throughout the request, clone - // the filter - let filter = Some(ctx.device_active_contexts.lock().unwrap().clone()); - let mut definitions = vec![]; - - match (symbols, reference) { - (None, None) => { - debug!("No symbol or reference at point"); - return Ok(vec![]); - }, - (Some(syms), refer) => { - if refer.is_some() { - error!("Obtained both symbol and reference at {:?}\ - (reference is {:?}), defaulted to symbol", - &fp, refer); - } - definitions.extend(syms); - }, - (None, Some(refr)) => { - debug!("Mapping {:?} to symbols", refr.loc_span()); - // Should be guaranteed by the context reference lookup above - // (isolated analysis does exist) - if refr.reference_kind() == ReferenceKind::Type { - relevant_limitations.insert(type_semantic_limitation()); - } - - let first_context = analysis.first_context_at_pos(fp).unwrap(); - let mut any_template_used = false; - for device in analysis.filtered_device_analysises_containing_file( - &canon_path, - filter.as_ref()) { - // NOTE: This ends up being the correct place to warn users - // about references inside uninstantiated templates, - // but we have to perform some extra work to find out we are - // in that case - if let Some(ContextKey::Template(ref sym)) = first_context { - if device.templates.templates.get(sym.name_ref()) - .and_then(|t|t.location.as_ref()) - .and_then( - |loc|device.template_object_implementation_map.get(loc)) - .map_or(false, |impls|!impls.is_empty()) { - any_template_used = true; - } - } - - definitions.append( - &mut device.symbols_of_ref(*refr.loc_span())); - } - if let Some(ContextKey::Template(templ)) = first_context { - if !any_template_used { - relevant_limitations.insert( - isolated_template_limitation(templ.name.as_str()) - ); - } - } - }, - } - debug!("Resolved to symbols: {:?}", definitions); - Ok(definitions) -} - /// The result of a deglob action for a single wildcard import. /// /// The `location` is the position of the wildcard. @@ -519,16 +416,10 @@ impl RequestAction for GotoImplementation { wait_for_device_path!(ctx, canon_path); let mut limitations = HashSet::new(); - match fp_to_symbol_refs(&fp, &ctx, &mut limitations) { - Ok(symbols) => { - let mut unique_locations: HashSet - = HashSet::default(); - for symbol in symbols { - for implementation in &symbol.lock().unwrap().implementations { - unique_locations.insert(*implementation); - } - } - let lsp_locations: Vec<_> = unique_locations.into_iter() + + match implementations_at_fp(&ctx, &fp, &mut limitations) { + Ok(locs) => { + let lsp_locations: Vec<_> = locs.into_iter() .map(|l|ls_util::dls_to_location(&l)) .collect(); debug!("Requested implementations are {:?}", lsp_locations); @@ -542,8 +433,7 @@ impl RequestAction for GotoImplementation { }, Err(lookuperror) => { let main_file_name = fp.path(); - warn_miss_lookup(lookuperror, - main_file_name.to_str()); + warn_miss_lookup(lookuperror, main_file_name.to_str()); Self::fallback_response() }, } @@ -585,12 +475,9 @@ impl RequestAction for GotoDeclaration { wait_for_device_path!(ctx, canon_path); let mut limitations = HashSet::new(); - match fp_to_symbol_refs(&fp, &ctx, &mut limitations) { - Ok(symbols) => { - let unique_locations = symbols.into_iter() - .flat_map(|d|d.lock().unwrap().declarations.clone()) - .collect::>(); - let lsp_locations = unique_locations.into_iter() + match declarations_at_fp(&ctx, &fp, &mut limitations) { + Ok(locs) => { + let lsp_locations = locs.into_iter() .map(|l|ls_util::dls_to_location(&l)) .collect(); trace!("Requested declarations are {:?}", lsp_locations); @@ -645,13 +532,9 @@ impl RequestAction for GotoDefinition { wait_for_device_path!(ctx, canon_path); let mut limitations = HashSet::new(); - match fp_to_symbol_refs(&fp, &ctx, &mut limitations) { - Ok(symbols) => { - let unique_locations = symbols.into_iter() - .flat_map(|d|d.lock().unwrap().definitions.clone()) - .collect::>(); - - let lsp_locations: Vec<_> = unique_locations.into_iter() + match definitions_at_fp(&ctx, &fp, &mut limitations) { + Ok(locs) => { + let lsp_locations: Vec<_> = locs.into_iter() .map(|l|ls_util::dls_to_location(&l)) .collect(); Ok(response_maybe_with_limitations( @@ -670,25 +553,6 @@ impl RequestAction for GotoDefinition { } } -/* fn filter_out_unpointed_defaults(fp: &ZeroFilePosition, - symbols: &[SymbolRef]) - -> HashSet { - symbols - .iter() - .flat_map(|d|{ - let sym = d.lock().unwrap(); - let ignored_defaults = sym.default_mappings - .iter() - .filter(|(_, ref_loc)|!ref_loc.contains_pos(fp)) - .map(|(decl_loc, _)|*decl_loc) - .collect::>(); - sym.references.iter().filter(|loc| { - !ignored_defaults.contains(loc) - }).cloned().collect::>() - }) - .collect() -} */ - impl RequestAction for References { type Response = ResponseWithMessage>; @@ -725,12 +589,9 @@ impl RequestAction for References { wait_for_device_path!(ctx, canon_path); let mut limitations = HashSet::new(); - match fp_to_symbol_refs(&fp, &ctx, &mut limitations) { - Ok(symbols) => { - let unique_locations = symbols.into_iter() - .flat_map(|d|d.lock().unwrap().references.clone()) - .collect::>(); - let lsp_locations: Vec<_> = unique_locations.into_iter() + match references_at_fp(&ctx, &fp, &mut limitations) { + Ok(locs) => { + let lsp_locations: Vec<_> = locs.into_iter() .map(|l|ls_util::dls_to_location(&l)) .collect(); trace!("Requested references are {:?}", lsp_locations); diff --git a/src/actions/semantic_lookup.rs b/src/actions/semantic_lookup.rs new file mode 100644 index 00000000..a62d3b5c --- /dev/null +++ b/src/actions/semantic_lookup.rs @@ -0,0 +1,357 @@ +// © 2024 Intel Corporation +// SPDX-License-Identifier: Apache-2.0 and MIT +//! Stores currently completed analysis. + +use log::{debug, error}; + +use std::collections::HashSet; +use std::{fmt, mem}; +use std::sync::Arc; + +use crate::actions::analysis_storage::{AnalysisLookupError, AnalysisStorage}; +use crate::actions::{ContextDefinition, InitActionContext}; +use crate::analysis::scope::{ContextedSymbol, ContextKey}; +use crate::analysis::symbols::DMLSymbolKind; +use crate::analysis::{DeviceAnalysis, IsolatedAnalysis, LocationSpan, SymbolRef}; + +use crate::analysis::parsing::tree::{ZeroSpan, ZeroFilePosition}; +use crate::analysis::reference::{Reference, ReferenceKind}; +use crate::file_management::CanonPath; +use crate::server::Output; + +// The issue number is used as the _unique identifier_ for this +// limitation, and should be the number of an open GITHUB +// issue. +// Example: +// DLSLimitation { +// issue_num: 42, +// description: "Example of a DLS limitation".to_string(), +// }; + +#[derive(Clone, Debug, Eq, PartialEq, Hash,)] +pub struct DLSLimitation { + pub issue_num: u64, + pub description: String, +} + +impl fmt::Display for DLSLimitation { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{} (issue#{})", self.description, self.issue_num) + } +} + +pub fn type_semantic_limitation() -> DLSLimitation { + DLSLimitation { + issue_num: 65, + description: "The DLS does not currently support semantic analysis of \ + types, including reference finding".to_string(), + } +} + +pub fn isolated_template_limitation(template_name: &str) -> DLSLimitation { + DLSLimitation { + issue_num: 31, + description: + format!("References from, and definitions inside, a template \ + cannot be evaluated \ + without an instantiating object. Open a device file \ + that uses the template '{}' to obtain such information.", + template_name), + } +} + +// Because symbols need to be tied to their source analysis that result is +// a [(DeviceAnalysis, [SymbolRef])] list +// The reference comes from an isolated analysis, and thus is disconnected from a device +// context +type DeviceSymbols<'t> = Vec<(&'t DeviceAnalysis, Vec)>; +enum SymbolsOrReference<'t> { + Symbols(DeviceSymbols<'t>), + Reference(Reference), + Nothing, +} + +struct AnalysisInfo <'a> { + pub isolated_analysis: &'a IsolatedAnalysis, + pub device_analysises: Vec<&'a DeviceAnalysis>, +} + +#[allow(dead_code)] +struct SemanticLookup<'t> { + pub stored_symbols: DeviceSymbols<'t>, + pub found_ref: Option, + pub analysis_info: AnalysisInfo<'t>, + pub recognized_limitations: HashSet, +} + +impl <'t> SemanticLookup<'t> { + pub fn create_lookup(fp: &ZeroFilePosition, + analysis_lock: &'t AnalysisStorage, + ctx: &'t InitActionContext) + -> Result { + let active_filter = ctx.device_active_contexts.lock().unwrap().clone(); + let analysis_info = analysises_at_fp(analysis_lock, fp, Some(&active_filter))?; + let mut recognized_limitations = HashSet::new(); + + let lookup_result = get_refs_and_syms_at_fp( + fp, + &analysis_info, + &mut recognized_limitations, + )?; + + let mut stored_symbols = vec![]; + let mut found_ref = None; + match lookup_result { + SymbolsOrReference::Symbols(symbol_refs) => stored_symbols = symbol_refs, + SymbolsOrReference::Reference(reference) => { + found_ref = Some(reference.clone()); + stored_symbols = get_symbols_of_ref( + &reference, + &analysis_info, + &mut recognized_limitations) + }, + SymbolsOrReference::Nothing => (), + } + + Ok(SemanticLookup { + stored_symbols, + found_ref, + analysis_info, + recognized_limitations, + }) + } + + pub fn symbols(&'t self) -> Vec { + self.stored_symbols.iter().flat_map(|(_, syms)|syms) + .map(|s|Arc::clone(s)).collect::>() + } +} + +fn analysises_at_fp<'t>( + analysis_storage: &'t AnalysisStorage, + fp: &ZeroFilePosition, + active_filter: Option<&HashSet>) + -> Result, AnalysisLookupError> { + let canon_path = CanonPath::from_path_buf(fp.path()) + .ok_or(AnalysisLookupError::NoFile)?; + let isolated_analysis = analysis_storage.get_isolated_analysis(&canon_path)?; + let analysises = analysis_storage.filtered_device_analysises_containing_file(&canon_path, active_filter); + if analysises.is_empty() { + return Err(AnalysisLookupError::NoDeviceAnalysis); + } + Ok(AnalysisInfo { + isolated_analysis, + device_analysises: analysises, + }) +} + +fn get_refs_and_syms_at_fp<'t>( + fp: &ZeroFilePosition, + analysis_info: &AnalysisInfo<'t>, + relevant_limitations: &mut HashSet) + -> Result, AnalysisLookupError> { + let ref_at_pos = analysis_info.isolated_analysis.lookup_reference(fp); + + let context_sym_at_pos = context_symbol_at_pos(analysis_info.isolated_analysis, fp); + let symbols_at_fp = context_sym_at_pos.map(|cs| { + analysis_info.device_analysises.iter().map(|a|(*a, a.lookup_symbols(&cs, relevant_limitations))).collect::>() + }); + if let Some(syms) = symbols_at_fp { + if ref_at_pos.is_some() { + error!("Obtained both symbol and reference at {:?}\ + (reference is {:?}), defaulted to symbol", + &fp, ref_at_pos); + } + return Ok(SymbolsOrReference::Symbols(syms)); + } + if let Some(refr) = ref_at_pos { + if refr.reference_kind() == ReferenceKind::Type { + relevant_limitations.insert(type_semantic_limitation()); + } + } + if let Some(refr) = ref_at_pos { + Ok(SymbolsOrReference::Reference(refr.clone())) + } else { + debug!("No symbol or reference at point"); + Ok(SymbolsOrReference::Nothing) + } +} + +fn get_symbols_of_ref<'t>(reference: &Reference, + analysis_info: &AnalysisInfo<'t>, + relevant_limitations: &mut HashSet) + -> DeviceSymbols<'t> { + debug!("Mapping {:?} to symbols", reference.loc_span()); + // Should be guaranteed by the context reference lookup above + // (isolated analysis does exist) + let mut symbols = vec![]; + let first_context = analysis_info.isolated_analysis + .lookup_first_context(&reference.loc_span().start_position()).unwrap(); + let mut any_template_used = false; + for device in &analysis_info.device_analysises { + // NOTE: This ends up being the correct place to warn users + // about references inside uninstantiated templates, + // but we have to perform some extra work to find out we are + // in that case + if let ContextKey::Template(ref sym) = first_context { + if device.templates.templates.get(sym.name_ref()) + .and_then(|t|t.location.as_ref()) + .and_then(|loc|device.template_object_implementation_map.get(loc)) + .map_or(false, |impls|!impls.is_empty()) { + any_template_used = true; + } + } + symbols.append(&mut device.symbols_of_ref(*reference.loc_span())); + } + if let ContextKey::Template(templ) = first_context { + if !any_template_used { + relevant_limitations.insert( + isolated_template_limitation(templ.name.as_str()) + ); + } + } + + let mut symbols: DeviceSymbols<'t> = vec![]; + for device_analysis in &analysis_info.device_analysises { + let syms = device_analysis.symbols_of_ref(*reference.loc_span()); + if !syms.is_empty() { + debug!("Mapped {:?} to symbols {:?} in device analysis of {}", reference.loc_span(), + syms.iter().map(|s|s.lock().unwrap().medium_debug_info()).collect::>(), + device_analysis.path.as_str()); + symbols.push((*device_analysis, syms)); + } + } + symbols +} + +fn context_symbol_at_pos<'t>(isolated_analysis: &'t IsolatedAnalysis, pos: &ZeroFilePosition) -> + Option> { + let mut context = isolated_analysis.lookup_context_symbol(pos); + // Patch out leading 'device' context, unneeded + if let Some(ref mut ic) = context { + ic.remove_head_context(); + } + context +} + +fn symbol_implementations_of_symbol<'t>(symbol: &'t SymbolRef, + analysis: &'t DeviceAnalysis) + -> Vec { + let symbol_lock = symbol.lock().unwrap(); + + // Special case for methods, recursively follow the implementations to find + // all methods + if symbol_lock.kind == DMLSymbolKind::Method { + // Re-dropping the lock here is sensible, as we want to iterate over non-locked symbols + drop(symbol_lock); + #[allow(clippy::mutable_key_type)] + let mut syms: HashSet::<&'t SymbolRef> = HashSet::default(); + let mut next_iteration = vec![symbol]; + while let Some(next) = next_iteration.pop() { + let next_lock = next.lock().unwrap(); + let (parent, _)= if let Some(meth_source) = next_lock.source.as_method() { + meth_source + } else { + internal_error!("Expected method symbol source of symbol iterated over by\ + implementations_of_symbol, symbol is {:?}", next_lock); + continue; + }; + if !syms.contains(next) { + syms.insert(next); + for impl_ref in &next_lock.implementations { + // Note: We only need to find the implementations of the variant of this method + // under parent. Other variants are handled by finding multiple symbols at + // the ref location + if let Some(impl_sym) = analysis.symbol_info.method_symbols.get(impl_ref) + .and_then(|m|m.get(parent)) { + next_iteration.push(impl_sym); + } else { + internal_error!("Expected method implementation symbol to exist in \ + analysis, symbol is {:?}, implementation ref is {:?}", + next_lock, impl_ref); + } + } + } + } + syms.into_iter().map(Arc::clone).collect() + } else { + vec![] + } +} + +fn extend_with_implementations_of_symbols<'t>(semantic_lookup: &mut SemanticLookup<'t>) { + for (_device_analysis, symbols) in &mut semantic_lookup.stored_symbols { + #[allow(clippy::mutable_key_type)] + let mut symbol_collection: HashSet<_> = HashSet::default(); + for symbol in symbols.iter() { + symbol_collection.extend(symbol_implementations_of_symbol(symbol, _device_analysis)); + } + symbol_collection.extend(symbols.drain(..)); + symbols.extend(symbol_collection.into_iter()); + } +} + +pub fn implementations_at_fp(context: &InitActionContext, + fp: &ZeroFilePosition, + relevant_limitations: &mut HashSet) + -> Result, AnalysisLookupError> { + let analysis_lock = context.analysis.lock().unwrap(); + let mut semantic_lookup = SemanticLookup::create_lookup( + fp, + &analysis_lock, + context)?; + extend_with_implementations_of_symbols(&mut semantic_lookup); + mem::swap(relevant_limitations, &mut semantic_lookup.recognized_limitations); + Ok(semantic_lookup.symbols() + .into_iter() + .map(|s|s.lock().unwrap().loc) + .collect()) +} + +pub fn definitions_at_fp(context: &InitActionContext, + fp: &ZeroFilePosition, + relevant_limitations: &mut HashSet) + -> Result, AnalysisLookupError> { + let analysis_lock = context.analysis.lock().unwrap(); + let mut semantic_lookup = SemanticLookup::create_lookup( + fp, + &analysis_lock, + context)?; + mem::swap(relevant_limitations, &mut semantic_lookup.recognized_limitations); + Ok(semantic_lookup.symbols() + .into_iter() + .flat_map(|s|s.lock().unwrap().definitions.clone()) + .collect()) +} + +pub fn declarations_at_fp(context: &InitActionContext, + fp: &ZeroFilePosition, + relevant_limitations: &mut HashSet) + -> Result, AnalysisLookupError> { + let analysis_lock = context.analysis.lock().unwrap(); + let mut semantic_lookup = SemanticLookup::create_lookup( + fp, + &analysis_lock, + context)?; + mem::swap(relevant_limitations, &mut semantic_lookup.recognized_limitations); + Ok(semantic_lookup.symbols() + .into_iter() + .flat_map(|s|s.lock().unwrap().declarations.clone()) + .collect()) +} + +pub fn references_at_fp(context: &InitActionContext, + fp: &ZeroFilePosition, + relevant_limitations: &mut HashSet) + -> Result, AnalysisLookupError> { + let analysis_lock = context.analysis.lock().unwrap(); + let mut semantic_lookup = SemanticLookup::create_lookup( + fp, + &analysis_lock, + context)?; + mem::swap(relevant_limitations, &mut semantic_lookup.recognized_limitations); + Ok(semantic_lookup.symbols() + .into_iter() + .flat_map(|s|s.lock().unwrap().references.clone()) + .collect()) +} \ No newline at end of file diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index a4c026c7..381b7b32 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -26,6 +26,7 @@ use rayon::prelude::*; use crate::actions::SourcedDMLError; use crate::actions::analysis_storage::TimestampedStorage; +use crate::actions::semantic_lookup::{DLSLimitation, isolated_template_limitation}; use crate::analysis::symbols::{DMLSymbolKind, SimpleSymbol, StructureSymbol, SymbolContainer, SymbolMaker, SymbolSource}; pub use crate::analysis::symbols::SymbolRef; use crate::analysis::reference::{Reference, @@ -77,26 +78,6 @@ pub struct FileSpec<'a> { pub const IMPLICIT_IMPORTS: [&str; 2] = ["dml-builtins.dml", "simics/device-api.dml"]; -// The issue number is used as the _unique identifier_ for this -// limitation, and should be the number of an open GITHUB -// issue. -// Example: -// DLSLimitation { -// issue_num: 42, -// description: "Example of a DLS limitation".to_string(), -// }; - -#[derive(Clone, Debug, Eq, PartialEq, Hash,)] -pub struct DLSLimitation { - pub issue_num: u64, - pub description: String, -} - -impl fmt::Display for DLSLimitation { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{} (issue#{})", self.description, self.issue_num) - } -} // For things whose names are in one spot as a dmlstring pub trait DMLNamed { @@ -371,6 +352,28 @@ impl SymbolStorage { .chain(self.method_symbols.values().flat_map(|h|h.values())) .chain(self.variable_symbols.values()) } + + pub fn all_symbols_at_lock<'a>(&'a self, loc: &ZeroSpan) + -> Vec<&'a SymbolRef> { + let mut result = vec![]; + if let Some(sym) = self.template_symbols.get(loc) { + result.push(sym); + } + if let Some(param_map) = self.param_symbols.get(&(*loc, "".to_string())) { + for sym in param_map.values() { + result.push(sym); + } + } + if let Some(var_sym) = self.variable_symbols.get(loc) { + result.push(var_sym); + } + if let Some(method_map) = self.method_symbols.get(loc) { + for sym in method_map.values() { + result.push(sym); + } + } + result + } } // This maps references to the symbol they reference, made as a lock @@ -598,20 +601,9 @@ fn gather_scopes<'c>(next_scopes: Vec<&'c dyn Scope>, } } -pub fn isolated_template_limitation(template_name: &str) -> DLSLimitation { - DLSLimitation { - issue_num: 31, - description: - format!("References from, and definitions inside, a template \ - cannot be evaluated \ - without an instantiating object. Open a device file \ - that uses the template '{}' to obtain such information.", - template_name), - } -} - impl DeviceAnalysis { - pub fn lookup_symbols<'t>(&self, context_sym: &ContextedSymbol<'t>, limitations: &mut HashSet) -> Vec { + pub fn lookup_symbols<'t>(&self, context_sym: &ContextedSymbol<'t>, limitations: &mut HashSet) + -> Vec { trace!("Looking up symbols at {:?}", context_sym); // Special handling for methods, since each method decl has its @@ -1086,10 +1078,11 @@ impl DeviceAnalysis { } } - pub fn lookup_symbols_by_contexted_symbol<'t>( - &self, - sym: &ContextedSymbol<'t>, - limitations: &mut HashSet) -> Vec { + // TODO/NOTE: This method seems slightly misplaced, consider moving to analysis storage + pub fn lookup_symbols_by_contexted_symbol<'t>(&self, + sym: &ContextedSymbol<'t>, + limitations: &mut HashSet) + -> Vec { if matches!(sym.symbol.kind, DMLSymbolKind::Template | DMLSymbolKind::Typedef | DMLSymbolKind::Extern) { diff --git a/src/analysis/symbols.rs b/src/analysis/symbols.rs index d74a5720..18192038 100644 --- a/src/analysis/symbols.rs +++ b/src/analysis/symbols.rs @@ -130,6 +130,13 @@ impl SymbolSource { _ => None } } + pub fn as_method(&self) -> Option<(&StructureKey, &Arc)> { + match self { + Self::Method(key, methref) => Some((key, methref)), + _ => None + } + } + pub fn as_metharg(&self) -> Option<(&Arc, &DMLString)> { match self { Self::MethodArg(arg, name) => Some((arg, name)), From d9e67d50773007c356d0af47f623c1930d8de247 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Fri, 23 Jan 2026 15:25:52 +0100 Subject: [PATCH 09/34] Fixes to method lookups Signed-off-by: Jonatan Waern --- USAGE.md | 4 +-- src/actions/semantic_lookup.rs | 42 ++++++++++++++++++++---------- src/analysis/mod.rs | 47 +++++++++++++++++++++++++++++----- 3 files changed, 70 insertions(+), 23 deletions(-) diff --git a/USAGE.md b/USAGE.md index ac0ceaf8..0ba5cdf4 100644 --- a/USAGE.md +++ b/USAGE.md @@ -45,19 +45,17 @@ directly. ### On Methods `goto-declaration` on a method will find the most-overridden declaration for this method, this will usually be a 'default' or abstract declaration. -TODO: this is wrong currently `goto-definition` on a method reference will find all defintions of that method that could be called from that reference. Note that this will NOT point to method declarations that are entirely overridden. `goto-definition` on a method name is a no-op and will just point back to that same method. -TODO: does not work correctly on default in overridden method body `goto-implementations` on a method will find all method declarations that could override it. `goto-references` will go to any location where the method is referred to -directly(including call sites). +directly (including call sites). ### On Parameters `goto-declaration` on a parameter will find the most-overridden declaration of diff --git a/src/actions/semantic_lookup.rs b/src/actions/semantic_lookup.rs index a62d3b5c..1877ca79 100644 --- a/src/actions/semantic_lookup.rs +++ b/src/actions/semantic_lookup.rs @@ -186,14 +186,15 @@ fn get_symbols_of_ref<'t>(reference: &Reference, // (isolated analysis does exist) let mut symbols = vec![]; let first_context = analysis_info.isolated_analysis - .lookup_first_context(&reference.loc_span().start_position()).unwrap(); + .lookup_first_context(&reference.loc_span() + .start_position()); let mut any_template_used = false; for device in &analysis_info.device_analysises { // NOTE: This ends up being the correct place to warn users // about references inside uninstantiated templates, // but we have to perform some extra work to find out we are // in that case - if let ContextKey::Template(ref sym) = first_context { + if let Some(ContextKey::Template(ref sym)) = first_context { if device.templates.templates.get(sym.name_ref()) .and_then(|t|t.location.as_ref()) .and_then(|loc|device.template_object_implementation_map.get(loc)) @@ -203,7 +204,7 @@ fn get_symbols_of_ref<'t>(reference: &Reference, } symbols.append(&mut device.symbols_of_ref(*reference.loc_span())); } - if let ContextKey::Template(templ) = first_context { + if let Some(ContextKey::Template(templ)) = first_context { if !any_template_used { relevant_limitations.insert( isolated_template_limitation(templ.name.as_str()) @@ -279,16 +280,20 @@ fn symbol_implementations_of_symbol<'t>(symbol: &'t SymbolRef, } } -fn extend_with_implementations_of_symbols<'t>(semantic_lookup: &mut SemanticLookup<'t>) { - for (_device_analysis, symbols) in &mut semantic_lookup.stored_symbols { - #[allow(clippy::mutable_key_type)] - let mut symbol_collection: HashSet<_> = HashSet::default(); - for symbol in symbols.iter() { - symbol_collection.extend(symbol_implementations_of_symbol(symbol, _device_analysis)); +// NOTE: It'd be nice for some of these methods to return hashsets of symbolrefs. +// however, SymbolRefs can in fact be modified while these requests are running, +// due to symbol->reference caching, so it would break hashset invariance +// Instead, we will rely on actions.rs to make locations unique before passing to client +fn extra_implementations_of_method_symbols<'t>(semantic_lookup: &SemanticLookup<'t>) + -> Vec { + let mut full_method_symbol_set: Vec = vec![]; + for (_device_analysis, symbols) in &semantic_lookup.stored_symbols { + for symbol in symbols { + full_method_symbol_set.extend( + symbol_implementations_of_symbol(symbol, _device_analysis).into_iter()); } - symbol_collection.extend(symbols.drain(..)); - symbols.extend(symbol_collection.into_iter()); } + full_method_symbol_set } pub fn implementations_at_fp(context: &InitActionContext, @@ -300,11 +305,14 @@ pub fn implementations_at_fp(context: &InitActionContext, fp, &analysis_lock, context)?; - extend_with_implementations_of_symbols(&mut semantic_lookup); + let method_symbols = extra_implementations_of_method_symbols(&semantic_lookup); mem::swap(relevant_limitations, &mut semantic_lookup.recognized_limitations); Ok(semantic_lookup.symbols() .into_iter() - .map(|s|s.lock().unwrap().loc) + .filter(|s|s.lock().unwrap().kind != DMLSymbolKind::Method) + .flat_map(|s|s.lock().unwrap().implementations.clone()) + .chain(method_symbols.into_iter() + .map(|s|s.lock().unwrap().loc)) .collect()) } @@ -336,7 +344,13 @@ pub fn declarations_at_fp(context: &InitActionContext, mem::swap(relevant_limitations, &mut semantic_lookup.recognized_limitations); Ok(semantic_lookup.symbols() .into_iter() - .flat_map(|s|s.lock().unwrap().declarations.clone()) + .flat_map(|s|{ + let symlock = s.lock().unwrap(); + if symlock.kind == DMLSymbolKind::Method { + symlock.bases.clone() + } else { + symlock.declarations.clone() + }}) .collect()) } diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index 381b7b32..c3c1f2bd 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -1078,7 +1078,7 @@ impl DeviceAnalysis { } } - // TODO/NOTE: This method seems slightly misplaced, consider moving to analysis storage + // TODO/NOTE: This method seems slightly misplaced, consider moving to semantic_lookup pub fn lookup_symbols_by_contexted_symbol<'t>(&self, sym: &ContextedSymbol<'t>, limitations: &mut HashSet) @@ -1103,7 +1103,7 @@ impl DeviceAnalysis { }; if let Some(objs) = mb_objs { - debug!("Found {:?}", objs); + trace!("Found {:?}", objs); let mut refs = ReferenceMatches::new(); let _: Vec<_> = objs.into_iter() .map(|o|self.lookup_def_in_obj(&o, sym.symbol, &mut refs)) @@ -1175,13 +1175,43 @@ impl DeviceAnalysis { method_structure: &HashMap , ref_matches: &mut ReferenceMatches) { - // TODO: is this 100% true? - // We cannot lookup things from within a method without - // the reference being contained within the span of the method + // When resolving a noderef from an overridden method, meth here will be + // a bottom-most overriding method. So instead find the correct method + // by recursing to parent if !meth.span().contains_pos(&node.span.start_position()) { + match meth.get_default() { + Some(DefaultCallReference::Valid(defmeth)) => { + if let Some(defmeth_sym) = self.get_method_symbol(defmeth, + parent_key) { + self.resolve_noderef_in_symbol( + defmeth_sym, + &NodeRef::Simple(node.clone()), + method_structure, + ref_matches); + } + }, + Some(DefaultCallReference::Ambiguous(defmeths)) => { + for defmeth in defmeths { + if let Some(defmeth_sym) = self.get_method_symbol(defmeth, + parent_key) { + self.resolve_noderef_in_symbol( + defmeth_sym, + &NodeRef::Simple(node.clone()), + method_structure, + ref_matches); + } + } + }, + _ => { + internal_error!("Wanted to look up a reference {:?} within method {:?}, \ + but the ref is not in the method span {:?} and there are \ + no overridden methods", + node, meth.identity(), meth.span()); + } + } return; } - + trace!("Resolving simple noderef {:?} in method {:?}", node, meth); match node.val.as_str() { "this" => ref_matches.add_match(Arc::clone( @@ -1295,6 +1325,7 @@ impl DeviceAnalysis { method_structure: &HashMap, ref_matches: &mut ReferenceMatches) { + trace!("Resolving noderef {:?} in obj {:?}", node, obj); match node { NodeRef::Simple(simple) => { let resolvedobj = obj.resolve(&self.objects); @@ -1381,6 +1412,8 @@ impl DeviceAnalysis { method_structure, ref_matches); } + } else { + debug!("Failed to obtain obj from context {:?}", context_chain); } } @@ -2242,6 +2275,8 @@ impl DeviceAnalysis { info!("Match references"); let reference_cache: Mutex = Mutex::default(); for scope_chain in all_scopes(bases) { + debug!("Got scope at {:?}", scope_chain.last() + .map(|s|s.span().start_position())); self.match_references_in_scope(scope_chain, errors, method_structure, From 70cfb4c999069ec55a7be708e38ec34c1361e549 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Fri, 23 Jan 2026 15:41:29 +0100 Subject: [PATCH 10/34] Allow references to carry extra information Signed-off-by: Jonatan Waern --- src/analysis/mod.rs | 12 ++++---- src/analysis/reference.rs | 62 ++++++++++++++++++++++++--------------- 2 files changed, 43 insertions(+), 31 deletions(-) diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index c3c1f2bd..1908d76b 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -29,9 +29,7 @@ use crate::actions::analysis_storage::TimestampedStorage; use crate::actions::semantic_lookup::{DLSLimitation, isolated_template_limitation}; use crate::analysis::symbols::{DMLSymbolKind, SimpleSymbol, StructureSymbol, SymbolContainer, SymbolMaker, SymbolSource}; pub use crate::analysis::symbols::SymbolRef; -use crate::analysis::reference::{Reference, - GlobalReference, VariableReference, - ReferenceKind, NodeRef}; +use crate::analysis::reference::{GlobalReference, NodeRef, Reference, ReferenceKind, ReferenceVariant, VariableReference}; use crate::analysis::scope::{Scope, SymbolContext, ContextKey, ContextedSymbol}; use crate::analysis::parsing::parser::{FileInfo, FileParser}; @@ -1523,13 +1521,13 @@ impl DeviceAnalysis { let mut local_reports = vec![]; for reference in references { trace!("In {:?}, Matching {:?}", context_chain, reference); - let symbol_lookup = match &reference { - Reference::Variable(var) => self.find_target_for_reference( + let symbol_lookup = match &reference.variant { + ReferenceVariant::Variable(var) => self.find_target_for_reference( context_chain.as_slice(), var, method_structure, reference_cache), - Reference::Global(glob) => + ReferenceVariant::Global(glob) => self.lookup_global_from_ref(glob), }; @@ -1612,7 +1610,7 @@ impl fmt::Display for IsolatedAnalysis { |mut s, path|{ write!(s, "\t\t{:?}\n, ", path) .expect("write string error"); - s + s }))?; writeln!(f, "\ttoplevel: {}\n}}", self.toplevel)?; Ok(()) diff --git a/src/analysis/reference.rs b/src/analysis/reference.rs index a7c0f470..99ca7d54 100644 --- a/src/analysis/reference.rs +++ b/src/analysis/reference.rs @@ -102,11 +102,19 @@ pub enum ReferenceKind { } #[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] -pub enum Reference { +pub enum ReferenceVariant { Variable(VariableReference), Global(GlobalReference), } +pub type ReferenceInfo = (); + +#[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] +pub struct Reference { + pub variant: ReferenceVariant, + pub extra_info: ReferenceInfo, +} + // NOTE: The locationspan of a reference is the actual source range its // considered to be selectable at // For example: @@ -116,17 +124,17 @@ pub enum Reference { // and the declarationspan covering the full reference impl LocationSpan for Reference { fn loc_span(&self) -> &ZeroSpan { - match self { - Reference::Variable(var) => var.loc_span(), - Reference::Global(glob) => glob.loc_span(), + match &self.variant { + ReferenceVariant::Variable(var) => var.loc_span(), + ReferenceVariant::Global(glob) => glob.loc_span(), } } } impl DeclarationSpan for Reference { fn span(&self) -> &ZeroSpan { - match self { - Reference::Variable(var) => var.span(), - Reference::Global(glob) => glob.span(), + match &self.variant { + ReferenceVariant::Variable(var) => var.span(), + ReferenceVariant::Global(glob) => glob.span(), } } } @@ -134,35 +142,41 @@ impl DeclarationSpan for Reference { impl std::fmt::Display for Reference { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { - match self { - Reference::Variable(var) => var.fmt(f), - Reference::Global(glob) => glob.fmt(f), + match &self.variant { + ReferenceVariant::Variable(var) => var.fmt(f), + ReferenceVariant::Global(glob) => glob.fmt(f), } } } impl Reference { pub fn as_variable_ref(&self) -> Option<&VariableReference> { - match self { - Reference::Variable(var) => Some(var), + match &self.variant { + ReferenceVariant::Variable(var) => Some(var), _ => None, } } pub fn from_noderef(node: NodeRef, kind: ReferenceKind) -> Reference { - Reference::Variable(VariableReference { - reference: node, - kind, - }) + Reference { + variant: ReferenceVariant::Variable(VariableReference { + reference: node, + kind, + }), + extra_info: ReferenceInfo::default(), + } } pub fn global_from_string(name: String, loc: ZeroSpan, kind: ReferenceKind) -> Reference { - Reference::Global(GlobalReference { - name, - loc, - kind, - }) + Reference { + variant: ReferenceVariant::Global(GlobalReference { + name, + loc, + kind, + }), + extra_info: ReferenceInfo::default(), + } } pub fn global_from_token<'a>(token: &LeafToken, file: FileSpec<'a>, @@ -175,9 +189,9 @@ impl Reference { kind)) } pub fn reference_kind(&self) -> ReferenceKind { - match self { - Reference::Variable(r) => r.kind, - Reference::Global(r) => r.kind, + match &self.variant { + ReferenceVariant::Variable(r) => r.kind, + ReferenceVariant::Global(r) => r.kind, } } } From aa751bba68d86c557b8b5955d36240ce31a7a05d Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Fri, 23 Jan 2026 15:35:10 +0100 Subject: [PATCH 11/34] Fix goto-implementations on templates Signed-off-by: Jonatan Waern --- USAGE.md | 1 - src/analysis/mod.rs | 3 +++ src/analysis/parsing/structure.rs | 7 ++++--- src/analysis/reference.rs | 8 +++++++- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/USAGE.md b/USAGE.md index 0ba5cdf4..84cc0c2b 100644 --- a/USAGE.md +++ b/USAGE.md @@ -75,7 +75,6 @@ template declaration site. `goto-implementations` will give each location where the template is directly instantiated. -TODO: does not actually work `goto-references` will go to any location where the template is referred to, including direct instantiation sites (so this is a super-set of diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index 1908d76b..b3845548 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -1502,6 +1502,9 @@ impl DeviceAnalysis { reference: &Reference) { let mut sym = symbol.lock().unwrap(); sym.references.insert(*reference.loc_span()); + if sym.kind == DMLSymbolKind::Template && reference.extra_info.was_instantiation { + sym.implementations.insert(*reference.loc_span()); + } } #[allow(clippy::ptr_arg)] diff --git a/src/analysis/parsing/structure.rs b/src/analysis/parsing/structure.rs index ee9e647b..51822e1d 100644 --- a/src/analysis/parsing/structure.rs +++ b/src/analysis/parsing/structure.rs @@ -628,9 +628,10 @@ impl TreeElement for Instantiation { |(tok,_)|tok).collect(), }; - let refs = toks.into_iter().filter_map( - |tok|Reference::global_from_token( - tok, file, ReferenceKind::Template)); + let refs = toks.into_iter() + .filter_map(|tok|Reference::global_from_token( + tok, file, ReferenceKind::Template)) + .map(|mut r|{r.extra_info.was_instantiation = true; r}); accumulator.extend(refs); } } diff --git a/src/analysis/reference.rs b/src/analysis/reference.rs index 99ca7d54..6321e0d8 100644 --- a/src/analysis/reference.rs +++ b/src/analysis/reference.rs @@ -107,7 +107,13 @@ pub enum ReferenceVariant { Global(GlobalReference), } -pub type ReferenceInfo = (); +// NOTE: Since this information is likely to be sparse, use +// boxes for any non-trivially small fields. If fields get to be more +// than a few, the entire info should probably be boxed instead +#[derive(Debug, Default, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] +pub struct ReferenceInfo { + pub was_instantiation: bool, +} #[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] pub struct Reference { From 45031dd78295636387aee36a88fab2a870eb75a7 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 26 Jan 2026 10:35:21 +0100 Subject: [PATCH 12/34] Fix goto-definition on abstract methods Signed-off-by: Jonatan Waern --- USAGE.md | 3 ++- src/actions/semantic_lookup.rs | 23 ++++++++++++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/USAGE.md b/USAGE.md index 84cc0c2b..e02f35d2 100644 --- a/USAGE.md +++ b/USAGE.md @@ -49,7 +49,8 @@ for this method, this will usually be a 'default' or abstract declaration. `goto-definition` on a method reference will find all defintions of that method that could be called from that reference. Note that this will NOT point to method declarations that are entirely overridden. `goto-definition` on a method -name is a no-op and will just point back to that same method. +name will go to the nearest definition of the method. For non-abstract methods +this is a no-op. `goto-implementations` on a method will find all method declarations that could override it. diff --git a/src/actions/semantic_lookup.rs b/src/actions/semantic_lookup.rs index 1877ca79..0fba1f60 100644 --- a/src/actions/semantic_lookup.rs +++ b/src/actions/semantic_lookup.rs @@ -11,6 +11,7 @@ use std::sync::Arc; use crate::actions::analysis_storage::{AnalysisLookupError, AnalysisStorage}; use crate::actions::{ContextDefinition, InitActionContext}; use crate::analysis::scope::{ContextedSymbol, ContextKey}; +use crate::analysis::structure::objects::MaybeAbstract; use crate::analysis::symbols::DMLSymbolKind; use crate::analysis::{DeviceAnalysis, IsolatedAnalysis, LocationSpan, SymbolRef}; @@ -316,6 +317,26 @@ pub fn implementations_at_fp(context: &InitActionContext, .collect()) } +fn definitions_of_symbol<'t>(symbol: &'t SymbolRef) + -> Vec { + let symbol_lock = symbol.lock().unwrap(); + if symbol_lock.kind == DMLSymbolKind::Method { + if let Some((_, methsrc)) = symbol_lock.source.as_method() { + if methsrc.is_abstract() { + symbol_lock.implementations.iter() + .cloned().collect() + } else { + symbol_lock.definitions.clone() + } + } else { + internal_error!("Expected method symbol source for method symbol {:?}", symbol_lock); + vec![] + } + } else { + symbol_lock.definitions.clone() + } +} + pub fn definitions_at_fp(context: &InitActionContext, fp: &ZeroFilePosition, relevant_limitations: &mut HashSet) @@ -328,7 +349,7 @@ pub fn definitions_at_fp(context: &InitActionContext, mem::swap(relevant_limitations, &mut semantic_lookup.recognized_limitations); Ok(semantic_lookup.symbols() .into_iter() - .flat_map(|s|s.lock().unwrap().definitions.clone()) + .flat_map(|s|definitions_of_symbol(&s)) .collect()) } From a9ae32a380b7f2e464979ca2be89fd2d145a6ecf Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 26 Jan 2026 16:48:32 +0100 Subject: [PATCH 13/34] Fix a debug print Signed-off-by: Jonatan Waern --- src/analysis/templating/traits.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/analysis/templating/traits.rs b/src/analysis/templating/traits.rs index 8eba464f..7b10c066 100644 --- a/src/analysis/templating/traits.rs +++ b/src/analysis/templating/traits.rs @@ -260,13 +260,12 @@ impl DMLTrait { traits: &HashMap>, report: &mut Vec) -> Arc { - debug!("Processing trait for {}", - traits.keys().fold( + debug!("Processing trait for {}", name); + trace!("other traits are {:?}", traits.keys().fold( "".to_string(), |mut s,k|{write!(s, "{}, ", k) .expect("write string error"); s})); - trace!("other traits are {:?}", name,); // Skip analysis for dummy trait if maybeloc.is_none() { trace!("Dummy trait, skipped"); From 2762dc154bed22274855d1067584c5c7f211f6b0 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 26 Jan 2026 11:20:17 +0100 Subject: [PATCH 14/34] Fix symbol binding around indirect method overrides Signed-off-by: Jonatan Waern --- src/analysis/templating/objects.rs | 88 +++++++++++++++++++++--------- src/analysis/templating/traits.rs | 24 +++++--- 2 files changed, 78 insertions(+), 34 deletions(-) diff --git a/src/analysis/templating/objects.rs b/src/analysis/templating/objects.rs index 9f29ef52..b1326fb3 100644 --- a/src/analysis/templating/objects.rs +++ b/src/analysis/templating/objects.rs @@ -33,8 +33,7 @@ use crate::analysis::templating::topology::{InEachStruct, InferiorVariant, Rank, RankDesc, RankDescKind, RankMaker, topsort}; use crate::analysis::templating::traits::{DMLTemplate, DMLTrait, - get_impls, merge_impl_maps, - TraitMemberKind, TemplateTraitInfo}; + get_impls, TraitMemberKind, TemplateTraitInfo}; use crate::analysis::{LocationSpan, DeclarationSpan, combine_vec_of_decls}; type InEachSpec = HashMap, Arc)>>; @@ -1768,7 +1767,7 @@ fn merge_composite_subobjs<'c>(parent_each_stmts: &InEachSpec, // and 'order' is a order of methods, with lowest rank being last fn sort_method_decls<'t>(decls: &[(&Rank, &'t MethodDecl)]) -> (HashMap<&'t MethodDecl, HashSet<&'t MethodDecl>>, Vec<&'t MethodDecl>) { - trace!("Sorting method declarations: {:?}", decls); + debug!("Sorting method decls {:?}", decls); let mut rank_to_method: HashMap<&Rank, (Vec<&MethodDecl>, Vec<&MethodDecl>)> = HashMap::default(); @@ -1780,15 +1779,55 @@ fn sort_method_decls<'t>(decls: &[(&Rank, &'t MethodDecl)]) -> defs.push(*decl); } } - + trace!("rank-to-method is: {:?}", rank_to_method); let ranks: HashSet<&Rank> = rank_to_method.keys() .cloned().collect(); - - let ancestry: HashMap<&Rank, Vec<&Rank>> = ranks.iter() + let mut ancestry: HashMap<&Rank, Vec<&Rank>> = ranks.iter() .map(|r|(*r, ranks.iter().filter( |r2|r.inferior.contains(&r2.id)).cloned().collect())) .collect(); + // Insert implicit special ancestry ordering to resolve unrelated ranks + // where a non-abstract method can override an abstract method + trace!("Initial ancestry is {:?}", ancestry); + let top_ancestry: HashSet<&Rank> = ranks.iter() + .filter(|a|!ancestry.values().any(|ra|ra.contains(*a))) + .cloned() + .collect(); + trace!("Top ancestry ranks are {:?}", top_ancestry); + let new_edges = { + let mut top_defs = vec![]; + let mut top_decls = vec![]; + for r in top_ancestry { + let (defs, decls) = rank_to_method.get(r).unwrap(); + top_defs.extend(defs.iter().map(|d|(r, *d))); + top_decls.extend(decls.iter().map(|d|(r, *d))); + } + if top_defs.len() == 1 { + let (top_rank, top_def) = &top_defs[0]; + if !top_def.is_abstract() { + top_decls.iter().filter_map(|(subrank, subdecl)| + // In-rank case is handled in method sorting + if subdecl.is_abstract() && top_rank != subrank { + Some((top_rank, subrank)) + } else { + None + }) + .map(|(from, to)|(*from, *to)) + .collect() + } else { + vec![] + } + } else { + vec![] + } + }; + + for (from, to) in new_edges { + trace!("Inserted implicit ancestry edge from {:?} to {:?}", from, to); + ancestry.entry(from).or_default().push(to); + } + trace!("Ancestry is {:?}", ancestry); let mut minimal_ancestry: HashMap<&Rank, Vec<&Rank>> = HashMap::default(); // Get ancestors that are not the ancestor of any ancestor @@ -1803,8 +1842,6 @@ fn sort_method_decls<'t>(decls: &[(&Rank, &'t MethodDecl)]) -> minimal_ancestry.insert(rank, minimal_ancestors); } - trace!("Minimal ancestors are {:?}", minimal_ancestry); - // Map method declaration to those that they directly override let mut method_map: HashMap<&'t MethodDecl, HashSet<&'t MethodDecl>> = HashMap::default(); @@ -1838,7 +1875,7 @@ fn sort_method_decls<'t>(decls: &[(&Rank, &'t MethodDecl)]) -> fn add_methods(obj: &mut DMLCompositeObject, methods: MethodMapping, - name_to_trait_map: &HashMap>, + decl_to_trait_map: &HashMap>, report: &mut Vec) { debug!("Adding methods to {}", obj.identity()); @@ -1849,18 +1886,10 @@ fn add_methods(obj: &mut DMLCompositeObject, } trace!("Handling method {}, which is declared by {:?}", name, regular_decls); - let mut all_decls: Vec<(&Rank, &MethodDecl)> + let all_decls: Vec<(&Rank, &MethodDecl)> = regular_decls.iter().map(|(a, b)|(a, b)).collect(); - // Note: Sorting and overrides between methods is sorted (hah) already - // find the topmost relevant method decl - if let Some(trait_ref) = name_to_trait_map.get(&name) { - if let Some(trait_decl) = trait_ref.get_method(&name) { - all_decls.push((&trait_ref.rank, trait_decl)); - } - } - trace!("All possible declarations are: {:?}", all_decls); + let (default_map, method_order) = sort_method_decls(&all_decls); - trace!("Method order is: {:?}", method_order); let mut decl_to_method: HashMap> = HashMap::default(); for method in method_order.iter().rev() { @@ -1905,7 +1934,8 @@ fn add_methods(obj: &mut DMLCompositeObject, method.check_override(*decl, report); } - let template_ref = name_to_trait_map.get(&name) + let template_ref = decl_to_trait_map + .get(method) .map(Arc::clone); let new_method = Arc::new( @@ -2002,7 +2032,13 @@ fn check_trait_overrides(obj: &DMLCompositeObject, variant: DMLShallowObjectVariant::Method(m), .. }) = collision { - m.check_override(meth, report); + // Special case, we are allowed conflict with + // an abstract method if we can override it + if m.is_abstract() { + meth.check_override(m.get_decl(), report); + } else { + m.check_override(meth, report); + } } } } else if let TraitMemberKind::Method(meth) = member { @@ -2220,10 +2256,12 @@ pub fn make_object(loc: ZeroSpan, add_subobjs(new_obj_key, subobj_keys, container); { let new_obj = container.get_mut(new_obj_key).unwrap(); - let trait_method_map = merge_impl_maps(&identity.val, &loc, - new_obj.templates.values().map( - |t|&t.traitspec), - report); + let trait_method_map = new_obj.templates.values() + .flat_map(|t|t.traitspec.methods.values() + .map(|m|(m.decl.clone(), Arc::clone(&t.traitspec))) + .collect::>()) + .collect(); + add_methods(new_obj, methods, &trait_method_map, report); } check_trait_overrides(container.get(new_obj_key).unwrap(), diff --git a/src/analysis/templating/traits.rs b/src/analysis/templating/traits.rs index 7b10c066..2c1c1fb0 100644 --- a/src/analysis/templating/traits.rs +++ b/src/analysis/templating/traits.rs @@ -630,7 +630,7 @@ where // Methods can override each other on trait level, // other declarations cannot match (decl, edecl) { - (TraitMemberKind::Method(_), TraitMemberKind::Method(_)) + (TraitMemberKind::Method(m1), TraitMemberKind::Method(m2)) => match relation { ImplementRelation::Implements => { map.insert(name, implr); @@ -643,14 +643,20 @@ where // analysis. // A minor improvement would be to pick both ImplementRelation::Unrelated => { - map.insert(name.clone(), Arc::clone(&mimplr)); - if let Some(ambdef) = - ambiguous_defs.get_mut(&name) { - ambdef.push(implr); - } - else { - ambiguous_defs.insert(name, - vec![mimplr, implr]); + // This conflict is allowed as long as exactly 1 + // is abstract + if m1.is_abstract() && !m2.is_abstract() { + map.insert(name, mimplr); + + } else if !m1.is_abstract() && m2.is_abstract() { + map.insert(name, implr); + } else { + // Both abstract or both non-abstract + // ambiguous + map.insert(name.clone(), Arc::clone(&implr)); + ambiguous_defs.entry(name) + .or_default() + .push(mimplr); } }, _ => unreachable!("should be covered by if clause"), From 6b9fa2d8637cbe9bcf544b62f5fcc06cd7c618ce Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Tue, 27 Jan 2026 10:54:36 +0100 Subject: [PATCH 15/34] Fix inconsistencies in lookups - Previously, goto-x at the start of a name would not work, while goto-x at the end or middle of it would. - If there is no definition for a lookup, fall-back to declaration Signed-off-by: Jonatan Waern --- CHANGELOG.md | 11 +++++++---- USAGE.md | 12 +++++++----- src/actions/semantic_lookup.rs | 23 ++++++++++++++++------- 3 files changed, 30 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index be294697..0cbdcc46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,10 +11,13 @@ - The DLS will no longer fail to parse files with unicode characters in cblock header or footers, or in multiline comments - The DLS will now report when an ambiguous default call is made -- Improved how definitions and references from/to methods and default calls are resolved, - goto-definition on a default call will now only go to the method it will call, - and goto-references on a method definition will no longer go to default declarations - that cannot call it +- Improvements and clarifications to connections between symbols and references, + for details, see [USAGE.md](USAGE.md). +-- Method declared in unrelated templates in an allowed way will now have their + references correctly resolved. +-- Goto-reference on default calls will now go to the methods that may be called. +-- Goto-implementation on templates will now go to all places where they are + instantiated. ## 0.9.17 - Fixed linter wrongly throwing an error on space after `defined` keyword diff --git a/USAGE.md b/USAGE.md index e02f35d2..90028f78 100644 --- a/USAGE.md +++ b/USAGE.md @@ -34,20 +34,22 @@ See issues [#13](https://github.com/intel/dml-language-server/issues/13), ### On Composite Objects (banks, registers, implements, etc.) `goto-declaration` and `goto-definition` on an composite object are equivalent. -They will pointreturn the locations of all object declarations that may be +They will find the locations of all object declarations that may be merged with the one at the name. `goto-implementations` on objects is currently unused. -`goto-references` will go to any location where the object is referred to +`goto-references` will go to any location where the symbol is referred to directly. ### On Methods `goto-declaration` on a method will find the most-overridden declaration -for this method, this will usually be a 'default' or abstract declaration. +or definition of the method, this will usually be a 'default' or +abstract declaration. -`goto-definition` on a method reference will find all defintions of that method -that could be called from that reference. Note that this will NOT point to +`goto-definition` on a method reference will find all definitions of that method +that could be called from that reference, or the declaration of the method if it +has no non-abstract definitions. Note that this will NOT point to method declarations that are entirely overridden. `goto-definition` on a method name will go to the nearest definition of the method. For non-abstract methods this is a no-op. diff --git a/src/actions/semantic_lookup.rs b/src/actions/semantic_lookup.rs index 0fba1f60..d3190a25 100644 --- a/src/actions/semantic_lookup.rs +++ b/src/actions/semantic_lookup.rs @@ -151,6 +151,7 @@ fn get_refs_and_syms_at_fp<'t>( analysis_info: &AnalysisInfo<'t>, relevant_limitations: &mut HashSet) -> Result, AnalysisLookupError> { + debug!("Looking up references and symbols at position {:?}", fp); let ref_at_pos = analysis_info.isolated_analysis.lookup_reference(fp); let context_sym_at_pos = context_symbol_at_pos(analysis_info.isolated_analysis, fp); @@ -317,17 +318,25 @@ pub fn implementations_at_fp(context: &InitActionContext, .collect()) } -fn definitions_of_symbol<'t>(symbol: &'t SymbolRef) +fn definitions_of_symbol<'t>(symbol: &'t SymbolRef, refr: Option<&Reference>) -> Vec { let symbol_lock = symbol.lock().unwrap(); if symbol_lock.kind == DMLSymbolKind::Method { if let Some((_, methsrc)) = symbol_lock.source.as_method() { - if methsrc.is_abstract() { - symbol_lock.implementations.iter() - .cloned().collect() - } else { - symbol_lock.definitions.clone() + let mut to_return = vec![]; + // If we goto-def directly on an abstract method, + // give its first-level overrides + if methsrc.is_abstract() && refr.is_none() { + to_return = symbol_lock.implementations.iter() + .cloned().collect(); + } + if to_return.is_empty() { + to_return = symbol_lock.definitions.clone(); + } + if to_return.is_empty() { + to_return = symbol_lock.declarations.clone(); } + to_return } else { internal_error!("Expected method symbol source for method symbol {:?}", symbol_lock); vec![] @@ -349,7 +358,7 @@ pub fn definitions_at_fp(context: &InitActionContext, mem::swap(relevant_limitations, &mut semantic_lookup.recognized_limitations); Ok(semantic_lookup.symbols() .into_iter() - .flat_map(|s|definitions_of_symbol(&s)) + .flat_map(|s|definitions_of_symbol(&s, semantic_lookup.found_ref.as_ref())) .collect()) } From 4f81075572bcc99248696b86cc2939f3b4a6c734 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Wed, 28 Jan 2026 11:39:23 +0100 Subject: [PATCH 16/34] Add explicit method decl provisional Signed-off-by: Jonatan Waern --- CHANGELOG.md | 3 ++- src/analysis/parsing/structure.rs | 5 ++++- src/analysis/provisionals.rs | 2 ++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cbdcc46..bce1cb0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ -- Goto-reference on default calls will now go to the methods that may be called. -- Goto-implementation on templates will now go to all places where they are instantiated. +- Added parser support for provisional 'explicit\_method\_decls' ## 0.9.17 - Fixed linter wrongly throwing an error on space after `defined` keyword @@ -29,7 +30,7 @@ ## 0.9.15 - Added support for line length and breaking rules regarding line-breaks after opening parentheses, method output arguments, conditional expressions and binary operands. -- Added support for indendation rule indent_continuation_line. +- Added support for indendation rule indent\_continuation\_line. - Optimizations in how the server resolves file paths, should reduce time-to-ready for the server when first starting by about 50%, depending on the complexity of the include tree. diff --git a/src/analysis/parsing/structure.rs b/src/analysis/parsing/structure.rs index 51822e1d..c0b9104b 100644 --- a/src/analysis/parsing/structure.rs +++ b/src/analysis/parsing/structure.rs @@ -97,6 +97,7 @@ pub struct MethodContent { pub returns: Option, pub throws: Option, pub default: Option, + pub colon: Option, pub statements: Statement, } @@ -327,12 +328,14 @@ fn parse_method(modifier: Option, }; let throws = pre_statements_context.next_if_kind( stream, TokenKind::Throws); + let colon = pre_statements_context.next_if_kind( + stream, TokenKind::Colon); let default = pre_statements_context.next_if_kind( stream, TokenKind::Default); let statements = Statement::parse(&outer, stream, file_info); DMLObjectContent::Method(MethodContent { modifier, independent, startup, memoized, method, name, lparen, - arguments, rparen, returns, throws, default, statements + arguments, rparen, returns, throws, colon, default, statements }).into() } diff --git a/src/analysis/provisionals.rs b/src/analysis/provisionals.rs index 14d46ea1..a921ac7c 100644 --- a/src/analysis/provisionals.rs +++ b/src/analysis/provisionals.rs @@ -14,6 +14,7 @@ use crate::analysis::FileSpec; #[strum(serialize_all = "snake_case")] pub enum Provisional { // TODO: implement the neccessary checks for explicit params + ExplicitMethodDecls, ExplicitParamDecls, SimicsUtilVect, } @@ -70,6 +71,7 @@ mod test { fn test_provisionals_parsing() { for (s, p) in [ ("explicit_param_decls", Provisional::ExplicitParamDecls), + ("explicit_method_decls", Provisional::ExplicitMethodDecls), ("simics_util_vect", Provisional::SimicsUtilVect), ] { assert_eq!(Provisional::from_str(s), Ok(p)); From 81e8dd0b1908bbe00669e87e2a072478504133c9 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Wed, 28 Jan 2026 17:04:36 +0100 Subject: [PATCH 17/34] Correctly report missing templates in in-each Signed-off-by: Jonatan Waern --- CHANGELOG.md | 1 + src/analysis/templating/topology.rs | 36 +++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bce1cb0a..a4b7f1d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ -- Goto-implementation on templates will now go to all places where they are instantiated. - Added parser support for provisional 'explicit\_method\_decls' +- The DLS will now correctly report missing template names in 'in each' constructs ## 0.9.17 - Fixed linter wrongly throwing an error on space after `defined` keyword diff --git a/src/analysis/templating/topology.rs b/src/analysis/templating/topology.rs index fa4de98d..539297e0 100644 --- a/src/analysis/templating/topology.rs +++ b/src/analysis/templating/topology.rs @@ -627,6 +627,42 @@ pub fn rank_templates_aux<'t>(mut templates: HashMap<&'t str, debug!("Implicit import of '{}' missing and intentionally ignored", missing_template_name); }, + InferiorVariant::InEach(decl) => { + if !BUILTIN_TEMPLATES.iter().any( + |name|name==missing_template_name) { + // The 'is' may contain more names than we + // are looking for, find the particular name + // that matches and use that span + // Because the same name could be used multiple + // times, but this only causes inferior binding + // report one error per such span + + let spans: Vec<_> = decl.obj.spec.iter() + .filter(|dmlname| + &dmlname.val == missing_template_name) + .map(|dmlname|dmlname.span()) + .collect(); + if spans.is_empty() { + internal_error!("Unexpectedly no name \ + matching missing template \ + in {:?} (wanted {})", + decl, missing_template_name); + continue; + } + for span in spans { + report.push( + DMLError { + span: *span, + description: format!( + "No template; '{}'", + missing_template_name), + related: vec![], + severity: Some( + DiagnosticSeverity::ERROR), + }); + } + } + }, inf => { internal_error!( "Unexpected template dependency through {:?}", From 72afc4f2a2539bbd7f94c1a10072944c5df5877b Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Thu, 29 Jan 2026 16:20:13 +0100 Subject: [PATCH 18/34] Allow reference-in-method to fall through default calls This can happen when methods are ambigiously overridden or overridden in a diamond shape. we will correctly search the entire hierarchy until we find the method from whence the reference came, but we will also search through adjacent hierarchies, so reaching the bottom of those is not actually an unexpected error Signed-off-by: Jonatan Waern --- src/analysis/mod.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index b3845548..c38313a5 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -1201,10 +1201,7 @@ impl DeviceAnalysis { } }, _ => { - internal_error!("Wanted to look up a reference {:?} within method {:?}, \ - but the ref is not in the method span {:?} and there are \ - no overridden methods", - node, meth.identity(), meth.span()); + trace!("Fell through recursive method noderef resolution for {:?} in method {:?}", node, meth); } } return; From 5b878327133db139d8324d750f22865dea35c3fb Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 2 Feb 2026 11:26:15 +0100 Subject: [PATCH 19/34] Only bind implementations between methods within the same object Signed-off-by: Jonatan Waern --- src/analysis/mod.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index c38313a5..6b562d51 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -2535,8 +2535,9 @@ pub fn from_device_and_bases<'a>(_device: &'a IsolatedAnalysis, fn bind_method_implementations(method_symbols: &mut HashMap>) { debug!("Bind method implementations"); - for method_symbol in method_symbols.values().flat_map(|m| m.values()) { - debug!("Binding for {}", method_symbol.lock().unwrap().medium_debug_info()); + for (parent, method_symbol) in method_symbols.values().flat_map(|m|m.iter()) { + debug!("Binding for method {} under {:?}", method_symbol.lock().unwrap().medium_debug_info(), parent); + // Cloning the arc is not strictly necessary, but avoiding holding the lock is good practice let method = match &method_symbol.lock().unwrap().source { SymbolSource::Method(_, methref) => Arc::clone(methref), @@ -2549,7 +2550,7 @@ fn bind_method_implementations(method_symbols: &mut HashMap>()) .collect::>(); debug!("Default decls are {:?}", default_decls); - let parent_symbols = default_decls.into_iter() + let overridden_methods = default_decls.into_iter() .filter_map(|d| if let Some(parent_syms) = method_symbols.get(d.location()) { Some(parent_syms) @@ -2557,10 +2558,10 @@ fn bind_method_implementations(method_symbols: &mut HashMap Date: Mon, 2 Feb 2026 14:31:24 +0100 Subject: [PATCH 20/34] Bind implementations on object symbols to in-eachs that apply to them Signed-off-by: Jonatan Waern --- CHANGELOG.md | 4 +- USAGE.md | 3 +- src/analysis/mod.rs | 1 + src/analysis/structure/objects.rs | 5 +++ src/analysis/templating/objects.rs | 64 ++++++++++++++++++------------ 5 files changed, 50 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4b7f1d7..7397c7ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,8 +16,10 @@ -- Method declared in unrelated templates in an allowed way will now have their references correctly resolved. -- Goto-reference on default calls will now go to the methods that may be called. --- Goto-implementation on templates will now go to all places where they are +-- Goto-implementations on templates will now go to all places where they are instantiated. +-- Goto-implementations on objects will now go to all the 'in each' declarations + which apply to that object. - Added parser support for provisional 'explicit\_method\_decls' - The DLS will now correctly report missing template names in 'in each' constructs diff --git a/USAGE.md b/USAGE.md index 90028f78..09468e4f 100644 --- a/USAGE.md +++ b/USAGE.md @@ -37,7 +37,8 @@ See issues [#13](https://github.com/intel/dml-language-server/issues/13), They will find the locations of all object declarations that may be merged with the one at the name. -`goto-implementations` on objects is currently unused. +`goto-implementations` on objects will find all `in each` declaration block that +applies to that object. `goto-references` will go to any location where the symbol is referred to directly. diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index 6b562d51..a8a54ab3 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -1847,6 +1847,7 @@ fn new_symbol_from_object(maker: &SymbolMaker, SymbolSource::DMLObject(DMLObject::CompObject(object.key)), definitions = all_decl_defs.clone(), declarations = all_decl_defs.clone(), + implementations = object.used_ineach_locs.clone().into_iter().collect(), // TODO: this does not follow from the new definition of bases bases = all_decl_defs) } diff --git a/src/analysis/structure/objects.rs b/src/analysis/structure/objects.rs index 6e34f51a..4d7d5376 100644 --- a/src/analysis/structure/objects.rs +++ b/src/analysis/structure/objects.rs @@ -325,6 +325,7 @@ impl Import { #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct InEach { + pub loc: ZeroSpan, pub span: ZeroSpan, pub spec: Vec, pub statements: Statements, @@ -399,6 +400,10 @@ impl ToStructure for InEach { content.spec.references(&mut references, file); content.statements.references(&mut references, file); Some(InEach { + loc: ZeroSpan::from_range( + ZeroRange::combine(content.intok.range(), + content.each.range()), + file.path), span, spec, statements, diff --git a/src/analysis/templating/objects.rs b/src/analysis/templating/objects.rs index b1326fb3..5015e14c 100644 --- a/src/analysis/templating/objects.rs +++ b/src/analysis/templating/objects.rs @@ -36,7 +36,7 @@ use crate::analysis::templating::traits::{DMLTemplate, DMLTrait, get_impls, TraitMemberKind, TemplateTraitInfo}; use crate::analysis::{LocationSpan, DeclarationSpan, combine_vec_of_decls}; -type InEachSpec = HashMap, Arc)>>; +type InEachSpec = HashMap, (ZeroSpan, Arc))>>; pub type StructureKey = DefaultKey; pub type StructureContainer = SlotMap; @@ -181,7 +181,7 @@ fn create_spec<'t>(loc: ZeroSpan, if let Some((first, rest)) = ineach.obj.spec.split_first() { if let Some(in_each_spec) = in_each_specs.get(ineach) { let to_add = (rest.iter().map(|t|t.val.clone()).collect(), - Arc::clone(in_each_spec)); + (ineach.obj.loc, Arc::clone(in_each_spec))); if let Some(e) = in_eachs.get_mut(&first.val) { e.push(to_add); } else { @@ -819,6 +819,8 @@ pub struct DMLCompositeObject { // None: 'none' for size in arraydim here means an _unknown_ size pub arraydimvars: Vec, pub components: HashMap, + // Used for goto-implementation on composite objects + pub used_ineach_locs: Vec, } impl DMLCompositeObject { @@ -917,8 +919,10 @@ impl DMLCompositeObject { // we need to add) // NOTE: new objectspecs are pathed with the full condition chain required // to instantiate them +// Returns all spans of final actually used ineachspecs, needed for +// implementations tracking for composite objects fn add_template_specs(obj_specs: &mut Vec>, - source_each_stmts: &InEachSpec) { + source_each_stmts: &InEachSpec) -> Vec{ let mut each_stmts = source_each_stmts.clone(); // TODO: We need to handle conditional imports and is-es here, as these are // allowed in _some_ cases. For now, we merely pretend the conditions do not @@ -945,6 +949,8 @@ fn add_template_specs(obj_specs: &mut Vec>, // true in whatever context it would work in let mut used_templates = HashSet::::default(); + let mut used_ineach_spans = vec![]; + while let Some((_, tpl)) = queue.pop() { // TODO: here we would have to consider cond when conditional // is/imports exist @@ -955,31 +961,35 @@ fn add_template_specs(obj_specs: &mut Vec>, tpl.name); used_templates.insert(tpl.name.to_string()); let mut modifications = vec![]; - if let Some(templ_specs) = each_stmts.get(&tpl.name) { - for (needed_templates, spec) in templ_specs { - let mut can_add = true; - for templ in needed_templates { - if !used_templates.contains(templ.as_str()) { - // We will need to re-add a check for this template, - // in case it will appear later in the queue - modifications.push((templ.clone(), - needed_templates.clone(), - Arc::clone(spec))); - can_add = false; - break; + { + if let Some(templ_specs) = each_stmts.get(&tpl.name) { + for (needed_templates, (loc, spec)) in templ_specs { + let mut can_add = true; + for templ in needed_templates { + if !used_templates.contains(templ.as_str()) { + // We will need to re-add a check for this template, + // in case it will appear later in the queue + modifications.push( + (templ.clone(), + needed_templates.clone(), + (*loc, Arc::clone(spec)))); + can_add = false; + break; + } + } + if can_add { + queue.extend(spec.instantiations.values() + .flat_map(|v|v.iter()) + .map(|t|(spec.condition.clone(), + Arc::clone(t)))); + obj_specs.push(Arc::clone(spec)); + used_ineach_spans.push(*loc); } - } - if can_add { - queue.extend(spec.instantiations.values() - .flat_map(|v|v.iter()) - .map(|t|(spec.condition.clone(), - Arc::clone(t)))); - obj_specs.push(Arc::clone(spec)); } } } - for (name, templs, spec) in modifications { - let to_insert = (templs, spec); + for (name, templs, (loc, spec)) in modifications { + let to_insert = (templs, (loc, spec)); if let Some(e) = each_stmts.get_mut(&name) { e.push(to_insert); } else { @@ -995,6 +1005,8 @@ fn add_template_specs(obj_specs: &mut Vec>, queue.push((d.cond.clone(), Arc::clone(v))); } } + + used_ineach_spans } // Simply adds the in_eachs specified in object specs to the ineachspec @@ -1336,6 +1348,7 @@ fn create_object_instance(loc: Option, declloc: loc.unwrap_or(identity.span), all_decls, identity: identity.clone(), + used_ineach_locs: vec![], key: StructureKey::null(), kind, definitions: vec![], @@ -2208,7 +2221,7 @@ pub fn make_object(loc: ZeroSpan, debug!("Making object {}", identity.val); let mut each_stmts = parent_each_stmts.clone(); - add_template_specs(&mut obj_specs, &each_stmts); + let used_ineach_locs = add_template_specs(&mut obj_specs, &each_stmts); add_template_ineachs(&obj_specs, &mut each_stmts); trace!("Has specs at {:?}", obj_specs.iter().map(|rc|rc.loc) @@ -2244,6 +2257,7 @@ pub fn make_object(loc: ZeroSpan, { let new_obj = container.get_mut(new_obj_key).unwrap(); new_obj.definitions = obj_specs.clone(); + new_obj.used_ineach_locs = used_ineach_locs; add_templates(new_obj, &obj_specs); add_constants(new_obj, constants); add_parameters(new_obj, parameters); From 96e9b8dbbc8be36ea7705fa31435d737eeade9ae Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Thu, 5 Feb 2026 16:02:55 +0100 Subject: [PATCH 21/34] Fix bug in goto-decl on parameters Signed-off-by: Jonatan Waern --- src/actions/semantic_lookup.rs | 2 +- src/analysis/mod.rs | 10 +++++----- src/analysis/templating/objects.rs | 9 ++++++--- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/actions/semantic_lookup.rs b/src/actions/semantic_lookup.rs index d3190a25..1d7ef989 100644 --- a/src/actions/semantic_lookup.rs +++ b/src/actions/semantic_lookup.rs @@ -376,7 +376,7 @@ pub fn declarations_at_fp(context: &InitActionContext, .into_iter() .flat_map(|s|{ let symlock = s.lock().unwrap(); - if symlock.kind == DMLSymbolKind::Method { + if matches!(symlock.kind, DMLSymbolKind::Method | DMLSymbolKind::Parameter){ symlock.bases.clone() } else { symlock.declarations.clone() diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index a8a54ab3..7459a60e 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -1938,11 +1938,11 @@ fn add_new_symbol_from_shallow(maker: &SymbolMaker, ) { let (bases, definitions, declarations) = match &shallow.variant { DMLShallowObjectVariant::Parameter(param) => - (vec![*param.get_likely_declaration().loc_span()], + (vec![*param.get_last_declaration().loc_span()], param.used_definitions.iter() - .map(|(_, def)|*def.loc_span()).collect(), + .map(|(_, def)|*def.loc_span()).collect(), param.declarations.iter() - .map(|(_, def)|*def.loc_span()).collect()), + .map(|(_, def)|*def.loc_span()).collect()), DMLShallowObjectVariant::Method(method_ref) => return add_new_symbol_from_method(maker, &shallow.parent, method_ref, errors, storage, method_structure), DMLShallowObjectVariant::Constant(constant) => @@ -1959,7 +1959,7 @@ fn add_new_symbol_from_shallow(maker: &SymbolMaker, vec![*hook.loc_span()], vec![*hook.loc_span()]), }; - debug!("Made shallow symbol for {:?}", shallow); + let new_sym = symbol_ref!( maker, *shallow.location(), @@ -1971,7 +1971,7 @@ fn add_new_symbol_from_shallow(maker: &SymbolMaker, bases = bases, definitions = definitions, declarations = declarations); - + debug!("Made shallow symbol {:?}", new_sym); match &shallow.variant { DMLShallowObjectVariant::Parameter(_) => { log_non_same_insert(storage.param_symbols.entry( diff --git a/src/analysis/templating/objects.rs b/src/analysis/templating/objects.rs index 5015e14c..d6bd4359 100644 --- a/src/analysis/templating/objects.rs +++ b/src/analysis/templating/objects.rs @@ -746,13 +746,16 @@ impl DMLAmbiguousDef { } } - pub fn get_likely_declaration(&self) -> &T { - if let Some(def) = self.declarations.first() { + pub fn get_last_declaration(&self) -> &T { + if let Some(decl) = self.declarations.last() { + &decl.1 + } else if let Some(def) = self.definitions.last() { &def.1 } else { - &self.used_definitions.first().unwrap().1 + &self.used_definitions.last().unwrap().1 } } + pub fn get_unambiguous_def(&self) -> Option<&T> { if self.used_definitions.len() == 1 { self.used_definitions.first().map(|t|&t.1) From 6985f56c218db371b833a2ada607ae6cf12eb7a9 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 9 Feb 2026 10:27:57 +0100 Subject: [PATCH 22/34] Do not return abstract method declaration in goto-impl Signed-off-by: Jonatan Waern --- src/actions/semantic_lookup.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/actions/semantic_lookup.rs b/src/actions/semantic_lookup.rs index 1d7ef989..46c270f2 100644 --- a/src/actions/semantic_lookup.rs +++ b/src/actions/semantic_lookup.rs @@ -252,15 +252,18 @@ fn symbol_implementations_of_symbol<'t>(symbol: &'t SymbolRef, let mut next_iteration = vec![symbol]; while let Some(next) = next_iteration.pop() { let next_lock = next.lock().unwrap(); - let (parent, _)= if let Some(meth_source) = next_lock.source.as_method() { - meth_source - } else { - internal_error!("Expected method symbol source of symbol iterated over by\ - implementations_of_symbol, symbol is {:?}", next_lock); - continue; - }; if !syms.contains(next) { - syms.insert(next); + let parent = if let Some(meth_source) = next_lock.source.as_method() { + if !meth_source.1.is_abstract() { + syms.insert(next); + } + meth_source.0 + } else { + internal_error!("Expected method symbol source of symbol iterated over by\ + implementations_of_symbol, symbol is {:?}", next_lock); + continue; + }; + for impl_ref in &next_lock.implementations { // Note: We only need to find the implementations of the variant of this method // under parent. Other variants are handled by finding multiple symbols at From e8bac52f8847c0667da4968c78d02f309422663d Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 9 Feb 2026 11:18:59 +0100 Subject: [PATCH 23/34] Use .identity() to index defined_methods consistently Signed-off-by: Jonatan Waern --- src/analysis/templating/traits.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/analysis/templating/traits.rs b/src/analysis/templating/traits.rs index 2c1c1fb0..f81149b3 100644 --- a/src/analysis/templating/traits.rs +++ b/src/analysis/templating/traits.rs @@ -439,7 +439,7 @@ impl DMLTrait { // Need one more check here, if we have multiple // non-shared decls and one or more shared decls, // we'd miss the extra non-shared ones otherwise - if defined_methods.contains_key(new_m.name.val.as_str()) { + if defined_methods.contains_key(new_m.identity()) { report.push(DMLError { span: *new_m.location(), description: From ccbd3a86b3a75de902e9850d242a1129a9f9d76c Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 9 Feb 2026 11:22:16 +0100 Subject: [PATCH 24/34] Optimize lookup_symbols slightly Signed-off-by: Jonatan Waern --- src/analysis/mod.rs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index 7459a60e..e7b99002 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -607,16 +607,10 @@ impl DeviceAnalysis { // Special handling for methods, since each method decl has its // own symbol if context_sym.kind() == DMLSymbolKind::Method { - let mut result = vec![]; - for sym in self.symbol_info.method_symbols.values().flat_map(|m|m.values()) { - let sym_lock = sym.lock().unwrap(); - if sym_lock.declarations.iter().any( - |decl|decl == context_sym.loc_span()) { - trace!("Found symbol {:?} at for {:?}", sym_lock, context_sym.loc_span()); - result.push(Arc::clone(sym)); - } - } - return result; + return self.symbol_info.method_symbols + .get(context_sym.loc_span()) + .map(|m|m.values().cloned()) + .into_iter().flatten().collect(); } self.lookup_symbols_by_contexted_symbol(context_sym, limitations) } From 08026510ef504d65714de49eb6967c4ee17ddfbc Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 9 Feb 2026 12:04:34 +0100 Subject: [PATCH 25/34] Remove redundant symbol iteration Signed-off-by: Jonatan Waern --- src/actions/semantic_lookup.rs | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/actions/semantic_lookup.rs b/src/actions/semantic_lookup.rs index 46c270f2..413d7a6f 100644 --- a/src/actions/semantic_lookup.rs +++ b/src/actions/semantic_lookup.rs @@ -198,13 +198,18 @@ fn get_symbols_of_ref<'t>(reference: &Reference, // in that case if let Some(ContextKey::Template(ref sym)) = first_context { if device.templates.templates.get(sym.name_ref()) - .and_then(|t|t.location.as_ref()) - .and_then(|loc|device.template_object_implementation_map.get(loc)) - .map_or(false, |impls|!impls.is_empty()) { + .and_then(|t|t.location.as_ref()) + .and_then(|loc|device.template_object_implementation_map.get(loc)) + .map_or(false, |impls|!impls.is_empty()) { any_template_used = true; } } - symbols.append(&mut device.symbols_of_ref(*reference.loc_span())); + let syms = device.symbols_of_ref(*reference.loc_span()); + debug!("Mapped {:?} to symbols {:?} in device analysis of {}", + reference.loc_span(), + syms.iter().map(|s|s.lock().unwrap().medium_debug_info()).collect::>(), + device.clientpath.display()); + symbols.push((*device, syms)); } if let Some(ContextKey::Template(templ)) = first_context { if !any_template_used { @@ -213,17 +218,7 @@ fn get_symbols_of_ref<'t>(reference: &Reference, ); } } - - let mut symbols: DeviceSymbols<'t> = vec![]; - for device_analysis in &analysis_info.device_analysises { - let syms = device_analysis.symbols_of_ref(*reference.loc_span()); - if !syms.is_empty() { - debug!("Mapped {:?} to symbols {:?} in device analysis of {}", reference.loc_span(), - syms.iter().map(|s|s.lock().unwrap().medium_debug_info()).collect::>(), - device_analysis.path.as_str()); - symbols.push((*device_analysis, syms)); - } - } + symbols } From b280ba5640090a40c5809ba351c78a40af7cfc20 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 9 Feb 2026 12:05:47 +0100 Subject: [PATCH 26/34] Typo fixes Signed-off-by: Jonatan Waern --- USAGE.md | 6 +++--- src/analysis/mod.rs | 2 +- src/analysis/templating/traits.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/USAGE.md b/USAGE.md index 09468e4f..ab1d23ad 100644 --- a/USAGE.md +++ b/USAGE.md @@ -15,7 +15,7 @@ missing or incomplete. ## Clarification of semantics of Symbol Lookups DML is at its core a _declarative_ language centered around a Device, and declarations are merged together according to the hierarchical -structure. Thus, the meaning of `goto-definiton`, `goto-declaration`, +structure. Thus, the meaning of `goto-definition`, `goto-declaration`, `goto-implementations`, and `goto-references` may not be similar to how they behave in other languages. Here is a clarification of what each operation means for each object @@ -33,7 +33,7 @@ See issues [#13](https://github.com/intel/dml-language-server/issues/13), [#65](https://github.com/intel/dml-language-server/issues/65). ### On Composite Objects (banks, registers, implements, etc.) -`goto-declaration` and `goto-definition` on an composite object are equivalent. +`goto-declaration` and `goto-definition` on a composite object are equivalent. They will find the locations of all object declarations that may be merged with the one at the name. @@ -82,7 +82,7 @@ instantiated. `goto-references` will go to any location where the template is referred to, including direct instantiation sites (so this is a super-set of -goto-implementations`) +`goto-implementations`) ## In-Line Linting Configuration It may be desireable to control linting on a per-file basis, rather than diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index e7b99002..f68daded 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -2420,7 +2420,7 @@ impl DeviceAnalysis { &mut errors, &mut method_structure); // This needs to be done after all symbols are created, because method - // symbol order is not correlted to the object iteration order + // symbol order is not correlated to the object iteration order bind_method_implementations(&mut symbol_info.method_symbols); status.assert_alive(); diff --git a/src/analysis/templating/traits.rs b/src/analysis/templating/traits.rs index f81149b3..ab2eaed7 100644 --- a/src/analysis/templating/traits.rs +++ b/src/analysis/templating/traits.rs @@ -198,7 +198,7 @@ impl DMLTrait { } } - // Lookup a definiton or declaration of a member in THIS trait, does not + // Lookup a definition or declaration of a member in THIS trait, does not // examine ancestors pub fn get_member<'t, T>(&'t self, member_name: &T) -> Option> From e3be04811b7b6d5fc354233fa58f2a52af480b1b Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 16 Feb 2026 12:52:57 +0100 Subject: [PATCH 27/34] Fix bug in get_bases Signed-off-by: Jonatan Waern --- src/analysis/templating/methods.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/analysis/templating/methods.rs b/src/analysis/templating/methods.rs index 8c467dd2..3e9d53fa 100644 --- a/src/analysis/templating/methods.rs +++ b/src/analysis/templating/methods.rs @@ -338,8 +338,9 @@ impl DMLMethodRef { } // Invariant: Return is non-empty pub fn get_bases(&self) -> Vec { + log::trace!("Getting bases of {:?}", self); let to_return = if let Some(default_refs) = &self.concrete_decl.default_call { - default_refs.flat_refs().into_iter().flat_map(|m|m.get_bases()).collect() + default_refs.get_bases() } else { vec![self.concrete_decl.decl.clone()] }; From b64de9e3c26bd14eec57c1ba05ff2c86f8b5c630 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 9 Feb 2026 15:51:45 +0100 Subject: [PATCH 28/34] Allow late-shared default calls Signed-off-by: Jonatan Waern --- src/analysis/templating/methods.rs | 6 +- src/analysis/templating/objects.rs | 191 ++++++++++++++++------------- 2 files changed, 106 insertions(+), 91 deletions(-) diff --git a/src/analysis/templating/methods.rs b/src/analysis/templating/methods.rs index 3e9d53fa..bf57a198 100644 --- a/src/analysis/templating/methods.rs +++ b/src/analysis/templating/methods.rs @@ -399,7 +399,7 @@ impl DeclarationSpan for DMLMethodRef { #[derive(Debug, Clone, PartialEq, Eq)] pub enum DefaultCallReference { - Abstract(MethodDecl), + Abstract(Arc), Valid(Arc), Ambiguous(Vec>), } @@ -407,7 +407,7 @@ pub enum DefaultCallReference { impl DefaultCallReference { pub fn get_bases(&self) -> Vec { match self { - DefaultCallReference::Abstract(method) => vec![method.clone()], + DefaultCallReference::Abstract(method) | DefaultCallReference::Valid(method) => method.get_bases(), DefaultCallReference::Ambiguous(methods) => methods.iter().flat_map(|m|m.get_bases()).collect(), @@ -442,7 +442,7 @@ impl DefaultCallReference { pub fn flat_refs(&self) -> Vec<&Arc> { match self { - DefaultCallReference::Abstract(_) => vec![], + DefaultCallReference::Abstract(method) | DefaultCallReference::Valid(method) => vec![method], DefaultCallReference::Ambiguous(methods) => methods.iter().collect() diff --git a/src/analysis/templating/objects.rs b/src/analysis/templating/objects.rs index d6bd4359..2a2299d5 100644 --- a/src/analysis/templating/objects.rs +++ b/src/analysis/templating/objects.rs @@ -1,6 +1,7 @@ // © 2024 Intel Corporation // SPDX-License-Identifier: Apache-2.0 and MIT use std::collections::{HashMap, HashSet}; +use std::hash::Hash; use std::iter; use std::sync::Arc; @@ -1777,80 +1778,55 @@ fn merge_composite_subobjs<'c>(parent_each_stmts: &InEachSpec, // TODO: Check collisions of 'name' parameters } -// return a tuple (default_map, order) -// where 'default_map' maps method declarations to declarations they directly -// override +// Maps a method decl to the method definitions it directly overrides, and the method declarations it directly overrides +type DeclMap<'t> = HashMap<&'t MethodDecl, HashSet<&'t MethodDecl>>; + +// return a tuple (default_map, order, declaration_map) +// where 'default_map' maps method declarations to definitions they directly override // and 'order' is a order of methods, with lowest rank being last +// and 'declaration_map' maps method declarations to the abstract method declarations they directly override fn sort_method_decls<'t>(decls: &[(&Rank, &'t MethodDecl)]) -> - (HashMap<&'t MethodDecl, HashSet<&'t MethodDecl>>, Vec<&'t MethodDecl>) { + (DeclMap<'t>, Vec<&'t MethodDecl>, DeclMap<'t>) { debug!("Sorting method decls {:?}", decls); - let mut rank_to_method: HashMap<&Rank, (Vec<&MethodDecl>, Vec<&MethodDecl>)> + let mut rank_to_method_def: HashMap<&Rank, Vec<&MethodDecl>> + = HashMap::default(); + let mut rank_to_method_decl: HashMap<&Rank, Vec<&MethodDecl>> = HashMap::default(); for (rank, decl) in decls { - let (ref mut defs, ref mut decls) = rank_to_method.entry(rank).or_default(); if decl.is_abstract() { - decls.push(*decl); + rank_to_method_decl.entry(rank).or_default().push(*decl); } else { - defs.push(*decl); + rank_to_method_def.entry(rank).or_default().push(*decl); } } - trace!("rank-to-method is: {:?}", rank_to_method); - let ranks: HashSet<&Rank> = rank_to_method.keys() - .cloned().collect(); - let mut ancestry: HashMap<&Rank, Vec<&Rank>> = ranks.iter() - .map(|r|(*r, ranks.iter().filter( + trace!("rank-to-method-def is: {:?}, rank-to-method-decl is: {:?}", rank_to_method_def, rank_to_method_decl); + + // Ancestry of defs->defs and defs->decls + #[allow(clippy::type_complexity)] + let (def_ancestry, decl_ancestry): (HashMap<&Rank, Vec<&Rank>>, + HashMap<&Rank, Vec<&Rank>>) = { + let def_ranks: HashSet<&Rank> = rank_to_method_def.keys() + .cloned().collect(); + let decl_ranks: HashSet<&Rank> = rank_to_method_decl.keys() + .cloned().collect(); + let def_ancestry = def_ranks.iter().map(|r|(*r, def_ranks.iter().filter( |r2|r.inferior.contains(&r2.id)).cloned().collect())) - .collect(); - - // Insert implicit special ancestry ordering to resolve unrelated ranks - // where a non-abstract method can override an abstract method - trace!("Initial ancestry is {:?}", ancestry); - let top_ancestry: HashSet<&Rank> = ranks.iter() - .filter(|a|!ancestry.values().any(|ra|ra.contains(*a))) - .cloned() - .collect(); - trace!("Top ancestry ranks are {:?}", top_ancestry); - let new_edges = { - let mut top_defs = vec![]; - let mut top_decls = vec![]; - for r in top_ancestry { - let (defs, decls) = rank_to_method.get(r).unwrap(); - top_defs.extend(defs.iter().map(|d|(r, *d))); - top_decls.extend(decls.iter().map(|d|(r, *d))); - } - if top_defs.len() == 1 { - let (top_rank, top_def) = &top_defs[0]; - if !top_def.is_abstract() { - top_decls.iter().filter_map(|(subrank, subdecl)| - // In-rank case is handled in method sorting - if subdecl.is_abstract() && top_rank != subrank { - Some((top_rank, subrank)) - } else { - None - }) - .map(|(from, to)|(*from, *to)) - .collect() - } else { - vec![] - } - } else { - vec![] - } + .collect(); + let decl_ancestry = def_ranks.iter().map(|r|(*r, decl_ranks.iter().filter( + |r2|r.inferior.contains(&r2.id)).cloned().collect())) + .collect(); + (def_ancestry, decl_ancestry) }; - for (from, to) in new_edges { - trace!("Inserted implicit ancestry edge from {:?} to {:?}", from, to); - ancestry.entry(from).or_default().push(to); - } - trace!("Ancestry is {:?}", ancestry); + trace!("Def ancestry is {:?}", def_ancestry); let mut minimal_ancestry: HashMap<&Rank, Vec<&Rank>> = HashMap::default(); - // Get ancestors that are not the ancestor of any ancestor - for (rank, ancestors) in &ancestry { + // For each rank, map to its first-level ancestry + for (rank, ancestors) in &def_ancestry { let mut minimal_ancestors = vec![]; for ancestor in ancestors { - if !ancestors.iter().flat_map(|a|ancestry.get(a).unwrap()) + if !ancestors.iter().flat_map(|a|def_ancestry.get(a).unwrap()) .any(|r|r == ancestor) { minimal_ancestors.push(*ancestor); } @@ -1858,35 +1834,50 @@ fn sort_method_decls<'t>(decls: &[(&Rank, &'t MethodDecl)]) -> minimal_ancestry.insert(rank, minimal_ancestors); } - // Map method declaration to those that they directly override - let mut method_map: HashMap<&'t MethodDecl, HashSet<&'t MethodDecl>> - = HashMap::default(); - for (r, (defs, decls)) in &rank_to_method { + // Map method definitions to abstract decls they override, which none of their minimal ancestors override + let mut method_map_decls: DeclMap<'t> = HashMap::default(); + for (rank, ancestors) in &decl_ancestry { + for def in rank_to_method_def.get(rank).unwrap() { + method_map_decls.entry(def).or_default(); + for ancestor in ancestors { + if !minimal_ancestry.get(rank).unwrap().iter().any( + |ma|decl_ancestry.get(ma).unwrap().iter().any(|a|a == ancestor)) { + if let Some(decls) = rank_to_method_decl.get(ancestor) { + method_map_decls.entry(def).or_default().extend(decls.iter()); + } + } + } + // Decls at the same rank are also directly overridden, by definition + if let Some(decls) = rank_to_method_decl.get(rank) { + method_map_decls.entry(def).or_default().extend(decls.iter()); + } + trace!("Method {:?} at rank {:?} directly overrides decls {:?}", + def, rank, method_map_decls.get(def).unwrap()); + } + } + + trace!("Minimal ancestry is {:?}", minimal_ancestry); + // Map method definitions to those that they directly override + let mut method_map_defs: DeclMap<'t> = HashMap::default(); + + for (r, defs) in &rank_to_method_def { for def in defs { + trace!("Handling method {:?} at rank {:?}", def, r); // Ensure the entry exists for later unwraps - method_map.entry(def).or_default(); + let mdefs = method_map_defs.entry(def).or_default(); for subrank in &minimal_ancestry[r] { - let (subdefs, subdecls) = rank_to_method.get(subrank).unwrap(); - if subdefs.is_empty() { - method_map.get_mut(def).unwrap() - .extend(subdecls.clone()); - } else { - method_map.get_mut(def).unwrap() - .extend(subdefs.clone()); - } + let subdefs = rank_to_method_def.get(subrank).unwrap(); + trace!("Set as overriding {:?}", subdefs); + mdefs.extend(subdefs.iter()); } - method_map.get_mut(def).unwrap() - .extend(decls.iter().cloned()); - } - for decl in decls { - method_map.entry(decl).or_default(); } } - trace!("Default map is {:?}", method_map); - let method_order = topsort(&method_map).unwrap_sorted(); + trace!("Default map is {:?}", method_map_defs); + trace!("Abstract override map is {:?}", method_map_decls); + let method_order = topsort(&method_map_defs).unwrap_sorted(); trace!("Method order is {:?}", method_order); - (method_map, method_order) + (method_map_defs, method_order, method_map_decls) } fn add_methods(obj: &mut DMLCompositeObject, @@ -1905,9 +1896,32 @@ fn add_methods(obj: &mut DMLCompositeObject, let all_decls: Vec<(&Rank, &MethodDecl)> = regular_decls.iter().map(|(a, b)|(a, b)).collect(); - let (default_map, method_order) = sort_method_decls(&all_decls); + let (default_map, method_order, decl_map) = sort_method_decls(&all_decls); let mut decl_to_method: HashMap> = HashMap::default(); + // Create abstract methodrefs for declarations first, order does not matter + for method in decl_map.values().flatten() { + if decl_to_method.contains_key(method) { + internal_error!("Unexpectedly an abstract method decl {:?} was generate twice", method); + continue; + } + let template_ref = decl_to_trait_map + .get(method) + .map(Arc::clone); + + let new_method = Arc::new( + DMLMethodRef { + template_ref, + concrete_decl: DMLConcreteMethod { + decl: (*method).clone(), + default_call: None, + }, + } + ); + trace!("Inserted dependent methoddecl {:?}", new_method); + decl_to_method.insert((*method).clone(), new_method); + } + trace!("Abstract decl methods are {:?}", decl_to_method); for method in method_order.iter().rev() { trace!("Handling overrides of {:?}", method); // Guaranteed by topsort @@ -1915,20 +1929,21 @@ fn add_methods(obj: &mut DMLCompositeObject, let default = match defaults.len() { 1 => { let decl = defaults.iter().next().unwrap(); - trace!("Default call decl is {:?}", decl); - trace!("And the current map is {:?}", decl_to_method); - if let Some(m) = decl_to_method.get(decl) { - Some(DefaultCallReference::Valid(Arc::clone(m))) - } else { - Some(DefaultCallReference::Abstract((*decl).clone())) - } + Some(DefaultCallReference::Valid(Arc::clone(decl_to_method.get(decl).unwrap()))) }, - 0 => None, + 0 => decl_map.get(method).and_then(|decls| { + match decls.len() { + 1 => Some(DefaultCallReference::Abstract( + Arc::clone(decls.iter().next().and_then(|d|decl_to_method.get(d)).unwrap()))), + // The case of conflicting abstract declarations is handled elsewhere + _ => None, + } + }), _ => Some(DefaultCallReference::Ambiguous( defaults.iter().map(|d| Arc::clone(decl_to_method.get(d).unwrap())).collect())), }; trace!("Default call would be {:?}", default); - for decl in defaults { + for decl in defaults.iter().chain(decl_map.get(method).unwrap_or(&HashSet::default())) { // TODO: I suspect we could improve this error message in cases // where several similar incorrect overrides are made over one // method From 966770855d07ba1afcd921ef4330dfdc2018afb6 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Wed, 18 Feb 2026 16:24:33 +0100 Subject: [PATCH 29/34] Don't panic on declaration-only methods Signed-off-by: Jonatan Waern --- src/analysis/templating/objects.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/analysis/templating/objects.rs b/src/analysis/templating/objects.rs index 2a2299d5..e4f9420d 100644 --- a/src/analysis/templating/objects.rs +++ b/src/analysis/templating/objects.rs @@ -1982,7 +1982,8 @@ fn add_methods(obj: &mut DMLCompositeObject, decl_to_method.insert((*method).clone(), new_method); } trace!("Complete decl-to-method map is: {:?}", decl_to_method); - let to_add = decl_to_method.get(method_order.first().unwrap()).unwrap(); + // If there are no definitions, skip to next method (declarations are separately added above) + let Some(to_add) = method_order.first().map(|m|decl_to_method.get(m).unwrap()) else { continue; }; debug!("Added {:?}", to_add.identity()); obj.add_shallow_component(DMLShallowObjectVariant::Method( Arc::clone(to_add))); From fc1ef16d8f3dcaf01216cc8b309f89e70a49ae54 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Fri, 20 Feb 2026 14:17:44 +0100 Subject: [PATCH 30/34] Do not warn about double-generating methods Values in the decl_map can be duplicated when two diamond inheritance occurs. So the prior assumption does not hold (and we do not need it) Signed-off-by: Jonatan Waern --- src/analysis/templating/objects.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/analysis/templating/objects.rs b/src/analysis/templating/objects.rs index e4f9420d..3728fa37 100644 --- a/src/analysis/templating/objects.rs +++ b/src/analysis/templating/objects.rs @@ -1902,7 +1902,6 @@ fn add_methods(obj: &mut DMLCompositeObject, // Create abstract methodrefs for declarations first, order does not matter for method in decl_map.values().flatten() { if decl_to_method.contains_key(method) { - internal_error!("Unexpectedly an abstract method decl {:?} was generate twice", method); continue; } let template_ref = decl_to_trait_map From 41881b4d33548300d04d1bb4071b534e0207687e Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 23 Feb 2026 09:49:08 +0100 Subject: [PATCH 31/34] Verify and preserve implementation variant on abstract methods Signed-off-by: Jonatan Waern --- src/analysis/mod.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index f68daded..63fcb34c 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -40,8 +40,7 @@ pub use crate::analysis::parsing::tree:: {ZeroRange, ZeroSpan, ZeroPosition, ZeroFilePosition}; use crate::analysis::parsing::tree::{MissingToken, MissingContent, TreeElement}; -use crate::analysis::structure::objects::{Template, Import, CompObjectKind, - ParamValue}; +use crate::analysis::structure::objects::{CompObjectKind, Import, MaybeAbstract, ParamValue, Template}; use crate::analysis::structure::statements::{ForPre, Statement, StatementKind}; use crate::analysis::structure::toplevel::{ObjectDecl, TopLevel}; use crate::analysis::structure::types::DMLType; @@ -2532,7 +2531,7 @@ fn bind_method_implementations(method_symbols: &mut HashMap Arc::clone(methref), @@ -2556,7 +2555,12 @@ fn bind_method_implementations(method_symbols: &mut HashMap Date: Mon, 23 Feb 2026 09:51:44 +0100 Subject: [PATCH 32/34] Slightly improve missing template message Signed-off-by: Jonatan Waern --- src/analysis/templating/topology.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/analysis/templating/topology.rs b/src/analysis/templating/topology.rs index 539297e0..7a1b0c75 100644 --- a/src/analysis/templating/topology.rs +++ b/src/analysis/templating/topology.rs @@ -612,7 +612,7 @@ pub fn rank_templates_aux<'t>(mut templates: HashMap<&'t str, DMLError { span: *span, description: format!( - "No template; '{}'", + "No template named '{}'", missing_template_name), related: vec![], severity: Some( From 555a7b9037ca818833d45ec7a06e2a230975ab52 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 23 Feb 2026 10:18:21 +0100 Subject: [PATCH 33/34] Remove unnecessary import Signed-off-by: Jonatan Waern --- src/analysis/templating/objects.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/analysis/templating/objects.rs b/src/analysis/templating/objects.rs index 3728fa37..bdf764fd 100644 --- a/src/analysis/templating/objects.rs +++ b/src/analysis/templating/objects.rs @@ -1,7 +1,6 @@ // © 2024 Intel Corporation // SPDX-License-Identifier: Apache-2.0 and MIT use std::collections::{HashMap, HashSet}; -use std::hash::Hash; use std::iter; use std::sync::Arc; From 095be614805927b8e80ee3587f947adf7afcf015 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Mon, 23 Feb 2026 10:20:28 +0100 Subject: [PATCH 34/34] Remove unused function Signed-off-by: Jonatan Waern --- src/analysis/mod.rs | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index 63fcb34c..eff48152 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -349,28 +349,6 @@ impl SymbolStorage { .chain(self.method_symbols.values().flat_map(|h|h.values())) .chain(self.variable_symbols.values()) } - - pub fn all_symbols_at_lock<'a>(&'a self, loc: &ZeroSpan) - -> Vec<&'a SymbolRef> { - let mut result = vec![]; - if let Some(sym) = self.template_symbols.get(loc) { - result.push(sym); - } - if let Some(param_map) = self.param_symbols.get(&(*loc, "".to_string())) { - for sym in param_map.values() { - result.push(sym); - } - } - if let Some(var_sym) = self.variable_symbols.get(loc) { - result.push(var_sym); - } - if let Some(method_map) = self.method_symbols.get(loc) { - for sym in method_map.values() { - result.push(sym); - } - } - result - } } // This maps references to the symbol they reference, made as a lock