fix(eslint-plugin): [return-await] clean up in-try-catch detection and make autofixes safe (#9031)

* [no-return-await] Clean up the logic for in-try-catch detection and make autofixes safe

* spell

* snapshot

* nicer diff

* adjust unreachable case

Author: kirkwaiblinger

Diff for: .cspell.json

@@ -149,6 +149,7 @@
     "tsvfs",
     "typedef",
     "typedefs",
+    "unawaited",
     "unfixable",
     "unoptimized",
     "unprefixed",

Diff for: packages/eslint-plugin/docs/rules/return-await.mdx

@@ -26,13 +26,15 @@ const defaultOptions: Options = 'in-try-catch';
 
 ### `in-try-catch`
 
-Requires that a returned promise must be `await`ed in `try-catch-finally` blocks, and disallows it elsewhere.
-Specifically:
+In cases where returning an unawaited promise would cause unexpected error-handling control flow, the rule enforces that `await` must be used.
+Otherwise, the rule enforces that `await` must _not_ be used.
 
-- if you `return` a promise within a `try`, then it must be `await`ed.
-- if you `return` a promise within a `catch`, and there **_is no_** `finally`, then it **_must not_** be `await`ed.
-- if you `return` a promise within a `catch`, and there **_is a_** `finally`, then it **_must_** be `await`ed.
-- if you `return` a promise within a `finally`, then it **_must not_** be `await`ed.
+Listing the error-handling cases exhaustively:
+
+- 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`.
+- if you `return` a promise within a `catch`, and there is _no_ `finally`, then it must _not_ be `await`ed.
+- if you `return` a promise within a `catch`, and there _is_ a `finally`, then it _must_ be `await`ed.
+- if you `return` a promise within a `finally`, then it must not be `await`ed.
 
 Examples of code with `in-try-catch`:
 
@@ -42,23 +44,34 @@ Examples of code with `in-try-catch`:
 ```ts option='"in-try-catch"'
 async function invalidInTryCatch1() {
   try {
-    return Promise.resolve('try');
-  } catch (e) {}
+    return Promise.reject('try');
+  } catch (e) {
+    // Doesn't execute due to missing await.
+  }
 }
 
 async function invalidInTryCatch2() {
   try {
     throw new Error('error');
   } catch (e) {
-    return await Promise.resolve('catch');
+    // Unnecessary await; rejections here don't impact control flow.
+    return await Promise.reject('catch');
   }
 }
 
+// Prints 'starting async work', 'cleanup', 'async work done'.
 async function invalidInTryCatch3() {
+  async function doAsyncWork(): Promise<void> {
+    console.log('starting async work');
+    await new Promise(resolve => setTimeout(resolve, 1000));
+    console.log('async work done');
+  }
+
   try {
     throw new Error('error');
   } catch (e) {
-    return Promise.resolve('catch');
+    // Missing await.
+    return doAsyncWork();
   } finally {
     console.log('cleanup');
   }
@@ -70,7 +83,8 @@ async function invalidInTryCatch4() {
   } catch (e) {
     throw new Error('error2');
   } finally {
-    return await Promise.resolve('finally');
+    // Unnecessary await; rejections here don't impact control flow.
+    return await Promise.reject('finally');
   }
 }
 
@@ -89,23 +103,32 @@ async function invalidInTryCatch6() {
 ```ts option='"in-try-catch"'
 async function validInTryCatch1() {
   try {
-    return await Promise.resolve('try');
-  } catch (e) {}
+    return await Promise.reject('try');
+  } catch (e) {
+    // Executes as expected.
+  }
 }
 
 async function validInTryCatch2() {
   try {
     throw new Error('error');
   } catch (e) {
-    return Promise.resolve('catch');
+    return Promise.reject('catch');
   }
 }
 
+// Prints 'starting async work', 'async work done', 'cleanup'.
 async function validInTryCatch3() {
+  async function doAsyncWork(): Promise<void> {
+    console.log('starting async work');
+    await new Promise(resolve => setTimeout(resolve, 1000));
+    console.log('async work done');
+  }
+
   try {
     throw new Error('error');
   } catch (e) {
-    return await Promise.resolve('catch');
+    return await doAsyncWork();
   } finally {
     console.log('cleanup');
   }
@@ -117,7 +140,7 @@ async function validInTryCatch4() {
   } catch (e) {
     throw new Error('error2');
   } finally {
-    return Promise.resolve('finally');
+    return Promise.reject('finally');
   }
 }
 

Diff for: packages/eslint-plugin/src/rules/return-await.ts

@@ -10,6 +10,7 @@ import {
   isAwaitKeyword,
   isTypeAnyType,
   isTypeUnknownType,
+  nullThrows,
 } from '../util';
 import { getOperatorPrecedence } from '../util/getOperatorPrecedence';
 
@@ -41,6 +42,10 @@ export default createRule({
         'Returning an awaited promise is not allowed in this context.',
       requiredPromiseAwait:
         'Returning an awaited promise is required in this context.',
+      requiredPromiseAwaitSuggestion:
+        'Add `await` before the expression. Use caution as this may impact control flow.',
+      disallowedPromiseAwaitSuggestion:
+        'Remove `await` before the expression. Use caution as this may impact control flow.',
     },
     schema: [
       {
@@ -68,64 +73,90 @@ export default createRule({
       scopeInfoStack.pop();
     }
 
-    function inTry(node: ts.Node): boolean {
-      let ancestor = node.parent as ts.Node | undefined;
-
-      while (ancestor && !ts.isFunctionLike(ancestor)) {
-        if (ts.isTryStatement(ancestor)) {
-          return true;
-        }
-
-        ancestor = ancestor.parent;
+    /**
+     * Tests whether a node is inside of an explicit error handling context
+     * (try/catch/finally) in a way that throwing an exception will have an
+     * impact on the program's control flow.
+     */
+    function affectsExplicitErrorHandling(node: ts.Node): boolean {
+      // If an error-handling block is followed by another error-handling block,
+      // control flow is affected by whether promises in it are awaited or not.
+      // Otherwise, we need to check recursively for nested try statements until
+      // we get to the top level of a function or the program. If by then,
+      // there's no offending error-handling blocks, it doesn't affect control
+      // flow.
+      const tryAncestorResult = findContainingTryStatement(node);
+      if (tryAncestorResult == null) {
+        return false;
       }
 
-      return false;
-    }
-
-    function inCatch(node: ts.Node): boolean {
-      let ancestor = node.parent as ts.Node | undefined;
+      const { tryStatement, block } = tryAncestorResult;
 
-      while (ancestor && !ts.isFunctionLike(ancestor)) {
-        if (ts.isCatchClause(ancestor)) {
+      switch (block) {
+        case 'try':
+          // Try blocks are always followed by either a catch or finally,
+          // so exceptions thrown here always affect control flow.
           return true;
-        }
-
-        ancestor = ancestor.parent;
-      }
-
-      return false;
-    }
-
-    function isReturnPromiseInFinally(node: ts.Node): boolean {
-      let ancestor = node.parent as ts.Node | undefined;
+        case 'catch':
+          // Exceptions thrown in catch blocks followed by a finally block affect
+          // control flow.
+          if (tryStatement.finallyBlock != null) {
+            return true;
+          }
 
-      while (ancestor && !ts.isFunctionLike(ancestor)) {
-        if (
-          ts.isTryStatement(ancestor.parent) &&
-          ts.isBlock(ancestor) &&
-          ancestor.parent.end === ancestor.end
-        ) {
-          return true;
+          // Otherwise recurse.
+          return affectsExplicitErrorHandling(tryStatement);
+        case 'finally':
+          return affectsExplicitErrorHandling(tryStatement);
+        default: {
+          const __never: never = block;
+          throw new Error(`Unexpected block type: ${String(__never)}`);
         }
-        ancestor = ancestor.parent;
       }
+    }
 
-      return false;
+    interface FindContainingTryStatementResult {
+      tryStatement: ts.TryStatement;
+      block: 'try' | 'catch' | 'finally';
     }
 
-    function hasFinallyBlock(node: ts.Node): boolean {
+    /**
+     * A try _statement_ is the whole thing that encompasses try block,
+     * catch clause, and finally block. This function finds the nearest
+     * enclosing try statement (if present) for a given node, and reports which
+     * part of the try statement the node is in.
+     */
+    function findContainingTryStatement(
+      node: ts.Node,
+    ): FindContainingTryStatementResult | undefined {
+      let child = node;
       let ancestor = node.parent as ts.Node | undefined;
 
       while (ancestor && !ts.isFunctionLike(ancestor)) {
         if (ts.isTryStatement(ancestor)) {
-          return !!ancestor.finallyBlock;
+          let block: 'try' | 'catch' | 'finally' | undefined;
+          if (child === ancestor.tryBlock) {
+            block = 'try';
+          } else if (child === ancestor.catchClause) {
+            block = 'catch';
+          } else if (child === ancestor.finallyBlock) {
+            block = 'finally';
+          }
+
+          return {
+            tryStatement: ancestor,
+            block: nullThrows(
+              block,
+              'Child of a try statement must be a try block, catch clause, or finally block',
+            ),
+          };
         }
+        child = ancestor;
         ancestor = ancestor.parent;
       }
-      return false;
-    }
 
-    // function findTokensToRemove()
+      return undefined;
+    }
 
     function removeAwait(
       fixer: TSESLint.RuleFixer,
@@ -202,33 +233,35 @@ export default createRule({
       if (isAwait && !isThenable) {
         // any/unknown could be thenable; do not auto-fix
         const useAutoFix = !(isTypeAnyType(type) || isTypeUnknownType(type));
-        const fix = (fixer: TSESLint.RuleFixer): TSESLint.RuleFix | null =>
-          removeAwait(fixer, node);
 
         context.report({
           messageId: 'nonPromiseAwait',
           node,
-          ...(useAutoFix
-            ? { fix }
-            : {
-                suggest: [
-                  {
-                    messageId: 'nonPromiseAwait',
-                    fix,
-                  },
-                ],
-              }),
+          ...fixOrSuggest(useAutoFix, {
+            messageId: 'nonPromiseAwait',
+            fix: fixer => removeAwait(fixer, node),
+          }),
         });
         return;
       }
 
+      const affectsErrorHandling = affectsExplicitErrorHandling(expression);
+      const useAutoFix = !affectsErrorHandling;
+
       if (option === 'always') {
         if (!isAwait && isThenable) {
           context.report({
             messageId: 'requiredPromiseAwait',
             node,
-            fix: fixer =>
-              insertAwait(fixer, node, isHigherPrecedenceThanAwait(expression)),
+            ...fixOrSuggest(useAutoFix, {
+              messageId: 'requiredPromiseAwaitSuggestion',
+              fix: fixer =>
+                insertAwait(
+                  fixer,
+                  node,
+                  isHigherPrecedenceThanAwait(expression),
+                ),
+            }),
           });
         }
 
@@ -240,35 +273,39 @@ export default createRule({
           context.report({
             messageId: 'disallowedPromiseAwait',
             node,
-            fix: fixer => removeAwait(fixer, node),
+            ...fixOrSuggest(useAutoFix, {
+              messageId: 'disallowedPromiseAwaitSuggestion',
+              fix: fixer => removeAwait(fixer, node),
+            }),
           });
         }
 
         return;
       }
 
       if (option === 'in-try-catch') {
-        const isInTryCatch = inTry(expression) || inCatch(expression);
-        if (isAwait && !isInTryCatch) {
+        if (isAwait && !affectsErrorHandling) {
           context.report({
             messageId: 'disallowedPromiseAwait',
             node,
-            fix: fixer => removeAwait(fixer, node),
+            ...fixOrSuggest(useAutoFix, {
+              messageId: 'disallowedPromiseAwaitSuggestion',
+              fix: fixer => removeAwait(fixer, node),
+            }),
           });
-        } else if (!isAwait && isInTryCatch) {
-          if (inCatch(expression) && !hasFinallyBlock(expression)) {
-            return;
-          }
-
-          if (isReturnPromiseInFinally(expression)) {
-            return;
-          }
-
+        } else if (!isAwait && affectsErrorHandling) {
           context.report({
             messageId: 'requiredPromiseAwait',
             node,
-            fix: fixer =>
-              insertAwait(fixer, node, isHigherPrecedenceThanAwait(expression)),
+            ...fixOrSuggest(useAutoFix, {
+              messageId: 'requiredPromiseAwaitSuggestion',
+              fix: fixer =>
+                insertAwait(
+                  fixer,
+                  node,
+                  isHigherPrecedenceThanAwait(expression),
+                ),
+            }),
           });
         }
 
@@ -321,3 +358,12 @@ export default createRule({
     };
   },
 });
+
+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] };
+}