Implement pin-project in pattern matching for &pin mut|const T
#139751
Conversation
|
rustbot has assigned @compiler-errors. Use |
rustbot
added
A-attributes
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #139996) made this pull request unmergeable. Please resolve the merge conflicts. |
frank-king
force-pushed
the
feature/pin-project
branch
from
0ad1543 to
e9c97df
Compare
This comment has been minimized.
This comment has been minimized.
frank-king
force-pushed
the
feature/pin-project
branch
3 times, most recently
from
24e0b14 to
aa27a06
Compare
frank-king
changed the title
&pin mut|const T&pin mut|const T
This comment has been minimized.
This comment has been minimized.
frank-king
force-pushed
the
feature/pin-project
branch
from
aa27a06 to
c9ca4f8
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Some changes occurred to the CTFE machinery Some changes occurred in match checking cc @Nadrieril Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt The Miri subtree was changed cc @rust-lang/miri Some changes occurred in compiler/rustc_monomorphize/src/partitioning/autodiff.rs cc @ZuseZ4 rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer Some changes occurred in exhaustiveness checking cc @Nadrieril Some changes occurred in match lowering cc @Nadrieril Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
| pub fn builtin_deref(self, explicit: bool, tcx: TyCtxt<'tcx>) -> Option<Ty<'tcx>> { | ||
| match *self.kind() { | ||
| _ if let Some(boxed) = self.boxed_ty() => Some(boxed), | ||
| _ if let Some(pinned) = self.pinned_ty() | ||
| && let Ref(_, ty, _) = *pinned.kind() => | ||
| { | ||
| tcx.features().pin_ergonomics().then_some(ty) | ||
| } |
There was a problem hiding this comment.
This function is extremely widely used and very fundamental. I don't think it makes any sense to make it depend on tcx.features() -- even a stable crate calling code inside std that uses this unstable feature will encounter these types during codegen, and call builtin_deref on it. Things will go pretty badly if the behavior of builtin_deref differs across crates.
There was a problem hiding this comment.
Oh yes, and the pin_ergonomics is an incomplete feature.
Maybe it's better to add a new method named builtin_deref_unstable and use the new method instead when dealing with auto-derefence of Pin?
There was a problem hiding this comment.
I don't know how HIR/THIR use this methods, so -- no idea. If these things stick around as pointer types all the way to MIR, and the * MIR operator can be used on them, then the regular builtin_deref must understand them. There's no way to have "unstable MIR", stability gating must be done during MIR construction or earlier.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry for thinking to myself in the edit history for the previous comment. I think I have a better understanding now. Is there a reason that this needs to support MIR * working on &pin, rather than unwrapping the pin before dereferencing it in the emitted MIR? I feel like that might have less codebase-wide fallout, since it wouldn't need a broader definition of builtin_deref.
Edit: i.e. I think I'm suggesting matching on place_builder.field(FieldIdx(0), ref_ty).deref() rather than place_builder.deref(). Would probably want to encode that in the THIR somehow too if you go that route.
There was a problem hiding this comment.
I think I'm suggesting matching on
place_builder.field(FieldIdx(0), ref_ty).deref()rather thanplace_builder.deref()
I agree. That could avoid adding builtin deref logics for Pin<&T> or Pin<&mut T>.
oli-obk
added
S-waiting-on-author
|
It seems like you're implementing a sort of match ergonomics through I think pinnedness should be part of the binding mode instead of a separate notion. I haven't looked deeply yet but presumably this will raise questions similar to deref patterns and the recent match ergonomics changes. cc @dianne |
|
+1 to representing pinnedness as part of the binding mode. Regarding match ergonomics interactions, I think we'll either want explicit Regarding deref patterns interactions, we'll probably also want pin ergonomics for deref patterns eventually. I don't think that'll be a problem, if I understand what |
| let pinnedness = if pinnedness == Pinnedness::Pinned | ||
| || pat_info.pinnedness == Pinnedness::Pinned |
There was a problem hiding this comment.
Won't pat_info.pinnedness always be Pinnedness::Not here? At this point, expected is known either to be a ref or a pinned ref, and both of those implement Unpin, so if I understand correctly it'd've been set to Not by the redefinition of pat_info on line 480.
There was a problem hiding this comment.
Won't
pat_info.pinnednessalways bePinnedness::Nothere?
Nope, the current implementation is: if expected is an &pin reference, pinnedness in line 505 is Pinnedness::Pinned, and thus the pinnedness would be Pinnedness::Not.
In the next recursion invoked in line 551, if the inner_ty is Unpin, its pinnedness would be set Pinnedness::Not in line 480.
There was a problem hiding this comment.
Right, pinnedness there would be the pinnedness of the reference per line 505, but unless I'm mistaken pat_info.pinnedness on line 531 refers to the definition of pat_info on line 497, which has the same pinnedness field as the definition on line 480. On line 480, if we're taking the code path that gets us to line 531, expected should be &(mut) T or &pin (mut|const) T, both of which are Unpin, making pat_info.pinnedness be Not on line 531, effectively making lines 530-537 be let pinnedness = pinnedness;. So when recurring it would always use the pinnedness of the ref as the new pat_info.pinnedness when checking the pattern against inner_ty.
Sorry if I'm misunderstanding something. What's the intent behind checking pinnedness == Pinned || pat_info.pinnedness == Pinned rather than just using the pinnedness of the reference? And if it's something subtle, could you add a comment?
|
Another ergonomics question: is there a way to get a non-pinned by-shared-ref binding when matching through |
| // If the expected type is `Unpin`, then remove the pinnedness in `pat_info`. | ||
| let pat_info = if pat_info.pinnedness == Pinnedness::Pinned | ||
| && expected.is_unpin(self.tcx, self.typing_env(self.param_env)) | ||
| { | ||
| PatInfo { pinnedness: Pinnedness::Not, ..pat_info } |
There was a problem hiding this comment.
expected might not be inferred at this point; it's often not known until unifying it with the pattern's type1. I think this might result in cases where we'd maintain Pinnedness::Pinned when matching on types that have no pinning guarantees, so whether bindings are by-ref or by-pinned-ref would depend on inference. I don't have a great answer for how to solve this at the moment, but could you add tests for it? My only idea so far is to check the pattern before determining its bindings' pinnedness2.
Footnotes
-
At least in the general case. I'm not sure when this would come up when dealing with pins; you'd have to be matching on a
&pin const Tor&pin mut Twithout fully knowingT. I'm not familiar enough with pinning to say whether that actually happens. ↩ -
There'd still be some awkwardness with uninferred bindings, since they still may not be known after checking the pattern, but the only part of type-checking I'm familiar with is the rules for patterns, so I couldn't say if there's a way to deal with that. ↩
There was a problem hiding this comment.
Actually, would it make sense for it to be a compilation error if a binding's pinnedness is ambiguous? i.e. after checking the pattern, the pinnedness of each binding will depend on the types between it and the innermost &pin relative to it; if the trait solver can't say for sure that either at least one of them is Unpin or all of them are !Unpin, compilation fails. I'm not sure that's the best we can do, but it'd at least mean not baking trait solver ambiguities into the types bindings are given.
Does "match ergonomics" refer to rfcs#2005? I see it has been stabilized in 2018, and I'm not quite familiar with "ancient" Rust before (I started to learn Rust in 2021). My intuition is just based on the crate pin_project. Take an example from its doc: use std::pin::Pin;
use pin_project::pin_project;
#[pin_project(project = EnumProj)]
enum Enum<T, U> {
Pinned(#[pin] T),
Unpinned(U),
}
impl<T, U> Enum<T, U> {
fn method(self: Pin<&mut Self>) {
match self.project() {
EnumProj::Pinned(x) => {
let _: Pin<&mut T> = x;
}
EnumProj::Unpinned(y) = {
let _: &mut U = y;
}
}
}
}It uses the With #![feature(pin_ergonomics)]
enum Enum<T, U> {
// `#[pin]` is no longer needed, as we can infer from the trait bound whether `T` is `Unpin`.
Pinned(T),
Unpinned(U),
}
// `U: Unpin` is needed to inform the compiler that `U` can be projected to a normal reference.
impl<T, U: Unpin> Enum<T, U> {
fn method(&pin mut self) {
// `self.projection()` is no longer needed, as the compiler
// would understand how to project a `&pin mut Enum<T, U>`
match self {
// `EnumProj` is no longer needed
Enum::Pinned(x) => {
// for `T: ?Unpin`, it is projected to `&pin mut T`
let _: &pin mut T = x;
}
Enum::Unpinned(y) = {
// for `U: Unpin`, it is projected to `&mut U`
let _: &mut U = y;
}
}
}
}That's how I implemented this PR. |
That RFC is indeed pretty old, and isn't quite what's implemented in Rust 20241. I'm using "match ergonomics" loosely to mean a couple things (sorry for the jargon!):
From what I could tell, this PR supports the former of these but not the latter; for consistency with how matching on normal references works, I'd expect to be able to use explicit reference patterns to match on
It should indeed be possible to utilize the There might be additional subtleties/complications I'm not aware of, of course. I'm not deeply familiar with the trait solver, so I'm not totally sure how unambiguous you can guarantee its results to be. Hence my suggestion to raise an error when there's ambiguities. Footnotes
|
This comment was marked as duplicate.
This comment was marked as duplicate.
This already isn't true of Rust due to the behavior of method resolution. E.g.: |
| fn foo_mut<T, U: Unpin>(foo: Pin<&mut Foo<T, U>>) { | ||
| let Foo { x, y } = foo; | ||
| //[normal]~^ ERROR mismatched types | ||
| assert_pin_mut(x); | ||
| assert_ref_mut(y); | ||
| } |
There was a problem hiding this comment.
This is not, I think, the behavior that I'd expect. I would expect this:
fn f<T: Unpin>([x]: &pin mut [T; 1]) -> &pin mut T { x }That is, I would not expect the match ergonomics behavior to swap out a pinned reference for a mutable one.
Of course, if we have a pinned reference to a T: Unpin, we can always get a plain mutable reference:
fn f<T: Unpin>(x: Pin<&mut T>) -> &mut T { Pin::into_inner(x) }If we felt it was justified to make this more ergonomic and avoid going through into_inner, I'd suspect it'd make more sense to add a coercion for this rather than handling this as part of match ergonomics. In that case, we'd also then support:
fn f<T: Unpin>(x: Pin<&mut T>) -> &mut T { x }(When T: Unpin, the reverse coercion would indeed also be conceivable.)
There was a problem hiding this comment.
Is this maybe a shortcoming of relying on Unpin/!Unpin to tell which fields support pin-preserving projections? If I'm understanding right, [T; 1] is Unpin when T is Unpin; since that's the only information used to tell when pin-projection is okay, the current implementation can't distinguish it from cases like pointer-like types, where it's not safe to pin-project. I am admittedly learning as I go, though. Is there an up-to-date design doc for pin ergonomics? The closest I could find was the doc from the design meeting, but the design there is different, based on manual annotations for fields supporting pin-projection. Has there been recent development that's rendered that no longer necessary?
There was a problem hiding this comment.
That is, I would not expect the match ergonomics behavior to swap out a pinned reference for a mutable one.
Indeed, that's better than the current implementation, which avoids implicit unpinning.
I wonder if there are any explicit ways to get directly an &mut T binding (where T: Unpin) like the deref! pattern (which I read in the PatKind's docs). What if we have a core::pin::unpin! pattern that unpins an &pin mut T binding into an &mut T binding (if T: Unpin doesn't hold, report a compile error)
fn f<T: Unpin>(x: &pin mut T) {
let unpin!(_y) = x; // `unpin!(_y)` makes `_y` have type `&mut T`.
}There was a problem hiding this comment.
Looking at it now, doing it as a coercion still seems the most promising to me (rather than unpin! or similar), though I haven't yet carefully analyzed this for reasons we might not want to do that. We have many similar coercions around references, and coercing here would seem to fit reasonably well with those.
fn f1<T: Send>(x: &mut T) -> &T { x }
fn f2<T: Send>(x: &mut T) -> *mut T { x }
fn f3<T: Send>(x: &mut T) -> *const T { x }
fn f4<T: Send>(x: &mut T) -> &mut dyn Send { x }
fn f5<T: Send>(x: &mut T) -> &dyn Send { x }
fn f6<T: Send>(x: &mut T) -> *mut (dyn Send + '_) { x }
fn f7<T: Send>(x: &mut T) -> *const (dyn Send + '_) { x }
// Do we want?:
fn f8<T: Unpin>(x: &pin mut T) -> &mut T { x }
fn f9<T: Unpin>(x: &mut T) -> &pin mut T { x }
Do you mean something like this? fn f<T: Unpin + Copy>(x: &pin mut T, y: &mut T) {
let &pin mut _x = x; // gets `_x: T`
let &mut _y = y; // like this, which gets `_y: T`
}I think that could be possible and could be implemented in the next PRs (it's an independent feature and requires introducing extra syntax).
If you mind this inconsistency, I can revert this diagnostic syntax and reintroduce it later if any following PRs implement that feature. |
This PR implements part of #130494. It supports pin-project in pattern matching for
&pin mut|const T.Pin-projection by field access (i.e.
&pin mut|const place.field) is not fully supported yet since pinned-borrow is not ready (#135731).CC @traviscross