Skip to content

When encountering move error on an Option, suggest using as_ref #61147

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

Merged
merged 2 commits into from
May 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
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
167 changes: 90 additions & 77 deletions src/librustc_mir/borrow_check/move_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,12 +242,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
let (mut err, err_span) = {
let (span, original_path, kind): (Span, &Place<'tcx>, &IllegalMoveOriginKind<'_>) =
match error {
GroupedMoveError::MovesFromPlace {
span,
ref original_path,
ref kind,
..
} |
GroupedMoveError::MovesFromPlace { span, ref original_path, ref kind, .. } |
GroupedMoveError::MovesFromValue { span, ref original_path, ref kind, .. } |
GroupedMoveError::OtherIllegalMove { span, ref original_path, ref kind } => {
(span, original_path, kind)
Expand All @@ -257,81 +252,99 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
debug!("report: original_path={:?} span={:?}, kind={:?} \
original_path.is_upvar_field_projection={:?}", original_path, span, kind,
self.is_upvar_field_projection(original_path));
(
match kind {
IllegalMoveOriginKind::Static => {
self.infcx.tcx.cannot_move_out_of(span, "static item", origin)
}
IllegalMoveOriginKind::BorrowedContent { target_place: place } => {
// Inspect the type of the content behind the
// borrow to provide feedback about why this
// was a move rather than a copy.
let ty = place.ty(self.mir, self.infcx.tcx).ty;
let is_upvar_field_projection =
self.prefixes(&original_path, PrefixSet::All)
.any(|p| self.is_upvar_field_projection(p).is_some());
debug!("report: ty={:?}", ty);
match ty.sty {
ty::Array(..) | ty::Slice(..) =>
self.infcx.tcx.cannot_move_out_of_interior_noncopy(
span, ty, None, origin
),
ty::Closure(def_id, closure_substs)
if def_id == self.mir_def_id && is_upvar_field_projection
=> {
let closure_kind_ty =
closure_substs.closure_kind_ty(def_id, self.infcx.tcx);
let closure_kind = closure_kind_ty.to_opt_closure_kind();
let place_description = match closure_kind {
Some(ty::ClosureKind::Fn) => {
"captured variable in an `Fn` closure"
}
Some(ty::ClosureKind::FnMut) => {
"captured variable in an `FnMut` closure"
}
Some(ty::ClosureKind::FnOnce) => {
bug!("closure kind does not match first argument type")
}
None => bug!("closure kind not inferred by borrowck"),
};
debug!("report: closure_kind_ty={:?} closure_kind={:?} \
place_description={:?}", closure_kind_ty, closure_kind,
place_description);

let mut diag = self.infcx.tcx.cannot_move_out_of(
span, place_description, origin);

for prefix in self.prefixes(&original_path, PrefixSet::All) {
if let Some(field) = self.is_upvar_field_projection(prefix) {
let upvar_hir_id = self.upvars[field.index()].var_hir_id;
let upvar_span = self.infcx.tcx.hir().span_by_hir_id(
upvar_hir_id);
diag.span_label(upvar_span, "captured outer variable");
break;
}
let err = match kind {
IllegalMoveOriginKind::Static => {
self.infcx.tcx.cannot_move_out_of(span, "static item", origin)
}
IllegalMoveOriginKind::BorrowedContent { target_place: place } => {
// Inspect the type of the content behind the
// borrow to provide feedback about why this
// was a move rather than a copy.
let ty = place.ty(self.mir, self.infcx.tcx).ty;
let is_upvar_field_projection =
self.prefixes(&original_path, PrefixSet::All)
.any(|p| self.is_upvar_field_projection(p).is_some());
debug!("report: ty={:?}", ty);
let mut err = match ty.sty {
ty::Array(..) | ty::Slice(..) =>
self.infcx.tcx.cannot_move_out_of_interior_noncopy(
span, ty, None, origin
),
ty::Closure(def_id, closure_substs)
if def_id == self.mir_def_id && is_upvar_field_projection
=> {
let closure_kind_ty =
closure_substs.closure_kind_ty(def_id, self.infcx.tcx);
let closure_kind = closure_kind_ty.to_opt_closure_kind();
let place_description = match closure_kind {
Some(ty::ClosureKind::Fn) => {
"captured variable in an `Fn` closure"
}
Some(ty::ClosureKind::FnMut) => {
"captured variable in an `FnMut` closure"
}
Some(ty::ClosureKind::FnOnce) => {
bug!("closure kind does not match first argument type")
}
None => bug!("closure kind not inferred by borrowck"),
};
debug!("report: closure_kind_ty={:?} closure_kind={:?} \
place_description={:?}", closure_kind_ty, closure_kind,
place_description);

let mut diag = self.infcx.tcx.cannot_move_out_of(
span, place_description, origin);

for prefix in self.prefixes(&original_path, PrefixSet::All) {
if let Some(field) = self.is_upvar_field_projection(prefix) {
let upvar_hir_id = self.upvars[field.index()].var_hir_id;
let upvar_span = self.infcx.tcx.hir().span_by_hir_id(
upvar_hir_id);
diag.span_label(upvar_span, "captured outer variable");
break;
}

diag
}
_ => {
let source = self.borrowed_content_source(place);
self.infcx.tcx.cannot_move_out_of(
span, &source.to_string(), origin
)
},

diag
}
_ => {
let source = self.borrowed_content_source(place);
self.infcx.tcx.cannot_move_out_of(
span, &source.to_string(), origin
)
},
};
let orig_path_ty = format!(
"{:?}",
original_path.ty(self.mir, self.infcx.tcx).ty,
);
let snippet = self.infcx.tcx.sess.source_map().span_to_snippet(span).unwrap();
let is_option = orig_path_ty.starts_with("std::option::Option");
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you feel about waiting a bit with merging this PR (a bit being until #60966 is merged)? Then you can annotate Option and Result with #[rustc_diagnostic_item = "option"] and "result" respectively and then check for these items via

if let Some(adt) = original_path.ty(self.mir, self.infcx.tcx).adt_def() {
	if self.infcx.tcx.is_diagnostic_item(adt.did, sym::option) {
        ...
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a couple of places where we're already doing string comparisons like this, so I love the fact that we'll be able to get rid of them soon!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... or we can just merge this and fix it up later, doesn't seem like a big deal in fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine either way

let is_result = orig_path_ty.starts_with("std::result::Result");
if is_option || is_result {
err.span_suggestion(
span,
&format!("consider borrowing the `{}`'s content", if is_option {
"Option"
} else {
"Result"
}),
format!("{}.as_ref()", snippet),
Applicability::MaybeIncorrect,
);
}
IllegalMoveOriginKind::InteriorOfTypeWithDestructor { container_ty: ty } => {
self.infcx.tcx
.cannot_move_out_of_interior_of_drop(span, ty, origin)
}
IllegalMoveOriginKind::InteriorOfSliceOrArray { ty, is_index } =>
self.infcx.tcx.cannot_move_out_of_interior_noncopy(
span, ty, Some(*is_index), origin
),
},
span,
)
err
}
IllegalMoveOriginKind::InteriorOfTypeWithDestructor { container_ty: ty } => {
self.infcx.tcx
.cannot_move_out_of_interior_of_drop(span, ty, origin)
}
IllegalMoveOriginKind::InteriorOfSliceOrArray { ty, is_index } =>
self.infcx.tcx.cannot_move_out_of_interior_noncopy(
span, ty, Some(*is_index), origin
),
};
(err, span)
};

self.add_move_hints(error, &mut err, err_span);
Expand Down
39 changes: 39 additions & 0 deletions src/test/ui/suggestions/option-content-move.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//run-rustfix

pub struct LipogramCorpora {
selections: Vec<(char, Option<String>)>,
}

impl LipogramCorpora {
pub fn validate_all(&mut self) -> Result<(), char> {
for selection in &self.selections {
if selection.1.is_some() {
if selection.1.as_ref().unwrap().contains(selection.0) {
//~^ ERROR cannot move out of borrowed content
return Err(selection.0);
}
}
}
Ok(())
}
}

pub struct LipogramCorpora2 {
selections: Vec<(char, Result<String, String>)>,
}

impl LipogramCorpora2 {
pub fn validate_all(&mut self) -> Result<(), char> {
for selection in &self.selections {
if selection.1.is_ok() {
if selection.1.as_ref().unwrap().contains(selection.0) {
//~^ ERROR cannot move out of borrowed content
return Err(selection.0);
}
}
}
Ok(())
}
}

fn main() {}
39 changes: 39 additions & 0 deletions src/test/ui/suggestions/option-content-move.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//run-rustfix

pub struct LipogramCorpora {
selections: Vec<(char, Option<String>)>,
}

impl LipogramCorpora {
pub fn validate_all(&mut self) -> Result<(), char> {
for selection in &self.selections {
if selection.1.is_some() {
if selection.1.unwrap().contains(selection.0) {
//~^ ERROR cannot move out of borrowed content
return Err(selection.0);
}
}
}
Ok(())
}
}

pub struct LipogramCorpora2 {
selections: Vec<(char, Result<String, String>)>,
}

impl LipogramCorpora2 {
pub fn validate_all(&mut self) -> Result<(), char> {
for selection in &self.selections {
if selection.1.is_ok() {
if selection.1.unwrap().contains(selection.0) {
//~^ ERROR cannot move out of borrowed content
return Err(selection.0);
}
}
}
Ok(())
}
}

fn main() {}
21 changes: 21 additions & 0 deletions src/test/ui/suggestions/option-content-move.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
error[E0507]: cannot move out of borrowed content
--> $DIR/option-content-move.rs:11:20
|
LL | if selection.1.unwrap().contains(selection.0) {
| ^^^^^^^^^^^
| |
| cannot move out of borrowed content
| help: consider borrowing the `Option`'s content: `selection.1.as_ref()`

error[E0507]: cannot move out of borrowed content
--> $DIR/option-content-move.rs:29:20
|
LL | if selection.1.unwrap().contains(selection.0) {
| ^^^^^^^^^^^
| |
| cannot move out of borrowed content
| help: consider borrowing the `Result`'s content: `selection.1.as_ref()`

error: aborting due to 2 previous errors

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