Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adopt ensureNoDisposablesLeaked in code action model tests #209279

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
1 change: 0 additions & 1 deletion .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,6 @@
"src/vs/base/test/browser/browser.test.ts",
"src/vs/base/test/browser/ui/scrollbar/scrollableElement.test.ts",
"src/vs/base/test/browser/ui/scrollbar/scrollbarState.test.ts",
"src/vs/editor/contrib/codeAction/test/browser/codeActionModel.test.ts",
"src/vs/editor/test/common/services/languageService.test.ts",
"src/vs/editor/test/node/classification/typescript.test.ts",
"src/vs/platform/configuration/test/common/configuration.test.ts",
Expand Down
2 changes: 1 addition & 1 deletion src/vs/editor/contrib/codeAction/browser/codeAction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ export async function getCodeActions(

try {
const actions = await Promise.all(promises);
const allActions = actions.map(x => x.actions).flat();
const allActions = actions.flatMap(x => x.actions);
const allDocumentation = [
...coalesce(actions.map(x => x.documentation)),
...getAdditionalDocumentationForShowingActions(registry, model, trigger, allActions)
Expand Down
16 changes: 11 additions & 5 deletions src/vs/editor/contrib/codeAction/browser/codeActionModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { CancelablePromise, createCancelablePromise, TimeoutTimer } from 'vs/base/common/async';
import { isCancellationError } from 'vs/base/common/errors';
import { Emitter } from 'vs/base/common/event';
import { Disposable, MutableDisposable } from 'vs/base/common/lifecycle';
import { Disposable, DisposableStore, MutableDisposable } from 'vs/base/common/lifecycle';
import { isEqual } from 'vs/base/common/resources';
import { URI } from 'vs/base/common/uri';
import { ICodeEditor } from 'vs/editor/browser/editorBrowser';
Expand Down Expand Up @@ -163,6 +163,8 @@ export class CodeActionModel extends Disposable {
private readonly _onDidChangeState = this._register(new Emitter<CodeActionsState.State>());
public readonly onDidChangeState = this._onDidChangeState.event;

private readonly disposables = this._register(new DisposableStore());

private _disposed = false;

constructor(
Expand Down Expand Up @@ -229,7 +231,7 @@ export class CodeActionModel extends Disposable {

const actions = createCancelablePromise(async token => {
if (this._settingEnabledNearbyQuickfixes() && trigger.trigger.type === CodeActionTriggerType.Invoke && (trigger.trigger.triggerAction === CodeActionTriggerSource.QuickFix || trigger.trigger.filter?.include?.contains(CodeActionKind.QuickFix))) {
const codeActionSet = await getCodeActions(this._registry, model, trigger.selection, trigger.trigger, Progress.None, token);
const codeActionSet = this.disposables.add(await getCodeActions(this._registry, model, trigger.selection, trigger.trigger, Progress.None, token));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this introduces a leak as items are added to disposables but never removed

Instead try to make sure the old disposable values are released once a new set of values come in

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like something for a DisposableStore and/or a MutableDisposable

const allCodeActions = [...codeActionSet.allActions];
if (token.isCancellationRequested) {
return emptyCodeActionSet;
Expand Down Expand Up @@ -270,7 +272,7 @@ export class CodeActionModel extends Disposable {
};

const selectionAsPosition = new Selection(trackedPosition.lineNumber, trackedPosition.column, trackedPosition.lineNumber, trackedPosition.column);
const actionsAtMarker = await getCodeActions(this._registry, model, selectionAsPosition, newCodeActionTrigger, Progress.None, token);
const actionsAtMarker = this.disposables.add(await getCodeActions(this._registry, model, selectionAsPosition, newCodeActionTrigger, Progress.None, token));

if (actionsAtMarker.validActions.length !== 0) {
for (const action of actionsAtMarker.validActions) {
Expand Down Expand Up @@ -315,8 +317,8 @@ export class CodeActionModel extends Disposable {
}
}
}
// temporarilly hiding here as this is enabled/disabled behind a setting.
return getCodeActions(this._registry, model, trigger.selection, trigger.trigger, Progress.None, token);
const codeActionSet = this.disposables.add(await getCodeActions(this._registry, model, trigger.selection, trigger.trigger, Progress.None, token));
return codeActionSet;
});
if (trigger.trigger.type === CodeActionTriggerType.Invoke) {
this._progressService?.showWhile(actions, 250);
Expand All @@ -337,6 +339,10 @@ export class CodeActionModel extends Disposable {
}
}, undefined);
this._codeActionOracle.value.trigger({ type: CodeActionTriggerType.Auto, triggerAction: CodeActionTriggerSource.Default });

// once we trigger, remove any disposables added.
this.disposables.dispose();
this.disposables.clear();
} else {
this._supportedCodeActions.reset();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@

import * as assert from 'assert';
import { promiseWithResolvers } from 'vs/base/common/async';
import { DisposableStore } from 'vs/base/common/lifecycle';
import { assertType } from 'vs/base/common/types';
import { URI } from 'vs/base/common/uri';
import { runWithFakedTimers } from 'vs/base/test/common/timeTravelScheduler';
import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils';
import { ICodeEditor } from 'vs/editor/browser/editorBrowser';
import { LanguageFeatureRegistry } from 'vs/editor/common/languageFeatureRegistry';
import * as languages from 'vs/editor/common/languages';
Expand Down Expand Up @@ -38,19 +38,18 @@ suite('CodeActionModel', () => {
let markerService: MarkerService;
let editor: ICodeEditor;
let registry: LanguageFeatureRegistry<languages.CodeActionProvider>;
const disposables = new DisposableStore();

setup(() => {
disposables.clear();
markerService = new MarkerService();
model = createTextModel('foobar foo bar\nfarboo far boo', languageId, undefined, uri);
editor = createTestCodeEditor(model);
editor.setPosition({ lineNumber: 1, column: 1 });
registry = new LanguageFeatureRegistry();
});

const store = ensureNoDisposablesAreLeakedInTestSuite();

teardown(() => {
disposables.clear();
editor.dispose();
model.dispose();
markerService.dispose();
Expand All @@ -61,11 +60,11 @@ suite('CodeActionModel', () => {

await runWithFakedTimers({ useFakeTimers: true }, () => {
const reg = registry.register(languageId, testProvider);
disposables.add(reg);
store.add(reg);

const contextKeys = new MockContextKeyService();
const model = disposables.add(new CodeActionModel(editor, registry, markerService, contextKeys, undefined));
disposables.add(model.onDidChangeState((e: CodeActionsState.State) => {
const model = store.add(new CodeActionModel(editor, registry, markerService, contextKeys, undefined));
store.add(model.onDidChangeState((e: CodeActionsState.State) => {
assertType(e.type === CodeActionsState.Type.Triggered);

assert.strictEqual(e.trigger.type, languages.CodeActionTriggerType.Auto);
Expand Down Expand Up @@ -93,7 +92,7 @@ suite('CodeActionModel', () => {
test('Oracle -> position changed', async () => {
await runWithFakedTimers({ useFakeTimers: true }, () => {
const reg = registry.register(languageId, testProvider);
disposables.add(reg);
store.add(reg);

markerService.changeOne('fake', uri, [{
startLineNumber: 1, startColumn: 1, endLineNumber: 1, endColumn: 6,
Expand All @@ -107,8 +106,8 @@ suite('CodeActionModel', () => {

return new Promise((resolve, reject) => {
const contextKeys = new MockContextKeyService();
const model = disposables.add(new CodeActionModel(editor, registry, markerService, contextKeys, undefined));
disposables.add(model.onDidChangeState((e: CodeActionsState.State) => {
const model = store.add(new CodeActionModel(editor, registry, markerService, contextKeys, undefined));
store.add(model.onDidChangeState((e: CodeActionsState.State) => {
assertType(e.type === CodeActionsState.Type.Triggered);

assert.strictEqual(e.trigger.type, languages.CodeActionTriggerType.Auto);
Expand All @@ -129,12 +128,12 @@ suite('CodeActionModel', () => {
const { promise: donePromise, resolve: done } = promiseWithResolvers<void>();
await runWithFakedTimers({ useFakeTimers: true }, () => {
const reg = registry.register(languageId, testProvider);
disposables.add(reg);
store.add(reg);

let triggerCount = 0;
const contextKeys = new MockContextKeyService();
const model = disposables.add(new CodeActionModel(editor, registry, markerService, contextKeys, undefined));
disposables.add(model.onDidChangeState((e: CodeActionsState.State) => {
const model = store.add(new CodeActionModel(editor, registry, markerService, contextKeys, undefined));
store.add(model.onDidChangeState((e: CodeActionsState.State) => {
assertType(e.type === CodeActionsState.Type.Triggered);

assert.strictEqual(e.trigger.type, languages.CodeActionTriggerType.Auto);
Expand Down
Loading