Rename refactoring follow-ups (#328) + Add Class Variable / Add Accessors - #392
Open
ericwinger wants to merge 14 commits into
Open
Rename refactoring follow-ups (#328) + Add Class Variable / Add Accessors#392ericwinger wants to merge 14 commits into
ericwinger wants to merge 14 commits into
Conversation
…328 item 9) Dictionary-scope membership matched a class by NAME, so a different class of the same name shadowed in another dictionary was pulled into a single-dictionary scope and staged as a duplicate change row (the duplicate the preview showed). Match by class identity instead, via a shared GsRefactoringEnvironment>>class:isDefinedInDictionaryNamed: used by the #dictionary branch of the rename-method, change-signature, and rename-class refactorings. Adds engine SUnit coverage: an environment-level identity test and a rename-method regression that reproduces the shadow and asserts exactly one implementor is staged (the sibling is counted out of scope). Full engine suite green on 3.6.2 (14 suites / 315 tests, 0 fail / 0 err); a negative control confirms both new tests fail against the pre-fix engine. Regenerated the resources/refactoring payloads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The class-rename preview banner now states whether the rename will commit: "Migrate all instances" and "Remove old versions from class history" each force a commit, and with neither chosen the rename applies without committing. Threads removeOldFromHistory through to the preview (it previously only knew migrateInstances) so the note reflects the actual selection, and drops the inline "(this commits the rename)" aside in favour of one explicit line. Corrects the imprecise "never commits" description for a class rename. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # resources/refactoring/manifest.gs
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # resources/refactoring/manifest.gs
# Conflicts: # resources/refactoring/manifest.gs
…defining class (#328 item 3) Invoked on an ivar inherited by a subclass method, the editor's Rename Instance Variable action no longer dead-ends. It resolves the defining class (walking up the superclass chain) and, after a one-line confirm, runs the existing rename there across that class's whole hierarchy — so renaming an instance variable is always reachable from a method. New getDefiningClassOfInstVar query returns the declaring class plus the SymbolList index that binds it, so the retarget resolves even when the superclass lives in a different dictionary than the subclass. instVarNames answers Symbols, so it matches by interning and comparing by identity — a String compare raises error 2718 (Unicode disallowed) on 3.6.x. Tests: command retarget/confirm/cancel/fallback/unresolved (unit); the new query (unit); and a GCI integration case that resolves an inherited ivar to its defining class on a live stone and renames it end-to-end with method-survival + new-name assertions, both boundaries. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…328 item 2) Replace the four separate Rename actions in the Refactor… menu with a single "Rename…" that resolves what the cursor is on and runs the matching rename: a message selector or the method header → Rename Method; otherwise the identifier is classified against the stone in scope order — temporary/argument, instance variable (own or inherited), class variable; a class reference → Rename Class; a plain global/shared or pseudo-variable declines with a reason. Selector detection (LSP, AST-based) runs first, so a selector that shares a name with an instance/class variable is never misclassified by the name-based checks. Dispatch runs the existing per-kind commands via executeCommand, reusing every preview/apply flow unchanged — including the inherited-ivar retarget. New: a resolveClassReference query (resolve a bareword to a class plus the SymbolList index binding it) and renameClassAtCursorCommand; renameClass(item) is refactored to share renameClassNamed(oldName, dictArg) with the editor path. Tests: dispatcher precedence, class-at-cursor resolve/decline, the code-action menu, and a class-reference resolution integration case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… Accessors - #11: renaming an inherited class variable no longer dead-ends — resolve its defining class (getDefiningClassOfClassVar) and, after a one-line confirm, rename there across the hierarchy, mirroring the instance-variable retarget. - Add Class Variable: a lightweight class-definition edit (addClassVarName:, no reshape/migration) from the class-variables side "+" and the class context menu. - Add Accessors: generate getter/setter for an instance or class variable (instance side, or class side with a lowercased selector), skipping any that already exist; offered as a row action and as an option when adding a variable. Navigates to the class and reveals the new getter so the accessors are shown. - Unified "Rename…": the offset-based temporary/argument probe is checked before the AST selector probe, so a method-pattern argument renames as an argument, not the method. - Add Instance Variable preview: minimal class-definition diff, expanded by default, plus an "accessors will be added" note when the user opts in. Tests: unit + live-stone integration for the new queries/commands, and controller tests for the add / accessor flows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Palette: declare the unified `gemstone.rename` ("Rename…"); hide the four
superseded granular renames from the Command Palette (still registered for the
dispatcher's internal executeCommand dispatch). Fixes the palette advertising the
old granular renames instead of the consolidated one.
- Scope the instance-variable classification probe: getInstVarNames takes an optional
dict and resolves via classLookupExpr (also fixing a previously-unescaped bareword);
the unified dispatcher and the ivar-at-cursor command pass dict, so every
classification probe resolves a shadowed class name identically (client-side match
for the #9 dictionary-scope hardening).
- Engine SUnit: add dict-scope shadowing tests for GsChangeSignatureRefactoring and
GsRenameClassRefactoring, pinning the #9 isClassInScope: identity gate at those two
call sites (previously covered only transitively via the shared helper);
regenerate engine-tests.gs.
- Tests/comments: correct the stale classification-order test header, ungate the
three base-image defining-class probe tests, and note refreshAfterClassReshape's
reuse for the non-reshaping class-variable add.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
12 tasks
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.
Closes #328.
Follow-ups from @npapagna's review of PR #191, plus two related Explorer conveniences that surfaced along the way. All twelve #328 items are now accounted for (checked off on the issue with resolution comments).
Issue #328 — how each item was handled
gemstone.renamecode action classifies the cursor (temporary/argument → selector/method → instance var → class var → class reference) and dispatchesmain(apply the class-def edit first)mainmainAlso included (not #328 items)
Two Explorer conveniences that pair naturally with the rename work:
addClassVarName:), no reshape/migration.Plus preview/UX polish they depend on: the Add-Instance-Variable preview now shows a clean minimal class-definition diff (expanded by default) with an "accessors will be added" note, and the unified Rename classifies a method-pattern argument correctly (offset-based probe before the AST selector probe).
Notes
Rename…is now the palette entry; the four superseded granular renames are hidden from the palette (still registered for the dispatcher's internal use).Testing
test:gci: green on both boundaries/worlds. Remaininggci/reds on 3.6.2 are the pre-existing base-image gaps (unrelated to this change); the on-demand suite is fully green on 3.7.5+.*.integration.test.tscoverage for every new query/command; nogci/-project tests added (integration convention).🤖 Generated with Claude Code