Skip to content

feat(eslint-plugin): [await-thenable] report unnecessary await using statements #10209

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 57 additions & 1 deletion packages/eslint-plugin/docs/rules/await-thenable.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ This rule also inspects [`for await...of` statements](https://developer.mozilla.
While `for await...of` can be used with synchronous iterables, and it will await each promise produced by the iterable, it is inadvisable to do so.
There are some tiny nuances that you may want to consider.

The biggest difference between using `for await...of` and using `for...of` (plus awaiting each result yourself) is error handling.
The biggest difference between using `for await...of` and using `for...of` (apart from awaiting each result yourself) is error handling.
When an error occurs within the loop body, `for await...of` does _not_ close the original sync iterable, while `for...of` does.
For detailed examples of this, see the [MDN documentation on using `for await...of` with sync-iterables](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for-await...of#iterating_over_sync_iterables_and_generators).

Expand Down Expand Up @@ -120,6 +120,62 @@ async function validUseOfForAwaitOnAsyncIterable() {
</TabItem>
</Tabs>

## Explicit Resource Management (`await using` Statements)

This rule also inspects [`await using` statements](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-2.html#using-declarations-and-explicit-resource-management).
If the disposable being used is not async-disposable, an `await using` statement is unnecessary.

### Examples

<Tabs>
<TabItem value="❌ Incorrect">

```ts
function makeSyncDisposable(): Disposable {
return {
[Symbol.dispose](): void {
// Dispose of the resource
},
};
}

async function shouldNotAwait() {
await using resource = makeSyncDisposable();
}
```

</TabItem>
<TabItem value="✅ Correct">

```ts
function makeSyncDisposable(): Disposable {
return {
[Symbol.dispose](): void {
// Dispose of the resource
},
};
}

async function shouldNotAwait() {
using resource = makeSyncDisposable();
}

function makeAsyncDisposable(): AsyncDisposable {
return {
async [Symbol.asyncDispose](): Promise<void> {
// Dispose of the resource asynchronously
},
};
}

async function shouldAwait() {
await using resource = makeAsyncDisposable();
}
```

</TabItem>
</Tabs>

## When Not To Use It

If you want to allow code to `await` non-Promise values.
Expand Down
83 changes: 69 additions & 14 deletions packages/eslint-plugin/src/rules/await-thenable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as tsutils from 'ts-api-utils';

import {
createRule,
getFixOrSuggest,
getParserServices,
isAwaitKeyword,
isTypeAnyType,
Expand All @@ -15,8 +16,9 @@ import { getForStatementHeadLoc } from '../util/getForStatementHeadLoc';

type MessageId =
| 'await'
| 'awaitUsingOfNonAsyncDisposable'
| 'convertToOrdinaryFor'
| 'forAwaitOfNonThenable'
| 'forAwaitOfNonAsyncIterable'
| 'removeAwait';

export default createRule<[], MessageId>({
Expand All @@ -31,8 +33,10 @@ export default createRule<[], MessageId>({
hasSuggestions: true,
messages: {
await: 'Unexpected `await` of a non-Promise (non-"Thenable") value.',
awaitUsingOfNonAsyncDisposable:
'Unexpected `await using` of a value that is not async disposable.',
convertToOrdinaryFor: 'Convert to an ordinary `for...of` loop.',
forAwaitOfNonThenable:
forAwaitOfNonAsyncIterable:
'Unexpected `for await...of` of a value that is not async iterable.',
removeAwait: 'Remove unnecessary `await`.',
},
Expand Down Expand Up @@ -80,21 +84,21 @@ export default createRule<[], MessageId>({
return;
}

const asyncIteratorSymbol = tsutils
const hasAsyncIteratorSymbol = tsutils
.unionTypeParts(type)
.map(t =>
tsutils.getWellKnownSymbolPropertyOfType(
t,
'asyncIterator',
checker,
),
)
.find(symbol => symbol != null);

if (asyncIteratorSymbol == null) {
.some(
typePart =>
tsutils.getWellKnownSymbolPropertyOfType(
typePart,
'asyncIterator',
checker,
) != null,
);

if (!hasAsyncIteratorSymbol) {
context.report({
loc: getForStatementHeadLoc(context.sourceCode, node),
messageId: 'forAwaitOfNonThenable',
messageId: 'forAwaitOfNonAsyncIterable',
suggest: [
// Note that this suggestion causes broken code for sync iterables
// of promises, since the loop variable is not awaited.
Expand All @@ -112,6 +116,57 @@ export default createRule<[], MessageId>({
});
}
},

'VariableDeclaration[kind="await using"]'(
node: TSESTree.VariableDeclaration,
): void {
for (const declarator of node.declarations) {
const init = declarator.init;
if (init == null) {
continue;
}
const type = services.getTypeAtLocation(init);
if (isTypeAnyType(type)) {
continue;
}

const hasAsyncDisposeSymbol = tsutils
.unionTypeParts(type)
.some(
typePart =>
tsutils.getWellKnownSymbolPropertyOfType(
typePart,
'asyncDispose',
checker,
) != null,
);

if (!hasAsyncDisposeSymbol) {
context.report({
node: init,
messageId: 'awaitUsingOfNonAsyncDisposable',
// let the user figure out what to do if there's
// await using a = b, c = d, e = f;
// it's rare and not worth the complexity to handle.
...getFixOrSuggest({
fixOrSuggest:
node.declarations.length === 1 ? 'suggest' : 'none',

suggestion: {
messageId: 'removeAwait',
fix(fixer): TSESLint.RuleFix {
const awaitToken = nullThrows(
context.sourceCode.getFirstToken(node, isAwaitKeyword),
NullThrowsReasons.MissingToken('await', 'await using'),
);
return fixer.remove(awaitToken);
},
},
}),
});
}
}
},
};
},
});
Original file line number Diff line number Diff line change
Expand Up @@ -413,11 +413,11 @@ function getReportDescriptor(
},
messageId: 'preferOptionalChain',
...getFixOrSuggest({
fixOrSuggest: useSuggestionFixer ? 'suggest' : 'fix',
suggestion: {
fix,
messageId: 'optionalChainSuggest',
},
useFix: !useSuggestionFixer,
}),
};

Expand Down
39 changes: 19 additions & 20 deletions packages/eslint-plugin/src/rules/return-await.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import * as ts from 'typescript';

import {
createRule,
getFixOrSuggest,
getParserServices,
isAwaitExpression,
isAwaitKeyword,
Expand Down Expand Up @@ -44,6 +45,7 @@ export default createRule({
requiresTypeChecking: true,
},
fixable: 'code',
// eslint-disable-next-line eslint-plugin/require-meta-has-suggestions -- suggestions are exposed through a helper.
hasSuggestions: true,
messages: {
disallowedPromiseAwait:
Expand Down Expand Up @@ -340,14 +342,17 @@ export default createRule({
context.report({
node,
messageId: 'requiredPromiseAwait',
...fixOrSuggest(useAutoFix, {
messageId: 'requiredPromiseAwaitSuggestion',
fix: fixer =>
insertAwait(
fixer,
node,
isHigherPrecedenceThanAwait(expression),
),
...getFixOrSuggest({
fixOrSuggest: useAutoFix ? 'fix' : 'suggest',
suggestion: {
messageId: 'requiredPromiseAwaitSuggestion',
fix: fixer =>
insertAwait(
fixer,
node,
isHigherPrecedenceThanAwait(expression),
),
},
}),
});
}
Expand All @@ -359,9 +364,12 @@ export default createRule({
context.report({
node,
messageId: 'disallowedPromiseAwait',
...fixOrSuggest(useAutoFix, {
messageId: 'disallowedPromiseAwaitSuggestion',
fix: fixer => removeAwait(fixer, node),
...getFixOrSuggest({
fixOrSuggest: useAutoFix ? 'fix' : 'suggest',
suggestion: {
messageId: 'disallowedPromiseAwaitSuggestion',
fix: fixer => removeAwait(fixer, node),
},
}),
});
}
Expand Down Expand Up @@ -446,12 +454,3 @@ function getConfiguration(option: Option): RuleConfiguration {
};
}
}

function fixOrSuggest<MessageId extends string>(
useFix: boolean,
suggestion: TSESLint.SuggestionReportDescriptor<MessageId>,
):
| { fix: TSESLint.ReportFixFunction }
| { suggest: TSESLint.SuggestionReportDescriptor<MessageId>[] } {
return useFix ? { fix: suggestion.fix } : { suggest: [suggestion] };
}
16 changes: 12 additions & 4 deletions packages/eslint-plugin/src/util/getFixOrSuggest.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,21 @@
import type { TSESLint } from '@typescript-eslint/utils';

export function getFixOrSuggest<MessageId extends string>({
fixOrSuggest,
suggestion,
useFix,
}: {
fixOrSuggest: 'fix' | 'none' | 'suggest';
suggestion: TSESLint.SuggestionReportDescriptor<MessageId>;
useFix: boolean;
}):
| { fix: TSESLint.ReportFixFunction }
| { suggest: TSESLint.SuggestionReportDescriptor<MessageId>[] } {
return useFix ? { fix: suggestion.fix } : { suggest: [suggestion] };
| { suggest: TSESLint.SuggestionReportDescriptor<MessageId>[] }
| undefined {
switch (fixOrSuggest) {
case 'fix':
return { fix: suggestion.fix };
case 'none':
return undefined;
case 'suggest':
return { suggest: [suggestion] };
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions packages/eslint-plugin/tests/docs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,9 +252,10 @@ describe('Validating rule docs', () => {
// Get all H2 headings objects as the other levels are variable by design.
const headings = tokens.filter(tokenIsH2);

headings.forEach(heading =>
expect(heading.text).toBe(titleCase(heading.text)),
);
headings.forEach(heading => {
const nonCodeText = heading.text.replace(/`[^`]*`/g, '');
expect(nonCodeText).toBe(titleCase(nonCodeText));
});
});

const headings = tokens.filter(tokenIsHeading);
Expand Down
Loading
Loading