Skip to content
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.

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

Granular handling for flycheck diagnostics #18332

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Strackeror
Copy link

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 :)

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

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

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
Copy link
Author

Strackeror commented Oct 25, 2024

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.

@Strackeror Strackeror force-pushed the granular-flycheck-diags branch 2 times, most recently from d0a5683 to c5ebca1 Compare October 29, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants