Skip to content

Conversation

frank-king
Copy link
Contributor

@frank-king frank-king commented Apr 13, 2025

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

@rustbot
Copy link
Collaborator

rustbot commented Apr 13, 2025

r? @compiler-errors

rustbot has assigned @compiler-errors.
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

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 13, 2025
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Apr 18, 2025

☔ The latest upstream changes (presumably #139996) made this pull request unmergeable. Please resolve the merge conflicts.

@frank-king frank-king force-pushed the feature/pin-project branch from 0ad1543 to e9c97df Compare April 20, 2025 12:53
@rustbot rustbot added the F-autodiff `#![feature(autodiff)]` label Apr 20, 2025
@rust-log-analyzer

This comment has been minimized.

@frank-king frank-king force-pushed the feature/pin-project branch 3 times, most recently from 24e0b14 to aa27a06 Compare April 20, 2025 13:47
@frank-king frank-king changed the title WIP: implement pin-project in pattern matching for &pin mut|const T Implement pin-project in pattern matching for &pin mut|const T Apr 20, 2025
@rust-log-analyzer

This comment has been minimized.

@frank-king frank-king force-pushed the feature/pin-project branch from aa27a06 to c9ca4f8 Compare April 20, 2025 14:22
@rust-log-analyzer

This comment has been minimized.

@frank-king frank-king marked this pull request as ready for review April 20, 2025 14:39
@rustbot
Copy link
Collaborator

rustbot commented Apr 20, 2025

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred in match checking

cc @Nadrieril

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

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

@oli-obk oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 22, 2025
@Nadrieril
Copy link
Member

It seems like you're implementing a sort of match ergonomics through &pin? What stage is this feature at?

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

@dianne
Copy link
Contributor

dianne commented Apr 24, 2025

+1 to representing pinnedness as part of the binding mode.

Regarding match ergonomics interactions, I think we'll either want explicit &pin patterns too or logic for using & to match on pinned refs (sorry if that's planned already! I couldn't find it). Writing out the &s leading up to a binding enables making by-value bindings of Copy types; there's not a general way to do that in the pattern without explicit & patterns. Likewise, some patterns (such as bindings) don't automatically peel references from the scrutinee, so &pin could be useful there too.
The diagnostic pretty-printing for exhaustiveness checking uses the &pin const and &pin mut syntax in patterns; without explicit &pin const and &pin mut patterns, those might need adjusting.

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 Pin<Ptr> means (morally, their pointee's address not changing means calling Deref::deref on a Pin<Ptr> returns a reference that could be wrapped in a pin, right? I'm not too familiar with pinning). Edit: I did a bit more reading; that invariant does seem to exist, so pin-projection ergonomics for deref patterns should be fine conceptually (I think). Not sure how it'd look implementation-wise, since DerefMut wouldn't always work. We'd morally want something more like matching through Pin::as_deref_mut than matching through DerefMut::deref_mut. I'm thinking we could call <Ptr as DerefMut>::deref_mut(&mut pin.pinned) and trust that pinnedness in binding modes will keep everything safe.

@dianne
Copy link
Contributor

dianne commented Apr 24, 2025

Another ergonomics question: is there a way to get a non-pinned by-shared-ref binding when matching through &pin const T or &pin mut T if no intervening types implement Unpin? If I understand correctly, this PR would always result in &pin const bindings in that case. I'm not sure this is an issue (at least not one that needs to be designed around) but I think that's the one case where the binding could be either &pin const or &, so there's a slight awkwardness when the user wants a reference and has to use &* or Pin::get_ref.

@frank-king
Copy link
Contributor Author

frank-king commented Apr 26, 2025

Regarding match ergonomics interactions, I think we'll either want explicit &pin patterns too or logic for using & to match on pinned refs

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 #[pin] attribute to determine whether to project a field to a pinned reference Pin<&{mut} T> or a normal reference &{mut} T. That's because the proc-macro doesn't know whether such a field is Unpin. However, rustc knows it (from the U: Unpin trait bound, I think), so we can leverage such information to determine whether to project to a pinned reference or a normal reference. (That may conflict with your words "expected might not be inferred at this point; it's often not known until unifying it with the pattern's type" in #139751 (comment), and please correct me if I'm wrong)

With pin_ergonomics enabled, I expect it to be:

#![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.

@dianne
Copy link
Contributor

dianne commented Apr 26, 2025

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

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!):

  • The ability to omit explicit reference patterns when matching on reference types; this allows binding by reference without explicit binding mode annotations. As I understand it, that is what this PR implements (with the addition of by-pinned-reference bindings).
  • The ability to add explicit reference patterns to bind Copy types by-value. Currently in Rust 2024, this requires explicitly writing out all reference patterns leading up to a binding. Tracking Issue for match ergonomics 2024 (RFC 3627) #123076 is tracking changes to allow more flexibility with where reference patterns are written, for more "ergonomic" control of the binding mode.

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 &pin (const|mut) T as well. Plus, that syntax is used (I think) by the match exhaustiveness diagnostics this PR implements for pinned refs, and I'd expect any patterns it outputs to be writable by users too.

However, rustc knows it (from the U: Unpin trait bound, I think), so we can leverage such information to determine whether to project to a pinned reference or a normal reference. (That may conflict with your words "expected might not be inferred at this point; it's often not known until unifying it with the pattern's type" in #139751 (comment), and please correct me if I'm wrong)

It should indeed be possible to utilize the U: Unpin trait bound, and at least as far as I can tell, that does seem like the smoothest way to do it from a user-facing perspective2. The tricky thing I was referring to is if you're matching on something that doesn't have a fully specified type at the point you're matching on it, rust's type-checker will try to infer the type from the patterns you use. However, it doesn't know what type the pattern has until the big match on the pattern's kind field. In this case, expected will be an unknown "inference variable" (or have inference variables in it) at the point you ask whether it's Unpin, which (in this specific case of expected being uninferred) will always conservatively return false even if you'll later learn that expected is Unpin once you learn the pattern's type.

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

  1. Rust 2024's current match ergonomics for references is specified in Tracking issue for Rust 2024: Match ergonomics rules 1C/2C #131414 (edit: also the edition guide as pointed out below). This was designed to be future-compatible with Tracking Issue for match ergonomics 2024 (RFC 3627) #123076 but easier to stabilize in time for the edition.

  2. The one gotcha I can think of besides what I said before is that if you have a type parameter T, you have to assume it's not Unpin when type-checking, but if you perform automatic source code transformations to substitute in a type that is Unpin, it can result in changes in the pinnedness of bindings, which can cause type errors. Thus substituting in a specific type for a type parameter at the source code level would no longer be a well-typedness-preserving operation. But I'm not sure there's really a way around that other than requiring explicit annotations. Tracking Issue for match ergonomics 2024 (RFC 3627) #123076 has a similar issue when trying to get it working on pre-2024 Editions in a backwards-compatible way.

@traviscross

This comment was marked as duplicate.

@frank-king
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 25, 2025
Copy link
Member

@Nadrieril Nadrieril left a comment

Choose a reason for hiding this comment

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

Ok, this looks good now, wtih three caveats:

  • Please remove the InheritedRefMatchRule::EatInner bit I commented on, it does nothing on stable rust and we'll have to be more careful about how this interacts with match ergonomics 2.0 later;
  • I'm requesting some changes to the tests;
  • I cannot review the part about negative impls. If at all possible split it to another PR so that we can merge this, otherwise we can ask for a second reviewer to look at it.

};

#[cfg(pin_ergonomics)]
let Foo { x, y } = foo;
Copy link
Member

Choose a reason for hiding this comment

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

Could you test the type of the bindings like you do in the other tests? Could you also remove the cfg so we can see what happens with just deref_patterns (I think it should work, but seeing the error is good too).

Copy link
Contributor Author

@frank-king frank-king Aug 3, 2025

Choose a reason for hiding this comment

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

I uncommented this, but got an error of cannot move out of a shared reference (under deref_patterns feature only). Seems that x and y have by-value bindings. Do you know if this behaviour is expected?

Comment on lines 2625 to 2780
if let ty::Ref(_, _, r_mutbl) = *expected.kind()
&& pat_mutbl <= r_mutbl
{
let expected_ref_or_pinned_ref = || {
if self.tcx.features().pin_ergonomics()
&& let Some(ty::Ref(_, _, r_mutbl)) =
expected.pinned_ty().map(|ty| *ty.kind())
&& pat_mutbl <= r_mutbl
{
return Some((Pinnedness::Pinned, r_mutbl));
}
if let ty::Ref(_, _, r_mutbl) = *expected.kind()
&& pat_mutbl <= r_mutbl
{
return Some((Pinnedness::Not, r_mutbl));
}
None
};
if let Some((_, r_mutbl)) = expected_ref_or_pinned_ref() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused, this should be allowing let &mut foo: &pin mut T, but you have a test that shows that this gives a type mismatch error. Do you understand why? If not that, what's the purpose of this change here?

Copy link
Member

Choose a reason for hiding this comment

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

Oh it's that this applies only to InheritedRefMatchRule::EatInner which is behavior gated under ref_pat_eat_one_layer_2024_structural. You have to support EatBoth for stable rust (you can probably ignore the other two variants for now, they're for feature gates). Anyway I think it's better not to allow &mut x to eat a &pin mut T for now, let's leave this to a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh it's that this applies only to InheritedRefMatchRule::EatInner which is behavior gated under ref_pat_eat_one_layer_2024_structural.

Thanks. Actually, I don't understand InheritedRefMatchRule very well, but it works now as your suggestion.

Anyway I think it's better not to allow &mut x to eat a &pin mut T for now, let's leave this to a future PR.

I think it is reasonable to allow &mut x to eat a &pin mut T if T: Unpin, but I agree that it can be resolved in a future PR.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2025
@rust-log-analyzer

This comment has been minimized.

@frank-king frank-king force-pushed the feature/pin-project branch from c33c6d8 to a4565b1 Compare August 3, 2025 14:16
@bors
Copy link
Collaborator

bors commented Aug 7, 2025

☔ The latest upstream changes (presumably #145043) made this pull request unmergeable. Please resolve the merge conflicts.

@frank-king frank-king force-pushed the feature/pin-project branch from a4565b1 to 646e905 Compare August 10, 2025 03:33
@frank-king
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 10, 2025
@bors
Copy link
Collaborator

bors commented Sep 18, 2025

☔ The latest upstream changes (presumably #146728) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot
Copy link
Collaborator

rustbot commented Sep 27, 2025

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

@rustbot rustbot added the T-rustfmt Relevant to the rustfmt team, which will review and decide on the PR/issue. label Sep 27, 2025
@rustbot

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Sep 27, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@frank-king
Copy link
Contributor Author

I reimplemented the projection rules according to the Alternative B part: using a #[pin_project] attribute to indicate that a type can be structurally pinned.

Checks for manual implementation of Unpin is not implemented yet.

@rust-log-analyzer

This comment has been minimized.

@traviscross traviscross added the I-lang-radar Items that are on lang's radar and will need eventual work or consideration. label Sep 27, 2025
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling tendril v0.4.3
error: cannot find attribute `pin` in this scope
  --> /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tower-0.4.13/src/util/either.rs:24:9
   |
24 |     A(#[pin] A),
   |         ^^^
   |
help: `pin` is an attribute that can be used by the derive macro `__PinProjectInternalDerive`, you might be missing a `derive` attribute
   |
22 + #[derive(__PinProjectInternalDerive)]
23 | pub enum Either<A, B> {
   |

error: cannot find attribute `pin` in this scope
  --> /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tower-0.4.13/src/util/either.rs:26:9
   |
26 |     B(#[pin] B),
   |         ^^^
   |
help: `pin` is an attribute that can be used by the derive macro `__PinProjectInternalDerive`, you might be missing a `derive` attribute
   |
22 + #[derive(__PinProjectInternalDerive)]
23 | pub enum Either<A, B> {
   |

error[E0659]: `pin_project` is ambiguous
  --> /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tower-0.4.13/src/util/either.rs:20:3
   |
20 | #[pin_project(project = EitherProj)]
   |   ^^^^^^^^^^^ ambiguous name
   |
   = note: ambiguous because of a name conflict with a builtin attribute
   = note: `pin_project` could refer to a built-in attribute
note: `pin_project` could also refer to the attribute macro imported here
  --> /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tower-0.4.13/src/util/either.rs:6:5
---

error[E0658]: the `#[pin_project]` attribute is an experimental feature
  --> /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tower-0.4.13/src/util/either.rs:20:1
   |
20 | #[pin_project(project = EitherProj)]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #130494 <https://github.com/rust-lang/rust/issues/130494> for more information
   = help: add `#![feature(pin_ergonomics)]` to the crate attributes to enable
   = note: this compiler was built on 2025-09-27; consider upgrading it if it is out of date

error: expected a literal (`1u8`, `1.0f32`, `"string"`, etc.) here, found `EitherProj`
  --> /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tower-0.4.13/src/util/either.rs:20:25
   |
20 | #[pin_project(project = EitherProj)]
   |                         ^^^^^^^^^^
   |
help: surround the identifier with quotation marks to make it into a string literal
   |
20 | #[pin_project(project = "EitherProj")]
   |                         +          +

error[E0599]: no method named `project` found for struct `Pin<&mut either::Either<A, B>>` in the current scope
  --> /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tower-0.4.13/src/util/either.rs:69:20
   |
69 |         match self.project() {
   |                    ^^^^^^^ method not found in `Pin<&mut either::Either<A, B>>`

error[E0282]: type annotations needed
  --> /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tower-0.4.13/src/util/either.rs:70:71
   |
70 |             EitherProj::A(fut) => Poll::Ready(Ok(ready!(fut.poll(cx)).map_err(Into::into)?)),
   |                                                                       ^^^^^^^ cannot infer type

error[E0282]: type annotations needed
  --> /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tower-0.4.13/src/util/either.rs:71:71
   |
71 |             EitherProj::B(fut) => Poll::Ready(Ok(ready!(fut.poll(cx)).map_err(Into::into)?)),
   |                                                                       ^^^^^^^ cannot infer type

[RUSTC-TIMING] tendril test:false 0.787
   Compiling dtoa-short v0.3.5
error[E0433]: failed to resolve: use of undeclared type `EitherProj`
  --> /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tower-0.4.13/src/util/either.rs:70:13
   |
70 |             EitherProj::A(fut) => Poll::Ready(Ok(ready!(fut.poll(cx)).map_err(Into::into)?)),
   |             ^^^^^^^^^^ use of undeclared type `EitherProj`

error[E0433]: failed to resolve: use of undeclared type `EitherProj`
  --> /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tower-0.4.13/src/util/either.rs:71:13
   |
71 |             EitherProj::B(fut) => Poll::Ready(Ok(ready!(fut.poll(cx)).map_err(Into::into)?)),
   |             ^^^^^^^^^^ use of undeclared type `EitherProj`

Some errors have detailed explanations: E0282, E0433, E0599, E0658, E0659.
For more information about an error, try `rustc --explain E0282`.
[RUSTC-TIMING] tower test:false 1.047
error: could not compile `tower` (lib) due to 10 previous errors
---
##[endgroup]
[TIMING:start] tool::LibcxxVersionTool { target: x86_64-unknown-linux-gnu }
[TIMING:end] tool::LibcxxVersionTool { target: x86_64-unknown-linux-gnu } -- 0.001
[TIMING:start] toolstate::ToolStateCheck {  }
ERROR: Tool `book` was not recorded in tool state.
ERROR: Tool `nomicon` was not recorded in tool state.
ERROR: Tool `reference` was not recorded in tool state.
ERROR: Tool `rust-by-example` was not recorded in tool state.
ERROR: Tool `edition-guide` was not recorded in tool state.
ERROR: Tool `embedded-book` was not recorded in tool state.
Bootstrap failed while executing `test --stage 2 check-tools`
Build completed unsuccessfully in 0:00:00
  local time: Sat Sep 27 07:50:01 UTC 2025
  network time: Sat, 27 Sep 2025 07:50:02 GMT
##[error]Process completed with exit code 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) F-autodiff `#![feature(autodiff)]` I-lang-radar Items that are on lang's radar and will need eventual work or consideration. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustfmt Relevant to the rustfmt team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.