Merge pull request microsoft#26930 from uniqueiniquity/onlyReportExpectedPromiseArgs

Only perform async code fix if it can successfully refactor all parts

Author: uniqueiniquity

src/services/codefixes/convertToAsyncFunction.ts

@@ -2,11 +2,13 @@
 namespace ts.codefix {
     const fixId = "convertToAsyncFunction";
     const errorCodes = [Diagnostics.This_may_be_converted_to_an_async_function.code];
+    let codeActionSucceeded = true;
     registerCodeFix({
         errorCodes,
         getCodeActions(context: CodeFixContext) {
+            codeActionSucceeded = true;
             const changes = textChanges.ChangeTracker.with(context, (t) => convertToAsyncFunction(t, context.sourceFile, context.span.start, context.program.getTypeChecker(), context));
-            return [createCodeFixAction(fixId, changes, Diagnostics.Convert_to_async_function, fixId, Diagnostics.Convert_all_to_async_functions)];
+            return codeActionSucceeded ? [createCodeFixAction(fixId, changes, Diagnostics.Convert_to_async_function, fixId, Diagnostics.Convert_all_to_async_functions)] : [];
         },
         fixIds: [fixId],
         getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, err) => convertToAsyncFunction(changes, err.file, err.start, context.program.getTypeChecker(), context)),
@@ -252,6 +254,7 @@ namespace ts.codefix {
     }
 
     // dispatch function to recursively build the refactoring
+    // should be kept up to date with isFixablePromiseHandler in suggestionDiagnostics.ts
     function transformExpression(node: Expression, transformer: Transformer, outermostParent: CallExpression, prevArgName?: SynthIdentifier): Statement[] {
         if (!node) {
             return [];
@@ -273,6 +276,7 @@ namespace ts.codefix {
             return transformPromiseCall(node, transformer, prevArgName);
         }
 
+        codeActionSucceeded = false;
         return [];
     }
 
@@ -381,13 +385,18 @@ namespace ts.codefix {
             (createVariableDeclarationList([createVariableDeclaration(getSynthesizedDeepClone(prevArgName.identifier), /*type*/ undefined, rightHandSide)], getFlagOfIdentifier(prevArgName.identifier, transformer.constIdentifiers))))]);
     }
 
+    // should be kept up to date with isFixablePromiseArgument in suggestionDiagnostics.ts
     function getTransformationBody(func: Node, prevArgName: SynthIdentifier | undefined, argName: SynthIdentifier, parent: CallExpression, transformer: Transformer): NodeArray<Statement> {
 
         const hasPrevArgName = prevArgName && prevArgName.identifier.text.length > 0;
         const hasArgName = argName && argName.identifier.text.length > 0;
         const shouldReturn = transformer.setOfExpressionsToReturn.get(getNodeId(parent).toString());
         switch (func.kind) {
+            case SyntaxKind.NullKeyword:
+                // do not produce a transformed statement for a null argument
+                break;
             case SyntaxKind.Identifier:
+                // identifier includes undefined
                 if (!hasArgName) break;
 
                 const synthCall = createCall(getSynthesizedDeepClone(func) as Identifier, /*typeArguments*/ undefined, [argName.identifier]);
@@ -443,6 +452,9 @@ namespace ts.codefix {
                         return createNodeArray([createReturn(getSynthesizedDeepClone(funcBody) as Expression)]);
                     }
                 }
+            default:
+                // We've found a transformation body we don't know how to handle, so the refactoring should no-op to avoid deleting code.
+                codeActionSucceeded = false;
                 break;
         }
         return createNodeArray([]);
@@ -492,14 +504,6 @@ namespace ts.codefix {
         return innerCbBody;
     }
 
-    function hasPropertyAccessExpressionWithName(node: CallExpression, funcName: string): boolean {
-        if (!isPropertyAccessExpression(node.expression)) {
-            return false;
-        }
-
-        return node.expression.name.text === funcName;
-    }
-
     function getArgName(funcNode: Node, transformer: Transformer): SynthIdentifier {
 
         const numberOfAssignmentsOriginal = 0;

src/services/suggestionDiagnostics.ts

@@ -160,7 +160,7 @@ namespace ts {
             }
 
             function addHandlers(returnChild: Node) {
-                if (isPromiseHandler(returnChild)) {
+                if (isFixablePromiseHandler(returnChild)) {
                     returnStatements.push(child as ReturnStatement);
                 }
             }
@@ -170,8 +170,39 @@ namespace ts {
         return returnStatements;
     }
 
-    function isPromiseHandler(node: Node): boolean {
-        return (isCallExpression(node) && isPropertyAccessExpression(node.expression) &&
-            (node.expression.name.text === "then" || node.expression.name.text === "catch"));
+    // Should be kept up to date with transformExpression in convertToAsyncFunction.ts
+    function isFixablePromiseHandler(node: Node): boolean {
+        // ensure outermost call exists and is a promise handler
+        if (!isPromiseHandler(node) || !node.arguments.every(isFixablePromiseArgument)) {
+            return false;
+        }
+
+        // ensure all chained calls are valid
+        let currentNode = node.expression;
+        while (isPromiseHandler(currentNode) || isPropertyAccessExpression(currentNode)) {
+            if (isCallExpression(currentNode) && !currentNode.arguments.every(isFixablePromiseArgument)) {
+                return false;
+            }
+            currentNode = currentNode.expression;
+        }
+        return true;
+    }
+
+    function isPromiseHandler(node: Node): node is CallExpression {
+        return isCallExpression(node) && (hasPropertyAccessExpressionWithName(node, "then") || hasPropertyAccessExpressionWithName(node, "catch"));
+    }
+
+    // should be kept up to date with getTransformationBody in convertToAsyncFunction.ts
+    function isFixablePromiseArgument(arg: Expression): boolean {
+        switch (arg.kind) {
+            case SyntaxKind.NullKeyword:
+            case SyntaxKind.Identifier: // identifier includes undefined
+            case SyntaxKind.FunctionDeclaration:
+            case SyntaxKind.FunctionExpression:
+            case SyntaxKind.ArrowFunction:
+                return true;
+            default:
+                return false;
+        }
     }
 }

src/services/utilities.ts

@@ -226,6 +226,14 @@ namespace ts {
         return undefined;
     }
 
+    export function hasPropertyAccessExpressionWithName(node: CallExpression, funcName: string): boolean {
+        if (!isPropertyAccessExpression(node.expression)) {
+            return false;
+        }
+
+        return node.expression.name.text === funcName;
+    }
+
     export function isJumpStatementTarget(node: Node): node is Identifier & { parent: BreakOrContinueStatement } {
         return node.kind === SyntaxKind.Identifier && isBreakOrContinueStatement(node.parent) && node.parent.label === node;
     }