Disallow variable declarations from shadowing variables declared in the outer scope (no-shadow)

[dcousens]

Couldn't find an issue for this, so I just thought I'd bring it up.

http://eslint.org/docs/rules/no-shadow

"no-shadow": ["error", { "builtinGlobals": true, "hoist": "functions", "allow": [] }]

Bad

if (true) {
    let b = 6;
}

function b () {}

OK

if (true) {
    let a = 3;
}

let a = 5;

---

Where can I see this being an issue?

let parallel = require('run-parallel')

function (array, callback) {
  let tasks = array.map((x) => {
    return (callback) => foo(x, callback)
  })

  parallel(tasks, callback)
}

Personally, the above has bitten me when callback was lost inside the inner scope, so, I run this rule locally now.

[dcousens]

Another annoying, commonly used pattern:

foo((err) => {
  if (err) return
  bar((err) => {
    if (err) return
  })
})

err is shadowed... would probably want to define it and others like it in "allow".

[feross]

Generally feel fine about this. Need to see how many repos in the test suite would be affected by the change, though.

[dcousens]

Having turned this on local for a few days now... it has caught quite a lot of potential issues I was not previously aware of, about a 5-10% hit rate for actual bugs though.

[cesarandreu]

Would the following case be an error?

const result = ['a', 'b', 'b'].reduce((result, letter) => {
  result[letter] = (result[letter] || 0) + 1
  return result
}, {})

[dcousens]

@cesarandreu yes, I'm not sure if that would be configurable

[ManuelFte]

I went a bit more strict and used "hoist": "all" in my local settings, it really did catch a lot of issues I probably wouldn't have noticed otherwise. o.O

Wouldn't it be preferable to turn that setting to "all" instead of "functions" in the core rules?

[feross]

While this rule seems appealing, it breaks way too many packages (33%!) even with me specifying as many exceptions as I could think of. And it's not automatically fixable. Most of these are harmless instances of shadowing.

Config:

"no-shadow": ["error", { "builtinGlobals": true, "hoist": "functions", "allow": ["err", "cb", "callback", "done", "resolve", "reject"] }]

Results:

1..188
# tests 188
# pass  121
# fail  67

I think it's too noisy to ship.

Note: we already have https://eslint.org/docs/rules/no-shadow-restricted-names enabled, which is related, but much more conservative. It only prevents shadowing important globals.

[mightyiam]

I don't have a problem with shadowing. I wouldn't go as far as to rule it out.

[LinusU]

I personally don't have a problem with shadowing either, I think it's appropriate in some situations ☺️

[dcousens]

For me, the problem with shadowing is the temporal nature of bugs that appear.

If eslint could compare code at point A and point B and warn me that by removing a variable, I am causing a previously shadowed variable to inherit another's state... shadowing ahoy!

Until that point, I think it is a risk kept alive only by our lack of creativity with our variable names.

[ManuelFte]

I think shadowing, if it wasn't deliberate, is in general a bad practice and a symptom of lazy programming, whether it causes problems or not. Yes, it's harmless in most cases, but that's also the case for most rules in this standard; which is not the point. The point is forcing some good habits of ideal programming, and a habit like shadowing that has the potential to cause bugs and that can be easily avoided by merely picking different names for variables doesn't fit in that goal.

However, I acknowledge that it's a very extended practice, and if so many packages would break by implementing it in the core standard then I guess it can't be helped. For my part I'll continue disallowing it for my projects.

[mightyiam]

I'd like to change my mind, please. I might have come across a shadowing bug in my short career. I wouldn't mind having this rule. I hardly ever shadow, anyway.

[feross]

@ManuelFte @mightyiam I don't want to prevent patterns like this:

foo(err => {
  if (err) return console.error(err)
  bar(err => {
    if (err) return console.error(err)
    console.log('success!')
  })
})

If you try changing the rule and running the test suite, you'll find tons of harmless cases like this one. It's also not automatically fixable. While it's regrettable to miss out on catching some possible bugs, we can't enable rules that are too noisy or people will find standard is too annoying and stop using it. It's always a balance between pushing to add more strictness and being pragmatic. We have to take baby steps. And a rule that has mostly false positives and fails in 33% of existing repos seems too big to me.

[dcousens]

@feross I think that is a reasonable position. Out of interest, were there any actual bugs found in your tests?

[feross]

@dcousens I only skimmed it, but I didn't see any.