Why is ESlint happy for me to modify object properties as long as the object is reassigned? no-param-reassign

Question

In ESLint, the no-param-reassign rule as documented here, forbids you from assigning the value of a function param.

This is to avoid having a function's arguments object be modified.

The correct way to code is to reassign the param to a local var and return the var. This is fine for some types, but it seems pointless for objects passed to a function.

For example, let's take this function;

function foo(param) {
    var copy = param; // This makes the linter happy
    copy.bar = 2;
    console.log('arg 0: ', arguments[0], 'param:', param, 'copy:', copy);
    return copy; // A pointless return, the original object has been modified.
}

let test = { bar: 1 };
foo(test); 
console.log(test); // Has been modified
test = foo(test); // a pointless reassignment, foo has already changed test.
console.log(test); // Same effect as previous function call.

To be fair, ESLint does allow you to turn this feature off with /*eslint no-param-reassign: ["error", { "props": false }]*/; but I have to wonder why?

The point of this rule is to get rid of mutability and keep the arguments object pure, but a simple reassignment of an object will not do this.

The only way to truly do that would be to deep clone the param and assign that to a function scoped variable.

Am I missing something here?

`function(optionalArray,callback){if (typeof optionalArray === 'function') { callback = optionalArray; } doSomeAsyncStuff(); callback();}` -- code i have seen in the wild — serakfalcon, Aug 30 '18 at 22:11
It might not have enough data flow analysis to determine whether `copy` still refers to the parameter when you modify it. — Barmar, Aug 30 '18 at 22:11
`{ "props": false }` is the default. I wouldn’t recommend setting `{ "props": true }`, because it hardly catches anything (as you showed) and what it does catch is a perfectly normal thing to do. — Ry-, Aug 30 '18 at 23:17
Since the default setting doesn't complain about your code, it's not clear what your issue is. — Barmar, Aug 30 '18 at 23:52
@Barmar we are using the AirBnB linter rules where they have enabled it. Was just wondering if there was some sensible reasoning behind having it enabled. I think perhaps I was right that it doesn't make sense to warn about reassigning when modifying an object property (unless the intention is to make people clone everything instead of just reassigning). — Christie, Aug 31 '18 at 13:12
I managed to find AirBnB's recommended approach to this: https://github.com/airbnb/javascript/blob/48448a81cc899b3cbabfc13eab5b1dc432d24f7f/README.md#functions--mutate-params — Christie, Aug 31 '18 at 13:18

score 4 · Accepted Answer · answered Aug 31 '18 at 13:19

The likely reason that it doesn't warn when you assign the parameter to a variable is that it would require complex data flow analysis. Suppose you have code like this:

function foo(param, flag) {
    var copy = flag ? param : {...param};
    copy.bar = 2;
    console.log('arg 0: ', arguments[0], 'param:', param, 'copy:', copy);
    return copy; // A pointless return, the original object has been modified.
}

Now it can't tell whether copy contains the same object as param or a clone, it depends on the value of flag.

Or something like this:

function foo(param) {
    var copy = param;
    var copy2 = copy;
    var copy3 = copy2;
    copy3.bar = 2;
    console.log('arg 0: ', arguments[0], 'param:', param, 'copy:', copy3);
    return copy3;
}

This would require keeping track of the entire chain of references to determine that copy3 is the same as param.

Tracking this isn't impossible, optimizing compilers often do it. But it may be overkill for a linter.

thisisashwani · Answer 2 · 2021-01-22T14:41:12.980

Barmar's answer really hits the point. I am still adding my two cents here because this rule is very important when it comes to large codebases. Starting with your example only, the job of ESLint is to point the wrong coding practices. We, as developers, can still fool the linter though in many ways!

The correct way to address this ESLint error in function foo would have been this -

function foo(param) {
    // This makes the linter happy, as well as the codebase :')
    const copy = { ...param, bar: 2 }; 
    console.log('arg 0: ', arguments[0], 'param:', param, 'copy:', copy);
    return copy; // No modifications to the original object
}

As you can see, deep clone of the param is not required.

Regarding your question about the why of this rule, have a look at this super-long thread running on Github discussing the very same question that you have mentioned - https://github.com/airbnb/javascript/issues/719. Hope it helps you learn something new and interesting :)

Side Note - Simply reassigning parameters also makes code hard to follow. It also seems a bad idea as it deoptimizes in many engines, specifically v8. To be honest, I am still reading more on this to understand this last line better. In case you want to read on this too, see here - https://github.com/airbnb/javascript/issues/641#issuecomment-167827978

Why is ESlint happy for me to modify object properties as long as the object is reassigned? no-param-reassign

2 Answers2