cessen
fix: add track_caller attr on panicking methods
When using panicking versions of the Rope methods, IMO it's better to get the location where you didn't respect the invariants that lead to the panic over the location of the methods in ropey's code.
Thanks for the PR!
I'm a little torn on this one. On the one hand, I definitely agree it makes the stack traces easier to read for the users of the library. But on the other hand, it makes the stack traces less useful when trying to track down bugs in Ropey itself (e.g. if someone runs into a panic they can't repro, but they do have a stack trace they can include in the report).
I'll have to think on this one for a bit. But regardless, I appreciate the time you took to put together and submit the PR!
I'm playing around with #[track_caller] in the Rust playground now, and it doesn't quite do what I expected:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=ef050a4bb7543109589d76826f520f6e
First of all, if you enable stack traces, the whole stack trace gets printed regardless. In retrospect I should have realized this, since it always goes deep into stdlib on panics regardless. In other words, my concern about stack traces is completely unwarranted: you always get the full stack trace. So nevermind.
But second, it reports that the error (in my linked example) is due to unwrapping None, which doesn't happen on the line it points you to. In other words, it gives you a line number from higher in the call stack, but still reports the error from the panic site.
This makes sense from a technical perspective: it's just printing the error message specified in the actual panic. But it does mean just adding the #[track_caller] attribute to the functions in the call stack path isn't enough. We'll also need to add explicit messages (e.g. by using expect() and match as appropriate, rather than unwrap()) that will make sense to the user of Ropey.
In short: I think we should do this, but it will involve additional work beyond this PR. I'll have a bit more of a think about it, and I'll also need to give the code a closer review. But I think what probably makes sense is to merge this PR as-is, and then we can start working on improving the panic messages to make sense to users of the library.
I totally agree that unwrap() on None value is way too cryptic if we bubble up panics. The commit I just pushed replaces unwraps with expects where most have the message index out of bounds. With the call location, I think it gives enough information to the user to figure it out.
I've also taken the liberty to remove custom panic messages that include lengths. I can revert anyway. To me, these take a lot of space and don't provide much error value. You can get this info but printing it yourself when debugging.
Apologies for not getting around to actually merging this. I wanted to give it a closer review before that, and just never got around to it.
Ropey 2.0 (currently in beta) uses track_caller for panics, and with the beta out I'm declaring Ropey 1.x "no new development, only maintenance". So I'm going to close this.
Really sorry about the run-around and eventual reject. If it makes you feel any better, this PR is what pushed me to include the use of track_caller in 2.0, so it wasn't completely wasted effort. Thank you for taking the time and effort to make this PR!