From 5a72da76c28044e7b19e22bda98dcaa09d108762 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Wed, 5 Sep 2018 16:26:20 -0700 Subject: [PATCH 1/4] Only perform async refactor if it won't delete code --- .../codefixes/convertToAsyncFunction.ts | 11 +- .../unittests/convertToAsyncFunction.ts | 210 +++++++----------- ...yncFunction_NestedFunctionRightLocation.js | 24 ++ ...yncFunction_NestedFunctionRightLocation.ts | 24 ++ .../convertToAsyncFunction_NoRes4.js | 16 ++ .../convertToAsyncFunction_NoRes4.ts | 16 ++ 6 files changed, 176 insertions(+), 125 deletions(-) create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NestedFunctionRightLocation.js create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NestedFunctionRightLocation.ts create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.js create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.ts diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index 76cdf471466..944bb06505d 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)), @@ -387,6 +389,10 @@ namespace ts.codefix { const hasArgName = argName && argName.identifier.text.length > 0; const shouldReturn = transformer.setOfExpressionsToReturn.get(getNodeId(parent).toString()); switch (func.kind) { + case SyntaxKind.NullKeyword: + case SyntaxKind.UndefinedKeyword: + // do not produce a transformed statement for a null or undefined argument + break; case SyntaxKind.Identifier: if (!hasArgName) break; @@ -443,6 +449,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([]); diff --git a/src/testRunner/unittests/convertToAsyncFunction.ts b/src/testRunner/unittests/convertToAsyncFunction.ts index 99788e1310e..9fac9e59e92 100644 --- a/src/testRunner/unittests/convertToAsyncFunction.ts +++ b/src/testRunner/unittests/convertToAsyncFunction.ts @@ -1,66 +1,8 @@ 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 enum TestExpectation { + Normal, + NoDiagnostic, + NoAction } const libFile: TestFSWithWatch.File = { @@ -319,19 +261,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, expectedResult: TestExpectation = TestExpectation.Normal) { + 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 = expectedResult === TestExpectation.Normal ? [Extension.Ts, Extension.Js] : [Extension.Ts]; + + 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 +290,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 +302,45 @@ interface Array {}` }; const diagnostics = languageService.getSuggestionDiagnostics(f.path); - const diagnostic = find(diagnostics, diagnostic => diagnostic.messageText === diagnosticDescription.message); + 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) { + assert.isUndefined(diagnostic); + return; + } + assert.exists(diagnostic); - assert.equal(diagnostic!.start, context.span.start); - assert.equal(diagnostic!.length, context.span.length); const actions = codefix.getFixes(context); - const action = find(actions, action => action.description === codeFixDescription.message)!; + const action = find(actions, action => action.description === Diagnostics.Convert_to_async_function.message); + if (expectedResult === TestExpectation.NoAction) { + assert.isUndefined(action); + return; + } + assert.exists(action); 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 +349,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{ @@ -547,7 +475,13 @@ function [#|f|]():Promise { } ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_NoSuggestion", ` + _testConvertToAsyncFunction("convertToAsyncFunction_NoRes4", ` +function [#|f|]() { + return fetch('https://typescriptlang.org').then(undefined, rejection => console.log("rejected:", rejection)); +} +` + ); + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_NoSuggestion", ` function [#|f|]():Promise { return fetch('https://typescriptlang.org'); } @@ -561,7 +495,7 @@ function [#|f|]():Promise{ } ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_NoSuggestionNoPromise", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_NoSuggestionNoPromise", ` function [#|f|]():void{ } ` @@ -614,21 +548,21 @@ function [#|f|]():Promise { } ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_Finally1", ` + _testConvertToAsyncFunctionNoDiagnostic("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!")); } ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_Finally2", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_Finally2", ` function [#|finallyTest|](): Promise { return fetch("https://typescriptlang.org").then(res => console.log(res)).finally(console.log("finally!")); } ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_Finally3", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_Finally3", ` function [#|finallyTest|](): Promise { return fetch("https://typescriptlang.org").finally(console.log("finally!")); } @@ -656,14 +590,14 @@ function [#|innerPromise|](): Promise { ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn01", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn01", ` function [#|f|]() { let blob = fetch("https://typescriptlang.org").then(resp => console.log(resp)); return blob; } ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn02", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn02", ` function [#|f|]() { let blob = fetch("https://typescriptlang.org"); blob.then(resp => console.log(resp)); @@ -671,7 +605,7 @@ function [#|f|]() { } ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn03", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn03", ` function [#|f|]() { let blob = fetch("https://typescriptlang.org") let blob2 = blob.then(resp => console.log(resp)); @@ -684,7 +618,7 @@ function err (rej) { } ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn04", ` + _testConvertToAsyncFunctionNoDiagnostic("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; @@ -695,7 +629,7 @@ function err (rej) { ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn05", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn05", ` function [#|f|]() { var blob = fetch("https://typescriptlang.org").then(res => console.log(res)); blob.then(x => x); @@ -704,7 +638,7 @@ function [#|f|]() { ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn06", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn06", ` function [#|f|]() { var blob = fetch("https://typescriptlang.org"); return blob; @@ -712,7 +646,7 @@ function [#|f|]() { ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn07", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn07", ` function [#|f|]() { let blob = fetch("https://typescriptlang.org"); let blob2 = fetch("https://microsoft.com"); @@ -723,7 +657,7 @@ function [#|f|]() { ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn08", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn08", ` function [#|f|]() { let blob = fetch("https://typescriptlang.org"); if (!blob.ok){ @@ -735,7 +669,7 @@ function [#|f|]() { ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn09", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn09", ` function [#|f|]() { let blob3; let blob = fetch("https://typescriptlang.org"); @@ -749,7 +683,7 @@ function [#|f|]() { ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn10", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn10", ` function [#|f|]() { let blob3; let blob = fetch("https://typescriptlang.org"); @@ -763,7 +697,7 @@ function [#|f|]() { ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_VarReturn11", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_VarReturn11", ` function [#|f|]() { let blob; return blob; @@ -773,7 +707,7 @@ function [#|f|]() { - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_Param1", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_Param1", ` function [#|f|]() { return my_print(fetch("https://typescriptlang.org").then(res => console.log(res))); } @@ -830,7 +764,7 @@ function [#|f|](): Promise { ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_SeperateLines", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_SeperateLines", ` function [#|f|](): Promise { var blob = fetch("https://typescriptlang.org") blob.then(resp => { @@ -1093,7 +1027,7 @@ function [#|f|]() { } `); -_testConvertToAsyncFunctionFailed("convertToAsyncFunction_CatchFollowedByCall", ` +_testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_CatchFollowedByCall", ` function [#|f|](){ return fetch("https://typescriptlang.org").then(res).catch(rej).toString(); } @@ -1157,7 +1091,7 @@ function [#|f|]() { ` ); - _testConvertToAsyncFunctionFailed("convertToAsyncFunction_NestedFunction", ` + _testConvertToAsyncFunctionNoDiagnostic("convertToAsyncFunction_NestedFunctionWrongLocation", ` function [#|f|]() { function fn2(){ function fn3(){ @@ -1167,6 +1101,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 +1140,30 @@ const [#|foo|] = function () { } `); + _testConvertToAsyncFunctionNoAction("convertToAsyncFunction_thenArgumentNotFunction", ` +function [#|f|]() { + return Promise.resolve().then(f ? (x => x) : (y => y)); +} +`); + +_testConvertToAsyncFunctionNoAction("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); + 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); } } \ 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); + } +} From f7f5b1ac87e88ba3a3b809de606b2eaf269e7136 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Wed, 5 Sep 2018 16:28:53 -0700 Subject: [PATCH 2/4] Don't case on type node --- src/services/codefixes/convertToAsyncFunction.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index 944bb06505d..61e142a8a08 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -390,7 +390,6 @@ namespace ts.codefix { const shouldReturn = transformer.setOfExpressionsToReturn.get(getNodeId(parent).toString()); switch (func.kind) { case SyntaxKind.NullKeyword: - case SyntaxKind.UndefinedKeyword: // do not produce a transformed statement for a null or undefined argument break; case SyntaxKind.Identifier: From 95d57885c5efd05d3cb35fb8f7816a98bf56f1c6 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Fri, 7 Sep 2018 14:14:01 -0700 Subject: [PATCH 3/4] 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 From 57a6dbd6fa7715e3176339743b30c1de7ed55d73 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Fri, 14 Sep 2018 08:50:18 -0700 Subject: [PATCH 4/4] Add clarifying comments --- src/services/codefixes/convertToAsyncFunction.ts | 3 ++- src/services/suggestionDiagnostics.ts | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index ba230aa95e4..b12d4daf1c8 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -393,9 +393,10 @@ namespace ts.codefix { const shouldReturn = transformer.setOfExpressionsToReturn.get(getNodeId(parent).toString()); switch (func.kind) { case SyntaxKind.NullKeyword: - // do not produce a transformed statement for a null or undefined argument + // 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]); diff --git a/src/services/suggestionDiagnostics.ts b/src/services/suggestionDiagnostics.ts index 3df40c8d9df..66360948ea7 100644 --- a/src/services/suggestionDiagnostics.ts +++ b/src/services/suggestionDiagnostics.ts @@ -196,7 +196,7 @@ namespace ts { function isFixablePromiseArgument(arg: Expression): boolean { switch (arg.kind) { case SyntaxKind.NullKeyword: - case SyntaxKind.Identifier: + case SyntaxKind.Identifier: // identifier includes undefined case SyntaxKind.FunctionDeclaration: case SyntaxKind.FunctionExpression: case SyntaxKind.ArrowFunction: