Merge pull request microsoft#29214 from uniqueiniquity/nestedAsyncCod…

…eFix Only provide suggestion for outermost async fix
Kingwl · Jan 1, 2019 · 799656a · 799656a
2 parents 11585d2 + cb57f17
commit 799656a
Show file tree

Hide file tree

Showing 21 changed files with 134 additions and 54 deletions.
diff --git a/src/services/suggestionDiagnostics.ts b/src/services/suggestionDiagnostics.ts
@@ -1,5 +1,7 @@
 /* @internal */
 namespace ts {
+    const visitedNestedConvertibleFunctions = createMap<true>();
+
     export function computeSuggestionDiagnostics(sourceFile: SourceFile, program: Program, cancellationToken: CancellationToken): DiagnosticWithLocation[] {
         program.getSemanticDiagnostics(sourceFile, cancellationToken);
         const diags: DiagnosticWithLocation[] = [];
@@ -13,6 +15,7 @@ namespace ts {
 
         const isJsFile = isSourceFileJS(sourceFile);
 
+        visitedNestedConvertibleFunctions.clear();
         check(sourceFile);
 
         if (getAllowSyntheticDefaultImports(program.getCompilerOptions())) {
@@ -114,17 +117,22 @@ namespace ts {
     }
 
     function addConvertToAsyncFunctionDiagnostics(node: FunctionLikeDeclaration, checker: TypeChecker, diags: Push<DiagnosticWithLocation>): void {
-        if (!isAsyncFunction(node) &&
-            node.body &&
-            isBlock(node.body) &&
-            hasReturnStatementWithPromiseHandler(node.body) &&
-            returnsPromise(node, checker)) {
+        // need to check function before checking map so that deeper levels of nested callbacks are checked
+        if (isConvertibleFunction(node, checker) && !visitedNestedConvertibleFunctions.has(getKeyFromNode(node))) {
             diags.push(createDiagnosticForNode(
                 !node.name && isVariableDeclaration(node.parent) && isIdentifier(node.parent.name) ? node.parent.name : node,
                 Diagnostics.This_may_be_converted_to_an_async_function));
         }
     }
 
+    function isConvertibleFunction(node: FunctionLikeDeclaration, checker: TypeChecker) {
+        return !isAsyncFunction(node) &&
+            node.body &&
+            isBlock(node.body) &&
+            hasReturnStatementWithPromiseHandler(node.body) &&
+            returnsPromise(node, checker);
+    }
+
     function returnsPromise(node: FunctionLikeDeclaration, checker: TypeChecker): boolean {
         const functionType = checker.getTypeAtLocation(node);
         const callSignatures = checker.getSignaturesOfType(functionType, SignatureKind.Call);
@@ -169,14 +177,20 @@ namespace ts {
     // 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:
+                visitedNestedConvertibleFunctions.set(getKeyFromNode(arg as FunctionLikeDeclaration), true);
+                /* falls through */
+            case SyntaxKind.NullKeyword:
+            case SyntaxKind.Identifier: // identifier includes undefined
                 return true;
             default:
                 return false;
         }
     }
+
+    function getKeyFromNode(exp: FunctionLikeDeclaration) {
+        return `${exp.pos.toString()}:${exp.end.toString()}`;
+    }
 }
diff --git a/src/testRunner/unittests/services/convertToAsyncFunction.ts b/src/testRunner/unittests/services/convertToAsyncFunction.ts
@@ -255,7 +255,7 @@ interface String { charAt: any; }
 interface Array<T> {}`
     };
 
-    function testConvertToAsyncFunction(caption: string, text: string, baselineFolder: string, includeLib?: boolean, expectFailure = false) {
+    function testConvertToAsyncFunction(caption: string, text: string, baselineFolder: string, includeLib?: boolean, expectFailure = false, onlyProvideAction = false) {
         const t = extractTest(text);
         const selectionRange = t.ranges.get("selection")!;
         if (!selectionRange) {
@@ -307,7 +307,7 @@ interface Array<T> {}`
 
             const actions = codefix.getFixes(context);
             const action = find(actions, action => action.description === Diagnostics.Convert_to_async_function.message);
-            if (expectFailure) {
+            if (expectFailure && !onlyProvideAction) {
                 assert.isNotTrue(action && action.changes.length > 0);
                 return;
             }
@@ -1151,25 +1151,25 @@ function [#|f|]() {
     _testConvertToAsyncFunction("convertToAsyncFunction_simpleFunctionExpression", `
 const [#|foo|] = function () {
     return fetch('https://typescriptlang.org').then(result => { console.log(result) });
-} 
+}
 `);
 
     _testConvertToAsyncFunction("convertToAsyncFunction_simpleFunctionExpressionWithName", `
 const foo = function [#|f|]() {
     return fetch('https://typescriptlang.org').then(result => { console.log(result) });
-} 
+}
 `);
 
     _testConvertToAsyncFunction("convertToAsyncFunction_simpleFunctionExpressionAssignedToBindingPattern", `
 const { length } = [#|function|] () {
     return fetch('https://typescriptlang.org').then(result => { console.log(result) });
-} 
+}
 `);
 
     _testConvertToAsyncFunction("convertToAsyncFunction_catchBlockUniqueParams", `
 function [#|f|]() {
-	return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x); 
-} 
+	return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x);
+}
 `);
 
     _testConvertToAsyncFunction("convertToAsyncFunction_bindingPattern", `
@@ -1178,7 +1178,7 @@ function [#|f|]() {
 }
 function res({ status, trailer }){
     console.log(status);
-} 
+}
 `);
 
     _testConvertToAsyncFunction("convertToAsyncFunction_bindingPatternNameCollision", `
@@ -1188,7 +1188,7 @@ function [#|f|]() {
 }
 function res({ status, trailer }){
     console.log(status);
-} 
+}
 `);
 
     _testConvertToAsyncFunctionFailed("convertToAsyncFunction_thenArgumentNotFunction", `
@@ -1209,7 +1209,7 @@ function [#|f|]() {
 }
 function res(result) {
     return Promise.resolve().then(x => console.log(result));
-} 
+}
 `);
 
     _testConvertToAsyncFunction("convertToAsyncFunction_callbackReturnsPromise", `
@@ -1241,7 +1241,7 @@ function [#|f|]() {
     return Promise.resolve(1)
         .then(x => Promise.reject(x))
         .catch(err => console.log(err));
-}    
+}
 `);
 
 _testConvertToAsyncFunction("convertToAsyncFunction_nestedPromises", `
@@ -1266,6 +1266,22 @@ _testConvertToAsyncFunction("convertToAsyncFunction_exportModifier", `
 export function [#|foo|]() {
     return fetch('https://typescriptlang.org').then(s => console.log(s));
 }
+`);
+
+_testConvertToAsyncFunction("convertToAsyncFunction_OutermostOnlySuccess", `
+function [#|foo|]() {
+    return fetch('a').then(() => {
+        return fetch('b').then(() => 'c');
+    })
+}
+`);
+
+_testConvertToAsyncFunctionFailedSuggestion("convertToAsyncFunction_OutermostOnlyFailure", `
+function foo() {
+    return fetch('a').then([#|() => {|]
+        return fetch('b').then(() => 'c');
+    })
+}
 `);
     });
 
@@ -1276,4 +1292,8 @@ export function [#|foo|]() {
     function _testConvertToAsyncFunctionFailed(caption: string, text: string) {
         testConvertToAsyncFunction(caption, text, "convertToAsyncFunction", /*includeLib*/ true, /*expectFailure*/ true);
     }
+
+    function _testConvertToAsyncFunctionFailedSuggestion(caption: string, text: string) {
+        testConvertToAsyncFunction(caption, text, "convertToAsyncFunction", /*includeLib*/ true, /*expectFailure*/ true, /*onlyProvideAction*/ true);
+    }
 }
diff --git a/...baselines/reference/convertToAsyncFunction/convertToAsyncFunction_OutermostOnlyFailure.ts b/...baselines/reference/convertToAsyncFunction/convertToAsyncFunction_OutermostOnlyFailure.ts
@@ -0,0 +1,16 @@
+// ==ORIGINAL==
+
+function foo() {
+    return fetch('a').then(/*[#|*/() => {/*|]*/
+        return fetch('b').then(() => 'c');
+    })
+}
+
+// ==ASYNC FUNCTION::Convert to async function==
+
+function foo() {
+    return fetch('a').then(async () => {
+        await fetch('b');
+        return 'c';
+    })
+}
diff --git a/...baselines/reference/convertToAsyncFunction/convertToAsyncFunction_OutermostOnlySuccess.js b/...baselines/reference/convertToAsyncFunction/convertToAsyncFunction_OutermostOnlySuccess.js
@@ -0,0 +1,15 @@
+// ==ORIGINAL==
+
+function /*[#|*/foo/*|]*/() {
+    return fetch('a').then(() => {
+        return fetch('b').then(() => 'c');
+    })
+}
+
+// ==ASYNC FUNCTION::Convert to async function==
+
+async function foo() {
+    await fetch('a');
+    await fetch('b');
+    return 'c';
+}
diff --git a/...baselines/reference/convertToAsyncFunction/convertToAsyncFunction_OutermostOnlySuccess.ts b/...baselines/reference/convertToAsyncFunction/convertToAsyncFunction_OutermostOnlySuccess.ts
@@ -0,0 +1,15 @@
+// ==ORIGINAL==
+
+function /*[#|*/foo/*|]*/() {
+    return fetch('a').then(() => {
+        return fetch('b').then(() => 'c');
+    })
+}
+
+// ==ASYNC FUNCTION::Convert to async function==
+
+async function foo() {
+    await fetch('a');
+    await fetch('b');
+    return 'c';
+}
diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.js
@@ -5,7 +5,7 @@ function /*[#|*/f/*|]*/() {
 }
 function res({ status, trailer }){
     console.log(status);
-} 
+}
 
 // ==ASYNC FUNCTION::Convert to async function==
 
@@ -15,4 +15,4 @@ async function f() {
 }
 function res({ status, trailer }){
     console.log(status);
-} 
+}
diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts
@@ -5,7 +5,7 @@ function /*[#|*/f/*|]*/() {
 }
 function res({ status, trailer }){
     console.log(status);
-} 
+}
 
 // ==ASYNC FUNCTION::Convert to async function==
 
@@ -15,4 +15,4 @@ async function f() {
 }
 function res({ status, trailer }){
     console.log(status);
-} 
+}
diff --git a/...es/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.js b/...es/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.js
@@ -6,7 +6,7 @@ function /*[#|*/f/*|]*/() {
 }
 function res({ status, trailer }){
     console.log(status);
-} 
+}
 
 // ==ASYNC FUNCTION::Convert to async function==
 
@@ -17,4 +17,4 @@ async function f() {
 }
 function res({ status, trailer }){
     console.log(status);
-} 
+}
diff --git a/...es/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.ts b/...es/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.ts
@@ -6,7 +6,7 @@ function /*[#|*/f/*|]*/() {
 }
 function res({ status, trailer }){
     console.log(status);
-} 
+}
 
 // ==ASYNC FUNCTION::Convert to async function==
 
@@ -17,4 +17,4 @@ async function f() {
 }
 function res({ status, trailer }){
     console.log(status);
-} 
+}
diff --git a/...convertToAsyncFunction/convertToAsyncFunction_callbackReturnsRejectedPromiseInTryBlock.js b/...convertToAsyncFunction/convertToAsyncFunction_callbackReturnsRejectedPromiseInTryBlock.js
@@ -4,7 +4,7 @@ function /*[#|*/f/*|]*/() {
     return Promise.resolve(1)
         .then(x => Promise.reject(x))
         .catch(err => console.log(err));
-}    
+}
 
 // ==ASYNC FUNCTION::Convert to async function==
 
@@ -16,4 +16,4 @@ async function f() {
     catch (err) {
         return console.log(err);
     }
-}    
+}
diff --git a/...convertToAsyncFunction/convertToAsyncFunction_callbackReturnsRejectedPromiseInTryBlock.ts b/...convertToAsyncFunction/convertToAsyncFunction_callbackReturnsRejectedPromiseInTryBlock.ts
@@ -4,7 +4,7 @@ function /*[#|*/f/*|]*/() {
     return Promise.resolve(1)
         .then(x => Promise.reject(x))
         .catch(err => console.log(err));
-}    
+}
 
 // ==ASYNC FUNCTION::Convert to async function==
 
@@ -16,4 +16,4 @@ async function f() {
     catch (err) {
         return console.log(err);
     }
-}    
+}
diff --git a/...selines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParams.js b/...selines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParams.js
@@ -1,8 +1,8 @@
 // ==ORIGINAL==
 
 function /*[#|*/f/*|]*/() {
-	return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x); 
-} 
+	return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x);
+}
 
 // ==ASYNC FUNCTION::Convert to async function==
 
@@ -15,5 +15,5 @@ async function f() {
     catch (x_1) {
         x_2 = "a";
     }
-    return !!x_2; 
-} 
+    return !!x_2;
+}
diff --git a/...selines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParams.ts b/...selines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParams.ts
@@ -1,8 +1,8 @@
 // ==ORIGINAL==
 
 function /*[#|*/f/*|]*/() {
-	return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x); 
-} 
+	return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x);
+}
 
 // ==ASYNC FUNCTION::Convert to async function==
 
@@ -15,5 +15,5 @@ async function f() {
     catch (x_1) {
         x_2 = "a";
     }
-    return !!x_2; 
-} 
+    return !!x_2;
+}
diff --git a/...lines/reference/convertToAsyncFunction/convertToAsyncFunction_runEffectfulContinuation.js b/...lines/reference/convertToAsyncFunction/convertToAsyncFunction_runEffectfulContinuation.js
@@ -5,7 +5,7 @@ function /*[#|*/f/*|]*/() {
 }
 function res(result) {
     return Promise.resolve().then(x => console.log(result));
-} 
+}
 
 // ==ASYNC FUNCTION::Convert to async function==
 
@@ -16,4 +16,4 @@ async function f() {
 }
 function res(result) {
     return Promise.resolve().then(x => console.log(result));
-} 
+}
diff --git a/...lines/reference/convertToAsyncFunction/convertToAsyncFunction_runEffectfulContinuation.ts b/...lines/reference/convertToAsyncFunction/convertToAsyncFunction_runEffectfulContinuation.ts
@@ -5,7 +5,7 @@ function /*[#|*/f/*|]*/() {
 }
 function res(result) {
     return Promise.resolve().then(x => console.log(result));
-} 
+}
 
 // ==ASYNC FUNCTION::Convert to async function==
 
@@ -16,4 +16,4 @@ async function f() {
 }
 function res(result) {
     return Promise.resolve().then(x => console.log(result));
-} 
+}