Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: update no-array-constructor rule #17711

Merged
merged 4 commits into from Nov 14, 2023
Merged

feat: update no-array-constructor rule #17711

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
275d277
update rule `no-array-constructor`
fasttime Nov 4, 2023
c99888e
add comments to conditional statements
fasttime Nov 9, 2023
fa3b126
add unit tests for edge cases
fasttime Nov 14, 2023
e94ec9e
update JSDoc for `needsPrecedingSemicolon`
fasttime Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/src/_data/rules.json
View file
Open in desktop
Expand Up @@ -659,7 +659,7 @@
"description": "Disallow `Array` constructors",
"recommended": false,
"fixable": false,
"hasSuggestions": false
"hasSuggestions": true
},
{
"name": "no-bitwise",
Expand Down
3 changes: 2 additions & 1 deletion docs/src/_data/rules_meta.json
View file
Open in desktop
Expand Up @@ -758,7 +758,8 @@
"description": "Disallow `Array` constructors",
"recommended": false,
"url": "https://eslint.org/docs/latest/rules/no-array-constructor"
}
},
"hasSuggestions": true
},
"no-async-promise-executor": {
"type": "problem",
Expand Down
16 changes: 11 additions & 5 deletions docs/src/rules/no-array-constructor.md
View file
Open in desktop
Expand Up @@ -24,9 +24,13 @@ Examples of **incorrect** code for this rule:
```js
/*eslint no-array-constructor: "error"*/

Array(0, 1, 2)
Array();

new Array(0, 1, 2)
Array(0, 1, 2);

new Array(0, 1, 2);

Array(...args);
```

:::
Expand All @@ -38,11 +42,13 @@ Examples of **correct** code for this rule:
```js
/*eslint no-array-constructor: "error"*/

Array(500)
Array(500);

new Array(someOtherArray.length);

new Array(someOtherArray.length)
[0, 1, 2];

[0, 1, 2]
const createArray = Array => new Array();
```

:::
Expand Down
81 changes: 75 additions & 6 deletions lib/rules/no-array-constructor.js
View file
Open in desktop
Expand Up @@ -5,6 +5,18 @@

"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const {
getVariableByName,
isClosingParenToken,
isOpeningParenToken,
isStartOfExpressionStatement,
needsPrecedingSemicolon
} = require("./utils/ast-utils");

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand All @@ -20,15 +32,45 @@ module.exports = {
url: "https://eslint.org/docs/latest/rules/no-array-constructor"
},

hasSuggestions: true,

schema: [],

messages: {
preferLiteral: "The array literal notation [] is preferable."
preferLiteral: "The array literal notation [] is preferable.",
useLiteral: "Replace with an array literal.",
useLiteralAfterSemicolon: "Replace with an array literal, add preceding semicolon."
}
},

create(context) {

const sourceCode = context.sourceCode;

/**
* Gets the text between the calling parentheses of a CallExpression or NewExpression.
* @param {ASTNode} node A CallExpression or NewExpression node.
* @returns {string} The text between the calling parentheses, or an empty string if there are none.
*/
function getArgumentsText(node) {
const lastToken = sourceCode.getLastToken(node);

if (!isClosingParenToken(lastToken)) {
return "";
}

let firstToken = node.callee;

do {
firstToken = sourceCode.getTokenAfter(firstToken);
if (!firstToken || firstToken === lastToken) {
return "";
}
} while (!isOpeningParenToken(firstToken));
Comment on lines +62 to +69
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's at this point known that there must be an opening parenthesis, can we simply the code like this:

Suggested change
let firstToken = node.callee;
do {
firstToken = sourceCode.getTokenAfter(firstToken);
if (!firstToken || firstToken === lastToken) {
return "";
}
} while (!isOpeningParenToken(firstToken));
const firstToken = sourceCode.getTokenAfter(node.callee, isOpeningParenToken);

Copy link
Member Author

@fasttime fasttime Nov 14, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, there's an edge case for new (Array) and new (Array) && (foo), that's what the additional checks are needed for. But I totally forgot to add unit tests for that. Sorry... will add the tests now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added unit tests in fa3b126. Any other way to simplify the above logic?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, my suggestion wouldn't work for new (Array). Given this case, the code looks good and I don't think any other way would simplify it.


return sourceCode.text.slice(firstToken.range[1], lastToken.range[0]);
}

/**
* Disallow construction of dense arrays using the Array constructor
* @param {ASTNode} node node to evaluate
Expand All @@ -37,11 +79,38 @@ module.exports = {
*/
function check(node) {
if (
node.arguments.length !== 1 &&
node.callee.type === "Identifier" &&
node.callee.name === "Array"
) {
context.report({ node, messageId: "preferLiteral" });
node.callee.type !== "Identifier" ||
node.callee.name !== "Array" ||
node.arguments.length === 1 &&
node.arguments[0].type !== "SpreadElement") {
return;
}

const variable = getVariableByName(sourceCode.getScope(node), "Array");

if (variable && variable.identifiers.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment here explaining what this check does?

const argsText = getArgumentsText(node);
let fixText;
let messageId;

if (isStartOfExpressionStatement(node) && needsPrecedingSemicolon(sourceCode, node)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Just a short explanation of what we are checking and why.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All done in c99888e. Thanks for the suggestions, I should learn to use comments more than I'm used to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Yeah, I'm being more specific about comments in PRs now after spending weeks in the code path analysis code where there were very few comments. Trying to save future-us more pain.

fixText = `;[${argsText}]`;
messageId = "useLiteralAfterSemicolon";
} else {
fixText = `[${argsText}]`;
messageId = "useLiteral";
}

context.report({
node,
messageId: "preferLiteral",
suggest: [
{
messageId,
fix: fixer => fixer.replaceText(node, fixText)
}
]
});
}
}

Expand Down
113 changes: 7 additions & 106 deletions lib/rules/no-object-constructor.js
View file
Open in desktop
Expand Up @@ -9,67 +9,12 @@
// Requirements
//------------------------------------------------------------------------------

const { getVariableByName, isArrowToken, isClosingBraceToken, isClosingParenToken } = require("./utils/ast-utils");

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------

const BREAK_OR_CONTINUE = new Set(["BreakStatement", "ContinueStatement"]);

// Declaration types that must contain a string Literal node at the end.
const DECLARATIONS = new Set(["ExportAllDeclaration", "ExportNamedDeclaration", "ImportDeclaration"]);

const IDENTIFIER_OR_KEYWORD = new Set(["Identifier", "Keyword"]);

// Keywords that can immediately precede an ExpressionStatement node, mapped to the their node types.
const NODE_TYPES_BY_KEYWORD = {
__proto__: null,
break: "BreakStatement",
continue: "ContinueStatement",
debugger: "DebuggerStatement",
do: "DoWhileStatement",
else: "IfStatement",
return: "ReturnStatement",
yield: "YieldExpression"
};

/*
* Before an opening parenthesis, postfix `++` and `--` always trigger ASI;
* the tokens `:`, `;`, `{` and `=>` don't expect a semicolon, as that would count as an empty statement.
*/
const PUNCTUATORS = new Set([":", ";", "{", "=>", "++", "--"]);

/*
* Statements that can contain an `ExpressionStatement` after a closing parenthesis.
* DoWhileStatement is an exception in that it always triggers ASI after the closing parenthesis.
*/
const STATEMENTS = new Set([
"DoWhileStatement",
"ForInStatement",
"ForOfStatement",
"ForStatement",
"IfStatement",
"WhileStatement",
"WithStatement"
]);

/**
* Tests if a node appears at the beginning of an ancestor ExpressionStatement node.
* @param {ASTNode} node The node to check.
* @returns {boolean} Whether the node appears at the beginning of an ancestor ExpressionStatement node.
*/
function isStartOfExpressionStatement(node) {
const start = node.range[0];
let ancestor = node;

while ((ancestor = ancestor.parent) && ancestor.range[0] === start) {
if (ancestor.type === "ExpressionStatement") {
return true;
}
}
return false;
}
const {
getVariableByName,
isArrowToken,
isStartOfExpressionStatement,
needsPrecedingSemicolon
} = require("./utils/ast-utils");

//------------------------------------------------------------------------------
// Rule Definition
Expand Down Expand Up @@ -120,50 +65,6 @@ module.exports = {
return false;
}

/**
* Determines whether a parenthesized object literal that replaces a specified node needs to be preceded by a semicolon.
* @param {ASTNode} node The node to be replaced. This node should be at the start of an `ExpressionStatement` or at the start of the body of an `ArrowFunctionExpression`.
* @returns {boolean} Whether a semicolon is required before the parenthesized object literal.
*/
function needsSemicolon(node) {
const prevToken = sourceCode.getTokenBefore(node);

if (!prevToken || prevToken.type === "Punctuator" && PUNCTUATORS.has(prevToken.value)) {
return false;
}

const prevNode = sourceCode.getNodeByRangeIndex(prevToken.range[0]);

if (isClosingParenToken(prevToken)) {
return !STATEMENTS.has(prevNode.type);
}

if (isClosingBraceToken(prevToken)) {
return (
prevNode.type === "BlockStatement" && prevNode.parent.type === "FunctionExpression" ||
prevNode.type === "ClassBody" && prevNode.parent.type === "ClassExpression" ||
prevNode.type === "ObjectExpression"
);
}

if (IDENTIFIER_OR_KEYWORD.has(prevToken.type)) {
if (BREAK_OR_CONTINUE.has(prevNode.parent.type)) {
return false;
}

const keyword = prevToken.value;
const nodeType = NODE_TYPES_BY_KEYWORD[keyword];

return prevNode.type !== nodeType;
}

if (prevToken.type === "String") {
return !DECLARATIONS.has(prevNode.parent.type);
}

return true;
}

/**
* Reports on nodes where the `Object` constructor is called without arguments.
* @param {ASTNode} node The node to evaluate.
Expand All @@ -183,7 +84,7 @@ module.exports = {

if (needsParentheses(node)) {
replacement = "({})";
if (needsSemicolon(node)) {
if (needsPrecedingSemicolon(sourceCode, node)) {
fixText = ";({})";
messageId = "useLiteralAfterSemicolon";
} else {
Expand Down