-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement parsing of pinned borrows #135731
base: master
Are you sure you want to change the base?
Conversation
rustbot has assigned @petrochenkov. Use |
Some changes occurred to the CTFE machinery cc @rust-lang/wg-const-eval |
This comment has been minimized.
This comment has been minimized.
ceaa211
to
e57789f
Compare
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
r? compiler |
This is not the semantic we want or a good placeholder for it. What we want is that fn f<T: Unpin>(x: &mut T) {
let _: Pin<&mut T> = &pin mut *x; // i.e., `Pin::new(&mut *x)`.
} That is, if the type in the place is After that, we start talking about how to extend this to safe pin projection to struct fields. |
r? traviscross |
I agree with you, and my previous statement was not precise. What I mean is similar to yours. I expect The pinned places might be defined as: A place is a pinned place if one of the following conditions holds:
Here are a few examples to explain: #[derive(Default)]
struct Foo<T> {
x: T,
}
fn f<T: Default, U: Default + Unpin>() {
// Case 1: not `Unpin`, and not pinned
let mut a = Foo::<T>::default(); // `a` and `a.x` are non-pinned places but not pinned places
let _: &mut Foo = &mut a; // ok
let _: &mut T = &mut a.x; // ok
let _: Pin<&mut Foo> = &pin mut a; // ERROR
let _: Pin<&mut T> = &pin mut a.x; // ERROR
// Case 2: not `Unpin`, and pinned
let pin mut b = Foo::<T>::default(); // `b` and `b.x` are pinned places but not non-pinned places
let _: &mut Foo = &mut b; // ERROR
let _: &mut Foo = &mut b.x; // ERROR
let _: &pin mut Foo = &pin mut b; // ok
let _: &pin mut Foo = &pin mut b.x; // ok
// Case 3: `Unpin`, and not pinned
let mut c = Foo::<U>::default(); // `c` and `c.x` are both pinned and non-pinned places
let _: &mut Foo = &mut c; // ok
let _: &mut T = &mut c.x; // ok
let _: Pin<&mut Foo> = &pin mut c; // ok
let _: Pin<&mut T> = &pin mut c.x; // ok
// Case 4: `Unpin`, and pinned
let pin mut d = Foo::<U>::default(); // WARN (useless `pin`) // `d` and `d.x` are both pinned and non-pinned places
let _: &mut Foo = &mut d; // ok
let _: &mut T = &mut d.x; // ok
let _: Pin<&mut Foo> = &pin mut d; // ok (useless `pin`)
let _: Pin<&mut T> = &pin mut d.x; // ok (useless `pin`)
} |
Thanks, that's helpful. Going through the examples, starting on Case 1, we're actually instead interested in this working: struct Foo<T> { x: T }
fn f<T>(mut x: Foo<T>) {
let _: Pin<&mut Foo<T>> = &pin mut x; //~ OK
let _: &Foo<T> = &x; //~ OK
let _: &mut Foo<T> = &mut x; //~ ERROR place has been pinned
let _move: Foo<T> = x; //~ ERROR place has been pinned
} The In this way, This is also why we're not sold on doing Regarding the safe pin projections to fields, let's do that in a separate PR so we can set those questions aside for the moment. That a place is pinned doesn't necessarily imply that we can safely pin project to a field without taking a number of other steps. |
Thanks for your feedback too! Regarding your code snippet, I have some questions. First, this is the semantics of struct Foo<T> { x: T }
fn f<T>(mut x: Foo<T>) {
let _: Pin<&mut Foo<T>> = pin!(x); //~ OK
let _: &Foo<T> = &x; //~ ERROR place has been moved
let _: &mut Foo<T> = &mut x; //~ ERROR place has been moved
let _move: Foo<T> = x; //~ ERROR place has been moved
} Then in your version,
What I expect is, the borrow checker should allow the original place to be mutably borrowed or moved after all pinned borrows expired (if we don't introduce fn ignore<T>(_: T) {}
struct Foo<T> { x: T }
fn f<T>(mut x: Foo<T>) {
{
let _pinned: Pin<&mut Foo<T>> = &pin mut x; //~ OK
let _: &Foo<T> = &x; //~ OK
// Mutable borrows or moves are not allowed during the pinned borrow is alive
let _: &mut Foo<T> = &mut x; //~ ERROR place has been pinned
let _move: Foo<T> = x; //~ ERROR place has been pinned
// Make sure `_pinned` is used
ignore(_pinned);
}
let _: &Foo<T> = &x; //~ OK
// Mutable borrows or moves are allowed again after the pinned borrow expires
let _: &mut Foo<T> = &mut x; //~ OK
let _move: Foo<T> = x; //~ OK
} |
e57789f
to
1c081ea
Compare
This can't be allowed. The contract with What the borrow checker must do here, then, is to flag the struct Foo<T> { x: T }
fn f<T>(mut x: Foo<T>) {
let _: &mut Foo<T> = &mut x; //~ OK
//~^ Before we pin the thing, it's OK for mutable references to
//~| exist to it.
//~|
//~| Note that the above mutable reference is dead at this point.
let _: Pin<&mut Foo<T>> = &pin mut x; //~ OK
//~^ Now we create a pinned mutable reference. This is allowed
//~| since the earlier mutable reference is dead. Once this
//~| pinned reference is created, we can no longer allow the
//~| pointed to memory to be invalidated or repurposed until
//~| after `drop` has been run, even after this pinned reference
//~| is dead.
//~|
//~| Note that the pinned reference is dead at this point.
let _: &Foo<T> = &x; //~ OK
//~^ We can allow shared references because they don't allow us
//~| to invalidate or repurpose the memory.
let _: Pin<&mut Foo<T>> = &pin mut x; //~ OK
//~^ We can allow a new pinned mutable reference for the same
//~| reason.
let _: Pin<&Foo<T>> = &pin const x; //~ OK
//~^ Or a new pinned shared reference, for that matter.
let _: &mut Foo<T> = &mut x; //~ ERROR place has been pinned
//~^ We can't allow a mutable reference to exist, though, because
//~| that would violate the contract.
let _move: Foo<T> = x; //~ ERROR place has been pinned
//~^ Neither can we allow the once-pinned thing to be moved.
} What the |
Oh, yes, I forgot that |
Yes, the intuition point is something we'll I'm sure consider later. We want to try it first in the way mentioned. |
What @traviscross said above sounds right to me. I would also mention that we want dispatch on |
☔ The latest upstream changes (presumably #138114) made this pull request unmergeable. Please resolve the merge conflicts. |
1c081ea
to
7010181
Compare
Some changes occurred to constck cc @fee1-dead |
@rustbot ready |
// Makes sure we can handle `&pin mut place` and `&pin const place` as sugar for | ||
// `unsafe { Pin::new_unchecked(&mut place) }` and `Pin::new(&place)`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wildly unsound, which gives me pause even about doing it on nightly, but the feature flag is marked as incomplete, and we're planning to follow-up with the borrow checker rules, so I suppose this is OK.
@rustbot author |
7010181
to
1c172ca
Compare
1c172ca
to
fceb223
Compare
According to this requirement, using struct Foo<T> { x: T }
fn f<T>(mut x: Foo<T>) {
{
let _: Pin<&mut Foo<T>> = &pin mut x; //~ OK
let _: &Foo<T> = &x; //~ OK
}
// The following code error is confusing.
// Because the pinned reference has disappeared,
// but mutable references still cannot be used.
let _: &mut Foo<T> = &mut x; //~ ERROR place has been pinned
let _move: Foo<T> = x; //~ ERROR place has been pinned
} However, the following approach seems much clearer and more consistent with the current habits of mutable and immutable references. struct Foo<T> { x: T }
fn f<T>(mut x: Foo<T>) {
let pin mut x = x;
{
let _: Pin<&mut Foo<T>> = &pin mut x; //~ OK
let _: &Foo<T> = &x; //~ OK
}
let _: &mut Foo<T> = &mut x; //~ ERROR place is pinned, like `&mut x` fail because `let x = 1` is immutable.
let _move: Foo<T> = x; //~ ERROR place is pinned
} This is just a beginner's perspective. The latter approach makes the code easier to understand for me, even without knowing some of the deeper underlying reasons. If there are any mistakes, I apologize in advance. Regards, |
@rustbot ready |
This PR implements part of #130494.
EDIT: It introduces
&pin mut $place
and&pin const $place
as sugars forstd::pin::pin!($place)
and its shared reference equivalent, except that$place
will not be moved when borrowing. The borrow check will be in charge of enforcing places cannot be moved or mutably borrowed since being pinned till dropped.Implementation steps:
&pin mut $place
and&pin const $place
syntaxes&pin mut|const
&pin mut|const
when needed