fix(graphical): Fix panic when span covers full line

[Benjamin-L]

Fixes: #203

A classic off-by-one error, sorry :)

[zkat]

Note: I haven't merged this yet because I want to load it up in nushell and give it a shot to make doubleplus sure they won't just run into the same issue on upgrade again (and that there's no other surprises), but I just haven't made time for that. If you want to do it yourself, that would speed up getting this merged, though.

[Benjamin-L]

I made a build pointed at the v5.3.1 tag on git, and was able to reproduce the original behavior from #203:

/home/benjamin/nushell> 〉blah
Error: nu::shell::external_command (link)

  × External command failed
   ╭─[entry #1:1:1]
 1 │ blah
   · thread 'main' panicked at 'assertion failed: line_range.contains(&offset)', /home/benjamin/.cargo/git/checkouts/miette-45ce4058eaf7b1b9/9d665d9/src/handlers/graphical.rs:622:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

There's also a unit test failing against 5.3.1:

---- tests::test_parser::proper_missing_param stdout ----
stdout:
stderr: Error: nu::parser::missing_positional (link)

  × Missing required positional argument.
   ╭─[/tmp/nix-shell.YL7Iro/.tmpOWMQrK:1:1]
 1 │ def foo [x y z w] { }; foo a b c
   · thread 'main' panicked at 'assertion failed: line_range.contains(&offset)', /home/benjamin/.cargo/git/checkouts/miette-45ce4058eaf7b1b9/9d665d9/src/handlers/graphical.rs:622:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

thread 'tests::test_parser::proper_missing_param' panicked at 'assertion failed: !stderr.is_empty() && stderr.contains(expected)', src/tests.rs:121:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:---- tests::test_parser::proper_missing_param stdout ----
stdout:
stderr: Error: nu::parser::missing_positional (link)

  × Missing required positional argument.
   ╭─[/tmp/nix-shell.YL7Iro/.tmpOWMQrK:1:1]
 1 │ def foo [x y z w] { }; foo a b c
   · thread 'main' panicked at 'assertion failed: line_range.contains(&offset)', /home/benjamin/.cargo/git/checkouts/miette-45ce4058eaf7b1b9/9d665d9/src/handlers/graphical.rs:622:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

thread 'tests::test_parser::proper_missing_param' panicked at 'assertion failed: !stderr.is_empty() && stderr.contains(expected)', src/tests.rs:121:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::test_parser::proper_missing_param

test result: FAILED. 335 passed; 1 failed; 5 ignored; 0 measured; 0 filtered out; finished in 5.08s
    tests::test_parser::proper_missing_param

test result: FAILED. 335 passed; 1 failed; 5 ignored; 0 measured; 0 filtered out; finished in 5.08s

Trying the same thing pointed at this PR's branch:

/home/benjamin/nushell〉> blah
Error: nu::shell::external_command (link)

  × External command failed
   ╭─[entry #1:1:1]
 1 │ blah
   · ──┬─
   ·   ╰── can't run executable
   ╰────
  help: No such file or directory (os error 2)

However, the unit test is still failing, with a different error this time:

---- tests::test_parser::proper_missing_param stdout ----
stdout:
stderr: Error: nu::parser::missing_positional (link)

  × Missing required positional argument.
   ╭─[/tmp/nix-shell.YL7Iro/.tmpnakncE:1:1]
 1 │ def foo [x y z w] { }; foo a b c
   · thread 'main' panicked at 'byte index 33 is out of bounds of `def foo [x y z w] { }; foo a b c`', /home/benjamin/.cargo/git/checkouts/miette-79329ecf4b2054f7/dd256fd/src/handlers/graphical.rs:624:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

thread 'tests::test_parser::proper_missing_param' panicked at 'assertion failed: !stderr.is_empty() && stderr.contains(expected)', src/tests.rs:121:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::test_parser::proper_missing_param

---

It might be a while before I have time to look at this, might be better to just revert #202 for now if you want to get another release out soon.

[zkat]

I’m not really in a rush to put a release out but I’ll keep this in mind and consider reverting. I might end up taking as look at some point too but I also can’t manage it right now

[zkat]

Also, thanks for doing this legwork for me, I appreciate it

[Benjamin-L]

So I wrote that while pretty sleep deprived, and apparently did not realize that nu::parser::missing_positional is failing in both cases

[Benjamin-L]

Forgot to run cargo fmt on the first push :)

All the nushell tests are passing now. I also did some basic smoke testing in an interactive nushell session and nothing fell apart.

The issue turned out to be that I was conflating byte offsets with visual offsets in a way that only mattered with zero-width spans on the end of lines. I've added a test for this.

[Benjamin-L]

A possible future improvement would be to introduce newtype wrappers for visual and byte offsets, to avoid this kind of mistake at the type level. This would, however, add a fair amount of conversion boilerplate.

[Benjamin-L]

It looks like CI is busted for reasons unrelated to this PR (#209).

[Benjamin-L]

Alright, I've rebased against main now that #209 is merged. Hopefully CI will be happy now...