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: add suggestions to array-callback-return #17590

Merged
merged 7 commits into from Sep 22, 2023
Merged

feat: add suggestions to array-callback-return #17590

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7b8f4c7
feat: add suggestions to array-callback-return
Tanujkanti4441 Sep 19, 2023
49a979f
fix: spacing errors
Tanujkanti4441 Sep 19, 2023
4a2c4d2
fix: spacing errors
Tanujkanti4441 Sep 19, 2023
7e5a19c
feat: change meta type to problem again
Tanujkanti4441 Sep 19, 2023
86f3aa8
feat: disable eslint-plugin/require-meta-has-suggestions
Tanujkanti4441 Sep 19, 2023
ae5db96
Merge branch 'add-suggestion' of https://github.com/Tanujkanti4441/es…
Tanujkanti4441 Sep 19, 2023
1b9675d
feat: update tests
Tanujkanti4441 Sep 22, 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
156 changes: 133 additions & 23 deletions lib/rules/array-callback-return.js
View file
Open in desktop
Expand Up @@ -136,6 +136,76 @@ function getArrayMethodName(node) {
return null;
}

/**
* Checks if the given node is a void expression.
* @param {ASTNode} node The node to check.
* @returns {boolean} - `true` if the node is a void expression
*/
function isExpressionVoid(node) {
return node.type === "UnaryExpression" && node.operator === "void";
}

/**
* Fixes the linting error by prepending "void " to the given node
* @param {Object} sourceCode context given by context.sourceCode
* @param {ASTNode} node The node to fix.
* @param {Object} fixer The fixer object provided by ESLint.
* @returns {Array<Object>} - An array of fix objects to apply to the node.
*/
function voidPrependFixer(sourceCode, node, fixer) {

const requiresParens =

// prepending `void ` will fail if the node has a lower precedence than void
astUtils.getPrecedence(node) < astUtils.getPrecedence({ type: "UnaryExpression", operator: "void" }) &&

// check if there are parentheses around the node to avoid redundant parentheses
!astUtils.isParenthesised(sourceCode, node);

// avoid parentheses issues
const returnOrArrowToken = sourceCode.getTokenBefore(
node,
node.parent.type === "ArrowFunctionExpression"
? astUtils.isArrowToken

// isReturnToken
: token => token.type === "Keyword" && token.value === "return"
);

const firstToken = sourceCode.getTokenAfter(returnOrArrowToken);

const prependSpace =

// is return token, as => allows void to be adjacent
returnOrArrowToken.value === "return" &&

// If two tokens (return and "(") are adjacent
returnOrArrowToken.range[1] === firstToken.range[0];

return [
fixer.insertTextBefore(firstToken, `${prependSpace ? " " : ""}void ${requiresParens ? "(" : ""}`),
fixer.insertTextAfter(node, requiresParens ? ")" : "")
];
}

/**
* Fixes the linting error by `wrapping {}` around the given node's body.
* @param {Object} sourceCode context given by context.sourceCode
* @param {ASTNode} node The node to fix.
* @param {Object} fixer The fixer object provided by ESLint.
* @returns {Array<Object>} - An array of fix objects to apply to the node.
*/
function curlyWrapFixer(sourceCode, node, fixer) {
const arrowToken = sourceCode.getTokenBefore(node.body, astUtils.isArrowToken);
const firstToken = sourceCode.getTokenAfter(arrowToken);
const lastToken = sourceCode.getLastToken(node);

return [
fixer.insertTextBefore(firstToken, "{"),
fixer.insertTextAfter(lastToken, "}")
];
}

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand All @@ -151,6 +221,9 @@ module.exports = {
url: "https://eslint.org/docs/latest/rules/array-callback-return"
},

// eslint-disable-next-line eslint-plugin/require-meta-has-suggestions -- false positive
hasSuggestions: true,

schema: [
{
type: "object",
Expand All @@ -176,7 +249,9 @@ module.exports = {
expectedAtEnd: "{{arrayMethodName}}() expects a value to be returned at the end of {{name}}.",
expectedInside: "{{arrayMethodName}}() expects a return value from {{name}}.",
expectedReturnValue: "{{arrayMethodName}}() expects a return value from {{name}}.",
expectedNoReturnValue: "{{arrayMethodName}}() expects no useless return value from {{name}}."
expectedNoReturnValue: "{{arrayMethodName}}() expects no useless return value from {{name}}.",
wrapBraces: "Wrap the expression in `{}`.",
prependVoid: "Prepend `void` to the expression."
}
},

Expand Down Expand Up @@ -209,32 +284,57 @@ module.exports = {
return;
}

let messageId = null;
// let messageId = null;
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
const messageAndSuggestions = { messageId: "", suggest: [] };

if (funcInfo.arrayMethodName === "forEach") {
if (options.checkForEach && node.type === "ArrowFunctionExpression" && node.expression) {
if (options.allowVoid &&
node.body.type === "UnaryExpression" &&
node.body.operator === "void") {
return;
}

messageId = "expectedNoReturnValue";
if (options.allowVoid) {
if (isExpressionVoid(node.body)) {
return;
}

messageAndSuggestions.messageId = "expectedNoReturnValue";
messageAndSuggestions.suggest = [
{
messageId: "wrapBraces",
fix(fixer) {
return curlyWrapFixer(sourceCode, node, fixer);
}
},
{
messageId: "prependVoid",
fix(fixer) {
return voidPrependFixer(sourceCode, node.body, fixer);
}
}
];
} else {
messageAndSuggestions.messageId = "expectedNoReturnValue";
messageAndSuggestions.suggest = [{
messageId: "wrapBraces",
fix(fixer) {
return curlyWrapFixer(sourceCode, node, fixer);
}
}];
}
}
} else {
if (node.body.type === "BlockStatement" && isAnySegmentReachable(funcInfo.currentSegments)) {
messageId = funcInfo.hasReturn ? "expectedAtEnd" : "expectedInside";
messageAndSuggestions.messageId = funcInfo.hasReturn ? "expectedAtEnd" : "expectedInside";
}
}

if (messageId) {
if (messageAndSuggestions.messageId) {
const name = astUtils.getFunctionNameWithKind(node);

context.report({
node,
loc: astUtils.getFunctionHeadLoc(node, sourceCode),
messageId,
data: { name, arrayMethodName: fullMethodName(funcInfo.arrayMethodName) }
messageId: messageAndSuggestions.messageId,
data: { name, arrayMethodName: fullMethodName(funcInfo.arrayMethodName) },
suggest: messageAndSuggestions.suggest.length !== 0 ? messageAndSuggestions.suggest : null
});
}
}
Expand Down Expand Up @@ -295,36 +395,46 @@ module.exports = {

funcInfo.hasReturn = true;

let messageId = null;
const messageAndSuggestions = { messageId: "", suggest: [] };

if (funcInfo.arrayMethodName === "forEach") {

// if checkForEach: true, returning a value at any path inside a forEach is not allowed
if (options.checkForEach && node.argument) {
if (options.allowVoid &&
node.argument.type === "UnaryExpression" &&
node.argument.operator === "void") {
return;
}

messageId = "expectedNoReturnValue";
if (options.allowVoid) {
if (isExpressionVoid(node.argument)) {
return;
}

messageAndSuggestions.messageId = "expectedNoReturnValue";
messageAndSuggestions.suggest = [{
messageId: "prependVoid",
fix(fixer) {
return voidPrependFixer(sourceCode, node.argument, fixer);
}
}];
} else {
messageAndSuggestions.messageId = "expectedNoReturnValue";
}
}
} else {

// if allowImplicit: false, should also check node.argument
if (!options.allowImplicit && !node.argument) {
messageId = "expectedReturnValue";
messageAndSuggestions.messageId = "expectedReturnValue";
}
}

if (messageId) {
if (messageAndSuggestions.messageId) {
context.report({
node,
messageId,
messageId: messageAndSuggestions.messageId,
data: {
name: astUtils.getFunctionNameWithKind(funcInfo.node),
arrayMethodName: fullMethodName(funcInfo.arrayMethodName)
}
},
suggest: messageAndSuggestions.suggest !== 0 ? messageAndSuggestions.suggest : null
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
});
}
},
Expand Down
79 changes: 79 additions & 0 deletions tests/lib/rules/array-callback-return.js
View file
Open in desktop
Expand Up @@ -239,6 +239,85 @@ ruleTester.run("array-callback-return", rule, {
{ code: "foo.forEach((x) => { if (a === b) { return x; } })", options: checkForEachAllowVoid, parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "expectedNoReturnValue" }] },
{ code: "foo.forEach((x) => { if (a === b) { return !x } })", options: checkForEachAllowVoid, parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "expectedNoReturnValue" }] },

{
code: "foo.forEach(x => x)",
options: checkForEachOptions,
parserOptions: { ecmaVersion: 6 },
errors: [{
messageId: "expectedNoReturnValue",
suggestions: [
{ output: "foo.forEach(x => {x})" }
Copy link
Member

Choose a reason for hiding this comment

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

Can you add messageId to all suggestion test objects? Also, can you add suggestions to all test cases where the rule provides suggestions? This is for better testing and will soon be mandatory per RFC #84.

]
}]
},
{
code: "foo.forEach(x => x)",
options: checkForEachAllowVoid,
parserOptions: { ecmaVersion: 6 },
errors: [{
messageId: "expectedNoReturnValue",
suggestions: [
{ output: "foo.forEach(x => {x})" },
{ output: "foo.forEach(x => void x)" }
]
}]
},
{
code: "foo.forEach((x) => { return x; })",
options: checkForEachAllowVoid,
parserOptions: { ecmaVersion: 6 },
errors: [{
messageId: "expectedNoReturnValue",
suggestions: [
{ output: "foo.forEach((x) => { return void x; })" }
]
}]
},
{
code: "foo.forEach((x) => { return(x); })",
options: checkForEachAllowVoid,
parserOptions: { ecmaVersion: 6 },
errors: [{
messageId: "expectedNoReturnValue",
suggestions: [
{ output: "foo.forEach((x) => { return void (x); })" }
]
}]
},
{
code: "foo.forEach((x) => { return (x + 1); })",
options: checkForEachAllowVoid,
parserOptions: { ecmaVersion: 6 },
errors: [{
messageId: "expectedNoReturnValue",
suggestions: [
{ output: "foo.forEach((x) => { return void (x + 1); })" }
]
}]
},
{
code: "foo.forEach((x) => { if (a === b) { return x; } })",
options: checkForEachAllowVoid,
parserOptions: { ecmaVersion: 6 },
errors: [{
messageId: "expectedNoReturnValue",
suggestions: [
{ output: "foo.forEach((x) => { if (a === b) { return void x; } })" }
]
}]
},
{
code: "foo.forEach((x) => { if (a === b) { return (x + a); } })",
options: checkForEachAllowVoid,
parserOptions: { ecmaVersion: 6 },
errors: [{
messageId: "expectedNoReturnValue",
suggestions: [
{ output: "foo.forEach((x) => { if (a === b) { return void (x + a); } })" }
]
}]
},

// full location tests
{
code: "foo.filter(bar => { baz(); } )",
Expand Down