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

Panic when span is on \n in v5.4.1 #215

Closed
Boshen opened this issue Oct 29, 2022 · 6 comments
Closed

Panic when span is on \n in v5.4.1 #215

Boshen opened this issue Oct 29, 2022 · 6 comments

Comments

@Boshen
Copy link
Contributor

Boshen commented Oct 29, 2022

Input text: "/\n"
Input span: 0..2

thread '<unnamed>' panicked at 'byte index 2 is out of bounds of /', /Users/boshenchen/.cargo/registry/src/github.com-1ecc6299db9ec823/miette-5.4.1/src/handlers/graphical.rs:624:21

let text = &line.text[..offset - line.offset];

cc @Benjamin-L

@Boshen
Copy link
Contributor Author

Boshen commented Oct 29, 2022

The previous version was able to point to the newline.

  × Syntax Error
   ╭─[./target/main.ts:1:1]
 1 │ /
   · ─┬
   ·  ╰── Unexpected Token
   ╰────

@Boshen
Copy link
Contributor Author

Boshen commented Oct 29, 2022

It also panics if the span is on EOF.

@zkat
Copy link
Owner

zkat commented Oct 29, 2022

@Benjamin-L lol are we still being cursed by the same issue?

@zh217
Copy link

zh217 commented Oct 30, 2022

I also had this problem on 5.4.1 and 5.4.0. Version 5.3.0 is fine.

@Benjamin-L
Copy link
Contributor

Benjamin-L commented Nov 3, 2022

Oof, yeah I didn't realize that spans that contained the EOL were previously supported. I'll have a fix for this up in a bit. While working this out, I found a few edge cases that we'll want to make sure we have good behavior for:

Edge Cases

CRLFs

In v5.3.0, these end up rendering like this:

Source: source\r\ntext\r\n here
Span: (8, 6)

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?

This seems... probably not desirable, and instead the rendered highlight should only extend one visual column past the end of line.

EOF

The output for this is bad on all versions:

Source: source\ntext
Span: (7, 5)

Error: oops::my::bad

  × oops!
   ╭─[bad_file.rs:1:1]
 1 │     source
 2 │ ╭─▶ text
   ╰────
  help: try doing it better next time?

Similar to the CRLF case, I think the rendered highlight should only extend one column past the end of line.

@Benjamin-L
Copy link
Contributor

It also panics if the span is on EOF.

@Boshen I actually wasn't able to reproduce this. When I tested v5.4.1 with a span including the EOF, I get the same bad behavior as v5.3.0, but I don't get a panic. Is this what you're seeing as well?

Benjamin-L added a commit to Benjamin-L/miette that referenced this issue Nov 3, 2022
)

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.
@zkat zkat closed this as completed in 8b56d27 Nov 24, 2022
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

4 participants