Further enhance needless_borrow, mildly refactor redundant_clone

[smoelius]

This PR does the following:

• Moves some code from redundant_clone into a new clippy_utils module called mir, and wraps that code in a function called dropped_without_further_use.
• Relaxes the "is copyable" condition condition from Enhance needless_borrow to consider trait implementations #9136 by also suggesting to remove borrows from values dropped without further use. The changes involve the just mentioned function.
• Separates redundant_clone into modules.

Strictly speaking, the last bullet is independent of the others. redundant_clone is somewhat hairy, IMO. Separating it into modules makes it slightly less so, by helping to delineate what depends upon what.

I've tried to break everything up into digestible commits.

r? @Jarcho

(@Jarcho I hope you don't mind.)

changelog: continuation of #9136

[Jarcho]

Just did a quick check. Does the extension to needless_borrow handle closures? I think those have a separate MIR body from the containing function. I think what you have works fine, but it's better to check.

[smoelius]

I just add a closure-related test. Please let me know if it doesn't address your specific concern.

[Jarcho]

The use of env in the test could have the borrow removed (not trivially, you would have to prove the closure is only called once). It would be better to have some use of it after the closure is created.

As this suggestion changes the drop order it would be better to limit this to types without a significant drop function. You can use has_significant_drop.

[smoelius]

It would be better to have some use of it after the closure is created.

This has revealed a bug. Please give me a day or so to look into it.

[smoelius]

Any idea what I might have done to cause this? https://github.com/rust-lang/rust-clippy/runs/8089040744?check_suite_focus=true#step:7:1473

[Jarcho]

You can try rebasing and see if that helps. Is it working locally?

[smoelius]

I think I figured it out. Sorry for the noise.

Today's changes move the possible_borrower code under clippy_utils::mir so that it can be used in dereference.rs.

The bug I mentioned is resolved, and the PR should be ready for review (again).

[bors]

☔ The latest upstream changes (presumably #9400) made this pull request unmergeable. Please resolve the merge conflicts.

[smoelius]

Okay to rebase?

[Jarcho]

A couple of things here:

• This can calculate possible borrowers multiple times per function. It also fully recalculates the liveness bitset each time as well. What's the perf impact from this?
• Some suggestions from this are arguably downgrades (see later comment for example).

Given the second point it might be better to only lint on temporaries.

[smoelius]

A couple of things here:

• This can calculate possible borrowers multiple times per function. It also fully recalculates the liveness bitset each time as well. What's the perf impact from this?
• Some suggestions from this are arguably downgrades (see later comment for example).

Given the second point it might be better to only lint on temporaries.

This strategy feels a little extreme to me. For example, it would seem to eliminate a large number of valid suggestions from 29a7057.

To address the first bullet, we could compute the possible borrowers once (lazily) in a check_body implementation, and make the current check_expr a method of a visitor called from check_body (e.g., make the Option<PossibleBorrowMap<...>> a field of the visitor).

To address the second bullet, may I suggest the following alternative? For each body, build a map from def ids to substitutions used in calls to that def id. (This could also be done lazily.) Then, suggest to remove a & from an argument in a call to def_id only if it would not increase the total number of substitutions used in calls to def_id within the body.

Whichever way we go, can we make the current, unhindered strategy something that could be optionally enabled?

[Jarcho]

Most of the changes are on temporaries (52/74) by a quick count. Could limit locals to those which are used only once, which would lint almost all cases. The main point is to avoid changing something like:

let x = ...;
use_x(&x);
...
use_x_again(&x);

---

To address the first bullet, we could compute the possible borrowers once (lazily) in a check_body implementation, and make the current check_expr a method of a visitor called from check_body (e.g., make the Option<PossibleBorrowMap<...>> a field of the visitor).

That would unconditionally add another traversal over every function. which would probably be worse on average. Ideally we could store this in the lint pass, but that's not possible.

[smoelius]

That would unconditionally add another traversal over every function.

I thought what I was proposing would conditionally add another traversal (i.e., if all of the conditions were met so that the possible borrowers were needed to proceed).

Why would it be unconditional?

Could limit locals to those which are used only once,

That sounds reasonable to me. Okay to turn this off with an option, though?

[Jarcho]

You suggested to switching the lint to use check_body with the current check_expr moved into a visitor, correct? That leaves the visitor with all the lints, plus the new visitor from check_body running over the code.

[smoelius]

You suggested to switching the lint to use check_body with the current check_expr moved into a visitor, correct? That leaves the visitor with all the lints, plus the new visitor from check_body running over the code.

OK. I see what you are saying.

Then what if we essentially queued the arguments to needless_borrow_impl_arg_position inside Position::ImplArg, and actually processed those arguments/performed those calls in an implementation of check_body_post?

Admittedly, one thing I am not clear on is whether returning an Position::ImplArg as a possibility rather than a certainty would break the surrounding code. For example, would this code break if no warning were ultimately produced for the ImplArg? Maybe the answer is obvious to you.

rust-clippy/clippy_lints/src/dereference.rs

Lines 364 to 365 in 5860abf

	} else if let Position::ImplArg(hir_id) = position {
	(0, impl_msg, Some(hir_id))

[Jarcho]

I think you can make that work by storing the expression's HirId the target function's DefId and argument position and then running over all those in check_body_post. This would be slightly complicated by nested functions.

I'll check to see if we can change rustc to allow storing TyCtxt bounded items in lint passes as that would allow caching the MIR result directly.

Posted on zulip: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Allowing.20non.20.60'static.60.20lint.20passes/near/297154930

[smoelius]

I'll check to see if we can change rustc to allow storing TyCtxt bounded items in lint passes as that would allow caching the MIR result directly.

Thanks!

Did you already start those changes? If not, I could make them. Please just let me know.

[Jarcho]

Implemented in rust-lang/rust#101501. Hopefully that will be synced to clippy this week.

[smoelius]

Thanks, @Jarcho!

[smoelius]

I saw you PR was merged. But it looks like it didn't make it in time for the most recent rustup?

If I'm reading everything right, the current toolchain (nightly-2022-09-08) is based on commit c2804e6, which is older than 8778809 (the one which includes your PR).

[Jarcho]

That is correct. Two weeks till the next sync.

[smoelius]

I think this is ready for review again, @Jarcho.

(Rebasing was a bit of bear, so hopefully I did not screw it up.)

[bors]

☔ The latest upstream changes (presumably #9510) made this pull request unmergeable. Please resolve the merge conflicts.

[smoelius]

I think the three most recent comments have been addressed.

[bors]

☔ The latest upstream changes (presumably #9547) made this pull request unmergeable. Please resolve the merge conflicts.

[Jarcho]

I think the only thing remaining is the previous point about changing f1(&x); f2(&x); to f1(&x); f2(x);

[smoelius]

I think the only thing remaining is the previous point about changing f1(&x); f2(&x); to f1(&x); f2(x);

Meaning don't suggest that, right?

It's possible there's a problem, but I think I addressed that, actually, e.g.:

• rust-clippy/clippy_lints/src/dereference.rs

  Line 1126 in 252f598

  || !referent_used_exactly_once(cx, possible_borrowers, reference))

• rust-clippy/tests/ui/needless_borrow.rs

  Lines 367 to 383 in 252f598

  	#[allow(dead_code)]
  	mod used_exactly_once {
  	fn foo(x: String) {
  	use_x(&x);
  	}
  	fn use_x(_: impl AsRef<str>) {}
  	}
  	#[allow(dead_code)]
  	mod used_more_than_once {
  	fn foo(x: String) {
  	use_x(&x);
  	use_x_again(&x);
  	}
  	fn use_x(_: impl AsRef<str>) {}
  	fn use_x_again(_: impl AsRef<str>) {}
  	}

• rust-clippy/tests/ui/needless_borrow.stderr

  Lines 207 to 211 in 252f598

  	error: the borrowed expression implements the required traits
  	--> $DIR/needless_borrow.rs:370:15
  	|
  	LL | use_x(&x);
  	| ^^ help: change this to: `x`

[Jarcho]

So it is. I was going by the change in clippy_dev/src/new_lint.rs still being there. (there's no need to change this).

You can rebase and squash.

[smoelius]

So it is. I was going by the change in clippy_dev/src/new_lint.rs still being there. (there's no need to change this).

It should have occurred to me to fix that.

I will fix that and any other similar changes before rebasing.

[smoelius]

The most recent push squashes all of the commits except for the "Fix adjacent code" ones.

The current "Fix adjacent code" commit is the result of rerunning the lint.

Sadly, the introduction of possible_borrowers eliminated more suggestions than I realized.

I think there may be ways possible_borrowers could be improved, but that could be the subject of a separate PR.

[Jarcho]

Everything looks good. If you can catch more single use cases that would be great.

Thank you. @bors r+

[bors]

📌 Commit 9cc8da2 has been approved by Jarcho

It is now in the queue for this repository.

[bors]

⌛ Testing commit 9cc8da2 with merge 272bbfb...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 272bbfb to master...

[smoelius]

Thank you. @bors r+

Thank you as well! Especially for rust-lang/rust#101501. 🙏

Cheers.