#![feature(negative_impls)] breaks the guarantee for Unpin provided by pin-project

[taiki-e]

pin-project provides an appropriate Unpin implementation by default. Since overlapping implementations are prohibited, this ensures that users cannot add inappropriate Unpin implementations.

However, currently, this guarantee can be broken by using #[feature(negative_impls)]: playground

Thanks @danielhenrymantilla for pointing out the interaction with this feature.

[taiki-e]

It seems that a warning that catches this has been implemented.

error: cross-crate traits with a default impl, like `Unpin`, should not be specialized
  --> tests/run-pass/negative_impls.rs:16:1
   |
16 | impl Unpin for Foo<MyPhantomPinned, ()> {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D suspicious-auto-trait-impls` implied by `-D warnings`
   = warning: this will change its meaning in a future release!
   = note: for more information, see issue #93367 <https://github.com/rust-lang/rust/issues/93367>
   = note: `MyPhantomPinned` is not a generic parameter

[danielhenrymantilla]

Update: I have discovered today that there is another way to let the coherence checker know that a local type for sure does not implement a trait, which thus triggers this issue as well:

::pin_project_lite::pin_project! {
    struct Foo<Pinned, Unpinned> {
        #[pin]
        pinned: Pinned,
        
        unpinned: Unpinned,
    }
}

struct MyPhantomPinned(::core::marker::PhantomPinned);

impl Unpin for MyPhantomPinned
where
    for<'cursed> str : Sized,
{}

impl Unpin for Foo<MyPhantomPinned, ()> {}

fn is_unpin<T: Unpin>() {}

fn main() {
    is_unpin::<Foo<MyPhantomPinned, ()>>()
}

• Playground

• Discord conversation

[taiki-e]

@danielhenrymantilla Thanks for finding that trick. I knew that trivial_bounds could be avoided with a lifetime or type parameter (#102 (comment)), but I did not know that it was possible to avoid "conflicting implementations" errors and emulate the complete negative_impls.

To be honest, I feel that is a compiler bug of exposing the complete negative_impls to stable.

That said, since private-in-public has been implemented (rust-lang/rust#48054), I guess going back to the pre-#53 approach would address a compiler bug.

[danielhenrymantilla]

To clarify: there is only so much you / a macro can do at this point; we always get back to "Unpin should have been an unsafe trait" position.

I'm afraid reverting #53 won't do the trick, since the mechanism for both is the same (my "negative impl" makes the generated impl, be it post-#53 or pre-#53, be seen as unreachable for the given generic instantiation, and thus, as non-overlapping.

---

However 🤔 there is a "known limitation" of coherence which we could take advantage of, here.

• (Those having written impls of FromResidual may have run into it, because of the defaulted type argument of FromResidual; cc @conradludgate.)

If an associated type defined in a foreign impl —such as as IntoIterator>::Item from some impl in the stdlib— is used in an impl, then such impl is deemed "fluid" over that type: it is treated as if this whole impl was written over a fully blanket generic type used in its stead. The result of this is that any impl involving such a thing is deemed to overlap with any other impl for that same implementor, which is exactly what we need here.

So, if we wrap __Origin<…> within an "identity" <Option<_> as IntoIterator>::Item layer in the where clause of the impl… Unpin, we'll end up:

• with a clause "trivially" equivalent to __Origin<…> in practice, so everything works as expected;
• but it will be deemed blankety enough to disable the "overly-clever coherence checker optimization" which allowed the problematic manual impl to begin with.

impl<'__pin, Pinned, Unpinned>
    ::pin_project_lite::__private::Unpin
for
    Foo<Pinned, Unpinned>
where
    <Option< // 👈
        __Origin<'__pin, Pinned, Unpinned>
    > as IntoIterator>::Item // 👈
        : ::pin_project_lite::__private::Unpin,
{}

• Playground (line 100)

You could even define, in your __private module:

// Identity type alias.
pub type PinnedFieldsOf<T> = <Option<T> as IntoIterator>::Item;

so as to end up with:

impl<'__pin, Pinned, Unpinned>
    ::pin_project_lite::__private::Unpin
for
    Foo<Pinned, Unpinned>
where
    ::pin_project_lite::__private::PinnedFieldsOf< // 👈
        __Foo<'__pin, Pinned, Unpinned>
    > // 👈
        : ::pin_project_lite::__private::Unpin,
{}

• Aside: I don't really get the role of '__pin in these impls (e.g., line https://docs.rs/crate/pin-project-lite/0.2.14/source/src/lib.rs#1229 and any mention of '__pin should be removable, IIUC)

[taiki-e]

I'm afraid reverting #53 won't do the trick, since the mechanism for both is the same (my "negative impl" makes the generated impl, be it post-#53 or pre-#53, be seen as unreachable for the given generic instantiation, and thus, as non-overlapping.

Ah, I haven't tested but I think you are right.

So, if we wrap __Origin<…> within an "identity" <Option<_> as IntoIterator>::Item layer in the where clause of the impl… Unpin, we'll end up:

• with a clause "trivially" equivalent to __Origin<…> in practice, so everything works as expected;
• but it will be deemed blankety enough to disable the "overly-clever coherence checker optimization" which allowed the problematic manual impl to begin with.

Thanks. That's an interesting approach...

• Aside: I don't really get the role of '__pin in these impls (e.g., line https://docs.rs/crate/pin-project-lite/0.2.14/source/src/lib.rs#1229 and any mention of '__pin should be removable, IIUC)

This is needed to avoid trivial_bounds error. See #102 (comment) for more info.