Add methods that return results / options for fallible operations

[joshka]

Problem

There are a variety of places, usually in calls to Buffer, where things can fail.
These methods should generally have alternatives that fail with an Err result, or return None instead of panicking.

Solution

1. Exhaustively determine all the places where we can fail. Pay particular attention to the Buffer indexing methods. (Use clippy lints to help this process as noted below)
2. Determine for each whether it's reasonable to change the existing method by breaking compatibility or whether it's better to do so in an additive way (e.g. adding try_xxx() -> Result
3. Determine an Error type that we use
4. Implement the changes
5. Transition the example code and docs to use the new code where possible
6. Where things can safely ignore the failures, do so (e.g. rendering a widget outside of the buffer should generally just be ignored, and perhaps logged, instead of causing a panic)

Alternatives

Additional context

Specifically Buffer::get() is a place that often causes panics.

[EdJoPaTo]

The currently allowed lint clippy::missing_panics_doc catches a bunch of these cases and requires to write a panic documentation. Then we just need to seek documentation including this panic section.

Also, other lints detect arithmetic or array access out of bounds that can panic. A lot of this should be detectable by enforcing more lints.

[joshka]

Good idea - added to the solution.

[EdJoPaTo]

Buffer::index_of could get an alternative Buffer::index_of_opt returning Option<usize> instead. Same with Buffer::get(_mut) → Buffer::get(_mut)_opt returning Option<&Cell>.
The methods potentially panicing should be deprecated in favor of the _opt methods.

See #1047 which does something similar despite Rect::intersect not panicing.

[joshka]

I was thinking about how to best migrate some of the methods that have poor adherence to the general rust API standards like this. An alternate approach is to fix the methods properly (i.e. the get() method returns Option<&Cell>), but put the old behavior behind a feature flag to make it easy for anyone to upgrade. What about something like:

#[cfg(not(feature = "legacy-buffer-methods"))]
fn get<T: Into<Position>>(position: T) -> Option<&Cell> {
   ...
}

/// This is included to make it easy to upgrade, blah blah blah feature flag, 
#[cfg(feature = "legacy-buffer-methods")]
#[deprecated]
fn get(x: u16, y: u16) -> &Cell {
  ...
}

This would allow us to release the new feature in a (mostly) non-breaking manner, but then change the default feature flags in a future release.

We might also consider implementing the Index trait (though I'm not sure whether this should be indexed using Position, Into<Position> or (x, y)).

[EdJoPaTo]

This forces someone to migrate everything at once. Also, I am not sure what happens with dependencies. My widget is in a different crate, it enables a feature. What about the one using that widget. Is that feature needed then too?

[kdheepak]

Also, rust best practices guidelines have this to say:

For getters that do runtime validation such as bounds checking, consider adding unsafe _unchecked variants. Typically those will have the following signatures.

fn get(&self, index: K) -> Option<&V>;
fn get_mut(&mut self, index: K) -> Option<&mut V>;
unsafe fn get_unchecked(&self, index: K) -> &V;
unsafe fn get_unchecked_mut(&mut self, index: K) -> &mut V;

https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter

[joshka]

This forces someone to migrate everything at once. Also, I am not sure what happens with dependencies. My widget is in a different crate, it enables a feature. What about the one using that widget. Is that feature needed then too?

I vaguely recall writing a response along the lines of "ah yes, you're right of course, feature flags are a problem when there are multiple paths to the crate that could have different feature flags". Either it was on a different issue with a similar idea or I might have not posted it. Either way, yeah you're right that's not a viable approach.

[joshka]

@EdJoPaTo do you have any thoughts about making whether to make this a breaking change compared to using _opt (for Buffer::get and get_mut?

For context the impact of this external to ratatui is ~118 files:
https://github.com/search?q=ratatui%20buf.get%20AND%20(NOT%20org%3Aratatui-org)%20AND%20(NOT%20is%3Afork)&type=code

[EdJoPaTo]

Probably results in a lot of internal churn. Earlier when looking at git blame for the set_stringn stuff I noticed it was -> Option<usize> ages ago so it might be interesting to see why this changed. Maybe performance…

[EdJoPaTo]

For context the impact of this external to ratatui is ~118 files

Was curious why my usage of it doesnt show up: I dont name them buf, I name them buffer. Interestingly, they still dont show up.

Query used: ratatui "buffer.get(" AND (NOT org:ratatui-org) AND (NOT is:fork)

[joshka]

At least one result in ratatui "buffer.get" AND (NOT org:ratatui-org) AND (NOT is:fork)

[EdJoPaTo]

Ah… my get( removed the get_mut which I used…