Fix lacking space panic

[nuid64]

Reincarnation of #5529.
I apologize for the mess around this PR and thank you all for your patience.

[pascalkuthe]

I think the additional saturating subs and defensive clear are definitly correct fixes so this LGTM.

I am not entirely sure if we want to stop returning an optiom required_size to avoid similar crashes in the future and remove the footgun. That's mostly a code clarity issue tough and can be a separate PR

[archseer]

I'm a bit wary about changing this: the window size won't change halfway through a render. If clear_with is called with an out of bounds rectangle then that's a bug that should be addressed and skipping it here would hide the bug.

[nuid64]

I had a problem with that. Why window can not change it size during render?

[pascalkuthe]

I had a problem with that. Why window can not change it size during render?

The Buffer is an internal representation of the terminal. Therefore it only changes size if we manually call the resize method. This is only ever done in the event loop which is "paused" while rendering. So during a render the size of the buffer never changes as @archseer said.

You likely saw crashes here because of the incorrect use of wrapping sub before: #6109 (comment) or the crashed could have been caused by a separate bug that has not been discovered yet.

That being said, it might make more sense to add an assert at the start of the function¹ instead of using the panicing version of the function on every iteration. We could use a debug_assert. to avoid crashing the editor in daily use Simply clearing less screen is not really a problem until we try to draw on it which would lead to a panic anyway. Cases where we are really just clearing too much screen would not crash the editor this way while still being just as easy to debug.

Footnotes

1. Compilers will not move assets to the start of the function to preserve the order of operations/the work done before the panic. Moving a bounds-check to an explicit assert before a loop is a nice performance optimization and usually more readable while keeping the boundscheck intact. It's used commonly in the std library and I wish more crates would use it too. ↩

[nuid64]

Thank you for the detailed answer. clear_with is not related with crashes I had indeed.

[archseer]

I am not entirely sure if we want to stop returning an optiom required_size to avoid similar crashes in the future

The idea there is that required_size is a hint to the parent component on how much space is needed. Similar to https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.size_hint

[pascalkuthe]

I am not entirely sure if we want to stop returning an optiom required_size to avoid similar crashes in the future

The idea there is that required_size is a hint to the parent component on how much space is needed. Similar to https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.size_hint

Hmm but size hint also always returns a lower bound and only the upper bound is optional. The value is optional because allocating a larger value is only a performance optimization and never causes a panic, because the implementation can always fallback to the lower bound (which is not optional and just defaults to 0).

If I read the code right required_size is not actually treated like the upper bound of size_hint. It's treated like the exact size used by the component which would be equivalent to size_hint returning (size Some(size)) or the lower bound (which is also not an Option for size_hint). Right now I don't really see why a component would ever return None from required_size as all called of required_size unwrap the return value.

[nuid64]

Well now I don't think any bound-check is redundant.

thread 'main' panicked at 'index out of bounds: the len is 8 but the index is 8', helix-tui/src/buffer.rs:476:17
stack backtrace:
   0: rust_begin_unwind
             at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:64:14
   2: core::panicking::panic_bounds_check
             at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:147:5
   3: helix_tui::buffer::Buffer::set_style
   4: <F as helix_term::ui::document::LineDecoration>::render_background
   5: helix_term::ui::document::render_document
   6: helix_term::ui::editor::EditorView::render_view
   7: <helix_term::ui::editor::EditorView as helix_term::compositor::Component>::render
   8: helix_term::application::Application::render::{{closure}}
   9: helix_term::application::Application::run::{{closure}}
  10: tokio::runtime::park::CachedParkThread::block_on
  11: tokio::runtime::runtime::Runtime::block_on
  12: hx::main

Here's set_style function:

pub fn set_style(&mut self, area: Rect, style: Style) {
    for y in area.top()..area.bottom() {
        for x in area.left()..area.right() {
            self[(x, y)].set_style(style);
        }
    }
}

I think bound-check won't harm readability more than it's absence harms sustainability.

[pascalkuthe]

Well now I don't think any bound-check is redundant.

thread 'main' panicked at 'index out of bounds: the len is 8 but the index is 8', helix-tui/src/buffer.rs:476:17
stack backtrace:
   0: rust_begin_unwind
             at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:64:14
   2: core::panicking::panic_bounds_check
             at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:147:5
   3: helix_tui::buffer::Buffer::set_style
   4: <F as helix_term::ui::document::LineDecoration>::render_background
   5: helix_term::ui::document::render_document
   6: helix_term::ui::editor::EditorView::render_view
   7: <helix_term::ui::editor::EditorView as helix_term::compositor::Component>::render
   8: helix_term::application::Application::render::{{closure}}
   9: helix_term::application::Application::run::{{closure}}
  10: tokio::runtime::park::CachedParkThread::block_on
  11: tokio::runtime::runtime::Runtime::block_on
  12: hx::main

Here's set_style function:

pub fn set_style(&mut self, area: Rect, style: Style) {
    for y in area.top()..area.bottom() {
        for x in area.left()..area.right() {
            self[(x, y)].set_style(style);
        }
    }
}

I think bound-check won't harm readability more than it's absence harms sustainability.

This is a completly separate bug, totally unrelated to the issue this PR is adressing, It looks like it might be caused caused by a regression in #5420. Please open a separate issue for that. While it might be ok to ignore clear_with calls with an out of bounds rect, set_style is actually used for rendering and should under no circumstance allow any out of bounds positions. Anytime set_style is called with an out of bounds rect there is a bug which would manifest in rendering issues. We should always panic in that case otherwise debugging rendering issues basically becomes extremely hard. Removing the panic here is really just a bandage and not the real fix.

Please remove the changes to set_style and clear_with so we can merge the other two (correct) fixes

[nuid64]

I don't understand why set_style call with an out of bounds Rect is ok. Isn't it a bug itself?

[pascalkuthe]

I don't understand why set_style call with an out of bounds Rect is ok. Isn't it a bug itself?

No it's not ok it is definitely a bug that is why it should panic. The idea is to fail fast and fail hard. If set_style just silently ignores out of bounds positions we would never be able to find those kinds bugs. The bug is not that set_style panics, the bug is that set_style is called with an out of bounds position. To fix this problem the callsite should be fixed to not call set_style incorrectly instead of making set_style ignore an incorrect argument.

[nuid64]

Maybe it caused by the window size changing during clear_with, but it's strange helix not crashes during rendering, so I guess you're right.

[pascalkuthe]

As I said earlier it's impossible for the buffer size to change during rendering , the cause of the crash is a bug somewhere else in the code that should be fixed. If you can find out what caused the crash in set_style above I am happy to look into a fix. Panics make it a lot easier to find the actual causes of bugs.

[nuid64]

People can lose unsaved changes due to panic, aren't there ways to keep helix sustainable while not hiding information for debug? Maybe debug code which will be executed in debug build?

[nuid64]

Maybe it caused by the window size changing during clear_with, but it's strange helix not crashes during rendering, so I guess you're right.

Wait, it does.

[pascalkuthe]

People can lose unsaved changes due to panic, aren't there ways to keep helix sustainable while not hiding information for debug? Maybe debug code which will be executed in debug build?

That's what debug_assert does. It still makes bug reports a lot harder to track down (often I can guess a bug and fix just from the panic back-trace without a reproduction case which most users can't/won't provide). I think it's ok to only use debug_assert in clear_with because there are no adverse effects to not clear something off screen (that's why I suggested debug_assert in #6109 (comment)) but for set_style this would lead to visual artifacts so in that case a crash is preferable so we can quickly identify the problems

[archseer]

That's what debug_assert does.

Very few people actually use debug builds outside of CI/integration tests yeah. Even locally I only run release builds

[pascalkuthe]

That's what debug_assert does.

Very few people actually use debug builds outside of CI/integration tests yeah. Even locally I only run release builds

Hmm yeah it does make bug reports harder to track down (see second sentence). I suggested this because we use debug_assert in quite a few other places to assert an argument is correct. Maybe we want to make some of those debug_assert normal assert too.