-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Add quick fix to add missing 'await' #32356
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
Merged
andrewbranch
merged 16 commits into
microsoft:master
from
andrewbranch:enhancement/add-missing-await
Jul 12, 2019
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
0612c89
Start prototyping addMissingAwait codefix
andrewbranch 9013ce3
Filter by diagnostics that have missing-await related info
andrewbranch 5a5b15f
Start writing tests and checking precedence
andrewbranch a873d6b
Implement codeFixAll, add test for binary expressions
andrewbranch c692209
Add test for iterables
andrewbranch 8237f12
Add test for passing argument
andrewbranch 7be4e0a
Add test for call/construct signatures
andrewbranch 01e2e53
Add test for awaiting initializer
andrewbranch fce502d
Improve assertion error
andrewbranch f160336
Replace specific property access error with general one and add await…
andrewbranch 4643d27
Add test for property access
andrewbranch 355469e
Require code to be inside a function body to offer await
andrewbranch 9e5f261
Accept suggestion
andrewbranch 3b90f5b
Add explicit test for code fix being not available unless something i…
andrewbranch 657daa4
Skip looking for function body if already in AwaitContext flags
andrewbranch 09853be
Inline getCodeActions function for symmetry
andrewbranch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,173 @@ | ||
/* @internal */ | ||
namespace ts.codefix { | ||
type ContextualTrackChangesFunction = (cb: (changeTracker: textChanges.ChangeTracker) => void) => FileTextChanges[]; | ||
const fixId = "addMissingAwait"; | ||
const propertyAccessCode = Diagnostics.Property_0_does_not_exist_on_type_1.code; | ||
const callableConstructableErrorCodes = [ | ||
Diagnostics.This_expression_is_not_callable.code, | ||
Diagnostics.This_expression_is_not_constructable.code, | ||
]; | ||
const errorCodes = [ | ||
Diagnostics.An_arithmetic_operand_must_be_of_type_any_number_bigint_or_an_enum_type.code, | ||
Diagnostics.The_left_hand_side_of_an_arithmetic_operation_must_be_of_type_any_number_bigint_or_an_enum_type.code, | ||
Diagnostics.The_right_hand_side_of_an_arithmetic_operation_must_be_of_type_any_number_bigint_or_an_enum_type.code, | ||
Diagnostics.Operator_0_cannot_be_applied_to_type_1.code, | ||
Diagnostics.Operator_0_cannot_be_applied_to_types_1_and_2.code, | ||
Diagnostics.This_condition_will_always_return_0_since_the_types_1_and_2_have_no_overlap.code, | ||
Diagnostics.Type_0_is_not_an_array_type.code, | ||
Diagnostics.Type_0_is_not_an_array_type_or_a_string_type.code, | ||
Diagnostics.Type_0_is_not_an_array_type_or_a_string_type_Use_compiler_option_downlevelIteration_to_allow_iterating_of_iterators.code, | ||
Diagnostics.Type_0_is_not_an_array_type_or_a_string_type_or_does_not_have_a_Symbol_iterator_method_that_returns_an_iterator.code, | ||
Diagnostics.Type_0_is_not_an_array_type_or_does_not_have_a_Symbol_iterator_method_that_returns_an_iterator.code, | ||
Diagnostics.Type_0_must_have_a_Symbol_iterator_method_that_returns_an_iterator.code, | ||
Diagnostics.Type_0_must_have_a_Symbol_asyncIterator_method_that_returns_an_async_iterator.code, | ||
Diagnostics.Argument_of_type_0_is_not_assignable_to_parameter_of_type_1.code, | ||
propertyAccessCode, | ||
...callableConstructableErrorCodes, | ||
]; | ||
|
||
registerCodeFix({ | ||
fixIds: [fixId], | ||
errorCodes, | ||
getCodeActions: context => { | ||
const { sourceFile, errorCode, span, cancellationToken, program } = context; | ||
const expression = getAwaitableExpression(sourceFile, errorCode, span, cancellationToken, program); | ||
if (!expression) { | ||
return; | ||
} | ||
|
||
const checker = context.program.getTypeChecker(); | ||
const trackChanges: ContextualTrackChangesFunction = cb => textChanges.ChangeTracker.with(context, cb); | ||
return compact([ | ||
getDeclarationSiteFix(context, expression, errorCode, checker, trackChanges), | ||
getUseSiteFix(context, expression, errorCode, checker, trackChanges)]); | ||
}, | ||
getAllCodeActions: context => { | ||
andrewbranch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const { sourceFile, program, cancellationToken } = context; | ||
const checker = context.program.getTypeChecker(); | ||
return codeFixAll(context, errorCodes, (t, diagnostic) => { | ||
const expression = getAwaitableExpression(sourceFile, diagnostic.code, diagnostic, cancellationToken, program); | ||
if (!expression) { | ||
return; | ||
} | ||
const trackChanges: ContextualTrackChangesFunction = cb => (cb(t), []); | ||
return getDeclarationSiteFix(context, expression, diagnostic.code, checker, trackChanges) | ||
|| getUseSiteFix(context, expression, diagnostic.code, checker, trackChanges); | ||
}); | ||
}, | ||
}); | ||
|
||
function getDeclarationSiteFix(context: CodeFixContext | CodeFixAllContext, expression: Expression, errorCode: number, checker: TypeChecker, trackChanges: ContextualTrackChangesFunction) { | ||
const { sourceFile } = context; | ||
const awaitableInitializer = findAwaitableInitializer(expression, sourceFile, checker); | ||
if (awaitableInitializer) { | ||
const initializerChanges = trackChanges(t => makeChange(t, errorCode, sourceFile, checker, awaitableInitializer)); | ||
return createCodeFixActionNoFixId( | ||
"addMissingAwaitToInitializer", | ||
initializerChanges, | ||
[Diagnostics.Add_await_to_initializer_for_0, expression.getText(sourceFile)]); | ||
} | ||
} | ||
|
||
function getUseSiteFix(context: CodeFixContext | CodeFixAllContext, expression: Expression, errorCode: number, checker: TypeChecker, trackChanges: ContextualTrackChangesFunction) { | ||
const changes = trackChanges(t => makeChange(t, errorCode, context.sourceFile, checker, expression)); | ||
return createCodeFixAction(fixId, changes, Diagnostics.Add_await, fixId, Diagnostics.Fix_all_expressions_possibly_missing_await); | ||
} | ||
|
||
function isMissingAwaitError(sourceFile: SourceFile, errorCode: number, span: TextSpan, cancellationToken: CancellationToken, program: Program) { | ||
const checker = program.getDiagnosticsProducingTypeChecker(); | ||
const diagnostics = checker.getDiagnostics(sourceFile, cancellationToken); | ||
return some(diagnostics, ({ start, length, relatedInformation, code }) => | ||
isNumber(start) && isNumber(length) && textSpansEqual({ start, length }, span) && | ||
code === errorCode && | ||
!!relatedInformation && | ||
some(relatedInformation, related => related.code === Diagnostics.Did_you_forget_to_use_await.code)); | ||
} | ||
|
||
function getAwaitableExpression(sourceFile: SourceFile, errorCode: number, span: TextSpan, cancellationToken: CancellationToken, program: Program): Expression | undefined { | ||
const token = getTokenAtPosition(sourceFile, span.start); | ||
// Checker has already done work to determine that await might be possible, and has attached | ||
// related info to the node, so start by finding the expression that exactly matches up | ||
// with the diagnostic range. | ||
const expression = findAncestor(token, node => { | ||
if (node.getStart(sourceFile) < span.start || node.getEnd() > textSpanEnd(span)) { | ||
return "quit"; | ||
} | ||
return isExpression(node) && textSpansEqual(span, createTextSpanFromNode(node, sourceFile)); | ||
}) as Expression | undefined; | ||
|
||
return expression | ||
&& isMissingAwaitError(sourceFile, errorCode, span, cancellationToken, program) | ||
&& isInsideAwaitableBody(expression) | ||
? expression | ||
: undefined; | ||
} | ||
|
||
function findAwaitableInitializer(expression: Node, sourceFile: SourceFile, checker: TypeChecker): Expression | undefined { | ||
if (!isIdentifier(expression)) { | ||
return; | ||
} | ||
|
||
const symbol = checker.getSymbolAtLocation(expression); | ||
if (!symbol) { | ||
return; | ||
} | ||
|
||
const declaration = tryCast(symbol.valueDeclaration, isVariableDeclaration); | ||
const variableName = tryCast(declaration && declaration.name, isIdentifier); | ||
const variableStatement = getAncestor(declaration, SyntaxKind.VariableStatement); | ||
if (!declaration || !variableStatement || | ||
declaration.type || | ||
!declaration.initializer || | ||
variableStatement.getSourceFile() !== sourceFile || | ||
hasModifier(variableStatement, ModifierFlags.Export) || | ||
!variableName || | ||
!isInsideAwaitableBody(declaration.initializer)) { | ||
return; | ||
} | ||
|
||
const isUsedElsewhere = FindAllReferences.Core.eachSymbolReferenceInFile(variableName, checker, sourceFile, identifier => { | ||
return identifier !== expression; | ||
}); | ||
|
||
if (isUsedElsewhere) { | ||
return; | ||
} | ||
|
||
return declaration.initializer; | ||
} | ||
|
||
function isInsideAwaitableBody(node: Node) { | ||
return node.kind & NodeFlags.AwaitContext || !!findAncestor(node, ancestor => | ||
ancestor.parent && isArrowFunction(ancestor.parent) && ancestor.parent.body === ancestor || | ||
isBlock(ancestor) && ( | ||
ancestor.parent.kind === SyntaxKind.FunctionDeclaration || | ||
ancestor.parent.kind === SyntaxKind.FunctionExpression || | ||
ancestor.parent.kind === SyntaxKind.ArrowFunction || | ||
ancestor.parent.kind === SyntaxKind.MethodDeclaration)); | ||
} | ||
|
||
function makeChange(changeTracker: textChanges.ChangeTracker, errorCode: number, sourceFile: SourceFile, checker: TypeChecker, insertionSite: Expression) { | ||
if (isBinaryExpression(insertionSite)) { | ||
const { left, right } = insertionSite; | ||
const leftType = checker.getTypeAtLocation(left); | ||
const rightType = checker.getTypeAtLocation(right); | ||
const newLeft = checker.getPromisedTypeOfPromise(leftType) ? createAwait(left) : left; | ||
const newRight = checker.getPromisedTypeOfPromise(rightType) ? createAwait(right) : right; | ||
changeTracker.replaceNode(sourceFile, left, newLeft); | ||
changeTracker.replaceNode(sourceFile, right, newRight); | ||
} | ||
else if (errorCode === propertyAccessCode && isPropertyAccessExpression(insertionSite.parent)) { | ||
changeTracker.replaceNode( | ||
sourceFile, | ||
insertionSite.parent.expression, | ||
createParen(createAwait(insertionSite.parent.expression))); | ||
} | ||
else if (contains(callableConstructableErrorCodes, errorCode) && isCallOrNewExpression(insertionSite.parent)) { | ||
changeTracker.replaceNode(sourceFile, insertionSite, createParen(createAwait(insertionSite))); | ||
} | ||
else { | ||
changeTracker.replaceNode(sourceFile, insertionSite, createAwait(insertionSite)); | ||
} | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/// <reference path="fourslash.ts" /> | ||
////async function fn(a: Promise<string>, b: string) { | ||
//// fn(a, a); | ||
////} | ||
|
||
verify.codeFix({ | ||
description: ts.Diagnostics.Add_await.message, | ||
index: 0, | ||
newFileContent: | ||
`async function fn(a: Promise<string>, b: string) { | ||
fn(a, await a); | ||
}` | ||
}); |
50 changes: 50 additions & 0 deletions
50
tests/cases/fourslash/codeFixAddMissingAwait_binaryExpressions.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
/// <reference path="fourslash.ts" /> | ||
////async function fn(a: Promise<number>, b: number) { | ||
//// a | b; | ||
//// b + a; | ||
//// a + a; | ||
////} | ||
|
||
verify.codeFix({ | ||
description: ts.Diagnostics.Add_await.message, | ||
index: 0, | ||
newFileContent: | ||
`async function fn(a: Promise<number>, b: number) { | ||
await a | b; | ||
b + a; | ||
a + a; | ||
}` | ||
}); | ||
|
||
verify.codeFix({ | ||
description: ts.Diagnostics.Add_await.message, | ||
index: 1, | ||
newFileContent: | ||
`async function fn(a: Promise<number>, b: number) { | ||
a | b; | ||
b + await a; | ||
a + a; | ||
}` | ||
}); | ||
|
||
verify.codeFix({ | ||
description: ts.Diagnostics.Add_await.message, | ||
index: 2, | ||
newFileContent: | ||
`async function fn(a: Promise<number>, b: number) { | ||
a | b; | ||
b + a; | ||
await a + await a; | ||
}` | ||
}); | ||
|
||
verify.codeFixAll({ | ||
fixAllDescription: ts.Diagnostics.Fix_all_expressions_possibly_missing_await.message, | ||
fixId: "addMissingAwait", | ||
newFileContent: | ||
`async function fn(a: Promise<number>, b: number) { | ||
await a | b; | ||
b + await a; | ||
await a + await a; | ||
}` | ||
}); |
28 changes: 28 additions & 0 deletions
28
tests/cases/fourslash/codeFixAddMissingAwait_initializer.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
/// <reference path="fourslash.ts" /> | ||
////async function fn(a: string, b: Promise<string>) { | ||
//// const x = b; | ||
//// fn(x, b); | ||
//// fn(b, b); | ||
////} | ||
|
||
verify.codeFix({ | ||
description: "Add 'await' to initializer for 'x'", | ||
index: 0, | ||
newFileContent: | ||
`async function fn(a: string, b: Promise<string>) { | ||
const x = await b; | ||
fn(x, b); | ||
fn(b, b); | ||
}` | ||
}); | ||
|
||
verify.codeFixAll({ | ||
fixAllDescription: ts.Diagnostics.Fix_all_expressions_possibly_missing_await.message, | ||
fixId: "addMissingAwait", | ||
newFileContent: | ||
`async function fn(a: string, b: Promise<string>) { | ||
const x = await b; | ||
fn(x, b); | ||
fn(await b, b); | ||
}` | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This will show for any old type combination, like
number + Event
, right? What happens in that case? Does it just fail to do anything when you try to "add missing await"?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's supposed to be filtered out further in
getCodeActions
(it doesn’t show anything if you return undefined / empty array in there). This is back to the tradeoffs we were discussing about using related info as a “bonus error message” vs. copying and pasting all these into new, unique messages. I wrote this down as a larger LS design/API change to consider at some later point.