-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
New: no-global-assign
rule (fixes #6586)
#6746
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
# Disallow assignment to native objects or read-only global variables (no-global-assign) | ||
|
||
JavaScript environments contain a number of built-in global variables, such as `window` in browsers and `process` in Node.js. In almost all cases, you don't want to assign a value to these global variables as doing so could result in losing access to important functionality. For example, you probably don't want to do this in browser code: | ||
|
||
```js | ||
window = {}; | ||
``` | ||
|
||
While examples such as `window` are obvious, there are often hundreds of built-in global objects provided by JavaScript environments. It can be hard to know if you're assigning to a global variable or not. | ||
|
||
## Rule Details | ||
|
||
This rule disallows modifications to read-only global variables. | ||
|
||
ESLint has the capability to configure global variables as read-only. | ||
|
||
* [Specifying Environments](../user-guide/configuring#specifying-environments) | ||
* [Specifying Globals](../user-guide/configuring#specifying-globals) | ||
|
||
Examples of **incorrect** code for this rule: | ||
|
||
```js | ||
/*eslint no-global-assign: "error"*/ | ||
|
||
Object = null | ||
undefined = 1 | ||
``` | ||
|
||
```js | ||
/*eslint no-global-assign: "error"*/ | ||
/*eslint-env browser*/ | ||
|
||
window = {} | ||
length = 1 | ||
top = 1 | ||
``` | ||
|
||
```js | ||
/*eslint no-global-assign: "error"*/ | ||
/*globals a:false*/ | ||
|
||
a = 1 | ||
``` | ||
|
||
Examples of **correct** code for this rule: | ||
|
||
```js | ||
/*eslint no-global-assign: "error"*/ | ||
|
||
a = 1 | ||
var b = 1 | ||
b = 2 | ||
``` | ||
|
||
```js | ||
/*eslint no-global-assign: "error"*/ | ||
/*eslint-env browser*/ | ||
|
||
onload = function() {} | ||
``` | ||
|
||
```js | ||
/*eslint no-global-assign: "error"*/ | ||
/*globals a:true*/ | ||
|
||
a = 1 | ||
``` | ||
|
||
## Options | ||
|
||
This rule accepts an `exceptions` option, which can be used to specify a list of builtins for which reassignments will be allowed: | ||
|
||
```json | ||
{ | ||
"rules": { | ||
"no-global-assign": ["error", {"exceptions": ["Object"]}] | ||
} | ||
} | ||
``` | ||
|
||
## When Not To Use It | ||
|
||
If you are trying to override one of the native objects. | ||
|
||
## Related Rules | ||
|
||
* [no-extend-native](no-extend-native.md) | ||
* [no-redeclare](no-redeclare.md) | ||
* [no-shadow](no-shadow.md) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
/** | ||
* @fileoverview Rule to disallow assignments to native objects or read-only global variables | ||
* @author Ilya Volodin | ||
*/ | ||
|
||
"use strict"; | ||
|
||
//------------------------------------------------------------------------------ | ||
// Rule Definition | ||
//------------------------------------------------------------------------------ | ||
|
||
module.exports = { | ||
meta: { | ||
docs: { | ||
description: "disallow assignments to native objects or read-only global variables", | ||
category: "Best Practices", | ||
recommended: false | ||
}, | ||
|
||
schema: [ | ||
{ | ||
type: "object", | ||
properties: { | ||
exceptions: { | ||
type: "array", | ||
items: {type: "string"}, | ||
uniqueItems: true | ||
} | ||
}, | ||
additionalProperties: false | ||
} | ||
] | ||
}, | ||
|
||
create: function(context) { | ||
let config = context.options[0]; | ||
let exceptions = (config && config.exceptions) || []; | ||
|
||
/** | ||
* Reports write references. | ||
* @param {Reference} reference - A reference to check. | ||
* @param {int} index - The index of the reference in the references. | ||
* @param {Reference[]} references - The array that the reference belongs to. | ||
* @returns {void} | ||
*/ | ||
function checkReference(reference, index, references) { | ||
let identifier = reference.identifier; | ||
|
||
if (reference.init === false && | ||
reference.isWrite() && | ||
|
||
// Destructuring assignments can have multiple default value, | ||
// so possibly there are multiple writeable references for the same identifier. | ||
(index === 0 || references[index - 1].identifier !== identifier) | ||
) { | ||
context.report({ | ||
node: identifier, | ||
message: "Read-only global '{{name}}' should not be modified.", | ||
data: identifier | ||
}); | ||
} | ||
} | ||
|
||
/** | ||
* Reports write references if a given variable is read-only builtin. | ||
* @param {Variable} variable - A variable to check. | ||
* @returns {void} | ||
*/ | ||
function checkVariable(variable) { | ||
if (variable.writeable === false && exceptions.indexOf(variable.name) === -1) { | ||
variable.references.forEach(checkReference); | ||
} | ||
} | ||
|
||
return { | ||
Program: function() { | ||
let globalScope = context.getScope(); | ||
|
||
globalScope.variables.forEach(checkVariable); | ||
} | ||
}; | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
/** | ||
* @fileoverview Rule to disallow assignments to native objects or read-only global variables | ||
* @author Ilya Volodin | ||
* @deprecated in ESLint v3.3.0 | ||
*/ | ||
|
||
"use strict"; | ||
|
@@ -14,9 +15,12 @@ module.exports = { | |
docs: { | ||
description: "disallow assignments to native objects or read-only global variables", | ||
category: "Best Practices", | ||
recommended: true | ||
recommended: true, | ||
replacedBy: ["no-global-assign"] | ||
}, | ||
|
||
deprecated: true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I put it there because it's what others suggested and what @mysticatea implemented in #6756. That's an example of usage for something which is not documentation. That said, I don't have a problem to move it. If we add I should probably keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with @platinumazure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good points, I didn't realize it was being used outside of docs. 👍 |
||
|
||
schema: [ | ||
{ | ||
type: "object", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
/** | ||
* @fileoverview Tests for no-global-assign rule. | ||
* @author Ilya Volodin | ||
*/ | ||
|
||
"use strict"; | ||
|
||
//------------------------------------------------------------------------------ | ||
// Requirements | ||
//------------------------------------------------------------------------------ | ||
|
||
let rule = require("../../../lib/rules/no-global-assign"), | ||
RuleTester = require("../../../lib/testers/rule-tester"); | ||
|
||
//------------------------------------------------------------------------------ | ||
// Tests | ||
//------------------------------------------------------------------------------ | ||
|
||
let ruleTester = new RuleTester(); | ||
|
||
ruleTester.run("no-global-assign", rule, { | ||
valid: [ | ||
"string = 'hello world';", | ||
"var string;", | ||
{ code: "Object = 0;", options: [{exceptions: ["Object"]}] }, | ||
{ code: "top = 0;" }, | ||
{ code: "onload = 0;", env: {browser: true} }, | ||
{ code: "require = 0;" }, | ||
{ code: "a = 1", globals: {a: true}}, | ||
"/*global a:true*/ a = 1" | ||
], | ||
invalid: [ | ||
{ code: "String = 'hello world';", errors: [{ message: "Read-only global 'String' should not be modified.", type: "Identifier"}] }, | ||
{ code: "String++;", errors: [{ message: "Read-only global 'String' should not be modified.", type: "Identifier"}] }, | ||
{ | ||
code: "({Object = 0, String = 0} = {});", | ||
parserOptions: { ecmaVersion: 6 }, | ||
errors: [ | ||
{message: "Read-only global 'Object' should not be modified.", type: "Identifier"}, | ||
{message: "Read-only global 'String' should not be modified.", type: "Identifier"} | ||
] | ||
}, | ||
{ | ||
code: "top = 0;", | ||
env: {browser: true}, | ||
errors: [{ message: "Read-only global 'top' should not be modified.", type: "Identifier"}] | ||
}, | ||
{ | ||
code: "require = 0;", | ||
env: {node: true}, | ||
errors: [{ message: "Read-only global 'require' should not be modified.", type: "Identifier"}] | ||
}, | ||
|
||
// Notifications of readonly are moved from no-undef: https://github.com/eslint/eslint/issues/4504 | ||
{ code: "/*global b:false*/ function f() { b = 1; }", errors: [{ message: "Read-only global 'b' should not be modified.", type: "Identifier"}] }, | ||
{ code: "function f() { b = 1; }", global: { b: false }, errors: [{ message: "Read-only global 'b' should not be modified.", type: "Identifier"}] }, | ||
{ code: "/*global b:false*/ function f() { b++; }", errors: [{ message: "Read-only global 'b' should not be modified.", type: "Identifier"}] }, | ||
{ code: "/*global b*/ b = 1;", errors: [{ message: "Read-only global 'b' should not be modified.", type: "Identifier"}] }, | ||
{ code: "Array = 1;", errors: [{ message: "Read-only global 'Array' should not be modified.", type: "Identifier"}] } | ||
] | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to skip this entirely? Shouldn't there be a category for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add it to the
categoriesData.deprecated.rules
array here instead of doing it manually in https://github.com/eslint/eslint/pull/6746/files#r72645142There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is a good idea. Less manual work is always preferable.