Skip to content
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

Jump to bottom

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

Merged
merged 3 commits into from
Feb 14, 2023
Merged

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

merged 3 commits into from
Feb 14, 2023

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Feb 8, 2023

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 8, 2023
Andarist
Andarist commented Feb 8, 2023
View reviewed changes
src/compiler/checker.ts
@@ -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),
runWithoutResolvedSignatureCaching(call, () => getResolvedSignatureWorker(call, candidatesOutArray, /*argumentCount*/ undefined, CheckMode.IsForStringLiteralArgumentCompletions, editingArgument)),
Copy link
Contributor Author

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

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

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.

@sandersn sandersn mentioned this pull request Feb 10, 2023
Closed
sandersn
sandersn requested changes Feb 10, 2023
View reviewed changes
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Nice fix for a tricky bug! Couple of suggestions.

src/compiler/checker.ts
@@ -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),
runWithoutResolvedSignatureCaching(call, () => getResolvedSignatureWorker(call, candidatesOutArray, /*argumentCount*/ undefined, CheckMode.IsForStringLiteralArgumentCompletions, editingArgument)),
Copy link
Member

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

src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
@Andarist Andarist mentioned this pull request Feb 10, 2023
Closed
@Andarist
Copy link
Contributor Author

@sandersn @DanielRosenwasser I pushed out some changes to the structure of the code. Overall, I don't think we need to juggle things more - but that's subjective. Yes, there is some duplication of the work done in runWithInferenceBlockedFromSourceNode and in runWithoutResolvedSignatureCaching but that's a fairly simple ancestry lookup and this is only relevant for LSP requests~. I think that it's not exactly a hot path and that we don't have to be worried about it. Let me know what do you think.

andrewbranch
andrewbranch approved these changes Feb 11, 2023
View reviewed changes
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Very excited to have this fixed 🌟

@DanielRosenwasser
Copy link
Member

@typescript-bot pack this
@typescript-bot user test tsserver
@typescript-bot test tsserver top100

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 13, 2023 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at 7b79e1a. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 13, 2023 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at 7b79e1a. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 13, 2023 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 7b79e1a. You can monitor the build here.

@DanielRosenwasser DanielRosenwasser changed the title Fixed lack of errors on call expressions after LSP requests that read their signatures Avoid caching resolved signatures for language service requests like signature help Feb 14, 2023
@sandersn sandersn merged commit ab40f5e into microsoft:main Feb 14, 2023
@Andarist Andarist deleted the fix/no-call-errors-after-signature-help branch February 14, 2023 18:07
@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/146368/artifacts?artifactName=tgz&fileId=B7E8D7117C23D25F0EE586E6B738A2C09AC0E1F6775B9ECFEACAE51482EB6B9302&fileName=/typescript-5.0.0-insiders.20230216.tgz"
    }
}

and then running npm install.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandersn sandersn sandersn approved these changes

@DanielRosenwasser DanielRosenwasser DanielRosenwasser approved these changes

@andrewbranch andrewbranch andrewbranch approved these changes

@weswigham weswigham Awaiting requested review from weswigham

Assignees

@sandersn sandersn

Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Signature help suppresses arity error in editor
5 participants
@Andarist @DanielRosenwasser @typescript-bot @sandersn @andrewbranch