convertToAsyncFunction: Fix two-argument then#37588
convertToAsyncFunction: Fix two-argument then#37588andrewbranch wants to merge 3 commits intomicrosoft:masterfrom
Conversation
sandersn
added
the
For Milestone Bug
amcasey
left a comment
amcasey
left a comment
There was a problem hiding this comment.
I didn't dig into the reasons this change was needed, but I agree that the code change has the behavior you claim.
| const transformationBody = getTransformationBody(onFulfilled, prevArgName, onFulfilledArgumentName, node, transformer); | ||
|
|
||
| if (onRejected) { | ||
| // From: |
There was a problem hiding this comment.
This is great. I wish more of our transformations had comments like these.
| function ensureReturnOrThrow(statements: readonly Statement[]): readonly Statement[] { | ||
| const lastStatement = lastOrUndefined(statements); | ||
| if (lastStatement?.kind !== SyntaxKind.ReturnStatement && lastStatement?.kind !== SyntaxKind.ThrowStatement) { | ||
| return statements.concat(createReturn()); |
There was a problem hiding this comment.
Looks like this could be unreachable (e.g. if the last statement is actually an if-else in which both branches return)?
There was a problem hiding this comment.
Hm, you’re right. I’m not sure there’s an efficient way to know what to do here.
| const transformationBody2 = getTransformationBody(onRejected, prevArgName, onRejectedArgumentName, node, transformer); | ||
| const onFulfilledArgType = onFulfilledArgumentName && !transformer.isInJSFile && transformer.checker.getTypeAtLocation(getNode(onFulfilledArgumentName)); | ||
| const onFulfilledArgTypeNode = onFulfilledArgType && !(onFulfilledArgType.flags & TypeFlags.Any) ? transformer.checker.typeToTypeNode(onFulfilledArgType) : undefined; | ||
| const onFulfilledArgVariableDeclaration = createVariableOrAssignmentOrExpressionStatement(onFulfilledArgumentName, /*rightHandSide*/ undefined, onFulfilledArgTypeNode); |
There was a problem hiding this comment.
Does this name need to be unique-ified?
tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchAndRej.ts
Show resolved
Hide resolved
|
I think I’m convinced that any refactored code that comes from a two-argument then is either going to be ugly, wrong, or both. I’m going to open a new PR disabling the refactor for two-argument thens. |
5 participants
Fixes #27621 - a two-argument
thenis supported only if no other.thenfollows it in the chain.