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
feat: update no-array-constructor
rule
#17711
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 |
---|---|---|
|
@@ -5,6 +5,18 @@ | |
|
||
"use strict"; | ||
|
||
//------------------------------------------------------------------------------ | ||
// Requirements | ||
//------------------------------------------------------------------------------ | ||
|
||
const { | ||
getVariableByName, | ||
isClosingParenToken, | ||
isOpeningParenToken, | ||
isStartOfExpressionStatement, | ||
needsPrecedingSemicolon | ||
} = require("./utils/ast-utils"); | ||
|
||
//------------------------------------------------------------------------------ | ||
// Rule Definition | ||
//------------------------------------------------------------------------------ | ||
|
@@ -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)); | ||
|
||
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 | ||
|
@@ -37,11 +79,48 @@ 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"); | ||
|
||
/* | ||
* Check if `Array` is a predefined global variable: predefined globals have no declarations, | ||
* meaning that the `identifiers` list of the variable object is empty. | ||
*/ | ||
if (variable && variable.identifiers.length === 0) { | ||
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. Can we add a comment here explaining what this check does? |
||
const argsText = getArgumentsText(node); | ||
let fixText; | ||
let messageId; | ||
|
||
/* | ||
* Check if the suggested change should include a preceding semicolon or not. | ||
* Due to JavaScript's ASI rules, a missing semicolon may be inserted automatically | ||
* before an expression like `Array()` or `new Array()`, but not when the expression | ||
* is changed into an array literal like `[]`. | ||
*/ | ||
if (isStartOfExpressionStatement(node) && needsPrecedingSemicolon(sourceCode, node)) { | ||
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. Same here. Just a short explanation of what we are checking and why. 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. All done in c99888e. Thanks for the suggestions, I should learn to use comments more than I'm used to. 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. 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) | ||
} | ||
] | ||
}); | ||
} | ||
} | ||
|
||
|
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.
Since it's at this point known that there must be an opening parenthesis, can we simply the code like this:
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.
Hm, there's an edge case for
new (Array)
andnew (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.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.
Added unit tests in fa3b126. Any other way to simplify the above logic?
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.
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.