Update: Add 'lexicalBindings' to no-implicit-globals and change messages

[mdjermanovic]

What is the purpose of this pull request? (put an "X" next to item)

[X] Changes an existing rule

Two changes based on #11981

---

-- New option --

What rule do you want to change?

no-implicit-globals

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

Same by default, more if the option is set.

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

New option.

Please provide some example code that this change will affect:

/*eslint no-implicit-globals: "error"*/

const a = 1;

let b;

class C {}

What does the rule currently do for this code?

No warnings.

What will the rule do after it's changed?

/*eslint no-implicit-globals: ["error", {"lexicalBindings": true}]*/

// Unexpected 'const' declaration in the global scope, wrap in a block or in an IIFE.
const a = 1; 

// Unexpected 'let' declaration in the global scope, wrap in a block or in an IIFE.
let b;

// Unexpected class declaration in the global scope, wrap in a block or in an IIFE.
class C {}

---

-- Changed messages --

What rule do you want to change?

no-implicit-globals

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

Same.

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

Same behavior with new messages.

Please provide some example code that this change will affect:

/*eslint no-implicit-globals: "error"*/

function f() {}

var a;

b = 1;

var Array;

Object = {};

What does the rule currently do for this code?

/*eslint no-implicit-globals: "error"*/

// Implicit global variable, assign as global property instead.
function f() {}

// Implicit global variable, assign as global property instead.
var a;

// Implicit global variable, assign as global property instead.
b = 1;

// Implicit global variable, assign as global property instead.
var Array;

// Implicit global variable, assign as global property instead.
Object = {};

What will the rule do after it's changed?

/*eslint no-implicit-globals: "error"*/

// Unexpected function declaration in the global scope, wrap in an IIFE for a local variable, assign as global property for a global variable.
function f() {}

// Unexpected 'var' declaration in the global scope, wrap in an IIFE for a local variable, assign as global property for a global variable.
var a;

// Global variable leak, declare the variable if it is intended to be local.
b = 1;

// Unexpected redeclaration of read-only global variable.
var Array;

// Unexpected assignment to read-only global variable.
Object = {};

What changes did you make? (Give an overview)

Added the new option and changed messages.

Is there anything you'd like reviewers to focus on?

Some messages might be too long.

[platinumazure]

Sorry for losing track of this!

Question: What should happen in module scope, or in the CJS scope? I assume we would want no messages in those cases for lexical declarations as they wouldn't be leaking to global scope?

[kaicataldo]

I think it makes sense to make the rule aware of whether sourceType is set to "script" or "module".

[mdjermanovic]

That should already work well, as the code uses scope manager to find declared globals only (doesn't just report all top-level declarations).

These are some test cases from valid[]:

        // Different scoping
        {
            code: "var foo = 1;",
            parserOptions: { ecmaVersion: 2015, sourceType: "module" }
        },
        {
            code: "function foo() {}",
            parserOptions: { ecmaVersion: 2015, sourceType: "module" }
        },
        {
            code: "function *foo() {}",
            parserOptions: { ecmaVersion: 2015, sourceType: "module" }
        },
        {
            code: "var foo = 1;",
            parserOptions: { ecmaFeatures: { globalReturn: true } }
        },
        {
            code: "function foo() {}",
            parserOptions: { ecmaFeatures: { globalReturn: true } }
        },
        {
            code: "var foo = 1;",
            env: { node: true }
        },
        {
            code: "function foo() {}",
            env: { node: true }
        },

        // different scoping
        {
            code: "const foo = 1; let bar; class Baz {}",
            parserOptions: { ecmaVersion: 2015, sourceType: "module" }
        },
        {
            code: "const foo = 1; let bar; class Baz {}",
            parserOptions: { ecmaVersion: 2015 },
            env: { node: true }
        },
        {
            code: "const foo = 1; let bar; class Baz {}",
            parserOptions: { ecmaVersion: 2015, ecmaFeatures: { globalReturn: true } }
        },

[mdjermanovic]

It might be also good to revisit this rule at some point. Seems that the rule overlaps a lot with other rules, namely no-global-assign, no-redeclare and no-undef.

In particular, I'm not sure why this rule disallows assignments to readonly globals. Also, why the rule allows global var declaration for writable globals because the preferred way is to assign to the global object instead.

(this PR didn't change behavior, the messages are just more explicit about the reason for an error).

[kaicataldo]

Thanks for the all tests and the fantastic documentation! LGTM.

[btmills]

This is a phenomenal improvement to the documentation! I really like what you've done with the messages. They should be much more helpful to users. Implementation LGTM, tests look plenty thorough. Nice work @mdjermanovic!