Skip to content

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Sep 14, 2018
Merged

Only perform async code fix if it can successfully refactor all parts #26930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a72da7
Only perform async refactor if it won't delete code
Sep 5, 2018
f7f5b1a
Don't case on type node
Sep 5, 2018
95d5788
Ensure diagnostic reporting matches code fix ability
Sep 7, 2018
57a6dbd
Add clarifying comments
Sep 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions src/services/codefixes/convertToAsyncFunction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

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)),
Expand Down Expand Up @@ -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 [];
Expand All @@ -273,6 +276,7 @@ namespace ts.codefix {
return transformPromiseCall(node, transformer, prevArgName);
}

codeActionSucceeded = false;
return [];
}

Expand Down Expand Up @@ -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<Statement> {

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]);
Expand Down Expand Up @@ -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([]);
Expand Down Expand Up @@ -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;
Expand Down
39 changes: 35 additions & 4 deletions src/services/suggestionDiagnostics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ namespace ts {
}

function addHandlers(returnChild: Node) {
if (isPromiseHandler(returnChild)) {
if (isFixablePromiseHandler(returnChild)) {
returnStatements.push(child as ReturnStatement);
}
}
Expand All @@ -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 {
Copy link
Member

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.

Copy link
Contributor Author

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...

Copy link

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.

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 {
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suits me.

switch (arg.kind) {
case SyntaxKind.NullKeyword:
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird.

case SyntaxKind.Identifier: // identifier includes undefined
case SyntaxKind.FunctionDeclaration:
case SyntaxKind.FunctionExpression:
case SyntaxKind.ArrowFunction:
return true;
default:
return false;
}
}
}
8 changes: 8 additions & 0 deletions src/services/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
Loading