diff --git a/eslint.config.mjs b/eslint.config.mjs index 195ca4293e4..9fd6276e584 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -155,6 +155,7 @@ export default tseslint.config( "local/no-keywords": "error", "local/jsdoc-format": "error", "local/js-extensions": "error", + "local/no-array-mutating-method-expressions": "error", }, }, { diff --git a/package-lock.json b/package-lock.json index eb0db3e839a..baeb2565e5f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -27,6 +27,7 @@ "@types/source-map-support": "^0.5.10", "@types/which": "^3.0.4", "@typescript-eslint/rule-tester": "^8.1.0", + "@typescript-eslint/type-utils": "^8.1.0", "@typescript-eslint/utils": "^8.1.0", "azure-devops-node-api": "^14.0.2", "c8": "^10.1.2", diff --git a/package.json b/package.json index 05e08606a75..2528f6b59b3 100644 --- a/package.json +++ b/package.json @@ -53,6 +53,7 @@ "@types/source-map-support": "^0.5.10", "@types/which": "^3.0.4", "@typescript-eslint/rule-tester": "^8.1.0", + "@typescript-eslint/type-utils": "^8.1.0", "@typescript-eslint/utils": "^8.1.0", "azure-devops-node-api": "^14.0.2", "c8": "^10.1.2", diff --git a/scripts/eslint/rules/no-array-mutating-method-expressions.cjs b/scripts/eslint/rules/no-array-mutating-method-expressions.cjs new file mode 100644 index 00000000000..b085933953b --- /dev/null +++ b/scripts/eslint/rules/no-array-mutating-method-expressions.cjs @@ -0,0 +1,126 @@ +const { ESLintUtils } = require("@typescript-eslint/utils"); +const { createRule } = require("./utils.cjs"); +const { getConstrainedTypeAtLocation, isTypeArrayTypeOrUnionOfArrayTypes } = require("@typescript-eslint/type-utils"); + +/** + * @import { TSESTree } from "@typescript-eslint/utils" + */ +void 0; + +module.exports = createRule({ + name: "no-array-mutating-method-expressions", + meta: { + docs: { + description: ``, + }, + messages: { + noSideEffectUse: `This call to {{method}} appears to be unintentional as it appears in an expression position. Sort the array in a separate statement or explicitly copy the array with slice.`, + noSideEffectUseToMethod: `This call to {{method}} appears to be unintentional as it appears in an expression position. Sort the array in a separate statement or explicitly copy and slice the array with slice/{{toMethod}}.`, + }, + schema: [], + type: "problem", + }, + defaultOptions: [], + + create(context) { + const services = ESLintUtils.getParserServices(context, /*allowWithoutFullTypeInformation*/ true); + if (!services.program) { + return {}; + } + + const checker = services.program.getTypeChecker(); + + /** + * This is a heuristic to ignore cases where the mutating method appears to be + * operating on a "fresh" array. + * + * @type {(callee: TSESTree.MemberExpression) => boolean} + */ + const isFreshArray = callee => { + const object = callee.object; + + if (object.type === "ArrayExpression") { + return true; + } + + if (object.type !== "CallExpression") { + return false; + } + + if (object.callee.type === "Identifier") { + // TypeScript codebase specific helpers. + // TODO(jakebailey): handle ts. + switch (object.callee.name) { + case "arrayFrom": + case "getOwnKeys": + return true; + } + return false; + } + + if (object.callee.type === "MemberExpression" && object.callee.property.type === "Identifier") { + switch (object.callee.property.name) { + case "concat": + case "filter": + case "map": + case "slice": + return true; + } + + if (object.callee.object.type === "Identifier") { + if (object.callee.object.name === "Array") { + switch (object.callee.property.name) { + case "from": + case "of": + return true; + } + return false; + } + + if (object.callee.object.name === "Object") { + switch (object.callee.property.name) { + case "values": + case "keys": + case "entries": + return true; + } + return false; + } + } + } + + return false; + }; + + /** @type {(callee: TSESTree.MemberExpression & { parent: TSESTree.CallExpression; }, method: string, toMethod: string | undefined) => void} */ + const check = (callee, method, toMethod) => { + if (callee.parent.parent.type === "ExpressionStatement") return; + if (isFreshArray(callee)) return; + + const calleeObjType = getConstrainedTypeAtLocation(services, callee.object); + if (!isTypeArrayTypeOrUnionOfArrayTypes(calleeObjType, checker)) return; + + if (toMethod) { + context.report({ node: callee.property, messageId: "noSideEffectUseToMethod", data: { method, toMethod } }); + } + else { + context.report({ node: callee.property, messageId: "noSideEffectUse", data: { method } }); + } + }; + + // Methods with new copying variants. + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array#copying_methods_and_mutating_methods + const mutatingMethods = { + reverse: undefined, + sort: "toSorted", // This exists as `ts.toSorted`, so recommend that. + splice: undefined, + }; + + return Object.fromEntries( + Object.entries(mutatingMethods).map(([method, toMethod]) => [ + `CallExpression > MemberExpression[property.name='${method}'][computed=false]`, + node => check(node, method, toMethod), + ]), + ); + }, +}); diff --git a/src/harness/fourslashImpl.ts b/src/harness/fourslashImpl.ts index 04beb984ae5..33e0f84a744 100644 --- a/src/harness/fourslashImpl.ts +++ b/src/harness/fourslashImpl.ts @@ -963,7 +963,7 @@ export class TestState { const fileName = this.activeFile.fileName; const hints = this.languageService.provideInlayHints(fileName, span, preferences); - const annotations = ts.map(hints.sort(sortHints), hint => { + const annotations = ts.map(hints.slice().sort(sortHints), hint => { if (hint.displayParts) { hint.displayParts = ts.map(hint.displayParts, part => { if (part.file && /lib.*\.d\.ts$/.test(part.file)) { @@ -3257,8 +3257,8 @@ export class TestState { allSpanInsets.push({ text: "|]", pos: span.textSpan.start + span.textSpan.length }); }); - const reverseSpans = allSpanInsets.sort((l, r) => r.pos - l.pos); - ts.forEach(reverseSpans, span => { + allSpanInsets.sort((l, r) => r.pos - l.pos); + ts.forEach(allSpanInsets, span => { annotated = annotated.slice(0, span.pos) + span.text + annotated.slice(span.pos); }); Harness.IO.log(`\nMockup:\n${annotated}`); @@ -3783,7 +3783,7 @@ export class TestState { return { baselineContent: baselineContent + activeFile.content + `\n\n--No linked edits found--`, offset }; } - let inlineLinkedEditBaselines: { start: number; end: number; index: number; }[] = []; + const inlineLinkedEditBaselines: { start: number; end: number; index: number; }[] = []; let linkedEditInfoBaseline = ""; for (const edit of linkedEditsByRange) { const [linkedEdit, positions] = edit; @@ -3802,7 +3802,7 @@ export class TestState { offset++; } - inlineLinkedEditBaselines = inlineLinkedEditBaselines.sort((a, b) => a.start - b.start); + inlineLinkedEditBaselines.sort((a, b) => a.start - b.start); const fileText = activeFile.content; baselineContent += fileText.slice(0, inlineLinkedEditBaselines[0].start); for (let i = 0; i < inlineLinkedEditBaselines.length; i++) { @@ -4058,7 +4058,7 @@ export class TestState { public verifyRefactorKindsAvailable(kind: string, expected: string[], preferences = ts.emptyOptions) { const refactors = this.getApplicableRefactorsAtSelection("invoked", kind, preferences); const availableKinds = ts.flatMap(refactors, refactor => refactor.actions).map(action => action.kind); - assert.deepEqual(availableKinds.sort(), expected.sort(), `Expected kinds to be equal`); + assert.deepEqual(availableKinds.slice().sort(), expected.slice().sort(), `Expected kinds to be equal`); } public verifyRefactorsAvailable(names: readonly string[]): void { @@ -4938,7 +4938,7 @@ function parseFileContent(content: string, fileName: string, markerMap: Map a.pos < b.pos ? -1 : a.pos === b.pos && a.end > b.end ? -1 : 1); + localRanges.sort((a, b) => a.pos < b.pos ? -1 : a.pos === b.pos && a.end > b.end ? -1 : 1); localRanges.forEach(r => ranges.push(r)); return { diff --git a/src/harness/projectServiceStateLogger.ts b/src/harness/projectServiceStateLogger.ts index 599769b8e73..42f64fde7f3 100644 --- a/src/harness/projectServiceStateLogger.ts +++ b/src/harness/projectServiceStateLogger.ts @@ -9,6 +9,7 @@ import { isString, noop, SourceMapper, + toSorted, } from "./_namespaces/ts.js"; import { AutoImportProviderProject, @@ -93,7 +94,7 @@ export function patchServiceForStateBaseline(service: ProjectService) { function sendLogsToLogger(title: string, logs: StateItemLog[] | undefined) { if (!logs) return; logger.log(title); - logs.sort((a, b) => compareStringsCaseSensitive(a[0], b[0])) + toSorted(logs, (a, b) => compareStringsCaseSensitive(a[0], b[0])) .forEach(([title, propertyLogs]) => { logger.log(title); propertyLogs.forEach(p => isString(p) ? logger.log(p) : p.forEach(s => logger.log(s))); diff --git a/src/services/mapCode.ts b/src/services/mapCode.ts index b87ea5e7530..11d9fb5acf2 100644 --- a/src/services/mapCode.ts +++ b/src/services/mapCode.ts @@ -113,11 +113,12 @@ function parse(sourceFile: SourceFile, content: string): NodeArray { } } // Heuristic: fewer errors = more likely to be the right kind. - const { body } = parsedNodes.sort( + parsedNodes.sort( (a, b) => a.sourceFile.parseDiagnostics.length - b.sourceFile.parseDiagnostics.length, - )[0]; + ); + const { body } = parsedNodes[0]; return body; } diff --git a/src/services/navigateTo.ts b/src/services/navigateTo.ts index cb75e147811..e2858e48fed 100644 --- a/src/services/navigateTo.ts +++ b/src/services/navigateTo.ts @@ -164,7 +164,8 @@ function getContainers(declaration: Declaration): readonly string[] { container = getContainerNode(container); } - return containers.reverse(); + containers.reverse(); + return containers; } function compareNavigateToItems(i1: RawNavigateToItem, i2: RawNavigateToItem) { diff --git a/src/services/outliningElementsCollector.ts b/src/services/outliningElementsCollector.ts index 0254620c1e6..7248133ec8c 100644 --- a/src/services/outliningElementsCollector.ts +++ b/src/services/outliningElementsCollector.ts @@ -60,7 +60,8 @@ export function collectElements(sourceFile: SourceFile, cancellationToken: Cance const res: OutliningSpan[] = []; addNodeOutliningSpans(sourceFile, cancellationToken, res); addRegionOutliningSpans(sourceFile, res); - return res.sort((span1, span2) => span1.textSpan.start - span2.textSpan.start); + res.sort((span1, span2) => span1.textSpan.start - span2.textSpan.start); + return res; } function addNodeOutliningSpans(sourceFile: SourceFile, cancellationToken: CancellationToken, out: OutliningSpan[]): void { diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 4093074b957..423922f6913 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -1081,11 +1081,11 @@ function extractFunctionInScope( }); const typeParametersAndDeclarations = arrayFrom(typeParameterUsages.values(), type => ({ type, declaration: getFirstDeclarationBeforePosition(type, context.startPosition) })); - const sortedTypeParametersAndDeclarations = typeParametersAndDeclarations.sort(compareTypesByDeclarationOrder); + typeParametersAndDeclarations.sort(compareTypesByDeclarationOrder); - const typeParameters: readonly TypeParameterDeclaration[] | undefined = sortedTypeParametersAndDeclarations.length === 0 + const typeParameters: readonly TypeParameterDeclaration[] | undefined = typeParametersAndDeclarations.length === 0 ? undefined - : mapDefined(sortedTypeParametersAndDeclarations, ({ declaration }) => declaration as TypeParameterDeclaration); + : mapDefined(typeParametersAndDeclarations, ({ declaration }) => declaration as TypeParameterDeclaration); // Strictly speaking, we should check whether each name actually binds to the appropriate type // parameter. In cases of shadowing, they may not. diff --git a/src/services/suggestionDiagnostics.ts b/src/services/suggestionDiagnostics.ts index a5daf3e6666..3c4cf5c39b9 100644 --- a/src/services/suggestionDiagnostics.ts +++ b/src/services/suggestionDiagnostics.ts @@ -97,7 +97,8 @@ export function computeSuggestionDiagnostics(sourceFile: SourceFile, program: Pr addRange(diags, sourceFile.bindSuggestionDiagnostics); addRange(diags, program.getSuggestionDiagnostics(sourceFile, cancellationToken)); - return diags.sort((d1, d2) => d1.start - d2.start); + diags.sort((d1, d2) => d1.start - d2.start); + return diags; function check(node: Node) { if (isJsFile) { diff --git a/src/testRunner/unittests/services/extract/ranges.ts b/src/testRunner/unittests/services/extract/ranges.ts index 9fd9bee8beb..c54760476dd 100644 --- a/src/testRunner/unittests/services/extract/ranges.ts +++ b/src/testRunner/unittests/services/extract/ranges.ts @@ -1,7 +1,7 @@ import * as ts from "../../../_namespaces/ts.js"; import { extractTest } from "./helpers.js"; -function testExtractRangeFailed(caption: string, s: string, expectedErrors: string[]) { +function testExtractRangeFailed(caption: string, s: string, expectedErrors: readonly string[]) { return it(caption, () => { const t = extractTest(s); const file = ts.createSourceFile("a.ts", t.source, ts.ScriptTarget.Latest, /*setParentNodes*/ true); @@ -12,7 +12,7 @@ function testExtractRangeFailed(caption: string, s: string, expectedErrors: stri const result = ts.refactor.extractSymbol.getRangeToExtract(file, ts.createTextSpanFromRange(selectionRange), /*invoked*/ false); assert(result.targetRange === undefined, "failure expected"); const sortedErrors = result.errors.map(e => e.messageText as string).sort(); - assert.deepEqual(sortedErrors, expectedErrors.sort(), "unexpected errors"); + assert.deepEqual(sortedErrors, expectedErrors.slice().sort(), "unexpected errors"); }); }