Auto merge of #9701 - smoelius:improve-possible-borrower, r=Jarcho

Improve `possible_borrower`

This PR makes several improvements to `clippy_uitls::mir::possible_borrower`. These changes benefit both `needless_borrow` and `redundant clone`.

1. **Use the compiler's `MaybeStorageLive` analysis**

I could spot not functional differences between the one in the compiler and the one in Clippy's repository. So, I removed the latter in favor of the the former.

2. **Make `PossibleBorrower` a dataflow analysis instead of a visitor**

The main benefit of this change is that allows `possible_borrower` to take advantage of statements' relative locations, which is easier to do in an analysis than in a visitor.

This is easier to illustrate with an example, so consider this one:
```rust
    fn foo(cx: &LateContext<'_>, lint: &'static Lint) {
        cx.struct_span_lint(lint, rustc_span::Span::default(), "", |diag| diag.note(&String::new()));
        //                                                                          ^
    }
```
We would like to flag the `&` pointed to by the `^` for removal. `foo`'s MIR begins like this:
```rust
fn span_lint::foo::{closure#0}(_1: [closure@$DIR/needless_borrow.rs:396:68: 396:74], _2: &mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()>) -> &mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()> {
    debug diag => _2;                    // in scope 0 at $DIR/needless_borrow.rs:396:69: 396:73
    let mut _0: &mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()>; // return place in scope 0 at $DIR/needless_borrow.rs:396:75: 396:75
    let mut _3: &mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()>; // in scope 0 at $DIR/needless_borrow.rs:396:75: 396:100
    let mut _4: &mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()>; // in scope 0 at $DIR/needless_borrow.rs:396:75: 396:100
    let mut _5: &std::string::String;    // in scope 0 at $DIR/needless_borrow.rs:396:85: 396:99
    let _6: std::string::String;         // in scope 0 at $DIR/needless_borrow.rs:396:86: 396:99

    bb0: {
        StorageLive(_3);                 // scope 0 at $DIR/needless_borrow.rs:396:75: 396:100
        StorageLive(_4);                 // scope 0 at $DIR/needless_borrow.rs:396:75: 396:100
        _4 = &mut (*_2);                 // scope 0 at $DIR/needless_borrow.rs:396:75: 396:100
        StorageLive(_5);                 // scope 0 at $DIR/needless_borrow.rs:396:85: 396:99
        StorageLive(_6);                 // scope 0 at $DIR/needless_borrow.rs:396:86: 396:99
        _6 = std::string::String::new() -> bb1; // scope 0 at $DIR/needless_borrow.rs:396:86: 396:99
                                         // mir::Constant
                                         // + span: $DIR/needless_borrow.rs:396:86: 396:97
                                         // + literal: Const { ty: fn() -> std::string::String {std::string::String::new}, val: Value(<ZST>) }
    }

    bb1: {
        _5 = &_6;                        // scope 0 at $DIR/needless_borrow.rs:396:85: 396:99
        _3 = rustc_errors::diagnostic_builder::DiagnosticBuilder::<'_, ()>::note::<&std::string::String>(move _4, move _5) -> [return: bb2, unwind: bb4]; // scope 0 at $DIR/needless_borrow.rs:396:75: 396:100
                                         // mir::Constant
                                         // + span: $DIR/needless_borrow.rs:396:80: 396:84
                                         // + literal: Const { ty: for<'a> fn(&'a mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()>, &std::string::String) -> &'a mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()> {rustc_errors::diagnostic_builder::DiagnosticBuilder::<'_, ()>::note::<&std::string::String>}, val: Value(<ZST>) }
    }
```
The call to `diag.note` appears in `bb1` on the line beginning with `_3 =`. The `String` is owned by `_6`. So, in the call to `diag.note`, we would like to know whether there are any references to `_6` besides `_5`.

The old, visitor approach did not consider the relative locations of statements. So all borrows were treated the same, *even if they occurred after the location of interest*.

For example, before the `_3 = ...` call, the possible borrowers of `_6` would be just `_5`. But after the call, the possible borrowers would include `_2`, `_3`, and `_4`.

So, in a sense, the call from which we are try to remove the needless borrow is trying to prevent us from removing the needless borrow(!).

With an analysis, things do not get so muddled. We can determine the set of possible borrowers at any specific location, e.g., using a `ResultsCursor`.

3. **Change `only_borrowers` to `at_most_borrowers`**

`possible_borrowers` exposed a function `only_borrowers` that determined whether the borrowers of some local were *exactly* some set `S`. But, from what I can tell, this was overkill. For the lints that currently use `possible_borrower` (`needless_borrow` and `redundant_clone`), all we really want to know is whether there are borrowers *other than* those in `S`. (Put another way, we only care about the subset relation in one direction.) The new function `at_most_borrowers` takes this more tailored approach.

4. **Compute relations "on the fly" rather than using `transitive_relation`**

The visitor would compute and store the transitive closure of the possible borrower relation for an entire MIR body.

But with an analysis, there is effectively a different possible borrower relation at each location in the body. Computing and storing a transitive closure at each location would not be practical.

So the new approach is to compute the transitive closure on the fly, as needed. But the new approach might actually be more efficient, as I now explain.

In all current uses of `at_most_borrowers` (previously `only_borrowers`), the size of the set of borrowers `S` is at most 2. So you need only check at most three borrowers to determine whether the subset relation holds. That is, once you have found a third borrower, you can stop, since you know the relation cannot hold.

Note that `transitive_relation` is still used by `clippy_uitls::mir::possible_origin` (a kind of "subroutine" of `possible_borrower`).

cc: `@Jarcho`

---

changelog: [`needless_borrow`], [`redundant_clone`]: Now track references better and detect more cases
[#9701](#9701)
<!-- changelog_checked -->

Author: bors

clippy_lints/src/casts/cast_slice_different_sizes.rs

@@ -54,7 +54,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, msrv: &Msrv
 
                         diag.span_suggestion(
                             expr.span,
-                            &format!("replace with `ptr::slice_from_raw_parts{mutbl_fn_str}`"),
+                            format!("replace with `ptr::slice_from_raw_parts{mutbl_fn_str}`"),
                             sugg,
                             rustc_errors::Applicability::HasPlaceholders,
                         );

clippy_lints/src/dereference.rs

@@ -1282,10 +1282,10 @@ fn referent_used_exactly_once<'tcx>(
             possible_borrowers.push((body_owner_local_def_id, PossibleBorrowerMap::new(cx, mir)));
         }
         let possible_borrower = &mut possible_borrowers.last_mut().unwrap().1;
-        // If `only_borrowers` were used here, the `copyable_iterator::warn` test would fail. The reason is
-        // that `PossibleBorrowerVisitor::visit_terminator` considers `place.local` a possible borrower of
-        // itself. See the comment in that method for an explanation as to why.
-        possible_borrower.bounded_borrowers(&[local], &[local, place.local], place.local, location)
+        // If `place.local` were not included here, the `copyable_iterator::warn` test would fail. The
+        // reason is that `PossibleBorrowerVisitor::visit_terminator` considers `place.local` a possible
+        // borrower of itself. See the comment in that method for an explanation as to why.
+        possible_borrower.at_most_borrowers(cx, &[local, place.local], place.local, location)
             && used_exactly_once(mir, place.local).unwrap_or(false)
     } else {
         false

clippy_lints/src/format_args.rs

@@ -377,7 +377,7 @@ fn check_format_in_format_args(cx: &LateContext<'_>, call_site: Span, name: Symb
         call_site,
         &format!("`format!` in `{name}!` args"),
         |diag| {
-            diag.help(&format!(
+            diag.help(format!(
                 "combine the `format!(..)` arguments with the outer `{name}!(..)` call"
             ));
             diag.help("or consider changing `format!` to `format_args!`");

clippy_lints/src/large_enum_variant.rs

@@ -111,7 +111,7 @@ impl<'tcx> LateLintPass<'tcx> for LargeEnumVariant {
                         );
                         diag.span_label(
                             def.variants[variants_size[1].ind].span,
-                            &if variants_size[1].fields_size.is_empty() {
+                            if variants_size[1].fields_size.is_empty() {
                                 "the second-largest variant carries no data at all".to_owned()
                             } else {
                                 format!(

clippy_lints/src/len_zero.rs

@@ -361,7 +361,7 @@ fn check_for_is_empty<'tcx>(
             db.span_note(span, "`is_empty` defined here");
         }
         if let Some(self_kind) = self_kind {
-            db.note(&output.expected_sig(self_kind));
+            db.note(output.expected_sig(self_kind));
         }
     });
 }

clippy_lints/src/lib.rs

@@ -336,7 +336,7 @@ pub fn read_conf(sess: &Session, path: &io::Result<Option<PathBuf>>) -> Conf {
         Ok(Some(path)) => path,
         Ok(None) => return Conf::default(),
         Err(error) => {
-            sess.struct_err(&format!("error finding Clippy's configuration file: {error}"))
+            sess.struct_err(format!("error finding Clippy's configuration file: {error}"))
                 .emit();
             return Conf::default();
         },

clippy_lints/src/loops/explicit_counter_loop.rs

@@ -77,7 +77,7 @@ pub(super) fn check<'tcx>(
                                 applicability,
                             );
 
-                            diag.note(&format!(
+                            diag.note(format!(
                                 "`{name}` is of type `{int_name}`, making it ineligible for `Iterator::enumerate`"
                             ));
                         },

clippy_lints/src/manual_async_fn.rs

@@ -76,7 +76,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualAsyncFn {
                                 let help = format!("make the function `async` and {ret_sugg}");
                                 diag.span_suggestion(
                                     header_span,
-                                    &help,
+                                    help,
                                     format!("async {}{ret_snip}", &header_snip[..ret_pos]),
                                     Applicability::MachineApplicable
                                 );

clippy_lints/src/manual_strip.rs

@@ -109,7 +109,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualStrip {
 
                     let test_span = expr.span.until(then.span);
                     span_lint_and_then(cx, MANUAL_STRIP, strippings[0], &format!("stripping a {kind_word} manually"), |diag| {
-                        diag.span_note(test_span, &format!("the {kind_word} was tested here"));
+                        diag.span_note(test_span, format!("the {kind_word} was tested here"));
                         multispan_sugg(
                             diag,
                             &format!("try using the `strip_{kind_word}` method"),

clippy_lints/src/methods/inefficient_to_string.rs

@@ -36,7 +36,7 @@ pub fn check(
                 expr.span,
                 &format!("calling `to_string` on `{arg_ty}`"),
                 |diag| {
-                    diag.help(&format!(
+                    diag.help(format!(
                         "`{self_ty}` implements `ToString` through a slower blanket impl, but `{deref_self_ty}` has a fast specialization of `ToString`"
                     ));
                     let mut applicability = Applicability::MachineApplicable;