From 2b174599294af89eccc45831981be97f3c29a742 Mon Sep 17 00:00:00 2001 From: alberto Date: Mon, 1 Aug 2016 21:18:15 +0200 Subject: [PATCH] New: `no-global-assign` rule (fixes #6586) (#6746) --- Makefile.js | 29 +++++--- conf/category-list.json | 7 +- conf/eslint.json | 1 + docs/rules/no-extend-native.md | 2 +- docs/rules/no-global-assign.md | 89 +++++++++++++++++++++++ docs/rules/no-native-reassign.md | 2 + docs/user-guide/configuring.md | 2 +- lib/rules/no-global-assign.js | 83 +++++++++++++++++++++ lib/rules/no-native-reassign.js | 6 +- packages/eslint-config-eslint/default.yml | 3 +- tests/lib/cli.js | 2 +- tests/lib/rules/no-global-assign.js | 61 ++++++++++++++++ tests/lib/rules/no-native-reassign.js | 1 + 13 files changed, 271 insertions(+), 17 deletions(-) create mode 100644 docs/rules/no-global-assign.md create mode 100644 lib/rules/no-global-assign.js create mode 100644 tests/lib/rules/no-global-assign.js diff --git a/Makefile.js b/Makefile.js index a4948031d1e..16e863595a5 100644 --- a/Makefile.js +++ b/Makefile.js @@ -208,21 +208,28 @@ function generateRuleIndexPage(basedir) { find(path.join(basedir, "/lib/rules/")).filter(fileType("js")).forEach(function(filename) { let rule = require(filename); + let basename = path.basename(filename, ".js"); - let basename = path.basename(filename, ".js"), - output = { + if (rule.meta.deprecated) { + categoriesData.deprecated.rules.push({ name: basename, - description: rule.meta.docs.description, - recommended: rule.meta.docs.recommended || false, - fixable: !!rule.meta.fixable - }, - category = lodash.find(categoriesData.categories, {name: rule.meta.docs.category}); + replacedBy: rule.meta.docs.replacedBy + }); + } else { + let output = { + name: basename, + description: rule.meta.docs.description, + recommended: rule.meta.docs.recommended || false, + fixable: !!rule.meta.fixable + }, + category = lodash.find(categoriesData.categories, {name: rule.meta.docs.category}); + + if (!category.rules) { + category.rules = []; + } - if (!category.rules) { - category.rules = []; + category.rules.push(output); } - - category.rules.push(output); }); let output = yaml.safeDump(categoriesData, {sortKeys: true}); diff --git a/conf/category-list.json b/conf/category-list.json index d7e84b6c15f..b5020c1f00d 100644 --- a/conf/category-list.json +++ b/conf/category-list.json @@ -8,6 +8,11 @@ { "name": "Stylistic Issues", "description": "These rules relate to style guidelines, and are therefore quite subjective:" }, { "name": "ECMAScript 6", "description": "These rules relate to ES6, also known as ES2015:" } ], + "deprecated": { + "name": "Deprecated", + "description": "These rules have been deprecated and replaced by newer rules:", + "rules": [] + }, "removed": { "name": "Removed", "description": "These rules from older versions of ESLint have been replaced by newer rules:", @@ -32,4 +37,4 @@ { "removed": "spaced-line-comment", "replacedBy": ["spaced-comment"] } ] } -} \ No newline at end of file +} diff --git a/conf/eslint.json b/conf/eslint.json index 53b2e755906..c4a13bbdc6b 100755 --- a/conf/eslint.json +++ b/conf/eslint.json @@ -41,6 +41,7 @@ "no-fallthrough": "error", "no-floating-decimal": "off", "no-func-assign": "error", + "no-global-assign": "off", "no-implicit-coercion": "off", "no-implicit-globals": "off", "no-implied-eval": "off", diff --git a/docs/rules/no-extend-native.md b/docs/rules/no-extend-native.md index 14976ad2151..c3ccaeba66a 100644 --- a/docs/rules/no-extend-native.md +++ b/docs/rules/no-extend-native.md @@ -72,4 +72,4 @@ You may want to disable this rule when working with polyfills that try to patch ## Related Rules -* [no-native-reassign](no-native-reassign.md) +* [no-global-assign](no-global-assign.md) diff --git a/docs/rules/no-global-assign.md b/docs/rules/no-global-assign.md new file mode 100644 index 00000000000..c4c1914daa3 --- /dev/null +++ b/docs/rules/no-global-assign.md @@ -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) diff --git a/docs/rules/no-native-reassign.md b/docs/rules/no-native-reassign.md index 9fbfa0eb175..fb1fe9f3f68 100644 --- a/docs/rules/no-native-reassign.md +++ b/docs/rules/no-native-reassign.md @@ -1,5 +1,7 @@ # Disallow Reassignment of Native Objects (no-native-reassign) +This rule was **deprecated** in ESLint v3.3.0 and replaced by the [no-global-assign](no-global-assign.md) rule. + 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 diff --git a/docs/user-guide/configuring.md b/docs/user-guide/configuring.md index f6c55f73140..d530f5761fa 100644 --- a/docs/user-guide/configuring.md +++ b/docs/user-guide/configuring.md @@ -227,7 +227,7 @@ And in YAML: These examples allow `var1` to be overwritten in your code, but disallow it for `var2`. -**Note:** Enable the [no-native-reassign](../rules/no-native-reassign.md) rule to disallow modifications to read-only global variables in your code. +**Note:** Enable the [no-global-assign](../rules/no-global-assign.md) rule to disallow modifications to read-only global variables in your code. ## Configuring Plugins diff --git a/lib/rules/no-global-assign.js b/lib/rules/no-global-assign.js new file mode 100644 index 00000000000..1b2fd58b98e --- /dev/null +++ b/lib/rules/no-global-assign.js @@ -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); + } + }; + } +}; diff --git a/lib/rules/no-native-reassign.js b/lib/rules/no-native-reassign.js index aa8314dda67..92aeecfe356 100644 --- a/lib/rules/no-native-reassign.js +++ b/lib/rules/no-native-reassign.js @@ -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, + schema: [ { type: "object", diff --git a/packages/eslint-config-eslint/default.yml b/packages/eslint-config-eslint/default.yml index 26f592eae98..2a742eced52 100644 --- a/packages/eslint-config-eslint/default.yml +++ b/packages/eslint-config-eslint/default.yml @@ -44,6 +44,7 @@ rules: no-extra-bind: "error" no-fallthrough: "error" no-floating-decimal: "error" + no-global-assign: "error" no-implied-eval: "error" no-invalid-this: "error" no-iterator: "error" @@ -54,7 +55,7 @@ rules: no-mixed-spaces-and-tabs: ["error", false] no-multi-spaces: "error" no-multi-str: "error" - no-native-reassign: "error" + no-native-reassign: "off" no-nested-ternary: "error" no-new: "error" no-new-func: "error" diff --git a/tests/lib/cli.js b/tests/lib/cli.js index 2bba5354502..eb4d1323b57 100644 --- a/tests/lib/cli.js +++ b/tests/lib/cli.js @@ -472,7 +472,7 @@ describe("cli", function() { describe("when executing with global flag", function() { it("should default defined variables to read-only", function() { let filePath = getFixturePath("undef.js"); - let exit = cli.execute("--global baz,bat --no-ignore --rule no-native-reassign:2 " + filePath); + let exit = cli.execute("--global baz,bat --no-ignore --rule no-global-assign:2 " + filePath); assert.isTrue(log.info.calledOnce); assert.equal(exit, 1); diff --git a/tests/lib/rules/no-global-assign.js b/tests/lib/rules/no-global-assign.js new file mode 100644 index 00000000000..656d0350d0e --- /dev/null +++ b/tests/lib/rules/no-global-assign.js @@ -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"}] } + ] +}); diff --git a/tests/lib/rules/no-native-reassign.js b/tests/lib/rules/no-native-reassign.js index 5d50021801e..294639a0980 100644 --- a/tests/lib/rules/no-native-reassign.js +++ b/tests/lib/rules/no-native-reassign.js @@ -1,6 +1,7 @@ /** * @fileoverview Tests for no-native-reassign rule. * @author Ilya Volodin + * @deprecated in ESLint v3.3.0 */ "use strict";