Skip to content
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

Add &pin (mut|const) T type position sugar #130635

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

eholk
Copy link
Contributor

@eholk eholk commented Sep 20, 2024

This adds parser support for &pin mut T and &pin const T references. These are desugared to Pin<&mut T> and Pin<&T> in the AST lowering phases.

This PR currently includes #130526 since that one is in the commit queue. Only the most recent commits (bd45002 and following) are new.

Tracking:

r? @compiler-errors

@rustbot rustbot added 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 Sep 20, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 20, 2024

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@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 Sep 20, 2024
@eholk
Copy link
Contributor Author

eholk commented Sep 20, 2024

I'm missing a test to make sure that &pin i32 doesn't parse. I'm also failing that test, so let me add that and fix it.

@rustbot author

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

compiler/rustc_ast/src/ast.rs Show resolved Hide resolved
src/tools/rustfmt/src/types.rs Outdated Show resolved Hide resolved
@compiler-errors
Copy link
Member

@rustbot author

Remember to switch back to S-waiting-on-review when it's rebased and updated

@bors
Copy link
Contributor

bors commented Sep 23, 2024

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

@eholk
Copy link
Contributor Author

eholk commented Sep 23, 2024

@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 Sep 23, 2024
@traviscross traviscross changed the title Add &pin (mut|const) T sugar Add &pin (mut|const) T type position sugar Sep 24, 2024
@traviscross
Copy link
Contributor

@rustbot labels +I-lang-nominated +I-lang-easy-decision

In Rust 1.41, we broke this program...

macro_rules! m {
    ($x:expr) => { compile_error!("bad") };
    ($($t:tt)*) => {}
}
m!(&raw const ());

...even though we didn't end up stabilizing &raw const .. until Rust 1.82.

Then we broke this in Rust 1.77:

macro_rules! m {
    ($x:ty) => { compile_error!("bad") };
    ($($t:tt)*) => {}
}
m!(impl const Sized);

(We haven't stabilized const trait modifiers yet.) And then in Rust 1.78, we broke:

macro_rules! m {
    ($x:ty) => { compile_error!("bad") };
    ($($t:tt)*) => {}
}
m!(impl async Sized);

(We haven't stabilized async trait modifiers yet.)

Now we plan to break this program when this PR lands:

macro_rules! m {
    ($x:ty) => { compile_error!("bad") };
    ($($t:tt)*) => {}
}
m!(&pin const ());

This breakage is believed to be theoretical-only. Does this sound OK to us?

(See this thread for background.)

@rustbot rustbot added I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy. I-lang-nominated Nominated for discussion during a lang team meeting. labels Sep 25, 2024
@compiler-errors
Copy link
Member

compiler-errors commented Sep 25, 2024

The breakage specifically represents an inherent limitation to the "macro follow-set" formulation which is supposed to make us more resilient against breakages due to extensions to the grammar like this.

Given two macro matcher arms:

  • ($ty:ty) => ...
  • (($tt:tt)*) => ...

And given tokens like:

  • & pin mut [...more tokens may follow...]

On nightly today, &pin gets parsed as a type. However, we run out of matchers but still have tokens left (the mut token is next), so we fall through to the next arm. Since it's written like ($tt:tt)*, everything is allowed, and we match the second arm successfully...

I think that's weird, because if this second arm were written like $ty:ty mut, that would be illegal, since mut is not in the follow-set of the :ty matcher. Thus, we can use :tt matchers to observe whether the compiler actually parses things not in our grammar that should otherwise be protected against, which seems pretty gross.

I think this breakage is purely theoretical, though. I would like to hear any suggestions to close this hole and make macros a bit more resilient, but nothing really comes to mind :(

@eholk
Copy link
Contributor Author

eholk commented Sep 26, 2024

Given that this changes behavior even without the pin_ergonomics feature flag, I'd like to take a little time to find a workaround. I'm not particularly optimistic since this has been challenging in the past, but I'd like to at least take a look.

@rustbot author

@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 Sep 26, 2024
@tmandry
Copy link
Member

tmandry commented Sep 26, 2024

We are going to need pin in expressions, too, right?

Today we have let x = pin!(foo);
Tomorrow we might want let x = &pin mut foo;

Looks like you are relying on &pin const and &pin mut to disambiguate, which worked for raw references so I guess it can work here.

We can keep using this or the pin macro until the next edition and reserve a keyword then (but we'll have to figure out what to do about Box::pin).

@traviscross
Copy link
Contributor

We are going to need pin in expressions, too, right?

Yes, so we'd also be intending to break:

//@ check-pass
macro_rules! m {
    ($x:expr) => { compile_error!("bad") };
    ($($t:tt)*) => {}
}
m!(&pin const ());

@Noratrieb
Copy link
Member

I would like to hear any suggestions to close this hole and make macros a bit more resilient, but nothing really comes to mind :(

a bit off-topic for the thread but I believe a solution to this would be the following new logic:

  • after the end of a macro matcher arm has been reached
  • and there are still input tokens remaining
  • and if the last part of the matcher is a metavar
  • ensure that the first remaining token is in the follow set of this metavar
  • if it is, move on to the next arm
  • if it is not, emit an error

What this semantically does is strengthen the "commit to fully matching metavars or error" behavior such that it extends past the end.
I don't know how many macros rely on this, but it seems like emitting an FCW (instead of error) on such macro invocations would find all these cases and ensure that the follow-set logic is actually robust past the end.
But imo this shouldn't block this PR (which should probably just ship as-is) and can be done separately.

@traviscross
Copy link
Contributor

That seems plausible to me. I've filed and nominated...

...for the proposal for that FCW.

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated +T-lang +relnotes

We discussed this in the meeting today. We agreed that this seems like theoretical-only breakage and is OK to move forward as far as lang is concerned. We'll mention this in the release notes as it is technically breaking, and in that way we'll help to make this information available to those doing the beta crater runs, just in case anything is found there.

Our general sentiment was that anyone using match arms as an oracle to detect what gets parsed in other arms is probably asking for trouble; it's unwise to rely on the subtleties of this, given the limitations of the follow-set behavior with respect to the interaction between arms, and we're interested in better ways to explicitly reserve this space.

@rustbot rustbot added relnotes Marks issues that should be documented in the release notes of the next release. T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed I-lang-nominated Nominated for discussion during a lang team meeting. labels Oct 2, 2024
@eholk
Copy link
Contributor Author

eholk commented Oct 2, 2024

Based on the lang team discussion, I think we should move forward on this PR and look at solving the problem more generally separately (perhaps with #131025).

@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 Oct 2, 2024
@bors
Copy link
Contributor

bors commented Oct 3, 2024

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

@bors
Copy link
Contributor

bors commented Oct 5, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy. relnotes Marks issues that should be documented in the release notes of the next release. 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. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants