feat: Allow void in rule no-promise-executor-return

[nopeless]

Prerequisites checklist

• [x ] I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

• Allow void in no-promise-executor
• Update relevant documentation
• Add auto fix that adds a void in front of the returned expression (open for debate)
• Update relevant tests

Is there anything you'd like reviewers to focus on?

Edge cases that I might have not considered. I found one myself: adding void in front of () => ... when it is in front of another () =>will cause the expression to not be parsed

resolves #17278, #13668

[eslint-github-bot]

Hi @nopeless!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
• There should be a space following the initial tag and colon, for example 'feat: Message'.
• The first letter of the tag should be in lowercase
• The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

[eslint-github-bot]

Hi @nopeless!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
• There should be a space following the initial tag and colon, for example 'feat: Message'.
• The first letter of the tag should be in lowercase
• The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

[eslint-github-bot]

Hi @nopeless!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

[nopeless]

@mdjermanovic @nzakas

[Rec0iL99]

Hi @nopeless, please avoid pinging team members for review, it creates unnecessary spam in their notifications :(

Once you open a pr/issue we will definitely look into it 👍 .

Once your pr is ready for review move it from draft to open, this will request the team for review.

[mdjermanovic]

Thanks for the PR! Few notes about functionalities:

• Allow void in no-promise-executor

This should be behind an option, for example "allowVoid": boolean. The option should also allow return void.

• Add auto fix that adds a void in front of the returned expression (open for debate)

I think this rule shouldn't provide autofixes. Generally, rules that relate to possible logic errors in code should not be auto-fixable. For example, new Promise(() => 1); looks like a logic error. Automatically fixing that code to new Promise(() => void 1); would hide the error.

[nopeless]

Thanks for the PR! Few notes about functionalities:

• Allow void in no-promise-executor

This should be behind an option, for example "allowVoid": boolean. The option should also allow return void.

• Add auto fix that adds a void in front of the returned expression (open for debate)

I think this rule shouldn't provide autofixes. Generally, rules that relate to possible logic errors in code should not be auto-fixable. For example, new Promise(() => 1); looks like a logic error. Automatically fixing that code to new Promise(() => void 1); would hide the error.

Understandable, what should allowVoid's default be?

[mdjermanovic]

Understandable, what should allowVoid's default be?

false, to retain the current behavior of this rule.

[nopeless]

@mdjermanovic I have ran into an edge case that doesn't seem to have a good solution to

Currently, there are two suggestions avaliable: prepend void, wrap in {}

The second suggestion seems to produce a syntax error because of, in my opinion, a perculiarity in eslint's AST

() => (1);
// structure looks like
const ast = {
  type: "ArrowFunctionExpression",
  body: {
    type: "Literal",
    raw: "1"
  }
}

So, wrapping {} around the arrow body doesn't result in {(1)}, but ({1}), which is obviously a syntax error.

What should I do?

[mdjermanovic]

So, wrapping {} around the arrow body doesn't result in {(1)}, but ({1}), which is obviously a syntax error.

Yes, parentheses around node are not part of the node in AST. Instead of wrapping node.body, we could add { before the first token after =>, and } after the last token of the arrow expression node. Something like this:

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

//... 

const astUtils = require("./utils/ast-utils");

fix(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, "}")
    ];
}

[mdjermanovic]

/easycla

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: nopeless / name: nopeless (285b39e, a168690, 3669608, 1eac2dd, 274d3f2, a5fa6f5, ce8d993, f27b58b, 739a217, ac4a33f, deab66c, fe77e87)
• ✅ login: mdjermanovic / name: Milos Djermanovic (739a217, ac4a33f, deab66c, 2f9127b)

[nopeless]

@mdjermanovic should allowVoid even allow return void someValue? I personally think its ok to inline

// allowVoid: false
new Promise(r => {
  // invalid
  return 1;
});

// currently this doesn't work, allowing it is just adding one more condition, let me know what you think
// allowVoid: true
new Promise(r => {
  // valid
  return void 1;
});

[nzakas]

@mdjermanovic you mentioned we shouldn't be doing autofix for this, but then showed an autofix example a couple comments later. I also don't believe we should be autofixing. Has your opinion changed?

[nopeless]

@mdjermanovic you mentioned we shouldn't be doing autofix for this, but then showed an autofix example a couple comments later. I also don't believe we should be autofixing. Has your opinion changed?

Well it is a confusing conversation

I'll clarify now

I agree with @mdjermanovic that this problem should NOT be auto fixed

[nopeless]

Can you specify which comment refers to auto fixing? I might have forgotten to remove it

[Rec0iL99]

Can you specify which comment refers to auto fixing? I might have forgotten to remove it

#17282 (comment) and #17282 (comment)

[nopeless]

Can you specify which comment refers to auto fixing? I might have forgotten to remove it

#17282 (comment) and #17282 (comment)

Those are for suggestions, I hope that addresses your concern

[netlify]

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	2f9127b
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/64e1e0401b0a850008cb82da

[nzakas]

Waiting for @mdjermanovic to re-review. He's away for a bit but may be checking in periodically, so we appreciate your patience.

[nopeless]

sorry, got into some personal issues. I'll implement this in a couple days

[github-actions]

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

[Rec0iL99]

@nopeless is this ready for a re-review?

[nopeless]

@Rec0iL99 no. I am side tracked atm. I'll ping a member for review when im done 👍

[snitin315]

@nopeless No worries, let us know if you'd like someone else to finish the PR.

[nopeless]

@snitin315 @mdjermanovic Ready for another review 👍

[mdjermanovic]

Just a few suggestions about comments and JSDoc, then LGTM.

Tests are missing some properties that will become required, but we can update that in a follow-up (I think we should switch these tests to the basic format without helper functions).

[nopeless]

sorry, little busy with college and other projects. I'll get back on this when I get time

[snitin315]

LGTM. @mdjermanovic I feel we can apply your suggestions directly from the GitHub UI & merge this PR as this has been open for a long time.

Also, I can update the tests in a follow-up PR.

[mdjermanovic]

I feel we can apply your suggestions directly from the GitHub UI & merge this PR as this has been open for a long time.

Good idea! Just waiting for CI to finish and then I'll merge this.

[mdjermanovic]

LGTM, thanks!