Map LSP documents by normalised FilePath#1252
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
left a comment
DavisVaughan
left a comment
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
| 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> { |
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
| /// 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>, |
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
There was a problem hiding this comment.
oops sorry, my fault. I asked Claude to get rid of Option and didn't closely review that one. The placeholder is not only unsightly, it would leak to the user if we made a mistake somewhere.
I did consider hoisting url out of there but it conflicted with existing code IIRC, and I wanted to avoid making too many changes to legacy Ark handlers. Let me take another look.
There was a problem hiding this comment.
oh wait I forgot it's already gone in an ulterior PR!
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
lionel-
force-pushed
the
oak/salsa-15-file-path-refactor
branch
from
10deebe to
276eba8
Compare
lionel-
force-pushed
the
oak/salsa-15-file-path-refactor
branch
from
26ed4dd to
11deb77
Compare
lionel-
force-pushed
the
oak/salsa-16-document-wire-url
branch
from
7743983 to
a9b0bf2
Compare
2 participants
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.