Remove diagnostics when documents change by thomasmarshall · Pull Request #441 · Shopify/rubydex

thomasmarshall · 2026-01-12T16:24:15Z

This PR is part of #330. When a document changes or is removed, we want to clear/invalidate any parts of the graph that were created by or associated to that document. Eventually we will implement a more granular approach that clears/invalidates as little as possible, but for now we'll blow away everything to do with the document.

This ensures the diagnostics related to a document are removed when it is changed or deleted. It does so by moving the diagnostics from the top-level of the graph to be stored in the Document itself. @vinistock suggested this approach, as there is currently nothing else in the graph that connects to diagnostics and they are always (currently?) associated to a single document.

Morriar · 2026-01-12T20:50:54Z

+    pub fn diagnostics(&self) -> &[Diagnostic] {
+        self.document.diagnostics()
    }

    pub fn add_diagnostic(&mut self, diagnostics: Diagnostics, offset: Offset, message: String) {
        let diagnostic = Diagnostic::make(diagnostics, self.uri_id, offset, message);
-        self.diagnostics.push(diagnostic);
+        self.document.add_diagnostic(diagnostic);


Maybe we can delete these methods to make it clearer at this stage the diagnostics are in the document?

Morriar · 2026-01-12T20:52:03Z

+    fn tracking_diagnostics() {
+        use crate::diagnostic::{Diagnostics, Severity};
+        use crate::model::ids::UriId;
+        use crate::offset::Offset;
+
+        let mut document = Document::new("file:///foo.rb".to_string());
+        let diagnostic = Diagnostic::make(
+            Diagnostics::ParseError,
+            UriId::from("file:///foo.rb"),
+            Offset::new(0, 10),
+            "test error".to_string(),
+        );
+
+        document.add_diagnostic(diagnostic);
+
+        assert_eq!(document.diagnostics().len(), 1);
+        assert_eq!(document.diagnostics()[0].message(), "test error");
+        assert!(matches!(document.diagnostics()[0].severity(), Severity::Error));
+    }


I'm not sold on the value of this test?

Yeah that's fair, I wasn't sure either. Removed now 👍

Morriar · 2026-01-12T20:55:46Z

    // Map of method references that still need to be resolved
    method_references: IdentityHashMap<ReferenceId, MethodRef>,
-
-    diagnostics: Vec<Diagnostic>,


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.

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.

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.

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?

☝️ 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.

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

@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.

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.

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

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

vinistock · 2026-01-14T13:42:50Z

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


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

vinistock · 2026-01-14T13:43:45Z

+        let (uri_id, mut document, definitions, strings, names, constant_references, method_references, diagnostics) =
            local_graph.into_parts();

+        for diagnostic in diagnostics {


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

thomasmarshall · 2026-01-21T11:10:13Z

Closing in favor of #484.

thomasmarshall force-pushed the remove-diagnostics-when-documents-change branch from 21520ac to a72e1e1 Compare January 12, 2026 16:26

thomasmarshall force-pushed the remove-references-when-document-changes branch from 0a2e923 to c47805f Compare January 12, 2026 20:03

thomasmarshall force-pushed the remove-diagnostics-when-documents-change branch from a72e1e1 to 2e708d4 Compare January 12, 2026 20:05

Base automatically changed from remove-references-when-document-changes to main January 12, 2026 20:13

thomasmarshall marked this pull request as ready for review January 12, 2026 20:19

thomasmarshall requested a review from a team as a code owner January 12, 2026 20:19

Morriar reviewed Jan 12, 2026

View reviewed changes

thomasmarshall force-pushed the remove-diagnostics-when-documents-change branch 3 times, most recently from e22af33 to c416e4b Compare January 13, 2026 13:09

Clear diagnostics when document changes

3230cb3

thomasmarshall force-pushed the remove-diagnostics-when-documents-change branch from c416e4b to 3230cb3 Compare January 13, 2026 13:52

egiurleo assigned thomasmarshall Jan 13, 2026

vinistock reviewed Jan 14, 2026

View reviewed changes

thomasmarshall mentioned this pull request Jan 19, 2026

Remove diagnostics when documents change #484

Merged

thomasmarshall closed this Jan 21, 2026

Uh oh!

Conversation

thomasmarshall commented Jan 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasmarshall Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinistock Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasmarshall commented Jan 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thomasmarshall Jan 13, 2026 •

edited

Loading

vinistock Jan 14, 2026 •

edited

Loading