Safe delete: check what still references a method, class or variable before removing it

CHANGELOG.md

@@ -4,6 +4,24 @@ All notable changes to the **GemStone Smalltalk** extension will be documented i
 
 ## [Unreleased]
 
+### Changed
+
+- **The two variable removals now show the trash can, like every other Explorer delete.** Removing an instance variable or a class variable used the `$(remove)` minus sign while removing a method, a class or a dictionary used the trash can — so the variable rows read as "take out of this list" rather than "delete this", a different gesture from the rows right above them. All five now match. ([#433](https://github.com/GemTalk/Jasper/issues/433))
+
+### Fixed
+
+- **An instance variable could not be removed from a class with no subclasses.** The Explorer's Remove Instance Variable action was gated on the row's context value being `explorerIvar`, but a class without subclasses builds its instance-variable rows as `explorerIvarNoSubs` — so on a leaf class the row offered no delete at all, inline or in the context menu. It now matches both, as the rename and push-up actions on the same row already did. ([#433](https://github.com/GemTalk/Jasper/issues/433))
+- **A reference scan could miss methods outside environment 0, and then report that nothing referenced the target.** Scanning for references to a class built its `ClassOrganizer` without an environment, so it searched environment 0 whatever it was asked for; the two variable scans enumerated `selectors`, which likewise lists environment 0 only. On a stone with `gemstone.maxEnvironment` above 0 that made a real reference invisible — and safe delete would then take the silent path and announce that nothing referenced it. Found methods also now carry the environment they were found in, so a selector implemented in two environments counts as two methods and each opens the document it actually lives in — on both paths: picking a result switches an already-open System Browser to that environment, where it used to open whatever environment the browser happened to be showing, and with no browser open the method is opened directly there. That last part fixes the same latent bug in Senders, Implementors, hierarchy implementors and References, which folded two environments' methods into one row and opened results as environment 0; all of them now share one fold that counts the environment as part of a method's identity. ([#433](https://github.com/GemTalk/Jasper/issues/433))
+- **A reference count could hit the scan's row cap and still be reported as exact.** The scan returns at most 500 rows per query, and the confirmation is supposed to say "At least 500 … (the list below is not complete)" rather than state a number that may really be thousands. Whether the cap was hit was worked out from the count the dialog was handed — after the references that go away with the target had already been dropped — so a capped scan of 500 that lost a recursive method's own send arrived as 499, and the warning silently disappeared from the one dialog that most needed it. The cap is now observed on the raw scan, per environment, and travels with the result: exclusions can no longer hide it, and several environments summing past 500 without any one query filling up is correctly reported as a complete list. ([#433](https://github.com/GemTalk/Jasper/issues/433))
+- **Removing a class could discard a real reference from an unrelated class of the same name.** The subtree going away with the class is not a reason to ask, so its own methods are excluded — but they were excluded by class *name*, while the scan deliberately resolves through the dictionary by object identity so a shadowed name elsewhere cannot collide. A class in another dictionary that merely shared a name with a doomed subclass therefore had its surviving reference thrown away, and the deletion went through announcing that nothing referenced it. The exclusion now matches on name and home dictionary together. ([#433](https://github.com/GemTalk/Jasper/issues/433))
+- **The delete confirmation never said which method environment anything was in.** A class can implement the same selector in more than one environment, and those are different methods — but the reference list named a receiver once, so two of them collapsed into a single line and the count said "1 method" for what was really two. References outside environment 0 are now labelled (`Account [env 1] >> #balance`) and get a line of their own, and environment 0 stays unlabelled so a stone that never raised `gemstone.maxEnvironment` reads exactly as 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 however many environments implement a selector, so without it a removal appears to take the selector off the class while an implementation is still standing. ([#433](https://github.com/GemTalk/Jasper/issues/433))
+- **Removing a method could hide a sender that survives it.** A method's send of its own selector goes away with it, so it is discounted — but the match ignored the environment, and a class can implement the same selector on the same side in two environments. Deleting the one the Methods pane acts on crossed off the other environment's method as if it were the removed method's own recursion, understating the references. ([#433](https://github.com/GemTalk/Jasper/issues/433))
+
+### Added
+
+- **Deleting now looks for what still uses the thing first.** Removing a method, a class, an instance variable or a class variable used to pop the same confirmation whatever the target, and check nothing — you found out afterwards, from a doesNotUnderstand or a failed recompile, that something still needed it. Each of the four now scans first: senders for a method, references and subclasses for a class, accessing methods for either kind of variable. When nothing references it the deletion just happens and is reported in a notification, so the click is one step instead of two; when something does, the confirmation says how many and which — one line per referencing class, the class named once however many of its methods are involved — and **Show References…** lists them for browsing before you decide whether to remove it anyway. Opening one of those references abandons the deletion rather than re-raising the question — you asked to go read that method, and a modal whose default action is destructive should not follow you there; closing the list without opening anything brings the question back, since that is still deciding rather than going somewhere. References that go away *with* the target don't count: a recursive method's send of its own selector, or a doomed subclass's use of the class being removed. Nor does removing an **override** raise a question — every send that resolved there resolves to the inherited implementation instead, so the notification says where they now resolve rather than pretending nothing referenced it, and the whole-image sender scan is skipped entirely. A scan that cannot answer falls back to asking. The variable scans match the way the refactoring engine does — bytecode access for an instance variable, binding identity for a class variable — so a name in a comment, or a same-named global, is not a reference; none of it needs the server plugin installed. A method scan asks about the selector image-wide, which is what the image can answer, so a dispatch through `perform:` is invisible to it. Every scan sweeps the method environments you have configured, runs under a progress notification because it walks the image, and hedges its count rather than stating it as fact when it comes back at the row cap. ([#433](https://github.com/GemTalk/Jasper/issues/433))
+- **Remove Class Variable.** The Explorer could add and rename a class variable but never remove one. The trash can on a class-variable row (and its context-menu entry) now does, guarded like every other delete. Like adding one it is lightweight — a class variable is not part of instance layout, so nothing is reshaped, no class version is created, and no refactoring engine is involved — and the removal is refused server-side for a variable a class only inherits, so the query and MCP paths cannot take one off the wrong class either. Nothing is committed until you commit the session. ([#433](https://github.com/GemTalk/Jasper/issues/433))
+
 ## [1.8.13] - 2026-08-20
 
 A follow-up release for **GemStone Search**: correctness fixes for multi-session and multi-environment use, matching and debounce repairs found by a review pass over the feature, one naming pass, and the senders/implementors counts moving off the method source.

README.md

@@ -174,7 +174,8 @@ Beyond browsing, the Explorer is where the code-changing operations live:
 
 - Filter any pane by name, with `*` as a wildcard, plus `reads:`/`writes:`/`accesses:` in the Methods pane to find the methods touching an instance variable
 - Group methods by category, or list them flat
-- Add, rename, and delete dictionaries, class categories, classes, methods, and instance variables; rename class variables
+- Add, rename, and delete dictionaries, class categories, classes, methods, instance variables, and class variables
+- Deleting first looks for what still references the target: nothing does, and it just goes (and says so); something does, and you are shown the methods before you decide
 - The refactorings — rename, extract/inline method and temporary, change signature, move/push up/push down method, instance-variable structure changes, extract superclass, split class — each previewed before it is applied
 - Browse senders, implementors, references, and the class hierarchy
 - Drag and drop methods between categories, and classes between dictionaries

client/src/__tests__/browserQueries.test.ts

@@ -66,13 +66,15 @@ describe('browserQueries', () => {
         isMeta: false,
         selector: 'size',
         category: 'accessing',
+        environmentId: 0,
       });
       expect(results[1]).toEqual({
         dictName: 'UserGlobals',
         className: 'MyClass',
         isMeta: true,
         selector: 'printOn:',
         category: 'printing',
+        environmentId: 0,
       });
     });
 
@@ -105,6 +107,7 @@ describe('browserQueries', () => {
         isMeta: false,
         selector: 'size',
         category: 'accessing',
+        environmentId: 0,
       });
     });
 
@@ -276,6 +279,7 @@ describe('browserQueries', () => {
         isMeta: false,
         selector: 'subarray',
         category: 'accessing',
+        environmentId: 0,
       });
     });
   });

client/src/__tests__/debuggerPanel.test.ts

@@ -89,6 +89,7 @@ vi.mock('../debugQueries', () => ({
     isMeta: false,
     dictName: 'UserGlobals',
     category: 'running',
+    environmentId: 0,
   })),
   // Whole-stack dump (#10/#11): one batched call. Default = a Receiver row per
   // frame whose printString/oop mirror the per-frame receiverOop (level * 100).
@@ -774,6 +775,7 @@ describe('DebuggerPanel', () => {
           isMeta: false,
           selector: 'halt',
           category: 'running',
+          environmentId: 0,
         },
         vscode.ViewColumn.Beside,
       );
@@ -1056,6 +1058,7 @@ describe('DebuggerPanel', () => {
       className: 'JasperDebugDemo',
       isMeta: false,
       category: 'accessing',
+      environmentId: 0,
       selector: 'accumulateFrom:to:',
     };
 
@@ -2560,6 +2563,7 @@ describe('DebuggerPanel', () => {
         className: 'JasperDebugDemo',
         isMeta: false,
         category: 'accessing',
+        environmentId: 0,
         selector: 'accumulateFrom:to:',
       };
       const flush = () => new Promise((resolve) => setTimeout(resolve, 0));
@@ -3187,6 +3191,7 @@ describe('DebuggerPanel', () => {
       className: 'JasperDebugDemo',
       isMeta: false,
       category: 'accessing',
+      environmentId: 0,
       selector: 'accumulateFrom:to:',
     };
 
@@ -3343,6 +3348,7 @@ describe('DebuggerPanel', () => {
       className: 'JasperDebugDemo',
       isMeta: false,
       category: 'running',
+      environmentId: 0,
       selector: 'finish',
     };
 

client/src/__tests__/explorerInstVar.test.ts

@@ -7,8 +7,14 @@ vi.mock('vscode', () => import('../__mocks__/vscode.js'));
 vi.mock('../browserQueries', () => ({
   addAccessors: vi.fn(),
   getClassEnvironments: vi.fn(() => []),
+  methodsAccessingInstVar: vi.fn(() => []),
 }));
 vi.mock('../refactoring/instVarRefactorCommand', () => ({ runInstVarRefactor: vi.fn() }));
+vi.mock('../methodResultsPicker', () => ({
+  showMethodResults: vi.fn(),
+  describeMethodResult: (r: { className: string; isMeta: boolean; selector: string }) =>
+    `${r.className}${r.isMeta ? ' class' : ''} >> #${r.selector}`,
+}));
 
 import * as vscode from 'vscode';
 import { ExplorerController } from '../gemstoneExplorer';
@@ -54,6 +60,7 @@ const outcome = (over: Partial<InstVarRefactorOutcome> = {}): InstVarRefactorOut
   applied: 2,
   committed: false,
   dropped: [],
+  autoApplied: true,
   ...over,
 });
 
@@ -62,6 +69,10 @@ beforeEach(() => {
   // Default the accessors prompt to "No accessors" so the add flows through without
   // generating them; individual tests override to opt in or to escape.
   vi.mocked(vscode.window.showQuickPick).mockResolvedValue('No accessors' as never);
+  // clearAllMocks drops call history but keeps implementations, so restore the two a
+  // remove test overrides — otherwise the override leaks into a shuffled neighbour.
+  vi.mocked(queries.methodsAccessingInstVar).mockReturnValue([]);
+  vi.mocked(vscode.window.showWarningMessage).mockResolvedValue(undefined);
 });
 
 describe('ExplorerController add instance variable', () => {
@@ -210,6 +221,15 @@ describe('ExplorerController add instance variable from the variable-side node',
 });
 
 describe('ExplorerController remove instance variable', () => {
+  const accessor = (selector: string) => ({
+    dictName: 'UserGlobals',
+    className: 'Foo',
+    isMeta: false,
+    selector,
+    category: 'accessing',
+    environmentId: 0,
+  });
+
   it('does nothing when there is no selected session', async () => {
     const { ctl, refresh } = makeController(undefined);
 
@@ -239,4 +259,135 @@ describe('ExplorerController remove instance variable', () => {
 
     expect(refresh).not.toHaveBeenCalled();
   });
+
+  it('removes a variable no method accesses without a preview or a question', async () => {
+    const { ctl } = makeController({} as ActiveSession);
+    vi.mocked(runInstVarRefactor).mockResolvedValue(outcome());
+
+    await ctl.removeInstVar({ className: 'Foo', ivarName: 'count' });
+
+    expect(vscode.window.showWarningMessage).not.toHaveBeenCalled();
+    expect(runInstVarRefactor).toHaveBeenCalledWith(expect.objectContaining({ autoApply: true }));
+  });
+
+  it('announces a removal nothing had to be asked about', async () => {
+    const { ctl } = makeController({} as ActiveSession);
+    vi.mocked(runInstVarRefactor).mockResolvedValue(outcome());
+
+    await ctl.removeInstVar({ className: 'Foo', ivarName: 'count' });
+
+    expect(vscode.window.showInformationMessage).toHaveBeenCalledWith(
+      expect.stringContaining('Removed instance variable count from Foo'),
+    );
+  });
+
+  it('asks before removing a variable methods still access', async () => {
+    vi.mocked(queries.methodsAccessingInstVar).mockReturnValue([accessor('total')]);
+    vi.mocked(vscode.window.showWarningMessage).mockResolvedValue(undefined);
+    const { ctl } = makeController({} as ActiveSession);
+
+    await ctl.removeInstVar({ className: 'Foo', ivarName: 'count' });
+
+    expect(vscode.window.showWarningMessage).toHaveBeenCalled();
+    expect(runInstVarRefactor).not.toHaveBeenCalled();
+  });
+
+  it('names the accessing methods in the confirmation', async () => {
+    vi.mocked(queries.methodsAccessingInstVar).mockReturnValue([accessor('total')]);
+    vi.mocked(vscode.window.showWarningMessage).mockResolvedValue(undefined);
+    const { ctl } = makeController({} as ActiveSession);
+
+    await ctl.removeInstVar({ className: 'Foo', ivarName: 'count' });
+
+    const detail = vi.mocked(vscode.window.showWarningMessage).mock.calls[0][1] as {
+      detail: string;
+    };
+    expect(detail.detail).toContain('Foo >> #total');
+  });
+
+  it('opens the preview once the user chooses to remove an accessed variable anyway', async () => {
+    vi.mocked(queries.methodsAccessingInstVar).mockReturnValue([accessor('total')]);
+    vi.mocked(vscode.window.showWarningMessage).mockResolvedValue('Remove Anyway' as never);
+    vi.mocked(runInstVarRefactor).mockResolvedValue(outcome({ autoApplied: false }));
+    const { ctl } = makeController({} as ActiveSession);
+
+    await ctl.removeInstVar({ className: 'Foo', ivarName: 'count' });
+
+    expect(runInstVarRefactor).toHaveBeenCalledWith(expect.objectContaining({ autoApply: false }));
+  });
+
+  it('does not announce a removal the engine sent to the preview after all', async () => {
+    // The client scan found no accessors, but the engine reported methods that will not
+    // recompile — so the panel opened and the user was asked. That is not a silent delete.
+    vi.mocked(runInstVarRefactor).mockResolvedValue(outcome({ autoApplied: false }));
+    const { ctl } = makeController({} as ActiveSession);
+
+    await ctl.removeInstVar({ className: 'Foo', ivarName: 'count' });
+
+    expect(vscode.window.showInformationMessage).not.toHaveBeenCalled();
+  });
+
+  it('asks rather than removing unasked when the access scan fails', async () => {
+    vi.mocked(queries.methodsAccessingInstVar).mockImplementation(() => {
+      throw new Error('a SecurityError occurred');
+    });
+    vi.mocked(vscode.window.showWarningMessage).mockResolvedValue(undefined);
+    const { ctl } = makeController({} as ActiveSession);
+
+    await ctl.removeInstVar({ className: 'Foo', ivarName: 'count' });
+
+    expect(vscode.window.showWarningMessage).toHaveBeenCalled();
+    expect(runInstVarRefactor).not.toHaveBeenCalled();
+  });
+});
+
+// The scan is capped per query, server-side, and the client cannot tell a full page from an
+// exact answer — so at the cap the count has to read as a floor. Each delete kind wires the cap
+// through from its own scan, so each needs its own test: one kind getting it right says nothing
+// about the other three.
+describe('ExplorerController.removeInstVar — reporting a scan that came back full', () => {
+  const CAP = 500;
+  const accessorRow = (className: string, selector: string) => ({
+    dictName: 'UserGlobals',
+    className,
+    isMeta: false,
+    selector,
+    category: 'accessing',
+    environmentId: 0,
+  });
+
+  beforeEach(() => {
+    vi.mocked(runInstVarRefactor).mockResolvedValue(undefined);
+  });
+
+  it('states the count as a floor and says the list is incomplete', async () => {
+    vi.mocked(queries.methodsAccessingInstVar).mockReturnValue(
+      Array.from({ length: CAP }, (_, i) => accessorRow(`C${i}`, 'usesIt')),
+    );
+    vi.mocked(vscode.window.showWarningMessage).mockResolvedValue(undefined);
+    const { ctl } = makeController({} as ActiveSession);
+
+    await ctl.removeInstVar({ className: 'Foo', ivarName: 'count' });
+
+    const detail = vi.mocked(vscode.window.showWarningMessage).mock.calls[0][1] as {
+      detail: string;
+    };
+    expect(detail.detail).toContain(`At least ${CAP} methods still reference it`);
+    expect(detail.detail).toContain('not complete');
+  });
+
+  it('states a short count plainly, with no hedge', async () => {
+    vi.mocked(queries.methodsAccessingInstVar).mockReturnValue([accessorRow('Foo', 'total')]);
+    vi.mocked(vscode.window.showWarningMessage).mockResolvedValue(undefined);
+    const { ctl } = makeController({} as ActiveSession);
+
+    await ctl.removeInstVar({ className: 'Foo', ivarName: 'count' });
+
+    const detail = vi.mocked(vscode.window.showWarningMessage).mock.calls[0][1] as {
+      detail: string;
+    };
+    expect(detail.detail).toContain('1 method still references it:');
+    expect(detail.detail).not.toContain('At least');
+    expect(detail.detail).not.toContain('not complete');
+  });
 });