feat(hir): add package and subroutine containers#4
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive support for SystemVerilog packages, including package declarations, imports/exports, and various member types (typedefs, structs, classes, subroutines). It extends the HIR (High-level Intermediate Representation) to handle packages alongside existing constructs like modules and blocks.
Key changes:
- Introduces package declaration support with wrapper types (
PackageDeclaration,PackageHeader) - Extends HIR to support packages, typedefs, structs, classes, and subroutines across all container types
- Adds comprehensive handling of package imports/exports and member types
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/syntax/src/slang_ext/package.rs | New wrapper types for package declarations and headers with AST node trait implementations |
| crates/hir/src/hir_def/package.rs | Complete package HIR definition including lowering logic for package members and exports |
| crates/hir/src/hir_def/typedef.rs | New typedef HIR representation with source mapping and lowering utilities |
| crates/hir/src/hir_def/subroutine.rs | Extensive subroutine support with port definitions, body lowering, and container integration |
| crates/hir/src/hir_def/aggregate.rs | New aggregate type support (structs, unions, classes) with member definitions |
| crates/hir/src/hir_def/module.rs | Extended module lowering to handle new member types and comprehensive pattern matching |
| crates/hir/src/hir_def/file.rs | File-level package registration and support for file-scoped subroutines and other constructs |
| crates/hir/src/hir_def/block.rs | Block-level typedef and struct support with proper lowering |
| crates/hir/src/container.rs | Container system extended to include packages and subroutines as first-class containers |
| crates/hir/src/db.rs | Database queries for package and subroutine retrieval |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn to_node(&self, tree: &'a syntax::SyntaxTree) -> Option<ast::TypedefDeclaration<'a>> { | ||
| let mut node = self.node.to_node(tree)?; | ||
| while !ast::TypedefDeclaration::can_cast(node.kind()) { | ||
| node = node.children().find_map(|elem| elem.as_node()).unwrap(); |
There was a problem hiding this comment.
Potential panic if no node is found. The .unwrap() call will panic if find_map returns None. Consider propagating the error by using ? instead: node = node.children().find_map(|elem| elem.as_node())?;
| node = node.children().find_map(|elem| elem.as_node()).unwrap(); | |
| node = node.children().find_map(|elem| elem.as_node())?; |
| fn to_node(&self, tree: &'a syntax::SyntaxTree) -> Option<ast::PackageImportDeclaration<'a>> { | ||
| let mut node = self.node.to_node(tree)?; | ||
| while !ast::PackageImportDeclaration::can_cast(node.kind()) { | ||
| node = node.children().find_map(|elem| elem.as_node()).unwrap(); |
There was a problem hiding this comment.
Potential panic if no node is found. The .unwrap() call will panic if find_map returns None. Consider propagating the error by using ? instead: node = node.children().find_map(|elem| elem.as_node())?;
| node = node.children().find_map(|elem| elem.as_node()).unwrap(); | |
| node = node.children().find_map(|elem| elem.as_node())?; |
| fn to_node(&self, tree: &'a syntax::SyntaxTree) -> Option<ast::StructUnionType<'a>> { | ||
| let mut node = self.node.to_node(tree)?; | ||
| while !ast::StructUnionType::can_cast(node.kind()) { | ||
| node = node.children().find_map(|elem| elem.as_node()).unwrap(); |
There was a problem hiding this comment.
Potential panic if no node is found. The .unwrap() call will panic if find_map returns None. Consider propagating the error by using ? instead: node = node.children().find_map(|elem| elem.as_node())?;
| fn to_node(&self, tree: &'a syntax::SyntaxTree) -> Option<ast::ClassDeclaration<'a>> { | ||
| let mut node = self.node.to_node(tree)?; | ||
| while !ast::ClassDeclaration::can_cast(node.kind()) { | ||
| node = node.children().find_map(|elem| elem.as_node()).unwrap(); |
There was a problem hiding this comment.
Potential panic if no node is found. The .unwrap() call will panic if find_map returns None. Consider propagating the error by using ? instead: node = node.children().find_map(|elem| elem.as_node())?;
| node = node.children().find_map(|elem| elem.as_node()).unwrap(); | |
| node = node.children().find_map(|elem| elem.as_node())?; |
| fn to_node(&self, tree: &'a syntax::SyntaxTree) -> Option<PackageDeclaration<'a>> { | ||
| let mut node = self.node.to_node(tree)?; | ||
| while !PackageDeclaration::can_cast(node.kind()) { | ||
| node = node.children().find_map(|elem| elem.as_node())?; |
There was a problem hiding this comment.
Inconsistent error handling pattern. Lines 106, and similar patterns in other files use .unwrap(), while this line uses ?. For consistency and better error handling, all these patterns should use ? to propagate the error rather than panicking.
| #[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)] | ||
| pub struct LocalBlockId(pub StmtId); |
There was a problem hiding this comment.
The field of LocalBlockId is now public, changing it from a newtype pattern with encapsulation to an exposed implementation detail. If this is intentional, consider whether this breaks encapsulation or if accessor methods would be more appropriate.
| impl From<InstantiationSrc> for syntax::ptr::SyntaxNodePtr { | ||
| fn from(src: InstantiationSrc) -> Self { | ||
| match src { | ||
| InstantiationSrc::HierarchyInstantiation(ptr) => ptr, | ||
| InstantiationSrc::PrimitiveInstantiation(ptr) => ptr, |
There was a problem hiding this comment.
[nitpick] The From implementation for InstantiationSrc could be better served by implementing a ptr() method similar to other source types (see TypedefSrc::ptr() on line 75-78 of typedef.rs). This would be more consistent with the codebase pattern and provide better encapsulation.
| impl From<InstantiationSrc> for syntax::ptr::SyntaxNodePtr { | |
| fn from(src: InstantiationSrc) -> Self { | |
| match src { | |
| InstantiationSrc::HierarchyInstantiation(ptr) => ptr, | |
| InstantiationSrc::PrimitiveInstantiation(ptr) => ptr, | |
| impl InstantiationSrc { | |
| pub fn ptr(&self) -> syntax::ptr::SyntaxNodePtr { | |
| match self { | |
| InstantiationSrc::HierarchyInstantiation(ptr) => *ptr, | |
| InstantiationSrc::PrimitiveInstantiation(ptr) => *ptr, |
| hashbrown = { version = "0.12.3", features = [ | ||
| "inline-more", | ||
| ], default-features = false } | ||
| hashbrown = { version = "0.12.3", features = ["inline-more"], default-features = false } |
There was a problem hiding this comment.
[nitpick] Formatting-only change that condenses multi-line dependency into single line. While this doesn't affect functionality, consider whether this aligns with project formatting standards for readability of complex dependency specifications.
| hashbrown = { version = "0.12.3", features = ["inline-more"], default-features = false } | |
| hashbrown = { | |
| version = "0.12.3", | |
| features = ["inline-more"], | |
| default-features = false | |
| } |
feat(vscode): add TextMate grammars for verilog/systemverilog
* chore: add .clice/ to .gitignore * chore(vscode): add packaging script and ignore artifacts * fix(ci): package vscode extension non-interactively
|
Why was this PR closed, I made some modifications to it, planning to push it up |
|
I thought we weren’t going to support SV features, so I reimplemented the completion features without modifying the hir crates in #6. I should have confirmed this with you beforehand and sorry about that. Let me reopen this PR. |
1a75fc2 to
e48823e
Compare
This PR adds support for packages and subroutines as new container types in the hir crate. It updates the container system and database queries so that packages and subroutines can be resolved and displayed like other containers.