-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Added diagnostic/suggestion for core::pin::pin! in addition to Box::pin if Unpin isn't implemented #109988
Conversation
…in if Unpin isn't implemented
…in if Unpin isn't implemented
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
library/core/src/marker.rs
Outdated
@@ -823,7 +823,7 @@ unsafe impl<T: ?Sized> Freeze for &mut T {} | |||
/// [`pin` module]: crate::pin | |||
#[stable(feature = "pin", since = "1.33.0")] | |||
#[rustc_on_unimplemented( | |||
note = "consider using `Box::pin`", | |||
note = "consider using `core::pin::pin!`\nconsider using `Box::pin` if you want ownership of the pinned value", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the policy about mentioning core
in an diagnostic message? Should we mention the pin!
macro instead?
I'm not convinced about the "if you want ownership of the pinned value" part. The important difference between the two is the local storage. What about: "if you need to access the pinned value outside of the current scope"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I suppose the std::pin::pin! macro would be better
Also yeah, that message works better, it captures the "local pin" semantics far better. I'll make the changes and commit
@rustbot review |
@@ -823,7 +823,7 @@ unsafe impl<T: ?Sized> Freeze for &mut T {} | |||
/// [`pin` module]: crate::pin | |||
#[stable(feature = "pin", since = "1.33.0")] | |||
#[rustc_on_unimplemented( | |||
note = "consider using `Box::pin`", | |||
note = "consider using `std::pin::pin!`\nconsider using `Box::pin` if you need to access the pinned value outside of the current scope", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note = "consider using `std::pin::pin!`\nconsider using `Box::pin` if you need to access the pinned value outside of the current scope", | |
note = "consider using the `pin!` macro\nconsider using `Box::pin` if you need to access the pinned value outside of the current scope", |
Works for both std and no-std 😄
r=me with commits squashed
Whoops, I was trying to sort out some git stuff (git won't let me switch to the branch for this issue for some reason, git clone literally didn't clone it), and in the process of renaming the branch I accidentally closed it... Is it OK for me to create a new PR and r= you, @cjgillot ? Sorry😅 I re-made the pull request as #110259 |
Currently, if an operation on
Pin<T>
is performed that requiresT
to implementUnpin
, the diagnostic suggestion is to useBox::pin
("note: consider usingBox::pin
").As others have noted, using
pin!
avoids a heap allocation, so it should be suggested as well (and probably overBox::pin
whenever possible).This PR adds the diagnostic "consider using
Box::pin
if you want ownership of the pinned value", and updates tests to match.Addresses/attempts to address #109964