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

Suggest pin::pin! over Box::pin in diagnostics #109964

Closed
yoshuawuyts opened this issue Apr 5, 2023 · 4 comments · Fixed by #110259
Closed

Suggest pin::pin! over Box::pin in diagnostics #109964

yoshuawuyts opened this issue Apr 5, 2023 · 4 comments · Fixed by #110259
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Apr 5, 2023

Code

Consider the following code, taken from the async book (playground):

pub fn main() {
    let mut test1 = Test::new("test1");
    let mut test1 = unsafe { Pin::new_unchecked(&mut test1) };
    Test::init(test1.as_mut());

    let mut test2 = Test::new("test2");
    let mut test2 = unsafe { Pin::new_unchecked(&mut test2) };
    Test::init(test2.as_mut());

    println!("a: {}, b: {}", Test::a(test1.as_ref()), Test::b(test1.as_ref()));
    std::mem::swap(test1.get_mut(), test2.get_mut());
    println!("a: {}, b: {}", Test::a(test2.as_ref()), Test::b(test2.as_ref()));
}

Current output

error[E0277]: `PhantomPinned` cannot be unpinned
  --> src/main.rs:47:26
   |
47 |     std::mem::swap(test1.get_mut(), test2.get_mut());
   |                          ^^^^^^^ within `Test`, the trait `Unpin` is not implemented for `PhantomPinned`
   |
   = note: consider using `Box::pin`
note: required because it appears within the type `Test`

Desired output

error[E0277]: `PhantomPinned` cannot be unpinned
  --> src/main.rs:47:26
   |
47 |     std::mem::swap(test1.get_mut(), test2.get_mut());
   |                          ^^^^^^^ within `Test`, the trait `Unpin` is not implemented for `PhantomPinned`
   |
   = note: consider using `core::pin::pin!`
note: required because it appears within the type `Test`

Rationale and extra context

Local pin construction was stabilized in Rust 1.68. We should update the diagnostics to suggest this instead of the more expensive Box::pin call.

Other cases

No response

Anything else?

cc/ @wg-async

This is probably a "good first issue" as well. If someone can tag it as such?

@yoshuawuyts yoshuawuyts added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 5, 2023
@taiki-e
Copy link
Member

taiki-e commented Apr 5, 2023

In some cases, Box::pin works but pin! does not (e.g., if you want ownership of the pinned future), so I think it's better to suggest both.

note: consider using `core::pin::pin!` or `Box::pin`

Or something like:

note: consider using `core::pin::pin!`
note: consider using `Box::pin` if you want ownership of the pinned value

@Noratrieb Noratrieb added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Apr 5, 2023
@ndrewxie
Copy link
Contributor

ndrewxie commented Apr 5, 2023

@rustbot claim

@ndrewxie
Copy link
Contributor

ndrewxie commented Apr 6, 2023

Alright, the suggestion has been added in #109988. IDK how to link a PR to an issue though, should I just reference the issue in the description of the PR?

@jyn514
Copy link
Member

jyn514 commented Apr 6, 2023

IDK how to link a PR to an issue though, should I just reference the issue in the description of the PR

if you put "fixes #109964" in the PR description, it will automatically close this issue when the PR is merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants