prevent deref coercions in pin!

[dianne]

View all comments

Mitigates #153438 using a (hopefully temporary!) typed macro idiom to ensure that when pin! produces a Pin<&mut T>, its argument is of type T. See #153438 (comment) for my ideas on how this could be changed in the future.

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 15 candidates

[dianne]

I expect this needs crater, so: @bors try

[dianne]

Just realized I could format things more cleanly (diff). This shouldn't affect the correctness of the try build.

[eggyal]

A more explicit approach could be to do something like:

super let mut pinned = $value;

#[allow(unreachable_code)]
'p: {
    fn unreachable_type_constraint<'a, T>(_: T) -> Pin<&'a mut T> {
        unreachable!()
    }

    break 'p unsafe { Pin::new_unchecked(&mut pinned) };
    unreachable_type_constraint(pinned)
}

[dianne]

So that's how to add types to macros! Funky idiom, but I do prefer the explicitness of actually having types, yeah. I think that'd be less likely to break inference too if anything's started relying on it; iirc type expectations are propagated from the block to the break expression (via the block's coercion target type). But it's fine because we'll encounter an error instead of silently adding a coercion if things don't match up.. I think.

Would you prefer to make your own PR @eggyal, or should I just work that into this one? I'm not a library reviewer so I can't say which would be preferable from that perspective, but from the perspective I do have, I think adding a type constraint is a better fix.

[eggyal]

By all means work it into this one. This idiom was originally suggested to me by lcnr some years ago, so I claim no credit for it!

[Kivooeo]

this change requires someone from libs team to be approved, so i'm reassigning

r? libs

[rust-bors]

☀️ Try build successful (CI)
Build commit: 048f484 (048f484e31141edc1fa71e20406300e92a9c16ba, parent: 64b72a1fa5449d928d5f553b01a596b78ee255d2)

[dianne]

Went ahead with the unreachable type constraint version of this (diff). There's unfortunately a bit of a diagnostics regression due to the extra type checking. I still think it's worth the lower likelihood of breakage, but it's a tradeoff. I'll be firing off a fresh try build, but if the diagnostics are a sticking point, maybe it'd be worth comparing crater results for both approaches?

[dianne]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 5d96fa0 (5d96fa0e954d77528204a1ba3b8847ec083c779b, parent: 64b72a1fa5449d928d5f553b01a596b78ee255d2)

[dianne]

I'll crater the type constraint approach first since that's my preferred quick fix at the moment.
@craterbot check

Also going to try re-running pr-check-2. It never restarted after being canceled it looks like?

[craterbot]

👌 Experiment pr-153457 created and queued.
🤖 Automatically detected try build 5d96fa0
🔍 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

[theemathas]

I'm not yet convinced that this approach is 100% sound. Although I think it would be a good idea to do this fix even if it's not 100% sound, since any soundness exploit of this would be significantly more convoluted than the current exploit.

Given a value x of type Src, it might be possible with this PR for the expression pin!(x) to create a value of type Pin<&mut Dst>, where Src and Dst are different types. This can happen and cause unsoundness if:

• Src can be coerced to Dst (this coercion would be done in the parameter of unreachable_type_constraint); and
• &mut Src can be coerced to &mut Dst.(this coercion would be done in the parameter of Pin::new_unchecked); and
• Src and Dst are different types and have no subtyping relationship with each other; and
• Dst does not implement Unpin

I am not sure whether there exist types Src and Dst such that these two coercions can happen.

Edit: Added two more bullet points

Edit 2: Changed a requirement into the Unpin requirement.

[rustbot]

This PR was rebased onto a different main 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.

[dianne]

I'm not familiar with T-libs or T-libs-api procedure, so I couldn't say there. The process I'm used to for language bug fixes is to get team approval via fcp merge, sometimes waiving the 10-day wait (though waiving the comment period might just be for beta regressions?).

Leaving #153438 open here and closing it via types FCP sounds reasonable to me! I've changed the description so it shouldn't automatically be closed.

Looking over the diagnostics again, I'm still not happy with how adding the extra function call affected those, but I think I've convinced myself that those are issues with the diagnostics, rather than the macro idiom. I can try and open a separate PR to make note_wrong_return_ty_due_to_generic_arg not leak implementation details of external macros. It already doesn't fire on calls from desugarings.

One last change, also: I've finally moved the function with the type signature of pin! into a separate helper so that it's not recreated for each pin! invocation. I'll once again hand this over to libs review to make sure I did that right. r? @Mark-Simulacrum since you've already reviewed this

[Mark-Simulacrum]

@bors r=Mark-Simulacrum,jackh726 rollup=iffy

View changes since this review

[rust-bors]

📌 Commit 5276fcd has been approved by Mark-Simulacrum,jackh726

It is now in the queue for this repository.