Make borrow check results available to Clippy

[smoelius]

These changes allow Clippy to reconstruct the MIR on which the borrow checker is run, so that Clippy can reproduce its results.

Most of the changes are about making functions public. A notable exception is the addition of the NllCtxt structure. The purpose of this struct is to make it easier to call nll::compute_regions. IMHO, the introduction of this struct also untangles the code a bit (i.e., helps to expose what depends on what), as an additional benefit.

For context, Clippy now uses a MIR visitor called PossibleBorrowers. Intuitively, this visitor produces an approximation of the compiler's borrower check results. But its imprecision can lead to false negatives in certain circumstances (see rust-lang/rust-clippy#9386 (comment), for example). My previous attempt to make PossibleBorrowers more precise did not end well: it had bad worst case complexity, which was exposed by some in-the-wild programs.

The reviewer (@Jarcho) and I have discussed ways of improving PossibleBorrowers. But one could argue that the "right" approach is to use "ground truth" rather than an approximation, "ground truth" being the results of the compiler's borrow check analysis.

You can see how these changes will be used in rust-lang/rust-clippy#10173. Relevant discussion starts at rust-lang/rust-clippy#10173 (comment).

[rustbot]

r? @TaKO8Ki

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

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[bors]

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

[oli-obk]

r? @oli-obk

I want to make sure this is the right approach, as just exposing some internal APIs is a refactoring hazard

[oli-obk]

I read this PR and the clippy PR and got a few questions now:

• is your main problem with the mir building functions that you can't get that MIR anymore because it usually is already stolen?
• is the information you need (region information) computed exactly in the way you need it by borrowck, but then thrown away instead of returned in https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/query/struct.BorrowCheckResult.html?
  • if so, would polonius facts give you all the information that you need, too?

[smoelius]

To answer your questions, I need to enumerate a couple of my assumptions.

The way I use these results is to answer questions of the form: are there borrowers of a variable beyond some set of known borrowers? (Please see here for my details.)

The assumptions are:

1. Using the region information is a reasonable way to answer such questions.
2. Rerunning nll::compute_regions is a reasonable way to obtain the region information.

• is your main problem with the mir building functions that you can't get that MIR anymore because it usually is already stolen?

Assuming 1 and 2, yes.

• is the information you need (region information) computed exactly in the way you need it by borrowck, but then thrown away instead of returned in https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/query/struct.BorrowCheckResult.html?

I'm a little unsure about the "exactly" part. But assuming 1, yes.

• if so, would polonius facts give you all the information that you need, too?

Maybe? I know very little about Polonius, but there are a couple of things that concern me.

First, the intent of this PR is to give Clippy access to the same borrow check results the compiler uses in deciding whether code is valid. If I understand correctly, Polonius is not currently used in production. So I would be concerned that using it in Clippy could inadvertently cause bugs in Clippy.

A secondary concern is this:

rust/compiler/rustc_borrowck/src/consumers.rs

Line 30 in bd4a96a

/// * Polonius is highly unstable, so expect regular changes in its signature or other details.

I understand the compiler APIs provide no stability guarantees, generally. But I worry about Clippy relying on a feature that is explicitly unstable.

[smoelius]

@oli-obk Were there any other questions I could answer on this topic?

As an aside, if you're concerned about exposing internal APIs, another idea could be the following: add a flag to the compiler configuration to clone the various borrow checker structures after they are created, and then expose the cloned structures through a query.

[oli-obk]

Sorry, I read that but then must have dismissed the notification before replying

So... about obtaining the right MIR:

My recommendation would be to change RedundantClone to run right after borrowck, while the input to borrowck is still available. Basically, you can overwrite the borrowck query, run the original query within your overwrite, then run the RedundantClone lint on the promoted_mir (which isn't stolen at this point). This would also make the redundant clone lint stop using the optimized_mir.

For the Dereferencing lint it's a bit harder to do the refactoring, but it should work similarly by running it in the borrowck query.

This refactoring seems useful on its own, as it allows clippy to stop using optimized_mir and thus get reliable input and potentially even be faster due to less work (I know optimized_mir is already cheap in miri due to various flags, but still).

The second part is obtaining the borrowing information you need. Since what you actually want is to access NllOutput, not to run compute_regions, you could make the return_body_with_facts arg of do_mir_borrowck be a Mode enum instead, that has Normal, Polonius, Clippy modes, and in clippy mode it'll return the values you need similarly to the polonius Option<Box<BodyWithBorrowckFacts<'tcx>>> return value.

Do you think that plan could work?

[smoelius]

My recommendation would be to change RedundantClone to run right after borrowck, while the input to borrowck is still available. Basically, you can overwrite the borrowck query, run the original query within your overwrite, then run the RedundantClone lint on the promoted_mir (which isn't stolen at this point). This would also make the redundant clone lint stop using the optimized_mir.

I don't think I understand. When you say "overwrite the borrowck query," you mean replace its code?

Are you suggesting to somehow inline RedundantClone's code into the replacement?

(I'm sorry if I'm being dense.)

RedundantClone is a late pass lint, which I think means it gets run here:

rust/compiler/rustc_interface/src/passes.rs

Line 841 in 22f247c

rustc_lint::BuiltinCombinedLateLintPass::new()

The most straightforward way I can see realize your idea would be to add something like a "middle" pass that runs just after this:

rust/compiler/rustc_interface/src/passes.rs

Lines 786 to 788 in 22f247c

	sess.time("MIR_borrow_checking", || {
	tcx.hir().par_body_owners(|def_id| tcx.ensure().mir_borrowck(def_id));
	});

And then to change RedundantClone and NeedlessBorrow to be "middle" pass lints.

Do you think that could work?

The second part is obtaining the borrowing information you need. Since what you actually want is to access NllOutput, not to run compute_regions, you could make the return_body_with_facts arg of do_mir_borrowck be a Mode enum instead, that has Normal, Polonius, Clippy modes, and in clippy mode it'll return the values you need similarly to the polonius Option<Box<BodyWithBorrowckFacts<'tcx>>> return value.

I think this part makes sense to me.

[oli-obk]

Sorry, I got compiler-driver brain worms. Let me explain from the beginning:

the mir_borrowck query is essentially just a function that the compiler calls (and memoizes, and caches to disk, and ...). The way this works is by rustc having a big function pointer table where it looks up what function to call when anyone calls tcx.mir_borrowck(some_id). compiler drivers (like clippy, rustdoc or miri) can overwrite any query's function pointer. For clippy this can be done in

rust/src/tools/clippy/src/driver.rs

Line 131 in 01ffa51

fn config(&mut self, config: &mut interface::Config) {

. For existing examples that overwrite queries you can look at rustdoc:

rust/src/librustdoc/core.rs

Line 263 in 2625518

override_queries: Some(|_sess, providers, _external_providers| {

so... basically you make a new query, and within that query you can call do_mir_borrowck manually, by basically copying

rust/compiler/rustc_borrowck/src/lib.rs

Line 136 in 7e2ecb3

fn mir_borrowck(tcx: TyCtxt<'_>, def: ty::WithOptConstParam<LocalDefId>) -> &BorrowCheckResult<'_> {

and using the Mode::Clippy.

This way you get the NllOutput and you can then run the logic of RedundantClone::check_fn directly, without needing to build a LintContext or similar. It's a different way of doing lints, for any lints that don't actually visit expressions but only work on MIR, there's no fundamental difference.

There are some things you'll have to do manually, as you don't have a LintContext that gives you a ParamEnv or similar, but all of that information can be obtained via tcx.param_env(def_id) or similar queries (this is what LintContext does internally)

[smoelius]

@oli-obk Thanks very much for your patient response. I didn't know it was possible overwrite a query's function pointer.

I think I'm going to have to try implement this idea to understand its implications.

I'll reply once I've had a chance to do so. Thanks again.

[oli-obk]

Oh one thing I just remembered: you can do the "run lints in borrowck query" part even without changing borrowck or rustc at all. This may be worthwile on its own and can done entirely within the clippy repo. Should probably bug some other clippy devs about whether that approach seems maintainable and generally good

[apiraino]

Switching to waiting on author to incorporate changes (by readin the latest comments). Feel free to request a review with @rustbot ready, thanks!

@rustbot author

[jyn514]

Hi @smoelius, it's been a while - are you still planning to work on this? Do you know how to make progress? Feel free to ask for help here or on Zulip if you get stuck.

[smoelius]

Hi, @jyn514. Thanks for checking in. Yes, I still plan to work on this. I have simply had other priorities. I will try to get back to this soon. Thanks.

[smoelius]

@oli-obk I've been trying to implement your suggestion. (Here is the latest version of the associated Clippy PR.) However, I've run into some difficulties.

TBH, even if these difficulties can be overcome, I am concerned that writing lints this way is too prohibitive. Is exposing BodyWithBorrowckFacts through a query off the table?

The problems I still face include the following:

• Clippy's dogfood test currently fails with "cycle detected" errors involving type checking, borrow checking, and effective visibilities. The following is an example:

    error[E0391]: cycle detected when computing type of `recursive::<impl at src/recursive.rs:82:1: 82:21>::warnings::{opaque#0}`
    --> src/recursive.rs:117:30
        |
    117 |     pub fn warnings(self) -> impl Iterator<Item = ClippyWarning> {
        |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        |
    note: ...which requires borrow-checking `recursive::<impl at src/recursive.rs:82:1: 82:21>::warnings`...
    --> src/recursive.rs:117:5
        |
    117 |     pub fn warnings(self) -> impl Iterator<Item = ClippyWarning> {
        |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        = note: ...which requires checking effective visibilities...
        = note: ...which again requires computing type of `recursive::<impl at src/recursive.rs:82:1: 82:21>::warnings::{opaque#0}`, completing the cycle
  

• It's not clear (to me) how to cache a combined lint pass structure across mir_borrowck calls. I'm trying to run the lints that need borrow check information in a combined pass, similar to how existing lints are run. Right now, I'm reconstructing the LintPass objects and the combined pass on each call to mir_borrowck (obviously bad). Note that the lint pass members must not outlive 'tcx, which complicates things.
• Clippy uses enter_lint_attr and exit_lint_attr to determined the MSRV in tests (example). Since mir_borrowck is effectively called on a body, it sidesteps those attributes. One could probably walk the enclosing items, etc. to find those attributes, but having two different approaches to finding them seems like a negative.

If exposing BodyWithBorrowckFacts through a query is off the table, I am open to suggestions on diagnosing/fixing the above.

[rust-log-analyzer]

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:c85c95e3d7251135ab7dc9ce3241c5835cc595a9)
Download action repository 'actions/upload-artifact@v3' (SHA:0b7f8abb1508181956e8e162db84b466c27e18ce)
Complete job name: PR - mingw-check-tidy
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  CI_JOB_NAME: mingw-check-tidy
---
Building wheels for collected packages: reuse
  Building wheel for reuse (pyproject.toml): started
  Building wheel for reuse (pyproject.toml): finished with status 'done'
  Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=180116 sha256=351235b2326fb4db7a18e257e13ce7896c5f77339521e2c2612e71e154800a19
  Stored in directory: /tmp/pip-ephem-wheel-cache-k7wtee2d/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
  Attempting uninstall: setuptools
    Found existing installation: setuptools 59.6.0
    Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
---
Successfully tagged rust-ci:latest
Built container sha256:a9d3e741559db16c8c47646338d56dde459bf9fb7a80523955b6cfca423ddf7e
Uploading finished image to https://ci-caches.rust-lang.org/docker/39289e18f1ac1b1a573d1a3c658528cab0cbfec4029e2def8ed615ecd4b1295e23213ab5b134f4a50a4bd45af0a8a4266b7571ba256dc217696d0f040efcd003

<botocore.awsrequest.AWSRequest object at 0x7fa14aa90090>
gzip: stdout: Broken pipe
xargs: docker: terminated by signal 13
[CI_JOB_NAME=mingw-check-tidy]
[CI_JOB_NAME=mingw-check-tidy]
---
   Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
    Finished release [optimized] target(s) in 12.93s
##[endgroup]
fmt check
##[error]Diff in /checkout/compiler/rustc_lint/src/context.rs at line 190:
         &mut self,
         &mut self,
         pass: impl for<'tcx> Fn(TyCtxt<'tcx>) -> LateLintPassObject<'tcx>
-            + 'static
-            + sync::DynSend
-            + sync::DynSync,
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/rustfmt/bin/rustfmt" "--config-path" "/checkout" "--edition" "2021" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_lint/src/tests.rs" "/checkout/compiler/rustc_lint/src/context.rs" "/checkout/compiler/rustc_lint/src/passes.rs" "/checkout/compiler/rustc_lint/src/lints.rs" "/checkout/compiler/rustc_lint/src/nonstandard_style/tests.rs" "/checkout/compiler/rustc_lint/src/pass_by_value.rs" "/checkout/compiler/rustc_lint/src/multiple_supertrait_upcastable.rs" "/checkout/compiler/rustc_lint/src/for_loops_over_fallibles.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
+        + 'static
+        + 'static
+        + sync::DynSend
+        + sync::DynSync,
     ) {
         self.late_passes.push(Box::new(pass));
     }
##[error]Diff in /checkout/compiler/rustc_lint/src/context.rs at line 200:
         &mut self,
         &mut self,
         pass: impl for<'tcx> Fn(TyCtxt<'tcx>) -> LateLintPassObject<'tcx>
-            + 'static
-            + sync::DynSend
-            + sync::DynSync,
+        + 'static
+        + sync::DynSend
+        + sync::DynSync,
     ) {
         self.late_module_passes.push(Box::new(pass));

[oli-obk]

It's not off the table, I would just like to figure out how to ensure we don't end up exposing everything in the compiler that isn't actually used within the compiler. I'd like to have a good way for clippy to just register its very own queries, but that's not possible at present.

Stepping back from the exact implementation: What you want is a way to compute (and obtain!) things from inside the compiler. As a side constraint, we don't want to make the compiler internal APIs more complex.

So, the options I see are:

• what you originally proposed, just returning more things from the mir_borrowck query
• what you did now, running lints in the query (bad because it is very different from how all other crates work, and now you have the issue of getting some precomputed data into the query)
• unsafe shenanigans like writing the data into a global static hashmap. So you invoke borrowck, throw away the result, but now you can fetch the information from the global static (ugh, that's obviously ugly)
• add a new query to rustc, clippy_borrowck_results or something making it obvious that it is not to be used by anyone outside of clippy. I would go as far as to not even give it an implementation at all, and instead only overwrite it in clippy. To ensure you don't duplicate work, you can make clippy's mir_borrowck query invoke your new query and just return the subset of data that it would have otherwise computed. (unclear if really better than just sticking the information into the borrowck results like you originally had)

So yea, I'm not sure where to go from here, but I guess as long as clippy is ok with that lint getting removed at a moments notice if borrowck makes a large enough refactoring so that we can't provide this information anymore easily, we can go with your original option.

[smoelius]

Thanks very much for your detailed response.

So yea, I'm not sure where to go from here, but I guess as long as clippy is ok with that lint getting removed at a moments notice if borrowck makes a large enough refactoring so that we can't provide this information anymore easily, we can go with your original option.

May I ask, can you see a point in the future when the borrow checker is more stable?

My impression is that "keeping Clippy working" is a factor influencing API changes today. Put another way, API changes may require changes to a Clippy lint, but the information the lint needs will still be available in some form. "Keeping Clippy working" is one of the reasons why Clippy is mirrored in the Rust repo, for example. (Is this impression wrong?)

Can you see a point in the future when a Clippy lint could rely on borrow checker results, and the same standard (assuming it's correct) could apply?

[oli-obk]

Can you see a point in the future when a Clippy lint could rely on borrow checker results, and the same standard (assuming it's correct) could apply?

yea, I'd assume once we get poloniusification (not switching to, but migrating to a similar scheme) done, we have a certain stability of the output for a decade or so. But that migration may be unduly burdened by having to support clippy. It may be reasonable to port, or it may require rewriting every lint using borrowck results from scratch.

[smoelius]

It sounds then like the most reasonable option would be to wait for the Polonius migration to complete, and revisit this issue thereafter. Would you agree?

[oli-obk]

That seems reasonable, but you may be waiting over a year.

My impression is that "keeping Clippy working" is a factor influencing API changes today. Put another way, API changes may require changes to a Clippy lint, but the information the lint needs will still be available in some form. "Keeping Clippy working" is one of the reasons why Clippy is mirrored in the Rust repo, for example. (Is this impression wrong?)

No, this impression is spot on. Maybe it would be ok for the lint to just not do anything for a bit while it gets patched up, instead of having to fix it immediately when a big refactoring happens?

[smoelius]

Maybe it would be ok for the lint to just not do anything for a bit while it gets patched up, instead of having to fix it immediately when a big refactoring happens?

The lints affected by these changes would redundant_clone and needless_borrow. Your suggestion is that the Clippy source code somehow include changed and unchanged versions of those lints, so that:

• Effects of borrow checker refactorings on Clippy would be visible.
• Clippy continues to function either way.

Is that right? (Sorry if I'm misunderstanding.)

[oli-obk]

Basically we'd just suddenly feed you empty borrowck data. This will break those lints, and it would then be up to the refactoring author and reviewer to decide whether to ask you to fix it or whether they can fix it in a follow up PR. This may end up in stable for a release or two if it can't be done fast enough

[smoelius]

@Jarcho Can I ask your opinion on having two versions of redundant_clone and needless_borrow in Clippy, one version like now and one version that uses borrowck results?

Based on #108328 (comment) and what follows, I see the implied subgoals as:

• The borrowck versions should be built regularly as part of the Rust repo, so that API changes affecting them are visible.
• The borrowck versions should be easy to disable, if fixing them would be onerous.
• Clippy's infrastructure should allow testing both versions, since the borrowck versions should produce additional warnings.
• Having the two versions should not be detrimental to Clippy's performance.

The easiest way I can see to achieve the above is:

• put the borrowck versions behind both a Cargo feature and a clippy.toml setting.

Both options would need to be enabled to use the borrowck versions of the lints. The Cargo feature would be on-by-default, but could be turned off by Rust developers. The clippy.toml setting would be off-by-default, but could be enabled by ui-toml tests and users wanting to try out the borrowck versions.

What you do think?

(@oli-obk Please correct me if I've misinterpreted or contradicted anything you wrote.)

[oli-obk]

I don't think we need cargo features. Just having borrowck return an option that we can turn into None at some point seems sufficient.

[smoelius]

I don't think we need cargo features. Just having borrowck return an option that we can turn into None at some point seems sufficient.

The situation I was imagining was (say) the contents of the BodyWithBorrowckFacts struct need to change, but incorporating those changes into Clippy would be a hassle. A Cargo feature could provide an easy way to just "turn off" that code and avoid any associated compilation problems.

[oli-obk]

I expect changes to be either small enough for that to be ok, or massive enough for it to become unrecognizeable. We could keep the old datastructures, so no code breakage, but return None.

Though at this point I'm feeling like this puts too much burden on the maintenance side of clippy. I'm sorry for making you go through all these hoops only to end up accepting that your original proposal is probably the best one. Sticking the data you need into a field that rustc sets to None, but clippy can safely unwrap by overwriting the query. As long as the field is named and documented in ways that make it obvious to rustc internal query invokers that they should not use the field (it's None anyway), we can go with that and figure out issues with maintenance when we get there.

[Jarcho]

Maintaining two implementations of every lint that uses the borrowck results isn't a great option. Especially as more lints start to make use of it the maintenance burden will become untenable.

Would it be reasonable to commit to a narrow interface wrapping the results? With the current two lints the only thing that needs to be queried is if a local has more than a single borrower as a given location.

[oli-obk]

That seems like something any change to the borrow checker could keep around, yes

[Jarcho]

That sounds like a good way forward then. As far as deciding whether or not to emit a lint, we should be fine with just three queries:

• Is a local currently borrowed
• Is a local currently mutably borrowed
• Is a local currently borrowed by multiple locals

More might be needed to improve lint messages, but losing anything here temporarily wouldn't be too big of a deal.

[smoelius]

@Jarcho You're imagining something like the possible borrower adaptation I proposed, but in the compiler instead of in Clippy?

Before going down that path, please let me clear up some confusion I may have caused. When I said "two versions" of those lints, I meant in an intuitive sense, e.g., there would be a handful of locations where those lints would choose between one of two code paths.

To be a little more concrete, I imagined the clippy.toml setting doing mainly two things:

• Selecting either the existing possible_borrower implementation, or one that uses borrowck results (the one I linked above).
• Selecting either optimized mir or borrowck mir in redundant_clone here.

Adding to that, I wouldn't expect there to be much practical difference between having just a clippy.toml setting, or having a clippy.toml setting and a Cargo feature.

In short, I wouldn't expect Clippy's maintenance burden to be very high, either way.

Does this affect your opinion at all, @Jarcho?

[Jarcho]

For the lints that currently exist, the maintenance burden isn't all that high. With that said, both forms would still need to be fully tested. Even with testing it still runs the risk that switching over could still completely break the lints (very unlikely for the current lints, but could be more so for future lints). See #108944 as an example of completely breaking redundant_clone in a way that only got detected after the fact.

My main point here is having a limited interface that can, with reasonable ease, also be implemented by future borrowck implementations is better than picking through things on clippy's side.

So the answer to your original question is yes, but limited to the minimum interface clippy actually needs.

[smoelius]

So the answer to your original question is yes, but limited to the minimum interface clippy actually needs.

OK. I'll try to make it happen.

[bors]

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

[Dylan-DPC]

@smoelius any updates on this?

[smoelius]

@Dylan-DPC Not currently. But thank you for the ping. I will try to get back to this soon.

[smoelius]

... we should be fine with just three queries:

• Is a local currently borrowed
• Is a local currently mutably borrowed
• Is a local currently borrowed by multiple locals

@Jarcho Could you please elaborate on how you see these being used by Clippy's existing lints?

EDIT: The PossibleBorrowerMap methods that redundant_clone and needless_borrow_for_generic_arg currently use are only_borrowers and bounded_borrowers. I'm presuming that the uses of those methods would be replaced by the queries above, and I am wondering what you had in mind.

[smoelius]

I'm going to close this. I may revisit this at some point in the future.