diff --git a/.eslint-plugin-local/code-no-redundant-has-before-delete.ts b/.eslint-plugin-local/code-no-redundant-has-before-delete.ts new file mode 100644 index 00000000000..a13671f4217 --- /dev/null +++ b/.eslint-plugin-local/code-no-redundant-has-before-delete.ts @@ -0,0 +1,140 @@ +/*--------------------------------------------------------------------------------------------- + * 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'; +import type * as ESTree from 'estree'; +import { TSESTree } from '@typescript-eslint/utils'; + +export default new class NoRedundantHasBeforeDelete implements eslint.Rule.RuleModule { + + readonly meta: eslint.Rule.RuleMetaData = { + messages: { + noRedundantHasBeforeDelete: 'Do not check for existence before deleting. Map.delete/Set.delete returns a boolean indicating if the element was present.', + }, + fixable: 'code', + schema: false, + }; + + create(context: eslint.Rule.RuleContext): eslint.Rule.RuleListener { + + return { + IfStatement(node: any) { + const ifStatement = node as TSESTree.IfStatement; + const test = ifStatement.test; + const consequent = ifStatement.consequent; + const hasElse = ifStatement.alternate !== null && ifStatement.alternate !== undefined; + + // Check if the test is a .has() call + if (test.type !== 'CallExpression' || + test.callee.type !== 'MemberExpression' || + test.callee.property.type !== 'Identifier' || + test.callee.property.name !== 'has' || + test.arguments.length !== 1) { + return; + } + + const hasCall = test; + const hasCollection = hasCall.callee.object; + const hasKey = hasCall.arguments[0]; + + // Get the first statement from the consequent + let deleteStatement: TSESTree.ExpressionStatement | undefined; + if (consequent.type === 'BlockStatement') { + if (consequent.body.length === 0) { + return; + } + const firstStatement = consequent.body[0]; + if (firstStatement.type !== 'ExpressionStatement') { + return; + } + deleteStatement = firstStatement; + } else if (consequent.type === 'ExpressionStatement') { + deleteStatement = consequent; + } else { + return; + } + + // Check if the first statement is a .delete() call + const expr = deleteStatement.expression; + if (expr.type !== 'CallExpression' || + expr.callee.type !== 'MemberExpression' || + expr.callee.property.type !== 'Identifier' || + expr.callee.property.name !== 'delete' || + expr.arguments.length !== 1) { + return; + } + + const deleteCall = expr; + const deleteCollection = deleteCall.callee.object; + const deleteKey = deleteCall.arguments[0]; + + // Compare collection and key using source text + const sourceCode = context.sourceCode; + const toNode = (n: TSESTree.Node) => n as unknown as ESTree.Node; + if (sourceCode.getText(toNode(hasCollection)) !== sourceCode.getText(toNode(deleteCollection)) || + sourceCode.getText(toNode(hasKey)) !== sourceCode.getText(toNode(deleteKey))) { + return; + } + + context.report({ + node: ifStatement, + messageId: 'noRedundantHasBeforeDelete', + fix(fixer) { + const deleteCallText = sourceCode.getText(toNode(deleteCall)); + const ifNode = toNode(ifStatement); + const isOnlyDelete = consequent.type === 'ExpressionStatement' || + (consequent.type === 'BlockStatement' && consequent.body.length === 1); + + // Helper to get the range including trailing whitespace + const getDeleteRangeWithWhitespace = () => { + const deleteNode = toNode(deleteStatement!); + const [start, end] = deleteNode.range!; + const nextToken = sourceCode.getTokenAfter(deleteNode); + if (nextToken && nextToken.range![0] > end) { + const textBetween = sourceCode.text.substring(end, nextToken.range![0]); + if (textBetween.trim() === '') { + return [start, nextToken.range![0]] as [number, number]; + } + } + return [start, end] as [number, number]; + }; + + // Case 1: Has else clause + if (hasElse) { + const elseText = sourceCode.getText(toNode(ifStatement.alternate!)); + + if (isOnlyDelete) { + // Only delete in consequent: negate and use else + // if (m.has(key)) m.delete(key); else ... → if (!m.delete(key)) ... + return fixer.replaceText(ifNode, `if (!${deleteCallText}) ${elseText}`); + } else { + // Multiple statements: replace test and remove delete statement + // if (m.has(key)) { m.delete(key); other(); } else ... → if (m.delete(key)) { other(); } else ... + return [ + fixer.replaceTextRange(hasCall.range!, deleteCallText), + fixer.removeRange(getDeleteRangeWithWhitespace()) + ]; + } + } + + // Case 2: No else clause + if (isOnlyDelete) { + // Replace entire if with just the delete call + // if (m.has(key)) m.delete(key); → m.delete(key); + return fixer.replaceText(ifNode, deleteCallText + ';'); + } else { + // Multiple statements: replace test and remove delete statement + // if (m.has(key)) { m.delete(key); other(); } → if (m.delete(key)) { other(); } + return [ + fixer.replaceTextRange(hasCall.range!, deleteCallText), + fixer.removeRange(getDeleteRangeWithWhitespace()) + ]; + } + } + }); + } + }; + } +}; diff --git a/.eslint-plugin-local/tests/code-no-redundant-has-before-delete-test.ts b/.eslint-plugin-local/tests/code-no-redundant-has-before-delete-test.ts new file mode 100644 index 00000000000..f87c8f87705 --- /dev/null +++ b/.eslint-plugin-local/tests/code-no-redundant-has-before-delete-test.ts @@ -0,0 +1,38 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +export function testRedundantHasBeforeDelete() { + const m = new Map(); + const s = new Set(); + + // Invalid cases + m.delete('key'); + + s.delete('key'); + + // Cases with else clause + if (!m.delete('key')) { + console.log('not found'); + } + + if (m.delete('key')) { + console.log('deleted'); + } else { + console.log('not found'); + } + + // Valid cases + m.delete('key'); + s.delete('key'); + + if (m.has('key')) { + console.log('deleting'); + m.delete('key'); + } + + if (m.has('key')) { + m.delete('otherKey'); + } +} diff --git a/eslint.config.js b/eslint.config.js index 9d83f9269e3..a2c242bcc19 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -92,6 +92,7 @@ export default tseslint.config( 'local/code-no-localized-model-description': 'warn', 'local/code-policy-localization-key-match': 'warn', 'local/code-no-localization-template-literals': 'error', + 'local/code-no-redundant-has-before-delete': 'warn', 'local/code-no-deep-import-of-internal': ['error', { '.*Internal': true, 'searchExtTypesInternal': false }], 'local/code-layering': [ 'warn', diff --git a/src/vs/base/common/lifecycle.ts b/src/vs/base/common/lifecycle.ts index f78aca05b53..2345bfd7028 100644 --- a/src/vs/base/common/lifecycle.ts +++ b/src/vs/base/common/lifecycle.ts @@ -505,8 +505,7 @@ export class DisposableStore implements IDisposable { if (!o) { return; } - if (this._toDispose.has(o)) { - this._toDispose.delete(o); + if (this._toDispose.delete(o)) { setParentOfDisposable(o, null); } } diff --git a/src/vs/base/common/paging.ts b/src/vs/base/common/paging.ts index 295600dac63..74123dd25b5 100644 --- a/src/vs/base/common/paging.ts +++ b/src/vs/base/common/paging.ts @@ -258,9 +258,7 @@ export class PageIteratorPager implements IPager { } return this.cachedPages[pageIndex]; } finally { - if (this.pendingRequests.has(pageIndex)) { - this.pendingRequests.delete(pageIndex); - } + this.pendingRequests.delete(pageIndex); } } diff --git a/src/vs/platform/files/common/inMemoryFilesystemProvider.ts b/src/vs/platform/files/common/inMemoryFilesystemProvider.ts index 54effa0ce0c..82ff1b532c7 100644 --- a/src/vs/platform/files/common/inMemoryFilesystemProvider.ts +++ b/src/vs/platform/files/common/inMemoryFilesystemProvider.ts @@ -201,8 +201,7 @@ export class InMemoryFileSystemProvider extends Disposable implements const dirname = resources.dirname(resource); const basename = resources.basename(resource); const parent = this._lookupAsDirectory(dirname, false); - if (parent.entries.has(basename)) { - parent.entries.delete(basename); + if (parent.entries.delete(basename)) { parent.mtime = Date.now(); parent.size -= 1; this._fireSoon({ type: FileChangeType.UPDATED, resource: dirname }, { resource, type: FileChangeType.DELETED }); diff --git a/src/vs/platform/lifecycle/electron-main/lifecycleMainService.ts b/src/vs/platform/lifecycle/electron-main/lifecycleMainService.ts index eed06364769..bd41c5619e1 100644 --- a/src/vs/platform/lifecycle/electron-main/lifecycleMainService.ts +++ b/src/vs/platform/lifecycle/electron-main/lifecycleMainService.ts @@ -433,9 +433,7 @@ export class LifecycleMainService extends Disposable implements ILifecycleMainSe // The window already acknowledged to be closed const windowId = window.id; - if (this.windowToCloseRequest.has(windowId)) { - this.windowToCloseRequest.delete(windowId); - + if (this.windowToCloseRequest.delete(windowId)) { return; } diff --git a/src/vs/platform/tunnel/node/sharedProcessTunnelService.ts b/src/vs/platform/tunnel/node/sharedProcessTunnelService.ts index 46500cf661b..f6cb50f5482 100644 --- a/src/vs/platform/tunnel/node/sharedProcessTunnelService.ts +++ b/src/vs/platform/tunnel/node/sharedProcessTunnelService.ts @@ -81,9 +81,8 @@ export class SharedProcessTunnelService extends Disposable implements ISharedPro throw new Error(`Could not create tunnel`); } - if (this._disposedTunnels.has(id)) { + if (this._disposedTunnels.delete(id)) { // This tunnel was disposed in the meantime - this._disposedTunnels.delete(id); tunnelData.dispose(); await tunnel.dispose(); throw canceled(); diff --git a/src/vs/workbench/api/browser/mainThreadShare.ts b/src/vs/workbench/api/browser/mainThreadShare.ts index cb63bb3fd03..115b45ce0b8 100644 --- a/src/vs/workbench/api/browser/mainThreadShare.ts +++ b/src/vs/workbench/api/browser/mainThreadShare.ts @@ -41,12 +41,8 @@ export class MainThreadShare implements MainThreadShareShape { } $unregisterShareProvider(handle: number): void { - if (this.providers.has(handle)) { - this.providers.delete(handle); - } - if (this.providerDisposables.has(handle)) { - this.providerDisposables.delete(handle); - } + this.providers.delete(handle); + this.providerDisposables.delete(handle); } dispose(): void { diff --git a/src/vs/workbench/api/common/extHostCodeInsets.ts b/src/vs/workbench/api/common/extHostCodeInsets.ts index 01cc0023b1d..707c9621c17 100644 --- a/src/vs/workbench/api/common/extHostCodeInsets.ts +++ b/src/vs/workbench/api/common/extHostCodeInsets.ts @@ -107,8 +107,7 @@ export class ExtHostEditorInsets implements ExtHostEditorInsetsShape { readonly onDidDispose: vscode.Event = onDidDispose.event; dispose(): void { - if (that._insets.has(handle)) { - that._insets.delete(handle); + if (that._insets.delete(handle)) { that._proxy.$disposeEditorInset(handle); onDidDispose.fire(); diff --git a/src/vs/workbench/api/common/extHostTesting.ts b/src/vs/workbench/api/common/extHostTesting.ts index 1cef0298f32..423b6d3b33a 100644 --- a/src/vs/workbench/api/common/extHostTesting.ts +++ b/src/vs/workbench/api/common/extHostTesting.ts @@ -1071,8 +1071,7 @@ class MirroredChangeCollector implements IncrementalChangeCollector { diff --git a/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts b/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts index f7070c78be5..30fce10b2d2 100644 --- a/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts +++ b/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts @@ -1286,16 +1286,14 @@ export abstract class AbstractTaskService extends Disposable implements ITaskSer } public removeRecentlyUsedTask(taskRecentlyUsedKey: string) { - if (this._getTasksFromStorage('historical').has(taskRecentlyUsedKey)) { - this._getTasksFromStorage('historical').delete(taskRecentlyUsedKey); + if (this._getTasksFromStorage('historical').delete(taskRecentlyUsedKey)) { this._saveRecentlyUsedTasks(); } } public removePersistentTask(key: string) { this._log(nls.localize('taskService.removePersistentTask', 'Removing persistent task {0}', key), true); - if (this._getTasksFromStorage('persistent').has(key)) { - this._getTasksFromStorage('persistent').delete(key); + if (this._getTasksFromStorage('persistent').delete(key)) { this._savePersistentTasks(); } } diff --git a/src/vs/workbench/contrib/tasks/common/problemCollectors.ts b/src/vs/workbench/contrib/tasks/common/problemCollectors.ts index dbad5612b09..13ef3e3d26a 100644 --- a/src/vs/workbench/contrib/tasks/common/problemCollectors.ts +++ b/src/vs/workbench/contrib/tasks/common/problemCollectors.ts @@ -546,8 +546,7 @@ export class WatchingProblemCollector extends AbstractProblemCollector implement } else { this._onDidRequestInvalidateLastMarker.fire(); } - if (this._activeBackgroundMatchers.has(background.key)) { - this._activeBackgroundMatchers.delete(background.key); + if (this._activeBackgroundMatchers.delete(background.key)) { this.resetCurrentResource(); this._onDidStateChange.fire(IProblemCollectorEvent.create(ProblemCollectorEventKind.BackgroundProcessingEnds)); result = true; diff --git a/src/vs/workbench/contrib/timeline/browser/timelinePane.ts b/src/vs/workbench/contrib/timeline/browser/timelinePane.ts index 79b8558ecd5..efca4183366 100644 --- a/src/vs/workbench/contrib/timeline/browser/timelinePane.ts +++ b/src/vs/workbench/contrib/timeline/browser/timelinePane.ts @@ -1362,9 +1362,7 @@ class TimelinePaneCommands extends Disposable { }); } run(accessor: ServicesAccessor, ...args: unknown[]) { - if (excluded.has(source.id)) { - excluded.delete(source.id); - } else { + if (!excluded.delete(source.id)) { excluded.add(source.id); } diff --git a/src/vs/workbench/services/authentication/browser/authenticationService.ts b/src/vs/workbench/services/authentication/browser/authenticationService.ts index c3aba1e9b86..3b4df87a6c3 100644 --- a/src/vs/workbench/services/authentication/browser/authenticationService.ts +++ b/src/vs/workbench/services/authentication/browser/authenticationService.ts @@ -239,9 +239,7 @@ export class AuthenticationService extends Disposable implements IAuthentication if (provider) { this._authenticationProviders.delete(id); // If this is a dynamic provider, remove it from the set of dynamic providers - if (this._dynamicAuthenticationProviderIds.has(id)) { - this._dynamicAuthenticationProviderIds.delete(id); - } + this._dynamicAuthenticationProviderIds.delete(id); this._onDidUnregisterAuthenticationProvider.fire({ id, label: provider.label }); } this._authenticationProviderDisposables.deleteAndDispose(id); diff --git a/src/vs/workbench/services/policies/common/accountPolicyService.ts b/src/vs/workbench/services/policies/common/accountPolicyService.ts index d81da4dffe1..2fd9117d80f 100644 --- a/src/vs/workbench/services/policies/common/accountPolicyService.ts +++ b/src/vs/workbench/services/policies/common/accountPolicyService.ts @@ -44,8 +44,7 @@ export class AccountPolicyService extends AbstractPolicyService implements IPoli updated.push(key); } } else { - if (this.policies.has(key)) { - this.policies.delete(key); + if (this.policies.delete(key)) { updated.push(key); } } diff --git a/src/vs/workbench/services/remote/common/tunnelModel.ts b/src/vs/workbench/services/remote/common/tunnelModel.ts index ea8629af1be..469d8f1b4a9 100644 --- a/src/vs/workbench/services/remote/common/tunnelModel.ts +++ b/src/vs/workbench/services/remote/common/tunnelModel.ts @@ -542,8 +542,7 @@ export class TunnelModel extends Disposable { private async onTunnelClosed(address: { host: string; port: number }, reason: TunnelCloseReason) { const key = makeAddress(address.host, address.port); - if (this.forwarded.has(key)) { - this.forwarded.delete(key); + if (this.forwarded.delete(key)) { await this.storeForwarded(); this._onClosePort.fire(address); } @@ -907,9 +906,7 @@ export class TunnelModel extends Disposable { detail: value.detail, pid: value.pid }); - if (removedCandidates.has(addressKey)) { - removedCandidates.delete(addressKey); - } + removedCandidates.delete(addressKey); const forwardedValue = mapHasAddressLocalhostOrAllInterfaces(this.forwarded, value.host, value.port); if (forwardedValue) { forwardedValue.runningProcess = value.detail;