Skip to content

Add elaboration & quickfix for async-able arrow function #36342

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
merged 2 commits into from
Feb 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 11 additions & 0 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14153,6 +14153,17 @@ namespace ts {
Diagnostics.The_expected_type_comes_from_the_return_type_of_this_signature,
));
}
if ((getFunctionFlags(node) & FunctionFlags.Async) === 0
// exclude cases where source itself is promisy - this way we don't make a suggestion when relating
// an IPromise and a Promise that are slightly different
&& !getTypeOfPropertyOfType(sourceReturn, "then" as __String)
&& checkTypeRelatedTo(createPromiseType(sourceReturn), targetReturn, relation, /*errorNode*/ undefined)
) {
addRelatedInfo(resultObj.errors[resultObj.errors.length - 1], createDiagnosticForNode(
node,
Diagnostics.Did_you_mean_to_mark_this_function_as_async
));
}
return true;
}
}
Expand Down
87 changes: 87 additions & 0 deletions src/services/codefixes/addMissingAsync.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/* @internal */
namespace ts.codefix {
type ContextualTrackChangesFunction = (cb: (changeTracker: textChanges.ChangeTracker) => void) => FileTextChanges[];
const fixId = "addMissingAsync";
const errorCodes = [
Diagnostics.Argument_of_type_0_is_not_assignable_to_parameter_of_type_1.code,
Diagnostics.Type_0_is_not_assignable_to_type_1.code,
Diagnostics.Type_0_is_not_comparable_to_type_1.code
];

registerCodeFix({
fixIds: [fixId],
errorCodes,
getCodeActions: context => {
const { sourceFile, errorCode, cancellationToken, program, span } = context;
const diagnostic = find(program.getDiagnosticsProducingTypeChecker().getDiagnostics(sourceFile, cancellationToken), getIsMatchingAsyncError(span, errorCode));
Copy link
Member

Choose a reason for hiding this comment

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

Can this code be shared with isMissingAwaitError in addMissingAwait.ts? It seems like finding a diagnostic with particular related information might be a general utility.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite! In the other quick fix, we're checking if the diagnostic's span matches the input span, then returning the diagnostic if it matches and has the target related span code. In this quick fix, we don't do the filtering on related span at the matching step, because we're looking to find the span of the related span (so we can use that instead). So, yes, there's a common iteration bit, but to handle the bits variable between the two is essentially just a some with variable condition, onto which we then append an optional extra filter/map on the final result. Which seems like a bit too multifunctional for a single helper for my tastes.

But it should totally feel similar, since I started writing this quick fix by copying the addMissingAwait quick fix, which is why the span-matching check is almost identical in structure.

const directSpan = diagnostic && diagnostic.relatedInformation && find(diagnostic.relatedInformation, r => r.code === Diagnostics.Did_you_mean_to_mark_this_function_as_async.code) as TextSpan | undefined;

const decl = getFixableErrorSpanDeclaration(sourceFile, directSpan);
if (!decl) {
return;
}

const trackChanges: ContextualTrackChangesFunction = cb => textChanges.ChangeTracker.with(context, cb);
return [getFix(context, decl, trackChanges)];
},
getAllCodeActions: context => {
const { sourceFile } = context;
const fixedDeclarations = createMap<true>();
return codeFixAll(context, errorCodes, (t, diagnostic) => {
const span = diagnostic.relatedInformation && find(diagnostic.relatedInformation, r => r.code === Diagnostics.Did_you_mean_to_mark_this_function_as_async.code) as TextSpan | undefined;
const decl = getFixableErrorSpanDeclaration(sourceFile, span);
if (!decl) {
return;
}
const trackChanges: ContextualTrackChangesFunction = cb => (cb(t), []);
return getFix(context, decl, trackChanges, fixedDeclarations);
});
},
});

type FixableDeclaration = ArrowFunction | FunctionDeclaration | FunctionExpression | MethodDeclaration;
function getFix(context: CodeFixContext | CodeFixAllContext, decl: FixableDeclaration, trackChanges: ContextualTrackChangesFunction, fixedDeclarations?: Map<true>) {
const changes = trackChanges(t => makeChange(t, context.sourceFile, decl, fixedDeclarations));
return createCodeFixAction(fixId, changes, Diagnostics.Add_async_modifier_to_containing_function, fixId, Diagnostics.Add_all_missing_async_modifiers);
}

function makeChange(changeTracker: textChanges.ChangeTracker, sourceFile: SourceFile, insertionSite: FixableDeclaration, fixedDeclarations?: Map<true>) {
if (fixedDeclarations) {
if (fixedDeclarations.has(getNodeId(insertionSite).toString())) {
return;
}
}
fixedDeclarations?.set(getNodeId(insertionSite).toString(), true);
const cloneWithModifier = getSynthesizedDeepClone(insertionSite, /*includeTrivia*/ true);
cloneWithModifier.modifiers = createNodeArray(createModifiersFromModifierFlags(getModifierFlags(insertionSite) | ModifierFlags.Async));
cloneWithModifier.modifierFlagsCache = 0;
changeTracker.replaceNode(
sourceFile,
insertionSite,
cloneWithModifier);
}

function getFixableErrorSpanDeclaration(sourceFile: SourceFile, span: TextSpan | undefined): FixableDeclaration | undefined {
if (!span) return undefined;
const token = getTokenAtPosition(sourceFile, span.start);
// Checker has already done work to determine that async might be possible, and has attached
// related info to the node, so start by finding the signature that exactly matches up
// with the diagnostic range.
const decl = findAncestor(token, node => {
if (node.getStart(sourceFile) < span.start || node.getEnd() > textSpanEnd(span)) {
return "quit";
}
return (isArrowFunction(node) || isMethodDeclaration(node) || isFunctionExpression(node) || isFunctionDeclaration(node)) && textSpansEqual(span, createTextSpanFromNode(node, sourceFile));
}) as FixableDeclaration | undefined;

return decl;
}

function getIsMatchingAsyncError(span: TextSpan, errorCode: number) {
return ({ start, length, relatedInformation, code }: Diagnostic) =>
isNumber(start) && isNumber(length) && textSpansEqual({ start, length }, span) &&
code === errorCode &&
!!relatedInformation &&
some(relatedInformation, related => related.code === Diagnostics.Did_you_mean_to_mark_this_function_as_async.code);
}
}
1 change: 1 addition & 0 deletions src/services/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"refactorProvider.ts",
"codefixes/addConvertToUnknownForNonOverlappingTypes.ts",
"codefixes/addEmptyExportDeclaration.ts",
"codefixes/addMissingAsync.ts",
"codefixes/addMissingAwait.ts",
"codefixes/addMissingConst.ts",
"codefixes/addMissingDeclareProperty.ts",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,41 +37,51 @@ tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts(27,16): err
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:3:8: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:18:10: Did you mean to mark this function as 'async'?
c: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:4:8: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:19:10: Did you mean to mark this function as 'async'?
Copy link
Member

Choose a reason for hiding this comment

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

This is kind of a weird thing now that I think about it - why would I want to mark this as async? There's no await anywhere there.

If a user did write await in the function body, we already provide the same error message/quick fix to mark the function as async, so this could be annoying.

interface Stuff {
    b: () => Promise<string>;
}

function foo(): Stuff | Date {
    return {
        b: _ =>
          await "hello" // two error spans with duplicate related spans
        
    }
}

So this is only distinct actionable advice information for a user if they hadn't written await. Would making this async actually be the right thing? It does come up, it's unclear if it's the real fix to a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is kind of a weird thing now that I think about it - why would I want to mark this as async? There's no await anywhere there.

Because async () => "constant" is way, way more canonical and concise than () => Promise.resolve("constant") when the API you're using expects a function returning a promise.

So this is only distinct actionable advice information for a user if they hadn't written await

More like, using await means the containing function should be async to make what you're trying to do work, while if assignability to a promise return fails, it should be async to satisfy the API. Different reasons entirely that happen to have the same fix.

d: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:5:8: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:20:10: Did you mean to mark this function as 'async'?
e: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:6:8: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:21:10: Did you mean to mark this function as 'async'?
f: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:7:8: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:22:10: Did you mean to mark this function as 'async'?
g: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:8:8: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:23:10: Did you mean to mark this function as 'async'?
h: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:9:8: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:24:10: Did you mean to mark this function as 'async'?
i: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:10:8: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:25:10: Did you mean to mark this function as 'async'?
j: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:11:8: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:26:10: Did you mean to mark this function as 'async'?
k: () => 123
~~~
!!! error TS2322: Type 'number' is not assignable to type 'Promise<number>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:12:8: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:27:10: Did you mean to mark this function as 'async'?
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,41 +37,51 @@ tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts(27,14): er
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:3:6: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:18:8: Did you mean to mark this function as 'async'?
c: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:4:6: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:19:8: Did you mean to mark this function as 'async'?
d: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:5:6: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:20:8: Did you mean to mark this function as 'async'?
e: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:6:6: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:21:8: Did you mean to mark this function as 'async'?
f: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:7:6: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:22:8: Did you mean to mark this function as 'async'?
g: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:8:6: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:23:8: Did you mean to mark this function as 'async'?
h: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:9:6: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:24:8: Did you mean to mark this function as 'async'?
i: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:10:6: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:25:8: Did you mean to mark this function as 'async'?
j: () => "hello",
~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:11:6: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:26:8: Did you mean to mark this function as 'async'?
k: () => 123
~~~
!!! error TS2322: Type 'number' is not assignable to type 'Promise<number>'.
!!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:12:6: The expected type comes from the return type of this signature.
!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate2.ts:27:8: Did you mean to mark this function as 'async'?
}
}
26 changes: 26 additions & 0 deletions tests/cases/fourslash/codeFixAddMissingAsync.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/// <reference path="fourslash.ts" />
////interface Stuff {
//// b: () => Promise<string>;
////}
////
////function foo(): Stuff | Date {
//// return {
//// b: () => "hello",
//// }
////}

verify.codeFix({
description: ts.Diagnostics.Add_async_modifier_to_containing_function.message,
index: 0,
newFileContent:
`interface Stuff {
b: () => Promise<string>;
}

function foo(): Stuff | Date {
return {
b: async () => "hello",
}
}`
});

26 changes: 26 additions & 0 deletions tests/cases/fourslash/codeFixAddMissingAsync2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/// <reference path="fourslash.ts" />
////interface Stuff {
//// b: () => Promise<string>;
////}
////
////function foo(): Stuff | Date {
//// return {
//// b: _ => "hello",
//// }
////}

verify.codeFix({
description: ts.Diagnostics.Add_async_modifier_to_containing_function.message,
index: 0,
newFileContent:
`interface Stuff {
b: () => Promise<string>;
}

function foo(): Stuff | Date {
return {
b: async (_) => "hello",
}
}`
});