rust-lang / rust-analyzer

A Rust compiler front-end for IDEs
https://rust-analyzer.github.io/
Apache License 2.0
14.3k stars 1.61k forks source link

VSCode - Suppress Error Toast #14193

Closed AThilenius closed 12 months ago

AThilenius commented 1 year ago

Feature request: Add an option to suppress all Rust Analyzer errors from bubbling up to VSCode toasts.


Each and every single Rust Analyzer crash results in a toast being displayed in VSCode. Even if you chose to ignore the toast and leave it up, repeat toasts will play an attention-grabbing animation. I love Rust, I love Rust Analyzer, I'm immensely grateful to everyone in the world that made those two possible, but the toasts are like a splinter I just can't ignore. Maybe it's some type of OCD, but it's really painful πŸ™ I brought this up in issue 717 as well.

"We should fix the bugs instead"

I get a deluge of popups when I'm trying to git rebase. As I would fully expect, git merge conflicts aren't valid Rust and I don't expect (or want) Rust Analyzer to understand them. There are an infinite number of edge cases that fit this description. Process was killed, container was closed, SSH terminated, filesystem is FUBAR... it's impossible to fix every bug. Making the server hardened against crashes is one option (IMO probably not a great option, panics are a good way to report stack traces and handle truly unrecoverable situations 'cleanly'). I don't think fixing every bug is a good substitution for hiding these toasts.

"We need bug reports"

Microsoft has a guide on user telemetry for VSCode. A few hundred people opted into telemetry is orders of magnitude more useful than hoping someone opens an issue for each of the many error toasts that popup throughout the day, many of which aren't useful because the user knows exactly why the Rust isn't valid. This kind of stuff is largely plug-and-play too, things like Sentry.io can help you filter out non-errors (like trying to parse a git merge conflict).

Zeenobit commented 1 year ago

I want to bump this issue because it has been slowly driving me insane lately. Usually whenever I'm developing in Bevy a chunk of my screen space is dedicated to Rust analyzer error notifications that seem to have absolutely no functional effect. I feel like lately it has just been getting worse, but I have no data to back up this claim.

Please give me my screen space and sanity back. πŸ₯²

Veykril commented 1 year ago

While adding a button to the error toasts to surpress further ones for the session sounds okay, its still less than ideal that a rebase can cause panics. r-a should ideally never panic no matter what text input it gets fed.

@Zeenobit does this occur to you due to fiddling with the git repo as well, or is it just a thing with your bevy project?

Zeenobit commented 1 year ago

For me, it's usually just as I'm editing code. I suspect it may have something to do with lots of macro usage in Bevy, but that's just a guess. I'll try to collect some data and post any findings here.

Veykril commented 1 year ago

please report panics like that if you can, the point of this issue is not being able to ignore rust-analyzer from failing.

AThilenius commented 1 year ago

r-a should ideally never panic no matter what text input it gets fed

Not wanting a server (a persistent process) to ever panic is a good goal. I think a one-shot process panicking on garbage input is not just reasonable, it's a good design. It communicates error state clearly to a calling process. It happens in every Rust codebase I've ever worked on, often when Rust/Cargo files are in transient nonsensical states from editing.

BUT I really want to stress this again, that is NOT what this issue is about. It is about disruptive error spam in VSCode, which is against almost every single recommendation Microsoft has on VSCode notification handling.

Edit: I'll take a crack at a PR. Do you guys have any 'getting started with dev' instructions? I got the VSCode extension to build and launch but get a Bootstrap error [Error: Failed to execute rust-analyzer --version when I open a Rust file.

flodiebold commented 1 year ago

@AThilenius have you actually reported your crash? I know you're saying this issue isn't about that, but before you start claiming that it's impossible to address your annoyance by just fixing the crashes, you could at least try; otherwise it's just a self-fulfilling prophecy.

Zeenobit commented 1 year ago

@AThilenius have you actually reported your crash? I know you're saying this issue isn't about that, but before you start claiming that it's impossible to address your annoyance by just fixing the crashes, you could at least try; otherwise it's just a self-fulfilling prophecy.

I think everyone here fully understands that the correct reaction to these errors is fixing them. This isn't about fixing rust-analyzer errors. This is about using telemetry to report these errors directly to people who are better equipped to do something about it, instead of passing it to the user and hoping they do that manually or try and fix it themselves.

Most people would simply dismiss these errors since it's very difficult (at least for me) to shift one's focus from solving a complicated programming problem to debugging rust-analyzer. So more often than not, the error message just serves to annoy the user and not provide any actual value.

flodiebold commented 1 year ago

Even if we introduce telemetry, it's still arguable whether we should completely hide the crash from the user. And the question is whether we really need telemetry; I would expect programmers using an open-source software to be capable of reporting crashes when they encounter them.

So more often than not, the error message just serves to annoy the user and not provide any actual value.

That's in the hands of the user who is choosing not to report the bug though. And if the IDE is just not working without an error message, can also be very annoying to users.

Zeenobit commented 1 year ago

I seem to be getting these 2 error notifications when moving/renaming source files and saving Cargo.toml mid-edit:

rust-analyzer failed to load workspace: Failed to read Cargo metadata from Cargo.toml file...

Followed by:

cargo check failed

Any edits which invalidate the cargo seem to cause these notifications. Why should these be reported to user as error notifications? I know the cargo is invalid, I'm in the process of fixing it.

AThilenius commented 1 year ago

@Zeenobit said it well.

And yes, I have. Like contributing to this issue which was opened in 2019. But once again... that is NOT what this issue is about. I'm going to keep repeating that ad nauseam πŸ˜‹

you could at least try

I have a day job and very limited time to contribute to the open source community, but I contribute where I can (and am offering a PR for this with some guidance). Speaking of Bevy, I wrote the original transform hierarchy impl Bevy is based on for Legion, and I donate to the project financially. Responses like "don't be lazy and fix it yourself" aren't productive or useful, they will quickly derail this conversation. I'm not lazy, nor am I entitled about open-source. I am human and only have 24 hours in my day.

lnicola commented 1 year ago

I think crashes while switching git branches are an old problem and we don't gain much from getting hundreds of reports about it.

flodiebold commented 1 year ago

And yes, I have. Like contributing to https://github.com/rust-lang/rust-analyzer/issues/717 which was opened in 2019. But once again... that is NOT what this issue is about. I'm going to keep repeating that ad nauseam yum

The problem you mentioned in that issue was supposedly fixed 5 days later. Are you saying you're still encountering that crash, even with current rust-analyzer? If so, that would be important to know.

Responses like "don't be lazy and fix it yourself" aren't productive or useful, they will quickly derail this conversation. I'm not lazy, nor am I entitled about open-source. I am human and only have 24 hours in my day.

I'm sorry for not expressing myself clearly; I meant you could try reporting the issue, not fixing it. I don't expect anyone to spend time trying to fix issues they encounter, I completely understand not having the time for that; but I do expect them to at least report issues they complain about.

flodiebold commented 1 year ago

I seem to be getting these 2 error notifications when moving/renaming source files and saving Cargo.toml mid-edit:

rust-analyzer failed to load workspace: Failed to read Cargo metadata from Cargo.toml file...

Followed by:

cargo check failed

Any edits which invalidate the cargo seem to cause these notifications. Why should these be reported to user as error notifications? I know the cargo is invalid, I'm in the process of fixing it.

Completely agree, these should be displayed as some kind of persistent error state instead of a notification.

AThilenius commented 1 year ago

I'm offering a PR to disable repeat errors if someone will help me get a development environment setup/offer guidance, and if the PR will be accepted. I am not offering to be a human telemetry gathering tool. Add telemetry if you need telemetry.

Zeenobit commented 1 year ago

Completely agree, these should be displayed as some kind of persistent error state instead of a notification.

@flodiebold Thing is, we already have this feature: xWhzduI I can get full information just by hovering over that button in VSCode.

But in addition to that, I also get this: IDjSR4h Notice how it overlaps my project explorer, stealing screen space as the errors pile up.

I can reproduce this issue by renaming a lib.rs to anything_else.rs in a multi-crate workspace.

Zeenobit commented 1 year ago

I also get stuff like this: image

This is completely valid code. My project compiles and runs ok, but rust-analyzer doesn't seem to like it. Ok, I can live with that. But then I also get this when I try to save the file (which runs cargo check): image Again, I know cargo check is failing. Why am I being notified about it?

Veykril commented 1 year ago

Workspace loading failures shouldn't cause a pop given the status bar icon that is indeed a good point. cargo check failing I am less sure about as that is a deliberate action that shouldn't fail silently (and as it isn't stateful it has no place to be in the status bar either) so I think that having cause a pop up is okay, as it usually means a configuration problem (and at worst the user can disable checking on save).

Zeenobit commented 1 year ago

Workspace loading failures shouldn't cause a pop given the status bar icon that is indeed a good point. cargo check failing I am less sure about as that is a deliberate action that shouldn't fail silently (and as it isn't stateful it has no place to be in the status bar either) so I think that having cause a pop up is okay, as it usually means a configuration problem (and at worst the user can disable checking on save).

To me, if the cargo check is failing due to compiler error, there is no need for a notification. We can see code errors in the Problems tab just fine.

But if the cargo check is failing due to an internal rust-analyzer error (which I think it may be in my case), then that's a perfect candidate for an error that should really be a telemetry event, because the user can't do anything about it at the time unless they drop everything they're doing at that moment to debug or report it. The way it works now is analogous to letting an exception bubble all the way up to UI in a GUI app, which is often not a good user experience.

If we can't differentiate between the different types of cargo check failures, then maybe that's a different problem that must be solved first.

Veykril commented 1 year ago

To me, if the cargo check is failing due to compiler error, there is no need for a notification

That's not what this means, if you get that pop up the invocation of cargo check failed, not that there were compiler errors.

But if the cargo check is failing due to an internal rust-analyzer error (which I think it may be in my case), then that's a perfect candidate for an error that should really be a telemetry event, because the user can't do anything about it at the time unless they drop everything they're doing at that moment to debug or report it

99% of the cases where this cargo check pop up appears its a problem with the users configuration or set up. Telemetry won't help there since it's not in rust-analyzer's hands.

AThilenius commented 1 year ago

I'm going to nudge again... what is causing the exceptions isn't germane to this issue.

Could be a software bug in r-a that desperately needs fixing, could be ChatGPT becoming sentient, but it's most likely an issue with the nut that connects my office chair to my keyboard. Doesn't matter, all are out of the scope of this issue. The scope of this issue is: I don't want to see error toasts for Rust Analyzer.


Telemetry deserves it's own issue IMO, the value proposition of telemetry is immense.

99% of the cases where this cargo check pop up appears its a problem with the users configuration or set up. Telemetry won't help there since it's not in rust-analyzer's hands.

Only telemetry can tell you that, otherwise you're just guessing. Maybe it's 98% and a nasty bug is lurking in that 1% that causing user pain. Telemetry can tell you that once you filter out the noise which is a standard feature of all telemetry solutions.

griffi-gh commented 1 year ago

Yep, these are annoying and sometimes pop up when i save too fast (Which I do instinctively every time I type something)

chenyukang commented 1 year ago

This is annoying! Even when I use Do not disturb mode in VSCode, the notification still comes up. Status bar is enough for most experienced users to get the current status, if we need to dig into the details we can go to have an investigation on it.

But most of the time, we don't want to be disturbed in daily workflow.

kryptan commented 1 year ago

I get constant "cargo check failed" popups while editing Cargo.toml. What is even their purpose? Like I know that Cargo.toml is invalid while I'm editing it.

Veykril commented 1 year ago

Do you have autosave enabled?

kryptan commented 1 year ago

Do you have autosave enabled?

Yes

Veykril commented 1 year ago

Opened https://github.com/rust-lang/rust-analyzer/issues/14453 for that, we shouldn't run cargo check when workspace loading fails

hecatia-elegua commented 1 year ago

I don't want to see error toasts for Rust Analyzer.

isn't there a way to turn off error toasts in vscode? then you can add this as a language-specific setting until it's fixed here

OmarTawfik commented 1 year ago

@AThilenius Thanks for writing this up!

Adding one more use case:

I work on a monorepo of tens of crates that use build.rs to edit other crates before building as well, relying on Cargo incrementability. When developing/editing these build scripts, fly-check will often run them, and possibly produce intermediate/broken output, which fails loading other crates downstream.

This makes the notification boxes (and the distraction they cause) almost constant for us.

While adding a button to the error toasts to suppress further ones for the session sounds okay

cc @Veykril, if I may suggest, I want to think about how we can eliminate notification boxes all together, instead of trying to figure out how to show it less frequently. Notifications should be reserved for:

  1. One-time questions, like a consent form, or a pending update.
  2. In response to direct user interactions.

Workspace loading, build scripts, and fly-check issues should not be triggering notifications IMO, given how distracting they are. I wonder if we can use the vscode.languages.createLanguageStatusItem() API, replacing both notification boxes, and the current status bar item.

LanguageStatusItem A language status item is the preferred way to present language status reports for the active text editors, such as selected linter or notifying about a configuration problem. https://code.visualstudio.com/api/references/vscode-api#LanguageStatusItem

IIUC, this also has the following additional benefits:

  1. It can be resolved automatically, once the next fly check is triggered, and the issue is cleared.
  2. It has a document selector, so it is hidden when users navigate away from rust files, unlike the current status bar item.
  3. It can have multiple actions/buttons, instead of the single click event for the the current status bar item to stop/restart the server.
image
dzdrav commented 1 year ago

Is it really that difficult to comprehend users may not want an annoying, repeating notification popping up, interrupting the programming flow, and taking up screen real-estate by covering parts of the terminal?

Every discussion like this derails to "just report the underlying issue". The popup being non-hideable, even when after 4 years of issues, MS finally decided to give an option to hide all non-error level popups. RA extensions outsmarts this convenience by emitting an error level popup.

The fact is: this is horrible UX. How could a non-configurable error toast that nags the user over and over again, and doesn't give them an option to have peace, even in Do Not Disturb mode, because the guiding design principle was from a moral high ground of "users have a right to know", be considered good design?

Please, don't speak for us (users). We want control over our tools, not for the tool to hit us in the face during regular workflow. If a user already doesn't know his workspace doesn't compile or the build.rs script failed, and wants a toast popup about it, that should at least be configurable.

To clarify why this isn't fixable by "just reporting the crash": it's normal part of the workflow for workspace to intermittently be non-buildable, or to open a project that we know has errors. As long as the discussion keeps derailing towards "report why it crashes", it's off-topic and doesn't bring us anywhere closer to the solution.

Is it really that much to ask to have an option (not even to disable this completely) to not get bombarded in the face with notifications we don't want?

flodiebold commented 1 year ago

To clarify why this isn't fixable by "just reporting the crash": it's normal part of the workflow for workspace to intermittently be non-buildable, or to open a project that we know has errors. As long as the discussion keeps derailing towards "report why it crashes", it's off-topic and doesn't bring us anywhere closer to the solution.

We agree that normal cargo check errors shouldn't cause an error notification. That argument is what's actually off-topic, because if you read the original issue it's about whether crashes should cause an error notification.

hecatia-elegua commented 1 year ago

@dzdrav I think the problem isn't that we're all idiots who don't want to improve UX, but that there's only a small amount of contributors. I like @OmarTawfik's idea, but e.g. I don't know how to start implementing this. On the same vein, maybe someone can guide you to implement this.

dzdrav commented 1 year ago

We agree that normal cargo check errors shouldn't cause an error notification. That argument is what's actually off-topic, because if you read the original issue it's about whether crashes should cause an error notification.

Quoting the original comment:

Feature request: Add an option to suppress all Rust Analyzer errors from bubbling up to VSCode toasts.

If there is an option to disable popups, then what happens when they're enabled would be another discussion. The main point of Do Not Disturb mode is to avoid disturbances such as those.

@dzdrav I think the problem isn't that we're all idiots who don't want to improve UX, but that there's only a small amount of contributors. I like @OmarTawfik's idea, but e.g. I don't know how to start implementing this. On the same vein, maybe someone can guide you to implement this.

That was not my aim to imply, but the main impression from the discussion was the adament stance to "decide which popups to show" or "fix the reason" some of them are showing, which just goes around the issue of having a toggle for them.

I would be happy to take a look if you could guide me to the relevant code (and if we agree that it could be configurable as an option).

AThilenius commented 1 year ago

I'll try my best to nudge this back on rails again. Seems this issue is a real hot potato 🫀

@dzdrav We all love Rust Analyzer and are grateful for the work done on it, this issue is self-evident that fact; we put up with something that really bothers us because the tool is so important and wonderful. I know this issue is frustrating, but your first post reads as entitled and derogatory, intended or not.

But tone aside... @dzdrav does also hit the nail on the head.

If there is an option to disable popups, then what happens when they're enabled would be another discussion.

R-A devs... that's the underlying theme and frustration with this issue and all the others like it. It's the same message I've repeated over, and over, and over again. I'm asking that an option to be added to disable all error toasts, nothing else. Everything else (telemetry, bug-fixes, filtering, et. al) is out of scope and shouldn't hold up this issue. They are all important, but they are all out of scope.


$50 Bounty

I'm charging tactic... who ever gets this issue fixed and deployed, gets a Venmo/Zelle transfer from me, for $50 USD. If anyone else wants to chip in a bounty I'm sure that will speed up the process.

Veykril commented 1 year ago

Alright, I did a second pass through things, with #14901 there are only 2 cases where the server itself sends a error/warning pop up request:

But there is actually a third error pop up that can occur, that is not from a request of the server to the client, but instead when the server responds to a client request with an error (as is the case for panics). VSCode screams at the user in this case that a request has failed, I'll look into whether thats configurable/hookable on the client code but if not I would consider that a VSCode "feature" or whatever since that handling is out of our hands (and I refuse to add a flag to make the server not report errors to the client, raise that problem upstream on the VSCode repo then). Assuming Do not disturb mode does not make VSCode ignore LSP request errors, I'd consider that part a VSCode bug.

AThilenius commented 1 year ago

@Veykril Can you TL;DR that for us?

I'm reading it as: 'Language server errors are handled by panicking, vscode displays those as error toasts, and the panic behavior is being marked "won't fix" by the RA team?' It's sounding like we have the answer to this issue then, and it can be closed as "won't fix", or is there a reason to leave it open?

Microsoft has made very clear they won't disable error notifications: https://github.com/microsoft/vscode/issues/41767 This is one of the more Kafkaesk situations I've found myself in with software; both you and Microsoft have valid arguments in isolation, but combined the situation is truly maddening.

juliusl commented 1 year ago

@AThilenius

kafkaesk situation

I have decided the only real workaround is either sticky notes, or this:

image

(Double right-click on a relaxing video in Youtube and click on picture-in-picture)

jprochazk commented 1 year ago

But there is actually a third error pop up that can occur, that is not from a request of the server to the client, but instead when the server responds to a client request with an error (as is the case for panics). VSCode screams at the user in this case that a request has failed, ~I'll look into whether thats configurable/hookable on the client code but if not I would consider that a VSCode "feature" or whatever since~ that handling is out of our hands (and I refuse to add a flag to make the server not report errors to the client, raise that problem upstream on the VSCode repo then). Assuming Do not disturb mode does not make VSCode ignore LSP request errors, I'd consider that part a VSCode bug.

It turns out this is something that can be hooked into. Here is the diff for an MVP which suppresses the error notifications completely. I confirmed this works by causing a panic and watching it appear in the output window, but without sending a notification. The solution is very barebones, but I'd be happy to work on this further and submit a PR which makes this user-configurable, if it's something that would be likely to get accepted.

Side note: It should be very easy to hook up telemetry on the language client side so that the r-a devs can be notified of every panic, not just the ones reported by users. I would also be more than happy to look into that.

AThilenius commented 1 year ago

@jprochazk If you can get a PR merged, my $50 bounty still stands 😁 8 months and 36 comments later, R-A error spam in vscode still drives me insane.

chenyukang commented 1 year ago

My solution for this issue is appending this CSS to /Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/workbench.desktop.main.css

.monaco-workbench>.notifications-toasts {
    position: absolute;
    z-index: 1000;
    right: 30px;
    display: none;
    overflow: hidden;
    bottom: -400px;
}

The drawback is I need to update it whenever VsCode is upgraded πŸ€”

jprochazk commented 12 months ago

@AThilenius please use the money to sponsor r-a development instead πŸ‘

AThilenius commented 11 months ago

@jprochazk Done Thank you so much, you've made at least one developer incredibly happy ❀️