Add SafeDeref/SafeDerefMut

[tomaka]

Rendered

I'm not a fanatic of this change, but I think it is necessary. I'd understand if it was closed, but in my opinion this is the least bad thing to do.

[SimonSapin]

Types that implement this trait are guaranteed to always return the same object

What does "same" mean? Does it mean that the returned reference, if coerced to a pointer, stays at the same address? Is it the same as https://docs.rs/owning_ref/*/owning_ref/trait.StableAddress.html ?

[tomaka]

What does "same" mean? Does it mean that the returned reference, if coerced to a pointer, stays at the same address? Is it the same as https://docs.rs/owning_ref/*/owning_ref/trait.StableAddress.html ?

No, it's not about staying at the same memory location.

It means that deref can return the object as it originally was, or the object after it has been moved, or the object after it has been mutated through its API.

The consequence is that if an object doesn't provide any way (after initialization) to mutate one of its fields, then the value of that field in the object returned by deref must always stay the same.

[eddyb]

cc @arielb1 @ubsan @nikomatsakis

[oli-obk]

Another alternative is to add a lint that checks Deref impls and whether they either end up giving a static offset into the struct itself, or if the impl somehow generates an address from an unsafe pointer, whether there are any safe &mut self functions that modify that pointer themselves or through other function calls.

After some time, make that lint a deny-by-default lint.

Not sure if all valid Deref impls can be proven valid by such a lint, but I can't think off the top of my head of a good impl that is disallowed by such a lint.

[sfackler]

We have used the Trusted prefix before, I think for TrustedExactSize maybe?

[burdges]

Is this an issue for AsRef/AsMut and Borrow/BorrowMut too? Is there no way to express this with an inline directive on <&T as Deref>::Deref::deref?

[tomaka]

Is this an issue for AsRef/AsMut and Borrow/BorrowMut too?

The issue that this RFC is trying to fix arises only in the situation described in the "motivation" example, and I think that in this situation Deref/DerefMut is the appropriate trait.
Therefore I'd say no.

[sfackler]

Is a separate SafeDerefMut trait necessary? It seems like we don't necessarily need to distinguish since Deref and DerefMut impls that behave that differently also seems like a thing that we'd want to prohibit with such a trait.

[tomaka]

Good point!

[eddyb]

To be fair, the pattern violating this would be Cow.

[sfackler]

Ah yeah, Cow is a good counter-example.

[SimonSapin]

Is Cow really different here from a Vec that has been pushed/extended beyond its previous capacity between two Deref calls?

[eddyb]

Impure DerefMut.

[burdges]

I'm confused. Cow does not satisfy DerefMut and its Deref method looks pure.

[eddyb]

Ah right, I forgot the types differ so it can't use DerefMut - for some reason I thought it did.

[strega-nil]

I would rather call it DerefPure, and have it be used for box, &, and &mut patterns (see #1646). This seems like a nice extension property, but not the core reason I'd want this.

[glaebhoerl]

Could we potentially have a general mechanism for this "trusted trait" thing (which has come up for other traits before, like Iterator and Hash and Ord)? E.g., as a strawman: if MyTrait is a trait, then unsafe MyTrait is also a trait - an unsafe trait which requires an unsafe impl? With the general meaning that "if the impl violates the contract set out in the documentation of MyTrait, it is undefined behavior".

I don't really want to encourage the use of these - as (I think it was @nikomatsakis?) put it back when "trusted Eq/Ord" traits were being discussed, the actual goal of Rust is to minimize the number of security vulnerabilities, not just to be able to say "well it's your fault you used unsafe" when they happen - but on the other hand, if we're going to have multiple specific instances of the pattern, we might as well have a general mechanism for it. (The situation is kind similar to "OIBITs" in this respect.)

[burdges]

Update: This comment can be ignored as it was based on my miss-understanding of the RFC.

There is always a contract that an impl satisfies the requirements specified by the trait. An unsafe trait indicates that not satisfying them might result in undefined behavior. I believe your unsafe MyTrait have different requirements from MyTrait, so using the same name sounds mildly problematic.

There is still no answer to whether this can be fixed with an inline directive, like :

impl<'a, T: ?Sized> Deref for &'a T {
    type Target = T;

    #[inline(always)]
    fn deref(&self) -> &T { *self }
}

In any case, I think one should not necessarily expect to solve optimization problems like this in the type system. We write #[inline] fn not inline fn because we do not want inlining decisions to me made in the type system. Just imagine if we needed specialization before inlining worked right! ;)

If a judicious #[inline] cannot solve this, then would an attribute #[pure] to express that a function should depend only upon its arguments work?

impl<'a, T: ?Sized> Deref for &'a T {
    type Target = T;

    #[pure]
    fn deref(&self) -> &T { *self }
}

In this case, I think *self merely turns an &&T into a &T, and cannot invoke deref coercions, so all should be well.

Now if *self here invoked deref coercions, then this hypothetical #[pure] attribute might invoke some non-#[pure] Deref::deref, which might not make sense. If that is the case, or something similar, then maybe that should be explained as the reason to do this in the type system as opposed to with more ad hoc optimization tricks, attributes, etc. It's not so surprising if deref coercions require special treatment in places, as they are are a rather unique feature of Deref.

Addendum: Just to be clear, one could imagine a pure fn which puts the issue back into the type system, but #[pure] fn could work similarly to #[inline] in that it itself should be pure, but actual stability gets pushed onto the method of its types parameters.

Edit: I've replaced #[stable] with #[pure] above as that more closely resembles terminology used in previous RFCs.

[burdges]

If I understand correctly, it sounds like this RFC's use case actually hangs on these two impls :

unsafe impl<'a, T: ?Sized> SafeDeref for &'a T { }
unsafe impl<'a, T: ?Sized> SafeDerefMut for &'a T { }

And I think their correctness actually hangs on the same issue of deref coercions not occurring inside Deref::deref anyways.

In other words, if an attribute based solution fails here, then I think the extra trait approach proposed by this RFC should fail too, so one needs to either modify the Deref trait itself, or else make SafeDeref/SafeDerefMut unrelated traits, possibly based on trait fields #1546.

[glaebhoerl]

There is always a contract that an impl satisfies the requirements specified by the trait.

Yes, but the stakes are 'merely correctness', not safety (UB), so unsafe code can't rely on them being satisfied. I think we both know this.

An unsafe trait indicates that not satisfying them might result in undefined behavior.

Yes.

I believe your unsafe MyTrait have different requirements from MyTrait

No, it just raises the stakes of not satisfying them from incorrect behavior to UB. The idea is just that instead of introducing new TrustedEq and TrustedOrd and SafeDeref traits and so on, we would simply use unsafe Eq and unsafe Ord and unsafe Deref to mean the same thing. (N.B.: This is still strawman syntax for the purpose of discussion.)

You're right (if that's what you meant to say) that this wouldn't by itself be sufficient for the SafeDeref use case in this RFC: because (as far as I can tell) right now Deref has no contracts at all!

So it would be a two-part solution:

1. Add to Deref's documentation that impls should always return the same object. I think doing this is completely justified on its own merits: most people probably intuitively expect this to be the case, and the possibility that it's not guaranteed might not even occur to them. So impls which don't satisfy this can't really be considered "well-behaved" IMHO. (Adding this requirement might be considered a breaking change in some abstract sense, but we've already done something vaguely similar with Copy clone semantics #1521.)

2. Use unsafe Deref where the safety of unsafe code depends on the given impl being well-behaved in the aforementioned way. (Note that like any unsafe trait, implementors of unsafe Deref would have to use unsafe impl to opt in to it.)

1. by itself is not enough because it doesn't let unsafe code rely on the guarantee, and 2. by itself would not be enough because Deref doesn't currently specify any guarantee it could rely on, but together they would be sufficient.

[burdges]

You mean the compiler should know that unsafe Deref means the deref method should always return the same object when called with the same arguments. If we look as say std::net::TcpListener then I'd think say local_addr might benefit from this optimization too, but bind should never be considered "stable" "pure" in that sense, so I take it that interpretation would be special to Deref.

What do you mean by "raises the stakes" in general though? Could one write a bound where T: unsafe Trait to call dangerously optimized methods? Could unsafe Trait overload methods of Trait with dangerously optimized variants? All these options sound complicated. I'd worry that if you must deploy them for a function as simple as <&T as Deref>::deref then you're missing more directly optimization oriented approaches like improving inlining, expressing that functions lack side effects, etc.

[glaebhoerl]

Maybe I've been misunderstanding, but I think this RFC isn't about what the compiler/optimizer may assume, but about what someone writing unsafe code may assume?

[burdges]

You're right. I missunderstood the RFC. Oops! The RFC wants a bound P: SafeDeref.

Could this be done without any unsafety by combining trait fields with specialization?

pub trait DerefField : Deref {
    _deref: Self::Target
}

impl<'a, T: ?Sized> DerefField for &'a T {
    _deref: *self
}
impl<'a, T: ?Sized> DerefField for &'a mut T {
    _deref: *self
}

// Use DerefField for Deref it exists
impl<P: ?Sized+DerefField> Deref for P {
    // We cannot define P::Target here, but it must exist since DerefField: Deref
    default(std) fn deref(&self) -> &P::Target { &self._deref }  // Disallow overloading outside std
}
impl<P: ?Sized+DerefField+P::'ref> DerefMut for P {
    default(std) fn deref_mut(&self) -> &mut P::Target { &self._deref }
}

// Finalize Deref/DerefMut to use DerefField
impl<'a, T: ?Sized> Deref for &'a T {  type Target = T;  }
impl<'a, T: ?Sized> Deref for &'a mut T {  type Target = T;  }
impl<'a, T: ?Sized> DerefMut for &'a mut T {  }

As written, the trait field _deref refers to a value outside self, which might be problematic. You can rephrase it in terms of a pair DerefField and DerefFieldMut whose fields are set only to self itself, but doing so requires an associated lifetime.

In any case, we want two default(std) fns that prohibit further specialization outside std, so almost like using specialization backwards.

Ain't clear if there is even a breaking change here. It'd makes implementing Deref properly require three traits instead of two, but reduces the boilerplate inside those two.

[glaebhoerl]

@burdges @tomaka Here is a crate which defines its own unsafe version of AsMut because it wants the analogous guarantees: https://docs.rs/atomic_ring_buffer/*/atomic_ring_buffer/trait.InvariantAsMut.html (cc @whitequark)

[burdges]

There are likely drop methods calling the remove<Q>, contains<Q>, etc. method of HashMap<K,V> with Q: Borrow<K> and K generic too. And Hash<T> requires more subtle invariants.

I wonder if all these traits should just be more explicit about the invariants that they require, while if you violate them then we judge that undefined behavior to be acceptable, or even intentional, as if you use a random number generator.

[tomaka]

So should I close this PR as a duplicate of #1646?

[burdges]

I'll post a partial summary over there in case you do.

Cow is potentially a counter example for my idea of a DerefField trait being a safe substitute for DerefPure or SafeDeref. I think DerefField could work for Cow if trait fields worked for enums and *Field variants of ToOwned and Borrow were added, but we'd wind up adding 7+ traits to std.

I'm sympathetic to @glaebhoerl idea that any trait could've an unsafe variant, but it'd kinda make grep unsafe *.rs less useful. I wonder if the logic could not be reversed where a typical impl Deref meant that Deref was stable, but you could do an unsafe impl<..> Deref for .. when you wanted to violate that rule. Assuming pure fn returned, and that deref can be made pure, this could become something like :

trait Deref {
    type Target;

    #[allow_unsafe_impure_as_pure]
    pure fn deref(&self) -> &Target;
}

impl Deref for MyFancyArc {
    type Target = Connection;
    unsafe fn dereff(&self) -> &Connection { .. }
}

[nikomatsakis]

I appreciate the minimal nature of this RFC -- when I first opened it to read it, I was expecting something quite different. But I think overall I would be inclined to postpone it for now -- it seems like something that we can evolve a bit "in library land" (as, indeed, has already happened), perhaps as a crates.io crate that offers an unsafe trait DerefPure that others can opt into (and which is implemented for &T and Arc<T>). In particular I don't think that (a) we have enough experience with the precise guarnatees that we will want and (b) I would want to consider integrating any such notion deeper into the language, so that we gain advantages for optimization, perhaps pattern matching (though I don't personally feel it's necessarily needed there), and so forth.

I find @glaebhoerl's idea of a generalized concept of "unsafe trait" interesting, but it seems... well, let's just say it'd require a bit more elaboration too. In particular, it's not like the "unsafe contract" for a given trait is clear a priori, so I think I would not want to allow unsafe impl of a trait unless the trait opted into it and spelled out the unsafe contract (at minimum). And it's not clear why doing this via a unsafe impl of the existing trait is better than having a subtrait, in that case, since that seems to be the 'less machinery' way of achieving precisely the same thing, right?

[aturon]

Thanks, @tomaka, for this RFC.

FWIW, I agree entirely with @nikomatsakis's synopsis. I believe that DerefPure is already building up steam in the ecosystem; let's move it into std when the need feels more clear. For the time being:

@rfcbot fcp postpone

[rfcbot]

Team member @aturon has proposed to postpone this. The next step is review by the rest of the tagged teams:

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @sfackler

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[mikeyhew]

There is now a crate on crates.io that provides SafeDeref, using the name StableDeref: stable_deref_trait. Crate authors can use that crate for now, and when this trait gets moved into std, implement the new std:: trait while keeping stable_deref_trait::StableDeref for backward compatibility.

[rfcbot]

The final comment period is now complete.

[alexcrichton]

Looks like not much new came up during the FCP, so closing. Thanks again though for the RFC @tomaka!