Suggest borrowing when doing if let Some(_) from non-Copy ADT

Just got this error:

error[E0382]: borrow of moved value: `x`
    --> src/librustc_mir/hair/pattern/_match.rs:1614:53
     |
1611 |                         if let Some(range) = x {
     |                                     ----- value moved here
...
1614 |                         debug!("intersection {:?}", x);
     |                                                     ^ value borrowed here after partial move
     |
     = note: move occurs because value has type `hair::pattern::_match::IntRange<'_>`, which does not implement the `Copy` trait

error: aborting due to previous error

It should suggest to borrow x.

@estebank

@estebank can you elaborate on the message you expect to see here? Something like

error[E0382]: borrow of moved value: `x`
    --> src/librustc_mir/hair/pattern/_match.rs:1614:53
     |
1611 |                         if let Some(range) = x {
     |                                     ----- value moved here
...
1614 |                         debug!("intersection {:?}", x);
     |                                                     ^ value borrowed here after partial move
     |
     = note: move occurs because value has type `hair::pattern::_match::IntRange<'_>`, which does not implement the `Copy` trait
       Consider borrowing `x` rather than moving it.

perhaps?

@ayazhafiz

@ayazhafiz I believe we should aim at:

error[E0382]: borrow of moved value: `x`
    --> src/librustc_mir/hair/pattern/_match.rs:1614:53
     |
1611 |                         if let Some(range) = x {
     |                                     -----    - help: consider borrowing here: `&x`
     |                                     |
     |                                     value moved here
...
1614 |                         debug!("intersection {:?}", x);
     |                                                     ^ value borrowed here after partial move
     |
     = note: move occurs because value has type `hair::pattern::_match::IntRange<'_>`, which does not implement the `Copy` trait

I took a look at this today. I am a first-time contributor to this project and would like to contribute this enhancement, but I'm not sure how much work this will end up requiring. Here are some notes/leads I've noticed; I'm hoping someone may be able to provide suggestions, and at the very least I hope this is helpful for anyone else wishing to tackle this.

Consider the example provided in the first comment of this issue (abbreviated for example):

error[E0382]: borrow of moved value: `x`
    --> src/librustc_mir/hair/pattern/_match.rs:1614:53
     |
1611 |                         if let Some(range) = x {
     |                                     ----- value moved here
...
1614 |                         debug!("intersection {:?}", x);
     |                                                     ^ value borrowed here after partial move

This error message is generated by MirBorrowckCtxt#report_use_of_moved_or_uninitialized:

rust/src/librustc_mir/borrow_check/conflict_errors.rs

Lines 47 to 53 in a6624ed

    
           pub(super) fn report_use_of_moved_or_uninitialized( 
        
               &mut self, 
        
               location: Location, 
        
               desired_action: InitializationRequiringAction, 
        
               (moved_place, used_place, span): (PlaceRef<'cx, 'tcx>, PlaceRef<'cx, 'tcx>, Span), 
        
               mpi: MovePathIndex, 
        
           ) {

This function knows about the erroring span (where the value is borrowed after a move; x in debug!("intersection {:?}", x) in the example above) and the location of where the value was moved (range in the example above).

It's not immediately clear to me that it's easy to recover the span of the RHS of Some(range) = x from here, which is the span we want to give a suggestion for. However, MirBorrowckCtxt seems to know about move sites, and about where values are moved out of:

rust/src/librustc_mir/dataflow/move_paths/mod.rs

Lines 100 to 102 in 7eb65df

    
           pub struct MoveData<'tcx> { 
        
               pub move_paths: IndexVec<MovePathIndex, MovePath<'tcx>>, 
        
               pub moves: IndexVec<MoveOutIndex, MoveOut>,

rust/src/librustc_mir/dataflow/move_paths/mod.rs

Lines 156 to 168 in 7eb65df

    
           /// `MoveOut` represents a point in a program that moves out of some 
        
           /// L-value; i.e., "creates" uninitialized memory. 
        
           /// 
        
           /// With respect to dataflow analysis: 
        
           /// - Generated by moves and declaration of uninitialized variables. 
        
           /// - Killed by assignments to the memory. 
        
           #[derive(Copy, Clone)] 
        
           pub struct MoveOut { 
        
               /// path being moved 
        
               pub path: MovePathIndex, 
        
               /// location of move 
        
               pub source: Location, 
        
           }

I have a feeling that a MoveOut.source is what we're looking for (I assume it would describe the moving out of x in Some(range) = x), but am not entirely certain of that. It's also not yet clear to me how to convert a MoveOut source into a Span that can be used to give a suggestion.

Looking from the top-down is interesting but doesn't seem too useful. One of the lower-stack callers that generates this error is MirBorrowckCtxt#consume_rvalue:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 1218 to 1261 in 7eb65df

    
           fn consume_rvalue( 
        
               &mut self, 
        
               location: Location, 
        
               (rvalue, span): (&'cx Rvalue<'tcx>, Span), 
        
               flow_state: &Flows<'cx, 'tcx>, 
        
           ) { 
        
               match *rvalue { 
        
                   Rvalue::Ref(_ /*rgn*/, bk, ref place) => { 
        
                       let access_kind = match bk { 
        
                           BorrowKind::Shallow => { 
        
                               (Shallow(Some(ArtificialField::ShallowBorrow)), Read(ReadKind::Borrow(bk))) 
        
                           }, 
        
                           BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))), 
        
                           BorrowKind::Unique | BorrowKind::Mut { .. } => { 
        
                               let wk = WriteKind::MutableBorrow(bk); 
        
                               if allow_two_phase_borrow(bk) { 
        
                                   (Deep, Reservation(wk)) 
        
                               } else { 
        
                                   (Deep, Write(wk)) 
        
                               } 
        
                           } 
        
                       }; 
        
                       self.access_place( 
        
                           location, 
        
                           (place, span), 
        
                           access_kind, 
        
                           LocalMutationIsAllowed::No, 
        
                           flow_state, 
        
                       ); 
        
                       let action = if bk == BorrowKind::Shallow { 
        
                           InitializationRequiringAction::MatchOn 
        
                       } else { 
        
                           InitializationRequiringAction::Borrow 
        
                       }; 
        
                       self.check_if_path_or_subpath_is_moved( 
        
                           location, 
        
                           action, 
        
                           (place.as_ref(), span), 
        
                           flow_state, 
        
                       ); 
        
                   }

Which does exactly what it sounds like, consuming an rvalue (seemingly x in debug!("intersection {:?}", x) in the example above) and checking for any errors. This doesn't seem too helpful for this problem because the actual checking of move sites isn't done until higher up the call stack, particularly in MirBorrowckCtxt#report_use_of_moved_or_uninitialized.

Right now, I think looking at MirBorrowckCtxt#report_use_of_moved_or_uninitialized and the MoveOut struct are the most likely paths to get closer to solving this. I definitely have many knowledge gaps, and am likely misunderstanding a lot, so any suggestions would be appreciated. Otherwise, I hope it can provide some baseline information for anyone else interested in this.

Provide suggestions for some moved value errors

Rollup merge of rust-lang#73534 - estebank:borrowck-suggestions, r=ma…

Provide suggestions for some moved value errors

Rollup merge of rust-lang#73534 - estebank:borrowck-suggestions, r=ma…

Navigation Menu

Repository navigation

Suggest borrowing when doing `if let Some(_)` from non-Copy ADT #63988

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	pub(super) fn report_use_of_moved_or_uninitialized(
	&mut self,
	location: Location,
	desired_action: InitializationRequiringAction,
	(moved_place, used_place, span): (PlaceRef<'cx, 'tcx>, PlaceRef<'cx, 'tcx>, Span),
	mpi: MovePathIndex,
	) {

	pub struct MoveData<'tcx> {
	pub move_paths: IndexVec<MovePathIndex, MovePath<'tcx>>,
	pub moves: IndexVec<MoveOutIndex, MoveOut>,

	/// `MoveOut` represents a point in a program that moves out of some
	/// L-value; i.e., "creates" uninitialized memory.
	///
	/// With respect to dataflow analysis:
	/// - Generated by moves and declaration of uninitialized variables.
	/// - Killed by assignments to the memory.
	#[derive(Copy, Clone)]
	pub struct MoveOut {
	/// path being moved
	pub path: MovePathIndex,
	/// location of move
	pub source: Location,
	}

	fn consume_rvalue(
	&mut self,
	location: Location,
	(rvalue, span): (&'cx Rvalue<'tcx>, Span),
	flow_state: &Flows<'cx, 'tcx>,
	) {
	match *rvalue {
	Rvalue::Ref(_ /rgn/, bk, ref place) => {
	let access_kind = match bk {
	BorrowKind::Shallow => {
	(Shallow(Some(ArtificialField::ShallowBorrow)), Read(ReadKind::Borrow(bk)))
	},
	BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))),
	BorrowKind::Unique \| BorrowKind::Mut { .. } => {
	let wk = WriteKind::MutableBorrow(bk);
	if allow_two_phase_borrow(bk) {
	(Deep, Reservation(wk))
	} else {
	(Deep, Write(wk))
	}
	}
	};

	self.access_place(
	location,
	(place, span),
	access_kind,
	LocalMutationIsAllowed::No,
	flow_state,
	);

	let action = if bk == BorrowKind::Shallow {
	InitializationRequiringAction::MatchOn
	} else {
	InitializationRequiringAction::Borrow
	};

	self.check_if_path_or_subpath_is_moved(
	location,
	action,
	(place.as_ref(), span),
	flow_state,
	);
	}

Suggest borrowing when doing if let Some(_) from non-Copy ADT #63988

Description

Activity

ayazhafiz commented on Sep 3, 2019

estebank commented on Sep 3, 2019

ayazhafiz commented on Sep 9, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Suggest borrowing when doing `if let Some(_)` from non-Copy ADT #63988