Safe delete: check what still references a method, class or variable before removing it - #476
Conversation
…ariable Deleting a method, class, instance variable or class variable was unguarded: the same modal appeared whatever the target, nothing was checked, and you found out afterwards -- from a doesNotUnderstand or a failed recompile -- that something still needed it. Each of the four now scans first. Nothing references it, and the deletion happens with no question and a notification saying so, making the click one step instead of two. Something does, and the confirmation says how many and which, one line per referencing class, with the class named once however many of its methods are involved. Show References... lists them for browsing; opening one abandons the deletion, because asking to read a method is not an answer to the question and a modal whose default is destructive should not follow the user there. Closing the list without opening anything brings the question back. References that go away WITH the target do not count: a recursive method's send of its own selector, or a method inside the subtree of the class being removed. A scan that cannot answer falls back to asking -- "could not check" must never resolve to "delete without asking". The scans match how the refactoring engine decides, not source text: bytecode access for an instance variable, literal-frame binding identity for a class variable, and dictionary-scoped object identity for a class -- so a mention in a comment, a same-named global, and a same-named class in another dictionary are all correctly excluded. They ask the base image directly rather than going through the engine, so the guard behaves the same with or without the server plugin installed; deleting a method and deleting a class work without it today, and gating them behind an engine install would have been a regression. Also here: - Remove Class Variable, which the Explorer never had -- it could add and rename one but not remove it. Lightweight like adding one: a class variable is not part of instance layout, so nothing is reshaped, no class version is created, and no engine is involved. It refuses a variable the row's class only inherits. - Removing an instance variable no method accesses now applies without opening the preview panel, since a preview of a change that breaks nothing is a question with one answer. The engine keeps the veto: if it reports methods that will not recompile, the panel opens after all. - The results picker moves out of a closure in activate() into its own module so the confirmation can share it, and reports whether the user opened anything. - Every Explorer removal now shows the trash can. The two variable removals used a minus sign, which reads as "take out of this list" rather than "delete this". - A hand-testing fixture under gs-src/fixtures, with each guarded kind present both referenced and unreferenced, plus a companion that builds the same class name in two dictionaries. Its selectors carry an sdDemo prefix because a selector is image-wide and an unprefixed one picks up unrelated senders. Verified against live 3.6.2 and 3.7.5 stones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ut what they saw
Follow-up to the safe-delete review. The feature looped over method environments
from the start, but three of the four scans underneath it did not, so the loop was
close to a no-op -- and the failure mode was the exact one the guard exists to
prevent: a reference the scan could not see, followed by "nothing referenced it"
and a deletion nobody was asked about.
Scanning for references to a class built its ClassOrganizer without an
environment, so it answered environment 0 whatever it was asked for. The two
variable scans enumerated `selectors`, which lists environment 0 only, so a method
living anywhere else was never even offered to the accessed-variables test. Both
confirmed on a live stone before and after: with the same method compiled into
environments 0 and 1, a bare organizer sees only the first and `selectors` lists
only the first. Both fixes have an integration test that fails against the old
code.
Found methods now carry the environment they were found in. That makes a selector
implemented in two environments count as two methods rather than collapsing into
one row, and opens each result in the environment it actually lives in -- which
also fixes the same latent bug in Senders, Implementors and References, all of
which have always opened their results as environment 0.
Removing an override no longer asks. Every send that resolved to it resolves to
the inherited implementation instead, so nothing is left calling into a hole; the
notification says where they now resolve rather than claiming nothing referenced
it. Previously this was the worst case in the feature: deleting something as
ordinary as printOn: meant a whole-image walk followed by a dialog listing
hundreds of methods that were never at risk. Asking the hierarchy is bounded by
its depth, and the check is one-directional -- finding an implementor above skips
the scan, failing to find one only means we fall through and ask.
The scans now run under a progress notification. They walk the image and block the
extension host, so without one the editor simply froze with no explanation where
the old unguarded delete popped a modal instantly.
A count that comes back at the server-side row cap is now reported as a floor
("At least 500 methods still reference it", with the list marked incomplete)
instead of being stated as fact when the truth may be thousands.
Also fixed, found while checking the icon sweep's own claim: an instance variable
could not be removed from a class with no subclasses at all. The action was gated
on the row's context value being `explorerIvar`, but a leaf class builds its rows
as `explorerIvarNoSubs`, so no delete affordance appeared -- inline or in the menu.
It now matches both, as rename and push-up on the same row already did.
Smaller review points: the ordering of the reference list sorts on class name and
side rather than on the rendered string, so it does not depend on how a collator
weighs the space in "Account class"; the class-variable guard reuses the memoized
accessor and its comment now says it is belt-and-braces rather than describing a
path the tree cannot produce; and the CHANGELOG no longer advertises that refusal
as a user-visible feature, since no row can name an inherited variable.
Verified against live 3.6.2 and 3.7.5 stones.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Explorer delete icons are now all 🗑️While building safe delete, the row actions turned out to disagree about what "delete" looks like. Removing a method, a class or a dictionary used the trash can; removing an instance variable or a class variable used That reads as a different gesture. 🗑️ says this is destroyed; ➖ says this is taken out of a list. The variable rows sit directly under the class rows in the same pane, so the two were adjacent and inconsistent. All five now use 🗑️:
Remove Class Variable is new in this work — the Explorer could add and rename a class variable but never remove one. Accepted trade-off: variable rows and class rows now look alike (pencil + trash), where the minus sign used to tell them apart at a glance. Consistency of meaning was judged more valuable than that distinction. This is a |
npapagna
left a comment
npapagna
left a comment
There was a problem hiding this comment.
Reviewed the safe-delete changes and want to flag a few things before this merges. Grouped by priority below; inline comments carry the details on the ones GitHub lets me anchor to a diff line.
Blockers
gemstoneExplorer.ts(removeClass) - doomed-subtree exclusion filters by class name, not identity (inline comment).safeDelete.ts- the "list not complete" cap hedge is computed after exclusion filters run, so it can go missing when it shouldn't (inline comment).gemstoneExplorer.ts(removeMethod) - self-send exclusion ignores environmentId (inline comment).extension.ts- the six pre-existing dedup blocks (sendersOfSelector,implementorsOfSelector,hierarchyImplementorsOf,browseReferences,sendersOf,implementorsOf, roughly lines 2167-2394) still dedupe onclassName|isMeta|selectoronly, not reusing the new environment-awarededupeMethodResults. I could not attach this inline since those lines are not part of this PR's diff, but the CHANGELOG claims this PR fixes the same latent bug in Senders/Implementors/References too, and as far as I can tell it doesn't yet.methodResultsPicker.ts- the open-browser path still ignores the row's environment (inline comment).
Important, but fine as a follow-up PR
6. gemstoneExplorer.ts (scanReferences) - one blocking round trip per environment instead of one server-side loop (inline comment).
Non-blockers, just a friendly ask
Nice work closing the environment-blindness bug across the scan paths. A few spots ended up with the same logic duplicated in two or more places; would be great to fold these together when convenient, no rush and not holding up this PR (inline comments on each).
Testing gapsWhile cross-checking the blockers above against the test suite, I noticed each one has a related test that passes but stops just short of the scenario that would actually catch the bug. Flagging in case it's useful when fixing them.
A couple of gaps beyond the blockers themselves:
Worth adding coverage for these alongside the fixes so the blockers stay fixed. |
…lete Conflict was the gemstoneFileSystemProvider import list in extension.ts: main added buildMethodUri and parseMethodUri, this branch had moved the buildMethodUri call out to methodResultsPicker.ts. Kept parseMethodUri, which main's code uses; dropped buildMethodUri, which nothing in extension.ts calls any more. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of the safe-delete PR found five ways the guard could tell the user that less depends on the target than really does — the one direction it cannot afford to be wrong in, since a delete that looks safe goes through without a question. - Removing a class excluded its doomed subtree by class NAME. The scan resolves its target through the dictionary by object identity precisely so a shadowed name elsewhere cannot collide, and excluding on the bare name put that collision straight back: an unrelated class in another dictionary that merely shared a name with a doomed subclass had its real, surviving reference thrown away, and the deletion announced that nothing referenced it. The exclusion now keys on name and home dictionary together. - The "list not complete" hedge was derived from the count the dialog was handed, which is the count AFTER the caller drops the references that go away with the target. A capped scan of 500 that lost a recursive method's own send arrived as 499, no longer looked capped, and its count was then stated as exact when the truth may be thousands. Truncation is now observed on the raw rows, per environment (the cap is applied per query), and travels on the target. The mirror case is fixed too: environments summing past the cap without any single query filling up is a complete list and is no longer hedged. - The self-send exclusion for a method matched class, side and selector but not environment, so for a class implementing the same selector in two environments it crossed off the OTHER environment's method as if it were the removed method's own recursion. The Methods pane collapses environments into one row and acts on environment 0, which is now named rather than written as a bare 0. - Senders, Implementors, hierarchy implementors and References each hand-rolled the same fold on class/side/selector, so two environments' methods collapsed into one row. All six sites now call the shared fold, which counts environment as part of a method's identity. It moves to queries/methodSearch.ts, beside the row type every caller already imports. - Picking a result opened the row's environment only when no System Browser was open. With one open — the usual case — navigateTo ignored the row entirely and opened whatever environment the browser was showing. It now switches the browser to the row's environment, and says so in the environment control. Each fix has a test verified to fail against the previous code, plus the mirror test that the exclusion it tightens still excludes what it should. The six command handlers are unreachable from a unit test without standing up the whole extension, so a source scan pins the absence of a second private copy of the fold; GemStone Search is allowed to keep its own, which deliberately shows one row per selector and carries the environment into the open action. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
safeDeleteDemo.gs walks the ordinary paths — referenced vs unreferenced, for each
of the four kinds. This one builds only the situations where the guard used to
under-report, which are the ones worth walking by hand because the failure is
silent: the deletion goes through with no question at all.
- two different classes sharing a name, one a doomed subclass and one not, both
referencing the class being removed, so only the surviving reference should be
named;
- a selector whose scan comes back exactly at the 500-row cap and then loses its
own recursive send to the exclusion, plus one plainly past the cap and one
well under it, so all three ways the count can read are visible side by side;
- the same selector implemented in environment 0 and environment 1, each sending
itself, next to one implemented in environment 0 alone;
- a method and a sender that exist only in environment 1, for the two navigation
paths (a System Browser already open, and none open).
It commits, unlike safeDeleteDemo.gs. Jasper logs in as its own session and cannot
see another session's uncommitted work, so an uncommitted fixture is invisible to
the thing it exists to test, and topaz logging out would throw it away.
A self-check runs at the end and names what is wrong rather than loading quietly:
hand-testing against a fixture that did not build the situation being tested is
worse than not testing at all. Two GemStone details it caught, both worth keeping:
sendersOf: answers a two-element Array whose first element is the methods, so a
size read on the Array itself answers 2 whatever the truth is; and there is no
printPaddedWith:to:.
Verified against 3.7.5: the two same-named classes come out distinct and both
reference the target; the three sender crowds measure 500, 600 and 3; the
environment methods land where intended; and removeSelector:, which is what the
delete calls, takes the environment-0 method and leaves environment 1 standing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A class can implement the same selector in more than one environment, and those
are different methods — the fact the scans in this branch were just fixed to stop
losing. The dialog was still losing it: the reference list names a receiver once,
so two environments' methods collapsed into a single line and the count said
"1 method" for what was really two.
References outside environment 0 are now labelled and get a line of their own:
Account >> #balance
Account [env 1] >> #balance
Environment 0 stays unlabelled, so a stone that never raised
gemstone.maxEnvironment reads exactly as it did before.
Removing a method whose selector is also implemented in another environment now
says so, and says that only the environment-0 method is going — on the
confirmation and on the notification alike. The Methods pane shows one row per
selector however many environments implement it, and a removal takes the
environment-0 method only, so without this a delete appears to take the selector
off the class while an implementation is still standing. The environments are read
off envLines, which the method list is already built from, so it costs no query.
The wider gap this sits in — the Explorer listing environment-1 methods it cannot
open, and giving no per-row indication of environment — is issue #470, whose
acceptance criteria already cover the row-level feedback. This is only the part
that belongs to the dialogs added here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d it A mutation pass over everything this branch changed — break one behaviour, require a test to fail — left three survivors, all the same shape: removing an instance variable, a class variable or a class each carries the scan's row cap through to its confirmation, and nothing checked that any of them did. Only removeMethod had cap tests, and one delete kind getting it right says nothing about the other three, because the wiring is per call site. Each now has one test at the cap and one comfortably under it. removeClass also gets the case that motivated the fix: a capped page whose last row belongs to a subclass going away with the target, so the exclusion drops the count to 499 where it no longer looks capped, and the hedge has to survive that anyway. With these, all 16 mutations are caught — including the ones for the fixes themselves (doomed-subtree identity, the self-send environment, the cap computed on raw rows, the shared environment-aware fold, the browser honouring a result's environment, the environment labels in the dialog). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All five blockers fixed, and the branch is up to date with
|
| Blocker | Fix | Commit |
|---|---|---|
| 1 · doomed subtree matched by name | keys on name and home dictionary | 236ba0a4 |
| 2 · cap hedge computed after exclusions | truncation read off raw rows, per environment | 236ba0a4 |
| 3 · self-send exclusion ignores environment | matches the environment actually being removed | 236ba0a4 |
4 · six dedup blocks in extension.ts |
all route through the shared fold | 236ba0a4 |
| 5 · open-browser path ignores row environment | browser switches to the row's environment | 236ba0a4 |
On blocker 4, which you couldn't anchor inline
You were right that the CHANGELOG claimed something the code didn't do. All six sites now call dedupeMethodResults, which has moved to client/src/queries/methodSearch.ts — beside MethodSearchResult and the row cap, where every caller that sweeps environments already looks. Importing the refactoring module into extension.ts's search commands would have been the wrong direction.
The CHANGELOG entry is corrected and now also names the half that was genuinely missing: results were folded environment-blind and opened as environment 0, and both are fixed.
Those six handlers can't be reached from a unit test without standing up the whole extension, so a source scan pins the absence of a second private copy of the rule (methodResultDedupe.manifest.test.ts), alongside the helper's own tests. It found a seventh site you hadn't listed — omniSearch/omniSearchCommand.ts. That one is deliberate and documented at the call site: GemStone Search shows one row per selector and carries the environment into the open action, which is a different contract from a reference list. It's allow-listed with that reason rather than silently skipped, so please push back if you disagree.
Testing gaps
Your second comment was the more useful of the two — every item was accurate, and the two extras at the end were the ones that mattered most. All closed.
Rather than trust line coverage, I mutated each behaviour this branch changed and required a test to fail. Sixteen mutations. The first pass left three survivors, all your second extra generalised: remove instance variable, remove class variable and remove class each wire the scan's cap through to their own confirmation, and nothing checked that any of them did — only removeMethod had cap tests, and the wiring is per call site. Seven tests added in ae8101aa; all sixteen mutations are now caught.
Also changed, from hand-testing
The dialogs never said which method environment anything was in, so two environments' methods collapsed into one line and the count read "1 method" for what was really two — the same collapse the scans here were just fixed to stop making. References outside environment 0 are now labelled Account [env 1] >> #balance and get their own line, environment 0 stays unlabelled, and removing a method whose selector is also implemented elsewhere says so and says only the environment-0 one is going (8682372b).
Follow-ups filed
- Safe-delete reference scans do one blocking round trip per method environment, so the editor freeze scales with maxEnvironment #483 — the per-environment round trips from your item 6, with the two constraints a server-side loop has to keep.
- Safe delete: fold the duplicated scan loops, reference-resolves-elsewhere rule, and delete-command shape into shared code #484 — the three non-blocking cleanups, consolidated. Your scan-primitive point is flagged there as the highest-value of them.
The Explorer's own multi-environment gaps (lists environment-1 methods it can't open, no per-row environment indication) are already #470; the two queries that ignore a given environment are #475.
Verification
Branch merged up to main (51e84001, one trivial import conflict). Gate: 411 test files, 6192 tests, lint and typecheck clean. Hand-tested against a live 3.7.5 stone with gemstone.maxEnvironment raised, using a fixture committed on the branch (gs-src/fixtures/safeDeleteScanDemo.gs) that builds each of these cases and self-checks that it did — all scenarios confirmed by hand except the already-open-System-Browser path, which was skipped because that file is frozen.
Ready for another look.
2 participants
What & why
Fixes #433.
Deleting a method, class, instance variable or class variable was unguarded. The same modal
appeared whatever the target, nothing was checked, and you found out afterwards — from a
doesNotUnderstand or a failed recompile — that something still needed it. Safe delete was the one
refactoring named in the "Done when" of #295 that had not been built.
Each of the four now scans first:
making the click one step instead of two;
with the class named once however many of its methods are involved. Show References… lists
them for browsing; opening one abandons the deletion, because asking to read a method is not an
answer to the question and a modal whose default action is destructive should not follow you
there. Closing the list without opening anything brings the question back.
References that go away with the target don't count — a recursive method's send of its own
selector, a method inside the subtree of the class being removed, or an override whose senders
will resolve to the inherited implementation. A scan that cannot answer falls back to asking:
"could not check" must never resolve to "delete without asking".
The scans match how the refactoring engine decides rather than reading source text — bytecode
access for an instance variable, literal-frame binding identity for a class variable,
dictionary-scoped object identity for a class — so a mention in a comment, a same-named global,
and a same-named class in another dictionary are all correctly excluded. They ask the base image
directly rather than going through the engine, so the guard behaves the same with or without the
server plugin installed; deleting a method and deleting a class work without it today, and gating
them behind an engine install would have been a regression.
Two commits, so the feature and the follow-up fixes can be read separately.
Changes
1eb9a0d5— the guard, plus Remove Class Variablerefactoring/safeDelete.ts— kept free ofany query so the whole decision table unit-tests without a stone.
remove it. Lightweight like adding one — a class variable is not part of instance layout, so
nothing is reshaped, no class version is created, and no engine is involved.
since a preview of a change that breaks nothing is a question with one answer. The engine keeps
the veto: if it reports methods that will not recompile, the panel opens after all.
activate()into its own module so theconfirmation can share it, and reports whether the user opened anything.
which reads as "take out of this list" rather than "delete this".
gs-src/fixtures, with each guarded kind present both referencedand unreferenced, plus a companion building the same class name in two dictionaries.
02c9595f— environments, and the scans' honesty about what they sawThe feature looped over method environments from the start, but three of the four scans underneath
it did not, so the loop was close to a no-op — and the failure mode was exactly the one the guard
exists to prevent.
ClassOrganizerwithout an environment, so it answeredenvironment 0 whatever it was asked for. The two variable scans enumerated
selectors,which lists environment 0 only, so a method living anywhere else was never offered to the
accessed-variables test. Confirmed on a live stone before and after; both fixes have an
integration test verified to fail against the old code.
environments counts as two methods instead of collapsing into one row, and each result opens the
document it actually lives in — which also fixes the same latent bug in Senders, Implementors and
References, all of which have always opened results as environment 0.
something as ordinary as
printOn:meant a whole-image walk followed by a dialog listinghundreds of methods that were never at risk. Asking the hierarchy is bounded by its depth, and
the check is one-directional — finding an implementor above skips the scan, failing to find one
only means we fall through and ask.
host, so without one the editor simply froze where the old delete popped a modal instantly.
reference it", list marked incomplete) instead of stated as fact when the truth may be thousands.
while checking the icon sweep's own claim. The action was gated on the row's context value being
explorerIvar, but a leaf class builds its rows asexplorerIvarNoSubs, so no delete affordanceappeared, inline or in the menu. It now matches both, as rename and push-up already did.
Testing
server and mcp-server suites; lint, format and compile clean on both.
confirming deletes and their exact reference lists, the three discriminations that must not
appear (a name in a comment, a same-named global, a reference from inside the doomed subtree),
and the two-dictionary shadow case, where deleting each
SdDemoShadownamed only its own caller.already covered by
GsInstVarRefactoringTest.Notes for the reviewer
a referenced method today, so refusing would remove a capability — the guard's job is that you
are told first.
expensive one to get wrong, and recovery is
abort, which discards every other uncommitted editin the session. Left consistent with the other three after manual testing; easy to make classes
always confirm if you'd rather.
answer — a dispatch through
perform:is invisible to it. Documented rather than papered over.block that is written out nine times, are deliberately not in this diff — they affect
commands outside this feature and are filed as Two method-search queries ignore the environment they are given, so results outside environment 0 are invisible #475.