From 8b696d65d36dac578d8d020f3e7bab8880ccebde Mon Sep 17 00:00:00 2001 From: Tim Fischer Date: Mon, 3 Aug 2026 18:45:21 +0200 Subject: [PATCH 1/2] fix(rewriter): rewrite renames as token edits instead of node replacements Every rename the pickler performs is a single identifier token, but the rewriter was replacing whole syntax nodes via deepClone + replace(). In slang, replace() records a change keyed by the old node pointer and the tree cloner stops descending as soon as it hits one, so any change queued on a descendant is silently dropped. Nested scoped names therefore had to be re-walked by hand (rewrite_scoped_names_inplace) at every replacement site, which is easy to forget: the packed-dimension case in #338 was one such missed site, and VirtualInterfaceTypeSyntax and PackageImportItemSyntax were two more latent ones. Token changes are keyed by (owning node, child index) and are applied during the clone walk, so they compose with edits at any other depth. Switching every handler to replaceToken() plus an unconditional visitDefault() removes the whole category of bug rather than patching one more site, and lets rewrite_scoped_names_inplace and every deepClone go. Renaming a scoped name also no longer skips visitDefault, so the right subtree of a renamed scoped name is now visited. The regression test and fixtures are taken unchanged from #339, which this supersedes. Pickled output is byte-identical to before for the existing test project. Co-Authored-By: Claude Opus 5 --- crates/bender-slang/cpp/rewriter.cpp | 163 +++++++++------------------ tests/pickle.rs | 10 ++ tests/pickle/src/common_pkg.sv | 2 + tests/pickle/src/core.sv | 3 + 4 files changed, 71 insertions(+), 107 deletions(-) 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 From 22cffe756270498ae3c7eb2c8170a086a9cce0f7 Mon Sep 17 00:00:00 2001 From: Tim Fischer Date: Mon, 3 Aug 2026 18:46:33 +0200 Subject: [PATCH 2/2] changelog: Add entry for scoped nested rename fix Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88b2562c..61bafade 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) a ### Changed - `bender clone`: primary flag for the checkout directory is now `--working-dir`, matching `bender snapshot`'s flag for the same concept; `-p`/`--path` are kept as hidden aliases for backwards compatibility. +### Fixed +- 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 - Add `git_submodules` config field and `--git-submodules ` flag (env `BENDER_GIT_SUBMODULES`) to control cloning of dependency submodules; defaults to `true`, the flag overrides the configured value in either direction (https://github.com/pulp-platform/bender/pull/314).