Rule suggestion: Don't reassign function parameters

[lxanders]

Reusing function parameters feels often like a code smell for me. If you consider something like the following example it could even cost you some debugging time (especially if you have long legacy functions):

function foo (bar) {
    bar = 13;
    // now we accidentally overwrote the function parameter which is most likely not good
}

I suggest the implementation of a rule that is similar to no-ex-assign but for functions instead of try-catch-blocks and is turned off by default.

What do you think about it?

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[nzakas]

Sounds good to me. New rules are fairly low priority right now, so this would probably have to come from an outside contribution.

[lo1tuma]

@nzakas Should this rule be combined with no-ex-assign somehow, or should this be separate?

[nzakas]

Good question. My initial reaction is to keep it separate.

[michaelficarra]

This wouldn't be able to protect you entirely from re-assigning a parameter due to the relationship between arguments and parameters. In your example, arguments[0] = 0 will change the value of bar to 0.

[nzakas]

I think that's fine. Arguably assigning to arguments makes it a bit more obvious that you're doing something bad.

[michaelficarra]

Agreed. It's just unfortunate. Maybe this rule could check for both?

[nzakas]

If it checks for both, then we probably need a way to specify each option individually. I sometimes reassign to named parameters but never to arguments, so I wouldn't want to choose all or none.

[michaelficarra]

Then turn the rule off entirely. If you never mutate arguments, you don't have to worry about doing it accidentally.

[nzakas]

That's not acceptable. If a rule checks two different patterns, we can't force people to choose all or nothing. Either we have two rules here or we have one with options. Or we just go with one rule for the original suggestion.

[lo1tuma]

I like the idea to check that arguments doesn’t get modified but I think it should be a separate rule.

The behavior of this rule is already mentioned in the webstorm rollup (#667):

3 Assignment to function parameter
This inspection reports any instances of assignment to a variable declared as a JavaScript function parameter. It also reports any attempt to increment or decrement the variable. While occasionally intended, this construct can be extremely confusing, and is often the result of a programmer error.

So we should also check for increment, decrement or other modifying operation.

[nzakas]

Agreed on all counts.

[bedney]

Completely agree on all counts. They should be separate rules and they are both code smells, IMHO.

[burnnat]

I'm working on this, should have a PR in the next day or two.

[nzakas]

Awesome!

[xjamundx]

What about things like:

function myFunc(boo) {

    // basic variations of default parameters
    boo = boo || 13;
    boo = boo === undefined ? 13 : boo;
    boo = typeof boo === 'undefined' ? 13 : boo;

    // defaults for an options object
    Object.assign(boo, {
      value: 13
    });
}

[lo1tuma]

@xjamundx I would consider your examples as a code smell and would prefer the rule to warn on such patterns. If you want to reassign function parameters, you can simple disable this rule.

[xjamundx]

I'd just suggest if that's intentional to specifically test and document that the default parameters style is considered bad. I think for many of us, it might be a surprise, though not necessarily worth carving out an exception for.

In ES6 environments the answer will just be to use proper default parameters anyway.

[bedney]

@lo1tuma - Yep. The exact purpose of this rule is to point out that this is a code smell.

[nzakas]

@xjamundx many do consider that as bad. This is why rules are configurable, if you disagree, you can turn it off. :)

[idchlife]

Guys, but what about .map? Lint saying this is bad, when I use reassignment in .map

[ilyavolodin]

Why would you ever want to do reassignment in map? The whole point of map is to translate from one array to another, not to mutate the original array.

[idchlife]

@ilyavolodin

You mean when I'm doing like this:

surveyWithIds.questions = surveyWithIds.questions.map((q: QuestionType): QuestionType => {
  q.id = uniqid();

  if (q.type === 'choice_list') {
    q.choices = q.choices.map((c: ChoiceType): ChoiceType => {
      c.id = uniqid();

      return c;
    });
  }

  return q;
});

It is considered bad practice? Old surveyWithIds.questions is gone anyway :/

[xjamundx]

Yeah so for example, what about q.choices.map(c => ({ ...c, id: uniqid() })) or even Object.assign({}, c, { id: uniqid() }), but again if you prefer mutating (which is totally fine) I'd just disable the rule.

[idchlife]

@xjamundx I suggest you to fix your example by adding additional curly brackets and return statement, because short function syntax won't work that way.

Yep, that's actually the stuff. Thanks!

[xjamundx]

@idchlife this is valid ES6 [1,2].map(c => ({a: 1})) not sure about TypeScript?

[idchlife]

@xjamundx oh, I see. I did not know about () around {} when returning object in short function.

That's actually just flowtype, not typescript.

[PaulDejardin]

I agree with the previous comments here that:

• reassigning arguments is a code smell
• reassigning named arguments is a code smell
• modifying properties (and nested properties) of named arguments is a code smell.

I'm currently evaluating no-param-reassign for a fairly large code base, because it's easy to perpetuate a pattern of mutating a single large argument blob over time, but it makes the code quite difficult to read and debug. The rule matches over 400 times, 90% of which could be considered extraneous to the core logic (stuff like assigning things to the global object, or to a parameter called scope (this is an Angular app). In practice that means it is difficult to employ this rule because those few arguments are "OK" to modify for structural reasons which would be difficult to refactor.

I'd like to propose adding another parameter to this rule for excluding specific property paths.

{
  "error",
  {
    "props": true,
    "exclude": {
      "global": false,
      "global.library": true,
      "scope": true,
      "some.other.specific.path.two": false
    }
  }
}

[platinumazure]

Can we please create a new issue for enhancement requests to this rule? Thanks!