Discuss use of unwrap

[ah801]

Should .unwrap() be allowed anywhere in safety critical code ?

If so, under what circumstances?

If not this would seem ideal as a first rule/guideline and something that could be enforced by the tooling quite easily.

[jonasthewolf]

unwrap is perfectly fine if there is a check beforehand.

There are many safety-related systems where an abort or panic is perfectly safe (though uncomfortable).

If panicing is unsafe, you could still use unwrap with a check that unwrap actually does not panic.

For my spare time projects (not safety-related at all), I typically put a comment to an unrwap explaining why it is not panicing.

[ah801]

In a safety critical software, the loss of a thread through a panic due to not checking before doing an unwrap, could be catastrophic to that safety critical software component as a whole.
The fact that it could be catastrophic would lead me to believe that there should be a rule or guideline in place, that is enforceable by the tooling rather than leaving it up to the developer to not make the mistake.

[vjonaswolf]

There are good use-cases for using unwrap(). See e.g. the first example in https://doc.rust-lang.org/book/ch09-03-to-panic-or-not-to-panic.html for one. More generalized: I don't see, e.g., a problem with unwrapping static/const expressions.

[PLeVasseur]

While somewhat tangential, I found the article by Niko on Unwind considered harmful? and the discussion around the article quite interesting.

It would appear even in mission-critical (not safety-critical) applications that sometimes it's best to have another warmed process in the same state to fail over to if truly in a challenging to recover state.

From that point of view, I think unwrap() (and expect()) can serve as a useful model of how we could write guidelines which note that they are generally speaking not great to reach for in "production code", but they have their legitimate use cases.

[darkwisebear]

I think that we need rules for all situations where panics could happen because it forces developers to think whether a panic is the appropriate thing to do in a particular situation. If panicking is (in that context and situation) safe, it's absolute legit to do so, and sometimes, when facing a locally unrecoverable error, it's actually often a good thing to do to stay in a safe state.

As already mentioned, we should not just cover unwrap() but any call that potentially could panic: Just like with unsafe, we would need rules that cover two aspects:

• On callee side, document when a function is potentially panicking and when this will happen
• On caller side, document why these conditions aren't met so that the panic won't happen or state why panicking is fine - maybe also with a general statement on a particular system part that panicking / crashing is - from a safety perspective - fine and the system can handle it, e.g. by having appropriate fallback mechanisms in place.

[PLeVasseur]

@darkwisebear -- That seems like reasonable high-level choose-your-own-adventure style guidance. If fleshed out this could make for a useful contribution to the coding guidelines subcommittee.

[ah801]

Generally in safety critical systems that I've worked on, there is a 2nd process ready to pick up the pieces and recover if the primary application fails. We may have potential unrecoverable situations for the primary application which force the secondary application to take action. But in many cases they are recoverable and it is usually not acceptable to have the safety critical software crash/exit where it could of continued and met all it's safety requirements.

However while all this is useful for how to design safety critical software, at the end of the day we need rules/guidelines that are enforceable by (an) automated tool(s).

And if that tool flags up an issue, with say the use of unwrap() where the developer believes they know better, then it is acceptable with a peer review to override the tool's flagged issue. And as mentioned above by @darkwisebear, document the fact as to why it's not a problem.

And as mentioned by @PLeVasseur, "not great to reach for in "production code", but they have their legitimate use cases.".
I.e. if we could have called:
if let Ok(value) or match rather than .unwrap()
then this should be flagged up a a potential issue by the tool enforcing the guideline.

[adria0pol]

A useful alternative to using .unwrap() is to call .expect(...) with a clear explanation of why it’s safe. Parity Technologies often follows this pattern, and it’s worth considering in critical systems.

Example from their codebase:
https://github.com/openethereum/parity-ethereum/blob/55c90d4016505317034e3e98f699af07f5404b63/ipfs/src/lib.rs#L157

[darkwisebear]

A useful alternative to using .unwrap() is to call .expect(...) with a clear explanation of why it’s safe. Parity Technologies often follows this pattern, and it’s worth considering in critical systems.

I think the concern is more about the theoretical existence of panics. Even if you use expect, there is still a codepath that leads to a panic in case the compiler cannot determine whether this code path is used or not. And if then either the reasoning or a precondition isn't upheld for whatever reason, you might lose the thread, which in some safety critical systems might be a safety hazard.