From 5bf577b08ae0c1a158b249569decc41e844a57f2 Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Fri, 8 Dec 2023 13:45:44 +0100 Subject: [PATCH] make sure `super.dispose` is called inside overridden dispose methods (#200342) --- .eslintplugin/code-must-use-super-dispose.ts | 33 +++++++++++++++++++ .eslintrc.json | 2 ++ .../ui/breadcrumbs/breadcrumbsWidget.ts | 2 +- .../ipc/electron-sandbox/ipc.electron.ts | 1 + .../editor/browser/controller/mouseHandler.ts | 1 + src/vs/platform/log/common/bufferLog.ts | 1 + src/vs/platform/log/common/log.ts | 12 +------ src/vs/platform/log/node/spdlogLog.ts | 1 + .../common/userDataSyncServiceIpc.ts | 1 + src/vs/server/node/serverServices.ts | 4 --- .../api/common/extHostDocumentData.ts | 1 + .../parts/editor/breadcrumbsControl.ts | 7 ++-- .../bulkEdit/browser/preview/bulkEditPane.ts | 1 + .../debug/browser/debugActionViewItems.ts | 1 + .../test/browser/testNotebookEditor.ts | 1 + .../activity/browser/activityService.ts | 1 + .../services/extensions/common/rpcProtocol.ts | 2 ++ 17 files changed, 54 insertions(+), 18 deletions(-) create mode 100644 .eslintplugin/code-must-use-super-dispose.ts diff --git a/.eslintplugin/code-must-use-super-dispose.ts b/.eslintplugin/code-must-use-super-dispose.ts new file mode 100644 index 00000000000..4f7f964699f --- /dev/null +++ b/.eslintplugin/code-must-use-super-dispose.ts @@ -0,0 +1,33 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import * as eslint from 'eslint'; + +export = new class NoAsyncSuite implements eslint.Rule.RuleModule { + + create(context: eslint.Rule.RuleContext): eslint.Rule.RuleListener { + function doesCallSuperDispose(node: any) { + + if (!node.override) { + return; + } + + const body = context.getSourceCode().getText(node) + + if (body.includes('super.dispose')) { + return; + } + + context.report({ + node, + message: 'dispose() should call super.dispose()' + }); + } + + return { + ['MethodDefinition[override][key.name="dispose"]']: doesCallSuperDispose, + }; + } +}; diff --git a/.eslintrc.json b/.eslintrc.json index f4018175af0..2f93e738fd2 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -73,6 +73,7 @@ "local/code-no-nls-in-standalone-editor": "warn", "local/code-no-standalone-editor": "warn", "local/code-no-unexternalized-strings": "warn", + "local/code-must-use-super-dispose": "warn", "local/code-declare-service-brand": "warn", "local/code-layering": [ "warn", @@ -119,6 +120,7 @@ "**/*.test.ts" ], "rules": { + "local/code-must-use-super-dispose": "off", "local/code-no-test-only": "error", "local/code-no-test-async-suite": "warn", "local/code-no-unexternalized-strings": "off", diff --git a/src/vs/base/browser/ui/breadcrumbs/breadcrumbsWidget.ts b/src/vs/base/browser/ui/breadcrumbs/breadcrumbsWidget.ts index 7e130b71d09..7094cc9c41e 100644 --- a/src/vs/base/browser/ui/breadcrumbs/breadcrumbsWidget.ts +++ b/src/vs/base/browser/ui/breadcrumbs/breadcrumbsWidget.ts @@ -14,7 +14,7 @@ import { ScrollbarVisibility } from 'vs/base/common/scrollable'; import 'vs/css!./breadcrumbsWidget'; export abstract class BreadcrumbsItem { - dispose(): void { } + abstract dispose(): void; abstract equals(other: BreadcrumbsItem): boolean; abstract render(container: HTMLElement): void; } diff --git a/src/vs/base/parts/ipc/electron-sandbox/ipc.electron.ts b/src/vs/base/parts/ipc/electron-sandbox/ipc.electron.ts index a394bf8a874..9b95f2b4c63 100644 --- a/src/vs/base/parts/ipc/electron-sandbox/ipc.electron.ts +++ b/src/vs/base/parts/ipc/electron-sandbox/ipc.electron.ts @@ -34,5 +34,6 @@ export class Client extends IPCClient implements IDisposable { override dispose(): void { this.protocol.disconnect(); + super.dispose(); } } diff --git a/src/vs/editor/browser/controller/mouseHandler.ts b/src/vs/editor/browser/controller/mouseHandler.ts index e4551a39cbe..55d58d6eef9 100644 --- a/src/vs/editor/browser/controller/mouseHandler.ts +++ b/src/vs/editor/browser/controller/mouseHandler.ts @@ -687,6 +687,7 @@ class TopBottomDragScrollingOperation extends Disposable { public override dispose(): void { this._animationFrameDisposable.dispose(); + super.dispose(); } public setPosition(position: IMouseTargetOutsideEditor, mouseEvent: EditorMouseEvent): void { diff --git a/src/vs/platform/log/common/bufferLog.ts b/src/vs/platform/log/common/bufferLog.ts index 5d8f31089fb..0eb3660744e 100644 --- a/src/vs/platform/log/common/bufferLog.ts +++ b/src/vs/platform/log/common/bufferLog.ts @@ -44,6 +44,7 @@ export class BufferLogger extends AbstractMessageLogger { override dispose(): void { this._logger?.dispose(); + super.dispose(); } override flush(): void { diff --git a/src/vs/platform/log/common/log.ts b/src/vs/platform/log/common/log.ts index 3a0a4b2a7e9..e2d69c0fe30 100644 --- a/src/vs/platform/log/common/log.ts +++ b/src/vs/platform/log/common/log.ts @@ -378,10 +378,6 @@ export class ConsoleMainLogger extends AbstractLogger implements ILogger { } } - override dispose(): void { - // noop - } - flush(): void { // noop } @@ -445,9 +441,6 @@ export class ConsoleLogger extends AbstractLogger implements ILogger { } } - override dispose(): void { - // noop - } flush(): void { // noop @@ -499,10 +492,6 @@ export class AdapterLogger extends AbstractLogger implements ILogger { return toErrorMessage(msg, this.checkLogLevel(LogLevel.Trace)); } - override dispose(): void { - // noop - } - flush(): void { // noop } @@ -564,6 +553,7 @@ export class MultiplexLogger extends AbstractLogger implements ILogger { for (const logger of this.loggers) { logger.dispose(); } + super.dispose(); } } diff --git a/src/vs/platform/log/node/spdlogLog.ts b/src/vs/platform/log/node/spdlogLog.ts index 11f078f941e..5c38b748e1c 100644 --- a/src/vs/platform/log/node/spdlogLog.ts +++ b/src/vs/platform/log/node/spdlogLog.ts @@ -123,6 +123,7 @@ export class SpdLogLogger extends AbstractMessageLogger implements ILogger { } else { this._loggerCreationPromise.then(() => this.disposeLogger()); } + super.dispose(); } private disposeLogger(): void { diff --git a/src/vs/platform/userDataSync/common/userDataSyncServiceIpc.ts b/src/vs/platform/userDataSync/common/userDataSyncServiceIpc.ts index 94e3fdd0028..85ac679bfa2 100644 --- a/src/vs/platform/userDataSync/common/userDataSyncServiceIpc.ts +++ b/src/vs/platform/userDataSync/common/userDataSyncServiceIpc.ts @@ -291,6 +291,7 @@ class ManualSyncTaskChannelClient extends Disposable implements IUserDataManualS override dispose(): void { this.channel.call('dispose'); + super.dispose(); } } diff --git a/src/vs/server/node/serverServices.ts b/src/vs/server/node/serverServices.ts index 019b7d3768a..6c7a0d7901d 100644 --- a/src/vs/server/node/serverServices.ts +++ b/src/vs/server/node/serverServices.ts @@ -320,10 +320,6 @@ class ServerLogger extends AbstractLogger { } } - override dispose(): void { - // noop - } - flush(): void { // noop } diff --git a/src/vs/workbench/api/common/extHostDocumentData.ts b/src/vs/workbench/api/common/extHostDocumentData.ts index b72dcdf442b..139f826be43 100644 --- a/src/vs/workbench/api/common/extHostDocumentData.ts +++ b/src/vs/workbench/api/common/extHostDocumentData.ts @@ -42,6 +42,7 @@ export class ExtHostDocumentData extends MirrorTextModel { super(uri, lines, eol, versionId); } + // eslint-disable-next-line local/code-must-use-super-dispose override dispose(): void { // we don't really dispose documents but let // extensions still read from them. some diff --git a/src/vs/workbench/browser/parts/editor/breadcrumbsControl.ts b/src/vs/workbench/browser/parts/editor/breadcrumbsControl.ts index cf307fec7e5..52f7c6d6776 100644 --- a/src/vs/workbench/browser/parts/editor/breadcrumbsControl.ts +++ b/src/vs/workbench/browser/parts/editor/breadcrumbsControl.ts @@ -53,7 +53,7 @@ class OutlineItem extends BreadcrumbsItem { super(); } - override dispose(): void { + dispose(): void { this._disposables.dispose(); } @@ -113,7 +113,7 @@ class FileItem extends BreadcrumbsItem { super(); } - override dispose(): void { + dispose(): void { this._disposables.dispose(); } @@ -331,6 +331,9 @@ export class BreadcrumbsControl { equals(other: BreadcrumbsItem): boolean { return other === this; } + dispose(): void { + + } }]); } else { this._widget.setEnabled(true); diff --git a/src/vs/workbench/contrib/bulkEdit/browser/preview/bulkEditPane.ts b/src/vs/workbench/contrib/bulkEdit/browser/preview/bulkEditPane.ts index 1b07c881fb6..4a18e62f256 100644 --- a/src/vs/workbench/contrib/bulkEdit/browser/preview/bulkEditPane.ts +++ b/src/vs/workbench/contrib/bulkEdit/browser/preview/bulkEditPane.ts @@ -102,6 +102,7 @@ export class BulkEditPane extends ViewPane { override dispose(): void { this._tree.dispose(); this._disposables.dispose(); + super.dispose(); } protected override renderBody(parent: HTMLElement): void { diff --git a/src/vs/workbench/contrib/debug/browser/debugActionViewItems.ts b/src/vs/workbench/contrib/debug/browser/debugActionViewItems.ts index 62bbae73504..525ebae3e80 100644 --- a/src/vs/workbench/contrib/debug/browser/debugActionViewItems.ts +++ b/src/vs/workbench/contrib/debug/browser/debugActionViewItems.ts @@ -174,6 +174,7 @@ export class StartDebugActionViewItem extends BaseActionViewItem { override dispose(): void { this.toDispose = dispose(this.toDispose); + super.dispose(); } private updateOptions(): void { diff --git a/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts b/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts index a4c615cd48e..74d22fc7bf2 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts @@ -222,6 +222,7 @@ function _createTestNotebookEditor(instantiationService: TestInstantiationServic let visibleRanges: ICellRange[] = [{ start: 0, end: 100 }]; const notebookEditor: IActiveNotebookEditorDelegate = new class extends mock() { + // eslint-disable-next-line local/code-must-use-super-dispose override dispose() { viewModel.dispose(); } diff --git a/src/vs/workbench/services/activity/browser/activityService.ts b/src/vs/workbench/services/activity/browser/activityService.ts index 962af523c69..ba6b8177f87 100644 --- a/src/vs/workbench/services/activity/browser/activityService.ts +++ b/src/vs/workbench/services/activity/browser/activityService.ts @@ -47,6 +47,7 @@ class ViewContainerActivityByView extends Disposable { override dispose() { this.activityDisposable.dispose(); + super.dispose(); } } diff --git a/src/vs/workbench/services/extensions/common/rpcProtocol.ts b/src/vs/workbench/services/extensions/common/rpcProtocol.ts index ddd44633910..c7fbf71b0b7 100644 --- a/src/vs/workbench/services/extensions/common/rpcProtocol.ts +++ b/src/vs/workbench/services/extensions/common/rpcProtocol.ts @@ -170,6 +170,8 @@ export class RPCProtocol extends Disposable implements IRPCProtocol { delete this._pendingRPCReplies[msgId]; pending.resolveErr(errors.canceled()); }); + + super.dispose(); } public drain(): Promise {