Don't suggest `ref` for `let` bindings.

[steveklabnik]

UPDATE: There are some initial mentoring instructions below.

---

fn main() {
    let v = vec![String::from("oh no")];
    
    let e = v[0];
}

suggests

error[E0507]: cannot move out of indexed content
 --> <anon>:4:13
  |
4 |     let e = v[0];
  |         -   ^^^^ cannot move out of indexed content
  |         |
  |         hint: to prevent move, use `ref e` or `ref mut e`

let ref e is not idiomatic here, so this is misleading (and confusing, frankly)

cc @eevee

[nikomatsakis]

@steveklabnik

First, would you prefer to just stop hinting about ref in any let pattern, or just if you write let e = ...? (e.g., let Foo { ref e, ref g } = x is something I at least do fairly regularly...)

Second, for the simple case, do you think we should hint something like "consider let e = &v[0]"? Perhaps only if the RHS is an lvalue? (Well, I guess this error will always result from an lvalue anyway.)

[nikomatsakis]

Fixing this requires doing a bit of plumbing. I don't have time to get into every detail so let me just jot down some notes about how the code works today (I was just reading it).

• The message is generated by the borrow-checker. It finds out about the move due to a call to consume_pat(), a callback invoked by the ExprUseVisitor (EUV) for moves from patterns. This callback does not currently distinguish moves that occur in let patterns from those that occur in match patterns, so the borrow-checker can't easily tell the difference.
  • We could fix this by extending the EUV interface, but we could also walk up the HIR map (in rustc::hir::map) from the pattern to find out the enclosing context. The latter might be better, since we could distinguish e.g. let x = ... from let Foo { x } = ..., and also gain access to the RHS of the let.
• From the callback, the borrow checker gathers up moves in librustc_borrowck/borrowck/gather_loans/gather_moves.rs, and specifically the function gather_move_from_pat().
  • This function creates a GatherMoveInfo struct whose field span_path_opt is Some(_)
• We then later report the errors. The code for this is found in src/librustc_borrowck/borrowck/gather_loans/move_errors.rs, and specifically the report_move_errors() function.
  • This will group together errors by location, and collect the span_path_opt fields into a vector. It then iterates over this vector (error.move_to_places) (which is empty if all the span_path_opt fields were None) to issue the ref notes.

So the most direct fix would be to notice that the pattern is in a let and set the field to None. But perhaps a better fix would be to detect cases where we can suggest writing &v[0] and do that instead. I'm not entirely sure what's the most elegant way to modify things here. I imagine we need to extend MoveSpanAndPath to contain the NodeId of the pattern. We could even replace the struct entirely, since the other info can be derived from the NodeId: we can use bccx.tcx.hir.find(id) and check that the result is the Node::NodePat variant (it must be) to extract the other information. Regardless, we can walk up the HIR map to find if the enclosing context for this pattern is a let, and also extract the initializer (the RHS of the let).

Anyway, that's a bit dense, I can try to expand some in a follow-up comments later on.

[gaurikholkar-zz]

Hi. Will take this up.

[steveklabnik]

First, would you prefer to just stop hinting about ref in any let pattern, or just if you write let e = ...? (e.g., let Foo { ref e, ref g } = x is something I at least do fairly regularly...)

I am mostly thinking about the former, that is, the latter is idiomatic use of ref.

Second, for the simple case, do you think we should hint something like "consider let e = &v[0]"? Perhaps only if the RHS is an lvalue? (Well, I guess this error will always result from an lvalue anyway.)

That would be great.

[nikomatsakis]

Expanding on my directions. I think that the first step would be to get more familiar with the HIR, which is the primary data structure that the compiler uses to encode the program it is compiling. The HIR reflects a lightly desugared view of Rust syntax (i.e., some constructs -- like for loops -- are broken down into simpler ones -- like while loops, but otherwise it's mostly reflecting the input in a fairly direct fashion).

One downside of the HIR is that sometimes there are many equivalent ways to write things. In this case, for example, these two are equivalent:

let ref x = v[0];
let x = &v[0];

To help later passes that do analysis, therefore, there is a bit of code called the ExprUseVisitor (EUV) which walks the HIR and invokes callbacks through the Delegate trait. I think in practice you won't have to really touch that code in particular, but I wanted to give some pointers to the context. Anyway, the idea of the EUV is to walk the tree and invoke callbacks indicating what 'basic operations' are happening (e.g., a value is being moved, a value is being borrowed, etc).

In any case, the summary here is that, at the time we report this error, we are in some code that received a callback from the EUV saying, effectively, "v[0] was moved by the pattern x". Now, because v[0] is using the overloaded index operator, and the overloaded operators don't support moves, that's an error.

Right now, whenever a move resulted from a pattern -- any kind of pattern -- we report the same suggestion ("add ref"). The idea here is to look at the source of the pattern to decide what to suggest. So I think the right starting goal should be to write a little function that returns "true" if the pattern is part of a let (and false if it is part of a match).

To do this, we're going to want to use the HIR map. In particular, the HIR map allows one to walk upwards from a HIR node to see its parents: if we walk upward from the pattern x, we should reach the let node. In HIR terms:

• A let <pat> = <expr> statement is a statement, specifically a StmtDecl.
• The kind of Decl is a DeclLocal.
• A Local contains a field pat that stores a pattern, which should contain x.

Every node in the HIR has a unique id (called its NodeId). For example, the id for a pattern can be found in its id field. The HIR map already has a private method walk_parent_nodes for walking upward to visit the parents of a given NodeId. Typically it is wrapped in an accessor that supplies a closure to indicate when to stop the iteration (e.g. get_module_parent()). I think we want to write an accessor like this that will decide if a given pattern is part of a let or a match. It might look something like this:

/// Where can patterns ultimately be used?
pub enum PatternSource<'hir> {
    MatchExpr(&'hir hir::Expr),
    LetDecl(&'hir hir::Local),
}

// Note: this impl already exists... I'm just adding a method
impl<'hir> Map<'hir> {
    ...

    /// Determines whether a pattern is located within a `let` statement or a `match` expression.
    ///
    /// Returns `Some(true)` if `id` 
    pub fn get_pattern_source(&self, pat: &hir::Pat) -> PatternSource<'hir> {
        let result = self.walk_parent_nodes(pat.id, |node| match *node {
            NodePat(_) => false, // keep walking as long as we are in a pattern
            _ => true, // stop walking once we exit patterns
        }).expect("never found a parent for the pattern");

        match result {
            NodeExpr(ref e) => {
                // the enclosing expression must be a `match`
                assert!(match e.node { ExprMatch(..) => true, _ => false });
                PatternSource::MatchExpr(e)
            }
            NodeStmt(ref s) => {
                // the enclosing statement must be a `let`
                match s.node {
                    StmtDecl(ref decl, _) => {
                        match decl.kind {
                            DeclLocal(ref local) => Some(PatternSource::LetDecl(decl)),
                            _ => span_bug!(s.span, "expected pattern to be in a `let` statement"),
                        }
                    }
                    _ => span_bug!(s.span, "expected pattern to be in a `let` statement"),
               }
            }
            _ => span_bug!(s.span, "pattern in unexpected location {:?}", result)
      }
}

Anyway, that's roughly how it should look. I think I actually forgot a case or two but we'll figure out as we go. Then the next thing to do would be to call this function from within the borrow checker (which is where the error is being reported). Specifically, I would add a call in the gather_move_from_pat() function in src/librustc_borrowck/borrowck/gather_loans/gather_moves.rs, and just report the result with debug! (so we can see it later):

pub fn gather_move_from_pat<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
                                      move_data: &MoveData<'tcx>,
                                      move_error_collector: &mut MoveErrorCollector<'tcx>,
                                      move_pat: &hir::Pat,
                                      cmt: mc::cmt<'tcx>) {
    let source = bccx.tcx.hir.get_pattern_source(move_pat);
    debug!("gather_move_from_pat: move_pat={:?} source={:?}", move_pat, source);
}

(The bccx.tcx.hir is a reference to the HIR map that we were adding methods to before.)

The idea here is just to invoke your code and see if it panics etc. OK, that's enough for now. =) Let's try to get that far... once we get it building, you should be able to inspect the output by running rustc with RUST_LOG set to the gather_moves module (I recommend dumping the output into a file, in this case killme):

RUST_LOG=rustc_borrowck::borrowck::gather_loans::gather_moves rustc example.rs >& killme

Then you can skim for that debug! call we added...

[nikhilshagri]

@nikomatsakis I tried coding up the get_pattern_source method you showed, and after I some corrections, I have this now: https://gist.github.com/cynicaldevil/4f5a2e8b96bb0d47d89ecb7626da5ccd

I have worked with Rust before, but I don't have much experience with lifetimes, and therefore I'm getting stuck on line 19, where the compiler throws an error saying that local.clone().unwrap() does not live long enough. I know that the lifetime of the value must be equal to the lifetime of the method's body, but I can't find a way to make it so. I did everything I could (adding/removing refs, cloning values), but no luck. Could you help me out?

[arielb1]

@cynicaldevil

There's no point in cloning HIR nodes. You can always take a reference to them that has lifetime 'hir.

[nikhilshagri]

@arielb1 Thanks, I solved it at last. Turns out, the whole problem was unrelated to lifetimes, I was actually having trouble extracting a value from a smart pointer, and was using the unwrap method the whole time, which moved the value into the match arm. I didn't know that the pointer implemented a deref too, which solved the issue for me.

[nikomatsakis]

@cynicaldevil sorry I've been unresponsive; been busy traveling around, hopefully better now. =) Everything working out ok?

[nikhilshagri]

@nikomatsakis no worries :) As you can see, I'm blocked because of #41169 now, so I don't know how to proceed further.

[nikomatsakis]

@cynicaldevil I see -- I can leave some comments on that issue to suggest how to resolve it. However, I also think that @gaurikholkar is still actively working on this as well... seems like a shame to duplicate effort. Not sure how far they have gotten.