-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Smaller refactors to server API in prep for notebook support #21095
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
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
| impl Borrow<SystemVirtualPath> for SystemVirtualPathBuf { | ||
| fn borrow(&self) -> &SystemVirtualPath { | ||
| self.as_path() | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so that we can use SystemVirtualPath as key when calling dict.get
7103813 to
6273b56
Compare
|
|
dcreager
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, and lets me learn more about the LSP internals. Though it probably deserves at least a cursory look from someone who's already more familiar with the LSP!
| params.notebook_document.version, | ||
| params.notebook_document.cells, | ||
| params.notebook_document.metadata.unwrap_or_default(), | ||
| params.notebook_document.uri, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can you pull out uri above too to be consistent with the other fields? Or is there an ownership issue?
I agree that this is great! |
Summary
While prototyping notebook support in ty's LSP, I realized that I'm having a hard time understanding
DocumentKey,DocumentQuery,Document(which is referred to as controller?). I then decided to take a closer look at those types and ended with a few refactors that I think are worthwhile, getting in independently of notebook support.AnySystemPath::key_from_urlnon-failable (and in turn,key_from_url) by using a virtual path for file URLs that don't represent a valid path.snapshot_documentreturn aResultthatErrs when the document wasn't found (instead of storing the document asResult). This was introduced in [ty] Make sure to always respond to client requests #19277 but I don't see how it is necessary to ensure we always send a response. The fix added in [ty] Make sure to always respond to client requests #19277, which sends a response if the URL isn't valid, can also be applied when the document wasn't found.DocumentQuery: ty uses salsa to access the file content (e.g.source_text) which then reads the latest document version using theLSPSystem. That eliminates the need to snapshot most document fields. The only exception to this is the notebook metadata which we now expose directly inDocumentSnapshot. I added newdocumentmethods that return a&Documentfor theLSPSystem.DocumentKeyto two variants roughly resemblingAnySystemPath. Its purpose is to uniquely identify a document withinIndex. We could probably useAnySystemPathfor that for now, but I wanted a distinct type to make it clear that a cell doesn't map to a ty-path (there's no corresponding file). Instead, cells should use the notebook file.DocumentHandletype, which is a read-only handle to a document inIndex. It takesDocumentKey's place in notification handlers because it now exposes theto_file_pathandurlmethods that were previously onDocumentKey. The motivation for the new type were:DocumentKeyto only be used to identify a document withinIndex, nothing moredocument.closeanddocument.update_text_documentto it (rather than having to deal with keys everywhere).Sessionoperations now take&Urlas an argument, hidingDocumentKey.My plan for notebooks is to store the cells as regular text documents. This is different from Ruff where the notebook cells are stored as part of the
Notebook. We'll see how well this goes but my thinking is that a model closer to how notebooks are handled within VS Code/the LSP specification, the less likely it is that we run into weird edge cases (e.g. where the notebook gets removed before the cells and we then can't find the cell anymore).Test plan
Tried different LSP operations