fix(graphical): Fix panic with span extending past end of line (#215)

[Benjamin-L]

This also changes the behavior with spans including a CRLF line-ending. Before the panic bug was introduced, these were rendered with the CRLF being two visual columns wide. Now, any span extending past the EOL is treated as including one extra visual column. See #215 (comment) for an example of this.

This PR notably does not fix the rendering bug with spans including the EOF. These were rendered incorrectly before the panic bug was introduced. @Boshen mentioned that they were seeing a panic with EOF in #215, but I haven't been able to reproduce that. We may want to wait for a response from them before merging this, since it seems like I'm missing something here.

[Boshen]

Yeah it was probably the bad behaviour.

To make sure everything is correct, I've tested your patch.
It seems the graphic handler need to be updated as well, it crashed in the same spot.

miette/src/handlers/graphical.rs

Lines 620 to 626 in 3e25fd5

	fn visual_offset(&self, line: &Line, offset: usize) -> usize {
	let line_range = line.offset..=(line.offset + line.length);
	assert!(line_range.contains(&offset));
	let text = &line.text[..offset - line.offset];
	self.line_visual_char_width(text).sum()
	}

[Boshen]

With the upgrade, I found another interesting case where I was pointing the span inside a unicode.

v5.3.0:

offset 5..7

  × Invalid Character `ⸯ`
    ╭─[language/identifiers/vertical-tilde-continue.js:15:1]
 15 │ 
 16 │ var aⸯ; // U+2E2F
    ·      ─┬
    ·       ╰── Invalid Character `ⸯ`
    ╰────

v5.4.0:
Span: 5..7 gives

'byte index 7 is not a char boundary; it is inside 'ⸯ' (bytes 5..8) of `var aⸯ;

I had to change the span to 5..8 to cover the unicode

  × Invalid Character `ⸯ`
   ╭─[main.ts:1:6]
 1 │ var aⸯ;
   ·      ┬
   ·      ╰── Invalid Character `ⸯ`
 2 │
   ╰────

[Boshen]

And to make things more annoying ...

Although v5.4.0 is more accurate when pointing to a unicode, but the graphical reporter will use the range pointer ┬, it would be even nicer to get it to use the single char pointer ▲.

[Benjamin-L]

Yeah it was probably the bad behaviour.

To make sure everything is correct, I've tested your patch. It seems the graphic handler need to be updated as well, it crashed in the same spot.

miette/src/handlers/graphical.rs

Lines 620 to 626 in 3e25fd5

	fn visual_offset(&self, line: &Line, offset: usize) -> usize {
	let line_range = line.offset..=(line.offset + line.length);
	assert!(line_range.contains(&offset));
	let text = &line.text[..offset - line.offset];
	self.line_visual_char_width(text).sum()
	}

To be clear, you tested input similar to the examples from #215 against this PR and are still seeing a panic? If so, could you send the input that you're seeing the panic on?

The code block linked in that comment is the same one that this PR is changing:

https://github.com/zkat/miette/pull/221/files#diff-a775e9b52316667cfcabc8d12a083cba8d18759534d48b7a5da6cd647bc6fe6dR620-R639

I tested the input from #215 during development of this. I was able to reproduce the panic with the main branch, but did not reproduce the panic with the code from this PR.

[Benjamin-L]

With the upgrade, I found another interesting case where I was pointing the span inside a unicode.

v5.3.0:

offset 5..7

  × Invalid Character `ⸯ`
    ╭─[language/identifiers/vertical-tilde-continue.js:15:1]
 15 │ 
 16 │ var aⸯ; // U+2E2F
    ·      ─┬
    ·       ╰── Invalid Character `ⸯ`
    ╰────

v5.4.0: Span: 5..7 gives

'byte index 7 is not a char boundary; it is inside 'ⸯ' (bytes 5..8) of `var aⸯ;

I had to change the span to 5..8 to cover the unicode

  × Invalid Character `ⸯ`
   ╭─[main.ts:1:6]
 1 │ var aⸯ;
   ·      ┬
   ·      ╰── Invalid Character `ⸯ`
 2 │
   ╰────

I've created a separate issue for this: #223.

And to make things more annoying ...

Although v5.4.0 is more accurate when pointing to a unicode, but the graphical reporter will use the range pointer ┬, it would be even nicer to get it to use the single char pointer ▲.

I don't think this is specific to multi-byte unicode characters. Miette currently only uses the ▲ pointer when the span has length 0 (it points to a specific index, but doesn't include the following character in the span). The ┬ pointer is used for any single-line span with nonzero length. This includes multi-byte characters that take a single visual column (length > 1), but also includes single-byte characters with a single visual column (length = 1).

IMO, it could go either way as to whether or not this behavior is correct. The one thing that I think is definitely not correct is that multi-byte spans with zero visual columns (for example, spanning a zero-width-space or the \r of a CRLF with the code in this PR) are rendered as ┬ rather than ▲, which is actively confusing. I'll make an issue for this one later today.

[Benjamin-L]

@Boshen I created an issue for the ┬ usage on spans pointing at zero-width nonempty spans: #224. That issue is probably the right place to discuss whether or not ▲ should be used for 1-wide spans.

[Boshen]

The code block linked in that comment is the same one that this PR is changing:

Oh I was looking at the wrong place again, I got dizzy from testing all these difference branches and versions 😵

Anyway, this PR passed all my tests (I have a JavaScript / TypeScript compiler running with all Test262 / Babel / TypeScript conformance tests).