fix(lsp): properly display x-deno-warning with redirects #13554
Conversation
cli/lsp/documents.rs
Outdated
| @@ -664,6 +671,7 @@ impl SpecifierResolver { | |||
| #[derive(Debug, Default)] | |||
| struct FileSystemDocuments { | |||
| docs: HashMap<ModuleSpecifier, Document>, | |||
| metadata: HashMap<ModuleSpecifier, Arc<HashMap<MetadataKey, String>>>, | |||
There was a problem hiding this comment.
I think this specifier metadata and #12864 are not really document related, but rather have to do with specifiers. Also, FileSystemDocuments contains only non-open documents.
Maybe we should move this out to a separate struct on its own and then I think we'd only need to use that struct in diagnostics.rs? (So we could remove most of the code in this file)
There was a problem hiding this comment.
I disagree... they are very much "FileSystemDocuments" as they are what is stored in the cache... while we don't load 100% of files in the cache as document nor do 100% of documents that are loaded get their meta data loaded, they both are representations of information in the cache and have a coupled shared state.
Documents open in the editor for editing can't have metadata, only FileSystemDocuments can (and only those in the cache).
There was a problem hiding this comment.
I think they should share the same underlying cache, but we should separate this out from FileSystemDocuments. The purpose of FileSystemDocuments is to prevent having multiple copies of the same Document loaded into memory.
I would also say this shouldn't be on the interface for Documents either. We're wanting to know what the metadata is for certain specifiers and not documents. For example, there could be two specifiers that point at the same document and only one of those specifiers would produce a certain diagnostic. Again, they would share the same underlying cache.
Maybe I'm misunderstanding the purpose of this though.
There was a problem hiding this comment.
I thought about it and refactored it... it is a bit cleaner, but it introduced another mutex, which I am not totally happy with, but it is "cleaner"
017a13e
to
27b50e5
Compare
| #[derive(Debug, Default, Clone)] | ||
| pub(crate) struct CacheMetadata { | ||
| cache: http_cache::HttpCache, | ||
| metadata: Arc<Mutex<HashMap<ModuleSpecifier, Metadata>>>, |
There was a problem hiding this comment.
This change highlights that the cache being disconnected from the data in the Arc<Mutex<_>> is a little sketch for the sake of the snapshots, but this state being in sync is currently enforced by the "lsp mutex". In the future, if we allow multiple threads in, this state being in sync should be further enforced by requiring mutability on set_location, so we're good.
We may consider some improvements to this in the future to ensure the two are forced to stay in sync when a snapshot occurs so the code is slightly more resilient to changes around it, but this could be a very minor future improvement.
Fixes: #13472
The best way to deal with this was to refactor out the metadata to a seperate structure. It makes extending this a lot easier in the future (specifically adding #12864 in a follow-up PR).
There is no new test, because I changed the test_util server to behave more like we see in practice, which broke the existing test, which then the PR fixes.