Rule proposal: `no-unnecessary-splice`

[burtek]

Description

Saw splice being used instead of push/pop/shift/unshift in some projects. AFAIK it brings no performance gain and is harder to read/understand than the latter ones.

Basically bans:

• array.splice(index, 0) (can be removed as no-op)
• array.splice(0, 1) (as substitutable by array.shift())
• array.splice(0, 0, element) (as substitutable by array.unshift(element) - for 1 or more elements)
• array.splice(array.length - 1, 1) (as substitutable by array.pop())
• array.splice(array.length, 0, element) (as substitutable by array.push(element) - for 1 or more elements)

Should be easy to implement auto-fixer for those replacements.

Fail

array.splice(start, 0)

array.splice(0, 1)

array.splice(array.length, 0, element)

Pass

array.shift()

array.push(element)

Proposed rule name

no-unnecessary-splice

Additional Info

Happy to work on this once accepted (already have a stub).

This would make #2165 not needed any more

[fisker]

Related https://github.com/sindresorhus/eslint-plugin-unicorn/issues/2165

[fisker]

Fail

array.splice(index, 1, newValue);

Pass

array[index] = newValue;

array = array.with(index, newValue);

[burtek]

@fisker good one, but I'd prefer to have that configurable, as it depends on the targeted ECMA version if array.with is supported

EDIT: actually, this doesn't look like that good of a replacement. splice mutates the array and returns old values, same for push/pop/shift/unshift, but neither array[index] = newValue or array.with does it, so it changes the logic, especially since array can't be const for the array.with solution.

Will leave this one for the end

[burtek]

One thing to watch out for is whether splice's return value is used or not.

Another thing is for all calls like getArray().splice(4, 0) (no-op case), we cant just remove the whole thing, as we'd be removing call to getArray which might introduce bugs. A lot to consider for auto-fixer. Again, have some stub for this already, working on more

[sindresorhus]

Accepted

[burtek]

#2361 is WIP, will try finishing it in the coming week.

[fregante]

Also to empty arrays:

array.splice(0, array.length); // ❌
array.splice(0); // ❌
array.length = 0; // ✅

or to limit them, maybe

MAX_LENGTH = 5;
array.splice(MAX_LENGTH, array.length); // ❌
array.splice(MAX_LENGTH); // ❌
array.length = Math.min(array.length, MAX_LENGTH); // ✅

and clone+empty them:

// ❌
const clone = array.splice(0);

// ✅ 😥
const clone = [...array];
array.length = 0;

But honestly I prefer array.splice(0) in that case. cc @fisker

[fisker]

I changed my mind, let's delete the "clone+empty" case?