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

Merged
merged 14 commits into from Dec 16, 2022
Merged

feat: add suggestion for no-return-await #16637

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1cb6e4a
refactor: extract error list into a function
dbartholomae Dec 10, 2022
60fe835
feat: suggest removing await in no-return-await
dbartholomae Dec 10, 2022
04675b3
Fix suggestion message
dbartholomae Dec 10, 2022
4a14630
refactor: remove unused data
dbartholomae Dec 11, 2022
1d20ef1
fix: only trim after await if there is a space
dbartholomae Dec 11, 2022
5cfaa84
refactor: allow expecting no suggestion
dbartholomae Dec 11, 2022
80bb8d7
refactor: move code for fixing into fix function
dbartholomae Dec 11, 2022
ee4087d
fix: do not suggest a fix if await and awaited expression are not on …
dbartholomae Dec 11, 2022
abd1d48
Simplify suggestion message
dbartholomae Dec 14, 2022
2a257d6
Simplify single-character access
dbartholomae Dec 14, 2022
ab271b1
Simplify removing a range
dbartholomae Dec 14, 2022
2331840
feat: suggest removing await when expression starts with (
dbartholomae Dec 15, 2022
725423a
refactor: use awaitToken to determine await range
dbartholomae Dec 15, 2022
6160c18
fix: use more performant way to identify tokens
dbartholomae Dec 16, 2022
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
29 changes: 28 additions & 1 deletion lib/rules/no-return-await.js
View file
Open in desktop
Expand Up @@ -13,6 +13,7 @@ const astUtils = require("./utils/ast-utils");
/** @type {import('../shared/types').Rule} */
module.exports = {
meta: {
hasSuggestions: true,
type: "suggestion",

docs: {
Expand All @@ -29,6 +30,7 @@ module.exports = {
],

messages: {
removeAwait: "Remove `await` after `return`.",
dbartholomae marked this conversation as resolved.
Show resolved Hide resolved
redundantUseOfAwait: "Redundant use of `await` on a return value."
}
},
Expand All @@ -44,7 +46,32 @@ module.exports = {
context.report({
node: context.getSourceCode().getFirstToken(node),
loc: node.loc,
messageId: "redundantUseOfAwait"
messageId: "redundantUseOfAwait",
suggest: [
{
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


if (!areAwaitAndAwaitedExpressionOnTheSameLine) {
return null;
}

const sourceCodeText = context.getSourceCode().text;

const startOfAwait = node.range[0];
const endOfAwait = startOfAwait + "await".length;
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved

const characterAfterAwait = sourceCodeText.slice(endOfAwait, endOfAwait + 1);
dbartholomae marked this conversation as resolved.
Show resolved Hide resolved
const trimLength = characterAfterAwait === " " ? 1 : 0;

const range = [startOfAwait, endOfAwait + trimLength];

return fixer.replaceTextRange(range, "");
dbartholomae marked this conversation as resolved.
Show resolved Hide resolved
}
}
]

});
}

Expand Down
153 changes: 122 additions & 31 deletions tests/lib/rules/no-return-await.js
View file
Open in desktop
Expand Up @@ -16,8 +16,24 @@ const { RuleTester } = require("../../../lib/rule-tester");
// Tests
//------------------------------------------------------------------------------

// pending https://github.com/eslint/espree/issues/304, the type should be "Keyword"
const errors = [{ messageId: "redundantUseOfAwait", type: "Identifier" }];
/**
* Creates the list of errors that should be found by this rule
* @param {Object} options Options for creating errors
* @param {string} options.suggestionOutput The suggested output
* @returns {Array} the list of errors
*/
function createErrorList({ suggestionOutput: output } = {}) {

// pending https://github.com/eslint/espree/issues/304, the type should be "Keyword"
return [{
messageId: "redundantUseOfAwait",
type: "Identifier",
suggestions: output ? [{
messageId: "removeAwait", output
}] : []
}];
}


const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 2017 } });

Expand Down Expand Up @@ -138,99 +154,103 @@ ruleTester.run("no-return-await", rule, {
invalid: [
{
code: "\nasync function foo() {\n\treturn await bar();\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn bar();\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn await(bar());\n}\n",
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (bar());\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (a, await bar());\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (a, bar());\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (a, b, await bar());\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (a, b, bar());\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (a && await bar());\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (a && bar());\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (a && b && await bar());\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (a && b && bar());\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (a || await bar());\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (a || bar());\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (a, b, (c, d, await bar()));\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (a, b, (c, d, bar()));\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (a, b, (c && await bar()));\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (a, b, (c && bar()));\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (await baz(), b, await bar());\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (await baz(), b, bar());\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (baz() ? await bar() : b);\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (baz() ? bar() : b);\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (baz() ? a : await bar());\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (baz() ? a : bar());\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (baz() ? (a, await bar()) : b);\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (baz() ? (a, bar()) : b);\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (baz() ? a : (b, await bar()));\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (baz() ? a : (b, bar()));\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (baz() ? (a && await bar()) : b);\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (baz() ? (a && bar()) : b);\n}\n" })
},
{
code: "\nasync function foo() {\n\treturn (baz() ? a : (b && await bar()));\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\n\treturn (baz() ? a : (b && bar()));\n}\n" })
},
{
code: "\nasync () => { return await bar(); }\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync () => { return bar(); }\n" })
},
{
code: "\nasync () => await bar()\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync () => bar()\n" })
},
{
code: "\nasync () => (a, b, await bar())\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync () => (a, b, bar())\n" })
},
{
code: "\nasync () => (a && await bar())\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync () => (a && bar())\n" })
},
{
code: "\nasync () => (baz() ? await bar() : b)\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync () => (baz() ? bar() : b)\n" })
},
{
code: "\nasync () => (baz() ? a : (b, await bar()))\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync () => (baz() ? a : (b, bar()))\n" })
},
{
code: "\nasync () => (baz() ? a : (b && await bar()))\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync () => (baz() ? a : (b && bar()))\n" })
},
{
code: "\nasync function foo() {\nif (a) {\n\t\tif (b) {\n\t\t\treturn await bar();\n\t\t}\n\t}\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync function foo() {\nif (a) {\n\t\tif (b) {\n\t\t\treturn bar();\n\t\t}\n\t}\n}\n" })
},
{
code: "\nasync () => {\nif (a) {\n\t\tif (b) {\n\t\t\treturn await bar();\n\t\t}\n\t}\n}\n",
errors
errors: createErrorList({ suggestionOutput: "\nasync () => {\nif (a) {\n\t\tif (b) {\n\t\t\treturn bar();\n\t\t}\n\t}\n}\n" })
},
{
code: `
Expand All @@ -241,7 +261,16 @@ ruleTester.run("no-return-await", rule, {
}
}
`,
errors
errors: createErrorList({
suggestionOutput: `
async function foo() {
try {}
finally {
return bar();
}
}
`
})
},
{
code: `
Expand All @@ -252,7 +281,16 @@ ruleTester.run("no-return-await", rule, {
}
}
`,
errors
errors: createErrorList({
suggestionOutput: `
async function foo() {
try {}
catch (e) {
return bar();
}
}
`
})
},
{
code: `
Expand All @@ -262,15 +300,29 @@ ruleTester.run("no-return-await", rule, {
}
} catch (e) {}
`,
errors
errors: createErrorList({
suggestionOutput: `
try {
async function foo() {
return bar();
}
} catch (e) {}
`
})
},
{
code: `
try {
async () => await bar();
} catch (e) {}
`,
errors
errors: createErrorList({
suggestionOutput: `
try {
async () => bar();
} catch (e) {}
`
})
},
{
code: `
Expand All @@ -284,7 +336,46 @@ ruleTester.run("no-return-await", rule, {
}
}
`,
errors
errors: createErrorList({
suggestionOutput: `
async function foo() {
try {}
catch (e) {
try {}
catch (e) {
return bar();
}
}
}
`
})
},
{
code: `
async function foo() {
return await new Promise(resolve => {
resolve(5);
});
}
`,
errors: createErrorList({
suggestionOutput: `
async function foo() {
return new Promise(resolve => {
resolve(5);
});
}
`
})
},
{
code: `
async function foo() {
return await // Test
5;
}
`,
errors: createErrorList()
}
]
});