Enable `no-param-assign` in `esnext`?

[sindresorhus]

http://eslint.org/docs/rules/no-param-reassign with {props: true} option.

It's useful to prevent mistakes like modifying the user supplied object (which I still manage to do...).

I've not enabled the rule yet as it's useful to be able to do foo = foo || {}, but with default arguments in Node.js 6, this will no longer be needed.

I would like to enable this on esnext when Node.js 6 has been out for a while.

@jamestalmage @jfmengels @vdemedes Thoughts?

Relevant: https://github.com/eslint/eslint/issues/2770

[SamVerschueren]

This would be very nice! Modifying the user supplied object is a very common mistake. I made it in the past very often and probably will be in the future as well. It's very easily overlooked.

[jfmengels]

I'm all for it, as I prefer to do something like this anyway:

functino foo(_bar) {
  var bar = _bar || {};
  // ...
}

Wanting to wait for some version of Node to end/start/celebrate a birthday is always tricky though IMO, and I don't have a good solution for it. Most of the modules you and I build run on active/in maintenance versions of Node.js, and if possible, I'd like them to keep working for all "in maintenance" versions.

[jamestalmage]

modifying function parameters will also mutate the arguments object

Wow. Did not know that. So yeah, :+1:.

For non-esnext, could we still enable it, but relax if arguments is never used? (probably means new plugin).

[sindresorhus]

For non-esnext, could we still enable it, but relax if arguments is never used?

Personally don't care about the arguments object thing. The important part for me is to protect about modifying user supplied objects, like options. I also don't think it's worth doing a plugin. In a year we'll be targeting Node.js 6 and can use default arguments, even earlier for CLI tools.

Most of the modules you and I build run on active/in maintenance versions of Node.js, and if possible, I'd like them to keep working for all "in maintenance" versions.

That's why we have the esnext flag.

[jamestalmage]

The important part for me is to protect about modifying user supplied objects, like options.

OK, is there a way to have it just be about modifying a properties of an object? (and ignore the param reassignment part)? Because this really applies even without esnext.

[sindresorhus]

@jamestalmage No, that's why I initially didn't add it here. The props option is in addition to existing behavior. Not possible to have it be instead without doing a custom plugin. I guess we could do a quick fork of it and modify it to only be about props (If so, should be a separate plugin).

[sindresorhus]

This rule will be problematic for cases like the following though:

module.exports = foo => {
    if (typeof foo === 'string' || Buffer.isBuffer(foo)) {
        foo = doSomething(foo);
    }

    return doSomethingMore(foo);
};

I know I can define a variable at the top and assign to it, but feels like a lot of boilerplate.

Any suggestions?

[jfmengels]

You could wrap it in another function

function doSomethingIfNeeded(foo) {
  if (typeof foo === 'string' || Buffer.isBuffer(foo)) {
    return doSomething(foo);
  }
  return foo;
}

module.exports = foo => doSomethingMore(doSomethingIfNeeded(foo))

[jamestalmage]

I guess we could do a quick fork of it and modify it to only be about props (If so, should be a separate plugin).

That would be my vote on how to handle it. @jfmengels suggestion is nice, but also feels like boilerplate.

If we were to go ahead with this, I would like the rule to have some of the following behaviors:

export default function (foo, bar) {
  foo = foo || {}; // allowed
  bar = processInput(bar); // allowed

  foo.baz = 'something'; // error - foo *might* not have been reassigned. 
  bar.quz = 'something'; // allowed - we assume processInput returns a safe copy
}

// rule only applies to exported functions
export function recursive(input) {
  recurse(input, {stack: []});
}

function recurse(input, state) {
  // not exported, so `state` can be manipulated.
  state.foo = 'bar';

  // this is bad, but much harder to detect. I propose we punt on it for the first iteration.
  input.foo = 'baz';
}

The no-param-assign rule is going to have problems with the recurse function above. This is why I am now 👎 for it's inclusion, and want our own rule.

As a second milestone we could expand the rule to track reassignment that occurs in non-exported functions:

// It is exported, so both inputs - are marked as user supplied objects.
export default function(foo, bar) {
  foo = Object.assign({}, foo); // foo is unmarked - we no longer care what other functions do to it. 
  internal(foo, bar); // we check if `internal` treats `bar` safely - warning if it does not.
}

// We perform same analysis on `internal` function
// We collect metadata about which parameters it treats "nicely".
function internal(foo, bar) {

}

Finally, we could go nuts and analyze the contents of imported functions, checking how "nicely" they handle parameters passed to them. (Maybe just assume imports from node_modules behave correctly?).

This last idea (and the imports plugin) pushes the limits of traditional "linting". I almost think there is room for a second type of tool that does this more advanced stuff where multiple passes and analysis of the entire codebase are required. I think we need basic linting to stay fast. WebStorm tries to accommodate this by offering a separate Inspect Code... command. That command takes a long time (i.e. multiple minutes) to run, and returns LOTS of suggestions (often too many to be useful). It's not a perfect solution, but it does keep the slower running rules from slowing you down.

[sindresorhus]

I agree with bar = processInput(bar); // allowed and bar.quz = 'something'; // allowed, but I don't think foo = foo || {}; should be allowed. Here you can just use a default parameter.

The part about what's exported and not sounds a bit fragile and complicated. Even if you're working on an internal object, should you really mutate it? Nested mutations makes code so much harder to read and reason about, even if internal.

[sindresorhus]

@jamestalmage Can you move your last paragraph to https://github.com/sindresorhus/xo/issues/104, so we can discuss there instead? I really want something like that, that can do deeper analysis where speed is unimportant. Something I can run once in awhile.

[sholladay]

I enabled this in Tidy, my own xo-based linting config, and it has been treating me well.

It ended up with a lot of my code looking like:

const foo = (option) => {
    const config = Object.assign({}, option);
    // ...
};

... even when it isn't strictly necessary, which annoyed me at first. But then later when I wanted to add defaults or overrides it was so pleasing to have that pattern already in place.

BTW...

module.exports = foo => {
    return doSomethingMore(
        typeof foo === 'string' || Buffer.isBuffer(foo) ?
            doSomething(foo) :
            foo
    );
};