Be kind of nice if we added some then. To be very very clear: I don't think just adding a warning is a complete solution... and it's a bit of a straw man to respond to that position.
1 Like
This deprecation seems to be an XY problem to me. What is this deprecation trying to solve really, and is there another way to solve that problem better?
The framing that "people don't know .unwrap() can panic" is very incomplete and shallow. Some novice users really don't know, but I bet that if you quizzed non-noob Rust users "does .unwrap() panic?" that most would correctly say "yes".
-
it's possible to know that
.unwrap()may panic in general, but incorrectly assume that a specific instance of.unwrap()won't ever panic because the value it's called on is supposed to be alwaysSome/Ok. -
it's possible to know that
.unwrap()may panic, and expect it to panic in exceptional circumstances, but underestimate when it can happen and how disruptive it can become (such as assuming that new columns won't be suddenly added to the database schema outside of a dev env…) -
it's possible for one developer to know that
.unwrap()may panic, and want it to panic, but their function/component/crate end up being used by someone else who did not expect that code to panic. Even with a full awareness that.unwrap()panics, it may be infeasible to know whether somecomponent.foobar()panics or not.
15 Likes
TBH I think that's on the developer of the crate to comment on the panics in the documentation. If the users of the crate did not read the documentation, then it's squarely their issue.
I assume you mean slice[x], arrays have statically known size. I would not expect production code to use that outside of tests. I suggest using clippy::indexing_slicing (and the one for strings too). I don't think there is one for indexing e.g. hashmaps, which is a bit of an oversight.
Well, .unwrap() is very clearly documented to panic. That singles it out, while giving other less obviously panicking functions a pass.
For cases where panics are unwanted, "RTFM" is not sufficient. Author of a library may miss a panicking case themselves.
A library may be written and integrated by others when it doesn't have any panics, and gain panicking code in a later version. This means that even reading docs of every function you use is not enough! You need to re-read docs of all functions of all libraries when you update them (everyone has to do this transitively for the whole call stack!)
2 Likes
pov: try to say "effect system" without saying "effect system"
1 Like
I would absolutely expect production code to use that outside of tests.
See, for example, how rustc uses panicking slicing into IndexVecs all the time by VariantIdx, by BasicBlock, by FieldIdx, etc.
Obviously it uses iter_enumerated or similar where feasible. But sometimes you have a Variants::Multiple with a FieldIdx of the field holding the tag, and you use fields[field_idx] to get it.
In those places it wouldn't be better to fields.get(field_idx).expect("the field index to exist in the layout that said to use it"). The fact that it's using indexing at all is the "well of course I expect it to be in-range".
Then hope you have a @matthiaskrgr to fuzz things and find out if there's some edge case that was missed and file bugs about it 
7 Likes
Wouldn't panicking be considered a breaking change, theoretically?
Since panic similarly affects system safety, it should also be restricted in a way that makes it annoying to use.
While I do wish something like Rustig or findpanics had a maintained alternative, and that the danger of sloppy panicking in my transitive dependencies is Rust's biggest weakness, that doesn't change that it's not relevant to the example given.
The Cloudflare system broke upstream and the Rust code was the first piece to hit the Emergency Stop, so arguing to push people away from using .unwrap() based on this incident is sort of like arguing that we should hide emergency stop buttons in factories or make them difficult to reach because production got shut down when someone fell off a catwalk.
22 Likes
I don't think there's any consensus around adding panics and semver in the community (except of course functions that explicitly promised to never panic).
Most functions' docs don't say anything whether the function panics or not. I think the default assumption should be that any function anywhere can panic, and it's caller's problem to use Drop guards, catch_unwind, etc.
There's still the practical problem that even if author of a function promises that the function will never panic, the author can be mistaken.
The rudra project found a bunch of panic safety bugs, usually caused by panics coming from user-provided callbacks, including unexpected ones like Ord or operator overloading implementations. This shows both that callers can be surprised by panics, and an assumption that any (safe) function is allowed to panic.
3 Likes
Deprecating .unwrap() in functions returning Result seems least controversial to me, and making the unwrap_in_result Clippy lint on by default would be the smallest step towards that:
5 Likes
If lint is considered a right solution to this problem, I suggest add something similar to the clippy::missing_safety_doc lint (which warns if you do not add a # Safety section in the documentation of an unsafe function). This lint can simply require developers to write a # Panic comment above all unwrap function to indicate the potential situations that will lead unwrap to panic.
2 Likes
Dealing with write! to &mut String or &mut Vec<u8> is annoying as is due to all the unwraps you have to make. Change like this will require additionally annotating each place with something either to allow deprecated functions or unwrap_in_result. Both .expect("") and .expect("this write never fails") feel wrong for some reason.
1 Like
Yes, but adding a potential panic path is as far as I know not considered a breaking change and to find it you'd have to look through the code after every dependency update to look for .unwrap(), array indexing and similar.
You can't even tell the compiler to warn you if a piece of code can panic (only via extra crates like no_panic, which have limits). Introducing a complete effect system (including effect generics) for this is difficult and has backwards compatibility pitfalls, but an opt-in for such a warning would already help a lot [1] if you ask me [2]:
#[maxpanics(1)] // Would have to apply for all T
fn foo<T>() {
// ...
}
fn bar() {
// Warns if the indexing check + panic is reachable/in binary
#[max_panics(0)] // Alias: no_panic?
{
arr[5];
}
#[max_panics(0)] // Make sure this specific T cannot panic
foo<usize>();
// Maybe allow more specific restrictions
// 2 panic paths max, only one of which can be from .unwrap()
#[max_panics(2, unwraps=1)]
#[debug_max_panics(4)] // Allow looser restrictions in debug mode.
some_dependency::baz();
}
// If we want to consider panic changes to be breaking changes
// opt-in + error if broken.
#[promise_max_panics(2)]
fn baz() {...}
This way we wouldn't just rely on "make sure you've read the documentation and hope a minor/patch update doesn't change it" but also give the developer the tools needed to get notified on changes related to panics and panic reach-ability. Perhaps even in a way that prints the locations of all possible/reachable panic paths so you can check them and adjust the max_panics annotations accordingly. [3]
Yes, I am aware that this is likely difficult to implement, especially since many optimizations are done in LLVM and that the amount usually differs between debug and release. [4]
But I think this would help a lot more than any renaming/deprecation could, even though you opt-into your code emitting warnings after a dependency update.
Real-world example where this would've been useful: Dependency that panics if a specific x86 instruction returns an unexpected error. In practice it's (hopefully) unreachable, but panicking in such a case is the best/correct choice, so I had to tell no_panic to assume the function cannot panic, even though I'd have loved to restrict it to a "only one line/expression can panic". In the end I could not get the compiler to check if there are other possible panic paths in that function.
By documenting and automatically checking the expected (amount of) panic paths. ↩︎
Yes, in reality it is likely not as easy as shown here, especially when generics are involved. ↩︎
Yes, in practice you may need to adjust a lot of locations on a dependency update, but that's likely what you want because it makes you think at every location with a
max_panicsif you're fine with this new panic path. ↩︎Unless I'm mistaken the compiler would have to (very late in compilation and not in
cargo check) be able to get a list of all panic locations (line) and types that end up being referenced in the assembly. ↩︎
1 Like
That's why the most viable solution is probably a maintained analogue to now-abandoned projects like Rustig and findpanics which analyzes the compiled artifact to identify paths leading to the panic handler and checks against a whitelist you provide for the functions which are allowed to panic.
No reliance on how the optimizer behaves. No effect system... just "which paths in the final binary's call graph lead to panic!?".
Heck, if something like that existed for Python and was reliable, I wouldn't hate exceptions so much.
3 Likes
It can also be a separate tool (perhaps a good idea). Perhaps even based on symbolic execution to further reduce false positives.
But in either case: It'd still depend on how the optimizer behaves because it can completely remove a panic path in one version and fail to do so after an update (even if the rust source code didn't change).
My bad. "No reliance on how the optimizer behaves for proving a program free of panics". no_panic can fail because the optimizer failed to see that a panic was unreachable. An ELF-analyzing tool should only be able to fail in the sense that the optimizer eliminated a panicking path which could show up to unexpectedly fail a CI that didn't involve a dependency bump.
(i.e. If it didn't see the panic because the optimizer eliminated it, mission ****ing accomplished... to quote the 'what if it stops a human?' comic I'm sure I saw on the first version of the Robot9000 announcement.)
If that's a concern, run the tool against a no-optimization debug build while you're choosing what to rewrite and what to whitelist.
IO can fail though, so it is usually best to bubble up the error to the application or a log. Panic on IO is rarely correct.
Normally, I'd agree (that's why, to this day, I'm wary of anything written by the author of goblin. I haven't trusted their judgment since the day I managed to panic goblin with one of my only mildly esoteric Open Watcom C/C++ "Hello, World!" test files.) but, it really does depend on how you architect your system.
Libraries should bubble up I/O errors so the binary can choose how they get handled, but you might want a binary to fail fast so some higher-level safety mechanism can either roll back the version or take it out of the pool.