Why aren't we allowed to implement unsafe trait method with safe one?

Basically, trait impls are allowed to reduce the constraints on a method. For example, lifetimes:

trait Trait<'a> {
    fn f(&self, arg: &'a str);
}

struct S;
impl<'a> Trait<'a> for S {
    fn f(&self, arg: &str) {}
}

Playground

The reason is obvious: I (the trait implementer) know that I don't need those constraints, the user can't violate safety (because if calling via the trait the constraints are still required), and everyone is happy.

Unfortunately, the same doesn't apply to unsafety:

trait Trait {
    unsafe fn f(&self);
}

struct S;
impl Trait for S {
    fn f(&self) {}
}

Fails with:

error[E0053]: method `f` has an incompatible type for trait
 --> src/main.rs:9:5
  |
4 |     unsafe fn f(&self);
  |     ------------------- type in trait
...
9 |     fn f(&self) {}
  |     ^^^^^^^^^^^ expected unsafe fn, found normal fn
  |
  = note: expected fn pointer `unsafe fn(&S)`
             found fn pointer `fn(&S)`

Playground

Is there a reason, is this an intentional design choice?

1 Like

You're looking for https://github.com/rust-lang/rfcs/pull/2316

The unresolved issue for it is the interaction between semver and what the caller can use -- can safe code call S.f()? That seems nice, but it's also a semver trap because it means it's a breaking change to add unsafe to the impl even though the trait said the method is unsafe. That's particularly troublesome because the safety promise is "invisible", so it'd be really easy to just do fn f(&self) { unimplemented!() } without thinking about whether you really meant to promise it. (The same thing for const is less bad, because you'd have to const fn f(&self) visibly to make a "it'll stay const" promise.)

Thanks for bringing this up, I'm supposed to have left a comment on that RFC

1 Like

I've thought about that, but adding unsafe to any method is a breaking change. If I have,

fn f() {}

changing it to

unsafe fn f() {}

is such a breaking change. The fact that here the trait's method is unsafe doesn't matter.

Maybe add a safe keyword as a promise that this method will never be unsafe:

trait Foo {
    unsafe fn f();
}

impl Foo for Baz {
    safe fn f() {}
}

I swear I have seen people (intentionally ab-)use unsafe fn with a default implementation in a trait to make sure implementors donʼt break assumptions about that methodʼs behavior (while still allowing for people who care about performance and dare to use unsafe to optimize the implementation when possible). Lifting the need for using the keyword unsafe would break the soundness of existing APIs like that.

I would have to search for a while to find out where exactly Iʼve seen this.

1 Like

People will still can do that, what's the problem?

Tokio’s AsyncRead::prepare_uninitialized_buffer is an example of a “safe to call, unsafe to implement” trait method.

Because it would let third-party implementers of the trait create UB without any unsafe code.

1 Like

I think that's just incorrect. The way to represent that something depends on an unchecked property of a trait implementation is to use unsafe trait.

4 Likes

Yep, that's the one I was remembering, thanks ^^

I think you would need specialization to provide a safe to leave unimplemented/default-implemented and unsafe to override kind of interface without this trick though. Which makes it a bit more legitimate in my view.

unsafe trait MyUnsafeToImplTrait {
    fn has_to_be_correctly_implemented (...);

    fn can_be_implemented_as_you_wish (...);
}
trait UseDefaultImpl {
    fn can_be_implemented_as_you_wish (...);
}
unsafe
    impl<T : UseDefaultImpl> MyUnsafeToImplTrait for T {
        fn can_be_implemented_as_you_wish (...) {
            UseDefaultImpl::can_be_implemented_as_you_wish(...)
        }

        fn has_to_be_correctly_implemented (...)
        {
            /* Default implementation here */
        }
    }

So that people can either unsafe impl MyUnsafeToImplTrait, or impl UseDefaultImpl, and in the latter case their unability to override has_to_be_correctly_implemented is what makes the pattern sound.

The only issue without specialization is that the above pattern may struggle with blanket impls involving unsafe impl MyUnsafeToImplTrait and coherence.

9 Likes

Oh, that's an interesting hack. I agree that in principle this is unsound; consumers of safe traits have to be able to handle every possible implementations of the trait methods with safe code. That's what unsafe trait is for.

Now that such code exists I guess it will be up to the lang team to at some point decide if such (ab)use of unsafe fn-in-trait

Really, unsafe fn in a safe trait makes no sense as such functions come with extra trait-documented preconditions (that's why they are unsafe to begin with) and you need unsafe trait because clients need to promise that these preconditions actually are sufficient to justify executing their code. See for example the SliceIndex trait. So probably unsafe fn in safe trait should not have been allowed in the first place...

3 Likes

I don't think this is correct. Consider this toy example:

pub trait SliceSetIndex<T> {
    fn set(self, slice: &[T], value: T);
    unsafe fn set_unchecked(self, slice: &[T], value: T);
}

In the std SliceIndex trait

/// Implementations of this trait have to promise that if the argument
/// to `get_(mut_)unchecked` is a safe reference, then so is the result.

But my SliceSetIndex doesn't have any such conditions it needs to uphold, so it doesn't need to promise anything beyond "not violating memory safety itself", and thus doesn't need to be unsafe trait. It would be sound to implement it as

impl<T, U: SliceIndex<T>> SliceSetIndex<T> for U {
    fn set(self, slice: &[T], value: T) {
        slice[self] = value;
    }
    unsafe fn set_unchecked(self, slice: &[T], value: T) {
        unsafe { *slice.get_unchecked(self) = value; }
    }
}

which needs to be unsafe fn because it depends on the precondition that the index is valid. It would also be sound to implement this trait as

pub struct NoIndex;
impl<T> SliceSetIndex<T> for NoIndex {
    fn set(self, slice: &[T], value: T) {}
    fn set_unchecked(self, slice: &[T], value: T) {}
}

which doesn't need to be unsafe fn.

3 Likes

But then why is set_unchecked an unsafe fn to begin with?

unsafe fn, by definition, means "this function has extra preconditions". In a safe trait, what this basically means it "it has arbitrary unknown extra preconditions", because you cannot impose constraints on the impl. This means the function may never be called as you do not know its preconditions!

So what if I now write an impl that depends on the precondition that the index is even? This is a correct impl, it does not violate the trait contract as there is no trait contract. Only unsafe trait can have (safety-critical) contracts.

You are implicitly making the assumption that every implementation of your trait has a precondition that is implied by "the index is valid", exactly like what SliceIndex in libcore does. This is a requirement very much like that of TrustedLen, and thus needs unsafe trait. IOW, your trait really needs a comment like

/// Implementations of this trait have to promise that if the self argument
/// to `set_unchecked` is a valid index, then the function is safe to call.

Could the trait not define the preconditions required to call the function, thereby providing those conditions (and only those conditions) to implementations (which could then also choose to relax them)

trait Foo {
  /// Safety: The provided byte must be <128
  unsafe fn with_ascii(byte: u8) -> String;
}

// Fully safe implementation
impl Foo for () {
  /// This implementation relaxes the preconditions required by `Foo
  fn with_ascii(byte: u8) -> String {
    String::from_utf8(vec![byte]).unwrap()
  }
}

// Implementation with unsafe body
impl Foo for u8 {
  /// Safety: See requirements imposed by `Foo`
  unsafe fn with_ascii(byte: u8) -> String {
    /// Safety: calling this function requires `byte` to be < 128
    unsafe {
      String::from_utf8_unchecked(vec![byte])
    }
  }
}

(Though, this doesn't fit with the previous examples of traits that attempt to apply postconditions via unsafe fn).

To me that's exactly like a trait saying "this len function must return precisely the actual length of the iterator". If you rely on such things for safety, it needs to be an unsafe trait.

Your trait docs define a "lower bound" on implementation behavior ("must be safe to call with byte < 128") that is not reflected in the type signature. This is analogous to defining a bound like "must return an even integer", that is not reflected in the type signature either, and that requires unsafe trait.

Isn't the lower bound on implementation just the type signature "must be safe to call with a u8"? Only by then adding unsafe in to the implementation with a reference back to the preconditions that have been provided to the function are you allowed to assume more than that, at most assuming that byte < 128.

Without the implementation making any assumptions there should be no possibility for it to cause UB by supporting input that violates the required pre-conditions, since that input cannot soundly be passed in through the trait interface.

1 Like

This whole "the trait must be unsafe" debate is just a philosophical one, where both sides have valid points. It's like trying to debate tabs vs spaces.

In the example above, Foo::with_ascii needs to be unsafe because it's unsafe to call. Callers need to uphold the invariant when calling the function. Not doing so may lead to UB.

Whether or not the trait also needs to be unsafe is just a matter of personal opinion. Some might argue that making the trait unsafe is just redundant. Others might argue that it is useful and signals that the trait has certain requirements as a whole that need to be fulfilled. Really, ya'll are both right.

1 Like

Because it requires the precondition that the index is in-bounds in the slice. That precondition would be defined in the trait docs; any code that calls the trait method must use an unsafe block to promise that it meets the condition. (I should've explicitly written that documentation down when I wrote the example in the first place; my bad.)

EDIT: Here's a revised trait signature, borrowing the method documentation wording from std's get_unchecked functions:

/// A trait for setting the value of a slice at a particular index.
///
/// (But if implementations don't actually do that, that's just a logic error.
/// Consumers of this trait aren't allowed to rely on that for memory safety.)
pub trait SliceSetIndex<T> {
    /// Calling this method with an out-of-bounds index is *undefined behavior*.
    unsafe fn set_unchecked(self, slice: &[T], value: T);
}

So, suppose your impl looks like this:

pub struct EvenOnly;
impl<T> SliceSetIndex<T> for EvenOnly {
    fn set_unchecked(self, slice: &[T], value: T) {
        if !self.is_even() {
            unsafe { immediate_ub(); }
        }
    }
}

As you say, this doesn't violate a trait contract because the trait does not impose any requirement on the impl. But it doesn't need to. This code violates the unsafe rules in general – you've written an unsafe block to use an unsafe operation where you cannot guarantee that the operation is not UB.

On the other hand, in my original impl

impl<T, U: SliceIndex<T>> SliceSetIndex<T> for U {
    unsafe fn set_unchecked(self, slice: &[T], value: T) {
        unsafe { *slice.get_unchecked(self) = value; }
    }
}

This impl is permitted. It relies on (1) the promise by the caller, arising from the SliceSetIndex::set_unchecked precondition, that self is in-bounds in the slice, and (2) the promise from std::slice::SliceIndex that if self is in-bounds in the slice, then the returned reference is valid. Because of those 2 promises, this impl can guarantee that it does not violate memory safety (violations can only arise from the caller violating the SliceSetIndex::set_unchecked precondition).

Like any trait impl, it is permitted to relax the precondition from the trait, and not permitted to tighten the precondition from the trait; the only difference here is that the precondition is defined by the documentation rather than the type system. Requiring the index to be even would be a tightening of the precondition, which is not permitted.

Edited to add:

The meaning of unsafe trait is that the trait promises that consumers of the trait can rely on some additional guarantee, beyond the normal guarantees of "methods are safe to call when their preconditions are met". std::slice::SliceIndex and TrustedLen both have such additional guarantees – consumers of TrustedLen are permitted to write unsafe code that relies on the len being accurate, and consumers of SliceIndex are permitted to rely on get_unchecked not only being safe to call, but returning a safe reference. (Note that since get_unchecked returns *const Output rather than &Output, implementations would be permitted to return an invalid pointer if SliceIndex didn't impose additional restrictions on them by being an unsafe trait). With my SliceSetIndex trait, a consumer of the trait can't rely on any additional behavior other than the methods being safe to call.

5 Likes

Okay so the way you see it, when there is an unsafe fn in a trait, even a safe one, and there is a safety precondition specified in the trait docs, then this is a proof obligation for every user of the trait that this condition is upheld, and thus every implementation of the trait can rely on it? Just like otherwise every implementation can rely on the user supplying well-typed arguments?

That makes a lot of sense. I will have to think about it some more, but I cannot immediately see anything wrong with this approach. Basically, in a trait, unsafe fn lets you specify extra preconditions, and unsafe trait lets you specify extra postconditions. Both make sense individually and when combined. The SliceIndex trait actually needs both, thus we have an unsafe fn in an unsafe trait. (This is just rephrasing what you said in my own words to make sure I understand.)

I do not think this is just a matter of opinion, actually. Strengthening the precondition and strengthening the postcondition are distinct actions (with opposite polarity!), so it actually is quite nice to see them both reflected in the unsafe story. I just had not realized that because the example I considered strengthens both precondition and postcondition.

Curious how you can think about unsafe Rust for years and still learn new things about it. :wink: The docs are much better than they used to be, but this discussion is not reflected in them.

5 Likes