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

📎 Improve internal error reporting #2730

Open
Conaclos opened this issue May 5, 2024 · 5 comments
Open

📎 Improve internal error reporting #2730

Conaclos opened this issue May 5, 2024 · 5 comments
Labels
A-Project Area: project S-Enhancement Status: Improve an existing feature

Comments

@Conaclos
Copy link
Member

Conaclos commented May 5, 2024

Description

#2659 brought something to light for me: internal error reporting is uninformative for users and doesn't help them to understand the source of the issue or how to find a workaround.

Biome is a toolchain, a bug can come from any tool, and it is hard to tell without investment of the user.

It would be nice if we could specify which tool caused the error, and even in what unit of work (e.g. a rule for the linter), and the linted/formatted node - if any - (the queried node in the case of the linter).

Taking #2659 as an example.
Here is the current error:

Biome encountered an unexpected error

This is a bug in Biome, not an error in your code, and we would
appreciate it if you could report it to
https://github.com/biomejs/biome/issues/ along with the
following information to help us fixing the issue:

Source Location: crates/biome_js_semantic/src/semantic_model/scope.rs:115:33
Thread Name: biome::worker_0
Message: no entry found for key

types.ts internalError/panic  INTERNAL  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

✖ processing panicked: no entry found for key
  
⚠ This diagnostic was derived from an internal Biome error.
  Potential bug, please report it if necessary.

And here is an example of a better diagnostic:

types.ts internalError/panic  INTERNAL  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

✖ Biome encountered an internal error while linting the file with the
 rule suspicious/noRedeclare on the following code

    1 │
  > 2 │ type Element<T> = T extends Array<infer U> ? U : never
    3 │ 

ℹ This diagnostic was derived from the following internal Biome error:
 Source Location: crates/biome_js_semantic/src/semantic_model/scope.rs:115:33
 Thread Name: biome::worker_0
 Message: no entry found for key

ℹ This is a bug in Biome, not an error in your code, and we would
 appreciate it if you could report it to
 https://github.com/biomejs/biome/issues/

ℹ As a workaround try to disable the lint rule suspicious/noRedeclare
 or ignoring the file `types.ts`.

@Conaclos Conaclos changed the title 📎 Improve error reporting 📎 Improve internal error reporting May 5, 2024
@ematipico
Copy link
Member

Have you checked if it's technically possible to retrieve the information you'd like to see?

Those are panics generated by Rust, and we catch these panics with some simple hooks, which means that we might not have all the desired information you wished to see.

The only information we might be able to have, is the stack trace, however if it's not printed here, I doubt we have it.

@Conaclos
Copy link
Member Author

Conaclos commented May 6, 2024

Have you checked if it's technically possible to retrieve the information you'd like to see?

No, I haven't checked how to proceed, it's an invitation to investigate. Perhaps I should clarify the issue description.
Because we have a multi-threaded architecture, panics are isolated in threads? I wonder if we could get some data from the main thread.

The only information we might be able to have, is the stack trace, however if it's not printed here, I doubt we have it.

I was thinking of doing this to get some information such as the currently running linter rule.

@Conaclos Conaclos added A-Project Area: project S-Enhancement Status: Improve an existing feature labels May 6, 2024
@dyc3
Copy link
Contributor

dyc3 commented May 9, 2024

We can wrap evaluating rules with catch_unwind to catch panics, and that would allow us to surface the rule that was being evaluated. I'm not so sure about being able to surface the actual user code that resulted in the panic.

I could take a stab at implementing catch_unwind if that sounds like a good idea.

@ematipico
Copy link
Member

@dyc3
Copy link
Contributor

dyc3 commented May 9, 2024

What if we did the catch_unwind somewhere around here, where the rule is being evaluated?

for result in R::run(&ctx) {

This function seems to have all the context we want in the new error messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Project Area: project S-Enhancement Status: Improve an existing feature
Projects
None yet
Development

No branches or pull requests

3 participants