Should we allow mutating function parameters?

[aljones15]

Eslint contains a rule that throws if you try to mutate function parameters:

https://eslint.org/docs/rules/no-param-reassign

Disallow Reassignment of Function Parameters (no-param-reassign) Assignment to variables declared as function parameters can be misleading and lead to confusing behavior, as modifying function parameters will also mutate the arguments object. Often, assignment to function parameters is unintended and indicative of a mistake or programmer error.

This rule can be also configured to fail when function parameters are modified. Side effects on parameters can cause counter-intuitive execution flow and make errors difficult to track down.

The major reason for this rule is here:

https://spin.atomicobject.com/2011/04/10/javascript-don-t-reassign-your-function-arguments/

The rule also contains the option {props: true} if {props: true} then the re-assignment also applies to properties of parameters. This is relevant to us as we often use an object called options as the parameter to a function and then destructure and use those properties. Setting {props: true} means than this will cause a linting error:

// note: this is not a good example, but it does
// get across the point
const foo = ({bar, milo}) => {
  if(Array.isArray(milo)) {
    // this will cause an eslint error if `{props: true}`
    milo = milo.join(',')
  }
  return `${bar}${milo}`;
}

Thumbs up if you support adding this rule with {props: truie} to the rule set. Thumbs down if not, leave a comment if you want something else.

[davidlehn]

What problem is this solving? Did this expose actual bugs? I'm guessing this will cause havoc if applied across all our repos. I'm pretty sure we do safe param reassignment in various places where it makes sense. And I'm almost certain we have some code that intentionally mutates objects. While I don't disagree that this pattern may cause confusion if used poorly, I think we've been doing ok without strict rules.

It would be interesting to have a script that could test just this rule on every repo and see what happens. Just applying this and then having to later deal with lint blockers on valid code seems like a problem.

I'm a -1 without data on how this effects all our repos and if any actual bugs are uncovered or it just creates busy work to refactor ok code. I'm fine with a general non-strict guideline to avoid the pattern where it might cause confusion.

[aljones15]

@davidlehn You could run eslint globally with only this rule on the repos you're concerned with or make a project with just eslint and this rule enabled and then tell it to lint all projects you're concerned with. I don't think this will cause major issues, as parameter re-assignment is very small.

As for what problem is it solving: it means function parameters are pure or at least we can program assuming function parameters won't be re-assigned, inside of functions.

[mandyvenables]

Negative one, I am actually working on a PR now that does this https://github.com/digitalbazaar/bedrock-oauth2-client/blob/add-bedrock-ready/lib/oauth2-client.js#L45-L49 and I don't think it would be needed.

[aljones15]

closing as 3 developers regularly mutate function parameters.

[dlongley]

Every time I've searched for this particular concept on the web by asking the question "Why is function param reassignment bad?" I haven't actually gotten an answer. Instead, I've been told "because it mutates what another reference points to". But this happens all the time in every language for good reasons and without killing me. So that's not an answer to the question. That's an answer to the question "What happens when function param reassignment occurs?".

So "Why is X bad?" cannot be answered by simply saying how X behaves. That behavior needs to be tied to clearly negative outcomes without proportionally positive trade offs. For example, saying "Why is smoking bad?" Cannot be answered by saying "Because stuff goes into your body." Yeah, that happens when I eat too -- but eating is good because it means I don't die. Rather, one should say, when smoking, the stuff that goes into your body rapidly kills your lung cells which ultimately leads to cancer -- which kills you (usually in an unpleasant way, for good measure).

Side note: ESM modules were specifically designed to export live bindings that change for everyone holding an "import reference" to them whenever they are updated from within the module.

Side note: If we could refactor every bit of code that sets a proper default for a function param using some language built-in, we'd do it. But sometimes we can't. But if/when we can -- such a refactoring would result in the exact same outcome, the arguments value would reflect the proper defaults. In other words, our preference would be for the current outcome regardless.

[mattcollier]

The benefits of pure functions without side effects are obvious to me.

https://dev.to/rthefounding/avoiding-mutations-and-side-effects-using-functional-programming-3o4g

I also appreciate pass by reference which provides for a lot of efficiencies.

This is why I really appreciate the concept of "mutable references" in Rust: https://doc.rust-lang.org/rust-by-example/scope/borrow/mut.html

When I let my friend borrow a laptop, I don't expect to get it back with a new operating system and covered with "cool programming" stickers.

For DB and the existing code base, making mutation of parameters a linting error would be a disaster. However, I would would entertain the idea of making it a warning. This warning could lead to a developer changing the function to not mutate if that's easily possible (and it often is) or they could be an upstanding citizen and add an eslint disable notation along with a comment as to what the side-effects of the function are.

[dlongley]

@mattcollier, I don't think this is about side effects from calling functions. This about effects that are all internal to the function. I'm generally with you in not having functions modify external state in unexpected ways.

[mattcollier]

This is the eslint directive in question right? https://eslint.org/docs/rules/no-param-reassign

Which as I understand it is aimed at preventing the mutation of function arguments/parameters.

[dmitrizagidulin]

Hey @mattcollier, can I borrow your laptop? ...

[mattcollier]

Hey @mattcollier, can I borrow your laptop? ...

Would that be a mutable or unmutable borrow? See how nice this is? :)

[dmitrizagidulin]

LOL ok nice.

Still, though, I'd be overall -1 to adding this directive. Partly because I've seen what strict adherence to it does. I've worked in Erlang, which does not allow variable reassignment. Ever. for any reason. So, it's basically like if we only could use const, with no var or let. And what ended up happening is, you still needed to reassign variables. So you had these const clientId1 = ... and then a couple lines later you'd declare const clientId2 = <reassignment>, and yes, it made for immutable purity, but it was super annoying to read and write.

I think we can just say "hey, let's try to minimize param reassignment as much as possible", and do our code reviews with that in mind.

[mattcollier]

I think we can just say "hey, let's try to minimize param reassignment as much as possible", and do our code reviews with that in mind.

+1, so do you think warnings would be helpful to this end? A warning does not produce a CI/CD failure, unlike an error. And then if we want to eliminate the warning, an explicit eslint disable next-line directive can be added along with a comment as to why mutation is necessary and acceptable.

[dmitrizagidulin]

Oh, sure. I forgot warnings are an option.

[mattcollier]

Since there may be some support for warnings, I'm reopening this issue.

[dlongley]

@mattcollier,

This is the eslint directive in question right? https://eslint.org/docs/rules/no-param-reassign

Which as I understand it is aimed at preventing the mutation of function arguments/parameters.

That's right. This is about doing this:

function fn(a) {
  a = a || 'myDefaultValue';
}
let test = ''; // note: could even use `const test = ''` here
fn(test);
// test === '' (unchanged)

Which is different from doing this:

function fn(a) {
  a.foo++;
}
let test = {foo: 0};
fn(test);
// test.foo !== 0 (changed)

The latter involves modifying the state of a in a way that leaves the function. The former does not. You have complained about the latter in the past and we've cleaned up code to try and avoid it where we can. But, to be clear, this issue is not about the latter.

[dlongley]

If doing this by accident this was actually a common problem for devs then adding a warning could potentially make sense. However, I suspect that almost without exception, anyone doing this meant to do what they are doing and will add the disable comment. Which means there's some extremely tiny number of times that the warning will be useful rather than just annoying and a waste of development cycles.

Another danger that generally applies to the approach of "using warnings" like this:

I think it will train people to add disabling comments -- and then they will start to apply them where they shouldn't. Maybe this particular pattern is rare enough that it wouldn't cause such training, but I don't think the benefits are worth the potential drawbacks.

Now, if there were a rule that was sophisticated enough to throw an error when this:

function fn(a) {
  a = a || 'myDefaultValue';
}

Could be safely changed to this:

function fn(a = 'myDefaultValue') {
...
}

Then I'd +1 it. However, my understanding is that the rule will also flag this:

async function fn(b, a) {
  if(!a) {
    a = await determineDefault(...);
  }
}

Which is not ok.