Skip to content

Avoid caching resolved signatures for language service requests like signature help #52679

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 3 commits into from
Feb 14, 2023
Merged

Avoid caching resolved signatures for language service requests like signature help #52679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9923102
Fixed lack of errors on call expressions after LSP requests that read…
Andarist Feb 8, 2023
5313604
Cleanup blocking calls
Andarist Feb 10, 2023
7b79e1a
add an extra regression test case for string completions
Andarist Feb 10, 2023
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
29 changes: 18 additions & 11 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1636,9 +1636,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
getResolvedSignature: (node, candidatesOutArray, argumentCount) =>
getResolvedSignatureWorker(node, candidatesOutArray, argumentCount, CheckMode.Normal),
getResolvedSignatureForStringLiteralCompletions: (call, editingArgument, candidatesOutArray) =>
getResolvedSignatureWorker(call, candidatesOutArray, /*argumentCount*/ undefined, CheckMode.IsForStringLiteralArgumentCompletions, editingArgument),
runWithInferenceBlockedFromSourceNode(editingArgument, () => getResolvedSignatureWorker(call, candidatesOutArray, /*argumentCount*/ undefined, CheckMode.IsForStringLiteralArgumentCompletions)),
Copy link
Contributor Author

@Andarist Andarist Feb 8, 2023

Choose a reason for hiding this comment

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

perhaps this blocking wrapper should be moved to getResolvedSignatureWorker (to be called conditionally)? I didn't want to incur a closure penalty there though (maybe that's negligible?)

Copy link
Member

@sandersn sandersn Feb 10, 2023

Choose a reason for hiding this comment

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

Does it need to be conditional? Currently 2 out of 3 callers of getResolvedSignatureWorkers need to skip the cache, so I suspect that the 3rd caller also needs to. It's worth checking out and trying.

The wrapper-like nature of runWithInferenceBlockedFromSourceNode might also be better if inlined, although it's used in getContextualType as well. I would inline and duplicate the wrapper functions to see if it looks better (and perhaps, runs faster).

Copy link
Contributor Author

@Andarist Andarist Feb 10, 2023

Choose a reason for hiding this comment

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

Does it need to be conditional? Currently 2 out of 3 callers of getResolvedSignatureWorkers need to skip the cache, so I suspect that the 3rd caller also needs to. It's worth checking out and trying.

The 3rd call doesn't look LSP-related - more like a type-checking one. So I assumed that this one doesn't need this and might actually don't want this as for "real" type-checking purposes we want to cache things.

Admittedly, I confused myself and thought that getResolvedSignatureWorker is the getResolvedSignature (*Worker functions are often used within the "core" type checking algorithm) and that the "core" type checking calls into that. I considered this to be a hot path and thus wanted to avoid adding extra things there.

The wrapper-like nature of runWithInferenceBlockedFromSourceNode might also be better if inlined, although it's used in getContextualType as well. I would inline and duplicate the wrapper functions to see if it looks better (and perhaps, runs faster).

Since this blocking~ is only relevant for LSP requests - does perf matter that much here? I would consider this to be less of the hot path (maybe I'm wrong though) and wouldn't be bothered as much with inlining and comparing perf. Let me know what you think.

getResolvedSignatureForSignatureHelp: (node, candidatesOutArray, argumentCount) =>
getResolvedSignatureWorker(node, candidatesOutArray, argumentCount, CheckMode.IsForSignatureHelp),
runWithoutResolvedSignatureCaching(node, () => getResolvedSignatureWorker(node, candidatesOutArray, argumentCount, CheckMode.IsForSignatureHelp)),
getExpandedParameters,
hasEffectiveRestParameter,
containsArgumentsReference,
Expand Down Expand Up @@ -1813,36 +1813,43 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
isTypeParameterPossiblyReferenced,
};

function runWithInferenceBlockedFromSourceNode<T>(node: Node | undefined, fn: () => T): T {
function runWithoutResolvedSignatureCaching<T>(node: Node | undefined, fn: () => T): T {
const containingCall = findAncestor(node, isCallLikeExpression);
const containingCallResolvedSignature = containingCall && getNodeLinks(containingCall).resolvedSignature;
if (containingCall) {
getNodeLinks(containingCall).resolvedSignature = undefined;
}
const result = fn();
if (containingCall) {
getNodeLinks(containingCall).resolvedSignature = containingCallResolvedSignature;
}
return result;
}

function runWithInferenceBlockedFromSourceNode<T>(node: Node | undefined, fn: () => T): T {
const containingCall = findAncestor(node, isCallLikeExpression);
if (containingCall) {
let toMarkSkip = node!;
do {
getNodeLinks(toMarkSkip).skipDirectInference = true;
toMarkSkip = toMarkSkip.parent;
} while (toMarkSkip && toMarkSkip !== containingCall);
getNodeLinks(containingCall).resolvedSignature = undefined;
}
const result = fn();
const result = runWithoutResolvedSignatureCaching(node, fn);
if (containingCall) {
let toMarkSkip = node!;
do {
getNodeLinks(toMarkSkip).skipDirectInference = undefined;
toMarkSkip = toMarkSkip.parent;
} while (toMarkSkip && toMarkSkip !== containingCall);
getNodeLinks(containingCall).resolvedSignature = containingCallResolvedSignature;
}
return result;
}

function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode, editingArgument?: Node): Signature | undefined {
function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode): Signature | undefined {
const node = getParseTreeNode(nodeIn, isCallLikeExpression);
apparentArgumentCount = argumentCount;
const res =
!node ? undefined :
editingArgument ? runWithInferenceBlockedFromSourceNode(editingArgument, () => getResolvedSignature(node, candidatesOutArray, checkMode)) :
getResolvedSignature(node, candidatesOutArray, checkMode);
const res = !node ? undefined : getResolvedSignature(node, candidatesOutArray, checkMode);
apparentArgumentCount = undefined;
return res;
}
Expand Down
22 changes: 22 additions & 0 deletions tests/cases/fourslash/arityErrorAfterSignatureHelp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
///<reference path="fourslash.ts"/>
// @strict: true
////
//// declare function f(x: string, y: number): any;
////
//// /*1*/f(/*2*/)/*3*/

goTo.marker("2");
verify.signatureHelp({
triggerReason: {
kind: "invoked"
}
})
edit.insert(`"`)
edit.insert(`"`)
verify.signatureHelp({
triggerReason: {
kind: "retrigger"
}
})
verify.not.codeFixAvailable() // trigger typecheck
verify.errorExistsBetweenMarkers("1", "3");
14 changes: 14 additions & 0 deletions tests/cases/fourslash/arityErrorAfterStringCompletions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
///<reference path="fourslash.ts"/>
// @strict: true
////
//// interface Events {
//// click: any;
//// drag: any;
//// }
////
//// declare function addListener<K extends keyof Events>(type: K, listener: (ev: Events[K]) => any): void;
////
//// /*1*/addListener("/*2*/")/*3*/

verify.completions({ marker: ["2"], exact: ["click", "drag"] });
verify.errorExistsBetweenMarkers("1", "3");