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

[chris-laplante]

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!

[djc]

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

[bes]

@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]

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

[bes]

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

[smoelius]

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]

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]

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:

indicatif/src/state.rs

Line 155 in 8941d5e

draw_state.orphan_lines_count = draw_state.lines.len();

Whereas the code in multi.rs seems to regard it as a visual line count (formerly real_len):

indicatif/src/multi.rs

Lines 315 to 323 in 8941d5e

	let orphan_lines_count = visual_line_count(&self.orphan_lines, width);
	force_draw |= orphan_lines_count > 0;
	let mut drawable = match self.draw_target.drawable(force_draw, now) {
	Some(drawable) => drawable,
	None => return Ok(()),
	};
	let mut draw_state = drawable.state();
	draw_state.orphan_lines_count = orphan_lines_count;

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]

@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]

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]

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.