diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index e8e24b40a0c..7f5174cf67b 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/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 { 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; diff --git a/src/services/suggestionDiagnostics.ts b/src/services/suggestionDiagnostics.ts index d4aec1b1c6f..271f0601186 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: // identifier includes undefined + 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 25678ad72a6..7f0d06b4800 100644 --- a/src/services/utilities.ts +++ b/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; } diff --git a/src/testRunner/unittests/convertToAsyncFunction.ts b/src/testRunner/unittests/convertToAsyncFunction.ts index 99788e1310e..9f675f1c89a 100644 --- a/src/testRunner/unittests/convertToAsyncFunction.ts +++ b/src/testRunner/unittests/convertToAsyncFunction.ts @@ -1,68 +1,4 @@ namespace ts { - interface Range { - pos: number; - end: number; - name: string; - } - - interface Test { - source: string; - ranges: Map; - } - - function getTest(source: string): Test { - const activeRanges: Range[] = []; - let text = ""; - let lastPos = 0; - let pos = 0; - const ranges = createMap(); - - while (pos < source.length) { - if (source.charCodeAt(pos) === CharacterCodes.openBracket && - (source.charCodeAt(pos + 1) === CharacterCodes.hash || source.charCodeAt(pos + 1) === CharacterCodes.$)) { - const saved = pos; - pos += 2; - const s = pos; - consumeIdentifier(); - const e = pos; - if (source.charCodeAt(pos) === CharacterCodes.bar) { - pos++; - text += source.substring(lastPos, saved); - const name = s === e - ? source.charCodeAt(saved + 1) === CharacterCodes.hash ? "selection" : "extracted" - : source.substring(s, e); - activeRanges.push({ name, pos: text.length, end: undefined! }); - lastPos = pos; - continue; - } - else { - pos = saved; - } - } - else if (source.charCodeAt(pos) === CharacterCodes.bar && source.charCodeAt(pos + 1) === CharacterCodes.closeBracket) { - text += source.substring(lastPos, pos); - activeRanges[activeRanges.length - 1].end = text.length; - const range = activeRanges.pop()!; - if (range.name in ranges) { - throw new Error(`Duplicate name of range ${range.name}`); - } - ranges.set(range.name, range); - pos += 2; - lastPos = pos; - continue; - } - pos++; - } - text += source.substring(lastPos, pos); - - function consumeIdentifier() { - while (isIdentifierPart(source.charCodeAt(pos), ScriptTarget.Latest)) { - pos++; - } - } - return { source: text, ranges }; - } - const libFile: TestFSWithWatch.File = { path: "/a/lib/lib.d.ts", content: `/// @@ -319,19 +255,22 @@ interface String { charAt: any; } interface Array {}` }; - function testConvertToAsyncFunction(caption: string, text: string, baselineFolder: string, diagnosticDescription: DiagnosticMessage, codeFixDescription: DiagnosticMessage, includeLib?: boolean) { - const t = getTest(text); + 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`); } - [Extension.Ts, Extension.Js].forEach(extension => + const extensions = expectFailure ? [Extension.Ts] : [Extension.Ts, Extension.Js]; + + extensions.forEach(extension => it(`${caption} [${extension}]`, () => runBaseline(extension))); function runBaseline(extension: Extension) { const path = "/a" + extension; - const program = makeProgram({ path, content: t.source }, includeLib)!; + const languageService = makeLanguageService({ path, content: t.source }, includeLib); + const program = languageService.getProgram()!; if (hasSyntacticDiagnostics(program)) { // Don't bother generating JS baselines for inputs that aren't valid JS. @@ -345,10 +284,6 @@ interface Array {}` }; const sourceFile = program.getSourceFile(path)!; - const host = projectSystem.createServerHost([f, libFile]); - const projectService = projectSystem.createProjectService(host); - projectService.openClientFile(f.path); - const languageService = projectService.inferredProjects[0].getLanguageService(); const context: CodeFixContext = { errorCode: 80006, span: { start: selectionRange.pos, length: selectionRange.end - selectionRange.pos }, @@ -361,37 +296,45 @@ interface Array {}` }; const diagnostics = languageService.getSuggestionDiagnostics(f.path); - const diagnostic = find(diagnostics, diagnostic => diagnostic.messageText === diagnosticDescription.message); - assert.exists(diagnostic); - assert.equal(diagnostic!.start, context.span.start); - assert.equal(diagnostic!.length, context.span.length); + 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 (expectFailure) { + assert.isUndefined(diagnostic); + } + else { + assert.exists(diagnostic); + } const actions = codefix.getFixes(context); - const action = find(actions, action => action.description === codeFixDescription.message)!; - assert.exists(action); + const action = find(actions, action => action.description === Diagnostics.Convert_to_async_function.message); + if (expectFailure) { + assert.isNotTrue(action && action.changes.length > 0); + return; + } + + assert.isTrue(action && action.changes.length > 0); const data: string[] = []; data.push(`// ==ORIGINAL==`); data.push(text.replace("[#|", "/*[#|*/").replace("|]", "/*|]*/")); - const changes = action.changes; + const changes = action!.changes; assert.lengthOf(changes, 1); - data.push(`// ==ASYNC FUNCTION::${action.description}==`); + data.push(`// ==ASYNC FUNCTION::${action!.description}==`); const newText = textChanges.applyChanges(sourceFile.text, changes[0].textChanges); data.push(newText); - const diagProgram = makeProgram({ path, content: newText }, includeLib)!; + const diagProgram = makeLanguageService({ path, content: newText }, includeLib).getProgram()!; assert.isFalse(hasSyntacticDiagnostics(diagProgram)); Harness.Baseline.runBaseline(`${baselineFolder}/${caption}${extension}`, data.join(newLineCharacter)); } - function makeProgram(f: { path: string, content: string }, includeLib?: boolean) { + function makeLanguageService(f: { path: string, content: string }, includeLib?: boolean) { const host = projectSystem.createServerHost(includeLib ? [f, libFile] : [f]); // libFile is expensive to parse repeatedly - only test when required const projectService = projectSystem.createProjectService(host); projectService.openClientFile(f.path); - const program = projectService.inferredProjects[0].getLanguageService().getProgram(); - return program; + return projectService.inferredProjects[0].getLanguageService(); } function hasSyntacticDiagnostics(program: Program) { @@ -400,27 +343,6 @@ interface Array {}` } } - function testConvertToAsyncFunctionFailed(caption: string, text: string, description: DiagnosticMessage) { - it(caption, () => { - const t = extractTest(text); - const selectionRange = t.ranges.get("selection"); - if (!selectionRange) { - throw new Error(`Test ${caption} does not specify selection range`); - } - const f = { - path: "/a.ts", - content: t.source - }; - const host = projectSystem.createServerHost([f, libFile]); - const projectService = projectSystem.createProjectService(host); - projectService.openClientFile(f.path); - const languageService = projectService.inferredProjects[0].getLanguageService(); - - const actions = languageService.getSuggestionDiagnostics(f.path); - assert.isUndefined(find(actions, action => action.messageText === description.message)); - }); - } - describe("convertToAsyncFunctions", () => { _testConvertToAsyncFunction("convertToAsyncFunction_basic", ` function [#|f|](): Promise{ @@ -545,6 +467,12 @@ function [#|f|]():Promise { function [#|f|]():Promise { return fetch('https://typescriptlang.org').catch(rej => console.log(rej)); } +` + ); + _testConvertToAsyncFunction("convertToAsyncFunction_NoRes4", ` +function [#|f|]() { + return fetch('https://typescriptlang.org').then(undefined, rejection => console.log("rejected:", rejection)); +} ` ); _testConvertToAsyncFunctionFailed("convertToAsyncFunction_NoSuggestion", ` @@ -1157,7 +1085,7 @@ function [#|f|]() { ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_NestedFunction", ` + _testConvertToAsyncFunctionFailed("convertToAsyncFunction_NestedFunctionWrongLocation", ` function [#|f|]() { function fn2(){ function fn3(){ @@ -1167,6 +1095,18 @@ function [#|f|]() { } return fn2(); } +`); + + _testConvertToAsyncFunction("convertToAsyncFunction_NestedFunctionRightLocation", ` +function f() { + function fn2(){ + function [#|fn3|](){ + return fetch("https://typescriptlang.org").then(res => console.log(res)); + } + return fn3(); + } + return fn2(); +} `); _testConvertToAsyncFunction("convertToAsyncFunction_UntypedFunction", ` @@ -1194,14 +1134,26 @@ const [#|foo|] = function () { } `); + _testConvertToAsyncFunctionFailed("convertToAsyncFunction_thenArgumentNotFunction", ` +function [#|f|]() { + return Promise.resolve().then(f ? (x => x) : (y => y)); +} +`); + + _testConvertToAsyncFunctionFailed("convertToAsyncFunction_thenArgumentNotFunctionNotLastInChain", ` +function [#|f|]() { + return Promise.resolve().then(f ? (x => x) : (y => y)).then(q => q); +} +`); + }); function _testConvertToAsyncFunction(caption: string, text: string) { - testConvertToAsyncFunction(caption, text, "convertToAsyncFunction", Diagnostics.This_may_be_converted_to_an_async_function, Diagnostics.Convert_to_async_function, /*includeLib*/ true); + testConvertToAsyncFunction(caption, text, "convertToAsyncFunction", /*includeLib*/ true); } function _testConvertToAsyncFunctionFailed(caption: string, text: string) { - testConvertToAsyncFunctionFailed(caption, text, Diagnostics.Convert_to_async_function); + testConvertToAsyncFunction(caption, text, "convertToAsyncFunction", /*includeLib*/ true, /*expectFailure*/ true); } } \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NestedFunctionRightLocation.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NestedFunctionRightLocation.js new file mode 100644 index 00000000000..fa55fb8ca22 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NestedFunctionRightLocation.js @@ -0,0 +1,24 @@ +// ==ORIGINAL== + +function f() { + function fn2(){ + function /*[#|*/fn3/*|]*/(){ + return fetch("https://typescriptlang.org").then(res => console.log(res)); + } + return fn3(); + } + return fn2(); +} + +// ==ASYNC FUNCTION::Convert to async function== + +function f() { + function fn2(){ + async function fn3(){ + const res = await fetch("https://typescriptlang.org"); + return console.log(res); + } + return fn3(); + } + return fn2(); +} diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NestedFunctionRightLocation.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NestedFunctionRightLocation.ts new file mode 100644 index 00000000000..fa55fb8ca22 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NestedFunctionRightLocation.ts @@ -0,0 +1,24 @@ +// ==ORIGINAL== + +function f() { + function fn2(){ + function /*[#|*/fn3/*|]*/(){ + return fetch("https://typescriptlang.org").then(res => console.log(res)); + } + return fn3(); + } + return fn2(); +} + +// ==ASYNC FUNCTION::Convert to async function== + +function f() { + function fn2(){ + async function fn3(){ + const res = await fetch("https://typescriptlang.org"); + return console.log(res); + } + return fn3(); + } + return fn2(); +} diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.js new file mode 100644 index 00000000000..2bbf32e46a6 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.js @@ -0,0 +1,16 @@ +// ==ORIGINAL== + +function /*[#|*/f/*|]*/() { + return fetch('https://typescriptlang.org').then(undefined, rejection => console.log("rejected:", rejection)); +} + +// ==ASYNC FUNCTION::Convert to async function== + +async function f() { + try { + await fetch('https://typescriptlang.org'); + } + catch (rejection) { + return console.log("rejected:", rejection); + } +} diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.ts new file mode 100644 index 00000000000..2bbf32e46a6 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.ts @@ -0,0 +1,16 @@ +// ==ORIGINAL== + +function /*[#|*/f/*|]*/() { + return fetch('https://typescriptlang.org').then(undefined, rejection => console.log("rejected:", rejection)); +} + +// ==ASYNC FUNCTION::Convert to async function== + +async function f() { + try { + await fetch('https://typescriptlang.org'); + } + catch (rejection) { + return console.log("rejected:", rejection); + } +}