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.

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

Fixed an issue with an incorrect resolved signature being cached/returned sometimes for signatures depending on the contextual type/outer inference #52146

Merged

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Jan 8, 2023

fixes #44879

Reproduction scenario - hover over call to $ before hovering over the call to m: TS playground.

Interestingly, it stopped being reproducible like this in 4.7: TS playground. I think this was fixed by #36747 . However, following the exact same scenario using the test utils in the codebase still reproduce the issue - so it could resurface any time and it means that it somehow depends on what the TS server (or something) is doing now. The test utils do the bare minimum so it's easy to reproduce it there.

…rned sometimes for signatures depending on the contextual type/outer inference
@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jan 8, 2023
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

This approach seems fine to me; there's probably some other detritus in the caches when the inner resolution of the signature proceeds differently than the outer one in cases like this - cached expression types for context-sensitive expressions during the inner signature resolution may take priority, and thus affect other checking down the line (see how we get string in the inference for the call to $ but "foo" in the inference to m? Yeah. That.)... But I think this is fairy low-impact as-is, even if it can't fix all potential knock-on effects of a recursive signature resolution, it'll at least get the signature error state more consistently aligned.

So there's probably more to fix down this rabbithole, but I think this is a decent start and pretty low risk. I'll run DT and, should that come back fine, merge this.

@weswigham
Copy link
Member

@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 15, 2023

Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at bf1ba58. You can monitor the build here.

Update: The results are in!

Update: The results are in!

@typescript-bot
Copy link
Collaborator

Hey @weswigham, it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

@jakebailey
Copy link
Member

jakebailey commented Mar 15, 2023

I hit rerun on the above one too, but if that doesn't work we can just rerun it from scratch again.

@typescript-bot
Copy link
Collaborator

Hey @weswigham, it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

@jakebailey
Copy link
Member

@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 15, 2023

Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at bf1ba58. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

@weswigham
Copy link
Member

@typescript-bot run dt and dodge the npm race crash this time thanks

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 20, 2023

Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at bf1ba58. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

Hey @weswigham, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@weswigham weswigham merged commit 218180d into microsoft:main Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Quickinfo incorrectly computes zero-candidate type parameter with contextual inference candidate
4 participants