no-mutating-methods sort after concat about eslint-plugin-fp HOT 8 OPEN

Hi @eddiemoore !

Sorry for the late answer, and thanks for the interesting suggestion. There isn't one at the moment, but we could implement it.
At first I thought that this is a good idea, but after thinking some more, I'm a little torn.

This rule is actually kind of a hack (or a heuristic of some sort), in the sense that it forbids some methods without actually knowing whether the object on which the method is called is actually an array and whether its sort method (and friends) is a mutating method or not.

In that sense, this rule is "limited" in its usefulness as it will allow code like this:

const array = { // array is not a real array
  map(fn) {
    window.foo++; // :O
  }
}

array.map(a => a); // .map() mutates stuff

Similarly, you could make this (Yes, they're all purposely trivial examples)

const someGlobalArray = [1, 2, 3];
const fakeArray = {
  concat(arg) {
    if (!arg) {
        return someGlobalArray;
    }
    return someGlobalArray.concat(arg);
  }
};

fakeArray.concat().sort(); // Sort will mutate someGlobalArray

This is the part where I am a bit torn. Sure, these examples are too simple, but someone might once use a library whose map/filter/... methods are not immutable (or not in every case as in my previous example). In this sense, this rule does not protect you from much, only the use of basic mutating array methods, and may forbid the use of some completely valid and immutable methods that are similarly named.

In practice, I think this should be safe for most cases, so I might be willing to have this implemented. Probably behind a flag though, to keep wary users feeling kind of secure.

What do you think?

from eslint-plugin-fp.

Yeah I get what you mean. If people are abusing it then that's their own fault I guess 😛. There will always be someone trying to abuse the code.

Just thinking that in most cases it would be safe. Besides, I think if someone is including the eslint-plugin-fp would mean that they are fairly serious about doing things in an immutable way.

In your examples with window.foo++ the plugin should catch that providing it's enabled.
The second example, not sure how you could check for that sort of case.

from eslint-plugin-fp.

Unless there is an option to make sure a function is not accessing things outside of the scope. So everything would need to be passed into the function.

from eslint-plugin-fp.

Found this when coming to submit an issue about Ramda's sort being flagged by this rule. Is there no way for ESLint to actually see whether the object that sort is being used on is an Array?

As for allowing sort after concat, I think that sounds brittle, and it's still technically mutating an array, so it might be better to keep that as an error? Just my two cents.

from eslint-plugin-fp.

@ahstro You could add R to the allowedObject list so then the Ramda sort should not come up with an error.

"fp/no-mutating-methods": ["error", {
  "allowedObjects": ['_', 'R', 'fp']
}]

Although yes, sort after concat is technically mutating an array, it's only doing it on a new array, so doesn't mutate the original. I guess it depends on how strict you are wanting to be 😛

from eslint-plugin-fp.

Oh, thanks, didn't know that allowedObjects was a thing :)

from eslint-plugin-fp.

Is there any way to configure the rule to allow this?
const sortedArray = [...someArray].sort()

It felt dirty, but I tried this with no success:

"fp/no-mutating-methods": ["error", {
  "allowedObjects": ['[]', ']']
}]

from eslint-plugin-fp.

ESLint and this plugin specifically are great at catching high level bugs and style issues, but for full accuracy and safety with (im)mutable types I'd recommend using a typechecker, as ESLint has fairly limited type inference.

For example, you can use TypeScript's ReadonlyArray type to ensure that your Array constants are immutable, but built in methods like concat and sort will still return normal Arrays that are mutable:

const myarray: ReadonlyArray<unknown> = []

// TS: Property 'sort' does not exist on type 'readonly unknown[]'.
myarray.sort()

// Works fine because concat is implemented on ReadonlyArray, and it returns an Array which implements sort
myarray.concat().sort()

Playground

from eslint-plugin-fp.