Skip to content

Remove diagnostics when documents change #441

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.

Sign up for GitHub

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

Closed
thomasmarshall wants to merge 1 commit into from
Closed

Remove diagnostics when documents change #441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion rust/rubydex/src/diagnostic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{model::ids::UriId, offset::Offset};
use crate::{model::ids::{DiagnosticId, UriId}, offset::Offset};

#[derive(Debug)]
pub struct Diagnostic {
Expand Down Expand Up @@ -26,6 +26,11 @@ impl Diagnostic {
Self::new(diagnostics.code(), uri_id, offset, message, diagnostics.severity())
}

#[must_use]
pub fn id(&self) -> DiagnosticId {
DiagnosticId::from(&format!("{}{}{}{}", *self.uri_id, self.offset.start(), self.code, self.message))
}

#[must_use]
pub fn code(&self) -> u16 {
self.code
Expand Down
13 changes: 12 additions & 1 deletion rust/rubydex/src/model/document.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::model::ids::{DefinitionId, ReferenceId};
use crate::model::ids::{DefinitionId, DiagnosticId, ReferenceId};

// Represents a document currently loaded into memory. Identified by its unique URI, it holds the edges to all
// definitions and references discovered in it
Expand All @@ -8,6 +8,7 @@ pub struct Document {
definition_ids: Vec<DefinitionId>,
method_reference_ids: Vec<ReferenceId>,
constant_reference_ids: Vec<ReferenceId>,
diagnostic_ids: Vec<DiagnosticId>,
}

impl Document {
Expand All @@ -18,6 +19,7 @@ impl Document {
definition_ids: Vec::new(),
method_reference_ids: Vec::new(),
constant_reference_ids: Vec::new(),
diagnostic_ids: Vec::new(),
}
}

Expand Down Expand Up @@ -57,6 +59,15 @@ impl Document {
pub fn add_constant_reference(&mut self, reference_id: ReferenceId) {
self.constant_reference_ids.push(reference_id);
}

#[must_use]
pub fn diagnostic_ids(&self) -> &[DiagnosticId] {
&self.diagnostic_ids
}

pub fn add_diagnostic_id(&mut self, diagnostic_id: DiagnosticId) {
self.diagnostic_ids.push(diagnostic_id);
}
}

#[cfg(test)]
Expand Down
38 changes: 30 additions & 8 deletions rust/rubydex/src/model/graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::model::declaration::Declaration;
use crate::model::definitions::Definition;
use crate::model::document::Document;
use crate::model::identity_maps::IdentityHashMap;
use crate::model::ids::{DeclarationId, DefinitionId, NameId, ReferenceId, StringId, UriId};
use crate::model::ids::{DeclarationId, DefinitionId, DiagnosticId, NameId, ReferenceId, StringId, UriId};
use crate::model::name::{NameRef, ResolvedName};
// use crate::model::integrity::IntegrityChecker;
use crate::model::references::{ConstantReference, MethodRef};
Expand Down Expand Up @@ -38,8 +38,7 @@ pub struct Graph {
constant_references: IdentityHashMap<ReferenceId, ConstantReference>,
// Map of method references that still need to be resolved
method_references: IdentityHashMap<ReferenceId, MethodRef>,

diagnostics: Vec<Diagnostic>,

@Morriar Morriar Jan 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be a problem.

Indexing diagnostics are indeed related to a specific document but resolution may not. They are more "global".

Take this example:

# file1.rb
class Foo < Bar; end
# file2.rb
class Bar < Foo; end

We have a circular dependency diagnostic and both documents participate to the error.

Let's say we put the error in file1.rb, then you update file2.rb to change the code to

# file2.rb
class Bar; end

You'll have to figure out you need to discard the diagnostic in file1.rb.

@vinistock vinistock Jan 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. Maybe we need diagnostics to be a top level node in the graph so that multiple other nodes can have edges to it?

For example, maybe indexing diagnostics get associated with documents and resolution diagnostics with declarations.

@thomasmarshall thomasmarshall Jan 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both. I pushed an alternative approach that keeps diagnostics at the top level, but indexed by DiagnosticId for lookup purposes.

I used URI/offset/code/message to generate the ID. However, this is not guaranteed to be unique by Prism which we use for parse error diagnostics. In some cases Prism has previously attached identical looking errors at the same location (even though they stem from different issues in the source code). Perhaps that would be considered a Prism bug and we don't care about attaching multiple instances of seemingly identical errors?

maybe indexing diagnostics get associated with documents and resolution diagnostics with declarations

Do you think they would be linked to the document as well, or just the declaration? Will there always be a suitable declaration for resolution issues?

Another option would be to have a DiagnosticGroupId which links together diagnostics for problems that span multiple documents. We could have an IdentityHashMap<DiagnosticGroupID, Vec[DiagnosticId]> index in the graph and each Diagnostic could store an optional group_id. In the cyclic references example there would be a separate Diagnostic in both file1.rb and file2.rb both with the same group_id.

@Morriar Morriar Jan 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we attached the resolution diagnostics to the declarations instead of the graph?

declaration.add_diagnostic(...)

That way when we remove a definition we also reset the declaration diagnostics before running the resolution again?

@vinistock vinistock Jan 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ that's what I was thinking too

For example, maybe indexing diagnostics get associated with documents and resolution diagnostics with declarations.

I believe if we make diagnostics a hashmap and allow documents and declarations to associate with them, we can account for indexing and resolution cases.

We can probably make this PR just about indexing ones though and then follow up with the resolution part.

@Morriar Morriar Jan 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parsing and indexing errors will be attached to Document

Yes.

Resolution errors will be attached to Declaration

For this one I don't know if it will be enough.

Let's consider this code:

# file1.rb
puts Foo # error: constant `Foo` doesn't exists

This is a resolution error (we need to have tried to resolve Foo to know it doesn't exist). But we don't really have a Declaration to attach the error? Or maybe it would be in Object?

Then if we later add the constant, the diagnostic should disappear:

# file2.rb

Foo = 42

@thomasmarshall thomasmarshall Jan 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vinistock and I just discussed that maybe we could attach diagnostics to any of documents, declarations, definitions, and constant references, depending on the type of issue. I am also going to take a look at how ty and Pyre to understand how they do it too.

@thomasmarshall thomasmarshall Jan 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Pyre attaches diagnostics to modules (files) and it has dependency tracking to determine which modules are impacted by changes in other modules.
  • ty attaches diagnostics to different scopes (function bodies, class bodies, modules). I don't quite understand the architecture but it uses Salsa for incremental computation so diagnostics are invalidated too by that mechanism.

@vinistock vinistock Jan 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it would make sense to have diagnostics attached to all possible nodes:

  • Documents: parse errors
  • Definitions: indexing errors like dynamic superclass
  • Declarations: resolution errors like superclass mismatch
  • Constant references: indexing errors like dynamic mixin and resolution errors like trying to include a class

@thomasmarshall thomasmarshall Jan 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just opened a draft PR here with that alternative approach.

diagnostics: IdentityHashMap<DiagnosticId, Diagnostic>,
}

impl Graph {
Expand All @@ -54,7 +53,7 @@ impl Graph {
names: IdentityHashMap::default(),
constant_references: IdentityHashMap::default(),
method_references: IdentityHashMap::default(),
diagnostics: Vec::new(),
diagnostics: IdentityHashMap::default(),
}
}

Expand Down Expand Up @@ -162,8 +161,8 @@ impl Graph {
}

#[must_use]
pub fn diagnostics(&self) -> &Vec<Diagnostic> {
&self.diagnostics
pub fn diagnostics(&self) -> Vec<&Diagnostic> {

@vinistock vinistock Jan 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably change this one to return the IdentityHashMap, so that it matches the readers we have for the other hashmaps too.

self.diagnostics.values().collect()
}

/// # Panics
Expand Down Expand Up @@ -266,16 +265,20 @@ impl Graph {
/// Merges everything in `other` into this Graph. This method is meant to merge all graph representations from
/// different threads, but not meant to handle updates to the existing global representation
pub fn extend(&mut self, local_graph: LocalGraph) {
let (uri_id, document, definitions, strings, names, constant_references, method_references, diagnostics) =
let (uri_id, mut document, definitions, strings, names, constant_references, method_references, diagnostics) =
local_graph.into_parts();

for diagnostic in diagnostics {

@vinistock vinistock Jan 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should register the diagnostics inside of the documents during indexing using the LocalGraph. That way, this method doesn't change.

document.add_diagnostic_id(diagnostic.id());
self.diagnostics.insert(diagnostic.id(), diagnostic);
}

self.documents.insert(uri_id, document);
self.definitions.extend(definitions);
self.strings.extend(strings);
self.names.extend(names);
self.constant_references.extend(constant_references);
self.method_references.extend(method_references);
self.diagnostics.extend(diagnostics);
}

/// Updates the global representation with the information contained in `other`, handling deletions, insertions and
Expand All @@ -296,6 +299,10 @@ impl Graph {
return;
};

for diagnostic_id in document.diagnostic_ids() {
self.diagnostics.remove(diagnostic_id);
}

let mut write_lock = self.declarations.write().unwrap();

// TODO: Remove method references from method declarations once method inference is implemented
Expand Down Expand Up @@ -616,6 +623,21 @@ mod tests {
context.graph().assert_integrity();
}

#[test]
fn updating_index_with_deleted_diagnostics() {
let mut context = GraphTest::new();

context.index_uri("file:///foo.rb", "class Foo");
assert!(!context.graph().diagnostics().is_empty());
let document = context.graph().documents().get(&UriId::from("file:///foo.rb")).unwrap();
assert!(!document.diagnostic_ids().is_empty());

context.index_uri("file:///foo.rb", "class Foo; end");
assert!(context.graph().diagnostics().is_empty());
let document = context.graph().documents().get(&UriId::from("file:///foo.rb")).unwrap();
assert!(document.diagnostic_ids().is_empty());
}

#[test]
fn updating_index_with_new_definitions() {
let mut context = GraphTest::new();
Expand Down
5 changes: 5 additions & 0 deletions rust/rubydex/src/model/ids.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,8 @@ pub type ReferenceId = Id<ReferenceMarker>;
pub struct NameMarker;
/// `NameId` represents an ID for any constant name that we find as part of a reference or definition
pub type NameId = Id<NameMarker>;

#[derive(PartialEq, Eq, Debug, Clone, Copy)]
pub struct DiagnosticMarker;
/// `DiagnosticId` represents the ID of a diagnostic found in a specific file
pub type DiagnosticId = Id<DiagnosticMarker>;
Loading