Improve `possible_borrower` #9701

smoelius · 2022-10-24T08:59:30Z

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

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.

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:

    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:

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.

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.

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

rust-highfive · 2022-10-24T08:59:34Z

r? @giraffate

(rust-highfive has picked a reviewer for you, use r? to override)

bors · 2022-10-27T06:57:29Z

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

xFrednet · 2022-10-29T12:17:21Z

Hey, thank you for the PR. Could you maybe expand on the changelog entry a bit and explain how you specific improved it? 🙃

smoelius · 2022-10-30T11:21:55Z

@xFrednet Please tell me if what I have now suffices.

Jarcho · 2022-10-31T00:52:03Z

The changelog lines shouldn't mention internal changes. only things visible from the user's perspective.

smoelius · 2022-10-31T09:17:16Z

The changelog lines shouldn't mention internal changes. only things visible from the user's perspective.

👍 I'm going to let @xFrednet reply before revising again, though.

xFrednet · 2022-10-31T10:40:23Z

I agree with @Jarcho, these changelog entries seam to focus on the actual change and not the user-facing effect. I guess that you improved the tracking of lifetimes to avoid false positives?

The problem with mentioning rustc related objects like MaybeStorageLive are also not known to everyone. I at least never heard of it 😅. With this, I would probably just read the uitest files and see what has chained.

@smoelius Thank you for taking the time to figure this out and also to document this discussion!

smoelius · 2022-10-31T23:53:25Z

I went with:

changelog: improved the tracking of references to avoid false negatives in `needless_borrow` and `redundant_clone`

But please tell me if this is still not what's desired. Thank you for your feedback, @xFrednet @Jarcho.

xFrednet · 2022-11-03T13:17:17Z

That should be good enough :), thank you!

Jarcho · 2022-11-26T22:50:46Z

Going to take over this. r? @Jarcho

Unless @giraffate has any objections.

smoelius · 2022-11-26T23:41:26Z

Thank you, @Jarcho. Thank you, @giraffate.

Jarcho · 2022-12-09T17:16:18Z

clippy_utils/src/mir/possible_borrower.rs

+        let maybe_live = &self.maybe_live;
+
+        let mut queued = BitSet::new_empty(self.body.local_decls.len());
+        let mut deque = VecDeque::with_capacity(self.body.local_decls.len());


Can this not just be a Vec? I don't see a reason to process the borrowers in order here.

It would also be better to allocate both of these up front when creating the PossibleBorrowerMap and just clear them at the start of the function.

smoelius · 2022-12-13T17:59:32Z

Sorry for the delay, @Jarcho. I rebased and addressed your comments thus far.

Jarcho · 2022-12-20T04:27:41Z

Thank you. @bors r+

bors · 2022-12-20T04:27:46Z

📌 Commit 9d1cb71 has been approved by Jarcho

It is now in the queue for this repository.

bors · 2022-12-20T04:27:55Z

⌛ Testing commit 9d1cb71 with merge be98a0e...

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)

bors · 2022-12-20T04:30:09Z

💔 Test failed - checks-action_test

smoelius · 2022-12-20T10:36:42Z

Thank you, @Jarcho, and sorry for the trouble. I rebased and pushed what I think is a fix for the build failure.

Jarcho · 2022-12-22T14:59:43Z

@bors retry

xFrednet · 2022-12-22T15:05:33Z

@bors r=Jarcho

Bors removes the approval, after any changes to the last commit. This should start the run :)

bors · 2022-12-22T15:05:35Z

📌 Commit 4dbd8ad has been approved by Jarcho

It is now in the queue for this repository.

bors · 2022-12-22T15:08:07Z

⌛ Testing commit 4dbd8ad with merge 4fe3727...

bors · 2022-12-22T15:19:28Z

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

smoelius · 2022-12-22T15:41:05Z

Thanks again, @Jarcho.

Partially revert #9701 This partially reverts #9701 due to #10134 r? `@flip1995` changelog: None

Only applies to those targets which opt-in to this lint. The lint was recently expanded to catch new instances in rust-lang/rust-clippy#9701 . Bug: 118659 Change-Id: Ic72b16783d0e5f6a804615fa7f792206fd2a534e Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/785723 Reviewed-by: Sen Jiang <senj@google.com> Commit-Queue: Auto-Submit <auto-submit@fuchsia-infra.iam.gserviceaccount.com> Reviewed-by: Joseph Ryan <josephry@google.com> Reviewed-by: Marc Khouri <mnck@google.com> Reviewed-by: Steven Grady <slgrady@google.com> Fuchsia-Auto-Submit: Dan Johnson <computerdruid@google.com>

rust-highfive assigned giraffate Oct 24, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 24, 2022

rust-highfive assigned Jarcho and unassigned giraffate Nov 26, 2022

Jarcho reviewed Dec 9, 2022

View reviewed changes

smoelius force-pushed the improve-possible-borrower branch from 204bed7 to 9d1cb71 Compare December 13, 2022 17:50

smoelius added 6 commits December 20, 2022 05:12

Use rustc_mir_dataflow::impls::MaybeStorageLive

cd3d38a

Add tests

c6477eb

Improve possible_borrower

ed519ad

Fix adjacent code

26df551

Address review comments

c7dc961

Address rust-lang/rust#105659

4dbd8ad

smoelius force-pushed the improve-possible-borrower branch from 9d1cb71 to 4dbd8ad Compare December 20, 2022 10:33

bors merged commit 4fe3727 into rust-lang:master Dec 22, 2022

smoelius deleted the improve-possible-borrower branch December 22, 2022 15:40

Jarcho mentioned this pull request Jan 2, 2023

Huge CPU and RAM usage in redundant_clone/possible_borrower on long functions #10134

Open

This was referenced Jan 12, 2023

Rustup #10191

Merged

Partially revert #9701 #10192

Merged

bors added a commit that referenced this pull request Jan 12, 2023

Auto merge of #10192 - Jarcho:revert_9701, r=flip1995

7f27e2e

Partially revert #9701 This partially reverts #9701 due to #10134 r? `@flip1995` changelog: None

smoelius mentioned this pull request Jan 12, 2023

Address #10134 OOM/timeout #10173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `possible_borrower` #9701

Improve `possible_borrower` #9701

smoelius commented Oct 24, 2022 •

edited by xFrednet

Loading

rust-highfive commented Oct 24, 2022

bors commented Oct 27, 2022

xFrednet commented Oct 29, 2022

smoelius commented Oct 30, 2022

Jarcho commented Oct 31, 2022

smoelius commented Oct 31, 2022

xFrednet commented Oct 31, 2022

smoelius commented Oct 31, 2022

xFrednet commented Nov 3, 2022

Jarcho commented Nov 26, 2022

smoelius commented Nov 26, 2022

Jarcho Dec 9, 2022

Jarcho Dec 9, 2022

smoelius commented Dec 13, 2022

Jarcho commented Dec 20, 2022

bors commented Dec 20, 2022

bors commented Dec 20, 2022

bors commented Dec 20, 2022

smoelius commented Dec 20, 2022

Jarcho commented Dec 22, 2022

xFrednet commented Dec 22, 2022

bors commented Dec 22, 2022

bors commented Dec 22, 2022

bors commented Dec 22, 2022

smoelius commented Dec 22, 2022

Improve possible_borrower #9701

Improve possible_borrower #9701

Conversation

smoelius commented Oct 24, 2022 • edited by xFrednet Loading

rust-highfive commented Oct 24, 2022

bors commented Oct 27, 2022

xFrednet commented Oct 29, 2022

smoelius commented Oct 30, 2022

Jarcho commented Oct 31, 2022

smoelius commented Oct 31, 2022

xFrednet commented Oct 31, 2022

smoelius commented Oct 31, 2022

xFrednet commented Nov 3, 2022

Jarcho commented Nov 26, 2022

smoelius commented Nov 26, 2022

Jarcho Dec 9, 2022

Choose a reason for hiding this comment

Jarcho Dec 9, 2022

Choose a reason for hiding this comment

smoelius commented Dec 13, 2022

Jarcho commented Dec 20, 2022

bors commented Dec 20, 2022

bors commented Dec 20, 2022

bors commented Dec 20, 2022

smoelius commented Dec 20, 2022

Jarcho commented Dec 22, 2022

xFrednet commented Dec 22, 2022

bors commented Dec 22, 2022

bors commented Dec 22, 2022

bors commented Dec 22, 2022

smoelius commented Dec 22, 2022

Improve `possible_borrower` #9701

Improve `possible_borrower` #9701

smoelius commented Oct 24, 2022 •

edited by xFrednet

Loading