miette icon indicating copy to clipboard operation
miette copied to clipboard

Label span is rendered incorrectly in report.

Open olson-sean-k opened this issue 2 years ago • 5 comments

I'm implementing Diagnostic manually for an error type so that it can provide different labels depending on the nature of the error. While testing, I noticed this report, which seems to render the span of the here label incorrectly:

Error: glob::rule

  x invalid glob expression: singular tree wildcard `**` in alternative

   ,----
 1 | {foo,**,bar,baz}
   : ^^^^^^^^|^^^^^^^
   :         |`-- here
   :         `-- in this alternative
   `----

I printed the LabeledSpans provided by Diagnostic::labels and they look correct to me:

[LabeledSpan { label: Some("in this alternative"), span: SourceSpan { offset: SourceOffset(0), length: SourceOffset(16) } }, LabeledSpan { label: Some("here"), span: SourceSpan { offset: SourceOffset(5), length: SourceOffset(2) } }]

Note that the span for the here label has an offset of 5 and length of 2, which should isolate ** in the source text for this error. I've tried relocating the token that ultimately emits this error and the span always appears correct but renders in the same way (spanning the entire alternative token). It almost looks as if the span is being replaced by the span of the in this alternative label.

I'm using version 3.0.0 of miette and other cases using as many as three separate labels render as expected. Any ideas? Thanks!

olson-sean-k avatar Sep 25 '21 00:09 olson-sean-k

Yeah, overlapping spans are not really... well supported (read: supported). Mostly because I couldn't figure out how to support them well? What do you think this should render as?

My recommendation otherwise is to make sure your single-line spans aren't overlapping, which isn't great. We do have choices, I just don't know which one to pick. I'd love a PR if that's something you're interested in doing, though :)

zkat avatar Sep 25 '21 03:09 zkat

What do you think this should render as?

I think the main problem is that the here label doesn't point at the beginning, end, or middle of the relevant source text. Just pointing at the beginning (or really anywhere within the span) could be better.

   ,----
 1 | {foo,**,bar,baz}
   : ^^^^^|^^^^^^^^^^
   :      `--|-- here
   :         `-- in this alternative
   `----

Even better (and possibly easier to implement) may be to provide separate underlines for spans over more than one character. In this case, that could look something like the following.

   ,----
 1 | {foo,**,bar,baz}
   : ^^^^^^^^^^^^^^^^
   :      ^^ |
   :      |  |
   :      |  `-- in this alternative
   :      `----- here
   `----

While it's a bit verbose, I think I would personally prefer this rendering since it makes the spans very clear. Any spans that refer to a single character could instead "punch through" a bit like the first example.

   ,----
 1 | {foo,**,bar,baz}
   : ^^^^^^^^^^^^|^^^
   :      ^^ |   `-- you goofed right here too
   :      |  |
   :      |  `-- in this alternative
   :      `----- here
   `----

I think this works so long as the difference between ^ and | beneath the source text is consistent. For multiline source text, this may remain clear so long as the number of necessary underlines wrap with the lines.

   ,----
 1 | Everything is okay. Oh No,
   :                     ^^^|^^
   :                        `-- uppercase?
 2 | I was very wrong!
   : ^^^^^^^^^^^^^^^^^
   :    |  ^^^^
   :    |   |
   :    |   `-- so very
   :     `----- it happens
   `----

   ,----
 1 | Memory safety isn't
   : ^^^^^^^^^^^^^^^^^^^
   :               ^^^^^
 2 | necessary at all.
   : ^^^^^^^^^^^^^^^^^
   : ^^^|^^^^^^^^^^^^
   :    |   |
   :    |   `-- especially this part
   :     `----- I don't like this statement
   `----

Colored output would make the various spans a lot clearer when things get more complex. 😄 Any thoughts on these examples?

olson-sean-k avatar Sep 25 '21 05:09 olson-sean-k

I think your last example might be my favorite honestly. I think it's pretty doable too.

Do you wanna give it a shot? I'm happy to give pointers!

zkat avatar Sep 25 '21 06:09 zkat