-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Add custom eslint rule 'no-array-mutating-method-expressions' #59526
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
Add custom eslint rule 'no-array-mutating-method-expressions' #59526
Conversation
We could instead add an earlier overload via declaration merging to return |
I'm not totally sure how we'd add an overload before the lib.d.ts, but I guess it must be possible because ts-reset or whatever can do it? But, that wouldn't allow us to ignore the calls that happen after we're sure the array has been cloned (which is what this PR does); we don't have that many calls to sort but most of them are actually ignored by this rule. |
Overloads from declaration merging to in "before" the overloads they merge with |
Planning on going through the list of mutating methods from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array#copying_methods_and_mutating_methods and adding them here too; same kind of bug. |
5ed0892
to
eb5e5ab
Compare
@@ -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])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug.
@@ -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`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug, more or less.
@@ -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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug.
Motivated by the bug found in #59523.
Needs tests.