feat(buffer): track_caller for index_of

[EdJoPaTo]

The caller put in the wrong x/y -> the caller is the cause.

In the future get and get_mut should return Option but thats a bigger change.

[kdheepak]

In the future get and get_mut should return Option but thats a bigger change.

I agree with this change. Can we open an issue for this to track it?

[EdJoPaTo]

In the future get and get_mut should return Option but thats a bigger change.

I agree with this change. Can we open an issue for this to track it?

#1011 is less specific to Buffer but includes this.

[joshka]

I think these might give some messages that might not make contextual sense, but I'm not sure. Perhaps a few unit tests for this would help clarify. I'd want to see them from the perspective of a direct call (i.e. what panic message will a widget see calling get(x, y).symbol("x")), as well as a call to methods in the buffer that call these methods e.g. buf.set_style(area, Color::Blue).

[EdJoPaTo]

get looks like this then:

--- a/tests/terminal.rs
+++ b/tests/terminal.rs
@@ -38,7 +38,7 @@ fn terminal_draw_returns_the_completed_frame() -> Result<(), Box<dyn Error>> {
         let paragraph = Paragraph::new("Test");
         f.render_widget(paragraph, f.size());
     })?;
-    assert_eq!(frame.buffer.get(0, 0).symbol(), "T");
+    assert_eq!(frame.buffer.get(30_000, 0).symbol(), "T");
     assert_eq!(frame.area, Rect::new(0, 0, 10, 10));

---- terminal_draw_returns_the_completed_frame stdout ----
thread 'terminal_draw_returns_the_completed_frame' panicked at tests/terminal.rs:41:29:
Trying to access position outside the buffer: x=30000, y=0, area=Rect { x: 0, y: 0, width: 10, height: 10 }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Note that the test output panicked at the diff line and not at the assertion inside the index_of method (which is used by get).

buf.set_style is not able to create this kind of error as it uses intersect to ensure the input is in range. But set_stringn uses index_of which can fail here:

--- a/src/widgets/list.rs
+++ b/src/widgets/list.rs
@@ -992,7 +992,7 @@ impl StatefulWidgetRef for List<'_> {
                 };
                 if selection_spacing {
                     buf.set_stringn(
-                        x,
+                        x + 30_000,
                         y + j as u16,
                         symbol,
                         list_area.width as usize,

---- widgets::list::tests::test_padding_offset_pushback_break stdout ----
thread 'widgets::list::tests::test_padding_offset_pushback_break' panicked at src/buffer/buffer.rs:219:30:
Trying to access position outside the buffer: x=30000, y=0, area=Rect { x: 0, y: 0, width: 10, height: 4 }

The panic happened inside the set_stringn method at the call of index_of.

[joshka]

Added / adjusted tests.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.4%. Comparing base (bef5bcf) to head (4116b9a).
Report is 8 commits behind head on main.

❗ Current head 4116b9a differs from pull request most recent head 87c239e. Consider uploading reports for the commit 87c239e to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1046     +/-   ##
=======================================
- Coverage   89.4%   89.4%   -0.1%     
=======================================
  Files         61      61             
  Lines      15430   15425      -5     
=======================================
- Hits       13799   13794      -5     
  Misses      1631    1631             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[EdJoPaTo]

Added / adjusted tests.

These tests are unrelated to this PR? This PR is about track_caller to simplify finding issues with x / y supplied.
The tests added check the debug_assert inside of index_of. Having them is probably good but not part of the original idea for this PR?
(Also probably won’t work on cargo test --release)

[joshka]

(Also probably won’t work on cargo test --release)

Good catch - I didn't think of that.

I'll remove and merge this without the added tests.