-
Notifications
You must be signed in to change notification settings - Fork 13k
Only perform async code fix if it can successfully refactor all parts #26930
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
Only perform async code fix if it can successfully refactor all parts #26930
Conversation
return true; | ||
} | ||
|
||
function isPromiseHandler(node: Node): node is CallExpression { |
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.
node is CallExpression [](start = 43, length = 22)
I remember being told not to do this if it didn't accept all CallExpressions, but I may have misunderstood.
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.
I'm not sure why that would be true...
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.
It is technically unsound since it filters that type out of unions:
class A { constructor(readonly a: number) {} }
class B { constructor(readonly b: number) {} }
function isA42(ab: A | B): ab is A {
return ab instanceof A && ab.a === 42;
}
function f(ab: A | B): number {
if (isA42(ab)) {
return ab.a;
} else {
return ab.b; // TS thinks this must be `B`
}
}
f(new A(43)).toFixed();
However, TypeScript doesn't give us a way to declare that a function accepts only inputs of some type but not all inputs of some type. And without the type guard you'll need casts elsewhere, which are also unsound. So I think this is fine.
// should be kept up to date with getTransformationBody in convertToAsyncFunction.ts | ||
function isFixablePromiseArgument(arg: Expression): boolean { | ||
switch (arg.kind) { | ||
case SyntaxKind.NullKeyword: |
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.
NullKeyword [](start = 28, length = 11)
Null but not undefined?
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.
Nope. From what I understand, SyntaxKind.UndefinedKeyword
refers to only the undefined
type keyword. undefined
in the value space is an Identifier
with the name undefined
.
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.
Weird.
} | ||
|
||
// should be kept up to date with getTransformationBody in convertToAsyncFunction.ts | ||
function isFixablePromiseArgument(arg: Expression): boolean { |
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.
isFixablePromiseArgument [](start = 13, length = 24)
I don't feel strongly about it but I thought we had concluded this belonged in the code fix (i.e. we could make suggestions more often than we offered fixes). Did something change?
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.
Yeah, I spoke to @RyanCavanaugh who felt strongly that the suggestion diagnostic should be equally conservative as the code fix.
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.
Suits me.
namespace ts.codefix { | ||
const fixId = "convertToAsyncFunction"; | ||
const errorCodes = [Diagnostics.This_may_be_converted_to_an_async_function.code]; | ||
let codeActionSucceeded = true; |
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.
codeActionSucceeded [](start = 8, length = 19)
I assume this is the thing you were talking about when you asked about exceptions. It's not what I would have done, but it sounds like you've already checked the local conventions. I might change it to codeActionFailed
though. As long as we're going down this path, would it make sense to have an error code indicating how it went wrong and then send a telemetry event when we see a failure?
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.
I'm not sure this is an appropriate place to bubble up an error code given the change to be more conservative about offering the diagnostic. That is, we won't show the diagnostic if we know the code fix is going to fail.
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.
Sorry, I think I was unclear. I didn't mean a diagnostic error code, I meant an enum or something that would tell you the reason the code action didn't succeed. And bubbling up would be to us, via telemetry, not to the user.
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.
Fixes #26375.
Currently, there's a limited set of syntax forms that we are able to refactor for the async code fix (namely, identifiers, arrow functions, and function expressions). If we try to refactor a promise operation (e.g.,
.then
), with one of its arguments not in this form, we end up deleting that code.This PR handles this more gracefully by not returning a code action if not all promise operations could be refactored successfully. Additionally, this PR introduces the ability to test for this situation (i.e., a diagnostic is produced but no action is available).
Note that in VS, no suggestion diagnostic is shown due to the lack of available codefixes. However, in VS Code, the suggestion diagnostic is shown, but no lightbulb is offered.