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

CHANGELOG.md

@@ -4,6 +4,20 @@ 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; that last part fixes the same latent bug in Senders, Implementors and References, which have always opened results as environment 0. ([#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,84 @@ 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();
+  });
 });