Skip to content

Suggest as_mut when Pin<T> is used after move #93181

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 21 additions & 12 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,11 +235,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
);
}
}
FnSelfUseKind::Normal {
self_arg,
implicit_into_iter,
is_option_or_result,
} => {
FnSelfUseKind::Normal { self_arg, implicit_into_iter, self_name } => {
if implicit_into_iter {
err.span_label(
fn_call_span,
Expand Down Expand Up @@ -287,14 +283,27 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
),
);
}
if is_option_or_result && maybe_reinitialized_locations.is_empty() {
err.span_suggestion_verbose(
fn_call_span.shrink_to_lo(),
"consider calling `.as_ref()` to borrow the type's contents",
"as_ref().".to_string(),
Applicability::MachineApplicable,
);

let borrow_method = match self_name {
Some(sym::Option | sym::Result) => Some("as_ref"),
Some(sym::Pin) => Some("as_mut"),
_ => None,
};

if let Some(borrow_method) = borrow_method {
if maybe_reinitialized_locations.is_empty() {
err.span_suggestion_verbose(
fn_call_span.shrink_to_lo(),
&format!(
"consider calling `.{}()` to borrow the type's contents",
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra nit, but I personally would love that level of precision in the diagnostics: in the case where P = &mut _, that is, when we have to deal with Pin<&mut …>, we already have a borrow; and what we need is a re-borrow.

  • This may look silly, but I think one of the main areas of confusion with Pin for beginners is this need for .as_mut(), precisely because it's a type which, contrary to &mut, features no implicit reborrowing.

    So even an extra note for that very case, stating this very fact, could be quite educational:

    note: this function takes ownership of the receiver `self`, which moves `foo`
      --> $DIR/pin-content-move.rs:6:12
       |
    LL |     fn foo(self: Pin<&mut Self>) {}
       |            ^^^^
    help: consider calling `.as_mut()` to reborrow the type's contents
       |
    LL |     foo.as_mut().foo();
       |         +++++++++
    note: this is needed because `Pin<&mut _>`, contrary to `&mut _`, does not feature implicit reborrowing.
       |
    LL |     foo.as_mut().foo();
       |         +++++++++
  • While that instance seems like a nit, in cases such as Pin<Rc<…>> etc. for shared-pinned stuff, the .as_mut() suggestion is also likely to be incorrect.

    So, if it weren't too annoying to implement, I'd do a heuristic whereby Rc, Arc, and &mut _ would be special-cased, and would ask for, respectively: .as_ref() to borrow, .as_ref() to borrow, .as_mut() to reborrow, and otherwise defaulting to what we have here: .as_mut() to borrow.

borrow_method
),
format!("{}().", borrow_method),
Applicability::MachineApplicable,
);
}
}

// Avoid pointing to the same function in multiple different
// error messages.
if span != DUMMY_SP && self.fn_self_span_reported.insert(self_arg.span)
Expand Down
27 changes: 14 additions & 13 deletions compiler/rustc_borrowck/src/diagnostics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ use rustc_middle::mir::{
use rustc_middle::ty::print::Print;
use rustc_middle::ty::{self, DefIdTree, Instance, Ty, TyCtxt};
use rustc_mir_dataflow::move_paths::{InitLocation, LookupResult};
use rustc_span::{hygiene::DesugaringKind, symbol::sym, Span};
use rustc_span::{
hygiene::DesugaringKind,
symbol::{sym, Symbol},
Span,
};
use rustc_target::abi::VariantIdx;

use super::borrow_set::BorrowData;
Expand Down Expand Up @@ -577,9 +581,8 @@ pub(super) enum FnSelfUseKind<'tcx> {
Normal {
self_arg: Ident,
implicit_into_iter: bool,
/// Whether the self type of the method call has an `.as_ref()` method.
/// Used for better diagnostics.
is_option_or_result: bool,
self_name: Option<Symbol>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self_name: Option<Symbol>,
self_diagnostic_name: Option<Symbol>,

},
/// A call to `FnOnce::call_once`, desugared from `my_closure(a, b, c)`
FnOnceCall,
Expand Down Expand Up @@ -948,18 +951,16 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
let kind = kind.unwrap_or_else(|| {
// This isn't a 'special' use of `self`
debug!("move_spans: method_did={:?}, fn_call_span={:?}", method_did, fn_call_span);

let implicit_into_iter = Some(method_did) == tcx.lang_items().into_iter_fn()
&& fn_call_span.desugaring_kind() == Some(DesugaringKind::ForLoop);
let parent_self_ty = parent
.filter(|did| tcx.def_kind(*did) == rustc_hir::def::DefKind::Impl)
.and_then(|did| match tcx.type_of(did).kind() {
ty::Adt(def, ..) => Some(def.did),
_ => None,
});
let is_option_or_result = parent_self_ty.map_or(false, |def_id| {
matches!(tcx.get_diagnostic_name(def_id), Some(sym::Option | sym::Result))
});
FnSelfUseKind::Normal { self_arg, implicit_into_iter, is_option_or_result }

let self_name = self.body.local_decls[target_temp]
.ty
.ty_adt_def()
.and_then(|def| tcx.get_diagnostic_name(def.did));

FnSelfUseKind::Normal { self_arg, implicit_into_iter, self_name }
});

return FnSelfUse {
Expand Down
1 change: 1 addition & 0 deletions library/core/src/pin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,7 @@ use crate::ops::{CoerceUnsized, Deref, DerefMut, DispatchFromDyn, Receiver};
// See <https://internals.rust-lang.org/t/unsoundness-in-pin/11311> for more details.
#[stable(feature = "pin", since = "1.33.0")]
#[lang = "pin"]
#[rustc_diagnostic_item = "Pin"]
#[fundamental]
#[repr(transparent)]
#[derive(Copy, Clone)]
Expand Down
23 changes: 23 additions & 0 deletions src/test/ui/suggestions/pin-content-move.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
use std::pin::Pin;

struct Foo;

impl Foo {
fn foo(self: Pin<&mut Self>) {}
}

fn main() {
let mut foo = Foo;
let foo = Pin::new(&mut foo);
foo.foo();
//~^ HELP consider calling `.as_mut()` to borrow the type's contents
foo.foo();
//~^ ERROR use of moved value

let mut x = 1;
let mut x = Pin::new(&mut x);
x.get_mut();
//~^ HELP consider calling `.as_mut()` to borrow the type's contents
x.get_mut();
//~^ ERROR use of moved value
}
45 changes: 45 additions & 0 deletions src/test/ui/suggestions/pin-content-move.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
error[E0382]: use of moved value: `foo`
--> $DIR/pin-content-move.rs:14:5
|
LL | let foo = Pin::new(&mut foo);
| --- move occurs because `foo` has type `Pin<&mut Foo>`, which does not implement the `Copy` trait
LL | foo.foo();
| ----- `foo` moved due to this method call
LL |
LL | foo.foo();
| ^^^ value used here after move
|
note: this function takes ownership of the receiver `self`, which moves `foo`
--> $DIR/pin-content-move.rs:6:12
|
LL | fn foo(self: Pin<&mut Self>) {}
| ^^^^
help: consider calling `.as_mut()` to borrow the type's contents
|
LL | foo.as_mut().foo();
| +++++++++

error[E0382]: use of moved value: `x`
--> $DIR/pin-content-move.rs:21:5
|
LL | let mut x = Pin::new(&mut x);
| ----- move occurs because `x` has type `Pin<&mut i32>`, which does not implement the `Copy` trait
LL | x.get_mut();
| --------- `x` moved due to this method call
LL |
LL | x.get_mut();
| ^ value used here after move
|
note: this function takes ownership of the receiver `self`, which moves `x`
--> $SRC_DIR/core/src/pin.rs:LL:COL
|
LL | pub const fn get_mut(self) -> &'a mut T
| ^^^^
help: consider calling `.as_mut()` to borrow the type's contents
|
LL | x.as_mut().get_mut();
| +++++++++

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0382`.