Idea: Enhance match ergonomics to match on pinned enums without unsafe

[Arnavion]

Apologies if this has been discussed already. I didn’t find anything in GH issues or the Pin RFC or in this forum.

Consider the case of implementing Future / Stream on an enum that wraps Futures / Streams in its variants, and wants to impl poll() by forwarding to the variants. Currently you have to use unsafe Pin::get_unchecked_mut + Pin::new_unchecked to match on the enum and access the variant fields.

futures 0.3’s impl of Stream on either::Either is a good example:

fn poll_next(self: Pin<&mut Self>, lw: &LocalWaker) -> Poll<Option<A::Item>> {
    unsafe {
        match Pin::get_unchecked_mut(self) {
            Either::Left(a) => Pin::new_unchecked(a).poll_next(lw),
            Either::Right(b) => Pin::new_unchecked(b).poll_next(lw),
        }
    }
}

It would be nice if match ergonomics was enhanced to let you write:

match self {
    Either::Left(a /* : Pin<&mut A>*/ ) => a.poll_next(lw),
    Either::Right(b /* : Pin<&mut B>*/ ) => b.poll_next(lw),
}

so that you didn’t have to use the unsafe Pin API.

---

In general, code like this:

match some_pinned_value {
    Enum::A(a, b, c) => { body },
    Enum::B(d, e, f) => { body },
}

would expand to something like:

match unsafe { Pin::get_unchecked_mut(some_pinned_value) } {
    Enum::A(a, b, c) => { let (a, b, c) = unsafe { (Pin::new_unchecked(a), Pin::new_unchecked(b), Pin::new_unchecked(c)) }; body },
    Enum::B(d, e, f) => { let (d, e, f) = unsafe { (Pin::new_unchecked(d), Pin::new_unchecked(e), Pin::new_unchecked(f)) }; body },
}

This also generalizes to more complicated / multiple-level nested patterns like Enum::C(Some(Struct { field1 }))

---

Counter-points to this idea:

1. A little unsafe doesn’t hurt, especially if it’s in common libraries like futures where it can be trusted to be correct. async also makes many custom Future impls unnecessary.

   But I believe custom impls in user code will still be common enough that it would be a nice to not require unsafe. I personally have crates on both stable (using futures 0.1) and nightly (using futures 0.3 and async fn) that have custom Future and Stream impls.

2. It’s not strictly necessary to use unsafe. You could wrap all the inner Futures / Streams in a Pin<Box> layer to make the enum itself Unpin. Then you could write match &mut *self { Either::Left(a) => a.as_mut().poll_next(lw), ... }.

   But the enum value itself is going to be pinned anyway, so deliberately making it Unpin for the sake of simpler syntax seems wasteful.

3. This is more complicated and magical than the transformation that match ergonomics does right now. It has to insert calls to Pin API inside unsafe blocks inside each match arm, and also around the matched expression itself.

4. Related to the previous point, this feature probably won’t see any use outside of Future::poll and Stream::poll impls (and Generator::resume in the future). Is the complication worth it?

5. This doesn’t help with the other type of custom impls - impls on structs - since those must still use the unsafe Pin::map_unchecked_mut to access their fields. While that is still way fewer uses of unsafe in comparison to enums (only one map_unchecked_mut per field, whereas enums require one get_unchecked_mut plus one new_unchecked per binding per variant), it still feels dissatisfactory to not have a complete solution.

[Nemo157]

See also rust-lang-nursery/pin-utils#21 for discussion on providing some macro sugar for this operation.

[Nemo157]

A bit more direct commentary on the proposal, I don’t think this can go via match ergonomics, because the pin-projection in the match is the point at which you are asserting soundness of your implementation as a whole.

For example, given an enum like

enum FutureOrDone<F: Future> {
    Future(F),
    Done(F::Output),
}

impl<F> Unpin for FutureOrDone<F> where F: Future + Unpin {}

it is unsound to project Pin<&mut FutureOrDone<F>> -> Pin<&mut F::Output> with the Done variant, but I can’t see any way with match ergonomics to tell it that only the Future variant should be pin-projected, while the Done variant is a Pin<&mut> -> &mut projection.

[Arnavion]

it is unsound to project Pin<&mut FutureOrDone<F>> -> Pin<&mut F::Output> with the Done variant

Sorry, I'm missing why this is unsound.

[Arnavion]

Related to your example, why is Unpin not unsafe to manually implement? A bad impl allows safe code to move a !Unpin value after it had been previously pinned, which invalidates any self-references it holds. playground

(I wanted to demonstrate with a generator like || { let x = 5; let y = &x; yield; *y } instead of Bar, but borrows can’t cross yield points so the self-reference isn’t observable.)

[H2CO3]

Could you provide an example with an ordinary, safe reference there? I was trying to rewrite your code but it doesn’t compile because borrow checking – and that seems right to me in the light of Unipin not being an unsafe trait. After all, in the code above, you have raw pointers, and whatever is pointed by a raw pointer can only be observed by unsafely dereferencing it, which means that in order to get undefined behavior from your code, one would need to use unsafe.

(However, I’m not arguing that Unpin should be a safe trait; intuitively, it would also make more sense to me at first sight if it were unsafe. However, I haven’t been following the evolution of the Pin API too closely recently, but I’m somewhat confident if this were an oversight it wouldn’t have slipped under the radar for so long.)

[Arnavion]

Could you provide an example with an ordinary, safe reference there?

I don't think I can. AFAIK generator impls and Future impls (via async) are the only way to create values that contain dynamic self-references without the user having to write any unsafe. Like I said in the previous post, I couldn't get a generator example to work because the compiler prohibits borrows across yield points, so you can't make a self-reference -> resume() -> observe the (now broken) self-reference.

(You can of course make a type with a static self-reference (struct S { t: T, r: Option<&'a T> } let mut s = S { t: T, r: None }; s.r = Some(&s.t);), but as you found the compiler will already stack-pin such a value by making it immovable, so you can't even Pin it.)

but I’m somewhat confident if this were an oversight it wouldn’t have slipped under the radar for so long

FWIW it did use to be unsafe, and was intentionally changed as documented here. So yes, I do feel I'm missing something, though I don't follow the logic of that comment.

[Nemo157]

@Arnavion

Sorry, I’m missing why this is unsound.

This is unsound because of the Unpin implementation, if you were to use some Foo: Future<Output: !Unpin> + Unpin and project Pin<&mut FutureOrDone<Foo>> -> Pin<&mut Foo::Output> then you have projected from an Unpin type to a !Unpin type and could move it even after it was observed to be pinned.

@Arnavion

Related to your example, why is Unpin not unsafe to manually implement?

You can already cause the same issues with Drop using safe code, so since you're already having to assert soundness related to Drop when constructing/projecting the Pin, having Unpin be unsafe doesn't reduce the number of places you have to check the invariants or make any other APIs safe.

@Arnavion

(I wanted to demonstrate with a generator like || { let x = 5; let y = &x; yield; *y } instead of Bar , but borrows can’t cross yield points so the self-reference isn’t observable.)

To construct a self-referential generator you need to use the static keyword: static || { let x = 5; let y = &x; yield; *y } (not all that useful to demonstrate runtime pinning issues as it will just cause undefined behaviour, much easier to throw together types that assert when the pinning invariants are broken (now that I think about it, maybe self-referential generators should assert in debug builds to make tracking down issues easier)).

let mut p = Pin::new(&mut foo); // foo.0 is now pinned and is free to hold self-references

foo.0 is not pinned here, you have not observed a Pin<&mut Bar>, so unless Foo adds an API for the pin projection (which would include unsafe code, and be unsound because of the Unpin implementation) you can't trust that &foo.0 won't move.

(EDIT: Technically it would be ok for Foo to internally assume that self.0 is pinned once it has seen a Pin<&mut Self> without directly producing a Pin<&mut Self.0>, but it would still need to ensure the same invariants, which it can't with its current Unpin implementation, and there would need to be some unsafe code somewhere to actually take advantage of its assumption).

[Arnavion]

@Nemo157

Sorry, I’m missing why this is unsound.

This is unsound because of the Unpin implementation, if you were to use some Foo: Future<Output: !Unpin> + Unpin and project Pin<&mut FutureOrDone<Foo>> -> Pin<&mut Foo::Output> then you have projected from an Unpin type to a !Unpin type and could move it even after it was observed to be pinned.

Okay, that makes sense. If Foo is Unpin then you can get the Foo::Output from it by directly matching it, without ever constructing the Pin<&mut Foo::Output>. So having the match ergonomics construct the Pin<&mut Foo::Output> automatically for you is undesirable. I now understand the discussion in the pin-utils issue you linked as well.

@Nemo157

Related to your example, why is Unpin not unsafe to manually implement?

You can already cause the same issues with Drop using safe code, so since you’re already having to assert soundness related to Drop when constructing/projecting the Pin , having Unpin be unsafe doesn’t reduce the number of places you have to check the invariants or make any other APIs safe.

Right, the type in my playground link is basically the same situation as your Foo: Future<Output: !Unpin> + Unpin case, ie an Unpin wrapper with a !Unpin field inside.

@Nemo157

(I wanted to demonstrate with a generator like || { let x = 5; let y = &x; yield; *y } instead of Bar , but borrows can’t cross yield points so the self-reference isn’t observable.)

To construct a self-referential generator you need to use the static keyword: static || { let x = 5; let y = &x; yield; *y }

Good to know, thanks.

I realized the flaw with what I was trying to do, which is that my Bar could be mutated to hold the self-reference without needing to be pinned, whereas Generator::resume requires the generator to have been pinned first, and constructing such a Pin requires unsafe.

So in retrospect the moral is that things that are !Unpin should only provide mutators that take Pin<&mut Self>, not &mut Self. Even if a wrapper implements Unpin incorrectly like my Foo did, it would still have to use unsafe to be able to abuse the !Unpin value, so it's fine.

@Nemo157

not all that useful to demonstrate runtime pinning issues as it will just cause undefined behaviour

Well my intention was to demonstrate undefined behavior anyway by having a &i32 end up pointing to arbitrary garbage.

@Nemo157

let mut p = Pin::new(&mut foo); // foo.0 is now pinned and is free to hold self-references

foo.0 is not pinned here, you have not observed a Pin<&mut Bar> , so unless Foo adds an API for the pin projection (which would include unsafe code, and be unsound because of the Unpin implementation) you can’t trust that &foo.0 won’t move.

(EDIT: Technically it would be ok for Foo to internally assume that self.0 is pinned once it has seen a Pin<&mut Self> without directly producing a Pin<&mut Self.0> , but it would still need to ensure the same invariants, which it can’t with its current Unpin implementation, and there would need to be some unsafe code somewhere to actually take advantage of its assumption).

Right, your edit is what my mental model was, and I agree that the Unpin implementation broke the invariant, which is why I was wondering why Unpin wasn't unsafe to impl. But it makes sense now.