Fix Statement.renameLhs to compare identifier names#467
Fix Statement.renameLhs to compare identifier names#467tautschnig wants to merge 3 commits intomainfrom
Conversation
renameLhs was comparing lhs.name against the full Identifier 'fr' instead of 'fr.name', causing renames to never match when the metadata differed.
There was a problem hiding this comment.
Pull request overview
This PR fixes Core.Statement.renameLhs so that LHS renaming matches identifiers by their name string rather than accidentally including identifier metadata, which previously prevented renames from matching when metadata differed.
Changes:
- Updated
Statement.renameLhsto comparelhs.nameagainstfr.namein most statement constructors (init,set,call,havoc). - Updated function-declaration renaming logic in the
.funcDeclbranch (but see review comment about a remaining mismatch there).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
In Statement.renameLhs, the funcDecl branch compared decl.name (a CoreIdent) with fr.name (a String). Due to Coe String CoreIdent, fr.name was coerced to CoreIdent.unres fr.name, so the comparison failed whenever decl.name.metadata ≠ Visibility.unres. Fix by comparing decl.name.name (String) with fr.name (String), consistent with all other branches (init, set, call, havoc).
Verify that Statement.renameLhs correctly renames a funcDecl when the function name has different metadata (e.g., glob) than the fr identifier (e.g., locl), and that it correctly skips renaming when names differ.
|
I'm a bit confused about this PR. The visibility parameter in identifiers exists specifically to make sure we can know that auto-generated names are distinct from other names because they have different visibility. Because of this, visibility is fundamentally part of the name, not metadata. Can you elaborate on what problem the current approach was causing? |
Let me reconsider - it seems #289 is now also passing all tests when reverting this change. I'll see whether it comes up again when Laurel evolves in |
Pull request was closed
Description of changes:
renameLhs was comparing lhs.name against the full Identifier 'fr' instead of 'fr.name', causing renames to never match when the metadata differed.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.