Prevent downstream impl DerefMut for Pin<LocalType>

[Darksonn]

The safety requirements for PinCoerceUnsized are essentially that the type does not have a malicious Deref or DerefMut impl. However, the Pin type is fundamental, so the end-user can provide their own implementation of DerefMut for Pin<&SomeLocalType>, so it's possible for Pin to have a malicious DerefMut impl. This unsoundness is known as #85099.

Unfortunately, this means that the implementation of PinCoerceUnsized for Pin is currently unsound. To fix that, modify the impl so that it becomes impossible for downstream crates to provide their own implementation of DerefMut for Pin by abusing a hidden struct that is not fundamental.

This PR is a breaking change, but it fixes #85099. The PR supersedes #144896.

r? lcnr

[Darksonn]

It doesn't immediately look like error messages have regressed, but the docs have:

I'm going to push another commit to improve the docs to this:

I think it's reasonable in this case since the impls really are equivalent except for the orphan rules treatment.

[lcnr]

I want to make sure I properly understand #85099 before merging this. I do think it's a very nice solution and gj for coming up with it!

I personally dislike "lying in the impl", even if it doesn't matter in practice. Gonna throw that question to @rust-lang/libs-api.

Let's crater

@bors try

[dtolnay]

We discussed this PR in today's standard library API meeting. Those present were on board with the approach, but it will be important to see a reasonably clean crater result and send PRs for any breakage, because not all downstream impls of DerefMut for Pin are necessarily unsound. The new implementation rules out correct as well as incorrect impls.

Once crater is finished, we would like to do a libs-api FCP to surface this to the rest of the team.

We noticed that the new pin::hidden::PinHelper type is now going to appear in diagnostics such as the pin-unsound-issue-85099-derefmut.stderr in this PR, but hopefully this mostly only happens when someone is doing funny business like writing their own DerefMut impl, and not for more typical use of Pin's methods and impls.

[Darksonn]

Ok, let's see what crater says. But I don't think there are any valid use-cases for impl DerefMut for Pin<P> for pointer types that aren't DerefMut.

[rust-bors]

☀️ Try build successful (CI)
Build commit: c659ee1 (c659ee110de67e82444e4b6c8407c1a9af9c2cf6, parent: 8c32e313cccf7df531e2d49ffb8227bb92304aee)

[lcnr]

@craterbot check

[craterbot]

👌 Experiment pr-145608 created and queued.
🤖 Automatically detected try build c659ee1
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[Darksonn]

Updating this with some additional tests for error messages. I'm not worried about PinHelper showing up in pin-unsound-issue-85099-derefmut.stderr, but that it also shows up in tests/ui/deref/pin-impl-deref.stderr is unfortunate.

(See individual commits for how the error messages change.)

[Darksonn]

A slightly different implementation seems to give somewhat better errors:

Darksonn@5e4d49a

But let's wait for crater before we think about that further.

[craterbot]

🚧 Experiment pr-145608 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-145608 is completed!
📊 6 regressed and 8 fixed (685389 total)
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[rust-rfcbot]

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

[Darksonn]

I think excluding certain traits from fundamental makes sense as what could be missing.

[traviscross]

@rustbot labels +S-waiting-on-documentation

@Darksonn, could you perhaps make a Reference PR here? We do cover fundamental in the Reference and discuss e.g. how the type T in Box<T>, Pin<T>, etc. is not considered "covered". We should add the needed caveats.

cc @rust-lang/lang-docs @tshepang @PLeVasseur

[lcnr]

What are the expected changes to the reference here? This does not change anything about fundamental types in the language. The most I can think of would be "note: the standard library sometimes works around this by 'sealing' traits for fundamental types via indirection, e.g. Pin: DerefMut...".

But I feel like given the way it is implemented as an ordinary trait implementation, I would expect us to document this in the standard library on the impl, if at all. If we think this exception to the coherence rules for fundamental types is worth documenting, imo we should add a comment to the DerefMut impl and potentially remove the #[cfg(docs)] version to always show the 'correct' trait bounds

[traviscross]

In my view, we should answer somewhere why impl DerefMut for Pin<LocalTy> doesn't work, because otherwise the extrapolation from the visible documentation in the standard library and the text in the Reference about the fact that Pin is fundamental and what that means suggests, as far as I can tell on a quick read, that it should work.

As for where that should go, we've discussed recently that somewhat more in the way of details about lang-item types may need to live in the Reference, so I can see this here from that perspective. I could see the core library docs saying something about this too, of course. It's OK if both say something about it. The key point, though, is that we should say this clearly and normatively somewhere in our documentation.

[Darksonn]

There are already a bunch of other examples of preventing downstream crates from implement traits on #[fundamental] things. One example is DerefMut for &T, which is prevented using an impl<T> !DerefMut for &T clause. This one isn't really so different from that.

[traviscross]

...which is prevented using an impl<T> !DerefMut for &T clause...

We document that. In this case, we're suppressing any documentation which would explain the behavior.

[BoxyUwU]

I don't think whether we have rustdoc hacks to avoid showing how an impl is written should have any bearing on whether something goes in the reference or not. This PR doesn't change language semantics in any way.

I do think it could possibly be confusing that people get errors that they "shouldn't" but the right place for that documentation to live would be the impl in std not the reference.

The fact that Pin is a Lang item is somewhat besides the point IMO. it may be special to the language but this behaviour is not a special Pin thing.

[traviscross]

As mentioned, I'm happy as long as we document it somewhere.

As for where that should go, we've discussed recently that somewhat more in the way of details about lang-item types may need to live in the Reference, so I can see this here from that perspective. I could see the core library docs saying something about this too, of course. It's OK if both say something about it. The key point, though, is that we should say this clearly and normatively somewhere in our documentation.

[Darksonn]

I looked at what the reference says about fundamental types, and I don't think this fits there. I added this to the docs:

[traviscross]

Thanks for adding the doc comment. Left some editorial notes.

[Darksonn]

Suggestions applied. I checked that the link is working.

[traviscross]

Thanks; all looks great to me.

View changes since this review

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[bors]

☔ The latest upstream changes (presumably #142771) made this pull request unmergeable. Please resolve the merge conflicts.