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

editor/code: add option to suppress internal error notifications #15846

Merged
merged 4 commits into from Nov 24, 2023

Conversation

jprochazk
Copy link
Contributor

@jprochazk jprochazk commented Nov 7, 2023

Fixes #14193

  • Added the rust-analyzer.showRequestFailedErrorNotification configuration option, which defaults to true
  • If rust-analyzer.showRequestFailedErrorNotification is set to true, the current behavior is preserved.
  • If rust-analyzer.showRequestFailedErrorNotification is set to false, no error toasts will be displayed for any of the failed requests caused by panics in r-a. This only applies to events that are triggered "implicitly", such as textDocument/hover.

To test this, you can manually introduce a panic in one of the language server LSP handlers for non-command events. I added an explicit panic!() in the textDocument/hover event handler:

rust-analyzer.showRequestFailedErrorNotification set to true (default)

2023-11-07.17-17-48.webm

rust-analyzer.showRequestFailedErrorNotification set to false

2023-11-07.17-16-49.webm

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2023
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.

Guess I was very much tunnel visioning when I looked into how to overwrite this given how simple the solution is after all. Thanks for looking into this!

import * as vscode from "vscode";

export class RaLanguageClient extends lc.LanguageClient {
override error(message: string, data?: any, showNotification?: boolean | "force"): void {
Copy link
Member

Choose a reason for hiding this comment

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

Do you know what data might contain? Does it contain the error code by any chance? Would be nice if we could match on that instead, because then we can filter out some specific errors that VSCode raises (on non panicking requests) unconditionally (and log them instead).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like this is the ResponseError emitted by r-a, so it does contain the error code. I changed this to override handleRequestFailed instead of error, and set showNotification to false if we detect that it's an InternalError.

editors/code/package.json Outdated Show resolved Hide resolved
@Veykril
Copy link
Member

Veykril commented Nov 24, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 24, 2023

📌 Commit 9c8727e has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Nov 24, 2023

⌛ Testing commit 9c8727e with merge fec3828...

@Veykril Veykril changed the title editor/code: add option to suppress error notifications editor/code: add option to suppress internal error notifications Nov 24, 2023
@bors
Copy link
Collaborator

bors commented Nov 24, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing fec3828 to master...

@bors bors merged commit fec3828 into rust-lang:master Nov 24, 2023
10 checks passed
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.

VSCode - Suppress Error Toast
4 participants