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.

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

Jump to bottom

Keep diagnostics at least until new ones are available #15934

Closed
dpc opened this issue Nov 19, 2023 · 5 comments · Fixed by #17248
Closed

Keep diagnostics at least until new ones are available #15934

dpc opened this issue Nov 19, 2023 · 5 comments · Fixed by #17248
Assignees
@matthri
Labels
A-diagnostics diagnostics / error reporting C-feature Category: feature request E-has-instructions Issue has some instructions and pointers to code to get started

Comments

@dpc
Copy link

dpc commented Nov 19, 2023 •
edited

This is something something I reported first in my text editor (helix) but I was told this is actually rust-analyzer's behavior, and thus can't be fixed there.

I'm working on a large Rust project. I make some refactoring changes that break existing callers. I hit :w and CPU's temperature rapidly increases. I wait and wait... I get over 9000 errors.

I fix some in one file and :w. LSP makes my laptop fan go jet engine mode again. I wait. I could be fixing some other errors already, because I know there's still plenty off them, but my text editor has already reset the previous list and <space>D shows an empty diagnostics picker. Bummer. I need to wait for the new diagnostics to arrive.

I wish previous LSP diagnostics were available for longer. At least until new LSP run completes or start returning something actionable again.

I was told this is because:

It's just that RA immidietly discards all checkOnSave diagnostics on save and therefore sends an empty list.

I'm not all that versed in LSP details, but I guess what I'm asking for is RA not sending that empty list, until it knows that project doesn't have any diagnostics.

This would be a biggest QA improvement in my workflow I can think of.
@dpc dpc added the C-feature Category: feature request label Nov 19, 2023
@Veykril Veykril added the A-diagnostics diagnostics / error reporting label Nov 19, 2023
@Veykril
Copy link
Member

Veykril commented Nov 19, 2023

We clear the check diagnostics here

rust-analyzer/crates/rust-analyzer/src/main_loop.rs

Lines 640 to 642 in 255eed4

flycheck::Progress::DidStart => {
self.diagnostics.clear_check(id);
(Progress::Begin, None)
and set them here

rust-analyzer/crates/rust-analyzer/src/main_loop.rs

Lines 489 to 492 in 255eed4

Task::Diagnostics(diagnostics_per_file) => {
for (file_id, diagnostics) in diagnostics_per_file {
self.diagnostics.set_native_diagnostics(file_id, diagnostics)
}

it might just suffice to move the clear away from the DidStart handling in front of the loop where we set the diagnostics now.

@Veykril Veykril added the E-has-instructions Issue has some instructions and pointers to code to get started label Nov 19, 2023
@matthri
Copy link

matthri commented Nov 25, 2023

@rustbot claim

@matthri
Copy link

matthri commented Nov 28, 2023

Hi,
first of all many thanks for the information you already provided :D

I was digging around in the code based on your instructions and a few questions have come up.
Removing the diagnostics clearing from here

rust-analyzer/crates/rust-analyzer/src/main_loop.rs

Lines 640 to 642 in 255eed4

flycheck::Progress::DidStart => {
self.diagnostics.clear_check(id);
(Progress::Begin, None)

I have understood, what I'm not sure is where to move the diagnostics clearing.

You suggested to add the clearing here

rust-analyzer/crates/rust-analyzer/src/main_loop.rs

Lines 489 to 492 in 255eed4

Task::Diagnostics(diagnostics_per_file) => {
for (file_id, diagnostics) in diagnostics_per_file {
self.diagnostics.set_native_diagnostics(file_id, diagnostics)
}

which as far as I can tell processes the native diagnostics whereas the previous location handles the flycheck diagnostics.
Therefore I wondered if the diagnostics clearing should instead get moved here

rust-analyzer/crates/rust-analyzer/src/main_loop.rs

Lines 620 to 623 in 255eed4

for diag in diagnostics {
match url_to_file_id(&self.vfs.read().0, &diag.url) {
Ok(file_id) => self.diagnostics.add_check_diagnostic(
id,

where the flycheck messages get processed and the diagnostics added.

My second question is if you have an idea how to automatically test this behavior or if there is an already existing test.

@Veykril
Copy link
Member

Veykril commented Nov 28, 2023

Right thats for native ones. The problem with clearing it where you proposed is that if multiple checks are running, we'll clear everything but the last one. I guess the easiest way would be to set a flag in GlobalState when running run_flycheck and in the place you linked, if the flag is set, unset it and clear the native diagnostics. That way if run_flycheck kicks off multiple it should still work out right.

@Veykril
Copy link
Member

Veykril commented Nov 28, 2023

My second question is if you have an idea how to automatically test this behavior or if there is an already existing test.

Not, I don't think we have any tests for this at all. I'd just say pick a big project (like r-a itself), edit some random crates with errors and check whether things work as expected.

@mladedav mladedav mentioned this issue May 16, 2024
Merged
@bors bors closed this as completed in 2dc22c6 May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@matthri matthri

Labels
A-diagnostics diagnostics / error reporting C-feature Category: feature request E-has-instructions Issue has some instructions and pointers to code to get started
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Clear diagnostics only after new ones were received mladedav/rust-analyzer
3 participants
@dpc @Veykril @matthri