The "expect_used" lint is useless?

This, so much.


I'll add that witch-hunting .expect()s and/or .unwrap()s without witch-hunting unreachable!()s, panic!()s, and assert!s is just contradictory: all of these can and should be used to make the code guard against unrecoverable situation: it's really supposed to represent a fatal error.

Some people, in some situations, may dislike the idea of unwinding altogether, since it could jeopardize the whole thread (if panic = unwind) or process (if panic = abort) altogether. In the former case, catch_unwind is there to help with that (although it could lead to memory leaks), and in the latter case, or if memory leaks are unacceptable, using subprocesses is then the more general solution to contain these disastrous / fatal situations.

  • So, in that context, trying to avoid the whole unwinding mechanism could be desirable (no panic guarantee): that would mean that all your APIs would be made -> Result-fallible, and that you'd have a FatalInternalError variant for the error case:

    macro_rules! bail {( $($tt:tt)* ) => ({
        error!($($tt)*);
        return ::core::result::Result::Err($crate::FatalInternalError);
    })}
    // etc.
    
    // and for unwrap/expect:
    let value = option.ok_or(FatalInternalError)?;
    let value = option.ok_or_else(|| bail!("Thing should be in here!"))?;
    

But, imho, doing this pervasively / systematically would really be like lifting a finger at the whole panic / unwinding mechanism (which some people may be genuinely tempted to do :sweat_smile:): the language was made with panics for a reason, so it's legitimate to use them as well (again, though: this should still be done sparingly, only for assertions / logic bugs / something that in a perfect program would be genuinely unreachable).


A practical example

Proc-macro code is famous for not really having "a Result-based API baked in", which may have lead people to use panic! as a way to error on invalid input. But this is not good practice / good style, since the error message becomes ominous (proc-macro panicked), and the error location, awful (it just blames the macro itself, not the problematic part of the input).

With libraries such as ::syn, one can get back to a Result-based API, with an unwrapping in the very final layer of .unwrap_or_else(|err| err.into_compile_error().into()).

Does that mean you should never panic in proc-macro code? No! Again, for logic bugs / supposedly unreachable / crazy code paths, I think panicking would be in order: should there be a bug and that panic be reachable, the "ominous" proc-macro panicked would be warranted, since it would clearly convey there being a bug in the proc-macro code, not a problem with the input the caller gave it.

  • An example when using syn is that if you .peek()ed a certain token, you ought then to follow-up with .parse().unwrap(), rather than .parse()?: fail loudly on logic bugs!

This is similar to nice compiler errors on incorrect input, versus fatal compiler bugs on mishandled input: ICEs (Internal Compiler Error), which stand for the compiler itself panicking rather than erroring.

So I personally find the distinction to be useful, and for these situations, the built-in unwinding mechanism to be quite convenient :slightly_smiling_face:

10 Likes

Now you're talking invariant. Invariant is expected to be true throughout all the program flow (with the exception of internal code that might temporarily render invariant invalid, usually protected by a mutex).
In your example, a None value is breaking the invariant and by definition is allowed/expected to panic as something exceptional. Some languages have invariants as a part of the syntax (D and - if I remember right - Ada).

The other my question rising here: If a variable can have 2 states (Some(x) and None), but v.is_some()==true is invariant, why Option<x> in the first place? If Some() is the only expected value, it should be "squashed" as soon as possible with relevant comment and (eventually) assert!.

1 Like

Um... Is this a syntactic sugar for

let value = if let Some(value) = opt.take(){
  value
} else {
  unreachable!("`opt` should only be taken from by one path");
}

?

Generally because your invariant is more complex: for example "this should always be Some in between A and B being called, both are internal to my crate, and they should always be called by my code in that order"

1 Like

If we're talking about bubbling Result vs panic me, the question is about how you deal with OS and other external APIs, which claim they can only error in specific situations, especially where it can only error with specific invalid parameters, and where you are sure you are not going to hit those documented cases. Rust has picked "bubble everything in a catch-all io::Result", but in this sort of situation that's just punting the problem to your caller, who has even less idea what to do with it and ability to fix it.

You can claim that "the API could add more error conditions, or have undocumented error cases", but if anything I'd want both of those to crash as soon as possible: if I'm trying to provide a safe wrapper and I don't understand an external, unsafe API then all bets are off!

You are making strong, unfounded assertions, and are actively absusing the word "unsafe". Unsafe has a very specific meaning in Rust, concretely, it means "not provable by the compiler to manage memory correctly, thus has the potential for undefined behavior".

Consequently, an I/O API isn't unsafe just because you don't know the exhaustive list of conditions when it might return an error. Your claim that errors should just crash ASAP is also wrong, because once again, panicking means that:

  • the user of the code can't control what happens when an error occurs (catch_unwind is not a reliable panic handling mechanism, because compiler flags outside the control of any crate authors can turn all panics into immediate aborts)
  • and s/he doesn't even know about the existence of error conditions, because panicking doesn't affect the type/signature of functions (and correctly so, because it's a diverging operation).

Both are highly problematic as a general error handling mechanism. Furthermore, in many situations, reliability requirements dictate that code must not panic (e.g. in a long-running webservice, in a real-time system, etc.) so the only acceptable thing is to return a Result::Err.

While this might sound appealing in theory, in practice it's almost always false. Dealing with arbitrary I/O means that several kinds of counter-intuitive error conditions can arise from several systems and levels of abstraction. Claiming that you know the exact, exhaustive set of error conditions is thus not productive and dangerous. Instead, you should aim to be able to propagate errors so that if something that your particular piece of code didn't expect happens, it can be handled down the line. If the caller recognizes it as a hard, irrecoverable error, then they can turn the Result::Err into a panic!(), but they can't (and won't) do the inverse.

You seem to have missed where I said:

That is, extern { fn blah() }. I hope it's not unfounded that those are unsafe?

Of course: but not all native APIs are IO, where the implications of, eg. write failing is pretty obvious and uniform: your write might not have gone through. The cause might be transient, it might require user interaction to solve, etc, so do whatever is appropriate for your application, prompt the user, log, etc..

But I'm talking about APIs like Window's CreateEvent() which is only documented to error if you pass a name and it's in use. If you don't pass a name and it errors... what are you supposed to do then? You don't have an event handle, and you have no idea how to fix that. Trying again will certainly not help; the only other case I could think of where it might fail is if you're out of kernel handles, in which case treating it like OOM seems pretty appropriate. If this is layers deep in your library and you just bubble up an io::Result saying something inscrutable, you're not helping them compared to the stack trace telling them "this call here failed".

If you're just wrapping up the API for your users, sure, bubble the error, maybe they have some weird use case and they know what that means. But if you actually need to use some external API internally and it's just done something really weird, I really don't see how bubbling out a Result that effectively says "this process might be completely hosed" is all that helpful.

1 Like

"OS and other external APIs" is still not the same as "extern fn", but of course extern fns are unsafe. However, their unsafety still has nothing to do with whether they are fallible; extern functions are unsafe because they allow to linking to external, potentially non-Rust (usually C) code. This has the potential of doing unsafe things and/or an ABI mismatch. This is true even if the code doesn't do anything related to I/O or is infallible.

You read the documentation, or/and you file a bug. You don't crash your users' code because "surely they won't want to handle it".

I don't know about the real-time system, but for a long-running webservice, catch_unwind is just perfect. You catch any panics, report and/or log the error somehow, and carry on serving more web requests. This is exactly how I use catch_unwind. Bubbling an error would also work, but there are always panics that can occur from indexing for example ( unless you avoid indexing entirely ) so you probably need the catch_unwind in any case.

1 Like

No, it's not perfect, in fact it's very far from it. It's a last-resort solution. Webservice handlers aren't supposed to panic exactly for the reason that with panic = "abort", they take down the entire server process with themselves.

Attempting to catch panics is just a best-effort, extra cautionary measure for the (fortunate) cases where panics happen to be able to be caught. It's not what you are supposed to use for general, robust error handling.

The fact that what you described is an abuse of the unwinding mechanism is proved by the official documentation of catch_unwind():

It is not recommended to use this function for a general try/catch mechanism. The Result type is more appropriate to use for functions that can fail on a regular basis.

1 Like

I regard that "recommendation" as having no rational basis.

catch_unwind works perfectly, and is perfectly suited for a web service that must continue to operate when a panic occurs during the processing of a request ( requests may be happening concurrently, so taking down the server due to a panic would be very undesirable ). In my opinion, the recommendation has no rational basis, at least for this scenario ( a web service ). It is not a "last resort", it is just a standard way for software to operate ( most high level languages have a similar mechanism, and for good reason ).

It's a last-resort solution.

It is important to distinguish between two cases:

  1. This function has situations in which it is expected to return an error (e.g. ā€œUser is not authorizedā€ or ā€œunable to connect to databaseā€).
  2. The server code has a previously-undetected bug.

Certainly case 1 should be expressed using Results. But, it is very difficult to write Rust code in which all instances of case 2 are converted to Result::Errs (because that's equivalent to ā€œI want to prove my code never panicsā€). catch_unwind addresses case 2 by replacing ā€œa bug kills the server; all requests are dropped until it restartsā€ into ā€œa bug makes the request that triggered it failā€.

Of course, unwinding from an arbitrary point still might leave the program with bad shared state, making it desirable to shut down the server. But even in those cases, catch_unwind can still help to allow a more graceful shutdown that doesn't abort other requests that could have finished.

Webservice handlers aren't supposed to panic exactly for the reason that with panic = "abort" , they take down the entire server process with themselves.

If I'm building a web service, I'm going to build my binary with panic = "unwind" precisely because it allows this recovery.

Certainly a library (that might be incorporated in arbitrary binaries) should avoid panicking whenever possible. But that's not ā€œthis is guaranteed to have no bugs that will cause panicsā€, and it's not the same as considerations for whether or not there is any value in the binary choosing to use catch_unwind().

This is designing for robustness: callee tries not to panic; caller tries to minimize the damage of panics.

Regarding the documentation:

The Result type is more appropriate to use for functions that can fail on a regular basis.

To be clear, I'm not claiming that this is wrong. I'm saying that, for a server, the failures that are not ā€œon a regular basisā€ can still be very important. A server that aborts on any panic can turn a serious but still narrow bug ā€” say, ā€œthe handler for this particular page reads a database record whose state makes it panicā€ ā€” into a total shutdown because the 1% of requests trying to load that particular page kill the server fast enough that it's practically not available for everyone else.

6 Likes

I am exclusively talking about case 1.

I don't think I said anything to the contrary. It is the right thing to do for a server to try and catch panics as a precautionary measure even if it's not 100% reliable; the thing that is wrong is individual handlers relying on this behavior and using panics for general error handling, with the tacit expectation that they will always be caught.

The thing you are still not understanding is that it doesn't "work perfectly". It's not guaranteed to work at all, so it's not suitable for desinging robust software on its own.

Anyway, it's pretty bold (and irrational!) of you to assert that the official documentation and recommendations are at fault. If you are on the highway, and everyone seems to be driving in the opposite direction, then you should suspect that in fact you are driving in the wrong direction.

most high level languages have a similar mechanism, and for good reason

But Rust is not most languages.

2 Likes

This is a false representation. panic=unwind is guaranteed to work, with specific restrictions[1]. Rustc is not at liberty to make arbitrary panics abort instead of unwind when -Cpanic=unwind is set.

The thing is, nobody else is. In fact, most people here are exclusively discussing case 2, e.g. "I called an external function and it returned a value it's documented not to return that I can't handle."

Yes, file a bug upstream if this happens. But before it happens, it's silly to suggest that I need to handle impossible but representable states because the FFI can't make the impossible states unrepresentable.

Salsa and rust-analyzer are using panic! unwinds for normal cancelation of synchronous operations.

The official policy is that unwinds should not be used for normal control flow. But actually exceptional cases, such as programmer error, a bug, or even cancellation, unwinds are fine, and the right answer. You arguing that code should never ever panic is going against the community consensus.

Your argument is the smallest step away from calling indexing panicking when given an out of bounds index improper. It is an expected failure case, after all!


  1. panics during an unwind, and maybe panics out of drops (under discussion as a potentially beneficial breaking change), will abort. ā†©ļøŽ

7 Likes

Yes, of course I try not to use Option where I need a value. In this case, I'm using this as an example of something that doesn't always return a value. For example, it can be HashMap, HashSet or something else. Especially in Rust sometimes you need Option where logically it is not needed. For example, if I want to take ownership of an object, but don't want to require Default for the value type.

By invariant, I mean invariant in a specific context, but not throughout the lifetime of the type. So I can store the value in Option, but at some specific place I can rely on the value is Some

1 Like

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.