Methods that can fail (panic) should be marked as unsafe

[sergei-kucher]

Some of core methods using fail! macros. Like expect and unwrap of core::option::Option and core::result::Result.

I found two primary things about this:

• RFC: rename fail! to panic! (issue)
• Common container/wrapper methods [RFC] (issue, issue)

Seems like some of fail (panic) methods will be preserved (like expect for a Option<V>). But using of such methods is highly error-prone.

To prevent mass usage of them I suggest to mark such methods as unsafe.

For example:

unsafe fn expect(self, msg: &str) -> T

Or maybe generate some special type of warning on compile.

[emberian]

These can't cause memory unsafety, there's no reason for these to be unsafe. Partiality isn't a bad thing, necessarily.

[reem]

Agree with @cmr. unsafe does not mean discouraged, it means unsafe.

[sergei-kucher]

@cmr, @reem, and @ALL

In wider meaning 'unsafe' indicate any operation that can emergently stop current task. For single-task programs it's also will stop whole proccess. I'm sure, in most of cases, such behaviour will be unexpected and undesireable.

But if rust have so strict meaning for unsafe keyword I don't insist on that. I think will be enough if rust produce compile-time error on macros fail! (panic!). Of course we need attribute to allow that macros (something like #[allow(panic)]) that can be placed on function.

Anyway we definitely need good solution to prevent rust code (especially libraries) from being highly error-prone.

At present, very sensitive parts of standard library predisposed to errors. For example trait std::ops::Index that implemented by std::vec::Vec have following declaration:

pub trait Index<Index, Result> {
    /// The method for the indexing (`Foo[Bar]`) operation
    fn index<'a>(&'a self, index: &Index) -> &'a Result;
}

Vec<T> implemetation is:

fn index<'a>(&'a self, index: &uint) -> &'a T {
    self.get(*index)
}
pub fn get<'a>(&'a self, index: uint) -> &'a T {
    &self.as_slice()[index]
}

Expression let value = vector[index]; will crush whole task in case index is out of vector bounds. To write safe code I must do bounds check. LIke this:

if index < 0 || index >= vector.len() { return false; }
let value = vector[index];

Except for it's excess work for a programmer and ugly. I forced to make redundant operation to prevent task crush. But rust supposed to be fast. Isn't it?

I seeing such situation totally wrong. Moreover actually it is inconsistancy with other vector methods. For example Vec<T> have implementation of following methods:

fn last(&'a self) -> Option<&'a T>;
/// Returns a reference to the last element of a vector, or None if it is empty

fn last_mut(&'a mut self) -> Option<&'a mut T>;
/// Returns a mutable reference to the last element of a vector, or None if it is empty

fn swap_remove(&mut self, index: uint) -> Option<T>
/// Removes an element from anywhere in the vector and return it, replacing it with the last element.
/// This does not preserve ordering, but is O(1).
/// Returns None if index is out of bounds.

In contrast to described above, those methods really safe and do not force to make excess checks. Furthermore programmer must deside that to do in case of None result. As consequence is no space for an accidental errors here.

Therefore std::ops::Index trait must be overwritten like this:

pub trait Index<Index, Result> {
    /// The method for the indexing (`Foo[Bar]`) operation
    fn index<'a>(&'a self, index: &Index) -> &'a Option<Result>;
}

But this example just subcase of big problem with unexpected fail! (panic!)

So my proposition is following:

• Make RFC where forbid unnecessary usage of fail! (panic!) macros in preference to Option<T> or Result<T> function results.
• In order to make sure of this, produce compiler error for a function with that macros.
• Create special attribute to allow such functions.
• Refactor standard library in compliance with RFC.

I'm sure we must do it before version 1.0.

Otherwise we have a risk of Rust will be not exceptional (fast and reliable), but just another ordinary programming language.

[pnkfelix]

@sergey-kucher I think you misunderstand some of the fundamental design goals and criteria for Rust.

Some of the things you describe are already underway, in terms of trying to revise the standard API's to encourage use of Option/Result rather than fail!. I believe some of the examples you cited, like Vector::get (which I should note is deprecated), are instances of places where we simply have not yet revised those API's to follow the rules.

But the simple truth is that fail!/panic! is not something we are striving to eliminate or avoid entirely, at least not with Rust as currently envisioned. Safe code that invokes fail!/panic! is still better than unsafe code, because it will unwind the task state cleanly, rather than venturing into undefined behavior.

If we required unsafe blocks around every place where fail!/panic! could occur, then we would need unsafe in far more places, and that would not be an improvement, because then we would be conflating the cases that have been tagged as unsafe due to potential panic! with the places that are tagged as unsafe due to actual potential memory unsafe operations. The latter cases are the ones that we care about explicitly tagging.

[Gankra]

Task panics are for when the program has entered an inconsistent state. There is no correct way for the program to proceed, so it simply panics and takes out the whole task (whether that's enough is a different issue).

They are for asserting that the program is correct, and as such provide greater safety than the same method without the panic. Usage of panic!/assert! should be encouraged whenever a programmer is making an assumption that they can't fully justify at the time of writing, or to guard against inappropriate usage (particularly, on internal methods to guard against refactoring errors). If need be, they can use debug_assert! to remove any production expense.

Of course, as @pnkfelix notes, most APIs that fail on simply bad user input should yield an option/result, or have an alternative that yields an option/result (which is the case with indexing, but we're in the middle of some major refactoring there, so the way to do that might not be intuitive).

For the record to get from a Vec in a non-failing way, you have to do vec[].get(i) (which coerces it to a slice, which has the appropriate API).

[sergei-kucher]

@pnkfelix @gankro Thanks for your response.

Some of the things you describe are already underway, in terms of trying to revise the standard API's to encourage use of Option/Result rather than fail!

I'm glad to hear this. Because I'm really was worried about API. Now I can calm a little bit.

I agree fail! / panic! is better than unsafe code. You convinse me to refuse my suggestion about unsafe keyword.

But I'm still sure fail!/panic! must not be used without strong urgency (like @gankro mentioned). And I don't know better way to prevent this than produce compiler error. Or at least warning (because error is apparently too much).

But maybe this is not so big proglem than it seems.

[pnkfelix]

@sergey-kucher you can probably write a lint that detects whether some code locally expands into a call to fail!/panic!. (You'd need an effect system to determine whether code that you call can cause a panic, which is not something we are planning to provide ourselves any time soon, beyond the naive effect system that you get with unsafe.)

Since the compiler supports dynamically loaded lints via plugins, I would suggest you go down that route, rather than try to convince the team that this is a necessary addition to the rust compiler as distributed.

[sergei-kucher]

you can probably write a lint that detects whether some code locally expands into a call to fail!/panic!

I'll think about it. Thanks

[brendanzab]

Yeah, I think that abusing unsafe as a crude form of effect system would be a mistake.

[pczarn]

FWIW: an effect similar to the opposite of this one is called nounwind in LLVM.

[brendanzab]

@huonw has a good post on what unsafe means.

[o01eg]

May be use panic attribute instead of unsafe one for panic! macro itself and panicing function. So it allow to forbid compilation of troublesome code:
rust-lang/rust#14875
rust-lang/rust#16135