8000 feat(eslint-plugin): [return-await] add option to report in error-han… · abrahamguo/typescript-eslint@970f3f1 · GitHub
[go: up one dir, main page]
Skip to content

Commit 970f3f1

Browse files
kirkwaiblingerJoshuaKGoldberg
kirkwaiblinger
and
JoshuaKGoldberg
authored
feat(eslint-plugin): [return-await] add option to report in error-handling scenarios only, and deprecate "never" (typescript-eslint#9364)
* first stab * fix CI stuff * Update packages/eslint-plugin/src/rules/return-await.ts Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> * change comments and stuff * add table to docs * dry it up * revert useAutoFix * Update packages/eslint-plugin/docs/rules/return-await.mdx Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> * to awaiting * changes and deprecation * mention motivation for each option in its section * subjunctive * Update packages/eslint-plugin/docs/rules/return-await.mdx Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> * move things out of closure --------- Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
1 parent dd965a4 commit 970f3f1

File tree

5 files changed

+241
-222
lines changed

5 files changed

+241
-222
lines changed

‎packages/eslint-plugin/docs/rules/return-await.mdx

Lines changed: 73 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -21,23 +21,43 @@ The extended rule is named `return-await` instead of `no-return-await` because t
2121
## Options
2222

2323
```ts
24-
type Options = 'in-try-catch' | 'always' | 'never';
24+
type Options =
25+
| 'in-try-catch'
26+
| 'always'
27+
| 'error-handling-correctness-only'
28+
| 'never';
2529

2630
const defaultOptions: Options = 'in-try-catch';
2731
```
2832

33+
The options in this rule distinguish between "ordinary contexts" and "error-handling contexts".
34+
An error-handling context is anywhere where returning an unawaited promise would cause unexpected control flow regarding exceptions/rejections.
35+
See detailed examples in the sections for each option.
36+
37+
- If you return a promise within a `try` block, it should be awaited in order to trigger subsequent `catch` or `finally` blocks as expected.
38+
- If you return a promise within a `catch` block, and there _is_ a `finally` block, it should be awaited in order to trigger the `finally` block as expected.
39+
- If you return a promise between a `using` or `await using` declaration and the end of its scope, it should be awaited, since it behaves equivalently to code wrapped in a `try` block followed by a `finally`.
40+
41+
Ordinary contexts are anywhere else a promise may be returned.
42+
The choice of whether to await a returned promise in an ordinary context is mostly stylistic.
43+
44+
With these terms defined, the options may be summarized as follows:
45+
46+
| Option | Ordinary Context <br/> (stylistic preference 🎨) | Error-Handling Context <br/> (catches bugs 🐛) | Should I use this option? |
47+
| :-------------------------------: | :----------------------------------------------: | :----------------------------------------------------------: | :--------------------------------------------------------: |
48+
| `always` | `return await promise;` | `return await promise;` | ✅ Yes! |
49+
| `in-try-catch` | `return promise;` | `return await promise;` | ✅ Yes! |
50+
| `error-handling-correctness-only` | don't care 🤷 | `return await promise;` | 🟡 Okay to use, but the above options would be preferable. |
51+
| `never` | `return promise;` | `return promise;` <br/> (⚠️ This behavior may be harmful ⚠️) | ❌ No. This option is deprecated. |
52+
2953
### `in-try-catch`
3054

31-
In cases where returning an unawaited promise would cause unexpected error-handling control flow, the rule enforces that `await` must be used.
32-
Otherwise, the rule enforces that `await` must _not_ be used.
55+
In error-handling contexts, the rule enforces that returned promises must be awaited.
56+
In ordinary contexts, the rule enforces that returned promises _must not_ be awaited.
3357

34-
Listing the error-handling cases exhaustively:
58+
This is a good option if you prefer the shorter `return promise` form for stylistic reasons, wherever it's safe to use.
3559

36-
- if you `return` a promise within a `try`, then it must be `await`ed, since it will always be followed by a `catch` or `finally`.
37-
- if you `return` a promise within a `catch`, and there is _no_ `finally`, then it must _not_ be `await`ed.
38-
- if you `return` a promise within a `catch`, and there _is_ a `finally`, then it _must_ be `await`ed.
39-
- if you `return` a promise within a `finally`, then it must not be `await`ed.
40-
- if you `return` a promise between a `using` or `await using` declaration and the end of its scope, it must be `await`ed, since it behaves equivalently to code wrapped in a `try` block.
60+
Examples of code with `in-try-catch`:
4161

4262
<Tabs>
4363
<TabItem value="❌ Incorrect">
@@ -169,7 +189,9 @@ async function validInTryCatch7() {
169189

170190
### `always`
171191

172-
Requires that all returned promises are `await`ed.
192+
Requires that all returned promises be awaited.
193+
194+
This is a good option if you like the consistency of simply always awaiting promises, or prefer not having to consider the distinction between error-handling contexts and ordinary contexts.
173195

174196
Examples of code with `always`:
175197

@@ -214,9 +236,49 @@ async function validAlways3() {
214236
</TabItem>
215237
</Tabs>
216238

239+
### `error-handling-correctness-only`
240+
241+
In error-handling contexts, the rule enforces that returned promises must be awaited.
242+
In ordinary contexts, the rule does not enforce any particular behavior around whether returned promises are awaited.
243+
244+
This is a good option if you only want to benefit from rule's ability to catch control flow bugs in error-handling contexts, but don't want to enforce a particular style otherwise.
245+
246+
:::info
247+
We recommend you configure either `in-try-catch` or `always` instead of this option.
248+
While the choice of whether to await promises outside of error-handling contexts is mostly stylistic, it's generally best to be consistent.
249+
:::
250+
251+
Examples of additional correct code with `error-handling-correctness-only`:
252+
253+
<Tabs>
254+
<TabItem value="✅ Correct">
255+
256+
```ts option='"error-handling-correctness-only"'
257+
async function asyncFunction(): Promise<void> {
258+
if (Math.random() < 0.5) {
259+
return await Promise.resolve();
260+
} else {
261+
return Promise.resolve();
262+
}
263+
}
264+
```
265+
266+
</TabItem>
267+
</Tabs>
268+
217269
### `never`
218270

219-
Disallows all `await`ing any returned promises.
271+
Disallows awaiting any returned promises.
272+
273+
:::warning
274+
275+
This option is deprecated and will be removed in a future major version of typescript-eslint.
276+
277+
The `never` option introduces undesirable behavior in error-handling contexts.
278+
If you prefer to minimize returning awaited promises, consider instead using `in-try-catch` instead, which also generally bans returning awaited promises, but only where it is _safe_ not to await a promise.
279+
280+
See more details at [typescript-eslint#9433](https://github.com/typescript-eslint/typescript-eslint/issues/9433).
281+
:::
220282

221283
Examples of code with `never`:
222284

‎packages/eslint-plugin/src/rules/return-await.ts

Lines changed: 94 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,12 @@ interface ScopeInfo {
2424
owningFunc: FunctionNode;
2525
}
2626

27+
type Option =
28+
| 'in-try-catch'
29+
| 'always'
30+
| 'never'
31+
| 'error-handling-correctness-only';
32+
2733
export default createRule({
2834
name: 'return-await',
2935
meta: {
@@ -50,7 +56,12 @@ export default createRule({
5056
schema: [
5157
{
5258
type: 'string',
53-
enum: ['in-try-catch', 'always', 'never'],
59+
enum: [
60+
'in-try-catch',
61+
'always',
62+
'never',
63+
'error-handling-correctness-only',
64+
] satisfies Option[],
5465
},
5566
],
5667
},
@@ -271,92 +282,70 @@ export default createRule({
271282
const type = checker.getTypeAtLocation(child);
272283
const isThenable = tsutils.isThenableType(checker, expression, type);
273284

274-
if (!isAwait && !isThenable) {
275-
return;
276-
}
277-
278-
if (isAwait && !isThenable) {
279-
// any/unknown could be thenable; do not auto-fix
280-
const useAutoFix = !(isTypeAnyType(type) || isTypeUnknownType(type));
281-
282-
context.report({
283-
messageId: 'nonPromiseAwait',
284-
node,
285-
...fixOrSuggest(useAutoFix, {
286-
messageId: 'nonPromiseAwait',
287-
fix: fixer => removeAwait(fixer, node),
288-
}),
289-
});
290-
return;
291-
}
285+
// handle awaited _non_thenables
292286

293-
const affectsErrorHandling =
294-
affectsExplicitErrorHandling(expression) ||
295-
affectsExplicitResourceManagement(node);
296-
const useAutoFix = !affectsErrorHandling;
287+
if (!isThenable) {
288+
if (isAwait) {
289+
// any/unknown could be thenable; do not auto-fix
290+
const useAutoFix = !(isTypeAnyType(type) || isTypeUnknownType(type));
297291

298-
if (option === 'always') {
299-
if (!isAwait && isThenable) {
300292
context.report({
301-
messageId: 'requiredPromiseAwait',
293+
messageId: 'nonPromiseAwait',
302294
node,
303295
...fixOrSuggest(useAutoFix, {
304-
messageId: 'requiredPromiseAwaitSuggestion',
305-
fix: fixer =>
306-
insertAwait(
307-
fixer,
308-
node,
309-
isHigherPrecedenceThanAwait(expression),
310-
),
296+
messageId: 'nonPromiseAwait',
297+
fix: fixer => removeAwait(fixer, node),
311298
}),
312299
});
313300
}
314-
315301
return;
316302
}
317303

318-
if (option === 'never') {
319-
if (isAwait) {
320-
context.report({
321-
messageId: 'disallowedPromiseAwait',
322-
node,
323-
...fixOrSuggest(useAutoFix, {
324-
messageId: 'disallowedPromiseAwaitSuggestion',
325-
fix: fixer => removeAwait(fixer, node),
326-
}),
327-
});
328-
}
304+
// At this point it's definitely a thenable.
329305

330-
return;
331-
}
306+
const affectsErrorHandling =
307+
affectsExplicitErrorHandling(expression) ||
308+
affectsExplicitResourceManagement(node);
309+
const useAutoFix = !affectsErrorHandling;
332310

333-
if (option === 'in-try-catch') {
334-
if (isAwait && !affectsErrorHandling) {
335-
context.report({
336-
messageId: 'disallowedPromiseAwait',
337-
node,
338-
...fixOrSuggest(useAutoFix, {
339-
messageId: 'disallowedPromiseAwaitSuggestion',
340-
fix: fixer => removeAwait(fixer, node),
341-
}),
342-
});
343-
} else if (!isAwait && affectsErrorHandling) {
344-
context.report({
345-
messageId: 'requiredPromiseAwait',
346-
node,
347-
...fixOrSuggest(useAutoFix, {
348-
messageId: 'requiredPromiseAwaitSuggestion',
349-
fix: fixer =>
350-
insertAwait(
351-
fixer,
352-
node,
353-
isHigherPrecedenceThanAwait(expression),
354-
),
355-
}),
356-
});
357-
}
311+
const ruleConfiguration = getConfiguration(option as Option);
358312

359-
return;
313+
const shouldAwaitInCurrentContext = affectsErrorHandling
314+
? ruleConfiguration.errorHandlingContext
315+
: ruleConfiguration.ordinaryContext;
316+
317+
switch (shouldAwaitInCurrentContext) {
318+
case "don't-care":
319+
break;
320+
case 'await':
321+
if (!isAwait) {
322+
context.report({
323+
messageId: 'requiredPromiseAwait',
324+
node,
325+
...fixOrSuggest(useAutoFix, {
326+
messageId: 'requiredPromiseAwaitSuggestion',
327+
fix: fixer =>
328+
insertAwait(
329+
fixer,
330+
node,
331+
isHigherPrecedenceThanAwait(expression),
332+
),
333+
}),
334+
});
335+
}
336+
break;
337+
case 'no-await':
338+
if (isAwait) {
339+
context.report({
340+
messageId: 'disallowedPromiseAwait',
341+
node,
342+
...fixOrSuggest(useAutoFix, {
343+
messageId: 'disallowedPromiseAwaitSuggestion',
344+
fix: fixer => removeAwait(fixer, node),
345+
}),
346+
});
347+
}
348+
break;
360349
}
361350
}
362351

@@ -406,6 +395,38 @@ export default createRule({
406395
},
407396
});
408397

398+
type WhetherToAwait = 'await' | 'no-await' | "don't-care";
399+
400+
interface RuleConfiguration {
401+
ordinaryContext: WhetherToAwait;
402+
errorHandlingContext: WhetherToAwait;
403+
}
404+
405+
function getConfiguration(option: Option): RuleConfiguration {
406+
switch (option) {
407+
case 'always':
408+
return {
409+
ordinaryContext: 'await',
410+
errorHandlingContext: 'await',
411+
};
412+
case 'never':
413+
return {
414+
ordinaryContext: 'no-await',
415+
errorHandlingContext: 'no-await',
416+
};
417+
case 'error-handling-correctness-only':
418+
return {
419+
ordinaryContext: "don't-care",
420+
errorHandlingContext: 'await',
421+
};
422+
case 'in-try-catch':
423+
return {
424+
ordinaryContext: 'no-await',
425+
errorHandlingContext: 'await',
426+
};
427+
}
428+
}
429+
409430
function fixOrSuggest<MessageId extends string>(
410431
useFix: boolean,
411432
suggestion: TSESLint.SuggestionReportDescriptor<MessageId>,

‎packages/eslint-plugin/tests/docs-eslint-output-snapshots/return-await.shot

Lines changed: 15 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)
0