zkat / miette

Fancy extension for std::error::Error with pretty, detailed diagnostic printing.
https://docs.rs/miette
Apache License 2.0
1.98k stars 113 forks source link

Errors with zero-length span and no context lines are mostly pointless #355

Open Nahor opened 6 months ago

Nahor commented 6 months ago

Code

    #[derive(Debug, Diagnostic, Error)]
    #[error("oops!")]
    #[diagnostic(code(oops::my::bad), help("try doing it better next time?"))]
    struct MyBad {
        #[source_code]
        src: NamedSource<String>,
        #[label("this bit here")]
        highlight: SourceSpan,
    }

    let src = "one\ntwoo\nthree".to_string();
    let err = MyBad {
        src: NamedSource::new("bad_file.rs", src),
        highlight: (6, 0).into(),
    };
    [...]
    handler.with_context_lines(0);
    [...]

Result

oops::my::bad

  × oops!
   ╭─[bad_file.rs:2:3]
   ╰────
  help: try doing it better next time?

Expected:

oops::my::bad

  × oops!
   ╭─[bad_file.rs:2:3]
 2 │ twoo
   ·   ▲
   ·   ╰── this bit here
   ╰────
  help: try doing it better next time?

Problem

The issue is read_span with 0 context line returns just the content of the span, not the whole line. Since the span is 0-length, there is no content. The alternative is to specify at least one context line, but that mean three lines will be display (1 before, error line, 1 after), which is more than what is wanted.

Also, this gets really weird when there are multiple 0-length spans, e.g. same as above but with a 2nd span/label at (12, 0), the first e in three:

oops::my::bad

  × oops!
   ╭─[bad_file.rs:2:3]
 2 │ oo
   · ▲
   · ╰── this bit here
 3 │ thr
   ╰────
  help: try doing it better next time?

which ought to look like this:

oops::my::bad

  × oops!
   ╭─[bad_file.rs:2:3]
 2 │ twoo
   ·   ▲
   ·   ╰── this bit here
 3 │ three
   ·    ▲
   ·    ╰── and here
   ╰────
  help: try doing it better next time?

Note how the second label doesn't show in the broken version, only the text just before it

Possible solutions:

  1. read_span and with_context_lines [^1] could take an Option<usize>, so one could distinguish between "just the span content" (None) and "just the error line" (Some(0)). The drawback is an API break.
  2. or just change the meaning of "0 context" to mean "error line". But that just inverts the problem since there wouldn't be any way to get "just the span content" anymore. This becomes an issue if the document is one single long line (e.g. minified javascript).
  3. or keep the status quo (which of course, does not solve anything)

[^1]: or to reduce the amount of API breakage, add a with_opt_context_lines(Option<usize>) function and convert with_context_lines(0) into with_opt_context_lines(None)

Nahor commented 6 months ago

I'm currently looking at implementing solution 1 with a with_opt_context_lines. Let me know if you don't think you'll take such a PR. And if you have a better solution, I can look at implementing that instead.