-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Fixed an issue with errors not being correctly reported after completion requests in functions within nested calls #54944
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 1 commit into
microsoft:main
from
Andarist:fix/no-error-after-completions-in-nested-call-2
Sep 13, 2023
Merged
Changes from all commits
Commits
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
121 changes: 121 additions & 0 deletions
121
tests/baselines/reference/typeErrorAfterStringCompletionsInNestedCall2.baseline
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,121 @@ | ||
Syntactic Diagnostics for file '/tests/cases/fourslash/typeErrorAfterStringCompletionsInNestedCall2.ts': | ||
|
||
|
||
==== /tests/cases/fourslash/typeErrorAfterStringCompletionsInNestedCall2.ts (0 errors) ==== | ||
|
||
type ActionFunction< | ||
TExpressionEvent extends { type: string }, | ||
out TEvent extends { type: string } | ||
> = { | ||
({ event }: { event: TExpressionEvent }): void; | ||
_out_TEvent?: TEvent; | ||
}; | ||
|
||
interface MachineConfig<TEvent extends { type: string }> { | ||
types: { | ||
events: TEvent; | ||
}; | ||
on: { | ||
[K in TEvent["type"]]?: ActionFunction< | ||
Extract<TEvent, { type: K }>, | ||
TEvent | ||
>; | ||
}; | ||
} | ||
|
||
declare function raise< | ||
TExpressionEvent extends { type: string }, | ||
TEvent extends { type: string } | ||
>( | ||
resolve: ({ event }: { event: TExpressionEvent }) => TEvent | ||
): { | ||
({ event }: { event: TExpressionEvent }): void; | ||
_out_TEvent?: TEvent; | ||
}; | ||
|
||
declare function createMachine<TEvent extends { type: string }>( | ||
config: MachineConfig<TEvent> | ||
): void; | ||
|
||
createMachine({ | ||
types: { | ||
events: {} as { type: "FOO" } | { type: "BAR" }, | ||
}, | ||
on: { | ||
FOO: raise(({ event }) => { | ||
return { | ||
type: "BAR" as const, | ||
}; | ||
}), | ||
}, | ||
}); | ||
|
||
Semantic Diagnostics for file '/tests/cases/fourslash/typeErrorAfterStringCompletionsInNestedCall2.ts': | ||
/tests/cases/fourslash/typeErrorAfterStringCompletionsInNestedCall2.ts(41,5): error TS2322: Type '{ ({ event }: { event: { type: "FOO"; }; }): void; _out_TEvent?: { type: "BARx"; } | undefined; }' is not assignable to type 'ActionFunction<{ type: "FOO"; }, { type: "FOO"; } | { type: "BAR"; }>'. | ||
Types of property '_out_TEvent' are incompatible. | ||
Type '{ type: "BARx"; } | undefined' is not assignable to type '{ type: "FOO"; } | { type: "BAR"; } | undefined'. | ||
Type '{ type: "BARx"; }' is not assignable to type '{ type: "FOO"; } | { type: "BAR"; } | undefined'. | ||
Type '{ type: "BARx"; }' is not assignable to type '{ type: "FOO"; } | { type: "BAR"; }'. | ||
Type '{ type: "BARx"; }' is not assignable to type '{ type: "BAR"; }'. | ||
Types of property 'type' are incompatible. | ||
Type '"BARx"' is not assignable to type '"BAR"'. | ||
|
||
|
||
==== /tests/cases/fourslash/typeErrorAfterStringCompletionsInNestedCall2.ts (1 errors) ==== | ||
|
||
type ActionFunction< | ||
TExpressionEvent extends { type: string }, | ||
out TEvent extends { type: string } | ||
> = { | ||
({ event }: { event: TExpressionEvent }): void; | ||
_out_TEvent?: TEvent; | ||
}; | ||
|
||
interface MachineConfig<TEvent extends { type: string }> { | ||
types: { | ||
events: TEvent; | ||
}; | ||
on: { | ||
[K in TEvent["type"]]?: ActionFunction< | ||
Extract<TEvent, { type: K }>, | ||
TEvent | ||
>; | ||
}; | ||
} | ||
|
||
declare function raise< | ||
TExpressionEvent extends { type: string }, | ||
TEvent extends { type: string } | ||
>( | ||
resolve: ({ event }: { event: TExpressionEvent }) => TEvent | ||
): { | ||
({ event }: { event: TExpressionEvent }): void; | ||
_out_TEvent?: TEvent; | ||
}; | ||
|
||
declare function createMachine<TEvent extends { type: string }>( | ||
config: MachineConfig<TEvent> | ||
): void; | ||
|
||
createMachine({ | ||
types: { | ||
events: {} as { type: "FOO" } | { type: "BAR" }, | ||
}, | ||
on: { | ||
FOO: raise(({ event }) => { | ||
~~~ | ||
!!! error TS2322: Type '{ ({ event }: { event: { type: "FOO"; }; }): void; _out_TEvent?: { type: "BARx"; } | undefined; }' is not assignable to type 'ActionFunction<{ type: "FOO"; }, { type: "FOO"; } | { type: "BAR"; }>'. | ||
!!! error TS2322: Types of property '_out_TEvent' are incompatible. | ||
!!! error TS2322: Type '{ type: "BARx"; } | undefined' is not assignable to type '{ type: "FOO"; } | { type: "BAR"; } | undefined'. | ||
!!! error TS2322: Type '{ type: "BARx"; }' is not assignable to type '{ type: "FOO"; } | { type: "BAR"; } | undefined'. | ||
!!! error TS2322: Type '{ type: "BARx"; }' is not assignable to type '{ type: "FOO"; } | { type: "BAR"; }'. | ||
!!! error TS2322: Type '{ type: "BARx"; }' is not assignable to type '{ type: "BAR"; }'. | ||
!!! error TS2322: Types of property 'type' are incompatible. | ||
!!! error TS2322: Type '"BARx"' is not assignable to type '"BAR"'. | ||
!!! related TS6500 /tests/cases/fourslash/typeErrorAfterStringCompletionsInNestedCall2.ts:14:7: The expected type comes from property 'FOO' which is declared here on type '{ FOO?: ActionFunction<{ type: "FOO"; }, { type: "FOO"; } | { type: "BAR"; }> | undefined; BAR?: ActionFunction<{ type: "BAR"; }, { type: "FOO"; } | { type: "BAR"; }> | undefined; }' | ||
return { | ||
type: "BAR" as const, | ||
}; | ||
}), | ||
}, | ||
}); |
54 changes: 54 additions & 0 deletions
54
tests/cases/fourslash/typeErrorAfterStringCompletionsInNestedCall2.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,54 @@ | ||
///<reference path="fourslash.ts"/> | ||
// @strict: true | ||
//// | ||
//// type ActionFunction< | ||
//// TExpressionEvent extends { type: string }, | ||
//// out TEvent extends { type: string } | ||
//// > = { | ||
//// ({ event }: { event: TExpressionEvent }): void; | ||
//// _out_TEvent?: TEvent; | ||
//// }; | ||
//// | ||
//// interface MachineConfig<TEvent extends { type: string }> { | ||
//// types: { | ||
//// events: TEvent; | ||
//// }; | ||
//// on: { | ||
//// [K in TEvent["type"]]?: ActionFunction< | ||
//// Extract<TEvent, { type: K }>, | ||
//// TEvent | ||
//// >; | ||
//// }; | ||
//// } | ||
//// | ||
//// declare function raise< | ||
//// TExpressionEvent extends { type: string }, | ||
//// TEvent extends { type: string } | ||
//// >( | ||
//// resolve: ({ event }: { event: TExpressionEvent }) => TEvent | ||
//// ): { | ||
//// ({ event }: { event: TExpressionEvent }): void; | ||
//// _out_TEvent?: TEvent; | ||
//// }; | ||
//// | ||
//// declare function createMachine<TEvent extends { type: string }>( | ||
//// config: MachineConfig<TEvent> | ||
//// ): void; | ||
//// | ||
//// createMachine({ | ||
//// types: { | ||
//// events: {} as { type: "FOO" } | { type: "BAR" }, | ||
//// }, | ||
//// on: { | ||
//// [|/*error*/FOO|]: raise(({ event }) => { | ||
//// return { | ||
//// type: "BAR/*1*/" as const, | ||
//// }; | ||
//// }), | ||
//// }, | ||
//// }); | ||
|
||
goTo.marker("1"); | ||
edit.insert(`x`) | ||
verify.completions({ exact: ["FOO", "BAR"] }); | ||
verify.baselineSyntacticAndSemanticDiagnostics() |
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.
I am quite worried that this is not enough to fix all the cases. I feel like focusing on
isCallLikeExpression
andisFunctionLike
won't handle everything. For example, a reverse mapped type (and its properties) might get cached and a "cleared' function/call might potentially pull from a "corrupted" type.I really wonder if this just shouldn't clear every cached link on the way from the
node
to the root. It's still hard to determine what exact properties might have to be cleared from links but likely this would cover for more cases and with time the list of cleared (and brought back later) properties would get more complete.This isn't ideal but with the given architecture, I don't see a better way around this. You had 2 separate checkers in the past but even that was likely prone to similar problems - they were just less apparent (one could request completions, or something else, at 2 different locations within the same call - each of those requests should not depend on the information computed by the other one but it definitely did depend on it in subtle ways).
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.
maybe we should just temporarily unset all symbol/node links altogether here? it feels that they should be able to recreate themselves from scratch during the request 🤔
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.
After a second though... it's probably not that unlikely that a cached type within the same call~ can mess things up (even if it's like in a separate subtree of the call).
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.
That's more correct, what we have here is a compromise for performance in the common case. Cache invalidation is hard. We want to do 2 things in this function, ideally:
This is... probably better, even if it's not a complete fix, just because it invalidates more potentially bad parts of the cache.
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 had one alternative idea for a potentially more complete fix. Symbol links and node links are primary cache mechanisms related to this class of problems. So maybe we could reassign
getSymbolLinks
andgetNodeLinks
to versions that would not persist results in the "real" links. We could likely still consult the real links for nodes/symbols from different source files. However, we'd use a different "cache" for all nodes/symbols from the current source file. This should avoid broken results altogether as broken results are related to cached information about the nodes close to the one that is the target~ location of the current LSP request.