Granular handling for flycheck diagnostics #18332

Strackeror · 2024-10-18T11:56:04Z

This is my proposed solution for #18283

This implements 2 things:

A generation counter for flycheck diagnostics, which allows for progressively replacing previous diagnostics file by file instead of erasing every previous diagnostic as soon as you get the first new one.
Handling for when the check command is done with a crate to clear all relevant files of outdated diagnostics.

This meant adding some facilities to find a crate by name and iterate over its files, which are the parts I'm the least confident about. This is my first PR for this project, so I'm ready for extensive feedback if need be :)

Veykril

I think we already have everything available to imrpove this without having to add generations and stuff,

In https://github.com/Veykril/rust-analyzer/blob/95298a2e61eb852b165faaaa2dcf1713001731ce/crates/rust-analyzer/src/flycheck.rs#L504 we drop the package_id of the message, we can keep that as our identifier for a crate (these diagnostics work on packages not crates so).

Here https://github.com/Veykril/rust-analyzer/blob/95298a2e61eb852b165faaaa2dcf1713001731ce/crates/rust-analyzer/src/flycheck.rs#L342-L349
we already know when we are done with checking a specific package, again msg.package contains an identiifer to identify the package with.

So we can change how flycheck works a bit, we can make it maybe collect all diagnostics first, then s end them out as one big message per package whenever we receive the corresponding CargoCheckMessage::CompilerArtifact message for a package.

Thats a very rough description of thinsg but I think with that and a bit of fleshing out the details we can have a nicer solution (I am not a big fan of the generation stuff)

Strackeror · 2024-10-25T12:10:49Z

In https://github.com/Veykril/rust-analyzer/blob/95298a2e61eb852b165faaaa2dcf1713001731ce/crates/rust-analyzer/src/flycheck.rs#L504 we drop the package_id of the message, we can keep that as our identifier for a crate (these diagnostics work on packages not crates so).

Hmm, I saw that the package id was accessible, but I'm not sure where/how to match it to an actual CrateId. I'll look into it, might have to actually store it from wherever we fetch the crate graph.

Thats a very rough description of thinsg but I think with that and a bit of fleshing out the details we can have a nicer solution (I am not a big fan of the generation stuff)

Batching diagnostics around crates was an idea I also experimented with, but I'm not confident it can fully work as is. Erasing diagnostics crate by crate without a generation counter means we can't do a 'full clear' of diagnostics at any point in the process, and so it means that if a file isn't correctly included in our representation of the crate graph (if it stops being included, or maybe some include! shenanigans), then it can't ever be cleared.
My idea was to use the CompilerArtifact as more of a hint, we can use it clear some files earlier, but all diagnostics from the previous flycheck will still be cleared at the end of the process.

Veykril · 2024-12-07T13:20:48Z

crates/base-db/src/input.rs

@@ -277,6 +277,7 @@ pub struct CrateData {
    pub root_file_id: FileId,
    pub edition: Edition,
    pub version: Option<String>,
+    pub package_id: Option<String>,


CrateData is agnostic to cargo packages so we shouldn't store this here (and we shouldn't have to either way)

Veykril · 2024-12-07T13:23:37Z

crates/rust-analyzer/src/main_loop.rs

@@ -1019,6 +1022,19 @@ impl GlobalState {
                    Some(format!("rust-analyzer/flycheck/{id}")),
                );
            }
+            FlycheckMessage::ClearCrateDiagnostics { id, generation, package_id } => {
+                let snap = self.snapshot();
+                let Ok(Some(crate_id)) = snap.analysis.crate_with_id(&package_id) else {


Here we should instead iterate the workspaces (and then the packages in those) to find the relevant crates

Veykril · 2024-12-07T13:25:03Z

crates/rust-analyzer/src/main_loop.rs

+                let Ok(files) = snap.analysis.files_for(crate_id) else {
+                    return;
+                };
+                for file in files {
+                    self.diagnostics.clear_file_previous_check(id, generation, file);
+                }


With our changes, I feel like it'd be better to change how we store native diagnostics, keep them in a map HashMap<PackageId, ...> instead of per file, then when we actually pull the diagnostics out to report them we can just iterate that and turn it into per file stuff accordingly.

Then we don't need to iterate the files of a crate which is a bit fishy (if a person removes a mod statement we lose the connection to a file and now we have stale diagnostics that wont get cleared there)

Hmmm, removing the correlation with FileId really doesn't play well with the way DiagnosticsCollection handles accessing the diagnostics. I'm honestly having quite a bit of trouble finding a way to do that cleanly, since we need more context than just the diagnostic to get back the exact file it applies to.

I did think of the dangling files though, it's one of the reasons there's a generation index, I did make it so that we clean up everything from the previous generation at the end of a flycheck, so dangling files will be included in that last pass.

I'll have a look

I've implemented what I meant in (sorry for stealing your work here now): #18729

Please give that a review, maybe I misunderstood your problem and there are issues with that approach that I don't see

Ha, I was definitely overthinking the initial comment ! I was very hesitant of nesting yet another level of hashmap into the diagnostics, but yeah that's just cleaner that way.

Ah that explains things. The nesting there is fine since we can assume not a lot of diagnostics to occur.

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2024

Veykril reviewed Oct 22, 2024

View reviewed changes

Strackeror force-pushed the granular-flycheck-diags branch 3 times, most recently from c5ebca1 to bf9b625 Compare November 4, 2024 12:28

Veykril reviewed Dec 7, 2024

View reviewed changes

Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2024

Strackeror force-pushed the granular-flycheck-diags branch 2 times, most recently from 504d4a7 to bf9b625 Compare December 13, 2024 13:38

Strackeror added 2 commits December 13, 2024 14:39

Granular handling for flycheck diagnostics

d2eb9c2

Clear diagnostics with package id instead of crate name

7203d51

Strackeror force-pushed the granular-flycheck-diags branch from bf9b625 to 7203d51 Compare December 13, 2024 13:40

This was referenced Dec 15, 2024

When an error is fixed but the remaining build takes a while, the error is shown in the editor for the entire build #18691

Closed

Clear flycheck diagnostics more granularly #18729

Merged

Veykril closed this in #18729 Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Granular handling for flycheck diagnostics #18332

Granular handling for flycheck diagnostics #18332

Strackeror commented Oct 18, 2024

Veykril left a comment

Strackeror commented Oct 25, 2024 •

edited

Loading

Veykril Dec 7, 2024

Veykril Dec 7, 2024

Veykril Dec 7, 2024

Strackeror Dec 18, 2024 •

edited

Loading

Veykril Dec 20, 2024

Veykril Dec 20, 2024

Strackeror Dec 20, 2024

Veykril Dec 20, 2024

Granular handling for flycheck diagnostics #18332

Granular handling for flycheck diagnostics #18332

Conversation

Strackeror commented Oct 18, 2024

Veykril left a comment

Choose a reason for hiding this comment

Strackeror commented Oct 25, 2024 • edited Loading

Veykril Dec 7, 2024

Choose a reason for hiding this comment

Veykril Dec 7, 2024

Choose a reason for hiding this comment

Veykril Dec 7, 2024

Choose a reason for hiding this comment

Strackeror Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Veykril Dec 20, 2024

Choose a reason for hiding this comment

Veykril Dec 20, 2024

Choose a reason for hiding this comment

Strackeror Dec 20, 2024

Choose a reason for hiding this comment

Veykril Dec 20, 2024

Choose a reason for hiding this comment

Strackeror commented Oct 25, 2024 •

edited

Loading

Strackeror Dec 18, 2024 •

edited

Loading