let_and_return: avoid "does not live long enough" errors

[ebroto]

EDIT: Add #3324 to the list of fixes

Description of old impl

Avoid suggesting turning the RHS expression of the last statement into the block tail expression if a temporary borrows from a local that would be destroyed before.

This is my first incursion into MIR so there's probably room for improvement!

Avoid linting if the return type of some method or function called in the last statement has a lifetime parameter.

changelog: Fix false positive in [let_and_return]

Fixes #3792
Fixes #3324

[rust-highfive]

r? @Manishearth

(rust_highfive has picked a reviewer for you, use r? to override)

[Manishearth]

I wonder if we really need the full set of checks here: MIR lints are trickier in general, perhaps just checking if the let-and-return statement contains any borrowed data is enough? This leads to some false negatives, but that's okay IMO.

[Manishearth]

thoughts, @flip1995 ?

[flip1995]

perhaps just checking if the let-and-return statement contains any borrowed data is enough?

Can we do this easily without the MIR? Can we check for borrows in e.g. a method chain in the HIR? If there is a short and simple way to do this, I would prefer it for simplicity. But I can't think of how to do this right now.

[Manishearth]

We can just check the type for lifetime params, no?

[ebroto]

It may be relevant to the discussion that the same fix (regardless of the implementation choice in the end) can be applied to needless_return, removing the "known problem" in the lint description.

@Manishearth TBH I did not think of that possibility, I guess it could be valid but I kinda like the possibility of being more precise and avoid the false negatives.

Also, (maybe due to inexperience with the code base) I'm not sure if that approach would be simpler to implement, if I'm not mistaken it would imply walking the RHS expression looking for function and method calls, and then checking the definition to see if the return value type has lifetimes (at least, maybe I'm missing other cases?)

Just wanted to give my 2 cents, but of course I'll implement whatever we decide in the end :)

[Manishearth]

Yeah, but MIR stuff is hard to maintain and I'd avoid it if we can. A couple false negatives is no biggie imo.

You don't need to walk anything, you just need to check the type of the returned value for any lifetime substitutions

[flip1995]

@Manishearth which return type do you mean? In this example:

    fn read_line() -> String {
        let stdin = io::stdin();
        let line = stdin.lock().lines().next().unwrap().unwrap();
        line
    }

The return type of the function is just String, without any lifetimes. But the FP comes from the stdin.lock() method call, which borrows stdin. So I think @ebroto is right and we would have to implement an expression visitor, that checks for borrows.

Am I missing something?

[Manishearth]

oh, I see. yeah, my bad, my model of the issue was incorrect.

[ebroto]

@Manishearth I've written the solution we discussed to avoid using the MIR, it should be ready for review :)

[Manishearth]

@bors r+

hmm, this has to go through every expression, so it's more likely that there will be false negatives. but i guess this is okay, @flip1995 do you have opinions on this vs the more complicated but more correct MIR approach?

(we can land that later if folks feel the MIR approach was also okay)

[bors]

📌 Commit dac8a3c has been approved by Manishearth

[bors]

⌛ Testing commit dac8a3c with merge f947644...

[flip1995]

As you already wrote, MIR lints are harder to maintain. I don't have experience with MIR, so reviewing code is already kind of hard. So if there is a nearly equivalent solution (a few more FNs) without MIR, I guess it's better than the MIR version.

But in cases where MIR is necessary to prevent FPs and still have an effective lint (e.g. redundant_clone), I'd always go with MIR lints.

[flip1995]

For this case especially, I prefer the solution with just HIR, since it at least didn't introduce FNs in our tests.

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Manishearth
Pushing f947644 to master...

[Manishearth]

@flip1995 yeah the current solution fixes FPs, but introduces some FNs. I care less about FNs.