From 95d57885c5efd05d3cb35fb8f7816a98bf56f1c6 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Fri, 7 Sep 2018 14:14:01 -0700 Subject: [PATCH] Ensure diagnostic reporting matches code fix ability --- .../codefixes/convertToAsyncFunction.ts | 11 +-- src/services/suggestionDiagnostics.ts | 39 +++++++++- src/services/utilities.ts | 8 ++ .../unittests/convertToAsyncFunction.ts | 76 ++++++++----------- 4 files changed, 79 insertions(+), 55 deletions(-) diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index 61e142a8a08..ba230aa95e4 100644 --- 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 { 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 index 167bcb6bbac..3df40c8d9df 100644 --- 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 index b8a235bbeb4..19bc029e855 100644 --- 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 index 9fac9e59e92..9f675f1c89a 100644 --- 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: `/// @@ -261,14 +255,14 @@ interface String { charAt: any; } interface Array {}` }; - 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 {}` 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 { return fetch('https://typescriptlang.org'); } @@ -495,7 +489,7 @@ function [#|f|]():Promise{ } ` ); - _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_NoSuggestionNoPromise", ` + _testConvertToAsyncFunctionFailed("convertToAsyncFunction_NoSuggestionNoPromise", ` function [#|f|]():void{ } ` @@ -548,21 +542,21 @@ function [#|f|]():Promise { } ` ); - _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_Finally1", ` + _testConvertToAsyncFunctionFailed("convertToAsyncFunction_Finally1", ` function [#|finallyTest|](): Promise { 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 { return fetch("https://typescriptlang.org").then(res => console.log(res)).finally(console.log("finally!")); } ` ); - _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_Finally3", ` + _testConvertToAsyncFunctionFailed("convertToAsyncFunction_Finally3", ` function [#|finallyTest|](): Promise { return fetch("https://typescriptlang.org").finally(console.log("finally!")); } @@ -590,14 +584,14 @@ function [#|innerPromise|](): Promise { ` ); - _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)); @@ -605,7 +599,7 @@ function [#|f|]() { } ` ); - _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,7 +632,7 @@ function [#|f|]() { ` ); - _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn06", ` + _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn06", ` function [#|f|]() { var blob = fetch("https://typescriptlang.org"); return blob; @@ -646,7 +640,7 @@ function [#|f|]() { ` ); - _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 { ); - _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_SeperateLines", ` + _testConvertToAsyncFunctionFailed("convertToAsyncFunction_SeperateLines", ` function [#|f|](): Promise { 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); } } \ No newline at end of file