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

I expect this needs crater, so: @bors try

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

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)
}

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.

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!

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

r? libs

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

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?

@bors try

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

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?

👌 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

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.

Ooh, I hadn't considered that. Even if this is 100% sound, that means the argument for it is subtle enough that I'm not sure I'd want to rely on it over something simpler. Maybe going back to an inference-limiting approach (or combining approaches) would be best? That way we can be sure there's no coercions in the first place. For ease of comparison, the trick in my original version of this PR was

let pinned_ref = unsafe { $crate::pin::Pin::new_unchecked(&mut pinned) };
pinned_ref

Since the type of pinned_ref isn't known when checking the let statement, we won't get coercions in the argument of Pin::new_unchecked.

If that's too subtle, I think there are other ways to limit inference/coercions too. This is probably even hackier, but to have an example, a trick I've used for preventing coercions in tests is defining trait Is<T> {} with impl<T> Is<T> for T {}. Making a function take impl Is<T> instead of T makes calls of it unable to coerce that argument even if T is known, since impl Is<T> is a different parameter and only constrained later. To combine this with the type constraint approach, unreachable_type_constraint could take an Is<T> to prevent coercions there. A #[diagnostic::on_unimplemented] attribute could maybe prevent the diagnostics from getting too much worse if a trait-based approach is desired? I've never used the attribute myself though.

🚧 Experiment pr-153457 is now running

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

🎉 Experiment pr-153457 is completed!
📊 1 regressed and 5 fixed (839795 total)
📊 2279 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 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

Wait what? Why did crater not catch https://github.com/restatedev/restate/blob/a472bad4e12427012f6215947edd35561d62d0b3/crates/vqueues/src/scheduler/drr.rs#L563 ?

Wait what? Why did crater not catch https://github.com/restatedev/restate/blob/a472bad4e12427012f6215947edd35561d62d0b3/crates/vqueues/src/scheduler/drr.rs#L563 ?

The create does not appear to be on crates.io (it has package.publish = "false") nor does the repo appear to be in https://github.com/rust-lang/rust-repos/blob/master/data/github.csv.