From af954575425678ed22d4e38bac3cf0e33921fb57 Mon Sep 17 00:00:00 2001 From: Lukas Scheller Date: Sun, 29 Dec 2024 18:44:58 +0100 Subject: [PATCH] refactor --- vhdl_lang/src/ast/util.rs | 13 ++ vhdl_lang/src/lint/sensitivity_list.rs | 202 ++++++++++++------------- 2 files changed, 114 insertions(+), 101 deletions(-) diff --git a/vhdl_lang/src/ast/util.rs b/vhdl_lang/src/ast/util.rs index 3eea7790..21ff904e 100644 --- a/vhdl_lang/src/ast/util.rs +++ b/vhdl_lang/src/ast/util.rs @@ -339,6 +339,19 @@ impl Name { } } + /// Like [self.get_suffix_reference], but disregards final indexes, such as + /// `foo.bar.baz(0)` + pub fn get_suffix_reference_disregard_index(&self) -> Option { + use Name::*; + match self { + Designator(suffix) => suffix.reference.get(), + Selected(_, suffix) => suffix.item.reference.get(), + Slice(name, _) => name.item.get_suffix_reference_disregard_index(), + CallOrIndexed(coi) => coi.name.item.get_suffix_reference_disregard_index(), + _ => None, + } + } + pub fn prefix(&self) -> Option<&Designator> { match self { Self::Attribute(attr) => attr.name.item.prefix(), diff --git a/vhdl_lang/src/lint/sensitivity_list.rs b/vhdl_lang/src/lint/sensitivity_list.rs index cf01c642..e7ed086e 100644 --- a/vhdl_lang/src/lint/sensitivity_list.rs +++ b/vhdl_lang/src/lint/sensitivity_list.rs @@ -133,22 +133,23 @@ fn lint_sensitivity_list( return vec![]; } - let mut signals_in_sensitivity_list: FnvHashMap<_, _> = names + let signals_in_sensitivity_list: FnvHashMap<_, _> = names .iter() .flat_map(|name| { name.item - .get_suffix_reference() + .get_suffix_reference_disregard_index() .map(|reference| (reference, name.span)) }) .collect(); - let required_sensitivity_list = - generate_sensitivity_list(root, process, signals_in_sensitivity_list.clone(), ctx); - let mut missing_signals = Vec::new(); - for (eid, src_pos) in required_sensitivity_list { - if signals_in_sensitivity_list.remove(&eid).is_none() { - missing_signals.push((eid, src_pos)) - } - } + let mut searcher = SensitivityListChecker { + root, + sensitivity_list: signals_in_sensitivity_list.clone(), + superfluous_entities: signals_in_sensitivity_list, + found_entities: FnvHashMap::default(), + }; + let _ = process.statements.search(ctx, &mut searcher); + + let mut missing_signals = searcher.found_entities.into_iter().collect_vec(); let mut diagnostics = Vec::new(); if !missing_signals.is_empty() { missing_signals.sort_by(|(_, pos1), (_, pos2)| pos1.cmp(pos2)); @@ -160,7 +161,7 @@ fn lint_sensitivity_list( pluralize(missing_signals.len(), "The signal", "Signals"), missing_signals .iter() - .map(|(sig, _)| root.get_ent(*sig).designator.to_string()) + .map(|(sig, _)| format!("'{}'", root.get_ent(*sig).designator)) .join(", "), pluralize(missing_signals.len(), "is", "are"), ), @@ -169,13 +170,16 @@ fn lint_sensitivity_list( for (signal, pos) in missing_signals { diag.add_related( pos, - format!("{} first read here", root.get_ent(signal).describe()), + format!( + "signal '{}' first read here", + root.get_ent(signal).designator + ), ) } diagnostics.push(diag); } - for (_, pos) in signals_in_sensitivity_list { + for (_, pos) in searcher.superfluous_entities { diagnostics.push(Diagnostic::new( pos.get_pos(ctx), "Signal is never read in the process", @@ -236,41 +240,18 @@ fn pluralize<'a>(len: usize, singular: &'a str, plural: &'a str) -> &'a str { } } -/// finds all signals that are present in a sensitivity list, -/// if the process contains the `all` keyword inside the sensitivity list. -/// This is also used to compare against the actual values of the sensitivity list -/// so that superfluous / lacking signals can be detected. -/// -/// Caveats: -/// - Information from extended names is not included -/// - Information from procedure calls is not included -struct SensitivityListGenerator<'a> { +struct SensitivityListChecker<'a> { root: &'a DesignRoot, /// The sensitivity list present at the generate statement sensitivity_list: FnvHashMap, + /// Additional entities that are in the Sensitivity list, but are never read in the process. + superfluous_entities: FnvHashMap, /// A set of named entities that form the sensitivity list. /// Additionally, the token span of the first occurrence is also provided. found_entities: FnvHashMap, } -/// Generate a sensitivity list from a process statement -fn generate_sensitivity_list( - root: &DesignRoot, - process: &ProcessStatement, - sensitivity_list: FnvHashMap, - ctx: &dyn TokenAccess, -) -> FnvHashMap { - let mut searcher = SensitivityListGenerator { - root, - sensitivity_list, - found_entities: FnvHashMap::default(), - }; - let _ = process.statements.search(ctx, &mut searcher); - - searcher.found_entities -} - -impl SensitivityListGenerator<'_> { +impl SensitivityListChecker<'_> { fn analyze_expression(&mut self, expr: &Expression, span: TokenSpan, ctx: &dyn TokenAccess) { match expr { Expression::Binary(_, lhs, rhs) => { @@ -295,7 +276,7 @@ impl SensitivityListGenerator<'_> { Expression::Qualified(qual_expr) => { self.analyze_expression(&qual_expr.expr.item, qual_expr.expr.span, ctx) } - Expression::Name(name) => self.analyze_name(name, span, ctx), + Expression::Name(name) => self.analyze_name(name, span, ctx, false), Expression::New(allocator) => match &allocator.item { Allocator::Qualified(qual_expr) => { self.analyze_expression(&qual_expr.expr.item, qual_expr.expr.span, ctx) @@ -307,21 +288,27 @@ impl SensitivityListGenerator<'_> { } } - fn analyze_name(&mut self, name: &Name, pos: TokenSpan, ctx: &dyn TokenAccess) { + fn analyze_name( + &mut self, + name: &Name, + pos: TokenSpan, + ctx: &dyn TokenAccess, + remove_only: bool, + ) { use Name::*; match name { Designator(designator) => { - self.analyze_designator(designator, pos, ctx); + self.analyze_designator(designator, pos, ctx, remove_only); } - Selected(_, suffix) => { - // self.analyze_name(&prefix.item, prefix.span, ctx); - self.analyze_designator(&suffix.item, pos, ctx); + Selected(prefix, suffix) => { + self.analyze_name(&prefix.item, prefix.span, ctx, true); + self.analyze_designator(&suffix.item, pos, ctx, remove_only); } - SelectedAll(prefix) => self.analyze_name(&prefix.item, prefix.span, ctx), - Slice(prefix, _) => self.analyze_name(&prefix.item, prefix.span, ctx), + SelectedAll(prefix) => self.analyze_name(&prefix.item, prefix.span, ctx, remove_only), + Slice(prefix, _) => self.analyze_name(&prefix.item, prefix.span, ctx, remove_only), Attribute(attr) => self.analyze_attribute_name(attr, ctx), CallOrIndexed(coi) => { - self.analyze_name(&coi.name.item, coi.name.span, ctx); + self.analyze_name(&coi.name.item, coi.name.span, ctx, remove_only); for item in &coi.parameters.items { match &item.actual.item { ActualPart::Expression(expr) => { @@ -342,17 +329,29 @@ impl SensitivityListGenerator<'_> { designator: &WithRef, pos: TokenSpan, ctx: &dyn TokenAccess, + remove_only: bool, ) { if let Some(reference) = designator.reference.get() { + self.superfluous_entities.remove(&reference); let ent = self.root.get_ent(reference); - if ent.is_signal() { - self.sensitivity_list.remove(&ent.id); + if ent.is_signal() && !self.sensitivity_list.contains_key(&reference) && !remove_only { + self.found_entities + .entry(reference) + .or_insert_with(|| pos.get_pos(ctx)); } } } fn analyze_attribute_name(&mut self, attr: &AttributeName, ctx: &dyn TokenAccess) { - self.analyze_name(&attr.name.item, attr.name.span, ctx); + use AttributeDesignator::*; + match attr.attr.item { + Ident(_) | Image => {} + // These likely do not belong inside a sensitivity list. + // However, true analysis can only be performed when it is known whether the name is + // static or not. + _ => return, + } + self.analyze_name(&attr.name.item, attr.name.span, ctx, false); if let Some(expr) = &attr.expr { self.analyze_expression(&expr.item, expr.span, ctx) } @@ -411,7 +410,7 @@ impl SensitivityListGenerator<'_> { fn analyze_discrete_range(&mut self, range: &DiscreteRange, ctx: &dyn TokenAccess) { match range { DiscreteRange::Discrete(name, range) => { - self.analyze_name(&name.item, name.span, ctx); + self.analyze_name(&name.item, name.span, ctx, false); if let Some(range) = range { self.analyze_range(range, ctx) } @@ -460,7 +459,7 @@ impl SensitivityListGenerator<'_> { } } -impl Searcher for SensitivityListGenerator<'_> { +impl Searcher for SensitivityListChecker<'_> { fn search_decl(&mut self, ctx: &dyn TokenAccess, decl: FoundDeclaration<'_>) -> SearchState { use SequentialStatement::*; if let DeclarationItem::SequentialStatement(stmt) = &decl.ast { @@ -546,29 +545,16 @@ enum ProcessCategory { /// ``` /// but won't work for some more exotic, mixed processes. fn get_likely_process_category(root: &DesignRoot, process: &ProcessStatement) -> ProcessCategory { - // This might be incorrect for weird mixed processes like - // ``` - // process (clk) is - // begin - // if rising_edge(clk) then - // // ... - // end if; - // a <= b or c; - // end process; - // ``` - if process.statements.len() > 1 { - return ProcessCategory::Combinational; - }; - // using iter().all(...) guards against cases like the following: + // using iter().any(...) guards against edge cases like the following: // ``` // process(clkA, clkB) is // begin // if rising_edge(clkA) then // end if; - // if rising_edge(clkB) then - // end if; + // + // some_assignment <= xy; // end process; - let is_likely_clocked = process.statements.iter().all(|stmt| { + let is_likely_clocked = process.statements.iter().any(|stmt| { match &stmt.statement.item { SequentialStatement::If(if_stmt) => { // this is always guaranteed to be present @@ -630,23 +616,14 @@ fn is_likely_clocked(root: &DesignRoot, expression: &Expression) -> bool { mod tests { use super::*; use crate::analysis::tests::{check_no_diagnostics, LibraryBuilder}; - use crate::syntax::test::Code; + use crate::syntax::test::check_diagnostics; use std::cell::Cell; - fn get_ent(root: &DesignRoot, code: Code) -> (EntityId, SrcPos) { - ( - root.search_reference(code.source(), code.start()) - .unwrap() - .id, - code.pos(), - ) - } - #[test] fn extract_sensitivity_list_no_items() { let mut builder = LibraryBuilder::new(); - let _ = builder.code( + builder.code( "libname", " entity ent is @@ -662,11 +639,12 @@ end architecture;", let (root, diagnostics) = builder.get_analyzed_root(); check_no_diagnostics(&diagnostics); + let num_of_searches = Cell::new(0); let mut searcher = ProcessSearcher::new(|proc, ctx| { num_of_searches.set(num_of_searches.get() + 1); - let res = generate_sensitivity_list(&root, proc, ctx); - assert_eq!(res, FnvHashMap::default()); + let diag = lint_sensitivity_list(&root, ctx, proc); + assert_eq!(diag, Vec::default()); }); let _ = root.search(&mut searcher); assert_eq!(num_of_searches.get(), 1) @@ -688,15 +666,33 @@ end entity; architecture a of ent is -- all of these signals should be considered as being 'read' in the sensitivity list signal sig_b, sig_c, sig_d, sig_e, sig_f, sig_g, sig_h, sig_i, sig_j, sig_k, sig_l : bit; + -- all of these signals are fine and present in the sensitivity list + signal sig_0 : bit; + signal sig_1 : bit_vector(1 downto 0); + type t_rec is record + foo : bit; + end record; + signal sig_2 : t_rec; + signal sig_3, sig_4, sig_5, sig_6 : bit; + -- superfluous signal + signal additional : bit; signal void : bit; function func(din: bit) return bit is begin end func; begin - process is + process(sig_0, sig_1(0), sig_2, sig_3, sig_4, sig_5, sig_6, additional) is variable void_var : bit; begin + if (sig_5 = '1') then + void <= sig_6; + end if; + void <= sig_3 or sig_4; + void <= sig_3; + void <= sig_2.foo; + void <= sig_0; + void <= sig_1(0); void <= sig_a; void <= sig_b; void <= sig_b; @@ -720,25 +716,29 @@ end architecture;", check_no_diagnostics(&diagnostics); let expected_signals = [ - get_ent(&root, code.s("sig_a", 2)), - get_ent(&root, code.s("sig_b", 2)), - get_ent(&root, code.s("sig_c", 2)), - get_ent(&root, code.s("sig_d", 2)), - get_ent(&root, code.s("sig_e", 2)), - get_ent(&root, code.s("sig_f", 2)), - get_ent(&root, code.s("sig_g", 2)), - get_ent(&root, code.s("sig_h", 2)), - get_ent(&root, code.s("sig_i", 2)), - get_ent(&root, code.s("sig_j", 2)), - get_ent(&root, code.s("sig_k", 2)), - get_ent(&root, code.s("sig_l", 2)), - ]; + "sig_a", "sig_b", "sig_c", "sig_d", "sig_e", "sig_f", "sig_g", "sig_h", "sig_i", + "sig_j", "sig_k", "sig_l", + ] + .map(|value| (value, code.s(value, 2).pos())); let num_of_searches = Cell::new(0); let mut searcher = ProcessSearcher::new(|proc, ctx| { num_of_searches.set(num_of_searches.get() + 1); - let res = generate_sensitivity_list(&root, proc, ctx); - assert_eq!(res, expected_signals.iter().cloned().collect()); + let res = lint_sensitivity_list(&root, ctx, proc); + let mut expected_missing_diag = Diagnostic::new( + code.s1("process").pos(), + "Signals 'sig_a', 'sig_b', 'sig_c', 'sig_d', 'sig_e', 'sig_f', 'sig_g', 'sig_h', 'sig_i', 'sig_j', 'sig_k', 'sig_l' are not read in the sensitivity list", + ErrorCode::MissingInSensitivityList + ); + for (name, pos) in &expected_signals { + expected_missing_diag.add_related(pos, format!("signal '{name}' first read here")); + } + let expected_superfluous_diag = Diagnostic::new( + code.s("additional", 2), + "Signal is never read in the process", + ErrorCode::SuperfluousInSensitivityList, + ); + check_diagnostics(res, vec![expected_missing_diag, expected_superfluous_diag]); }); let _ = root.search(&mut searcher); assert_eq!(num_of_searches.get(), 1) @@ -845,6 +845,6 @@ end architecture;", idx.set(idx.get() + 1); }); let _ = root.search(&mut searcher); - assert_eq!(idx.get(), 8); + assert_eq!(idx.get(), 9); } }