diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c48b42e..1dd03001 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) a ### Fixed - script: apply `override_files` before validation and the slang pass, so overriding files replace their targets in file-existence checks and in `--top`/`--trim-incdirs` reduction (previously slang saw both the original and the override as duplicate modules); the overridden-file annotation is preserved. +- pickle: Rename scoped names nested inside a renamed scoped name, such as a packed dimension on a scoped type (`common_pkg::state_t [common_pkg::NumStates-1:0]`). The rewriter now applies renames as token edits rather than replacing whole syntax nodes, which also fixes the same class of missed rename in virtual interface types and package imports (https://github.com/pulp-platform/bender/pull/342). ## 0.32.1 - 2026-07-07 ### Added diff --git a/crates/bender-slang/cpp/rewriter.cpp b/crates/bender-slang/cpp/rewriter.cpp index e2aaf717..4352176d 100644 --- a/crates/bender-slang/cpp/rewriter.cpp +++ b/crates/bender-slang/cpp/rewriter.cpp @@ -20,10 +20,36 @@ bool is_reserved_scope_root(string_view name) { } } // namespace +// Base for our rewriters. Every rename we perform is a single identifier token. +template class TokenRewriter : public SyntaxRewriter { + protected: + using SyntaxRewriter::alloc; + using SyntaxRewriter::replaceToken; + + // Queues a rename of `tok`, which must be a direct token child of `owner`. + // Trivia and source location are carried over from the original token. + // Returns false if the token isn't a child of `owner`. + bool rename_token(const SyntaxNode& owner, const Token& tok, string_view newName) { + for (size_t i = 0, n = owner.getChildCount(); i < n; i++) { + if (owner.childNode(i)) { + continue; + } + // Non-missing tokens within one node have distinct locations, so + // this identifies the child slot holding `tok`. + auto child = owner.childToken(i); + if (child && child.kind == tok.kind && child.location() == tok.location()) { + replaceToken(owner, i, tok.withRawText(alloc, newName)); + return true; + } + } + return false; + } +}; + std::unique_ptr new_syntax_tree_rewriter() { return std::make_unique(); } // Pass 1: collects declarations and renames declaration sites. -class DeclarationRewriter : public SyntaxRewriter { +class DeclarationRewriter : public TokenRewriter { public: DeclarationRewriter(std::unordered_map& renameMap, const std::string& prefix, const std::string& suffix, const std::unordered_set& excludes, @@ -56,20 +82,12 @@ class DeclarationRewriter : public SyntaxRewriter { return; } - auto newNameToken = node.header->name.withRawText(alloc, newName); - - ModuleHeaderSyntax* newHeader = deepClone(*node.header, alloc); - newHeader->name = newNameToken; - - replace(*node.header, *newHeader); + rename_token(*node.header, node.header->name, newName); declRenamed++; // Also rename the end label if present (e.g., `endmodule : module_name`). if (node.blockName && !node.blockName->name.isMissing()) { - auto newBlockNameToken = node.blockName->name.withRawText(alloc, newName); - NamedBlockClauseSyntax* newBlockName = deepClone(*node.blockName, alloc); - newBlockName->name = newBlockNameToken; - replace(*node.blockName, *newBlockName); + rename_token(*node.blockName, node.blockName->name, newName); } visitDefault(node); @@ -84,10 +102,7 @@ class DeclarationRewriter : public SyntaxRewriter { }; // Pass 2: rewrites references based on the map built in pass 1. -// Internally this is split into: -// - 2a structural references (instantiations / imports / virtual interfaces) -// - 2b scoped-name references -class ReferenceRewriter : public SyntaxRewriter { +class ReferenceRewriter : public TokenRewriter { public: ReferenceRewriter(const std::unordered_map& renameMap, std::uint64_t& refRenamed) : renameMap(renameMap), refRenamed(refRenamed) {} @@ -116,119 +131,53 @@ class ReferenceRewriter : public SyntaxRewriter { } // e.g.: "core u_core();" -> "p_core_s u_core();". + // visitDefault still descends into the parameter overrides and instance + // bodies, so scoped names nested in them are rewritten as usual. void handle(const HierarchyInstantiationSyntax& node) { - if (node.type.kind != TokenKind::Identifier) { - visitDefault(node); - return; - } - - auto newName = mapped_name(node.type.valueText()); - if (newName.empty()) { - visitDefault(node); - return; + if (node.type.kind == TokenKind::Identifier) { + auto newName = mapped_name(node.type.valueText()); + if (!newName.empty() && rename_token(node, node.type, newName)) { + refRenamed++; + } } - - auto newNameToken = node.type.withRawText(alloc, newName); - HierarchyInstantiationSyntax* newNode = deepClone(node, alloc); - newNode->type = newNameToken; - - // Preserve scoped renames in overridden parameters of this - // instantiation, which would otherwise be shadowed by replacing - // the whole instantiation node. - rewrite_scoped_names_inplace(*newNode); - - replace(node, *newNode); - refRenamed++; + visitDefault(node); } // e.g.: "import common_pkg::*;" -> "import p_common_pkg_s::*;". void handle(const PackageImportItemSyntax& node) { - if (node.package.isMissing()) { - return; - } - - auto newName = mapped_name(node.package.valueText()); - if (newName.empty()) { - visitDefault(node); - return; + if (!node.package.isMissing()) { + auto newName = mapped_name(node.package.valueText()); + if (!newName.empty() && rename_token(node, node.package, newName)) { + refRenamed++; + } } - auto newNameToken = node.package.withRawText(alloc, newName); - - PackageImportItemSyntax* newNode = deepClone(node, alloc); - newNode->package = newNameToken; - - replace(node, *newNode); - refRenamed++; + visitDefault(node); } // e.g.: "virtual bus_intf v_if;" -> "virtual p_bus_intf_s v_if;". void handle(const VirtualInterfaceTypeSyntax& node) { - if (node.name.isMissing()) { - return; - } - - auto newName = mapped_name(node.name.valueText()); - if (newName.empty()) { - visitDefault(node); - return; + if (!node.name.isMissing()) { + auto newName = mapped_name(node.name.valueText()); + if (!newName.empty() && rename_token(node, node.name, newName)) { + refRenamed++; + } } - auto newNameToken = node.name.withRawText(alloc, newName); - - VirtualInterfaceTypeSyntax* newNode = deepClone(node, alloc); - newNode->name = newNameToken; - - replace(node, *newNode); - refRenamed++; + visitDefault(node); } // e.g.: "common_pkg::state_t" -> "p_common_pkg_s::state_t". void handle(const ScopedNameSyntax& node) { auto newName = mapped_scoped_left_name(node); - if (newName.empty()) { - visitDefault(node); - return; - } - - auto& leftNode = node.left->as(); - auto newNameToken = leftNode.identifier.withRawText(alloc, newName); - - IdentifierNameSyntax* newLeft = deepClone(leftNode, alloc); - newLeft->identifier = newNameToken; - - ScopedNameSyntax* newNode = deepClone(node, alloc); - newNode->left = newLeft; - - replace(node, *newNode); - refRenamed++; - } - - private: - // Rewrites only the left identifier of a scoped name in-place if mapped. - void rewrite_scoped_name_left(ScopedNameSyntax& node) { - auto newName = mapped_scoped_left_name(node); - if (newName.empty()) { - return; - } - - auto& leftNode = node.left->as(); - leftNode.identifier = leftNode.identifier.withRawText(alloc, newName); - refRenamed++; - } - - // Walks a subtree and rewrites all scoped-name left identifiers in-place. - // Used on cloned instantiation subtrees before replacing the parent node. - void rewrite_scoped_names_inplace(SyntaxNode& root) { - if (auto* scoped = root.as_if()) { - rewrite_scoped_name_left(*scoped); - } - - for (size_t i = 0; i < root.getChildCount(); i++) { - if (auto* child = root.childNode(i)) { - rewrite_scoped_names_inplace(*child); + if (!newName.empty()) { + auto& leftNode = node.left->as(); + if (rename_token(leftNode, leftNode.identifier, newName)) { + refRenamed++; } } + visitDefault(node); } + private: const std::unordered_map& renameMap; std::uint64_t& refRenamed; }; diff --git a/tests/pickle.rs b/tests/pickle.rs index 912e2cdd..3d5a0243 100644 --- a/tests/pickle.rs +++ b/tests/pickle.rs @@ -146,6 +146,16 @@ mod tests { assert!(!renamed.contains("common_pkg::Idle")); } + #[test] + fn pickle_rename_renames_scoped_packed_dimensions() { + let renamed = run_pickle(&["--prefix", "p_", "--suffix", "_s", "--expand-macros"]); + + // A packed dimension is parsed as part of the scoped type name it follows, + // so a scoped name inside it must be renamed along with the type itself. + assert!(renamed.contains("p_common_pkg_s::state_t [p_common_pkg_s::NumStates-1:0]")); + assert!(!renamed.contains("common_pkg::NumStates-1:0")); + } + #[test] fn pickle_rename_renames_scoped_instantiation_params() { let renamed = run_pickle(&[ diff --git a/tests/pickle/src/common_pkg.sv b/tests/pickle/src/common_pkg.sv index 7a2d02d5..6f35f717 100644 --- a/tests/pickle/src/common_pkg.sv +++ b/tests/pickle/src/common_pkg.sv @@ -1,5 +1,7 @@ package common_pkg; + parameter int unsigned NumStates = 3; + typedef enum logic [1:0] { Idle = 2'b00, Busy = 2'b01, diff --git a/tests/pickle/src/core.sv b/tests/pickle/src/core.sv index 30c0baa4..87de7e77 100644 --- a/tests/pickle/src/core.sv +++ b/tests/pickle/src/core.sv @@ -1,5 +1,8 @@ module core #( parameter common_pkg::state_t DefaultState = common_pkg::Idle ) (); + // Scoped type name carrying a packed dimension that is itself a scoped name. + common_pkg::state_t [common_pkg::NumStates-1:0] state_history; + leaf u_leaf(); endmodule