-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Promises to Async Await Code Fix #24939
Changes from 28 commits
f3971df
6f803f6
b8d8183
69795c8
6c0a712
546ee1b
8019335
6e31ff9
469da81
8c8ebe9
635cc04
5099bd1
b8353dc
25efbed
0b760bf
d3a92d9
460c6dd
73ed97e
effb559
3b1aff1
71b422d
9b2aa30
2384440
b806922
602a8a9
c1e5de6
bece3e1
4820eff
84521cb
f9e3840
8a80eef
dd69f0d
6e4810d
024ad02
015f972
41e007e
5b958c5
68e0195
4f972ff
bc9d554
25ea1b1
515b5ff
8104e36
4075943
92ecaf1
e29e0e8
28dca79
4ac1742
cfc3418
0b75df3
4483758
f992010
2b56131
f113a41
b1ff6d1
abeace9
caf2431
806d14a
4ed331e
32d07ba
bd54e37
6e5b414
3b69059
b1f5837
f324936
2d3e5a9
3a23df8
1b72493
a4fcd03
cb1cfab
4c5af5a
40f136b
6856457
8dfeac3
8b7ed24
9895781
d64ddb5
97156d1
cd0d492
588cb56
a57c6d0
fc2c9be
7ba13d8
28aef07
81cd525
182648a
6eef8ad
aed1c73
b23946e
99bc200
ae5efa4
a030b1e
209a14f
2e470cd
bc618d5
13fefe6
f3dafad
fe1c20c
7e0c435
0eb995e
7d89e1e
78e6dd7
563d7b7
84139b6
9acf608
e6bcadd
2a98d64
2b31faa
70d4173
d0d4aff
b88022b
71f9419
08dd6a2
4e7a1c0
67275d8
f00af95
ac1046b
607aded
404705b
26f3787
50c6b4a
eae41dd
627ac68
82bc741
6b25e01
18e18d8
c90ce2c
dbf7928
3088e43
963a759
c2879de
b446923
3cf0dfd
9e3302e
2b04c4a
d3d43d3
99320bc
5989e3a
136e2f3
4d9584d
1ea6daf
c71c5a5
c374480
5065ce9
d9597f0
366b812
15249e1
cb98743
02c37d9
4f26e8c
8ba2097
906b2ff
ccca0c0
f0e1ff4
64b1276
d3f125a
ebd69f2
b39772d
db243ad
9e548a0
fd7977b
994c5e7
5b32226
aa41e2f
41f0bb8
35b9638
4d8f6d2
cf331b4
60ababa
d472124
6c7a10c
774aece
b73bb55
87dd683
1a4eb08
de28298
32d9bae
5c01d7b
57b7ad2
9d6a0e6
8f9e306
16fc028
d1d5e76
794c52b
9bc4aa5
e8a6fb9
55d4cad
1cfee77
a17738d
0f4d97c
52b7c96
bb6b869
f41da80
2d21376
af20f70
e36b55a
31e5433
39ba3ea
8777657
e6c676e
25edf4c
54b6f2a
e2c02d6
1e72a46
ee994f1
8bcac69
05cb7bf
12bd6fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3964,6 +3964,10 @@ | |
"category": "Suggestion", | ||
"code": 80005 | ||
}, | ||
"This may be converted to use async and await.": { | ||
"category": "Suggestion", | ||
"code": 80006 | ||
}, | ||
|
||
"Add missing 'super()' call": { | ||
"category": "Message", | ||
|
@@ -4280,5 +4284,13 @@ | |
"Remove all unused labels": { | ||
"category": "Message", | ||
"code": 95054 | ||
}, | ||
"Convert to use async and await":{ | ||
"category": "Message", | ||
"code": 95055 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is an extra space here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'd prefer "Convert to async function", in case anyone else does. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for @bterlson comment. also for the suggestion message as well. |
||
}, | ||
"Convert all to use async and await": { | ||
"category": "Message", | ||
"code": 95056 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,128 @@ | ||
namespace ts.codefix { | ||
const fixId = "convertPromisesToAwaitAndAsync"; | ||
const errorCodes = [Diagnostics.This_may_be_converted_to_use_async_and_await.code]; | ||
registerCodeFix({ | ||
errorCodes, | ||
getCodeActions(context: CodeFixContext) { | ||
const changes = textChanges.ChangeTracker.with(context, (t) => convertToAsyncAwait(t, context.sourceFile, context.span.start, context.program.getTypeChecker())); | ||
return [createCodeFixAction(fixId, changes, Diagnostics.Convert_to_use_async_and_await, fixId, Diagnostics.Convert_all_to_use_async_and_await)]; | ||
}, | ||
fixIds: [fixId], | ||
getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, err) => convertToAsyncAwait(changes, err.file!, err.start, context.program.getTypeChecker())), | ||
}); | ||
function convertToAsyncAwait(changes: textChanges.ChangeTracker, sourceFile: SourceFile, position: number, checker: TypeChecker): void { | ||
// get the function declaration - returns a promise | ||
const funcToConvert: FunctionLikeDeclaration = checker.getSymbolAtLocation(getTokenAtPosition(sourceFile, position, /*includeEndPosition*/ false)).valueDeclaration as FunctionLikeDeclaration; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that seems like a round about way to just get |
||
// add the async keyword | ||
changes.insertModifierBefore(sourceFile, SyntaxKind.AsyncKeyword, funcToConvert); | ||
|
||
const stmts: NodeArray<Statement> = (<FunctionBody>funcToConvert.body).statements; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. call the same function you had in suggestionDiagnostics.ts to ensure we are doing the same thing in both places. if the function returns you an empty set, return undefined immediately. |
||
for (const stmt of stmts) { | ||
forEachChild(stmt, function visit(node: Node) { | ||
if (node.kind === SyntaxKind.CallExpression) { | ||
const newNode = parseCallback(node as CallExpression, checker); | ||
if (newNode) { | ||
changes.replaceNodeWithNodes(sourceFile, stmt, newNode); | ||
} | ||
} | ||
else if (!isFunctionLike(node)) { | ||
forEachChild(node, visit); | ||
} | ||
}); | ||
} | ||
} | ||
|
||
function parseCallback(node: Expression, checker: TypeChecker, argName?: string, argUsed?: boolean): Statement[] { | ||
if (!node) { | ||
return; | ||
} | ||
|
||
if (node.kind === SyntaxKind.CallExpression && checker.isPromiseLikeType(checker.getTypeAtLocation(node)) && (<CallExpression>node).expression.kind !== SyntaxKind.PropertyAccessExpression) { | ||
return parsePromiseCall(node as CallExpression, argName, argUsed); | ||
} | ||
else if (node.kind === SyntaxKind.CallExpression && isCallback(node as CallExpression, "then", checker)) { | ||
return parseThen(node as CallExpression, checker); | ||
} | ||
else if (node.kind === SyntaxKind.CallExpression && isCallback(node as CallExpression, "catch", checker)) { | ||
return parseCatch(node as CallExpression, checker); | ||
} | ||
else if (node.kind === SyntaxKind.PropertyAccessExpression) { | ||
return parseCallback((<PropertyAccessExpression>node).expression, checker, argName, argUsed); | ||
} | ||
} | ||
|
||
function parseCatch(node: CallExpression, checker: TypeChecker): Statement[] { | ||
const func = node.arguments[0]; | ||
const argName = getArgName(func, "arg", checker); | ||
const tryBlock = createBlock(parseCallback(node.expression, checker, argName, argName !== "arg")); | ||
// instead of using e -> get the paramater of the catch function and use that | ||
const catchClause = createCatchClause(argName, createBlock(getCallbackBody(func, argName))); | ||
return [createTry(tryBlock, catchClause, /*finallyBlock*/ undefined)]; | ||
} | ||
|
||
function parseThen(node: CallExpression, checker: TypeChecker): Statement[] { | ||
const res = node.arguments[0]; | ||
const rej = node.arguments[1]; | ||
// TODO - what if this is a binding pattern and not an Identifier | ||
const argNameRes = getArgName(res, "val", checker); | ||
|
||
if (rej) { | ||
const argNameRej = getArgName(rej, "e", checker); | ||
|
||
const tryBlock = createBlock(parseCallback(node.expression, checker, argNameRes, argNameRes !== "val")); | ||
const catchClause = createCatchClause(argNameRej, createBlock(getCallbackBody(rej, argNameRej))); | ||
|
||
return [createTry(tryBlock, catchClause, /*finalllyBlock*/ undefined) as Statement].concat(getCallbackBody(res, argNameRes)); | ||
} | ||
else { | ||
return parseCallback(node.expression, checker, argNameRes, argNameRes !== "val").concat(getCallbackBody(res, argNameRes)); | ||
} | ||
} | ||
|
||
function parsePromiseCall(node: CallExpression, argName?: string, argUsed?: boolean): Statement[] { | ||
if (!argName) { | ||
argName = "val"; // fix this to maybe not always create a variable declaration if not necessary | ||
} | ||
return argUsed ? [createVariableStatement(/*modifiers*/ undefined, [createVariableDeclaration(createIdentifier(argName), /*type*/ undefined, createAwait(node))])] : | ||
[createStatement(createAwait(node))]; | ||
} | ||
|
||
function getCallbackBody(func: Node, argName: string): NodeArray<Statement> { | ||
switch (func.kind) { | ||
case SyntaxKind.Identifier: | ||
return createNodeArray([(createReturn(createAwait(createCall(func as Identifier, /*typeArguments*/ undefined, [createIdentifier(argName)]))))]); | ||
case SyntaxKind.FunctionDeclaration: | ||
case SyntaxKind.ArrowFunction: | ||
if ((<FunctionDeclaration>func).body.kind === SyntaxKind.Block) { | ||
return (<FunctionDeclaration>func).body.statements; | ||
} | ||
else { | ||
return createNodeArray([createStatement((<ArrowFunction>func).body as Expression)]); | ||
} | ||
} | ||
} | ||
function isCallback(node: CallExpression, funcName: string, checker: TypeChecker): boolean { | ||
if (node.expression.kind !== SyntaxKind.PropertyAccessExpression) { | ||
return false; | ||
} | ||
return (<PropertyAccessExpression>node.expression).name.text === funcName && checker.isPromiseLikeType(checker.getTypeAtLocation(node)); | ||
} | ||
function getArgName(funcNode: Node, defaultVal: string, checker: TypeChecker): string { | ||
if (isFunctionLikeDeclaration(funcNode) && funcNode.parameters.length > 0) { | ||
const name = (<Identifier>funcNode.parameters[0].name).text; | ||
if (name !== "_") { | ||
return name; | ||
} | ||
else { | ||
return defaultVal; | ||
} | ||
} | ||
else if (checker.getTypeAtLocation(funcNode).getCallSignatures().length > 0 && checker.getTypeAtLocation(funcNode).getCallSignatures()[0].parameters.length > 0) { | ||
const name = checker.getTypeAtLocation(funcNode).getCallSignatures()[0].parameters[0].name; | ||
return name; | ||
} | ||
else { | ||
return defaultVal; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,12 +17,30 @@ namespace ts { | |
switch (node.kind) { | ||
case SyntaxKind.FunctionDeclaration: | ||
case SyntaxKind.FunctionExpression: | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you should also handel methods, and lambdas |
||
if (isJsFile) { | ||
const symbol = node.symbol; | ||
if (symbol.members && (symbol.members.size > 0)) { | ||
diags.push(createDiagnosticForNode(isVariableDeclaration(node.parent) ? node.parent.name : node, Diagnostics.This_constructor_function_may_be_converted_to_a_class_declaration)); | ||
} | ||
} | ||
if (node.modifiers && isAsyncFunction(node)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do not need to check for node.modifiers. isAsyncFunction should be sufficient. |
||
break; | ||
} | ||
|
||
const returnType = checker.getReturnTypeOfSignature(checker.getSignatureFromDeclaration(<FunctionDeclaration | FunctionExpression>node)); | ||
if (!returnType || !returnType.symbol) { | ||
break; | ||
} | ||
|
||
// collect all the return statements | ||
// check that a property access expression exists in there and that it is a handler | ||
const retStmts: ReturnStatement[] = []; | ||
getReturnStmts(node, retStmts); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i would recommend having all this logic in one function, and sharing it between the quick fix and the suggestion code. this way we can use them to report the suggestion, and check in the quick fix if the assumptions are still valid. this comes up when VSCode, say call us to apply a fix after a code change, so it is possible that the previous code where we reported the suggestion has change. |
||
|
||
if (isPromiseType(returnType) && retStmts.length > 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. check for the promise first before getting the return statements. |
||
diags.push(createDiagnosticForNode(isVariableDeclaration(node.parent) ? node.parent.name : node, Diagnostics.This_may_be_converted_to_use_async_and_await)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how can this be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also. these nodes can span multiple lines. i would not put the span under the whole node, this way you would get teh suggestion deep inside a the handler in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. never mind my previous comments. i thought we are putting the span on the return statements.. now i see you are putting it on the function node. I think we should be putting the span on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the span should be on the entire function as we need to put the async keyword before the signature. Or should the two transformations: (1) adding async keyword (2) refactoring |
||
} | ||
break; | ||
} | ||
|
||
|
@@ -104,4 +122,34 @@ namespace ts { | |
function getErrorNodeFromCommonJsIndicator(commonJsModuleIndicator: Node): Node { | ||
return isBinaryExpression(commonJsModuleIndicator) ? commonJsModuleIndicator.left : commonJsModuleIndicator; | ||
} | ||
|
||
function isPromiseType(T: Type): boolean { | ||
return T.flags === TypeFlags.Object && T.symbol.name === "Promise"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as i mentioned in a previous iteration, i do not think this is the correct check. we should instead expose |
||
} | ||
|
||
function getReturnStmts(node: Node, retStmts: ReturnStatement[]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit. spell out Statements in the function name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also i would make the result created inside this function and not a parameter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would also call it |
||
forEachChild(node, visit); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit. extra empty line |
||
function visit(node: Node) { | ||
if (isFunctionLike(node)) { | ||
return; | ||
} | ||
|
||
if (node.kind === SyntaxKind.ReturnStatement) { | ||
forEachChild(node, hasCallback); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you do not need the call to function visit(node: Node) {
if (isReturnStatement(node)) {
if (isCallExpression(child) && isPropertyAccessExpression(child.expression) &&
(child.expression.name.text === "then" || child.expression.name.text === "catch" || child.expression.name.text === "finally")) {
result.push(node);
}
}
else if (!isFunctionLike(node)) {
forEachChild(node, visit);
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also using the functions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we do still need to visit all children. The node we are passing in is the entire function declaration. |
||
} | ||
|
||
function hasCallback(child: Node) { | ||
if (child.kind === SyntaxKind.CallExpression && (<CallExpression>child).expression.kind === SyntaxKind.PropertyAccessExpression && | ||
((<PropertyAccessExpression>(<CallExpression>child).expression).name.text === "then" || (<PropertyAccessExpression>(<CallExpression>child).expression).name.text === "catch")) { | ||
retStmts.push(node as ReturnStatement); | ||
} | ||
else if (!isFunctionLike(child)) { | ||
forEachChild(child, hasCallback); | ||
} | ||
} | ||
|
||
forEachChild(node, visit); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
// @target: es6 | ||
|
||
////function [|f|](): Promise<void> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might want to have a look at the way we test Extract Function (grab me for details). This will be a little different, since it's a code fix rather than a refactoring, but there's something to be said for re-generatable baselines. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this should use baseline machinery I think. |
||
//// return fetch('http://yahoo.com').then(result => { console.log(result); }); | ||
////} | ||
|
||
verify.getSuggestionDiagnostics([{ | ||
message: "This may be converted to use async and await.", | ||
code: 80006, | ||
}]); | ||
|
||
verify.codeFix({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The description here looks wrong. |
||
description: "Convert to use async and await", | ||
index: 0, | ||
newFileContent: | ||
`async function f(): Promise<void> { | ||
var result = await fetch('http://yahoo.com'); | ||
console.log(result); | ||
}`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
// @target: es6 | ||
|
||
////function [|f|]():Promise<void> { | ||
//// return fetch('http://yahoo.com').then(result => { console.log(result); }).catch(err => { console.log(err); }); | ||
////} | ||
|
||
verify.getSuggestionDiagnostics([{ | ||
message: "This may be converted to use async and await.", | ||
code: 80006, | ||
}]); | ||
|
||
|
||
verify.codeFix({ | ||
description: "Convert to use async and await", | ||
index: 0, | ||
newFileContent: | ||
`async function f():Promise<void> { | ||
try { | ||
var result = await fetch('http://yahoo.com'); | ||
console.log(result); | ||
} | ||
catch (err) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Predominant JS style is catch on the same line as the closing brace. Style is called "One True Brace Style" or 1TBS. |
||
console.log(err); | ||
} | ||
}`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
// @target: es6 | ||
|
||
////function [|f|]():Promise<void> { | ||
//// return fetch('http://yahoo.com').then(result => { console.log(result); }, rejection => { console.log("rejected:", rejection); }).catch(err => { console.log(err) }); | ||
////} | ||
|
||
verify.getSuggestionDiagnostics([{ | ||
message: "This may be converted to use async and await.", | ||
code: 80006, | ||
}]); | ||
|
||
|
||
verify.codeFix({ | ||
description: "Convert to use async and await", | ||
index: 0, | ||
newFileContent: | ||
`async function f():Promise<void> { | ||
try { | ||
try { | ||
var result = await fetch('http://yahoo.com'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think others have mentioned that using |
||
} | ||
catch (rejection) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would people prefer to see |
||
console.log("rejected:", rejection); | ||
} | ||
console.log(result); | ||
} | ||
catch (err) { | ||
console.log(err); | ||
} | ||
}`, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the commented out code.