Skip to content
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

Redeclaring global variables should trigger "no-redeclare", not "no-undef". #4504

Closed
isiahmeadows opened this Issue Nov 21, 2015 · 15 comments

Comments

Projects
None yet
6 participants
@isiahmeadows
Copy link

isiahmeadows commented Nov 21, 2015

Minimal repro case:

/* eslint no-undef: 2, no-redeclare: 2 */
var toString = 1;

Expected:

/home/isiah/dev-projects/js/ttyped/tmp.js
  2:5  error  "toString" is already defined  no-redeclare

✖ 1 problem (1 error, 0 warnings)

Found:

path/to/file.js
  2:5  error  "toString" is read only  no-undef

✖ 1 problem (1 error, 0 warnings)

@IanVS IanVS added the triage label Nov 21, 2015

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 21, 2015

Why not?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Nov 21, 2015

toString is reserved JavaScript keyword and you are trying to override it. While it might not be clear why no-undef is the rule that's reporting this issue, it's a valid issue never the less.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 21, 2015

To be more precise, toString exists globally due to Object.prototype.toString, so it should behave like any other global.

That being said, no-undef should not be making this check as it should be covered by no-redeclare. We should fix this for 2.0.0.

@nzakas nzakas added bug rule accepted breaking and removed triage labels Nov 21, 2015

@isiahmeadows

This comment has been minimized.

Copy link
Author

isiahmeadows commented Nov 21, 2015

Should there be an option to exclude Object prototype members?

Also, there's a lot of other cases where no-undef is erroneously complaining when other rules should be.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 21, 2015

Can you explain the other cases?

@isiahmeadows

This comment has been minimized.

Copy link
Author

isiahmeadows commented Nov 21, 2015

I meant other cases like the one above. The rest seem functionally identical to the fact it should be reported by no-redeclare. My bad on the part of really bad phrasing.

@isiahmeadows isiahmeadows changed the title Object.prototype members shouldn't trigger `<value> is read-only` when "no-undef" is active Redeclaring global variables should trigger "no-redeclare", not "no-undef". Nov 21, 2015

@isiahmeadows

This comment has been minimized.

Copy link
Author

isiahmeadows commented Nov 21, 2015

I updated the initial post.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Nov 21, 2015

Yes, no-redeclare: [2, {builtinGlobals: true}] is catching var toString = 0.
And no-native-reassign is catching toString = 0.

So I think no-undef 's message can be removed.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Nov 23, 2015

At the same time, I want to set {builtinGlobals: true} of no-redeclare by default.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 23, 2015

Why?

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Nov 23, 2015

Because now no-undef is catching it by default.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 24, 2015

Ah, so you're saying if we changed no-redeclare, we could keep the same behavior?

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Nov 26, 2015

Exactly!

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 27, 2015

I see. My thought was that we want to make as few breaking changes as possible to do this, so I'd prefer to keep no-redeclare as it is and just change no-undef.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Nov 28, 2015

Hmm, OK.

@mysticatea mysticatea self-assigned this Nov 28, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Nov 28, 2015

@nzakas nzakas closed this in #4566 Dec 2, 2015

@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.
You can’t perform that action at this time.