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.