search

console-rs / indicatif

A command line progress reporting library for Rust
MIT License
4.21k stars 238 forks source link

indicatif crashes with assertion failed: self.orphan_lines_count <= self.lines.len() if number of terminal columns is less than 135 #612

Closed bes closed 6 months ago

bes commented 6 months ago

Hello!

The fix for https://github.com/console-rs/indicatif/issues/582 introduced a crash in my code, and I can't figure out why.

My application crashes with this message:

thread 'main' panicked at /Users/bes/.cargo/registry/src/index.crates.io-6f17d22bba15001f/indicatif-0.17.7/src/draw_target.rs:501:9:
assertion failed: self.orphan_lines_count <= self.lines.len()

If my terminal has enough "width" - 135 columns, it works without crashing If my terminal has below 135 columns, it crashes!

The number of rows seems to be irrelevant, I've tried everything from 9 to 74 lines.

djc commented 6 months ago

Sorry for the regression! Can you try if it works better with the changes from #608?

bes commented 6 months ago

@djc nope still crashes unfortunately

thread 'main' panicked at /Users/bes/.cargo/git/checkouts/indicatif-c82e440bcf1e0928/9169539/src/draw_target.rs:501:9:
assertion failed: self.orphan_lines_count <= self.lines.len()
smoelius commented 6 months ago

@bes Can you share your code, or provide instructions to reproduce?

bes commented 6 months ago

@smoelius here is a proof-of-concept https://github.com/bes/indicatif-assert-panic-poc

smoelius commented 6 months ago

I can reproduce the crash.

Can you give me a sense for what you expect that program's output to look like (i.e., if everything worked correctly)?

smoelius commented 6 months ago

Can you give me a sense for what you expect that program's output to look like (i.e., if everything worked correctly)?

Oh, I can just run it on a larger width to get that. 🤦

smoelius commented 6 months ago

The code in state.rs and multi.rs seem to have different ideas about what orphan_lines_count represents.

The code in state.rs seems to regard it a number of lines entries: https://github.com/console-rs/indicatif/blob/8941d5e8f55fe32a629a80872ee20971c00ee53d/src/state.rs#L155

Whereas the code in multi.rs seems to regard it as a visual line count (formerly real_len): https://github.com/console-rs/indicatif/blob/8941d5e8f55fe32a629a80872ee20971c00ee53d/src/multi.rs#L315-L323

When I wrote the fix for #582, I had the former in mind. But I see now (and understand why) @oli-obk had the latter in mind.

I think the code could be made to work either way. One approach just has to be chosen over the other.

Could one of the maintainers render a decision? Then I can try to prepare a fix.

djc commented 6 months ago

@smoelius thanks for digging in!

I don't have a strong preference so open to arguments that I'm wrong, but I feel like a "line" being a horizontal stripe across the window (that is, counting multiple for wrapped lines) is probably the more intuitive concept? In any case, would be great to use comments in all the relevant places to disambiguate how these numbers should be interpreted.

Could even go with newtype wrappers if there are a bunch of different places where we have to pass around context on these, but I feel like that's probably overkill here?

chris-laplante commented 6 months ago

Could even go with newtype wrappers if there are a bunch of different places where we have to pass around context on these, but I feel like that's probably overkill here?

I like the idea of newtype wrappers. Feels like a good idea to lean on Rust's type safety here, especially since flavors of this issue have been coming back for a while now.

smoelius commented 6 months ago

I like the idea of newtype wrappers. Feels like a good idea to lean on Rust's type safety here, especially since flavors of this issue have been coming back for a while now.

I agree that adding newtype wrappers sounds like the "right" solution. But it also sounds significant and invasive.

Could I suggests merging something like #613 now, and adding newtype wrappers in a separate PR?

Note that there is still the question of what orphan_lines_count should count: slice entries or vertical lines?

@djc suggested the latter. But I ran into trouble with this, and so I went with the former.

If orphan_lines_count remains a number of slice entries, then I expect a PR adding newtype wrappers would have to incorporate something like #613.

chris-laplante commented 6 months ago

I like the idea of newtype wrappers. Feels like a good idea to lean on Rust's type safety here, especially since flavors of this issue have been coming back for a while now.

I agree that adding newtype wrappers sounds like the "right" solution. But it also sounds significant and invasive.

Hmm, good point.

Could I suggests merging something like #613 now, and adding newtype wrappers in a separate PR?

Yes, merged!