Preventing array mutations via standard methods

[mquandalle]

Looking at the implementation of the no-mutation rule, it seems that the following code wouldn’t be caught:

const names = ['foo'];
names.push('bar');

I imagine than catching these mutations is somehow difficult as it requires some advanced type inference to understand that names is a JavaScript array and push (or concat, slice, etc.) a method that is actually mutating the object. So I’m opening the thread to discuss this problem.

[StevenLangbroek]

concat and slice are actually safe, since they create a new array.

[mquandalle]

You are right, I didn’t put enough thoughts when I wrote this parenthesis :). Mozilla Developer Network actually have a list of mutating methods on array: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/prototype#Mutator_methods

[mquandalle]

Also note that other standard objects have the same problem, for instance:

const a = new Date();
a.setHour(0);

const b = { hello: 'world' };
Object.assign(b, { hello: 'mars' });

const c = new Uint8Array([20, 5, 15]);
c.sort();

In this example “constant” variables a, b, and c are actually mutated.

[eligolding]

I actually starting working on a similar plugin a while back, and implemented a no-push rule which will catch all uses of the push() method on arrays. I didn't use any type inference though. It's just assuming that any MemberExpression named push is trying to mutate an array.
I can work on adding the rule to this project also.
Should be a separate rule, or included in the current no-mutation rule?

[StevenLangbroek]

That can cause issues with immutable: https://facebook.github.io/immutable-js/docs/#/List/push

[eligolding]

True, though I might venture to say that if you're using Immutable.js you probably wouldn't benefit from this plugin, as you'll be relying on the library to help prevent mutations, not a linter.

[mquandalle]

Well quite the contrary, if you want to prevent mutations in your existing code base, you will probably want to detect use of Array.push and replace them by a Immutable.List.push, thus the need of a tool that would distinguish the two.

[eligolding]

I guess you do have a point. Is there any way to statically infer that it's an array without using typescript or flow?

[dtinth]

I’d like to suggest banning all ExpressionStatement (see full proposal in #4). This will catch:

names.push('bar')
names.concat([' bar ']) // no point in evaluating this anyway

But not:

const namesWithBar = names.concat([' bar ']) // ok, bound to another variable
return namesWithBar // ok, returned

However it can’t catch this:

const newSize = names.push('baz') // maybe `no-unused-vars` can catch this
const newObject = Object.assign(oldObject, { foo: 'bar' }) // tough luck

[jhusain]

I agree that this should be the resolution. Any method name based prohibition is too brittle.

[mquandalle]

As you are closing this issue, at least it would be good to document the mutations that won’t be caught in the README of the no-mutation rule.