Skip to content

Commit eea3e7e

Browse files
mdjermanovicmdjermanovic
authored
fix: Remove configured global variables from GlobalScope#implicit (#19779)
* fix: Remove configured global variables from `GlobalScope#implicit` Fixes #19766 * update no-implicit-globals * add tests for optional `implicit` and `implicit.left` * consistency
1 parent a95721f commit eea3e7e

5 files changed

Lines changed: 468 additions & 16 deletions

File tree

‎docs/src/extend/scope-manager-interface.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,7 @@ Those members are defined but not used in ESLint.
374374
| `"CatchClause"` | `CatchClause` |
375375
| `"ClassName"` | `ClassDeclaration` or `ClassExpression` |
376376
| `"FunctionName"` | `FunctionDeclaration` or `FunctionExpression` |
377-
| `"ImplicitGlobalVariable"` | `AssignmentExpression` |
377+
| `"ImplicitGlobalVariable"` | `AssignmentExpression` or `ForInStatement` or `ForOfStatement` |
378378
| `"ImportBinding"` | `ImportSpecifier`, `ImportDefaultSpecifier`, or `ImportNamespaceSpecifier` |
379379
| `"Parameter"` | `FunctionDeclaration`, `FunctionExpression`, or `ArrowFunctionExpression` |
380380
| `"Variable"` | `VariableDeclarator` |

‎lib/languages/js/source-code/source-code.js‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,36 @@ function addDeclaredGlobals(
316316

317317
return true;
318318
});
319+
320+
/*
321+
* "implicit" contains information about implicit global variables (those created
322+
* implicitly by assigning values to undeclared variables in non-strict code).
323+
* Since we augment the global scope using configuration, we need to remove
324+
* the ones that were added by configuration, as they are either built-in
325+
* or declared elsewhere, therefore not implicit.
326+
* Since the "implicit" property was not documented, first we'll check if it exists
327+
* because it's possible that not all custom scope managers create this property.
328+
* If it exists, we assume it has properties `variables` and `set`. Property
329+
* `left` is considered optional (for example, typescript-eslint's scope manage
330+
* has this property named `leftToBeResolved`).
331+
*/
332+
const { implicit } = globalScope;
333+
if (typeof implicit === "object" && implicit !== null) {
334+
implicit.variables = implicit.variables.filter(variable => {
335+
const name = variable.name;
336+
if (globalScope.set.has(name)) {
337+
implicit.set.delete(name);
338+
return false;
339+
}
340+
return true;
341+
});
342+
343+
if (implicit.left) {
344+
implicit.left = implicit.left.filter(
345+
reference => !globalScope.set.has(reference.identifier.name),
346+
);
347+
}
348+
}
319349
}
320350

321351
/**

‎lib/rules/no-implicit-globals.js‎

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,12 @@
55

66
"use strict";
77

8+
const ASSIGNMENT_NODES = new Set([
9+
"AssignmentExpression",
10+
"ForInStatement",
11+
"ForOfStatement",
12+
]);
13+
814
//------------------------------------------------------------------------------
915
// Rule Definition
1016
//------------------------------------------------------------------------------
@@ -142,27 +148,37 @@ module.exports = {
142148
}
143149
}
144150
});
145-
});
146151

147-
// Undeclared assigned variables.
148-
scope.implicit.variables.forEach(variable => {
149-
const scopeVariable = scope.set.get(variable.name);
150-
let messageId;
152+
if (
153+
isReadonlyEslintGlobalVariable &&
154+
variable.defs.length === 0
155+
) {
156+
variable.references.forEach(reference => {
157+
if (reference.isWrite() && !reference.isRead()) {
158+
let assignmentParent =
159+
reference.identifier.parent;
160+
161+
while (
162+
assignmentParent &&
163+
!ASSIGNMENT_NODES.has(assignmentParent.type)
164+
) {
165+
assignmentParent = assignmentParent.parent;
166+
}
151167

152-
if (scopeVariable) {
153-
// ESLint global variable
154-
if (scopeVariable.writeable) {
155-
return;
156-
}
157-
messageId = "assignmentToReadonlyGlobal";
158-
} else {
159-
// Reference to an unknown variable, possible global leak.
160-
messageId = "globalVariableLeak";
168+
report(
169+
assignmentParent ?? reference.identifier,
170+
"assignmentToReadonlyGlobal",
171+
);
172+
}
173+
});
161174
}
175+
});
162176

177+
// Undeclared assigned variables.
178+
scope.implicit.variables.forEach(variable => {
163179
// def.node is an AssignmentExpression, ForInStatement or ForOfStatement.
164180
variable.defs.forEach(def => {
165-
report(def.node, messageId);
181+
report(def.node, "globalVariableLeak");
166182
});
167183
});
168184
},

‎tests/lib/languages/js/source-code/source-code.js‎

Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3606,4 +3606,216 @@ describe("SourceCode", () => {
36063606
assert.isNull(problem.ruleId);
36073607
});
36083608
});
3609+
3610+
describe("finalize()", () => {
3611+
it("should remove ECMAScript globals from global scope's `implicit`", () => {
3612+
const code = "Array = 1; Foo = 1; Promise = 1; Array; Foo; Promise";
3613+
const ast = espree.parse(code, DEFAULT_CONFIG);
3614+
const scopeManager = eslintScope.analyze(ast, {
3615+
ignoreEval: true,
3616+
ecmaVersion: 6,
3617+
});
3618+
const sourceCode = new SourceCode({
3619+
text: code,
3620+
ast,
3621+
scopeManager,
3622+
});
3623+
3624+
sourceCode.applyLanguageOptions({
3625+
ecmaVersion: 2015,
3626+
});
3627+
3628+
sourceCode.finalize();
3629+
3630+
const globalScope = sourceCode.scopeManager.scopes[0];
3631+
const { implicit } = globalScope;
3632+
3633+
assert.deepStrictEqual(
3634+
[...implicit.set].map(([name]) => name),
3635+
["Foo"],
3636+
);
3637+
assert.deepStrictEqual(
3638+
implicit.variables.map(({ name }) => name),
3639+
["Foo"],
3640+
);
3641+
assert.deepStrictEqual(
3642+
implicit.left.map(reference => reference.identifier.name),
3643+
["Foo", "Foo"],
3644+
);
3645+
});
3646+
3647+
it("should remove custom globals from global scope's `implicit`", () => {
3648+
const code = "Bar = 1; Foo = 1; Baz = 1; Bar; Foo; Baz";
3649+
const ast = espree.parse(code, DEFAULT_CONFIG);
3650+
const scopeManager = eslintScope.analyze(ast, {
3651+
ignoreEval: true,
3652+
ecmaVersion: 6,
3653+
});
3654+
const sourceCode = new SourceCode({
3655+
text: code,
3656+
ast,
3657+
scopeManager,
3658+
});
3659+
3660+
sourceCode.applyLanguageOptions({
3661+
ecmaVersion: 2015,
3662+
globals: {
3663+
Bar: "writable",
3664+
Baz: "readonly",
3665+
},
3666+
});
3667+
3668+
sourceCode.finalize();
3669+
3670+
const globalScope = sourceCode.scopeManager.scopes[0];
3671+
const { implicit } = globalScope;
3672+
3673+
assert.deepStrictEqual(
3674+
[...implicit.set].map(([name]) => name),
3675+
["Foo"],
3676+
);
3677+
assert.deepStrictEqual(
3678+
implicit.variables.map(({ name }) => name),
3679+
["Foo"],
3680+
);
3681+
assert.deepStrictEqual(
3682+
implicit.left.map(reference => reference.identifier.name),
3683+
["Foo", "Foo"],
3684+
);
3685+
});
3686+
3687+
it("should remove commonjs globals from global scope's `implicit`", () => {
3688+
const code =
3689+
"exports = {}; Foo = 1; require = () => {}; exports; Foo; require";
3690+
const ast = espree.parse(code, DEFAULT_CONFIG);
3691+
const scopeManager = eslintScope.analyze(ast, {
3692+
ignoreEval: true,
3693+
nodejsScope: true,
3694+
ecmaVersion: 6,
3695+
});
3696+
const sourceCode = new SourceCode({
3697+
text: code,
3698+
ast,
3699+
scopeManager,
3700+
});
3701+
3702+
sourceCode.applyLanguageOptions({
3703+
ecmaVersion: 2015,
3704+
sourceType: "commonjs",
3705+
});
3706+
3707+
sourceCode.finalize();
3708+
3709+
const globalScope = sourceCode.scopeManager.scopes[0];
3710+
const { implicit } = globalScope;
3711+
3712+
assert.deepStrictEqual(
3713+
[...implicit.set].map(([name]) => name),
3714+
["Foo"],
3715+
);
3716+
assert.deepStrictEqual(
3717+
implicit.variables.map(({ name }) => name),
3718+
["Foo"],
3719+
);
3720+
assert.deepStrictEqual(
3721+
implicit.left.map(reference => reference.identifier.name),
3722+
["Foo", "Foo"],
3723+
);
3724+
});
3725+
3726+
it("should remove inline globals from global scope's `implicit`", () => {
3727+
const code =
3728+
"/* globals Bar: writable, Baz: readonly */ Bar = 1; Foo = 1; Baz = 1; Bar; Foo; Baz";
3729+
const ast = espree.parse(code, DEFAULT_CONFIG);
3730+
const scopeManager = eslintScope.analyze(ast, {
3731+
ignoreEval: true,
3732+
ecmaVersion: 6,
3733+
});
3734+
const sourceCode = new SourceCode({
3735+
text: code,
3736+
ast,
3737+
scopeManager,
3738+
});
3739+
3740+
sourceCode.applyInlineConfig();
3741+
sourceCode.finalize();
3742+
3743+
const globalScope = sourceCode.scopeManager.scopes[0];
3744+
const { implicit } = globalScope;
3745+
3746+
assert.deepStrictEqual(
3747+
[...implicit.set].map(([name]) => name),
3748+
["Foo"],
3749+
);
3750+
assert.deepStrictEqual(
3751+
implicit.variables.map(({ name }) => name),
3752+
["Foo"],
3753+
);
3754+
assert.deepStrictEqual(
3755+
implicit.left.map(reference => reference.identifier.name),
3756+
["Foo", "Foo"],
3757+
);
3758+
});
3759+
3760+
it("should not crash if global scope doesn't have `implicit` property", () => {
3761+
const code = "Array = 1; Foo = 1; Promise = 1; Array; Foo; Promise";
3762+
const ast = espree.parse(code, DEFAULT_CONFIG);
3763+
const scopeManager = eslintScope.analyze(ast, {
3764+
ignoreEval: true,
3765+
ecmaVersion: 6,
3766+
});
3767+
3768+
const globalScope = scopeManager.scopes[0];
3769+
delete globalScope.implicit;
3770+
3771+
const sourceCode = new SourceCode({
3772+
text: code,
3773+
ast,
3774+
scopeManager,
3775+
});
3776+
3777+
sourceCode.applyLanguageOptions({
3778+
ecmaVersion: 2015,
3779+
});
3780+
3781+
// should not throw
3782+
sourceCode.finalize();
3783+
});
3784+
3785+
it("should not crash if global scope doesn't have `implicit.left` property", () => {
3786+
const code = "Array = 1; Foo = 1; Promise = 1; Array; Foo; Promise";
3787+
const ast = espree.parse(code, DEFAULT_CONFIG);
3788+
const scopeManager = eslintScope.analyze(ast, {
3789+
ignoreEval: true,
3790+
ecmaVersion: 6,
3791+
});
3792+
3793+
const globalScope = scopeManager.scopes[0];
3794+
delete globalScope.implicit.left;
3795+
3796+
const sourceCode = new SourceCode({
3797+
text: code,
3798+
ast,
3799+
scopeManager,
3800+
});
3801+
3802+
sourceCode.applyLanguageOptions({
3803+
ecmaVersion: 2015,
3804+
});
3805+
3806+
// should not throw
3807+
sourceCode.finalize();
3808+
3809+
const { implicit } = globalScope;
3810+
3811+
assert.deepStrictEqual(
3812+
[...implicit.set].map(([name]) => name),
3813+
["Foo"],
3814+
);
3815+
assert.deepStrictEqual(
3816+
implicit.variables.map(({ name }) => name),
3817+
["Foo"],
3818+
);
3819+
});
3820+
});
36093821
});

0 commit comments

Comments
 (0)