From 3c21b0e91ed29c211ddb1245d899c7bc08a9b228 Mon Sep 17 00:00:00 2001 From: Anix Date: Mon, 23 Mar 2020 07:51:35 +0000 Subject: [PATCH 1/6] fix: no require-await check for async gen (fix #1691) --- .../eslint-plugin/src/rules/require-await.ts | 7 ++++++- .../tests/rules/require-await.test.ts | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/require-await.ts b/packages/eslint-plugin/src/rules/require-await.ts index e7b6a5f5959..876f0d60e83 100644 --- a/packages/eslint-plugin/src/rules/require-await.ts +++ b/packages/eslint-plugin/src/rules/require-await.ts @@ -62,7 +62,12 @@ export default util.createRule({ return; } - if (node.async && !scopeInfo.hasAwait && !isEmptyFunction(node)) { + if ( + !node.generator && + node.async && + !scopeInfo.hasAwait && + !isEmptyFunction(node) + ) { context.report({ node, loc: getFunctionHeadLoc(node, sourceCode), diff --git a/packages/eslint-plugin/tests/rules/require-await.test.ts b/packages/eslint-plugin/tests/rules/require-await.test.ts index 66db5497980..9ebb85bf6a1 100644 --- a/packages/eslint-plugin/tests/rules/require-await.test.ts +++ b/packages/eslint-plugin/tests/rules/require-await.test.ts @@ -104,6 +104,25 @@ async function testFunction(): Promise { )) } `, + 'async function* run() { yield * anotherAsyncGenerator() }', + `async function* foo() { + await Promise.resolve() + yield 1 + } + async function* bar() { + yield* foo() + }`, + ` + async function* run() { + await new Promise(resolve => setTimeout(resolve, 100)); + yield 'Hello'; + console.log('World'); + } + `, + 'async function* run() { }', + 'const foo : () => void = async function *(){}', + 'const foo = async function *(){ console.log("bar") }', + 'async function* run() { console.log("bar") }', ], invalid: [ From 6d8c439ed0b73c8ad5a54f31ea89f2958c1c5953 Mon Sep 17 00:00:00 2001 From: Anix Date: Wed, 25 Mar 2020 17:43:51 +0000 Subject: [PATCH 2/6] chore: added checks for async/sycn yield gen --- .../eslint-plugin/src/rules/require-await.ts | 53 ++++++++++- .../tests/rules/require-await.test.ts | 93 ++++++++++++++++++- 2 files changed, 141 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin/src/rules/require-await.ts b/packages/eslint-plugin/src/rules/require-await.ts index 876f0d60e83..d541c98b57e 100644 --- a/packages/eslint-plugin/src/rules/require-await.ts +++ b/packages/eslint-plugin/src/rules/require-await.ts @@ -11,6 +11,9 @@ interface ScopeInfo { upper: ScopeInfo | null; hasAwait: boolean; hasAsync: boolean; + isGen: boolean; + hasDelegateGen: boolean; + isAsyncYield: boolean; } type FunctionNode = | TSESTree.FunctionDeclaration @@ -49,9 +52,29 @@ export default util.createRule({ upper: scopeInfo, hasAwait: false, hasAsync: node.async, + isGen: node.generator || false, + hasDelegateGen: false, + isAsyncYield: false, }; } + /** + * Returns `true` if the node is asycn, generator and returnType is Promise + */ + function isValidAsyncGenerator(node: FunctionNode): Boolean { + const { async, generator, returnType } = node; + if (async && generator && returnType) { + if ( + !!~['Promise'].indexOf( + node.returnType?.typeAnnotation?.typeName?.name, + ) + ) { + return true; + } + } + return false; + } + /** * Pop the top scope info object from the stack. * Also, it reports the function if needed. @@ -63,10 +86,15 @@ export default util.createRule({ } if ( - !node.generator && + !isValidAsyncGenerator(node) && node.async && !scopeInfo.hasAwait && - !isEmptyFunction(node) + !isEmptyFunction(node) && + !( + scopeInfo.isGen && + scopeInfo.hasDelegateGen && + !scopeInfo.isAsyncYield + ) ) { context.report({ node, @@ -97,10 +125,28 @@ export default util.createRule({ if (!scopeInfo) { return; } - scopeInfo.hasAwait = true; } + /** + * mark `scopeInfo.hasDelegateGen` to `true` if its a generator + * function and the delegate is `true` + */ + function markAsHasDelegateGen(node: FunctionNode): void { + if (!scopeInfo || !scopeInfo.isGen) { + return; + } + + if (node?.argument?.type === AST_NODE_TYPES.TSLiteralType) { + // making this `true` as for literals we dont need to check the defination + // eg : async function* run() { yield* 1 } + scopeInfo.isAsyncYield = true; + } + + // TODO : async function* test1() {yield* asyncGenerator() } + scopeInfo.hasDelegateGen = true; + } + return { FunctionDeclaration: enterFunction, FunctionExpression: enterFunction, @@ -111,6 +157,7 @@ export default util.createRule({ AwaitExpression: markAsHasAwait, 'ForOfStatement[await = true]': markAsHasAwait, + 'FunctionDeclaration[generator = true] > BlockStatement > ExpressionStatement > YieldExpression[delegate = true]': markAsHasDelegateGen, // check body-less async arrow function. // ignore `async () => await foo` because it's obviously correct diff --git a/packages/eslint-plugin/tests/rules/require-await.test.ts b/packages/eslint-plugin/tests/rules/require-await.test.ts index 9ebb85bf6a1..7b1594908f7 100644 --- a/packages/eslint-plugin/tests/rules/require-await.test.ts +++ b/packages/eslint-plugin/tests/rules/require-await.test.ts @@ -120,9 +120,21 @@ async function testFunction(): Promise { } `, 'async function* run() { }', + 'async function* run() { yield* 1 }', + 'async function* asyncGenerator() { await Promise.resolve(); yield 1 }', + 'function* test6() { yield* syncGenerator() }', + 'function* test8() { yield syncGenerator() }', + 'function* syncGenerator() { yield 1 }', + // FIXME + ` + async function* asyncGenerator() { + await Promise.resolve() + yield 1 + } + async function* test1() {yield* asyncGenerator() } + `, 'const foo : () => void = async function *(){}', - 'const foo = async function *(){ console.log("bar") }', - 'async function* run() { console.log("bar") }', + 'async function* foo() : Promise { return new Promise((res) => res(`hello`)) }', ], invalid: [ @@ -179,6 +191,83 @@ async function testFunction(): Promise { }, ], }, + { + code: 'async function* foo() : void { doSomething() }', + errors: [ + { + messageId: 'missingAwait', + data: { + name: "Async generator function 'foo'", + }, + }, + ], + }, + { + code: 'async function* foo() { yield 1 }', + errors: [ + { + messageId: 'missingAwait', + data: { + name: "Async generator function 'foo'", + }, + }, + ], + }, + { + code: 'async function* run() { console.log("bar") }', + errors: [ + { + messageId: 'missingAwait', + data: { + name: "Async generator function 'run'", + }, + }, + ], + }, + { + code: 'const foo = async function *(){ console.log("bar") }', + errors: [ + { + messageId: 'missingAwait', + data: { + name: 'Async generator function', + }, + }, + ], + }, + { + code: 'async function* syncGenerator() { yield 1 }', + errors: [ + { + messageId: 'missingAwait', + data: { + name: "Async generator function 'syncGenerator'", + }, + }, + ], + }, + { + code: 'async function* test3() { yield asyncGenerator() }', + errors: [ + { + messageId: 'missingAwait', + data: { + name: "Async generator function 'test3'", + }, + }, + ], + }, + { + code: 'async function* test7() { yield syncGenerator() }', + errors: [ + { + messageId: 'missingAwait', + data: { + name: "Async generator function 'test7'", + }, + }, + ], + }, ], }); From 250ab628e16b0d9b8258c0b810fc141c748f6b24 Mon Sep 17 00:00:00 2001 From: Anix Date: Fri, 27 Mar 2020 20:08:49 +0000 Subject: [PATCH 3/6] chore: fixed false negatives --- .../eslint-plugin/src/rules/require-await.ts | 24 +-- .../tests/rules/require-await.test.ts | 160 +++++++++--------- 2 files changed, 93 insertions(+), 91 deletions(-) diff --git a/packages/eslint-plugin/src/rules/require-await.ts b/packages/eslint-plugin/src/rules/require-await.ts index d541c98b57e..c72564aaadb 100644 --- a/packages/eslint-plugin/src/rules/require-await.ts +++ b/packages/eslint-plugin/src/rules/require-await.ts @@ -12,7 +12,6 @@ interface ScopeInfo { hasAwait: boolean; hasAsync: boolean; isGen: boolean; - hasDelegateGen: boolean; isAsyncYield: boolean; } type FunctionNode = @@ -53,7 +52,6 @@ export default util.createRule({ hasAwait: false, hasAsync: node.async, isGen: node.generator || false, - hasDelegateGen: false, isAsyncYield: false, }; } @@ -90,11 +88,7 @@ export default util.createRule({ node.async && !scopeInfo.hasAwait && !isEmptyFunction(node) && - !( - scopeInfo.isGen && - scopeInfo.hasDelegateGen && - !scopeInfo.isAsyncYield - ) + !(scopeInfo.isGen && scopeInfo.isAsyncYield) ) { context.report({ node, @@ -129,7 +123,7 @@ export default util.createRule({ } /** - * mark `scopeInfo.hasDelegateGen` to `true` if its a generator + * mark `scopeInfo.isAsyncYield` to `true` if its a generator * function and the delegate is `true` */ function markAsHasDelegateGen(node: FunctionNode): void { @@ -137,14 +131,20 @@ export default util.createRule({ return; } - if (node?.argument?.type === AST_NODE_TYPES.TSLiteralType) { + if (node?.argument?.type === AST_NODE_TYPES.Literal) { // making this `true` as for literals we dont need to check the defination // eg : async function* run() { yield* 1 } scopeInfo.isAsyncYield = true; } - // TODO : async function* test1() {yield* asyncGenerator() } - scopeInfo.hasDelegateGen = true; + const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node?.argument); + const type = checker.getTypeAtLocation(tsNode); + const symbol = type.getSymbol(); + + // async function* test1() {yield* asyncGenerator() } + if (symbol?.getName() === 'AsyncGenerator') { + scopeInfo.isAsyncYield = true; + } } return { @@ -157,7 +157,7 @@ export default util.createRule({ AwaitExpression: markAsHasAwait, 'ForOfStatement[await = true]': markAsHasAwait, - 'FunctionDeclaration[generator = true] > BlockStatement > ExpressionStatement > YieldExpression[delegate = true]': markAsHasDelegateGen, + 'YieldExpression[delegate = true]': markAsHasDelegateGen, // check body-less async arrow function. // ignore `async () => await foo` because it's obviously correct diff --git a/packages/eslint-plugin/tests/rules/require-await.test.ts b/packages/eslint-plugin/tests/rules/require-await.test.ts index 7b1594908f7..ed8be9936d8 100644 --- a/packages/eslint-plugin/tests/rules/require-await.test.ts +++ b/packages/eslint-plugin/tests/rules/require-await.test.ts @@ -16,116 +16,107 @@ ruleTester.run('require-await', rule, { valid: [ // Non-async function declaration `function numberOne(): number { - return 1; - }`, + return 1; + }`, // Non-async function expression `const numberOne = function(): number { - return 1; - }`, + return 1; + }`, // Non-async arrow function expression (concise-body) `const numberOne = (): number => 1;`, // Non-async arrow function expression (block-body) `const numberOne = (): number => { - return 1; - };`, + return 1; + };`, // Non-async function that returns a promise // https://github.com/typescript-eslint/typescript-eslint/issues/1226 ` -function delay() { - return Promise.resolve(); -} - `, + function delay() { + return Promise.resolve(); + } + `, ` -const delay = () => { - return Promise.resolve(); -} - `, + const delay = () => { + return Promise.resolve(); + } + `, `const delay = () => Promise.resolve();`, // Async function declaration with await `async function numberOne(): Promise { - return await 1; - }`, + return await 1; + }`, // Async function expression with await `const numberOne = async function(): Promise { - return await 1; - }`, + return await 1; + }`, // Async arrow function expression with await (concise-body) `const numberOne = async (): Promise => await 1;`, // Async arrow function expression with await (block-body) `const numberOne = async (): Promise => { - return await 1; - };`, + return await 1; + };`, // Async function declaration with promise return `async function numberOne(): Promise { - return Promise.resolve(1); - }`, + return Promise.resolve(1); + }`, // Async function expression with promise return `const numberOne = async function(): Promise { - return Promise.resolve(1); - }`, + return Promise.resolve(1); + }`, // Async arrow function with promise return (concise-body) `const numberOne = async (): Promise => Promise.resolve(1);`, // Async arrow function with promise return (block-body) `const numberOne = async (): Promise => { - return Promise.resolve(1); - };`, + return Promise.resolve(1); + };`, // Async function declaration with async function return `async function numberOne(): Promise { - return getAsyncNumber(1); - } - async function getAsyncNumber(x: number): Promise { - return Promise.resolve(x); - }`, + return getAsyncNumber(1); + } + async function getAsyncNumber(x: number): Promise { + return Promise.resolve(x); + }`, // Async function expression with async function return `const numberOne = async function(): Promise { - return getAsyncNumber(1); - } - const getAsyncNumber = async function(x: number): Promise { - return Promise.resolve(x); - }`, + return getAsyncNumber(1); + } + const getAsyncNumber = async function(x: number): Promise { + return Promise.resolve(x); + }`, // Async arrow function with async function return (concise-body) `const numberOne = async (): Promise => getAsyncNumber(1); - const getAsyncNumber = async function(x: number): Promise { - return Promise.resolve(x); - }`, + const getAsyncNumber = async function(x: number): Promise { + return Promise.resolve(x); + }`, // Async arrow function with async function return (block-body) `const numberOne = async (): Promise => { - return getAsyncNumber(1); - }; - const getAsyncNumber = async function(x: number): Promise { - return Promise.resolve(x); - }`, + return getAsyncNumber(1); + }; + const getAsyncNumber = async function(x: number): Promise { + return Promise.resolve(x); + }`, // https://github.com/typescript-eslint/typescript-eslint/issues/1188 ` -async function testFunction(): Promise { - await Promise.all([1, 2, 3].map( - // this should not trigger an error on the parent function - async value => Promise.resolve(value) - )) -} - `, - 'async function* run() { yield * anotherAsyncGenerator() }', - `async function* foo() { - await Promise.resolve() - yield 1 - } - async function* bar() { - yield* foo() - }`, - ` - async function* run() { - await new Promise(resolve => setTimeout(resolve, 100)); - yield 'Hello'; - console.log('World'); + async function testFunction(): Promise { + await Promise.all([1, 2, 3].map( + // this should not trigger an error on the parent function + async value => Promise.resolve(value) + )) } - `, + `, + ` + async function* run() { + await new Promise(resolve => setTimeout(resolve, 100)); + yield 'Hello'; + console.log('World'); + } + `, 'async function* run() { }', 'async function* run() { yield* 1 }', 'async function* asyncGenerator() { await Promise.resolve(); yield 1 }', 'function* test6() { yield* syncGenerator() }', 'function* test8() { yield syncGenerator() }', 'function* syncGenerator() { yield 1 }', - // FIXME ` async function* asyncGenerator() { await Promise.resolve() @@ -133,6 +124,13 @@ async function testFunction(): Promise { } async function* test1() {yield* asyncGenerator() } `, + `async function* foo() { + await Promise.resolve() + yield 1 + } + async function* bar() { + yield* foo() + }`, 'const foo : () => void = async function *(){}', 'async function* foo() : Promise { return new Promise((res) => res(`hello`)) }', ], @@ -141,8 +139,8 @@ async function testFunction(): Promise { { // Async function declaration with no await code: `async function numberOne(): Promise { - return 1; - }`, + return 1; + }`, errors: [ { messageId: 'missingAwait', @@ -155,8 +153,8 @@ async function testFunction(): Promise { { // Async function expression with no await code: `const numberOne = async function(): Promise { - return 1; - }`, + return 1; + }`, errors: [ { messageId: 'missingAwait', @@ -270,7 +268,6 @@ async function testFunction(): Promise { }, ], }); - // base eslint tests // https://github.com/eslint/eslint/blob/03a69dbe86d5b5768a310105416ae726822e3c1c/tests/lib/rules/require-await.js#L25-L132 ruleTester.run('require-await', rule, { @@ -283,27 +280,23 @@ ruleTester.run('require-await', rule, { 'class A { async foo() { await doSomething() } }', '(class { async foo() { await doSomething() } })', 'async function foo() { await (async () => { await doSomething() }) }', - // empty functions are ok. 'async function foo() {}', 'async () => {}', - // normal functions are ok. 'function foo() { doSomething() }', - // for-await-of 'async function foo() { for await (x of xs); }', - // global await { code: 'await foo()', }, { code: ` - for await (let num of asyncIterable) { - console.log(num); - } - `, + for await (let num of asyncIterable) { + console.log(num); + } + `, }, ], invalid: [ @@ -397,5 +390,14 @@ ruleTester.run('require-await', rule, { }, ], }, + { + code: 'async function* run() { yield * anotherAsyncGenerator() }', + errors: [ + { + messageId: 'missingAwait', + data: { name: "Async generator function 'run'" }, + }, + ], + }, ], }); From 8cdec957f56581c899ea6ce3334dee48aa6161db Mon Sep 17 00:00:00 2001 From: Anix Date: Sun, 29 Mar 2020 16:05:17 +0000 Subject: [PATCH 4/6] chore: cleanup, removed un-neccesary functions --- .../eslint-plugin/src/rules/require-await.ts | 22 ++----------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/packages/eslint-plugin/src/rules/require-await.ts b/packages/eslint-plugin/src/rules/require-await.ts index c72564aaadb..09effc56a92 100644 --- a/packages/eslint-plugin/src/rules/require-await.ts +++ b/packages/eslint-plugin/src/rules/require-await.ts @@ -56,23 +56,6 @@ export default util.createRule({ }; } - /** - * Returns `true` if the node is asycn, generator and returnType is Promise - */ - function isValidAsyncGenerator(node: FunctionNode): Boolean { - const { async, generator, returnType } = node; - if (async && generator && returnType) { - if ( - !!~['Promise'].indexOf( - node.returnType?.typeAnnotation?.typeName?.name, - ) - ) { - return true; - } - } - return false; - } - /** * Pop the top scope info object from the stack. * Also, it reports the function if needed. @@ -84,7 +67,6 @@ export default util.createRule({ } if ( - !isValidAsyncGenerator(node) && node.async && !scopeInfo.hasAwait && !isEmptyFunction(node) && @@ -126,8 +108,8 @@ export default util.createRule({ * mark `scopeInfo.isAsyncYield` to `true` if its a generator * function and the delegate is `true` */ - function markAsHasDelegateGen(node: FunctionNode): void { - if (!scopeInfo || !scopeInfo.isGen) { + function markAsHasDelegateGen(node: TSESTree.YieldExpression): void { + if (!scopeInfo || !scopeInfo.isGen || !node.argument) { return; } From c9c14bd49b742fb7c02f392ef6c8823d2dba8548 Mon Sep 17 00:00:00 2001 From: Anix Date: Mon, 30 Mar 2020 03:54:28 +0000 Subject: [PATCH 5/6] chore: fixed false positive for yield literal --- packages/eslint-plugin/src/rules/require-await.ts | 4 ++-- .../eslint-plugin/tests/rules/require-await.test.ts | 10 +++++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/src/rules/require-await.ts b/packages/eslint-plugin/src/rules/require-await.ts index 09effc56a92..f1bedcdb8b0 100644 --- a/packages/eslint-plugin/src/rules/require-await.ts +++ b/packages/eslint-plugin/src/rules/require-await.ts @@ -114,9 +114,9 @@ export default util.createRule({ } if (node?.argument?.type === AST_NODE_TYPES.Literal) { - // making this `true` as for literals we dont need to check the defination + // making this `false` as for literals we dont need to check the defination // eg : async function* run() { yield* 1 } - scopeInfo.isAsyncYield = true; + scopeInfo.isAsyncYield = false; } const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node?.argument); diff --git a/packages/eslint-plugin/tests/rules/require-await.test.ts b/packages/eslint-plugin/tests/rules/require-await.test.ts index ed8be9936d8..caea93fbfb9 100644 --- a/packages/eslint-plugin/tests/rules/require-await.test.ts +++ b/packages/eslint-plugin/tests/rules/require-await.test.ts @@ -112,7 +112,6 @@ ruleTester.run('require-await', rule, { } `, 'async function* run() { }', - 'async function* run() { yield* 1 }', 'async function* asyncGenerator() { await Promise.resolve(); yield 1 }', 'function* test6() { yield* syncGenerator() }', 'function* test8() { yield syncGenerator() }', @@ -399,5 +398,14 @@ ruleTester.run('require-await', rule, { }, ], }, + { + code: 'async function* run() { yield* 1 }', + errors: [ + { + messageId: 'missingAwait', + data: { name: "Async generator function 'run'" }, + }, + ], + }, ], }); From 5f5f93464c7c744399c7271f061aae7f74ba8051 Mon Sep 17 00:00:00 2001 From: Anix Date: Mon, 30 Mar 2020 04:09:14 +0000 Subject: [PATCH 6/6] chore: fixed spell checks --- packages/eslint-plugin/src/rules/require-await.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/require-await.ts b/packages/eslint-plugin/src/rules/require-await.ts index f1bedcdb8b0..99c455cd0f6 100644 --- a/packages/eslint-plugin/src/rules/require-await.ts +++ b/packages/eslint-plugin/src/rules/require-await.ts @@ -114,7 +114,7 @@ export default util.createRule({ } if (node?.argument?.type === AST_NODE_TYPES.Literal) { - // making this `false` as for literals we dont need to check the defination + // making this `false` as for literals we don't need to check the definition // eg : async function* run() { yield* 1 } scopeInfo.isAsyncYield = false; }