Map LSP documents by normalised FilePath#1252
Open
lionel- wants to merge 2 commits into
Open
Conversation
lionel-
force-pushed
the
oak/salsa-16-document-wire-url
branch
from
10e2102 to
f6332d0
Compare
lionel-
force-pushed
the
oak/salsa-15-file-path-refactor
branch
from
4ac9528 to
e807fd2
Compare
lionel-
force-pushed
the
oak/salsa-16-document-wire-url
branch
from
f6332d0 to
14bacca
Compare
DavisVaughan
requested changes
Jun 9, 2026
DavisVaughan
left a comment
DavisVaughan
left a comment
Contributor
There was a problem hiding this comment.
I really like the idea of this PR! But one blocking comment about Document that I'd like to resolve first
Comment on lines
84
to
93
| pub(crate) fn get_document(&self, uri: &Url) -> anyhow::Result<&Document> { | ||
| if let Some(doc) = self.documents.get(uri) { | ||
| let key = FilePath::from_url(uri); | ||
| if let Some(doc) = self.documents.get(&key) { | ||
| Ok(doc) | ||
| } else { | ||
| Err(anyhow!("Can't find document for URI {uri}")) | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn get_document_mut(&mut self, uri: &Url) -> anyhow::Result<&mut Document> { |
Contributor
There was a problem hiding this comment.
I feel like these should take path: &FilePath. It would encourage conversion to FilePath at the LSP boundary
Comment on lines
+19
to
+22
| /// Watched documents, keyed on the normalised [`FilePath`] form. | ||
| /// The verbatim editor URL is preserved on each [`Document::url`] | ||
| /// for wire output. | ||
| pub(crate) documents: HashMap<FilePath, Document>, |
Contributor
There was a problem hiding this comment.
I would like to advocate for the following strategy
pub(crate) documents: HashMap<FilePath, (Url, Document)>,Documentno longer knows abouturl. I think this is correct, as you should be able to create aDocumentwithout a correspondingurl.Documentno longer has that weirdPLACEHOLDER_URLhack, which i really do not likeinsert_document()would be removed since we only.insert()in 1 place in production code, and then in a few tests, so the abstraction doesn't feel worth it
WorldState::get_document() and WorldState::get_document_mut() would then:
- Take a
path: FilePathas I mentioned elsewhere - Abstract over the
(Url, Document)enum and just pull out theDocumentand return it
We would need to go update 2 direct usages of state.document.get() to be state.get_document(), but I think we should do that anyways.
This feels much much better to me, and I looked at all call sites of state.documents. and think it would work quite well
I am quite strongly against PLACEHOLDER_URL, so am going to block this pr until that is resolved
lionel-
force-pushed
the
oak/salsa-15-file-path-refactor
branch
from
e807fd2 to
10deebe
Compare
lionel-
force-pushed
the
oak/salsa-16-document-wire-url
branch
from
14bacca to
7743983
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Branched from #1251
Progress towards #1212
See #1250 for more context.
WorldStatenow keys documents by normalisedFilePathinstead ofUrl. Seestate.rs. Keying by normalised paths will prevent bugs when matching to paths that don't come from the frontend, e.g.source()paths in user code.The original
Urlsent by the LSP frontend is stored inDocument. This is used in communications with the frontend so that we send back exact byte-for-byte Urls, preventing any file matching bugs that could be caused by our internal normalisation of file paths.Unlike the DAP which has to deal with paths normalised by R, we never resolve symlinks on the LSP side. All matching is done on
FilePathbased on pure lexical normalisation of paths.