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

Group related diagnostics visually #171

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

TheNeikos
Copy link
Contributor

I understand that this can be a controversial change, so this is very much a suggestion. The idea would be to group related errors so that end-user understands what exactly caused the problems they've encountered.

An example from the tests:

Error: oops::my::bad

  × oops!
   ╭─[bad_file.rs:1:1]
 1 │ source
 2 │   text
   ·   ──┬─
   ·     ╰── this bit here
 3 │     here
   ╰────
  help: try doing it better next time?
╭─There were 2 related diagnostics:
├─ 1.Error: oops::my::bad
│
│
│   × oops!
│    ╭─[bad_file.rs:1:1]
│  1 │ source
│    · ───┬──
│    ·    ╰── this bit here
│  2 │   text
│    ╰────
│   help: try doing it better next time?
│ ╭─There were 1 related diagnostics:
│ ├─ 1.Error: oops::my::bad
│ │
│ │
│ │   × oops!
│ │    ╭─[bad_file.rs:1:1]
│ │  1 │ source
│ │    · ───┬──
│ │    ·    ╰── this bit here
│ │  2 │   text
│ │    ╰────
│ │   help: try doing it better next time?
│ │ ├─There were 0 related diagnostics:
│ │
│
├─ 2.Error: oops::my::bad
│
│
│   × oops!
│    ╭─[bad_file.rs:1:1]
│  1 │ source
│    · ───┬──
│    ·    ╰── this bit here
│  2 │   text
│    ╰────
│   help: try doing it better next time?
│ ├─There were 0 related diagnostics:
│

AsRef is not reflexive, meaning that it is not implemented as &T for all
T. Borrow is though, so we use that to make sure that we always get a
reference that is correct, even in the presence of smart pointers.
Previously diagnostics just had their StdError Display implementation
called. This now uses a GraphicalReport if wished.
│ 2 │ text
│ ╰────
│ help: try doing it better next time?
│ ├─There were 0 related diagnostics:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect this line to simply be omitted when there are no related diagnostics (what I expect to be the most common case to be fair).

Comment on lines +820 to +821
Copy link
Contributor

Choose a reason for hiding this comment

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

Is doubling the number of empty new lines deliberate or an oversight?
It does hurt the sense of visual grouping for me.

· ╰── this bit here
2 │ text
╰────
╭─There were 1 related diagnostics:
Copy link
Contributor

Choose a reason for hiding this comment

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

The vertical | works quite nicely to "mark" that the entire block is composed of related diagnostics, but it does feel like it crowds the left area: I would find it more difficult to quickly scan the output and determine how many errors I encountered.

I wonder if this could be helped by nesting the vertical line itself a bit more - e.g.

  There were 1 related diagnostics:
  │ 
  ├─ 1.Error: oops::my::bad
  │ 
  │   × oops!
  │    ╭─[bad_file.rs:1:1]
  │  1 │ source
  │    · ───┬──
  │    ·    ╰── this bit here
  │  2 │   text
  │    ╰────

@kleinesfilmroellchen
Copy link
Contributor

Several considerations from a user that would benefit greatly from this change:

  • "There were 1 related diagnostics" => "There was 1 related diagnostic"
  • "1.Error" => "1. Error"
  • As @LukeMathWalker pointed out, remove the "related" lead line if there are no related errors.
  • I still think this is too verbose for my use case. It may be worth the consideration of making the "related errors" style configurable for the GraphicalReportHandler and introduce some other styles. I'm especially thinking of an alternate, fully inline style (like what is currently done, but without just listing the different errors).

@CyriacBr
Copy link

CyriacBr commented Oct 10, 2022

I also think it'd be nice to be able group related errors better. The way related errors are displayed right now, each error seem completely independent, which make them actually seem unrelated.
Here's (very) roughly what one of my use case look like:

  × A runtime error occurred

Error: 
  × Cannot add a number to a bool
   ╭─[5:1]
 5 │             fn b($n)
 6 │                 $n + a()
   ·                 ─┬ ┬ ─┬─
   ·                  │ │  ╰── number
   ·                  │ ╰── invalid operation
   ·                  ╰── bool
 7 │             end
   ╰────
Error: code

  × error originated from here
    ╭─[8:1]
  8 │             fn c($n)
  9 │                 b($n)
    ·                 ──┬──
    ·                   ╰── error originated here
 10 │             end
    ╰────
Error: code

  × error originated from here
    ╭─[10:1]
 10 │             end
 11 │             c(true)
    ·             ───┬───
    ·                ╰── error originated here
 12 │             
    ╰────

As you can see, it's rather unclean that these errors are related.

Regarding this issue I see that it's still unsure what direction to take.
I find the current proposal rather verbose, and in my case I think it'd look much better just without the Error: ... clause. If there was an easy way to remove them, it'd make related errors more... related.

@zkat zkat force-pushed the main branch 3 times, most recently from 92a28b7 to b045321 Compare April 1, 2023 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants