New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: add 'allowOnTop' option to no-param-reassign rule #3652

Closed
farin opened this Issue Sep 4, 2015 · 16 comments

Comments

Projects
None yet
@farin
Copy link

farin commented Sep 4, 2015

When allowOnTop is true, reassign is allowed at the beginning of function scope (same concept like var statement in vars-on-top rule).

The reason - I don't want to reassign params except intended setting default like:

function (data) {  
   data = data || 0;
   ...
}

This can be avoided in ES6 with default arguments, but they can't used in all cases, for instance making a defensive copy:

function (data) {  
   data = _.clone(data);
   ...
}

You can use _ prefix or new variable dataCopy but i don't like it much. In fact I want to hide original variable to prevent unwanted access to unsafe origin.

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

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Sep 4, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@eslintbot eslintbot added the triage label Sep 4, 2015

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Sep 4, 2015

What should happen in the case like this:

function (data, options) {
  var data = data;
  if (options) {
     var options = { items: options, data: data };
  }
}

Should options override be flagged?

@ilyavolodin ilyavolodin added enhancement rule and removed triage labels Sep 4, 2015

@farin

This comment has been minimized.

Copy link

farin commented Sep 4, 2015

@ilyavolodin
do you mean items: options.items?
in such case I am using

function (data, options) {
   options = Object.assign({ data: data }, options);
   ...
}

so I still ok with allow just pure top and flag your example.

@nzakas nzakas added the evaluating label Sep 5, 2015

@zallek

This comment has been minimized.

Copy link
Contributor

zallek commented Oct 8, 2015

👍 I'd really like to have this.

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Dec 16, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 16, 2015

On the fence. Anyone want to champion this?

@strburst

This comment has been minimized.

Copy link

strburst commented Jan 5, 2016

👍 Anecdotally, I ran into this exact issue just now, so I'd also love to have this.

(Incidentally, thanks @farin for your blog post on extending AirBnB's style guide; I found it helpful.)

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 5, 2016

We still need a champion from the ESLint team for this enhancement.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Apr 21, 2016

We were unable to find a champion on the team for this proposal, so closing.

@nzakas nzakas closed this Apr 21, 2016

@abecks

This comment has been minimized.

Copy link

abecks commented Jun 6, 2016

Do these issues get re-opened at any point? Or are they lost to their watery grave because no one was available at the time to implement?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Jun 7, 2016

@abecks It's not about the time. It's about nobody on the team feeling strongly enough that this exception is important and widespread to champion or support it. That doesn't mean that you couldn't implement it yourself as a plugin however. Not everything makes it into the core, but that's exactly why ESLint was designed from the start to support plugins.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Dec 21, 2016

For future explorers: v8 severely deoptimizes a function in sloppy mode when you assign arguments, even on top, so it's potentially a big performance hazard - not "just" style.

@romellem

This comment has been minimized.

Copy link

romellem commented Jul 7, 2017

@ljharb That is only if you reference the arguments variable in the body of your function.

From the link you gave:

Reassigning a defined parameter while also mentioning arguments in the body [causes the function to be unoptimizable.]

Some example files:


unoptimized.js

function test(a) {
    if (arguments.length < 1) a = 10;
}

console.time('test');

for (let i = 0; i < 100000000; i++) {
    test();
}

console.timeEnd('test');

// prints "test: 3000ms"

optimized.js

function test(a) {
    a = a || 10;
}

console.time('test');

for (let i = 0; i < 100000000; i++) {
    test();
}

console.timeEnd('test');

// prints "test: 230ms"

As you can see, the file optimized.js with the function that has the a = a || 10; line runs about 13 times faster (tested with node 7.10.0), and is in fact being optimized by V8.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Jul 8, 2017

@romellem fair enough; but variables are free. there's still no good reason to reassign to named arguments. Your test lacks an example that does var b = a || 10; instead.

(Also, the JIT likely optimizes out that statement, since it's a noop, so it's probably not a valid benchmark)

@cipri-tom

This comment has been minimized.

Copy link

cipri-tom commented Jul 9, 2017

would it be feasible to have no-param-reassign recognise that some params are literals so assigning or changing them would not have side effects ?

function fromBytes(dView, startOffset = 0) {
    while (startOffset < dView.byteLength) {
        // magic bytes extraction with dView.getUint8(startOffset)
        startOffset += 1;  // offending line :(
    }
}
@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Jul 9, 2017

@cipri-tom reassignment doesn't have side effects; primitives are immutable (so "changing them" doesn't apply). Reassignment isn't bad because it has side effects; it's bad because you should just make a new variable, for clarity.

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.