Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions compiler/rustc_hir_typeck/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1385,6 +1385,21 @@ pub(crate) struct ProjectOnNonPinProjectType {
pub sugg_span: Option<Span>,
}

#[derive(Diagnostic)]
#[diag("cannot directly pin an ADT that is not `#[pin_v2]`")]
pub(crate) struct DirectPinBorrowOfNonPinProjectType {
#[primary_span]
pub span: Span,
#[note("type defined here")]
pub def_span: Option<Span>,
#[suggestion(
"add `#[pin_v2]` here",
code = "#[pin_v2]\n",
applicability = "machine-applicable"
)]
pub sugg_span: Option<Span>,
}

#[derive(Diagnostic)]
#[diag("falling back to `f32` as the trait bound `f32: From<f64>` is not satisfied")]
pub(crate) struct FloatLiteralF32Fallback {
Expand Down
19 changes: 19 additions & 0 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
Ty::new_ptr(self.tcx, ty, mutbl)
}
hir::BorrowKind::Ref | hir::BorrowKind::Pin => {
if kind == hir::BorrowKind::Pin {
self.check_pin_borrow_of_adt_requires_pin_v2(ty, expr.span);
}

// Note: at this point, we cannot say what the best lifetime
// is to use for resulting pointer. We want to use the
// shortest lifetime possible so as to avoid spurious borrowck
Expand All @@ -523,6 +527,21 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

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()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
let def_span = self.tcx.hir_span_if_local(adt.did());
let sugg_span = def_span.map(|span| span.shrink_to_lo());
self.dcx().emit_err(crate::errors::DirectPinBorrowOfNonPinProjectType {
span,
def_span,
sugg_span,
});
}
}

/// Does this expression refer to a place that either:
/// * Is based on a local or static.
/// * Contains a dereference
Expand Down
1 change: 1 addition & 0 deletions tests/ui/pin-ergonomics/borrow-pinned-projection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
// unrelated disjoint fields, but it must reject later mutable borrows or moves
// of `pair.0` until reassignment.

#[pin_v2]
struct Foo;

fn mutable_borrow_of_pinned_projection() {
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/pin-ergonomics/borrow-pinned-projection.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: cannot borrow `pair.0` as mutable because it is pinned
--> $DIR/borrow-pinned-projection.rs:18:19
--> $DIR/borrow-pinned-projection.rs:19:19
|
LL | let _pin = &pin mut pair.0;
| --------------- pin of `pair.0` occurs here
Expand All @@ -8,7 +8,7 @@ LL | let _borrow = &mut pair.0;
| ^^^^^^^^^^^ borrow of `pair.0` as mutable occurs here

error: cannot move out of `pair.0` because it is pinned
--> $DIR/borrow-pinned-projection.rs:30:18
--> $DIR/borrow-pinned-projection.rs:31:18
|
LL | let _pin = &pin mut pair.0;
| --------------- pin of `pair.0` occurs here
Expand Down
26 changes: 13 additions & 13 deletions tests/ui/pin-ergonomics/borrow-unpin.pinned.stderr
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
error: cannot move out of `foo` because it is pinned
--> $DIR/borrow-unpin.rs:28:14
--> $DIR/borrow-unpin.rs:30:14
|
LL | foo_pin_mut(&pin mut foo); // ok
| ------------ pin of `foo` occurs here
LL | foo_move(foo);
| ^^^ move out of `foo` occurs here

error[E0505]: cannot move out of `foo` because it is borrowed
--> $DIR/borrow-unpin.rs:32:14
--> $DIR/borrow-unpin.rs:34:14
|
LL | let mut foo = Foo::default();
| ------- binding `foo` declared here
Expand All @@ -19,7 +19,7 @@ LL | foo_pin_mut(x); // ok
| - borrow later used here
|
note: if `Foo` implemented `Clone`, you could clone the value
--> $DIR/borrow-unpin.rs:13:1
--> $DIR/borrow-unpin.rs:14:1
|
LL | struct Foo(PhantomPinned);
| ^^^^^^^^^^ consider implementing `Clone` for this type
Expand All @@ -28,15 +28,15 @@ LL | let x = &pin mut foo;
| --- you could clone this value

error: cannot move out of `foo` because it is pinned
--> $DIR/borrow-unpin.rs:39:14
--> $DIR/borrow-unpin.rs:41:14
|
LL | foo_pin_mut(&pin mut foo); // ok
| ------------ pin of `foo` occurs here
LL | foo_move(foo);
| ^^^ move out of `foo` occurs here

error[E0505]: cannot move out of `foo` because it is borrowed
--> $DIR/borrow-unpin.rs:43:14
--> $DIR/borrow-unpin.rs:45:14
|
LL | let mut foo = Foo::default();
| ------- binding `foo` declared here
Expand All @@ -48,7 +48,7 @@ LL | foo_pin_mut(x); // ok
| - borrow later used here
|
note: if `Foo` implemented `Clone`, you could clone the value
--> $DIR/borrow-unpin.rs:13:1
--> $DIR/borrow-unpin.rs:14:1
|
LL | struct Foo(PhantomPinned);
| ^^^^^^^^^^ consider implementing `Clone` for this type
Expand All @@ -57,15 +57,15 @@ LL | let x = &pin mut foo; // ok
| --- you could clone this value

error: cannot move out of `foo` because it is pinned
--> $DIR/borrow-unpin.rs:50:14
--> $DIR/borrow-unpin.rs:52:14
|
LL | foo_pin_ref(&pin const foo); // ok
| -------------- pin of `foo` occurs here
LL | foo_move(foo);
| ^^^ move out of `foo` occurs here

error[E0505]: cannot move out of `foo` because it is borrowed
--> $DIR/borrow-unpin.rs:54:14
--> $DIR/borrow-unpin.rs:56:14
|
LL | let foo = Foo::default();
| --- binding `foo` declared here
Expand All @@ -77,7 +77,7 @@ LL | foo_pin_ref(x);
| - borrow later used here
|
note: if `Foo` implemented `Clone`, you could clone the value
--> $DIR/borrow-unpin.rs:13:1
--> $DIR/borrow-unpin.rs:14:1
|
LL | struct Foo(PhantomPinned);
| ^^^^^^^^^^ consider implementing `Clone` for this type
Expand All @@ -86,7 +86,7 @@ LL | let x = &pin const foo; // ok
| --- you could clone this value

error[E0502]: cannot borrow `foo` as immutable because it is also borrowed as mutable
--> $DIR/borrow-unpin.rs:65:13
--> $DIR/borrow-unpin.rs:67:13
|
LL | let x = &pin mut foo; // ok
| ------------ mutable borrow occurs here
Expand All @@ -96,7 +96,7 @@ LL | foo_pin_mut(x);
| - mutable borrow later used here

error[E0499]: cannot borrow `foo` as mutable more than once at a time
--> $DIR/borrow-unpin.rs:87:17
--> $DIR/borrow-unpin.rs:89:17
|
LL | let x = &pin mut foo; // ok
| ------------ first mutable borrow occurs here
Expand All @@ -106,7 +106,7 @@ LL | foo_pin_mut(x);
| - first borrow later used here

error[E0502]: cannot borrow `foo` as mutable because it is also borrowed as immutable
--> $DIR/borrow-unpin.rs:98:17
--> $DIR/borrow-unpin.rs:100:17
|
LL | let x = &pin const foo; // ok
| -------------- immutable borrow occurs here
Expand All @@ -116,7 +116,7 @@ LL | foo_pin_ref(x);
| - immutable borrow later used here

error[E0502]: cannot borrow `foo` as immutable because it is also borrowed as mutable
--> $DIR/borrow-unpin.rs:109:17
--> $DIR/borrow-unpin.rs:111:17
|
LL | let x = &pin mut foo; // ok
| ------------ mutable borrow occurs here
Expand Down
2 changes: 2 additions & 0 deletions tests/ui/pin-ergonomics/borrow-unpin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ use std::marker::PhantomPinned;
use std::pin::Pin;

#[cfg(pinned)]
#[pin_v2]
#[derive(Default)]
struct Foo(PhantomPinned);

#[cfg(unpin)]
#[pin_v2]
#[derive(Default)]
struct Foo;

Expand Down
26 changes: 13 additions & 13 deletions tests/ui/pin-ergonomics/borrow-unpin.unpin.stderr
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
error: cannot move out of `foo` because it is pinned
--> $DIR/borrow-unpin.rs:28:14
--> $DIR/borrow-unpin.rs:30:14
|
LL | foo_pin_mut(&pin mut foo); // ok
| ------------ pin of `foo` occurs here
LL | foo_move(foo);
| ^^^ move out of `foo` occurs here

error[E0505]: cannot move out of `foo` because it is borrowed
--> $DIR/borrow-unpin.rs:32:14
--> $DIR/borrow-unpin.rs:34:14
|
LL | let mut foo = Foo::default();
| ------- binding `foo` declared here
Expand All @@ -19,7 +19,7 @@ LL | foo_pin_mut(x); // ok
| - borrow later used here
|
note: if `Foo` implemented `Clone`, you could clone the value
--> $DIR/borrow-unpin.rs:17:1
--> $DIR/borrow-unpin.rs:19:1
|
LL | struct Foo;
| ^^^^^^^^^^ consider implementing `Clone` for this type
Expand All @@ -28,15 +28,15 @@ LL | let x = &pin mut foo;
| --- you could clone this value

error: cannot move out of `foo` because it is pinned
--> $DIR/borrow-unpin.rs:39:14
--> $DIR/borrow-unpin.rs:41:14
|
LL | foo_pin_mut(&pin mut foo); // ok
| ------------ pin of `foo` occurs here
LL | foo_move(foo);
| ^^^ move out of `foo` occurs here

error[E0505]: cannot move out of `foo` because it is borrowed
--> $DIR/borrow-unpin.rs:43:14
--> $DIR/borrow-unpin.rs:45:14
|
LL | let mut foo = Foo::default();
| ------- binding `foo` declared here
Expand All @@ -48,7 +48,7 @@ LL | foo_pin_mut(x); // ok
| - borrow later used here
|
note: if `Foo` implemented `Clone`, you could clone the value
--> $DIR/borrow-unpin.rs:17:1
--> $DIR/borrow-unpin.rs:19:1
|
LL | struct Foo;
| ^^^^^^^^^^ consider implementing `Clone` for this type
Expand All @@ -57,15 +57,15 @@ LL | let x = &pin mut foo; // ok
| --- you could clone this value

error: cannot move out of `foo` because it is pinned
--> $DIR/borrow-unpin.rs:50:14
--> $DIR/borrow-unpin.rs:52:14
|
LL | foo_pin_ref(&pin const foo); // ok
| -------------- pin of `foo` occurs here
LL | foo_move(foo);
| ^^^ move out of `foo` occurs here

error[E0505]: cannot move out of `foo` because it is borrowed
--> $DIR/borrow-unpin.rs:54:14
--> $DIR/borrow-unpin.rs:56:14
|
LL | let foo = Foo::default();
| --- binding `foo` declared here
Expand All @@ -77,7 +77,7 @@ LL | foo_pin_ref(x);
| - borrow later used here
|
note: if `Foo` implemented `Clone`, you could clone the value
--> $DIR/borrow-unpin.rs:17:1
--> $DIR/borrow-unpin.rs:19:1
|
LL | struct Foo;
| ^^^^^^^^^^ consider implementing `Clone` for this type
Expand All @@ -86,7 +86,7 @@ LL | let x = &pin const foo; // ok
| --- you could clone this value

error[E0502]: cannot borrow `foo` as immutable because it is also borrowed as mutable
--> $DIR/borrow-unpin.rs:65:13
--> $DIR/borrow-unpin.rs:67:13
|
LL | let x = &pin mut foo; // ok
| ------------ mutable borrow occurs here
Expand All @@ -96,7 +96,7 @@ LL | foo_pin_mut(x);
| - mutable borrow later used here

error[E0499]: cannot borrow `foo` as mutable more than once at a time
--> $DIR/borrow-unpin.rs:87:17
--> $DIR/borrow-unpin.rs:89:17
|
LL | let x = &pin mut foo; // ok
| ------------ first mutable borrow occurs here
Expand All @@ -106,7 +106,7 @@ LL | foo_pin_mut(x);
| - first borrow later used here

error[E0502]: cannot borrow `foo` as mutable because it is also borrowed as immutable
--> $DIR/borrow-unpin.rs:98:17
--> $DIR/borrow-unpin.rs:100:17
|
LL | let x = &pin const foo; // ok
| -------------- immutable borrow occurs here
Expand All @@ -116,7 +116,7 @@ LL | foo_pin_ref(x);
| - immutable borrow later used here

error[E0502]: cannot borrow `foo` as immutable because it is also borrowed as mutable
--> $DIR/borrow-unpin.rs:109:17
--> $DIR/borrow-unpin.rs:111:17
|
LL | let x = &pin mut foo; // ok
| ------------ mutable borrow occurs here
Expand Down
1 change: 1 addition & 0 deletions tests/ui/pin-ergonomics/borrow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use std::pin::Pin;

#[pin_v2]
struct Foo;

fn foo_pin_mut(_: Pin<&mut Foo>) {}
Expand Down
8 changes: 4 additions & 4 deletions tests/ui/pin-ergonomics/borrow.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: cannot borrow `x` as mutable because it is pinned
--> $DIR/borrow.rs:31:14
--> $DIR/borrow.rs:32:14
|
LL | let _x = &pin mut x;
| ---------- pin of `x` occurs here
Expand All @@ -8,7 +8,7 @@ LL | let _x = &mut x;
| ^^^^^^ borrow of `x` as mutable occurs here

error: cannot move out of `x` because it is pinned
--> $DIR/borrow.rs:32:14
--> $DIR/borrow.rs:33:14
|
LL | let _x = &pin mut x;
| ---------- pin of `x` occurs here
Expand All @@ -17,7 +17,7 @@ LL | let _x = x;
| ^ move out of `x` occurs here

error: cannot borrow `y` as mutable because it is pinned
--> $DIR/borrow.rs:40:14
--> $DIR/borrow.rs:41:14
|
LL | let _y = &pin const y;
| ------------ pin of `y` occurs here
Expand All @@ -26,7 +26,7 @@ LL | let _y = &mut y;
| ^^^^^^ borrow of `y` as mutable occurs here

error: cannot move out of `y` because it is pinned
--> $DIR/borrow.rs:41:14
--> $DIR/borrow.rs:42:14
|
LL | let _y = &pin const y;
| ------------ pin of `y` occurs here
Expand Down
17 changes: 17 additions & 0 deletions tests/ui/pin-ergonomics/direct-borrow-requires-pin-v2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#![feature(pin_ergonomics)]
#![allow(incomplete_features)]
//@ normalize-stderr: "\n\n\z" -> "\n"

struct NotPinV2;

fn direct_pin_mut(mut value: NotPinV2) {
let _ = &pin mut value;
//~^ ERROR cannot directly pin an ADT that is not `#[pin_v2]`
}

fn direct_pin_const(value: NotPinV2) {
let _ = &pin const value;
//~^ ERROR cannot directly pin an ADT that is not `#[pin_v2]`
}

fn main() {}
Loading
Loading