Lint for unused borrows as part of UNUSED_MUST_USE

[ecstatic-morse]

Resolves #76264.

This means an unused_must_use warning for statements like &expr; and &mut expr;. unused_must_use was chosen because it already lints for logical operators (&& and ||) whose result is unused. Unused borrows actually appear fairly often in rustc's test suite, since we have tests that rely on side-effects of the index operator (panicking). These cannot be written as expr[..];, since the result is unsized, but they can be written as let _ = &expr[..];, which is what this PR does.

See the linked issue for the motivating example. This lint also found a benign mistake in the derived impl of HashStable.

[rust-highfive]

r? @lcnr

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

[lcnr]

LGTM

does this need lang or compiler signoff considering that this is a userfacing change?

[matthiaskrgr]

How similar is this to clippys needless_borrow lint?
https://rust-lang.github.io/rust-clippy/master/#needless_borrow

[ecstatic-morse]

I think it needs @rust-lang/lang approval, but I'm not 100% sure.

@matthiaskrgr Sounds similar. I don't use clippy.

[ecstatic-morse]

Ah, so the transformation from &expr; to expr; isn't always correct. If expr returns an unsized value like a slice, it will not compile (without unsized rvalues anyways). I think we generally don't have lints in the compiler with false positives. However, I would really like to fix the pathological case in #76264 (reproduced below), a bare &x[0..4]; should be pretty rare outside of the compiler's test suite and we can suggest let _ = &expr; in this situation.

let trex = TyrannosaurusRex::new();
let is_a_dog = has_a_tail(trex)
  && has_bad_breath(trex)
  && is_a_carnivore(trex); // Misplaced semi-colon (perhaps due to reordering of lines)
  && is_adorable(trex);

if is_a_dog {
    give_hug(trex); // Ouch!
}

There are various alternatives with no false positives: Only triggering on && comes immediately to mind. I'll wait to see how others feel.

[lcnr]

If expr returns an unsized value like a slice, it will not compile (without unsized rvalues anyways).

Afaict the exprs which do so are the following:

&slice[..];
&y[0];
&*boxed;

all of which seem like something we can (and should) warn on, even if the suggestion is incorrect.

[ecstatic-morse]

For my own clarity, postfix indexing (a[idx]) desugars to *std::ops::Index{,Mut}::index{,_mut}(&{,mut }a, idx) unless a is a slice or an array and idx is a usize, so conceptually the only problem is reborrows (&*).

Also, I'm starting to think this should be folded into UNUSED_MUST_USE. The logical boolean operators are already included in that lint, and they have a similar "false-positive" problem, since someone could put a function with side-effects in the second half (e.g. condition || panic!()). The suggestion there is the same as for unused borrows: Assign the result to _.

[ecstatic-morse]

This is now part of unused_must_use, and I've updated the title and description. This is just waiting on @rust-lang/lang to weigh in. I'll nominate it for the meeting as well.

[scottmcm]

This makes sense to me. Since we lint on !x;, we might as well lint on &x too.

I don't know whether this needs a full FCP (we can talk about it on Monday), but in case it does,
@rfcbot fcp merge

[rfcbot]

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @withoutboats

Concerns:

• crater (Lint for unused borrows as part of UNUSED_MUST_USE #76894 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[ecstatic-morse]

I don't know whether this needs a full FCP (we can talk about it on Monday), but in case it does,

I definitely approved a similar PR (#74869) without lang team approval. lcnr asked about the process in #76894 (review), and it occurred to me that I didn't actually know. If the lang-team does want to weigh in on this, they should consider it in concert with #69466, which also involves side-effects and UNUSED_MUST_USE. Y'all might have better things to do, however.

[withoutboats]

@rfcbot concern crater

Enthusiastic about this intuitively, but I'd like a crater run to make sure that this isn't going to suddenly lint a large part of the ecosystem or anything.

[Dylan-DPC-zz]

@bors r-

failed in rollup

[Dylan-DPC-zz]

@ecstatic-morse any updates on this?

[lcnr]

I am not able to review any PRs in the near future.

r? @RalfJung

[RalfJung]

Given that @ecstatic-morse has been inactive here for months and in general does not seem to be doing much Rust work any more, I'm going to close this PR for now.
@ecstatic-morse if you (or anyone else) wants to pick this up again, feel free to reopen. :)