Skip to content

Commit b4a00ca

Browse files
mjbvzmjbvz
committed
Replacing heapservice with lifecycle for code actions
Part of microsoft#74846 Code Actions can use commands internally, which must be disposed of. We were previously using the heap service for this but this will not work for the web. Add a custom lifecycle instead
1 parent 0aaf00b commit b4a00ca

14 files changed

Lines changed: 189 additions & 110 deletions

File tree

‎src/vs/editor/common/modes.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -550,6 +550,10 @@ export interface CodeActionContext {
550550
trigger: CodeActionTrigger;
551551
}
552552

553+
export interface CodeActionList extends IDisposable {
554+
readonly actions: ReadonlyArray<CodeAction>;
555+
}
556+
553557
/**
554558
* The code action interface defines the contract between extensions and
555559
* the [light bulb](https://code.visualstudio.com/docs/editor/editingevolved#_code-action) feature.
@@ -559,7 +563,7 @@ export interface CodeActionProvider {
559563
/**
560564
* Provide commands for the given document and range.
561565
*/
562-
provideCodeActions(model: model.ITextModel, range: Range | Selection, context: CodeActionContext, token: CancellationToken): ProviderResult<CodeAction[]>;
566+
provideCodeActions(model: model.ITextModel, range: Range | Selection, context: CodeActionContext, token: CancellationToken): ProviderResult<CodeActionList>;
563567

564568
/**
565569
* Optional list of CodeActionKinds that this provider returns.

‎src/vs/editor/contrib/codeAction/codeAction.ts‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,9 @@ import { CodeAction, CodeActionContext, CodeActionProviderRegistry, CodeActionTr
1515
import { IModelService } from 'vs/editor/common/services/modelService';
1616
import { CodeActionFilter, CodeActionKind, CodeActionTrigger, filtersAction, mayIncludeActionsOfKind } from './codeActionTrigger';
1717
import { TextModelCancellationTokenSource } from 'vs/editor/browser/core/editorState';
18+
import { Disposable, DisposableStore } from 'vs/base/common/lifecycle';
1819

19-
export class CodeActionSet {
20+
export class CodeActionSet extends Disposable {
2021

2122
private static codeActionsComparator(a: CodeAction, b: CodeAction): number {
2223
if (isNonEmptyArray(a.diagnostics)) {
@@ -34,7 +35,9 @@ export class CodeActionSet {
3435

3536
public readonly actions: readonly CodeAction[];
3637

37-
public constructor(actions: readonly CodeAction[]) {
38+
public constructor(actions: readonly CodeAction[], disposables: DisposableStore) {
39+
super();
40+
this._register(disposables);
3841
this.actions = mergeSort([...actions], CodeActionSet.codeActionsComparator);
3942
}
4043

@@ -59,12 +62,14 @@ export function getCodeActions(
5962
const cts = new TextModelCancellationTokenSource(model, token);
6063
const providers = getCodeActionProviders(model, filter);
6164

65+
const disposables = new DisposableStore();
6266
const promises = providers.map(provider => {
6367
return Promise.resolve(provider.provideCodeActions(model, rangeOrSelection, codeActionContext, cts.token)).then(providedCodeActions => {
64-
if (cts.token.isCancellationRequested || !Array.isArray(providedCodeActions)) {
68+
if (cts.token.isCancellationRequested || !providedCodeActions) {
6569
return [];
6670
}
67-
return providedCodeActions.filter(action => action && filtersAction(filter, action));
71+
disposables.add(providedCodeActions);
72+
return providedCodeActions.actions.filter(action => action && filtersAction(filter, action));
6873
}, (err): CodeAction[] => {
6974
if (isPromiseCanceledError(err)) {
7075
throw err;
@@ -84,7 +89,7 @@ export function getCodeActions(
8489

8590
return Promise.all(promises)
8691
.then(flatten)
87-
.then(actions => new CodeActionSet(actions))
92+
.then(actions => new CodeActionSet(actions, disposables))
8893
.finally(() => {
8994
listener.dispose();
9095
cts.dispose();
@@ -106,7 +111,7 @@ function getCodeActionProviders(
106111
});
107112
}
108113

109-
registerLanguageCommand('_executeCodeActionProvider', function (accessor, args): Promise<ReadonlyArray<CodeAction>> {
114+
registerLanguageCommand('_executeCodeActionProvider', async function (accessor, args): Promise<ReadonlyArray<CodeAction>> {
110115
const { resource, range, kind } = args;
111116
if (!(resource instanceof URI) || !Range.isIRange(range)) {
112117
throw illegalArgument();
@@ -117,9 +122,11 @@ registerLanguageCommand('_executeCodeActionProvider', function (accessor, args):
117122
throw illegalArgument();
118123
}
119124

120-
return getCodeActions(
125+
const codeActionSet = await getCodeActions(
121126
model,
122127
model.validateRange(range),
123128
{ type: 'manual', filter: { includeSourceActions: true, kind: kind && kind.value ? new CodeActionKind(kind.value) : undefined } },
124-
CancellationToken.None).then(actions => actions.actions);
129+
CancellationToken.None);
130+
codeActionSet.dispose();
131+
return codeActionSet.actions;
125132
});

‎src/vs/editor/contrib/codeAction/codeActionCommands.ts‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
import { CancelablePromise } from 'vs/base/common/async';
77
import { KeyCode, KeyMod } from 'vs/base/common/keyCodes';
8-
import { Disposable } from 'vs/base/common/lifecycle';
8+
import { Disposable, dispose } from 'vs/base/common/lifecycle';
99
import { escapeRegExpCharacters } from 'vs/base/common/strings';
1010
import { ICodeEditor } from 'vs/editor/browser/editorBrowser';
1111
import { EditorAction, EditorCommand, ServicesAccessor } from 'vs/editor/browser/editorExtensions';
@@ -47,6 +47,7 @@ export class QuickFixController extends Disposable implements IEditorContributio
4747
private readonly _model: CodeActionModel;
4848
private readonly _codeActionContextMenu: CodeActionContextMenu;
4949
private readonly _lightBulbWidget: LightBulbWidget;
50+
private _currentCodeActions: CodeActionSet | undefined;
5051

5152
private _activeRequest: CancelablePromise<CodeActionSet> | undefined;
5253

@@ -63,7 +64,7 @@ export class QuickFixController extends Disposable implements IEditorContributio
6364
super();
6465

6566
this._editor = editor;
66-
this._model = new CodeActionModel(this._editor, markerService, contextKeyService, progressService);
67+
this._model = this._register(new CodeActionModel(this._editor, markerService, contextKeyService, progressService));
6768
this._codeActionContextMenu = this._register(new CodeActionContextMenu(editor, contextMenuService, action => this._onApplyCodeAction(action)));
6869
this._lightBulbWidget = this._register(new LightBulbWidget(editor));
6970

@@ -75,9 +76,9 @@ export class QuickFixController extends Disposable implements IEditorContributio
7576
this._register(this._keybindingService.onDidUpdateKeybindings(this._updateLightBulbTitle, this));
7677
}
7778

78-
public dispose(): void {
79+
dipose() {
7980
super.dispose();
80-
this._model.dispose();
81+
dispose(this._currentCodeActions);
8182
}
8283

8384
private _onDidChangeCodeActionsState(newState: CodeActionsState.State): void {
@@ -89,6 +90,11 @@ export class QuickFixController extends Disposable implements IEditorContributio
8990
if (newState.type === CodeActionsState.Type.Triggered) {
9091
this._activeRequest = newState.actions;
9192

93+
newState.actions.then(actions => {
94+
dispose(this._currentCodeActions);
95+
this._currentCodeActions = actions;
96+
});
97+
9298
if (newState.trigger.filter && newState.trigger.filter.kind) {
9399
// Triggered for specific scope
94100
newState.actions.then(fixes => {
@@ -115,6 +121,8 @@ export class QuickFixController extends Disposable implements IEditorContributio
115121
}
116122
}
117123
} else {
124+
dispose(this._currentCodeActions);
125+
this._currentCodeActions = undefined;
118126
this._lightBulbWidget.hide();
119127
}
120128
}

‎src/vs/editor/contrib/codeAction/test/codeAction.test.ts‎

Lines changed: 50 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,27 @@ import { DisposableStore } from 'vs/base/common/lifecycle';
77
import { URI } from 'vs/base/common/uri';
88
import { Range } from 'vs/editor/common/core/range';
99
import { TextModel } from 'vs/editor/common/model/textModel';
10-
import { CodeAction, CodeActionContext, CodeActionProvider, CodeActionProviderRegistry, Command, LanguageIdentifier, ResourceTextEdit, WorkspaceEdit } from 'vs/editor/common/modes';
10+
import * as modes from 'vs/editor/common/modes';
1111
import { getCodeActions } from 'vs/editor/contrib/codeAction/codeAction';
1212
import { CodeActionKind } from 'vs/editor/contrib/codeAction/codeActionTrigger';
1313
import { IMarkerData, MarkerSeverity } from 'vs/platform/markers/common/markers';
1414
import { CancellationToken } from 'vs/base/common/cancellation';
1515

16+
function staticCodeActionProvider(...actions: modes.CodeAction[]): modes.CodeActionProvider {
17+
return new class implements modes.CodeActionProvider {
18+
provideCodeActions(): modes.CodeActionList {
19+
return {
20+
actions: actions,
21+
dispose: () => { }
22+
};
23+
}
24+
};
25+
}
26+
27+
1628
suite('CodeAction', () => {
1729

18-
let langId = new LanguageIdentifier('fooLang', 17);
30+
let langId = new modes.LanguageIdentifier('fooLang', 17);
1931
let uri = URI.parse('untitled:path');
2032
let model: TextModel;
2133
const disposables = new DisposableStore();
@@ -46,7 +58,7 @@ suite('CodeAction', () => {
4658
},
4759
command: {
4860
abc: {
49-
command: new class implements Command {
61+
command: new class implements modes.Command {
5062
id: '1';
5163
title: 'abc';
5264
},
@@ -56,8 +68,8 @@ suite('CodeAction', () => {
5668
spelling: {
5769
bcd: {
5870
diagnostics: <IMarkerData[]>[],
59-
edit: new class implements WorkspaceEdit {
60-
edits: ResourceTextEdit[];
71+
edit: new class implements modes.WorkspaceEdit {
72+
edits: modes.ResourceTextEdit[];
6173
},
6274
title: 'abc'
6375
}
@@ -90,20 +102,16 @@ suite('CodeAction', () => {
90102

91103
test('CodeActions are sorted by type, #38623', async function () {
92104

93-
const provider = new class implements CodeActionProvider {
94-
provideCodeActions() {
95-
return [
96-
testData.command.abc,
97-
testData.diagnostics.bcd,
98-
testData.spelling.bcd,
99-
testData.tsLint.bcd,
100-
testData.tsLint.abc,
101-
testData.diagnostics.abc
102-
];
103-
}
104-
};
105+
const provider = staticCodeActionProvider(
106+
testData.command.abc,
107+
testData.diagnostics.bcd,
108+
testData.spelling.bcd,
109+
testData.tsLint.bcd,
110+
testData.tsLint.abc,
111+
testData.diagnostics.abc
112+
);
105113

106-
disposables.add(CodeActionProviderRegistry.register('fooLang', provider));
114+
disposables.add(modes.CodeActionProviderRegistry.register('fooLang', provider));
107115

108116
const expected = [
109117
// CodeActions with a diagnostics array are shown first ordered by diagnostics.message
@@ -123,17 +131,13 @@ suite('CodeAction', () => {
123131
});
124132

125133
test('getCodeActions should filter by scope', async function () {
126-
const provider = new class implements CodeActionProvider {
127-
provideCodeActions(): CodeAction[] {
128-
return [
129-
{ title: 'a', kind: 'a' },
130-
{ title: 'b', kind: 'b' },
131-
{ title: 'a.b', kind: 'a.b' }
132-
];
133-
}
134-
};
134+
const provider = staticCodeActionProvider(
135+
{ title: 'a', kind: 'a' },
136+
{ title: 'b', kind: 'b' },
137+
{ title: 'a.b', kind: 'a.b' }
138+
);
135139

136-
disposables.add(CodeActionProviderRegistry.register('fooLang', provider));
140+
disposables.add(modes.CodeActionProviderRegistry.register('fooLang', provider));
137141

138142
{
139143
const { actions } = await getCodeActions(model, new Range(1, 1, 2, 1), { type: 'auto', filter: { kind: new CodeActionKind('a') } }, CancellationToken.None);
@@ -155,32 +159,31 @@ suite('CodeAction', () => {
155159
});
156160

157161
test('getCodeActions should forward requested scope to providers', async function () {
158-
const provider = new class implements CodeActionProvider {
159-
provideCodeActions(_model: any, _range: Range, context: CodeActionContext, _token: any): CodeAction[] {
160-
return [
161-
{ title: context.only || '', kind: context.only }
162-
];
162+
const provider = new class implements modes.CodeActionProvider {
163+
provideCodeActions(_model: any, _range: Range, context: modes.CodeActionContext, _token: any): modes.CodeActionList {
164+
return {
165+
actions: [
166+
{ title: context.only || '', kind: context.only }
167+
],
168+
dispose: () => { }
169+
};
163170
}
164171
};
165172

166-
disposables.add(CodeActionProviderRegistry.register('fooLang', provider));
173+
disposables.add(modes.CodeActionProviderRegistry.register('fooLang', provider));
167174

168175
const { actions } = await getCodeActions(model, new Range(1, 1, 2, 1), { type: 'auto', filter: { kind: new CodeActionKind('a') } }, CancellationToken.None);
169176
assert.equal(actions.length, 1);
170177
assert.strictEqual(actions[0].title, 'a');
171178
});
172179

173180
test('getCodeActions should not return source code action by default', async function () {
174-
const provider = new class implements CodeActionProvider {
175-
provideCodeActions(): CodeAction[] {
176-
return [
177-
{ title: 'a', kind: CodeActionKind.Source.value },
178-
{ title: 'b', kind: 'b' }
179-
];
180-
}
181-
};
181+
const provider = staticCodeActionProvider(
182+
{ title: 'a', kind: CodeActionKind.Source.value },
183+
{ title: 'b', kind: 'b' }
184+
);
182185

183-
disposables.add(CodeActionProviderRegistry.register('fooLang', provider));
186+
disposables.add(modes.CodeActionProviderRegistry.register('fooLang', provider));
184187

185188
{
186189
const { actions } = await getCodeActions(model, new Range(1, 1, 2, 1), { type: 'auto' }, CancellationToken.None);
@@ -197,16 +200,16 @@ suite('CodeAction', () => {
197200

198201
test('getCodeActions should not invoke code action providers filtered out by providedCodeActionKinds', async function () {
199202
let wasInvoked = false;
200-
const provider = new class implements CodeActionProvider {
201-
provideCodeActions() {
203+
const provider = new class implements modes.CodeActionProvider {
204+
provideCodeActions(): modes.CodeActionList {
202205
wasInvoked = true;
203-
return [];
206+
return { actions: [], dispose: () => { } };
204207
}
205208

206209
providedCodeActionKinds = [CodeActionKind.Refactor.value];
207210
};
208211

209-
disposables.add(CodeActionProviderRegistry.register('fooLang', provider));
212+
disposables.add(modes.CodeActionProviderRegistry.register('fooLang', provider));
210213

211214
const { actions } = await getCodeActions(model, new Range(1, 1, 2, 1), {
212215
type: 'auto',

‎src/vs/editor/contrib/codeAction/test/codeActionModel.test.ts‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,19 +9,24 @@ import { URI } from 'vs/base/common/uri';
99
import { ICodeEditor } from 'vs/editor/browser/editorBrowser';
1010
import { Selection } from 'vs/editor/common/core/selection';
1111
import { TextModel } from 'vs/editor/common/model/textModel';
12-
import { CodeActionProviderRegistry, LanguageIdentifier } from 'vs/editor/common/modes';
12+
import * as modes from 'vs/editor/common/modes';
1313
import { CodeActionOracle, CodeActionsState } from 'vs/editor/contrib/codeAction/codeActionModel';
1414
import { createTestCodeEditor } from 'vs/editor/test/browser/testCodeEditor';
1515
import { MarkerService } from 'vs/platform/markers/common/markerService';
1616

1717
const testProvider = {
18-
provideCodeActions() {
19-
return [{ id: 'test-command', title: 'test', arguments: [] }];
18+
provideCodeActions(): modes.CodeActionList {
19+
return {
20+
actions: [
21+
{ title: 'test', command: { id: 'test-command', title: 'test', arguments: [] } }
22+
],
23+
dispose() { /* noop*/ }
24+
};
2025
}
2126
};
2227
suite('CodeAction', () => {
2328

24-
const languageIdentifier = new LanguageIdentifier('foo-lang', 3);
29+
const languageIdentifier = new modes.LanguageIdentifier('foo-lang', 3);
2530
let uri = URI.parse('untitled:path');
2631
let model: TextModel;
2732
let markerService: MarkerService;
@@ -44,7 +49,7 @@ suite('CodeAction', () => {
4449
});
4550

4651
test('Orcale -> marker added', done => {
47-
const reg = CodeActionProviderRegistry.register(languageIdentifier.language, testProvider);
52+
const reg = modes.CodeActionProviderRegistry.register(languageIdentifier.language, testProvider);
4853
disposables.add(reg);
4954

5055
const oracle = new CodeActionOracle(editor, markerService, (e: CodeActionsState.Triggered) => {
@@ -70,7 +75,7 @@ suite('CodeAction', () => {
7075
});
7176

7277
test('Orcale -> position changed', () => {
73-
const reg = CodeActionProviderRegistry.register(languageIdentifier.language, testProvider);
78+
const reg = modes.CodeActionProviderRegistry.register(languageIdentifier.language, testProvider);
7479
disposables.add(reg);
7580

7681
markerService.changeOne('fake', uri, [{
@@ -100,9 +105,9 @@ suite('CodeAction', () => {
100105
});
101106

102107
test('Lightbulb is in the wrong place, #29933', async function () {
103-
const reg = CodeActionProviderRegistry.register(languageIdentifier.language, {
104-
provideCodeActions(_doc, _range) {
105-
return [];
108+
const reg = modes.CodeActionProviderRegistry.register(languageIdentifier.language, {
109+
provideCodeActions(_doc, _range): modes.CodeActionList {
110+
return { actions: [], dispose() { /* noop*/ } };
106111
}
107112
});
108113
disposables.add(reg);
@@ -138,7 +143,7 @@ suite('CodeAction', () => {
138143
});
139144

140145
test('Orcale -> should only auto trigger once for cursor and marker update right after each other', done => {
141-
const reg = CodeActionProviderRegistry.register(languageIdentifier.language, testProvider);
146+
const reg = modes.CodeActionProviderRegistry.register(languageIdentifier.language, testProvider);
142147
disposables.add(reg);
143148

144149
let triggerCount = 0;

0 commit comments

Comments
 (0)