diff --git a/crates/hir/src/scope.rs b/crates/hir/src/scope.rs index 0f131563..b88196ca 100644 --- a/crates/hir/src/scope.rs +++ b/crates/hir/src/scope.rs @@ -1,4 +1,5 @@ use rustc_hash::FxHashMap; +use smol_str::SmolStr; use triomphe::Arc; use utils::{ define_enum_deriving_from, @@ -15,10 +16,10 @@ use crate::{ expr::declarator::{DeclId, DeclaratorParent}, file::{config::ConfigDeclId, library::LibraryDeclId, udp::UdpDeclId}, module::{ - ModuleId, + Module, ModuleId, generate::GenerateBlockId, instantiation::InstanceId, - port::{NonAnsiPortId, Ports}, + port::{NonAnsiPortId, PortDeclId, Ports}, }, stmt::{StmtId, StmtKind}, subroutine::{SubroutineId, SubroutineLoc, SubroutinePortId}, @@ -279,6 +280,22 @@ impl ModuleScope { Arc::new(scope) } + + pub fn non_ansi_port_decl_id_by_name( + &self, + module: &Module, + name: &SmolStr, + ) -> Option { + let ModuleEntry::NonAnsiPortEntry(NonAnsiPortEntry { port_decl, .. }) = self.get(name)? + else { + return None; + }; + let decl = module.get(port_decl?); + let DeclaratorParent::PortDeclId(port_decl_id) = decl.parent else { + return None; + }; + Some(port_decl_id) + } } impl GenerateBlockScope { diff --git a/crates/ide/src/definitions.rs b/crates/ide/src/definitions.rs index 84c340ac..86ad7ac0 100644 --- a/crates/ide/src/definitions.rs +++ b/crates/ide/src/definitions.rs @@ -589,4 +589,36 @@ mod tests { assert_eq!(range.file_id.file_id(), file_id); assert_eq!(range.value, TextRange::new(TextSize::from(9), TextSize::from(10))); } + + #[test] + fn named_port_connection_name_resolves_to_target_port() { + let text = "module child(input clk); endmodule\n\ + module top; logic clk; child u(.clk(clk)); endmodule"; + let (host, file_id) = host_with_file(text); + let db = host.raw_db(); + let sema = Semantics::::new(db); + let parsed_file = sema.parse_file(file_id); + let file = parsed_file.compilation_unit().unwrap(); + let conn_name_offset = TextSize::from(text.rfind(".clk").unwrap() as u32 + 2); + let token = file + .syntax() + .token_at_offset(conn_name_offset) + .pick_bext_token(crate::goto_definition::token_precedence) + .unwrap(); + let DefinitionClass::Definition(def) = + DefinitionClass::resolve(&sema, file_id.into(), token).unwrap() + else { + panic!("expected plain definition"); + }; + + let PathResolution::AnsiPort(port) = def.0 else { + panic!("expected ANSI port resolution"); + }; + let range = DefinitionOrigin::Decl(port.into()) + .name_range(db) + .expect("ANSI port should have a name range"); + assert_eq!(range.file_id.file_id(), file_id); + assert_eq!(&text[usize::from(range.value.start())..usize::from(range.value.end())], "clk"); + assert!(range.value.start() < conn_name_offset); + } } diff --git a/crates/ide/src/inlay_hint.rs b/crates/ide/src/inlay_hint.rs index 9a252da8..1d27b8ff 100644 --- a/crates/ide/src/inlay_hint.rs +++ b/crates/ide/src/inlay_hint.rs @@ -3,20 +3,25 @@ use hir::{ db::HirDb, file::HirFileId, hir_def::{ - expr::Expr, + Ident, + expr::{ + Expr, + declarator::{DeclId, DeclaratorParent}, + }, file::FileItem, module::{ Module, ModuleId, ModuleSourceMap, ModuleSrc, - instantiation::{Instantiation, ParamAssign, PortConn}, - port::Ports, + instantiation::{Instantiation, ParamAssign, PortConn, PortConnId}, + port::{NonAnsiPortId, PortDeclId, PortDirection, Ports}, }, }, - scope::UnitEntry, - source_map::IsSrc, + scope::{AnsiPortEntry, ModuleEntry, ModuleScope, NonAnsiPortEntry, UnitEntry}, + source_map::{IsNamedSrc, IsSrc}, }; use ide_db::root_db::RootDb; use syntax::{ast, match_ast_kind}; use utils::{ + check_or_throw, get::{Get, GetRef}, text_edit::{TextEdit, TextRange, TextSize}, }; @@ -72,6 +77,7 @@ impl InlayHintCollector { &mut self, src: impl IsSrc, target_src: Option>, + position: Option, label: String, text_edit: Option, ) { @@ -80,18 +86,22 @@ impl InlayHintCollector { let kind = match_ast_kind! { src.kind(), ast::ParamAssignment => InlayKind::ParamAssign, - ast::OrderedPortConnection | ast::EmptyPortConnection => InlayKind::Port, + ast::OrderedPortConnection | ast::EmptyPortConnection | ast::NamedPortConnection => InlayKind::Port, ast::ModuleDeclaration => InlayKind::EndStructure, _ => return, }; - let position = match_ast_kind! { src.kind(), - ast::ModuleDeclaration => range.end(), - _ => range.start(), - }; + let position = position.unwrap_or_else(|| { + match_ast_kind! { src.kind(), + ast::ModuleDeclaration => range.end(), + _ => range.start(), + } + }); let (padding_left, padding_right) = match_ast_kind! { src.kind(), ast::ModuleDeclaration => (true, false), + ast::OrderedPortConnection | ast::EmptyPortConnection => (false, true), + ast::NamedPortConnection => (true, true), _ => (false, false), }; @@ -114,20 +124,6 @@ impl InlayHintCollector { }); } - fn collect_port_hint( - &mut self, - name: &str, - conn_src: impl IsSrc, - target_src: InFile, - ) { - self.collect_hint( - conn_src, - Some(target_src), - format!("{name}: "), - edits_for_conn(name, conn_src), - ); - } - fn into_hints(self) -> Vec { self.hints } @@ -192,7 +188,13 @@ fn collect_module_items( if collector.config.end_structure && let Some(name) = &module.name { - collector.collect_hint(module_src, None::>, format!(": {name}"), None); + collector.collect_hint( + module_src, + None::>, + None, + format!(": {name}"), + None, + ); } } @@ -211,36 +213,32 @@ fn process_instantiation( let target_file = target_module_id.file_id; let (target_module, target_src_map) = db.module_with_source_map(target_module_id); + let (target_module, target_src_map) = (target_module.as_ref(), target_src_map.as_ref()); + let target_scope = db.module_scope(target_module_id); + let target_scope = target_scope.as_ref(); // handle param assignments if collector.config.parameter_assignment { for (id, &assign_id) in instantiation.param_assigns.iter().enumerate() { - let ParamAssign::Ordered(assign_expr) = module.get(assign_id) else { - continue; - }; - let Some(assign_src) = src_map.get(assign_id) else { - continue; - }; - if !collector.intersect(assign_src.range()) { - break; - } - - let Some(param_id) = target_module.param_port_id_by_idx(id) else { - continue; - }; - let Some(param_name) = target_module.get(param_id).name.as_ref() else { - continue; - }; - - if should_skip(module.get(*assign_expr), param_name) { - continue; - } - - let Some(target_src) = target_src_map.get(param_id) else { - continue; + try { + let ParamAssign::Ordered(assign_expr) = module.get(assign_id) else { + continue; + }; + let assign_src = src_map.get(assign_id)?; + check_or_throw!(collector.intersect(assign_src.range())); + + let param_id = target_module.param_port_id_by_idx(id)?; + let param_name = target_module.get(param_id).name.as_ref()?; + check_or_throw!(!should_skip(module.get(*assign_expr), param_name)); + let target_src = InFile::new(target_file, target_src_map.get(param_id)?); + collector.collect_hint( + assign_src, + Some(target_src), + None, + format!("{param_name}: "), + edits_for_conn(param_name, assign_src), + ); }; - let target_src = InFile::new(target_file, target_src); - collector.collect_port_hint(param_name, assign_src, target_src); } } @@ -252,73 +250,176 @@ fn process_instantiation( continue; }; if !collector.intersect(instance_src.range()) { - break; + continue; } - for (id, &conn_id) in instance.connections.iter().enumerate() { - let conn_expr = match module.get(conn_id) { - PortConn::Empty => None, - PortConn::Ordered(expr) => Some(expr), - PortConn::Named(..) | PortConn::Wildcard => continue, + for (idx, &conn_id) in instance.connections.iter().enumerate() { + try { + let conn = module.get(conn_id); + let conn_src = src_map.get(conn_id)?; + check_or_throw!(collector.intersect(conn_src.range())); + + match &target_module.ports { + Ports::NonAnsi { .. } => { + let (port_id, name, dir) = + non_ansi_port_id_for_conn(target_module, target_scope, conn, idx)?; + let target_src = InFile::new(target_file, target_src_map.get(port_id)?); + collect_connection_hint( + module, src_map, conn_id, name, dir, target_src, collector, + ); + } + Ports::Ansi(_) => { + let (port_decl_id, decl_id) = + ansi_port_decl_id_for_conn(db, target_module_id, conn, idx)?; + let port_decl = target_module.get(port_decl_id); + let name = target_module.get(decl_id).name.as_ref()?; + let dir = port_decl.header.dir(); + let target_src = InFile::new(target_file, target_src_map.get(decl_id)?); + collect_connection_hint( + module, src_map, conn_id, name, dir, target_src, collector, + ); + } + } }; + } + } + } - let Some(conn_src) = src_map.get(conn_id) else { - continue; + Some(()) +} + +fn collect_connection_hint( + module: &Module, + src_map: &ModuleSourceMap, + conn_id: PortConnId, + name: &str, + port_dir: PortDirection, + target_src: InFile, + collector: &mut InlayHintCollector, +) -> Option<()> { + let conn = module.get(conn_id); + let conn_src = src_map.get(conn_id)?; + let arrow = match port_dir { + PortDirection::Input => "←", + PortDirection::Output => "→", + PortDirection::Inout => "↔", + PortDirection::Ref => "&", + }; + + let conn_start = conn_src.range().start(); + match conn { + PortConn::Empty => { + let label = format!("{name} {arrow}"); + let edit = edits_for_conn(name, conn_src); + collector.collect_hint(conn_src, Some(target_src), None, label, edit); + } + PortConn::Ordered(expr) => { + let label = if should_skip(module.get(*expr), name) { + arrow.to_string() + } else { + format!("{name} {arrow}") + }; + let edit = edits_for_conn(name, conn_src); + let position = src_map.get(*expr).map_or_else(|| conn_start, |src| src.range().start()); + collector.collect_hint(conn_src, Some(target_src), Some(position), label, edit); + } + PortConn::Named(port_name, expr) => { + let (label, target_src) = + if port_name.as_ref().is_none_or(|port_name| port_name != name) { + (format!("{name} {arrow}"), Some(target_src)) + } else { + (arrow.to_string(), None) }; - if !collector.intersect(conn_src.range()) { - break; - } + let position = expr + .and_then(|expr| src_map.get(expr).map(|src| src.range().start())) + .or_else(|| conn_src.name_range().map(|range| range.start())) + .unwrap_or(conn_start); + collector.collect_hint(conn_src, target_src, Some(position), label, None); + } + PortConn::Wildcard => {} + } - match &target_module.ports { - Ports::NonAnsi { .. } => { - let Some(port_id) = target_module.non_ansi_port_id_by_idx(id) else { - continue; - }; - let Some(port_name) = target_module.get(port_id).label.as_ref() else { - continue; - }; - - if conn_expr.is_some_and(|expr| should_skip(module.get(*expr), port_name)) { - continue; - } + Some(()) +} - let Some(target_src) = target_src_map.get(port_id) else { - continue; - }; - let target_src = InFile::new(target_file, target_src); - collector.collect_port_hint(port_name, conn_src, target_src); - } - Ports::Ansi(_) => { - let Some(port_decl_id) = target_module.ansi_port_decl_id_by_idx(id) else { - continue; - }; - let port_decl = target_module.get(port_decl_id); - let Some(port_name) = port_decl.name.as_ref().or_else(|| { - port_decl - .decls - .clone() - .next() - .and_then(|decl_id| target_module.get(decl_id).name.as_ref()) - }) else { - continue; - }; - - if conn_expr.is_some_and(|expr| should_skip(module.get(*expr), port_name)) { - continue; - } +fn non_ansi_port_id_for_conn<'a>( + module: &'a Module, + scope: &ModuleScope, + conn: &'a PortConn, + idx: usize, +) -> Option<(NonAnsiPortId, &'a Ident, PortDirection)> { + match conn { + PortConn::Empty | PortConn::Ordered(_) => { + let Ports::NonAnsi { ports, .. } = &module.ports else { + return None; + }; + let (port_id, port) = ports.iter().nth(idx)?; + let name = port.label.as_ref()?; + let dir = non_ansi_port_dir_by_port_id(module, scope, port_id)?; + Some((port_id, name, dir)) + } + PortConn::Named(Some(name), _) => { + let ModuleEntry::NonAnsiPortEntry(NonAnsiPortEntry { label, .. }) = scope.get(name)? + else { + return None; + }; + let port_id = label?; + let port_name = module.get(port_id).label.as_ref()?; + let dir = non_ansi_port_dir_by_port_id(module, scope, port_id)?; + Some((port_id, port_name, dir)) + } + PortConn::Named(None, _) | PortConn::Wildcard => None, + } +} - let Some(target_src) = target_src_map.get(port_decl_id) else { - continue; - }; - let target_src = InFile::new(target_file, target_src); - collector.collect_port_hint(port_name, conn_src, target_src); - } - } +fn non_ansi_port_dir_by_port_id( + module: &Module, + scope: &ModuleScope, + port_id: NonAnsiPortId, +) -> Option { + let port = module.get(port_id); + + if let Some(refs) = port.refs.clone() { + for ref_id in refs { + let Some(name) = module.get(ref_id).ident.as_ref() else { + continue; + }; + if let Some(port_decl_id) = scope.non_ansi_port_decl_id_by_name(module, name) { + return Some(module.get(port_decl_id).header.dir()); } } } - Some(()) + let name = port.label.as_ref()?; + let port_decl_id = scope.non_ansi_port_decl_id_by_name(module, name)?; + Some(module.get(port_decl_id).header.dir()) +} + +fn ansi_port_decl_id_for_conn( + db: &RootDb, + module_id: ModuleId, + conn: &PortConn, + idx: usize, +) -> Option<(PortDeclId, DeclId)> { + let module = db.module(module_id); + match conn { + PortConn::Empty | PortConn::Ordered(_) => { + let port_decl_id = module.ansi_port_decl_id_by_idx(idx)?; + let decl_id = module.get(port_decl_id).decls.clone().next()?; + Some((port_decl_id, decl_id)) + } + PortConn::Named(Some(name), _) => { + let module_scope = db.module_scope(module_id); + let ModuleEntry::AnsiPortEntry(AnsiPortEntry(decl_id)) = module_scope.get(name)? else { + return None; + }; + let DeclaratorParent::PortDeclId(port_decl_id) = module.get(decl_id).parent else { + return None; + }; + Some((port_decl_id, decl_id)) + } + PortConn::Named(None, _) | PortConn::Wildcard => None, + } } fn edits_for_conn(param: &str, conn_src: impl IsSrc) -> Option { @@ -388,6 +489,24 @@ mod tests { .collect() } + fn port_hint_labels_in_range(text: &str, range: TextRange) -> Vec { + let (db, file_id) = db_with_file(text); + inlay_hint(&db, file_id, range, port_config()) + .into_iter() + .filter(|hint| matches!(hint.kind, InlayKind::Port)) + .map(|hint| hint.label) + .collect() + } + + fn port_hints(text: &str) -> Vec { + let (db, file_id) = db_with_file(text); + let range = TextRange::new(TextSize::from(0), TextSize::of(text)); + inlay_hint(&db, file_id, range, port_config()) + .into_iter() + .filter(|hint| matches!(hint.kind, InlayKind::Port)) + .collect() + } + fn param_hint_labels(text: &str) -> Vec { let (db, file_id) = db_with_file(text); let range = TextRange::new(TextSize::from(0), TextSize::of(text)); @@ -402,7 +521,7 @@ mod tests { fn extra_ordered_connections_do_not_invent_ansi_port_hints() { let text = "module child(input a); endmodule\nmodule top; child u(1'b0, 1'b1); endmodule\n"; - assert_eq!(port_hint_labels(text), vec!["a: "]); + assert_eq!(port_hint_labels(text), vec!["a ← "]); } #[test] @@ -410,7 +529,129 @@ mod tests { let text = "module child(a); input a; endmodule\nmodule top; child u(1'b0, 1'b1); endmodule\n"; - assert_eq!(port_hint_labels(text), vec!["a: "]); + assert_eq!(port_hint_labels(text), vec!["a ← "]); + } + + #[test] + fn port_hints_show_direction_arrows() { + let text = "module child(input i, output o, inout io, ref r); endmodule\n\ + module top; logic a, b, c, d; child u(a, b, c, d); endmodule\n"; + + assert_eq!(port_hint_labels(text), vec!["i ← ", "o → ", "io ↔ ", "r & "]); + } + + #[test] + fn ordered_port_hint_omits_same_name_but_keeps_direction() { + let text = "module child(output instr_addr_o); endmodule\n\ + module top; logic instr_addr_o; child u(instr_addr_o); endmodule\n"; + + assert_eq!(port_hint_labels(text), vec![" → "]); + } + + #[test] + fn ordered_same_name_arrow_hint_is_not_clickable() { + let text = "module child(output instr_addr_o); endmodule\n\ + module top; logic instr_addr_o; child u(instr_addr_o); endmodule\n"; + + let hints = port_hints(text); + assert_eq!(hints.iter().map(|hint| hint.label.as_str()).collect::>(), vec![" → "]); + assert!(hints[0].target_location.is_none()); + assert!(hints[0].text_edit.is_none()); + } + + #[test] + fn named_same_name_arrow_hint_is_not_clickable() { + let text = "module child(output clk); endmodule\n\ + module top; logic clk; child u(.clk(clk)); endmodule\n"; + + let hints = port_hints(text); + assert_eq!(hints.iter().map(|hint| hint.label.as_str()).collect::>(), vec![" → "]); + assert!(hints[0].target_location.is_none()); + } + + #[test] + fn named_port_hint_is_clickable() { + let text = "module child(output out); endmodule\n\ + module top; logic instr_addr_o; child u(instr_addr_o); endmodule\n"; + + let hints = port_hints(text); + assert_eq!( + hints.iter().map(|hint| hint.label.as_str()).collect::>(), + vec!["out → "] + ); + assert!(hints[0].target_location.is_some()); + } + + #[test] + fn ordered_port_hint_keeps_different_name_with_direction() { + let text = "module child(output out); endmodule\n\ + module top; logic instr_addr_o; child u(instr_addr_o); endmodule\n"; + + assert_eq!(port_hint_labels(text), vec!["out → "]); + } + + #[test] + fn named_port_hint_omits_visible_name_but_keeps_direction() { + let text = "module child(output instr_addr_o); endmodule\n\ + module top; logic instr_addr_o; child u(.instr_addr_o(instr_addr_o)); endmodule\n"; + + assert_eq!(port_hint_labels(text), vec![" → "]); + } + + #[test] + fn named_port_hints_resolve_ports_by_name_not_position() { + let text = "module child(input a, output b, input c); endmodule\n\ + module top; logic local_b, local_c; child u(.b(local_b), .c(local_c)); endmodule\n"; + + assert_eq!(port_hint_labels(text), vec![" → ", " ← "]); + } + + #[test] + fn port_hints_in_later_viewport_skip_previous_connections() { + let text = "module child(input a, output b); endmodule\n\ + module top; logic local_a, local_b; child u(.a(local_a), .b(local_b)); endmodule\n"; + let start = TextSize::from(text.find(".b(local_b)").expect("second connection") as u32); + let end = start + TextSize::of(".b(local_b)"); + + assert_eq!(port_hint_labels_in_range(text, TextRange::new(start, end)), vec![" → "]); + } + + #[test] + fn unknown_named_port_does_not_fall_back_to_position() { + let text = "module child(input a); endmodule\n\ + module top; logic sig; child u(.bogus(sig)); endmodule\n"; + + assert_eq!(port_hint_labels(text), Vec::::new()); + } + + #[test] + fn unknown_named_non_ansi_port_does_not_fall_back_to_position() { + let text = "module child(a); input a; endmodule\n\ + module top; logic sig; child u(.bogus(sig)); endmodule\n"; + + assert_eq!(port_hint_labels(text), Vec::::new()); + } + + #[test] + fn explicit_non_ansi_port_label_uses_internal_ref_for_direction() { + let text = "module child(.out(foo)); output foo; endmodule\n\ + module top; logic sig; child u(.out(sig)); endmodule\n"; + + assert_eq!(port_hint_labels(text), vec![" → "]); + } + + #[test] + fn implicit_named_port_hint_appears_before_local_name() { + let text = "module child(input clk_i); endmodule\n\ + module top; logic clk_i; child u(.clk_i,); endmodule\n"; + + let hints = port_hints(text); + assert_eq!(hints.iter().map(|hint| hint.label.as_str()).collect::>(), vec![" ← "]); + assert_eq!( + hints[0].position, + TextSize::from(text.rfind("clk_i,").expect("connection name") as u32) + ); + assert!(hints[0].target_location.is_none()); } #[test] diff --git a/crates/ide/src/semantic_tokens.rs b/crates/ide/src/semantic_tokens.rs index 2903a00c..a44b1850 100644 --- a/crates/ide/src/semantic_tokens.rs +++ b/crates/ide/src/semantic_tokens.rs @@ -259,7 +259,8 @@ fn collect_module( for (conn_id, conn) in module.inst_port_conns.iter() { match conn { PortConn::Named(Some(name), _) => { - let Some(range) = module_src_map.get(conn_id).map(|src| src.range()) else { + let Some(range) = module_src_map.get(conn_id).and_then(|src| src.name_range()) + else { continue; }; check_range!(collector, range); @@ -499,4 +500,48 @@ mod tests { assert_debug_snapshot!(name, tokens); } } + + #[test] + fn named_port_connection_token_uses_name_range() { + let text = "\ +module child(output logic instr_req_o); +endmodule + +module top(output logic instr_req_o); +child u_child ( + .instr_req_o (instr_req_o), +); +endmodule +"; + let (host, file_id) = setup(text); + let tokens = host + .make_analysis() + .semantic_tokens( + file_id, + SemaTokenConfig { port: SemaTokenPortConfig { clk_rst: false, io: true } }, + Some(TextRange::up_to(utils::text_edit::TextSize::of(text))), + ) + .unwrap(); + + let named_port_start = text.find(".instr_req_o").unwrap() + 1; + let named_port_range = TextRange::new( + (named_port_start as u32).into(), + ((named_port_start + "instr_req_o".len()) as u32).into(), + ); + let expr_start = text.find("(instr_req_o)").unwrap() + 1; + let expr_range = TextRange::new( + (expr_start as u32).into(), + ((expr_start + "instr_req_o".len()) as u32).into(), + ); + + assert!(tokens.iter().any(|token| token.range == named_port_range)); + assert!(tokens.iter().any(|token| token.range == expr_range)); + assert!( + tokens + .iter() + .all(|token| token.range + != TextRange::new(named_port_range.start(), expr_range.end())), + "named port connection must not produce a token spanning the whole connection" + ); + } } diff --git a/crates/utils/src/utry.rs b/crates/utils/src/utry.rs index 694a87b7..fa332728 100644 --- a/crates/utils/src/utry.rs +++ b/crates/utils/src/utry.rs @@ -20,3 +20,12 @@ macro_rules! try_or_default { try_or_default!{{$($tt)*}} }; } + +#[macro_export] +macro_rules! check_or_throw { + ($expr:expr) => { + if !($expr) { + None? + } + }; +}