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

Proposal: no-redeclare reports /*globals foo*/ comments if it's redeclaration #11370

Closed
mysticatea opened this issue Feb 9, 2019 · 9 comments
Closed

Comments

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Feb 9, 2019

What rule do you want to change?

Does this change cause the rule to produce more or fewer warnings?

  • more warnings

How will the change be implemented? (New option, new default behavior, etc.)?

  • I want to change the default behavior. But I'm OK if it's behind an option.

Please provide some example code that this change will affect:

config:

globals:
  window: readable
rules:
  no-redeclare:
    - error
    - builtinGlobals: true

code:

/*globals window */

What does the rule currently do for this code?

  • Does nothing.

What will the rule do after it's changed?

  • Reports the window in /*globals*/ comment because it's declared already.

For a reference, no-unused-vars rule reports unused variables that /*globals*/ comment defined. For example:

/*globals foo */ //ERROR: 'foo' is defined, but never used.

https://eslint.org/demo/#eyJ0ZXh0IjoiLypnbG9iYWxzIGZvbyAqLyIsIm9wdGlvbnMiOnsicGFyc2VyT3B0aW9ucyI6eyJlY21hVmVyc2lvbiI6OCwic291cmNlVHlwZSI6Im1vZHVsZSIsImVjbWFGZWF0dXJlcyI6e319LCJydWxlcyI6eyJuby11bnVzZWQtdmFycyI6Mn0sImVudiI6e319fQ==

Therefore, this change makes consistent between no-unused-vars rule and no-redeclare rule.

Are you willing to submit a pull request to implement this change?

Yes, I will do.

@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented Feb 9, 2019

I added core label because of a few changes required to implement this.

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Feb 13, 2019

Could you elaborate on what changes to core would be needed to implement this?

@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented Feb 13, 2019

Sure. The change will happen in

function addDeclaredGlobals(globalScope, configGlobals, commentDirectives) {

It needs to keep all locations rather than overriding. variable.eslintExplicitGlobalComment will be an array of globals comments, and variable.eslintConfigSetting or something like is needed to know eslintrc's setting.

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Feb 17, 2019

Thanks for elaborating. I think this sounds like a good idea overall, but it would be useful to have a more concrete proposal for what the breaking changes are. (For example, would there be two variables with the same name in the global scope? Or would additional metadata be added to a single variable?)

Before 0a3c3ff, I think /* global */ comments and configured globals with the same name would create separate variables, but this was changed to ensure /* global Foo: off */ would work correctly.

After this change, would the following create a no-redeclare error since it changes the config for Foo?

{
    "globals": {
        "Foo": "writable"
    }
}
// Change `Foo` from "writable" to "readable"
/* global Foo */

@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented Feb 18, 2019

Thank you for the feedback!

For example, would there be two variables with the same name in the global scope? Or would additional metadata be added to a single variable?

This proposal will do:

  • Modify metadata in existing variable objects.
    (makes variable.eslintExplicitGlobalComment to an array)
  • Add a metadata into existing variable objects.
    (variable.eslintConfigFileSetting which is the configuration in eslintrc file)

/* global Foo: off */

My preference is that no-redeclare rule will ignore such a removed declarations because it's a corner case and ECMAScript doesn't have the analogy of that.

Change Foo from "writable" to "readable"

My preference is that no-redeclare rule will warn it because it's similar to the redeclaration with let and const.

const Foo = 0
let Foo = 1 // 'Foo' is defined already.

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Feb 21, 2019

Changing variable.eslintExplicitGlobalComment to an array would probably break all users of the eslintExplicitGlobalComment property. I wasn't sure how many people are actually using eslintExplicitGlobalComment, so I did a GitHub search. I couldn't find any plugins using it, so it's probably fine to change it to an array as a breaking change.

However, would we want to name the property variable.eslintExplicitGlobalComments and deprecate or remove variable.eslintExplicitGlobalComment?

@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented Feb 23, 2019

However, would we want to name the property variable.eslintExplicitGlobalComments and deprecate or remove variable.eslintExplicitGlobalComment?

Sounds good to me.

@nzakas
Copy link
Member

@nzakas nzakas commented Feb 28, 2019

We should do an RFC for this.

@nzakas nzakas moved this from Needs discussion to Accepted, ready to implement in v6.0.0 Feb 28, 2019
@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Mar 1, 2019

This change was accepted in today's TSC meeting, but there was agreement that an RFC would be useful to decide on the precise changes to core.

mysticatea added a commit that referenced this issue Mar 15, 2019
- Implement eslint/rfcs#17
- Fixes #11370
- Remvoes the access to parserOptions from no-redeclare rule
- Adds several tests for lexical bindings to no-redeclare rule
@mysticatea mysticatea moved this from Accepted, ready to implement to Implemented, pending review in v6.0.0 Mar 15, 2019
@mysticatea mysticatea moved this from Implemented, pending review to Ready to merge in v6.0.0 Apr 8, 2019
@mysticatea mysticatea moved this from Ready to merge to Implemented, pending review in v6.0.0 Apr 8, 2019
v6.0.0 automation moved this from Implemented, pending review to Done Apr 25, 2019
mysticatea added a commit that referenced this issue Apr 25, 2019
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Oct 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
v6.0.0
  
Done
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants