Fixed an issue with errors not being correctly reported after completion requests in functions within nested calls

[Andarist]

Context-sensitive functions might also cache nodeLinks.resolvedSignature (and symbolLinks.type) and lead to incorrect results for semantic diagnostics requests.

cc @andrewbranch

[typescript-bot]

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

[Andarist]

I am quite worried that this is not enough to fix all the cases. I feel like focusing on isCallLikeExpression and isFunctionLike 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).

[Andarist]

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 🤔

[Andarist]

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

[weswigham]

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 🤔

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:

1. Unset any existing, partial result for the signature in question (or just ignore the current result), since we're going to rerun the signature check with some differing flags, and, by extension, any signature whose result depends on it (thus, any syntactic parent, at least).
2. Ensure that any calculations we do that are affected by those differing flags (hard to track) aren't cached into the checker long-term. Basically we want to speculate (like we do in the parser), but we lack a real speculation helper inside the checker.

This is... probably better, even if it's not a complete fix, just because it invalidates more potentially bad parts of the cache.

[Andarist]

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

[weswigham]

I'll leave it to @andrewbranch to decide if "better" is good enough for a change like this, or if we'd rather wait and see someone make an attempt at a bigger, more complete solution to the checker API speculation issue.

[weswigham]

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 🤔

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:

1. Unset any existing, partial result for the signature in question (or just ignore the current result), since we're going to rerun the signature check with some differing flags, and, by extension, any signature whose result depends on it (thus, any syntactic parent, at least).
2. Ensure that any calculations we do that are affected by those differing flags (hard to track) aren't cached into the checker long-term. Basically we want to speculate (like we do in the parser), but we lack a real speculation helper inside the checker.

This is... probably better, even if it's not a complete fix, just because it invalidates more potentially bad parts of the cache.

[andrewbranch]

This seems fine to me, given the current state is known to be bad and this is a small change that can only improve it. It would be great to have a more reliable speculation mechanism in general, though.