feat: add suggestion to require-await to remove async keyword

[reduckted]

Fixes #18713

Prerequisites checklist

• 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:

#18713

What changes did you make? (Give an overview)

The require-await rule now includes a suggestion to remove the async keyword.

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

No.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: reduckted / name: Dave (dd5fe0a, 83f902a, c54121f, fd50aff, 727e87f, 94de93b, 278cc2c, 3bec996, b7e44e3, c1e87b2, ec295d5)

[netlify]

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	3bec996
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/66aca61f73551f0008ece4cb

[fisker]

class A { 
  a = 0
  async [b](){}
}

class A { 
  a = 0
  async * b(){}
}

foo
async () => {}

[mdjermanovic]

If we want to avoid removing comments, we could remove the range up to the first comment or token after async:

const asyncToken = sourceCode.getFirstToken(nodeWithAsyncKeyword, token => token.value === "async");

return [asyncToken.range[0], sourceCode.getTokenAfter(asyncToken, { includeComments: true }).range[0]];

[mdjermanovic]

class A { 
  a = 0
  async [b](){}
}

class A { 
  a = 0
  async * b(){}
}

foo
async () => {}

To avoid continuation, we could check if the token after async is one of those that can cause it. In our case, I think those can be only (, [, or *, as in these examples. Then, we could check the token before async and skip providing suggestion if it isn't one of the safe tokens (e.g., ;, ,, =, ?, :, {, ( ...).

[reduckted]

class A { 
  a = 0
  async * b(){ yield 0; }
}

Generator functions are not checked by the require-await rule.

To avoid continuation, we could check if the token after async is one of those that can cause it. In our case, I think those can be only (, [, or *, as in these examples. Then, we could check the token before async and skip providing suggestion if it isn't one of the safe tokens (e.g., ;, ,, =, ?, :, {, ( ...).

What about using astUtils.canTokensBeAdjacent? This code passes all of the tests (with the addition of the two additional code examples above), but I'm not sure if it covers everything that we need it to.

/*
 * If the async token and the token before it cannot be adjacent,
 * then ASI is currently being used. If the previous token and
 * the next token can be placed next to each other, then removing
 * the async keyword will change the meaning of the code.
 */
const previousToken = sourceCode.getTokenBefore(asyncToken, { includeComments: false });

if (previousToken && !astUtils.canTokensBeAdjacent(previousToken, asyncToken)) {
    const nextToken = sourceCode.getTokenAfter(asyncToken, { includeComments: false });

    if (astUtils.canTokensBeAdjacent(previousToken, nextToken)) {
        return null;
    }
}

[fasttime]

The rules no-object-constructor and no-array-constructor will insert a semicolon in cases where the suggestion would otherwise suppress automatic semicolon insertion, e.g.

a = 0
Object()

with the suggestion of no-object-constructor becomes:

a = 0
;({})

or, similarly:

foo
Array("a", "b")

after the suggestion of no-array-constructor will be:

foo
;["a", "b"]

PLAYGROUND

This is done by calling the ast-utils function needsPrecedingSemicolon (example) to check if a semicolon is required or not. I think that function would work well also for suggestions of require-await in the case of arrow functions. Maybe adding a semicolon is better than providing no suggestion at all in some cases.

[fasttime]

class A { 
  a = 0
  async * b(){}
}

It looks like the rule does not report problems for async generators, nor for empty functions. Any issue I'm missing with the above code?

[fisker]

I didn't run this rule, I was looking at the fix logic yesterday, and bring up cases that remove async breaks AST.

[fasttime]

I didn't run this rule, I was looking at the fix logic yesterday, and bring up cases that remove async breaks AST.

Fair enough. It seems that we don't need to worry about the token after async being * then.

[reduckted]

This is done by calling the ast-utils function needsPrecedingSemicolon (example) to check if a semicolon is required or not.

Unfortunately that function doesn't give the desired result for various cases and causes a semi-colon to be inserted when it shouldn't.

-(async function() { doSomething() })
+(;function() { doSomething() })

-({ async foo() { doSomething() } })
+(;function() { doSomething() })

-class A { async foo() { doSomething() } }
+class A { ;foo() { doSomething() } }

-(class { async foo() { doSomething() } })
+(class { ;foo() { doSomething() } })

-(class { async ''() { doSomething() } })
+(class { ;''() { doSomething() } })

-async function foo() { await (async () => { doSomething() }) }
+async function foo() { await (;() => { doSomething() }) }

The code using canTokensBeAdjacent in #18716 (comment) does work, so I'll go with that solution for now.

[mdjermanovic]

astUtils.canTokensBeAdjacent isn't related to ASI. It just checks whether two tokens can be placed next to each other with no whitespace between them (e.g., two words cannot as they would form a new word, while a word and a punctuator typically can).

[fasttime]

This is done by calling the ast-utils function needsPrecedingSemicolon (example) to check if a semicolon is required or not.

Unfortunately that function doesn't give the desired result for various cases and causes a semi-colon to be inserted when it shouldn't.

-(async function() { doSomething() })
+(;function() { doSomething() })

-({ async foo() { doSomething() } })
+(;function() { doSomething() })

-class A { async foo() { doSomething() } }
+class A { ;foo() { doSomething() } }

-(class { async foo() { doSomething() } })
+(class { ;foo() { doSomething() } })

-(class { async ''() { doSomething() } })
+(class { ;''() { doSomething() } })

-async function foo() { await (async () => { doSomething() }) }
+async function foo() { await (;() => { doSomething() }) }

It works but you also need to check that the async keyword is the first token in an ExpressionStatement node and that the first token after async is one one of ( or [. In other cases, the semicolon should not be necessary. These requirements are indicated in the documentation of needsPrecedingSemicolon.

In no-array-constructor, the check that the token is at the beginning of an expression statement is done with isStartOfExpressionStatement:

eslint/lib/rules/no-array-constructor.js

Line 106 in 21ebf8a

if (isStartOfExpressionStatement(node) && needsPrecedingSemicolon(sourceCode, node)) {

Additionally, for require-await, we will need to check that the first non-comment token after async is either ( or [.

[reduckted]

Ah, right, I understand now. Thanks for the explanation. 😄

So this condition:

const addSemiColon =
    nextToken.type === "Punctuator" &&
    (nextToken.value === "[" || nextToken.value === "(") &&
    astUtils.isStartOfExpressionStatement(nodeWithAsyncKeyword) &&
    astUtils.needsPrecedingSemicolon(sourceCode, nodeWithAsyncKeyword);

works for all but this test:

class A {
    a = 0
    async [b](){ return 0; }
}

In that test, nodeWithAsyncKeyword is async [b](){ return 0; }, but isStartOfExpressionStatement does not consider that to be the start of an expression because it's a "MethodDefinition". Instead of just using isStartOfExpressionStatement, I've used:

(nodeWithAsyncKeyword.type === "MethodDefinition" || astUtils.isStartOfExpressionStatement(nodeWithAsyncKeyword))

and that makes the test pass. Is "MethodDefinition" the only other node type to consider there, or are there other node types that need to be accounted for?