[ty] Fix stale documents on Windows#18544
Conversation
- Update publish_diagnostics to accept DocumentKey instead of Url - Change LSPSystem::make_document_ref to take AnySystemPath directly - Add Session::text_document_keys method for DocumentKey iteration - Update notification handlers to use DocumentKey consistently - Remove URL conversion overhead in diagnostic workflows
MichaReiser
added
server
|
|
I still have to test if this fixes the bug on windows. I plan to do this later today. But I think this is good for review. |
MichaReiser
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
MichaReiser
requested review from
dhruvmanila
and removed request for
AlexWaygood,
carljm,
dcreager and
sharkdp
dhruvmanila
left a comment
dhruvmanila
left a comment
There was a problem hiding this comment.
Looks good, thank you for jumping on and fixing this quickly!
| let Ok(key) = session.key_from_url(uri.clone()) else { | ||
| tracing::debug!("Failed to create document key from URI: {}", uri); | ||
| return Ok(()); | ||
| }; |
There was a problem hiding this comment.
Unrelated to your PR, there are various instances of this kind of failures like this one and then there's key.to_url() where adding these debug logs consistently would be useful.
| #[derive(Clone, Debug)] | ||
| pub enum DocumentKey { | ||
| Notebook(Url), | ||
| NotebookCell(Url), | ||
| Text(Url), | ||
| pub(crate) enum DocumentKey { | ||
| Notebook(AnySystemPath), | ||
| NotebookCell { | ||
| cell_url: Url, | ||
| notebook_path: AnySystemPath, | ||
| }, | ||
| Text(AnySystemPath), | ||
| } |
There was a problem hiding this comment.
Should we keep the Url as well for both Notebook and Text variants? That might help to avoid converting between AnySystemPath and Url. This would make the key.to_url calls to return Url instead of Option<Url> and avoid the cost of converting a path to Url when we already had the Url before.
There was a problem hiding this comment.
Oh, but the Url is not available in the LSPSystem.
There was a problem hiding this comment.
Yes, and the client seems to be fine if the urls are slightly different. We could consider making AnySystemPath more strict so that toUrl could never fail but seems like a different refactor
I'm assuming that the video in the PR description is on Windows. |
3 participants
Summary
This is mostly a refactor which should fix the stale diagnostics on Windows (I've to test this when I'm back home and have access to my windows machine).
The main refactor is that
Indexnow stores a map fromAnySystemPathto the documents instead ofUrl. This removes the need to convert betweenUrl -> AnySystemPathandAnySystemPath -> Url, which can lead mismatches.Closes astral-sh/ty#601
Test Plan
Tested that completions and diagnostics work on macos.
Screen.Recording.2025-06-08.162410.mp4