Ensure diagnostic reporting matches code fix ability

Benjamin Lichtman · Benjamin Lichtman · commit 95d57885c5ef · 2018-09-07T14:14:01.000-07:00
diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts
@@ -254,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 [];
@@ -275,6 +276,7 @@ namespace ts.codefix {
             return transformPromiseCall(node, transformer, prevArgName);
         }
 
+        codeActionSucceeded = false;
         return [];
     }
 
@@ -383,6 +385,7 @@ 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;
@@ -500,14 +503,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;
diff --git a/src/services/suggestionDiagnostics.ts b/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:
+            case SyntaxKind.FunctionDeclaration:
+            case SyntaxKind.FunctionExpression:
+            case SyntaxKind.ArrowFunction:
+                return true;
+            default:
+                return false;
+        }
     }
 }
diff --git a/src/services/utilities.ts b/src/services/utilities.ts
@@ -224,6 +224,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;
     }
diff --git a/src/testRunner/unittests/convertToAsyncFunction.ts b/src/testRunner/unittests/convertToAsyncFunction.ts
@@ -1,10 +1,4 @@
 namespace ts {
-    const enum TestExpectation {
-        Normal,
-        NoDiagnostic,
-        NoAction
-    }
-
     const libFile: TestFSWithWatch.File = {
         path: "/a/lib/lib.d.ts",
         content: `/// <reference no-default-lib="true"/>
@@ -261,14 +255,14 @@ interface String { charAt: any; }
 interface Array<T> {}`
     };
 
-    function testConvertToAsyncFunction(caption: string, text: string, baselineFolder: string, includeLib?: boolean, expectedResult: TestExpectation = TestExpectation.Normal) {
+    function testConvertToAsyncFunction(caption: string, text: string, baselineFolder: string, includeLib?: boolean, expectFailure = false) {
         const t = extractTest(text);
         const selectionRange = t.ranges.get("selection")!;
         if (!selectionRange) {
             throw new Error(`Test ${caption} does not specify selection range`);
         }
 
-        const extensions = expectedResult === TestExpectation.Normal ? [Extension.Ts, Extension.Js] : [Extension.Ts];
+        const extensions = expectFailure ? [Extension.Ts] : [Extension.Ts, Extension.Js];
 
         extensions.forEach(extension =>
             it(`${caption} [${extension}]`, () => runBaseline(extension)));
@@ -304,21 +298,21 @@ interface Array<T> {}`
             const diagnostics = languageService.getSuggestionDiagnostics(f.path);
             const diagnostic = find(diagnostics, diagnostic => diagnostic.messageText === Diagnostics.This_may_be_converted_to_an_async_function.message &&
                 diagnostic.start === context.span.start && diagnostic.length === context.span.length);
-            if (expectedResult === TestExpectation.NoDiagnostic) {
+            if (expectFailure) {
                 assert.isUndefined(diagnostic);
-                return;
             }
-
-            assert.exists(diagnostic);
+            else {
+                assert.exists(diagnostic);
+            }
 
             const actions = codefix.getFixes(context);
             const action = find(actions, action => action.description === Diagnostics.Convert_to_async_function.message);
-            if (expectedResult === TestExpectation.NoAction) {
-                assert.isUndefined(action);
+            if (expectFailure) {
+                assert.isNotTrue(action && action.changes.length > 0);
                 return;
             }
 
-            assert.exists(action);
+            assert.isTrue(action && action.changes.length > 0);
 
             const data: string[] = [];
             data.push(`// ==ORIGINAL==`);
@@ -481,7 +475,7 @@ function [#|f|]() {
 }
 `
         );
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_NoSuggestion", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_NoSuggestion", `
 function [#|f|]():Promise<Response> {
     return fetch('https://typescriptlang.org');
 }
@@ -495,7 +489,7 @@ function [#|f|]():Promise<void>{
 }
 `
         );
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_NoSuggestionNoPromise", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_NoSuggestionNoPromise", `
 function [#|f|]():void{
 }
 `
@@ -548,21 +542,21 @@ function [#|f|]():Promise<void> {
 }
 `
         );
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_Finally1", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_Finally1", `
 function [#|finallyTest|](): Promise<void> {
     return fetch("https://typescriptlang.org").then(res => console.log(res)).catch(rej => console.log("error", rej)).finally(console.log("finally!"));
 }
 `
         );
 
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_Finally2", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_Finally2", `
 function [#|finallyTest|](): Promise<void> {
     return fetch("https://typescriptlang.org").then(res => console.log(res)).finally(console.log("finally!"));
 }
 `
         );
 
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_Finally3", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_Finally3", `
 function [#|finallyTest|](): Promise<void> {
     return fetch("https://typescriptlang.org").finally(console.log("finally!"));
 }
@@ -590,22 +584,22 @@ function [#|innerPromise|](): Promise<string> {
 `
         );
 
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn01", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn01", `
 function [#|f|]() {
     let blob = fetch("https://typescriptlang.org").then(resp => console.log(resp));
     return blob;
 }
 `
         );
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn02", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn02", `
 function [#|f|]() {
     let blob = fetch("https://typescriptlang.org");
     blob.then(resp => console.log(resp));
     return blob;
 }
 `
         );
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn03", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn03", `
 function [#|f|]() {
     let blob = fetch("https://typescriptlang.org")
     let blob2 = blob.then(resp => console.log(resp));
@@ -618,7 +612,7 @@ function err (rej) {
 }
 `
         );
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn04", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn04", `
 function [#|f|]() {
     var blob = fetch("https://typescriptlang.org").then(res => console.log(res)), blob2 = fetch("https://microsoft.com").then(res => res.ok).catch(err);
     return blob;
@@ -629,7 +623,7 @@ function err (rej) {
 `
         );
 
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn05", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn05", `
 function [#|f|]() {
     var blob = fetch("https://typescriptlang.org").then(res => console.log(res));
     blob.then(x => x);
@@ -638,15 +632,15 @@ function [#|f|]() {
 `
         );
 
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn06", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn06", `
 function [#|f|]() {
     var blob = fetch("https://typescriptlang.org");
     return blob;
 }
 `
         );
 
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn07", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn07", `
 function [#|f|]() {
     let blob = fetch("https://typescriptlang.org");
     let blob2 = fetch("https://microsoft.com");
@@ -657,7 +651,7 @@ function [#|f|]() {
 `
         );
 
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn08", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn08", `
 function [#|f|]() {
     let blob = fetch("https://typescriptlang.org");
     if (!blob.ok){
@@ -669,7 +663,7 @@ function [#|f|]() {
 `
         );
 
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn09", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn09", `
 function [#|f|]() {
     let blob3;
     let blob = fetch("https://typescriptlang.org");
@@ -683,7 +677,7 @@ function [#|f|]() {
         );
 
 
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn10", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn10", `
 function [#|f|]() {
     let blob3;
     let blob = fetch("https://typescriptlang.org");
@@ -697,7 +691,7 @@ function [#|f|]() {
 `
         );
 
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn11", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn11", `
 function [#|f|]() {
     let blob;
     return blob;
@@ -707,7 +701,7 @@ function [#|f|]() {
 
 
 
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_Param1", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_Param1", `
 function [#|f|]() {
     return my_print(fetch("https://typescriptlang.org").then(res => console.log(res)));
 }
@@ -764,7 +758,7 @@ function [#|f|](): Promise<void> {
         );
 
 
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_SeperateLines", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_SeperateLines", `
 function [#|f|](): Promise<string> {
     var blob = fetch("https://typescriptlang.org")
     blob.then(resp => {
@@ -1027,7 +1021,7 @@ function [#|f|]() {
 }
 
 `);
-_testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_CatchFollowedByCall", `
+_testConvertToAsyncFunctionFailed("convertToAsyncFunction_CatchFollowedByCall", `
 function [#|f|](){
     return fetch("https://typescriptlang.org").then(res).catch(rej).toString();
 }
@@ -1091,7 +1085,7 @@ function [#|f|]() {
 `
         );
 
-        _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_NestedFunctionWrongLocation", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_NestedFunctionWrongLocation", `
 function [#|f|]() {
     function fn2(){
         function fn3(){
@@ -1140,13 +1134,13 @@ const [#|foo|] = function () {
 } 
 `);
 
-    _testConvertToAsyncFunctionNoAction("convertToAsyncFunction_thenArgumentNotFunction", `
+    _testConvertToAsyncFunctionFailed("convertToAsyncFunction_thenArgumentNotFunction", `
 function [#|f|]() {
     return Promise.resolve().then(f ? (x => x) : (y => y));
 }
 `);
 
-_testConvertToAsyncFunctionNoAction("convertToAsyncFunction_thenArgumentNotFunctionNotLastInChain", `
+    _testConvertToAsyncFunctionFailed("convertToAsyncFunction_thenArgumentNotFunctionNotLastInChain", `
 function [#|f|]() {
     return Promise.resolve().then(f ? (x => x) : (y => y)).then(q => q);
 }
@@ -1159,11 +1153,7 @@ function [#|f|]() {
         testConvertToAsyncFunction(caption, text, "convertToAsyncFunction", /*includeLib*/ true);
     }
 
-    function _testConvertToAsyncFunctionNoDiagnostic(caption: string, text: string) {
-        testConvertToAsyncFunction(caption, text, "convertToAsyncFunction", /*includeLib*/ true, TestExpectation.NoDiagnostic);
-    }
-
-    function _testConvertToAsyncFunctionNoAction(caption: string, text: string) {
-        testConvertToAsyncFunction(caption, text, "convertToAsyncFunction", /*includeLib*/ true, TestExpectation.NoAction);
+    function _testConvertToAsyncFunctionFailed(caption: string, text: string) {
+        testConvertToAsyncFunction(caption, text, "convertToAsyncFunction", /*includeLib*/ true, /*expectFailure*/ true);
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -254,6 +254,7 @@ namespace ts.codefix {`
`254`	`254`	`}`
`255`	`255`
`256`	`256`	`// dispatch function to recursively build the refactoring`
	`257`	`+ // should be kept up to date with isFixablePromiseHandler in suggestionDiagnostics.ts`
`257`	`258`	`function transformExpression(node: Expression, transformer: Transformer, outermostParent: CallExpression, prevArgName?: SynthIdentifier): Statement[] {`
`258`	`259`	`if (!node) {`
`259`	`260`	`return [];`
`@@ -275,6 +276,7 @@ namespace ts.codefix {`
`275`	`276`	`return transformPromiseCall(node, transformer, prevArgName);`
`276`	`277`	`}`
`277`	`278`
	`279`	`+ codeActionSucceeded = false;`
`278`	`280`	`return [];`
`279`	`281`	`}`
`280`	`282`
`@@ -383,6 +385,7 @@ namespace ts.codefix {`
`383`	`385`	`(createVariableDeclarationList([createVariableDeclaration(getSynthesizedDeepClone(prevArgName.identifier), /type/ undefined, rightHandSide)], getFlagOfIdentifier(prevArgName.identifier, transformer.constIdentifiers))))]);`
`384`	`386`	`}`
`385`	`387`
	`388`	`+ // should be kept up to date with isFixablePromiseArgument in suggestionDiagnostics.ts`
`386`	`389`	`function getTransformationBody(func: Node, prevArgName: SynthIdentifier \| undefined, argName: SynthIdentifier, parent: CallExpression, transformer: Transformer): NodeArray<Statement> {`
`387`	`390`
`388`	`391`	`const hasPrevArgName = prevArgName && prevArgName.identifier.text.length > 0;`
`@@ -500,14 +503,6 @@ namespace ts.codefix {`
`500`	`503`	`return innerCbBody;`
`501`	`504`	`}`
`502`	`505`
`503`		`- function hasPropertyAccessExpressionWithName(node: CallExpression, funcName: string): boolean {`
`504`		`- if (!isPropertyAccessExpression(node.expression)) {`
`505`		`- return false;`
`506`		`- }`
`507`		`-`
`508`		`- return node.expression.name.text === funcName;`
`509`		`- }`
`510`		`-`
`511`	`506`	`function getArgName(funcNode: Node, transformer: Transformer): SynthIdentifier {`
`512`	`507`
`513`	`508`	`const numberOfAssignmentsOriginal = 0;`
Original file line number	Diff line number	Diff line change
`@@ -224,6 +224,14 @@ namespace ts {`
`224`	`224`	`return undefined;`
`225`	`225`	`}`
`226`	`226`
	`227`	`+ export function hasPropertyAccessExpressionWithName(node: CallExpression, funcName: string): boolean {`
	`228`	`+ if (!isPropertyAccessExpression(node.expression)) {`
	`229`	`+ return false;`
	`230`	`+ }`
	`231`	`+`
	`232`	`+ return node.expression.name.text === funcName;`
	`233`	`+ }`
	`234`	`+`
`227`	`235`	`export function isJumpStatementTarget(node: Node): node is Identifier & { parent: BreakOrContinueStatement } {`
`228`	`236`	`return node.kind === SyntaxKind.Identifier && isBreakOrContinueStatement(node.parent) && node.parent.label === node;`
`229`	`237`	`}`