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

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

Open
Nahor opened this issue Mar 11, 2024 · 1 comment
Open

Comments

@Nahor
Copy link
Contributor

Nahor commented Mar 11, 2024

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)

Footnotes

  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
Copy link
Contributor Author

Nahor commented Mar 11, 2024

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.

Nahor added a commit to Nahor/miette that referenced this issue Mar 11, 2024
…ent line" and "just the span"

Fixes zkat#355

Change the number of context lines from usize to Option<usize> to allow
choosing between "just the span" (as implemented previously when using
0 context line) using `None`, and displaying the error line without
extra context (not possible before) using `Some(0)`.
Nahor added a commit to Nahor/miette that referenced this issue Mar 11, 2024
…ent line" and "just the span"

Fixes zkat#355

Change the number of context lines from usize to Option<usize> to allow
choosing between "just the span" (as implemented previously when using
0 context line) using `None`, and displaying the error line without
extra context (not possible before) using `Some(0)`.
Nahor added a commit to Nahor/miette that referenced this issue Mar 20, 2024
…ent line" and "just the span"

Fixes zkat#355

Change the number of context lines from usize to Option<usize> to allow
choosing between "just the span" (as implemented previously when using
0 context line) using `None`, and displaying the error line without
extra context (not possible before) using `Some(0)`.
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

No branches or pull requests

1 participant