Add custom eslint rule 'no-array-mutating-method-expressions' (microsoft#59526)

Author: jakebailey

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",
         },
     },
     {

package-lock.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",

package.json

@@ -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),
+            ]),
+        );
+    },
+});

scripts/eslint/rules/no-array-mutating-method-expressions.cjs

@@ -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<stri
     const openRanges: RangeLocationInformation[] = [];
 
     /// A list of ranges we've collected so far */
-    let localRanges: Range[] = [];
+    const localRanges: Range[] = [];
 
     /// The latest position of the start of an unflushed plain text area
     let lastNormalCharPosition = 0;
@@ -5105,7 +5105,7 @@ function parseFileContent(content: string, fileName: string, markerMap: Map<stri
     }
 
     // put ranges in the correct order
-    localRanges = localRanges.sort((a, b) => 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 {

src/harness/fourslashImpl.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)));

src/harness/projectServiceStateLogger.ts

@@ -113,11 +113,12 @@ function parse(sourceFile: SourceFile, content: string): NodeArray<Node> {
         }
     }
     // 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;
 }
 

src/services/mapCode.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) {

src/services/navigateTo.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 {

src/services/outliningElementsCollector.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.

src/services/refactors/extractSymbol.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) {

src/services/suggestionDiagnostics.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");
     });
 }