Rule suggestion: Avoid modifying object references

[lxanders]

In my opinion it is generally dangerous to change the input parameter of a function if it is not a primitive type. It could e.g. result in side effects that are hard to debug because the referenced object is changed in the function.

While I think we won't be able to detect every possible fault, I suggest creating a rule that at least catches the simple case:

// These cases could be an error or a warning if the rule is enabled
function foo1 (bar) {
    bar.thisPropertyAlreadyExisted = somethingElse;
}

function foo2 (bar) {
    delete bar.thisPropertyAlreadyExisted;
}

// This is most likely to hard to implement
function foo3 (bar) {
    var temp = bar.thisPropertyAlreadyExisted;
    temp.deepProperty = somethingElse;
}

What do you think about this?

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

[michaelficarra]

👍 * 1000. This would be very useful for me.

[nzakas]

Good idea. New rules are low priority right now, but we are open to contributions.

[Delapouite]

Great proposal, this rule would help distinguishing pure functions from the ones with unintented side effects.

[nzakas]

From @sindresorhus:

I don't have a concrete solution for this, but I'm going to elaborate on the problem.

Problem

I make a node module. The main export takes an input and some options as an object:

module.exports = function (input, options) {
  return options.foo == 'unicorn' ? input.split('/') : input;
};

The module does something with the input based on these options.

The problem is when the module matures and gets more functionality:

module.exports = function (input, options) {
  options.foo = options.bar === true ? 'unicorn' : 'cake';
  return options.foo ? input.split('/') : input;
};

As you might notice, we're now suddenly modifying the user supplied object. The change looks harmless, but what if the user uses the same object for multiple invocations.

It's an easy mistake to make, but can result in some really hard to track down bugs.

I've seen this mistake made in countless of modules and code. Even just today.

Correct way

The correct solution here is using some kind of extend method like Object.assign():

var objectAssign = require('object-assign');

module.exports = function (input, options) {
  options = objectAssign({}, options);
  options.foo = options.bar === true ? 'unicorn' : 'cake';
  return options.foo ? input.split('/') : input;
};

Discuss

I was hoping we could discuss what ESLint can do to help with this.

[mysticatea]

I will work on this.

I will add an option to no-param-reassign to disallow assigning to properties of parameters.

{
    "no-param-reassign": [2, {"props": true}]
}

To start, I will not support the following pattern:

function foo3 (bar) {
    var temp = bar.thisPropertyAlreadyExisted;
    temp.deepProperty = somethingElse;
}

I guess this pattern needs execution path analysis...

How is this thought?

[gyandeeps]

@nzakas above post from @sindresorhus , the correct solution doesn't actually solve the problem since its a shallow copy.
I think Object.assign repo should say that so that the consumers are not confused. I bet very few people know this.

[mysticatea]

Yup, it looks a seed of a rule, but I guess very difficult to make...

[nzakas]

I'd say don't worry about anything that needs path analysis. Just doing a basic check is good enough to start.

[feross]

Is there any way to get the { props: true } behavior without enabling no-param-reassign?