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.

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: add suggestion for no-return-await #16637

Merged

Conversation

dbartholomae
Copy link
Contributor

Prerequisites checklist

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

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

Add suggestion to no-return-await, see #16632

What changes did you make? (Give an overview)

I've added a suggestion for removing await before return to the rule no-return-await and adapted the tests for all failing cases to ensure that the correct suggestion is generated.

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

The change itself should be straight forward. I'm unsure if the createErrorList function I added in the test follows the code style used in this repo, though, and if it doesn't, am happy about suggestions how to do it differently.

Suggestions will be different for different errors, so a constant object won't work anymore
@dbartholomae dbartholomae requested a review from a team as a code owner December 10, 2022 10:40
@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon feature This change adds a new feature to ESLint labels Dec 10, 2022
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 10, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@netlify
Copy link

netlify bot commented Dec 10, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 6160c18
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/639c57a13843ef000891087a

@amareshsm
Copy link
Member

Please sign EasyCLA (Contributor License Agreement).

@dbartholomae
Copy link
Contributor Author

@amareshsm just did :)

@amareshsm amareshsm added the rule Relates to ESLint's core rules label Dec 10, 2022
@harish-sethuraman harish-sethuraman added accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Dec 10, 2022
@harish-sethuraman harish-sethuraman linked an issue Dec 10, 2022 that may be closed by this pull request
1 task
@mdjermanovic mdjermanovic added the enhancement This change enhances an existing feature of ESLint label Dec 10, 2022
lib/rules/no-return-await.js Outdated Show resolved Hide resolved
lib/rules/no-return-await.js Outdated Show resolved Hide resolved
lib/rules/no-return-await.js Outdated Show resolved Hide resolved
lib/rules/no-return-await.js Outdated Show resolved Hide resolved
{
messageId: "removeAwait",
fix(fixer) {
const areAwaitAndAwaitedExpressionOnTheSameLine = node.loc.start.line === node.argument.loc.start.line;
Copy link
Member

Choose a reason for hiding this comment

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

We can safely fix the code if the argument is parenthesized and the opening parenthesis is on the same line as await.

For example:

async () => {
    return await (
        foo()
    )
};

In this example, however, node and node.argument do not start on the same line, so per the current condition the suggestion will not be provided.

We could do something like this:

const awaitToken = sourceCode.getFirstToken(node);
const tokenAfterAwait = sourceCode.getTokenAfter(awaitToken);
const areAwaitAndAwaitedExpressionOnTheSameLine = awaitToken.loc.start.line === tokenAfterAwait.loc.start.line;

(sourceCode is now needed in multiple places, so we could declare const sourceCode = context.getSourceCode(); in create(context)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to implement this, though I would actually recommend not to do it, as it increases the complexity, risks of potentially introducing errors when running the suggestion if we missed a special case, and because suggestion from my point of view don't need to cover all use cases.
I'm a bit torn, though, as this might be a relevant case in a situation like the following:

async () => {
  return await (
    await fetch("https://really-long-domain.com/with-an-even-longer-path")
  ).json()
}

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to implement this, it isn't overly complex and we have similar logic in other rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that getTokenAfter is not getting the token immediately after the note:

  1. For async () => await bar(), it seems that sourceCode.getTokenAfter(awaitToken) is null.
  2. For
    async function foo() {
      return await new Promise(resolve => {
        resolve(5);
      });
    }
    getTokenAfter gives the semicolon 2 lines after the await, which leads to missing a suggestion that could happen.

Instead, this worked:

const [awaitToken, tokenAfterAwait] = sourceCode.getTokens(node);

Copy link
Member

Choose a reason for hiding this comment

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

  • For async () => await bar(), it seems that sourceCode.getTokenAfter(awaitToken) is null.

  • For

    async function foo() {
      return await new Promise(resolve => {
        resolve(5);
      });
    }

    getTokenAfter gives the semicolon 2 lines after the await, which leads to missing a suggestion that could happen.

This looks like the result of sourceCode.getTokenAfter(node). Did you try this, it should work:

const awaitToken = sourceCode.getFirstToken(node);
const tokenAfterAwait = sourceCode.getTokenAfter(awaitToken);

Copy link
Member

Choose a reason for hiding this comment

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

Or this:

const [awaitToken, tokenAfterAwait] = sourceCode.getFirstTokens(node, 2);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Yes, I indeed tried to getTokenAfter(node) 🤦
Looking at the alternatives now, I actually feel like the current solution with getTokens reads cleanest and would keep it - what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

sourceCode.getTokens(node) would go from the start till the end of the node to create and return an array with all tokens of the node, while we need only the first two. If you prefer a solution with getting all the tokens we need at once, sourceCode.getFirstTokens(node, 2) would be more optimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll change it right away

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

lib/rules/no-return-await.js Outdated Show resolved Hide resolved
lib/rules/no-return-await.js Outdated Show resolved Hide resolved
lib/rules/no-return-await.js Outdated Show resolved Hide resolved
dbartholomae and others added 5 commits December 14, 2022 19:36
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I'll leave this open until today's release in case anyone else wants to review it before merging.

@dbartholomae
Copy link
Contributor Author

Thanks, it was a pleasure! In case working with me wasn't too much of a hassle for you, and you have an issue that you would like me to work on, I can take one additional PR sometime around Christmas this year :)

@mdjermanovic mdjermanovic merged commit 075ef2c into eslint:main Dec 16, 2022
@mdjermanovic
Copy link
Member

Merged, thanks for contributing! You can always check issues labeled accepted.

For more details:

https://eslint.org/docs/latest/developer-guide/contributing/working-on-issues

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Dec 24, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.29.0` -> `8.30.0`](https://renovatebot.com/diffs/npm/eslint/8.29.0/8.30.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.30.0`](https://github.com/eslint/eslint/releases/tag/v8.30.0)

[Compare Source](eslint/eslint@v8.29.0...v8.30.0)

#### Features

-   [`075ef2c`](eslint/eslint@075ef2c) feat: add suggestion for no-return-await ([#&#8203;16637](eslint/eslint#16637)) (Daniel Bartholomae)
-   [`7190d98`](eslint/eslint@7190d98) feat: update globals ([#&#8203;16654](eslint/eslint#16654)) (Sébastien Règne)

#### Bug Fixes

-   [`1a327aa`](eslint/eslint@1a327aa) fix: Ensure flat config unignores work consistently like eslintrc ([#&#8203;16579](eslint/eslint#16579)) (Nicholas C. Zakas)
-   [`9b8bb72`](eslint/eslint@9b8bb72) fix: autofix recursive functions in no-var ([#&#8203;16611](eslint/eslint#16611)) (Milos Djermanovic)

#### Documentation

-   [`6a8cd94`](eslint/eslint@6a8cd94) docs: Clarify Discord info in issue template config ([#&#8203;16663](eslint/eslint#16663)) (Nicholas C. Zakas)
-   [`ad44344`](eslint/eslint@ad44344) docs: CLI documentation standardization ([#&#8203;16563](eslint/eslint#16563)) (Ben Perlmutter)
-   [`293573e`](eslint/eslint@293573e) docs: fix broken line numbers ([#&#8203;16606](eslint/eslint#16606)) (Sam Chen)
-   [`fa2c64b`](eslint/eslint@fa2c64b) docs: use relative links for internal links ([#&#8203;16631](eslint/eslint#16631)) (Percy Ma)
-   [`75276c9`](eslint/eslint@75276c9) docs: reorder options in no-unused-vars ([#&#8203;16625](eslint/eslint#16625)) (Milos Djermanovic)
-   [`7276fe5`](eslint/eslint@7276fe5) docs: Fix anchor in URL ([#&#8203;16628](eslint/eslint#16628)) (Karl Horky)
-   [`6bef135`](eslint/eslint@6bef135) docs: don't apply layouts to html formatter example ([#&#8203;16591](eslint/eslint#16591)) (Tanuj Kanti)
-   [`dfc7ec1`](eslint/eslint@dfc7ec1) docs: Formatters page updates ([#&#8203;16566](eslint/eslint#16566)) (Ben Perlmutter)
-   [`8ba124c`](eslint/eslint@8ba124c) docs: update the `prefer-const` example ([#&#8203;16607](eslint/eslint#16607)) (Pavel)
-   [`e6cb05a`](eslint/eslint@e6cb05a) docs: fix css leaking ([#&#8203;16603](eslint/eslint#16603)) (Sam Chen)

#### Chores

-   [`f2c4737`](eslint/eslint@f2c4737) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).4.0 ([#&#8203;16675](eslint/eslint#16675)) (Milos Djermanovic)
-   [`ba74253`](eslint/eslint@ba74253) chore: standardize npm script names per [#&#8203;14827](eslint/eslint#14827) ([#&#8203;16315](eslint/eslint#16315)) (Patrick McElhaney)
-   [`0d9af4c`](eslint/eslint@0d9af4c) ci: fix npm v9 problem with `file:` ([#&#8203;16664](eslint/eslint#16664)) (Milos Djermanovic)
-   [`90c9219`](eslint/eslint@90c9219) refactor: migrate off deprecated function-style rules in all tests ([#&#8203;16618](eslint/eslint#16618)) (Bryan Mishkin)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43MC40IiwidXBkYXRlZEluVmVyIjoiMzQuNzAuNCJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1689
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 15, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion contributor pool enhancement This change enhances an existing feature of ESLint feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule Change: make no-return-await suggestable
4 participants