RFC: Error conventions

[aturon]

This RFC sets out the first part of a vision for refining error handling in
Rust, by proposing a more clear-cut set of conventions around fail! and
Result. A separate follow-up RFC will propose syntactic sugar to make the
proposed convention more ergonomic.

In a nutshell, the proposal is to isolate uses of fail! to an extremely small
set of well-known methods (and assertion violations), with all other kinds of
errors going through Option/Result for maximal flexibility.

Rendered

[gulbanana]

although returning-assert() would be surprising to learners, it's worth it for one reason: there would be just two failure rules instead of 3. assert! and .assert() (as well as .assert_msg()) would be the names signalling failure (panic), which is very clear.

[bluss]

Renaming Option::unwrap is excellent.

• We need a convention for argument checking. Today libstd is keen on using assert! for this.
• There is the even tricker case of Trait invariant checking exposing failure cases to users (although sometimes not possible), see for example ExactSize::len().
• I would suggest .at() to be the checked indexing method name of choice. "at" suggests a location (index) while "get" is a very generic word. This is taken from C++.
• An alternative would be that the Indexing traits return Option<&T> and the rust glue is responsible for "unwrapping"/failing. Then xs[i] would be the failure-prone case and xs.index(&i) the Option-returning case.

[huonw]

An alternative would be that the Indexing traits return Option<&T> and the rust glue is responsible for "unwrapping"/failing

I'm somewhat in favour of this, but we are almost at a point where we don't need failure in the language, the compiler currently only inserts fail calls for indexing (which Index/IndexMut solves, modulo rust-lang/rust#12825 and possibly handling [T, .. n]) and division (which could be solved by providing unchecked division intrinsics and using the overloading traits).

However, we do have failure related lang items like begin_unwind that won't go away, so it's not so bad if we can't remove the fail_bounds_check lang item entirely.

[nielsle]

If #204 is accepted, then it would be pretty to handle "Out-of-bounds" and "key-not-found" by always returning an option (but I don't know if the compiler can optimize this, and it does make the code slightly more verbose).

let x = @vec[1i,2,3,5]; 
let _y = x[3]!;

[jfager]

I was against assert on the basis that it maintained the same ergonomic rank order as unwrap for ways to work with Option. expect does, too, so I'm still not a fan.

Between expect and assert, though, if you have to have one of those, assert strikes me as clearly better, as it gives you 'one grep to rule them all' for potentially failing (panicking) assertions. The argument that something named assert must never return a value is pretty weak - it's perhaps momentarily surprising but is trivially documentable, easy to get used to, and should always be clear from context.

[arcto]

I would still prefer unwrap_or_panic. Not only is it consistent with other unwrap_* it also does not confound with the established meaning and use of assert. I don't mind the length or the explicitness - it's not supposed to be a common operation.

[japaric]

If #204 is accepted, then it would be pretty to handle "Out-of-bounds" and "key-not-found" by always returning an option

+1 for having sugary indexing (and slicing) that returns an Option. That way we can remove convention 3, which is an exception to convention 1 ([] uses assert (or equivalent) to validate input). I understand that this exception is necessary for ergonomics (v[0].unwrap() is not sugary at all), but having the ! operator solves the ergonomics issue.

(The desugaring will need some work though, because IIRC vec[i] currently desugars to *vec.index(&i) which won't work if the index method returns an Option. After the ! sugar lands, we could make vec[i] desugar to vec.index(&i) and vec[i]! desugar to *vec.index(&i).unwrap().)

[Florob]

I think something very important that the RFC currently neglects to mention is that all functions that can currently fail, will have to return something with an #[must_use] annotation. Otherwise it becomes trivial to produce uncaught programming errors.

I'm also wondering how out of memory errors would be handled (lots of failure in e.g. HashMap is in OOM conditions). Would HashMap::swap() return a Result<Option<V>, Err>, or even Option<Option<V>>? Or is this another case for an exception to the rule?

[Ericson2314]

This looks great! "Panic" is nice and scary, "unwrap" sounded too benign for something that causes panic, and I like the convention of not using assertions to sanitize function input.

@huonw

but we are almost at a point where we don't need failure in the language...
That would be awesome. You didn't mention stack overflow, how is it handled these days?

[aturon]

@Florob You're right, functions that currently fail would need to use Result rather than Option for the return type to retain checking.

As far as out-of-memory and stack overflow errors, I believe these both abort the process currently; see this issue for example. In the future they may fail! instead. I'll add a note to the RFC to clarify.

[Ericson2314]

Ah ok.

[SiegeLord]

I don't think I agree with lumping contract/programmer errors with other errors. Firstly, unlike what is claimed by the RFC, it does not seem hard to me to cleanly define what a contract/programmer error is. Every function will have a set of pre-conditions and post-conditions that specify a contract. In terms of pre-conditions, those can be clearly documented and if a programmer violates them by passing invalid input, then the said contract is violated. Contract errors are bugs that I want to eliminate from my program. It's convenient to have them fail! early since they're easy to break on in a debugger, get a backtrace, etc. It'd be inconvenient if I accidentally used .unwrap_or to provide a default to obscure a bug.

Other errors, on the other hand, are things that I am expected to handle as part of a bug-free program operations. I want to be able to handle them easily.

Consider this realistic example. I have a graphics library function that creates a texture of a given size:

fn create_texture(w: uint, h: uint) -> Result<Texture, Error>

That function won't return a valid texture if an invalid size is passed (0 for either dimension) or if there's insufficient GPU memory (amongst other issues). Now, let's say that I know for sure my size is valid (e.g. I use integer literals), how would I assert that contract? It would be inappropriate to .unwrap() it, because it still might not succeed despite the valid size. Instead, I'd have to use a complicated match/macro to filter out some variants of Error while handling the rest using some. I don't think this is a good way to do things (and I'm not sure sugar can help this). I'd rather that function fail! for invalid sizes.

So what I'd find more convenient would be something more along the lines of the current guidelines... i.e.:

1. Use fail! when for input when pre-conditions are easy to verify by the programmer, but only when they are impossible to express using the type-system. Every triggered fail! represents a bug in the program to be fixed. I imagine many if not all slice/vector methods qualify under this category, even those that return Option today.
2. Use Option for invalid input where convenient/efficient. E.g. the validity check might be very expensive. I don't think this is a big category, but, for instance, I think ToCStr is a good example (checking valid input is in principle an expensive, O(n) operation). I'd possibly provide both fail! and non-fail! variant.
3. Use Result for failure that can happen even in a bug-free program due to outside global state (e.g. stuff dealing with system resources).

I don't find the subjectiveness of the first option vs the second option to be an issue. Convenience functions are omnipresent in APIs and this would be one more place for them.

[arcto]

I sort of agree with @SiegeLord here. "Error" is an ambiguous word. It's important not to mix up contract errors (i.e. programmer mistakes) and reasonable but unsatisfactory results.

Assertions

fail! should only be used for the former type, and in my opinion, in the form of debug_asserts. They serve no purpose in production outside of the development process because the contract has already been broken. During development you are supposed to be paranoid and check for all kinds of mistakes on your part, using these asserts and tests. You do not ship your software with asserts and tests. Assertions is a progamming/debugging aid.

The caller must be able to rely on the callee

An unexpected fail! can cause problems as the program is completing a transaction. A transaction must either complete in its entirety or have no effect whatsoever. The functions that do apply the effect must guarantee not to fail!.

The callee must be able to rely on correct usage

If you're not able, with program-flow logic, to ascertain that arguments to a function are valid you add a condition before calling. Released software should not be expected to have duplicate checks - both by the caller, and the callee - which is why assertions are debug-time only.

Type system

Ideally you want to catch as many types of incorrect arguments as possible by restricting the input-domain using the type system. This makes it impossible to provide invalid arguments and makes the error checks unnecessary. They can be safely omitted and the code will be clearer. One of the supposed goals of Rust is to eliminate as many errors as possible statically.

[liigo]

unwrap_or_fail is still the best name IMO

[SiegeLord]

They serve no purpose in production outside of the development process because the contract has already been broken.

assertions cannot be merely debug assertions, because ignoring them may lead to memory unsafety. From my point of view, bugs can never be eliminated, and assertions are a way to quickly detect the bug and bring down a program safely. If contract assertions are debug_assert! then that function should be marked unsafe (perhaps conditionally on ndebug CFG variable). I think this is the strategy for duplicate checks as well, i.e. for performance critical functions where the check is a non-trivial fraction of the entire function's runtime, provide an unsafe function that omits the check. This is done today with the unsafe_* family of functions.

An unexpected fail! can cause problems as the program is completing a transaction.

I think this is just an argument for keeping unwinding (transactions can then be finilized by destructors). If unwinding is removed, then yes... I'd agree this may be important to do.

That said, the current RFC allows for failure, and in fact will have exactly as much failure as what I propose (assuming people don't paper over contract violations with logic), so it's a general criticism. If we are to have failure at all, there is a possibility that it won't interact well with these transactions.

[thestinger]

Transactions are not supposed to the finalized during failure, they're supposed to be rolled back. Explicitly rolling back a transaction is unnecessary if the process is just exiting immediately, because that also results in it being rolled back. That's the point of a transaction after all.

[sfackler]

It's pretty trivial to handle transactions correctly with respect to failure in either an unwinding or aborting model. In an unwinding model, the transaction's associated object's destructor will explicitly roll back.

In a world where fail! aborts, there shouldn't be any issue either. The entire point of a transaction model is to guarantee consistency in the face of things like abrupt disconnection.

[aturon]

@SiegeLord Thanks for the feedback. To clarify, the difficulty isn't in defining what a function's contract is -- as you say, the documentation for a function can and should detail its preconditions and postconditions.

The difficulty is rather in giving clear guidelines for choosing contracts in API design. When should you use a precondition (and fail on violation) versus not having a precondition, and instead passing on the result of input checking via Option or Result?

For example, Vec::pop returns an Option, while indexing and slicing fail on out of bounds. The from_utf8 function returns an Option, but to_c_str fails on interior nulls.

If v is an empty vector, why is v[0] a programmer error and v.pop() not? This is a rhetorical question; it's certainly possible to give arguments justifying these choices, but they are usually not clear cut.

When I wrote the current guidelines, I tried to give crisp criteria for choosing between preconditions versus returning Result when validating input. But it's very difficult, and the guidelines I gave have not been able to resolve disputes about specific APIs because it's possible to argue in both directions.

A couple of other points:

• The debugging help from fail! still applies with the proposed conventions, if or when the caller asserts that it has met the contract by using something like unwrap. I'd argue that this is usually the more helpful place to flag the error anyway, since it was the caller that provided the bad inputs.
• The example of create_texture is interesting, but I'm not sure I buy it. Robust code would not be unwraping the Result in this case, so you've already bought into a match or some other handling. For the Error variants corresponding to bad inputs (which could be given as a wildcard pattern), you can fail! in the client. Admittedly, that's one extra line of code on the client side. But the upshot is that the return type has alerted the client to the input validation; it's harder to ignore than comments about a precondition.

[SiegeLord]

My example was illustrating that these guidelines if followed to the letter will lead to complicated caller code that is not amenable to syntax sugar. The point is that if I encode the contract violation into the Result you will get caller code looking like this:

let tex = match create_texture(x, y) {
    Ok(tex) => tex,
    Err(ContractError) => fail!("{}", ContractError);
    Err(MemoryError) => return Err(MemoryError);
};

while if the contract failure was put inside the function, the code could be simplified:

let tex = try!(create_texture(x, y));

or, to get more fancy:

let tex = try!(create_texture(x, y).map_err(|_| create_texture(x / 2, y / 2));

It's not just 'add one more line to the match statement', it's actually 'opt-out of the macro/API sugar associated with Result'. Notably, using try! in the first example (that uses the new guidelines) is wrong because you are bubbling up a bug.

I disagree the that debugability is not an issue with the proposed guidelines. The complete call stack is a super-set of what you obtain by cutting it off at the place where you supply the inputs. Unless the Result encodes a very fine-grained reason for the failure (perhaps including the actual source position) some useful information will invariably be lost. If this is truly what this RFC is getting at, it needs to be spelled out.

When I wrote the current guidelines, I tried to give crisp criteria for choosing between preconditions versus returning Result when validating input. But it's very difficult, and the guidelines I gave have not been able to resolve disputes about specific APIs because it's possible to argue in both directions.

I think creating crisp guidelines that resolve any API design questions is a pipe dream. The goal of guidelines should be to write robust, bug-free software using clean-looking code. These new guidelines seem to encourage hiding bugs by makng the robust code look unclean. I recently switched some of my code to use callee-failure for obvious bugs, and I found my code to only benefit from it (as far as I can tell, at least).

[aturon]

@SiegeLord Thanks for the clarification, and sorry for the delay in getting back to you. I think you've raised a significant drawback for the proposal, and agree that clean and robust client-side code trumps having a rigid guideline.

I'm going to close this RFC in favor of a new one I've just opened. I think the guidelines in the new RFC are pretty well-aligned with what you proposed in an earlier comment.