fix(eslint-plugin): [require-await] handle async generators

[anikethsaha]

Fixes #1691

• added !node.generator before checking starts as to ignore the checks for generators
• added tests

[typescript-eslint]

Thanks for the PR, @anikethsaha!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[codecov]

Codecov Report

Merging #1782 into master will decrease coverage by 0.04%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##           master    #1782      +/-   ##
==========================================
- Coverage   94.96%   94.92%   -0.05%     
==========================================
  Files         159      159              
  Lines        7114     7126      +12     
  Branches     2035     2042       +7     
==========================================
+ Hits         6756     6764       +8     
- Misses        155      156       +1     
- Partials      203      206       +3

Flag	Coverage Δ
#unittest	94.92% <69.23%> (-0.05%)	⬇️

Impacted Files	Coverage Δ
packages/eslint-plugin/src/rules/require-await.ts	91.22% <69.23%> (-6.55%)	⬇️

[bradzacher]

Thanks for the PR

Could you please add some negative tests as well?

To ensure that code like the following aren't considered valid:

async function* foo() {} // no await or async yield
async function* bar() {
  return 1; // not a promise
}
async function* baz() {
  yield 1; // not an async yield
}

I.e. async generator is valid for this rule if it contains an await, returns a promise, or contains a yield*.

[anikethsaha]

To ensure that code like the following aren't considered valid:

async function* foo() {} // no await or async yield
async function* bar() {
  return 1; // not a promise
}
async function* baz() {
  yield 1; // not an async yield
}

These will be valid as these contains generators (along with async). Having generator will be a valid test irrespective of the function body

https://github.com/typescript-eslint/typescript-eslint/pull/1782/files#diff-0c97f9870cc2985da0e0acb232680f9bR122

Let me know if I am wrong !

[bradzacher]

Sorry, what I'm saying is that they should not be valid.

Adding * should not make the function bypass all checks this rule already does, it should just add an additional allowance in the form of checking for, and not reporting when the function contains a yield*.

[anikethsaha]

Actually in eslint repo, there is a similar issue in which it states that it should not check for yield as well cause async generators cant be yield that are coming generator function.

IMO, I would suggest having a separate rule like require-yield if not present currently

Let me know, if you want me to change this, I will add a check for yield * for generator function but I think its not in the scope of this rule to check yield

[bradzacher]

Note that you should always take the discussions in the ESLint repo with a pinch of salt in the context of this project, because we run in a typechecked world here.
Eg unlike the base repo, we can (and do) go one step further and deduce if a function returns a promise, making async () => Promise.resolve() valid in this rule.

---

Looking at it from this point of view, which is the question the rule is attempting to ask in a codebase: Does the async function require an await expression?

• an async function that returns a promise does not need an await.
  • as mentioned, this is one check we've added to the rule
• an async generator with just yield 1 does need an await.
  • otherwise, it's just a normal generator.
• an async generator with yield* syncGenerator() does need an await.
  • otherwise, it's just a normal generator.
• an async generator with yield* asyncGenerator() does not need an await, because the yielded generator is an async generator.

The last case is why they don't want to check them in the base rule - they cannot detect this case. OTOH, we can check it in here, because we have type information.

So rather than just ignoring all async generators, we should just ignore async generators that yield* asyncGenerator().

[anikethsaha]

we can (and do) go one step further and deduce if a function returns a promise,

I completely forgot the power of typescript for this 😅

So rather than just ignoring all async generators, we should just ignore async generators that yield* asyncGenerator()

also for returning promises as well right ?

[bradzacher]

also for returning promises as well right ?

Yes, sorry to clarify I meant this:
"in the case of async generators, we should additionally ignore if we detect yield* asyncGenerator()"
i.e. if it has an await, returns a promise, or has the above yield.

[anikethsaha]

ok cool, I will make the necessary changes

[anikethsaha]

• an async generator with yield* syncGenerator() does need an await.

  • otherwise, it's just a normal generator.

• an async generator with yield* asyncGenerator() does not need an await, because the yielded generator is an async generator.

@bradzacher can you give an example of these two cases

[bradzacher]

// OKAY - async generator has an await
async function* asyncGenerator() {
  await Promise.resolve()
  yield 1
}
// OKAY - no async
function* syncGenerator() {
  yield 1
}

// OKAY - async generator deferring to an async generator
async function* test1() {
  yield* asyncGenerator()
}
// TS error - non-async generator deferring to an async generator
function* test2() {
  yield* asyncGenerator()
}

// LINT ERROR - async generator has no await and is not deferring to an async generator
async function* test3() {
  yield asyncGenerator()
}
// OKAY - no async
function* test4() {
  yield asyncGenerator()
}

// LINT ERROR - async generator has no await and is deferring to a non-async generator
async function* test5() {
  yield* syncGenerator()
}
// OKAY - no async
function* test6() {
  yield* syncGenerator()
}

// LINT ERROR - async generator has no await and is not deferring to an async generator
async function* test7() {
  yield syncGenerator()
}
// OKAY - no async
function* test8() {
  yield syncGenerator()
}

repl

[anikethsaha]

Cool, I am adding these example as tests 👍

[anikethsaha]

• This should not be invalid for this rule right cause its not an async function so it wont be invalid

function* test2() {
  yield* asyncGenerator()
}

• This should be Lint free right ?

async function* test5() {
  yield* syncGenerator()
}

or is syncGenerator and asyncGenerator are special functions like a keyword ?

[bradzacher]

The former is ignored by the rule, yes, as it's not async.

The latter should be a lint error:

// LINT ERROR - async generator has no await and is deferring to a non-async generator

[anikethsaha]

is syncGenerator and asyncGenerator are special functions ? cause in AST it's simply an identifer

[anikethsaha]

whats the difference between

async function* test1() {
  yield* asyncGenerator()
}

and

async function* test1() {
  yield* syncGenerator()
}

[anikethsaha]

Ok got it, here is the defination,

// OKAY - async generator has an await
async function* asyncGenerator() {
  await Promise.resolve()
  yield 1
}
// OKAY - no async
function* syncGenerator() {
  yield 1
}

So, we need to check and test the function's declaration as well right to achieve this right ?

[anikethsaha]

cc @bradzacher

[bradzacher]

Yup! Similar to what we currently do for return statements.
With return statements - we grab the argument, and check to see if it's a promise.

With a yield* statement, we can similarly grab an argument, and check to see if its type is AsyncGenerator.

A good place to play with this is ts-ast-viewer

If you select one of the call expressions inside the foo function, then put this in the console, you can see how to uniquely tell what the type's name is.

type.target.getSymbol().getName()

[anikethsaha]

A good place to play with this is ts-ast-viewer

If you select one of the call expressions inside the foo function, then put this in the console, you can see how to uniquely tell what the type's name is.

I am not able to get this thing with this site.

is it possible if you can show it using AST-Explorer ?

Also,

this what the latest change looks like

repl.it

[TIP] Search using /* CHANGES HERE */ to move through the changes

Now ,

async function* asyncGenerator() {
  await Promise.resolve()
  yield 1
}


async function* test1() {
  yield* asyncGenerator()
}

We need to get the definition of asyncGenerator and need to just check whether its async or not right ? for this , we need to somehow get the definaitino of this function and check it. Might increase the complexity of this rule. (I might be wrong, I think I am missing something)

[anikethsaha]

@bradzacher you can check the diff here with reference to this PR's previous commit
6d8c439

[bradzacher]

Here's how to interact with the call expression in ts-ast-viewer:

Sorry about the size, the free converter wouldn't let me do full resolution.

unfortunately, astexplorer does not show type information - it only shows the AST.
OTOH, ts-ast-viewer shows you type information.

So when working with type info rules you kinda have to use both as you learn and get familiar with it all.

Essentially you will want do do something like the following:

'YieldExpression[delegate=true]'(node: TSESTree.YieldExpression): void {
  const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node.argument);
  const type = checker.getTypeAtLocation(tsNode);
  const symbol = type.getSymbol();
  if (symbol.getName() === 'AsyncGenerator' && scopeInfo) {
    scopeInfo.hasAsyncDelegateYield = true;
  }
},

[anikethsaha]

'YieldExpression[delegate=true]'(node: TSESTree.YieldExpression): void {
  const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node.argument);
  const type = checker.getTypeAtLocation(tsNode);
  const symbol = type.getSymbol(); // <--- undefined
  if (symbol.getName() === 'AsyncGenerator' && scopeInfo) {
    scopeInfo.hasAsyncDelegateYield = true;
  }
},

@bradzacher type.getSymbol(); and even type.symbol both are undefined

[bradzacher]

type.getSymbol(); and even type.symbol both are undefined

This is expected - some types won't have an associated symbol.
In this case you can just ignore the ones that don't - as they won't ever be AsyncGenerator types.

if (symbol?.getName() === 'AsyncGenerator' && scopeInfo) {

[anikethsaha]

@bradzacher any suggestion for type checks errors !?