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 an attribute that makes the spans from a macro edition 2021, and fix pin on edition 2024 with it #138717

Merged
merged 4 commits into from
Mar 21, 2025

Conversation

jdonszelmann
Copy link
Contributor

@jdonszelmann jdonszelmann commented Mar 19, 2025

Fixes a regression, see issue below. This is a temporary fix, super let is the real solution.

Closes #138596

@rustbot
Copy link
Collaborator

rustbot commented Mar 19, 2025

r? @Noratrieb

rustbot has assigned @Noratrieb.
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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 19, 2025
@jdonszelmann jdonszelmann changed the title Add an attribute that makes the spans from a macro edition 2021 Add an attribute that makes the spans from a macro edition 2021, and fix pin on edition 2024 with it Mar 19, 2025
@WaffleLapkin
Copy link
Member

r? WaffleLapkin
r=me with green CI

@rustbot rustbot assigned WaffleLapkin and unassigned Noratrieb Mar 19, 2025
@rust-log-analyzer

This comment was marked as resolved.

@petrochenkov

This comment was marked as off-topic.

@Noratrieb

This comment was marked as resolved.

@petrochenkov
Copy link
Contributor

@petrochenkov do you have a constructive suggestion for this issue or just unconstructive complaints? The latter is not welcome.

I need to think.
(Not blocking since this is a regression fix.)

@Noratrieb

This comment has been minimized.

@petrochenkov
Copy link
Contributor

One alternative not requiring compiler hacks is to put it into a separate 2021 edition crate and then reexport it from libcore, but that may (or may not) cause some larger build/infrastructure issues.

@workingjubilee
Copy link
Member

Probably will. And no one wants to spend a day debugging the CI issues when it does. And speaking from my POV as libs reviewer, I strongly prefer core's current dependency graph to it having any dependencies at all.

@m-ou-se
Copy link
Member

m-ou-se commented Mar 19, 2025

What are you people doing 🤦
I really hope this is temporary.

It really should be. If this isn't temporary, it means that Rust 2024 isn't as expressive as Rust 2021. That would be a problem!

Opened #138718 to track that.

@m-ou-se
Copy link
Member

m-ou-se commented Mar 19, 2025

One alternative not requiring compiler hacks is to put it into a separate 2021 edition crate and then reexport it from libcore, but that may (or may not) cause some larger build/infrastructure issues.

Definitely considered that. But adding a dependency to core isn't trivial. And unfortunately we don't have :expr2024 in Rust 2021, so it wouldn't just be moving the enitr pin macro to a 2021 crate.

Luckily this attribute is very simple to implement, and thus simple to remove again. (It just sets the SyntaxExtension::edition field.)

@theemathas
Copy link
Contributor

Is it reasonable to try running crater with the identity() fix to see how much code breaks? Broken code probably would need to look like pin!(&mut temporary()), which is kinda silly, and hopefully doesn't come up often enough to be an issue.

@m-ou-se
Copy link
Member

m-ou-se commented Mar 19, 2025

Is it reasonable to try running crater with the identity() fix to see how much code breaks? Broken code probably would need to look like pin!(&mut temporary()), which is kinda silly, and hopefully doesn't come up often enough to be an issue.

Considering that pin!() is used often in aync application code (not just libraries), I think crater might not be very representative, as there might be a lot of uses in code that is not on (public) github or crates.io.

@BoxyUwU

This comment was marked as off-topic.

@m-ou-se
Copy link
Member

m-ou-se commented Mar 20, 2025

@bors r=WaffleLapkin

@bors
Copy link
Contributor

bors commented Mar 20, 2025

📌 Commit 0577300 has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2025
Comment on lines +88 to +97
#[test]
#[cfg(not(bootstrap))]
fn temp_lifetime() {
// Check that temporary lifetimes work as in Rust 2021.
// Regression test for https://github.com/rust-lang/rust/issues/138596
match std::pin::pin!(foo(&mut 0)) {
_ => {}
}
async fn foo(_: &mut usize) {}
}
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla Mar 20, 2025

Choose a reason for hiding this comment

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

When I tried to work on this with my failed attempt, I thought of extra tests which seem important for pin, so you may want to add them if you like them 🙂:

fn transitive_extension {
    async fn temporary() {}

    // `pin!` witnessed in the wild being used like this, even if it yields
    // a `Pin<&mut &mut impl Unpin>`; it does work because `pin!`
    // happens to transitively extend the lifespan of `temporary()`.
    let p = pin!(&mut temporary());
    let _use = p;
}

as well as the following things which have to fail to compile to guarantee the soundness of pin!: pin! needs to consume ownership of the argument given to it, by having moved that value to its own super let local, or by having funneled it through a value expression such as { $value }.

  • struct NotCopy<T>(T);
    #[allow(unused_mut)]
    let mut pointee = NotCopy(core::marker::PhantomPinned);
    pin!(pointee);
    let _moved = pointee; // Error, use of moved value.
  • struct NotCopy<T>(T);
    let mut pointee = NotCopy(core::marker::PhantomPinned);
    pin!(*&mut pointee); // Error, cannot move out of `&mut`, `NotCopy` is not `Copy`.
    let _moved = pointee;

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Would you mind putting that in a PR?

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 21, 2025
…pkin

Add an attribute that makes the spans from a macro edition 2021, and fix pin on edition 2024 with it

Fixes a regression, see issue below. This is a temporary fix, super let is the real solution.

Closes rust-lang#138596
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2025
Rollup of 10 pull requests

Successful merges:

 - rust-lang#138364 (ports the compiler test cases to new rust_intrinsic format)
 - rust-lang#138570 (add `naked_functions_target_feature` unstable feature)
 - rust-lang#138623 ([bootstrap] Use llvm_runtimes for compiler-rt)
 - rust-lang#138627 (Autodiff cleanups)
 - rust-lang#138669 (tests: accept some noise from LLVM 21 in symbols-all-mangled)
 - rust-lang#138706 (Improve bootstrap git modified path handling)
 - rust-lang#138709 (Update GCC submodule)
 - rust-lang#138717 (Add an attribute that makes the spans from a macro edition 2021, and fix pin on edition 2024 with it)
 - rust-lang#138721 (Use explicit cpu in some asm and codegen tests.)
 - rust-lang#138728 (Update `compiler-builtins` to 0.1.152)

r? `@ghost`
`@rustbot` modify labels: rollup

try-job: aarch64-apple
try-job: x86_64-msvc-1
try-job: x86_64-gnu
try-job: dist-i586-gnu-i586-i686-musl
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2025
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#138364 (ports the compiler test cases to new rust_intrinsic format)
 - rust-lang#138570 (add `naked_functions_target_feature` unstable feature)
 - rust-lang#138623 ([bootstrap] Use llvm_runtimes for compiler-rt)
 - rust-lang#138627 (Autodiff cleanups)
 - rust-lang#138669 (tests: accept some noise from LLVM 21 in symbols-all-mangled)
 - rust-lang#138706 (Improve bootstrap git modified path handling)
 - rust-lang#138709 (Update GCC submodule)
 - rust-lang#138717 (Add an attribute that makes the spans from a macro edition 2021, and fix pin on edition 2024 with it)
 - rust-lang#138721 (Use explicit cpu in some asm and codegen tests.)
 - rust-lang#138728 (Update `compiler-builtins` to 0.1.152)

r? `@ghost`
`@rustbot` modify labels: rollup

try-job: aarch64-apple
try-job: x86_64-msvc-1
try-job: x86_64-gnu
try-job: dist-i586-gnu-i586-i686-musl
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2025
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#138364 (ports the compiler test cases to new rust_intrinsic format)
 - rust-lang#138570 (add `naked_functions_target_feature` unstable feature)
 - rust-lang#138623 ([bootstrap] Use llvm_runtimes for compiler-rt)
 - rust-lang#138627 (Autodiff cleanups)
 - rust-lang#138669 (tests: accept some noise from LLVM 21 in symbols-all-mangled)
 - rust-lang#138706 (Improve bootstrap git modified path handling)
 - rust-lang#138709 (Update GCC submodule)
 - rust-lang#138717 (Add an attribute that makes the spans from a macro edition 2021, and fix pin on edition 2024 with it)
 - rust-lang#138721 (Use explicit cpu in some asm and codegen tests.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7c2475e into rust-lang:master Mar 21, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 21, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2025
Rollup merge of rust-lang#138717 - jdonszelmann:pin-macro, r=WaffleLapkin

Add an attribute that makes the spans from a macro edition 2021, and fix pin on edition 2024 with it

Fixes a regression, see issue below. This is a temporary fix, super let is the real solution.

Closes rust-lang#138596
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pin macro no longer lifetime extends argument