Require #[pin_v2] for direct &pin borrows of ADTs#2
Conversation
|
The CI failure appears to come from The focused local checks passed and are listed in the PR body. This PR is intended as a patch against the active pin-borrowck feature branch for rust-lang#153693, not as an independent PR directly to |
|
Grateful for your help! Let's merge it first and see if the original PR can pass the CI. |
d7f4bbd
into
frank-king:feature/pin-borrowck
|
I saw the CI failure in https://github.com/rust-lang/rust/actions/runs/25086407095/job/73502863053 after my merged helper PR. I’m reproducing the exact failing If this was caused by my patch, I’ll follow up with a focused fix PR. Sorry for the churn. |
| fn check_pin_borrow_of_adt_requires_pin_v2(&self, ty: Ty<'tcx>, span: Span) { | ||
| let ty = self.structurally_resolve_type(span, ty); | ||
| if let Some(adt) = ty.ty_adt_def() | ||
| && !adt.is_pin_project() |
There was a problem hiding this comment.
Having read the failed CI cases thoroughly, instead of directly reporting cannot directly pin an ADT that is not `#[pin_v2]` , I now prefer checking if ADT: Unpin first and report this error only for ADTs that are !Unpin.
struct Foo; // no `#[pin_v2]`, but `Foo: Unpin`
// For `Foo: Unpin`, given that `&pin mut Foo` and `&mut Foo` are mutually coercible,
// I don't think it is reasonable to allow `&mut Foo` but not `&pin mut Foo` here.
let mut x: Pin<&mut _> = &pin mut Foo;There was a problem hiding this comment.
I opened a follow-up PR for the Unpin gate change here: #4
It keeps the diagnostic limited to explicit &pin borrows, but only emits it for non-#[pin_v2] ADTs that are known/proven to be !Unpin.
There was a problem hiding this comment.
Have you tested this behavior? In case that pinned borrow may be inconsistent with the pinned patterns.
pub struct NotUnpin(PhantomPinned);
pub struct GenericAdt<T> {
pub x: T,
}
// An ill-formed unconditional `Unpin` for `GenericAdt<T>`.
impl<T> Unpin for GenericAdt<T> {}
pub fn unsound_pin_borrow<T>(
input: &pin mut GenericAdt<T>,
) -> &pin mut T {
// This should be an error, or else you can get an `&pin mut NotUnpin`
// and later an `&mut NotUnpin` via `&mut (*input).x`
// as `GenericAdt<T>` is `Unpin`.
&pin mut input.x
}
let mut g = GenericAdt { x: NotUnpin(PhantomPinned) };
let f = &pin mut g; // Ok because of your new added rule.
let x: &pin mut NotUnpin = unsound_pin_borrow(f);
// This will be rejected in pinned patterns checks.
// let ref pin mut GenericAdt { x } = f;
drop(f); // Kills f, but the pinnedness of x won't expire.
let f: &mut _ = f; // Allowed by coercion.
let x: &mut NotUnpin = &mut f.x; // Unsound! Breaks `Pin`' s safety contract.There was a problem hiding this comment.
I also think allowing &pin borrows for GenericAdt<T> is unsound if it is unknown to be Unpin or !Unpin.
struct NotUnpin(PhantomPinned);
struct GenericAdt<T>(T);
impl Drop for GenericAdt<T> {
// Anyway, an `&mut GenericAdt<T>` is obtained.
fn drop(&mut self) { /* ... */ }
}
fn foo(mut input: GenericAdt<NotUnpin>) {
let x: &pin mut _ = &pin mut input; // Allowed by your new rule
// At `<GenericAdt<NotUnpin> as Drop>::drop`, it obtains an
// `&mut GenericAdt<NotUnpin>`, which breaks `Pin`'s safety contract.
}There was a problem hiding this comment.
Thanks, fixed.
Refer to: #4
Direct &pin no longer relies only on the outer type satisfying Unpin. Generic ADTs are now rejected unless they are #[pin_v2], and generic/unknown field targets like T are rejected instead of being accepted conservatively.
This prevents a manual impl<T> Unpin for Wrapper<T> from bypassing structural pin-safety restrictions and keeps the direct &pin borrow path aligned with pinned-pattern projection safety.
I added regression coverage for the generic field projection case, the unconditional manual impl<T> Unpin wrapper, and the generic Drop wrapper. I also updated the diagnostic wording from “ADT” to “type” because one rejected case can target a generic parameter rather than an ADT directly.
Tested with the focused pin-ergonomics suite, tidy, fmt --check, and git diff --check. I did not run the full tests/ui suite locally because it is too broad/expensive, but the focused pin-ergonomics suite passes.
Summary
Require direct user-written
&pinborrows of ADTs to use#[pin_v2].The check runs during HIR type checking for explicit
hir::BorrowKind::Pin, so itrejects direct
&pin mut/&pin constborrow expressions on non-#[pin_v2]ADTs without affecting compiler-generated
AutoBorrow::Pinadjustments orcoercions.
What changed
not
#[pin_v2].FnCtxt::check_expr_addr_offor explicit user-written&pinborrows.AdtDef::is_pin_project()as the source of truth for#[pin_v2].marking their local ADTs
#[pin_v2].&pin mutand&pin constborrows of anon-
#[pin_v2]ADT.Why
The active pin ergonomics borrowck PR still had an unchecked task to forbid
&pinborrows for ADTs without#[pin_v2].Pattern projection already rejects non-
#[pin_v2]ADTs in related cases, butdirect user-written
&pin mut fooand&pin const fooborrow expressions werestill accepted.
This PR enforces that direct rule while avoiding compiler-generated transient pin
adjustments.
Tests
Passed:
Notes for reviewers
The main review questions are:
&pinof thePinADT itself have any special exception, orshould the literal ADT rule apply?
#[pin_v2]suggestion appropriate here, matchingthe existing projection diagnostic style?
Related
&pin mut|const $placerust-lang/rust#153693