Skip to content

Commit

Permalink
Update: stricter array index check in no-magic-numbers (fixes #12845) (…
Browse files Browse the repository at this point in the history 
…#12851)

* Update: stricter array index check in no-magic-numbers (fixes #12845)

* Improve docs and comments in the code
  • Loading branch information
@mdjermanovic
mdjermanovic committed Mar 28, 2020
1 parent 362713c commit e0f1b6c
Show file tree
Hide file tree
Showing 3 changed files with 527 additions and 61 deletions.
45 changes: 42 additions & 3 deletions docs/rules/no-magic-numbers.md
View file
Expand Up @@ -78,15 +78,54 @@ foo(1n);

### ignoreArrayIndexes

A boolean to specify if numbers used as array indexes are considered okay. `false` by default.
A boolean to specify if numbers used in the context of array indexes (e.g., `data[2]`) are considered okay. `false` by default.

This option allows only valid array indexes: numbers that will be coerced to one of `"0"`, `"1"`, `"2"` ... `"4294967294"`.

Arrays are objects, so they can have property names such as `"-1"` or `"2.5"`. However, those are just "normal" object properties that don't represent array elements. They don't influence the array's `length`, and they are ignored by array methods like `.map` or `.forEach`.

Additionally, since the maximum [array length](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/length) is 2<sup>32</sup> - 1, all values above 2<sup>32</sup> - 2 also represent just normal property names and are thus not considered to be array indexes.

Examples of **correct** code for the `{ "ignoreArrayIndexes": true }` option:

```js
/*eslint no-magic-numbers: ["error", { "ignoreArrayIndexes": true }]*/

var data = ['foo', 'bar', 'baz'];
var dataLast = data[2];
var item = data[2];

data[100] = a;

f(data[0]);

a = data[-0]; // same as data[0], -0 will be coerced to "0"

a = data[0xAB];

a = data[5.6e1];

a = data[10n]; // same as data[10], 10n will be coerced to "10"

a = data[4294967294]; // max array index
```

Examples of **incorrect** code for the `{ "ignoreArrayIndexes": true }` option:

```js
/*eslint no-magic-numbers: ["error", { "ignoreArrayIndexes": true }]*/

f(2); // not used as array index

a = data[-1];

a = data[2.5];

a = data[5.67e1];

a = data[-10n];

a = data[4294967295]; // above the max array index

a = data[1e500]; // same as data["Infinity"]
```

### enforceConst
Expand Down
109 changes: 72 additions & 37 deletions lib/rules/no-magic-numbers.js
View file
Expand Up @@ -7,6 +7,9 @@

const { isNumericLiteral } = require("./utils/ast-utils");

// Maximum array length by the ECMAScript Specification.
const MAX_ARRAY_LENGTH = 2 ** 32 - 1;

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -76,83 +79,115 @@ module.exports = {
ignore = (config.ignore || []).map(normalizeIgnoreValue),
ignoreArrayIndexes = !!config.ignoreArrayIndexes;

const okTypes = detectObjects ? [] : ["ObjectExpression", "Property", "AssignmentExpression"];

/**
* Returns whether the number should be ignored
* @param {number} num the number
* @returns {boolean} true if the number should be ignored
* Returns whether the rule is configured to ignore the given value
* @param {bigint|number} value The value to check
* @returns {boolean} true if the value is ignored
*/
function shouldIgnoreNumber(num) {
return ignore.indexOf(num) !== -1;
function isIgnoredValue(value) {
return ignore.indexOf(value) !== -1;
}

/**
* Returns whether the number should be ignored when used as a radix within parseInt() or Number.parseInt()
* @param {ASTNode} parent the non-"UnaryExpression" parent
* @param {ASTNode} node the node literal being evaluated
* @returns {boolean} true if the number should be ignored
* Returns whether the given node is used as a radix within parseInt() or Number.parseInt()
* @param {ASTNode} fullNumberNode `Literal` or `UnaryExpression` full number node
* @returns {boolean} true if the node is radix
*/
function shouldIgnoreParseInt(parent, node) {
return parent.type === "CallExpression" && node === parent.arguments[1] &&
(parent.callee.name === "parseInt" ||
parent.callee.type === "MemberExpression" &&
parent.callee.object.name === "Number" &&
parent.callee.property.name === "parseInt");
function isParseIntRadix(fullNumberNode) {
const parent = fullNumberNode.parent;

return parent.type === "CallExpression" && fullNumberNode === parent.arguments[1] &&
(
parent.callee.name === "parseInt" ||
(
parent.callee.type === "MemberExpression" &&
parent.callee.object.name === "Number" &&
parent.callee.property.name === "parseInt"
)
);
}

/**
* Returns whether the number should be ignored when used to define a JSX prop
* @param {ASTNode} parent the non-"UnaryExpression" parent
* @returns {boolean} true if the number should be ignored
* Returns whether the given node is a direct child of a JSX node.
* In particular, it aims to detect numbers used as prop values in JSX tags.
* Example: <input maxLength={10} />
* @param {ASTNode} fullNumberNode `Literal` or `UnaryExpression` full number node
* @returns {boolean} true if the node is a JSX number
*/
function shouldIgnoreJSXNumbers(parent) {
return parent.type.indexOf("JSX") === 0;
function isJSXNumber(fullNumberNode) {
return fullNumberNode.parent.type.indexOf("JSX") === 0;
}

/**
* Returns whether the number should be ignored when used as an array index with enabled 'ignoreArrayIndexes' option.
* @param {ASTNode} node Node to check
* @returns {boolean} true if the number should be ignored
* Returns whether the given node is used as an array index.
* Value must coerce to a valid array index name: "0", "1", "2" ... "4294967294".
*
* All other values, like "-1", "2.5", or "4294967295", are just "normal" object properties,
* which can be created and accessed on an array in addition to the array index properties,
* but they don't affect array's length and are not considered by methods such as .map(), .forEach() etc.
*
* The maximum array length by the specification is 2 ** 32 - 1 = 4294967295,
* thus the maximum valid index is 2 ** 32 - 2 = 4294967294.
*
* All notations are allowed, as long as the value coerces to one of "0", "1", "2" ... "4294967294".
*
* Valid examples:
* a[0], a[1], a[1.2e1], a[0xAB], a[0n], a[1n]
* a[-0] (same as a[0] because -0 coerces to "0")
* a[-0n] (-0n evaluates to 0n)
*
* Invalid examples:
* a[-1], a[-0xAB], a[-1n], a[2.5], a[1.23e1], a[12e-1]
* a[4294967295] (above the max index, it's an access to a regular property a["4294967295"])
* a[999999999999999999999] (even if it wasn't above the max index, it would be a["1e+21"])
* a[1e310] (same as a["Infinity"])
* @param {ASTNode} fullNumberNode `Literal` or `UnaryExpression` full number node
* @param {bigint|number} value Value expressed by the fullNumberNode
* @returns {boolean} true if the node is a valid array index
*/
function shouldIgnoreArrayIndexes(node) {
const parent = node.parent;
function isArrayIndex(fullNumberNode, value) {
const parent = fullNumberNode.parent;

return ignoreArrayIndexes &&
parent.type === "MemberExpression" && parent.property === node;
return parent.type === "MemberExpression" && parent.property === fullNumberNode &&
(Number.isInteger(value) || typeof value === "bigint") &&
value >= 0 && value < MAX_ARRAY_LENGTH;
}

return {
Literal(node) {
const okTypes = detectObjects ? [] : ["ObjectExpression", "Property", "AssignmentExpression"];

if (!isNumericLiteral(node)) {
return;
}

let fullNumberNode;
let parent;
let value;
let raw;

// For negative magic numbers: update the value and parent node
// Treat unary minus as a part of the number
if (node.parent.type === "UnaryExpression" && node.parent.operator === "-") {
fullNumberNode = node.parent;
parent = fullNumberNode.parent;
value = -node.value;
raw = `-${node.raw}`;
} else {
fullNumberNode = node;
parent = node.parent;
value = node.value;
raw = node.raw;
}

if (shouldIgnoreNumber(value) ||
shouldIgnoreParseInt(parent, fullNumberNode) ||
shouldIgnoreArrayIndexes(fullNumberNode) ||
shouldIgnoreJSXNumbers(parent)) {
// Always allow radix arguments and JSX props
if (
isIgnoredValue(value) ||
isParseIntRadix(fullNumberNode) ||
isJSXNumber(fullNumberNode) ||
(ignoreArrayIndexes && isArrayIndex(fullNumberNode, value))
) {
return;
}

const parent = fullNumberNode.parent;

if (parent.type === "VariableDeclarator") {
if (enforceConst && parent.parent.kind !== "const") {
context.report({
Expand Down

0 comments on commit e0f1b6c

Please sign in to comment.