Overriding Promise does not trigger errors

[tkers]

What version are you using?
v1.5.1

What did you do?
We recently upgraded to Node 4.0.0 which includes Promises by default. As we have been using Bluebird Promises before, we want to remove this dependency where possible, and rename the variables we assign Bluebird to (e.g. to something like BBPromise to easily see the difference with native Promises).

JSHint always reported about the Redefinition of Promise when using var Promise = require("bluebird"); However, I can't seem to get the same from ESLint.

The following shows the problem:

/*eslint-env node, es6 */
/*eslint no-undef: 2*/
/*no-shadow: [2, {"builtinGlobals": true}]*/

// removing this line does not trigger the `no-undef` rule
// so Promise is correctly defined as a global
const Promise = require("bluebird");

Promise.resolve()
.then(() => { console.log("Done"); });

What happened?
Running ESLint on the piece of code returns without errors or warnings. Explicitly adding /*global Promise:false*/ does not change the results.

What did you expect to happen?
I expect ESLint to return an error about the fact that I'm redefining a global variable.

[eslintbot]

Thanks for the issue! If you're reporting a bug, please be sure to include:

1. The version of ESLint you are using (run eslint -v)
2. What you did (the source code and ESLint configuration)
3. The actual ESLint output complete with numbers
4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

[gyandeeps]

I think this is the rule you are looking for: http://eslint.org/docs/rules/no-native-reassign

[tkers]

Damn, have been looking through the docs forever.

Looks like only no-redeclare with {"builtinGlobals": true} does the trick, which is a bit weird because the docs make me think that this should behave the same as no-native-reassign in this case.

So thanks for pointing me in the right direction! My problem is solved for now, but this might still be a bug for no-native-reassign?

[gyandeeps]

I will furthur investigate this.
@ilyavolodin any feedback here?

[ilyavolodin]

I think this is correct behavior for all rules involved. This should not be flagged by no-shadow since, it's not really shadowing anything (same scope as built-in Promise). This should not be flagged by no-native-reassign because you are not assigning new value to an existing variable, you are creating a new variable with the same name as an existing variable - hence no-redeclare.

[tkers]

@ilyavolodin Makes sense. Trying to Promise = require("bluebird"); triggered no-native-reassign (and also no-undef for that matter) indeed. I guess I didn't read correctly, thanks anyway :)

[gyandeeps]

Alright thanks everyone for the discussion.

[michaelficarra]

@ilyavolodin

it's not really shadowing anything (same scope as built-in Promise)

That's not true. Top-level lexical declarations in modules shadow bindings in the global scope (see ES2015 §15.2.1.16.4 step 6). no-shadow should catch those. Top-level vars are just redeclarations of globals.

[ilyavolodin]

@michaelficarra I was thinking about it as I was writing my response. However, those are mechanics of the language itself. I'm not really sure it would be obvious to anyone outside of a few people who really know how global scope works in modules. And since this error is correctly reported by no-reassign already, I think it achieves expected result. We want to avoid situations where the same error is reported multiple times by different rules.

[nzakas]

Remember, node environment adds an extra scope, so const Promise is a shadow and not a redeclare. no-shadow should be able to flag that.

[ilyavolodin]

In that case, should no-redeclare not mark it?

[nzakas]

Correct. In the node environment, it's not a redeclaration, it's shadow.

[mysticatea]

I will try to fix this.
Is this a breaking change?

---

@michaelficarra

Top-level vars are just redeclarations of globals.

VarScopedDeclarations and LexicallyScopedDeclarations look creating bindings into the same environment record to me. (14.a.i and 16.a.ii.1)
What is a difference between VarScopedDeclarations and LexicallyScopedDeclarations?

[nzakas]

I don't think this is breaking, it's just a bug.