Remove diagnostics when documents change#484
Conversation
vinistock
left a comment
vinistock
left a comment
There was a problem hiding this comment.
I believe this is a good approach 👍
| #[must_use] | ||
| pub fn diagnostics(&self) -> &Vec<Diagnostic> { | ||
| &self.diagnostics | ||
| pub fn diagnostics(&self) -> Vec<&Diagnostic> { |
There was a problem hiding this comment.
Maybe we can name this all_diagnostics to make it clear that this returns the combination of all of them?
There was a problem hiding this comment.
When we extend declarations (i.e.: find a second definition for the same thing), we need to ensure we're also extending the diagnostics.
There was a problem hiding this comment.
I added a fixup commit for this, but I couldn't find any tests for it, nor where it is called from. Maybe my Rust code navigation skills aren't up to scratch!
There was a problem hiding this comment.
We might not be doing it yet, but it should happen in Graph#add_declaration.
a7ef984 to
bfe69c7
Compare
bfe69c7 to
bd8c431
Compare
| use crate::{model::ids::UriId, offset::Offset}; | ||
|
|
||
| #[derive(Debug)] | ||
| #[derive(Debug, Clone)] |
There was a problem hiding this comment.
Why do we need to clone diagnostics?
There was a problem hiding this comment.
I pushed a fixup that takes them instead of cloning, but it's possible I misunderstood what you were suggesting here, in which case neither cloning nor taking might make sense.
There was a problem hiding this comment.
Maybe my comment was not correct. I wanted to make sure we were not accidentally discarding Declaration level diagnostics, but I think this scenario doesn't happen.
And that is probably why we're not using extend yet.
vinistock
left a comment
vinistock
left a comment
There was a problem hiding this comment.
I believe this will be flexible enough for all diagnostic types 👍
|
|
||
| #[must_use] | ||
| pub fn diagnostics(&self) -> &[Diagnostic] { | ||
| all_definitions!(self, it => &it.diagnostics) |
There was a problem hiding this comment.
This is a costly change when most definitions won't have diagnostics associated.
For declarations I understand the need, but do we need to associate diagnostics to a specific definition, when is it not enough to associate it to the document?
If we do need it, why not store them on the document as a hash?
document.diagnostics_for_definition(definition) -> Diagnostic[]There was a problem hiding this comment.
Yeah that's a good point. I opened a PR here to move them to the document, as you suggest (and references too).
As for why we need diagnostics linked to specific definitions, I thought there might be some resolution related diagnostics that couldn't be linked to a declaration (maybe in cases where it doesn't exist yet?). Would your circular dependency be one example? @vinistock we discussed this together, but I can't remember the definition-specific example.
There was a problem hiding this comment.
I think that should be fine. At the end of the day, all diagnostics are in some way related to a document because you have to display them somewhere.
3 participants
This PR is an alternative approach to #441.
We can add diagnostics to documents, declarations, definitions, and references. Different kinds of diagnostics would be attached to different entities. For example, parse errors would be attached to the document, and resolution errors (like circular dependencies) could be attached to declarations, definitions, or references.
When the document is cleared, each of its definitions and references is removed, along with the diagnostics attached to those entities. Any declaration that is impacted (i.e. because one if its definitions is removed) would have its diagnostics explicitly cleared.
We can access all diagnostics from the top of the graph by iterating through all the entities in the graph and collection the diagnostics. This would be slow, but I'm not sure what the use-case would be. Perhaps it can be improved if we understand when we would want to do this, outside of tests.
We can access all the diagnostics for a particular document via its definitions, references, and associated declarations.