fix(rewriter): rewrite renames as token edits instead of node replacements - #342
Open
fischeti wants to merge 2 commits into
Open
fix(rewriter): rewrite renames as token edits instead of node replacements#342fischeti wants to merge 2 commits into
fischeti wants to merge 2 commits into
Conversation
…ments 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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fischeti
force-pushed
the
pickle-token-level-rewrite
branch
from
August 4, 2026 15:43
213edb3 to
22cffe7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #338. Replaces #339 — same test, different fix.
What was wrong
The pickler only ever renames single identifier tokens, but it did so by rebuilding and replacing whole syntax nodes. In slang, replacing a node makes everything inside it invisible to the rewriter, so renames queued deeper in that subtree are silently dropped.
The workaround was
rewrite_scoped_names_inplace, a manual re-walk that has to be added by hand at every place we replace a node. It's easy to forget, and #338 is a case where it was: incommon_pkg::state_t [common_pkg::NumStates-1:0], the packed dimension sits in the part of the tree that got skipped. Two other handlers on master (virtual interface types and package imports) are missing the same re-walk and have the same latent bug.#339 adds one more manual re-walk. This PR removes the need for them.
The fix
Slang also lets you replace a single token instead of a node. Token edits don't hide anything, so they combine freely with renames elsewhere in the tree. Since every rename we do is one token, no handler needs to replace a node at all.
Each handler now edits its token and then keeps walking as usual, so nested names get renamed by the normal traversal.
rewrite_scoped_names_inplaceand everydeepCloneare gone (-107/+62 inrewriter.cpp), and the virtual-interface and package-import cases are fixed along the way.Testing
bender-slangtests pass.tests/picklegives byte-identical output to master.🤖 Generated with Claude Code