Location-sensitive polonius prototype: endgame #134980

lqd · 2024-12-31T18:13:16Z

This PR sets up the naive location-sensitive analysis end-to-end, and replaces the location-insensitive analysis. It's roughly all the in-progress work I wanted to land for the prototype, modulo cleanups I still want to do after the holidays, or the polonius debugger, and so on.

Here, we traverse the localized constraint graph, have to deal with kills and time-traveling (👌), and record that as loan liveness for the existing scope and active loans computations.

Then the near future looks like this, especially if the 2025h1 project goal is accepted:

gradually bringing it up to completion
analyzing and fixing the few remaining test failures
going over the numerous fixmes in this prototype (one of which is similar to a hang on one test's millions and millions of constraints)
trying to see how to lower the impact of the lack of NLL liveness optimization on diagnostics, and their categorization of local variables and temporaries (the vast majority of blessed expectations differences), as well as the couple ICEs trying to find an NLL constraint to blame for errors.
dealing with the theoretical weakness around kills, conflating reachability for the two TCS, etc that is described ad nauseam in the code.
switching the compare mode to the in-tree implementation, and blessing the diagnostics
apart from the hang, it's not catastrophically slower on our test suite, so then we can try to enable it on CI
checking crater, maybe trying to make it faster :3, etc.

I've tried to gradually introduce this PR's work over 4 commits, because it's kind of subtle/annoying, and Niko/I are not completely convinced yet. That one comment explaining the situation is maybe 30% of the PR 😓. Who knew that spacetime reachability and time-traveling could be mind bending.

I kinda found this late and the impact on this part of the computation was a bit unexpected to us. A bit more care/thought will be needed here. I've described my plan in the comments though. In any case, I believe we have the current implementation is a conservative approximation that shouldn't result in unsoundness but false positives at worst. So it feels fine for now.

r? @jackh726

Fixes #127628 -- which was a assertion triggered for a difference in loan computation between NLLs and the location-insensitive analysis. That doesn't exist anymore so I've removed this crash test.

lqd · 2024-12-31T18:31:29Z

_{borrow checking is easy with time travel and spacetime reachability}

jackh726

Just starting to review

compiler/rustc_borrowck/src/polonius/mod.rs

lqd · 2025-01-08T10:15:12Z

Rebased and marked ready to review (but jack had already started) now that #134920 has landed.

lqd · 2025-01-09T15:06:08Z

(This last force push was to fix conflicts and can be ignored)

jackh726

Some more review comments not all the way done.

compiler/rustc_borrowck/src/polonius/loan_liveness.rs

compiler/rustc_borrowck/src/dataflow.rs

jackh726 · 2025-01-09T23:28:49Z

compiler/rustc_borrowck/src/polonius/mod.rs

@@ -98,7 +98,7 @@ impl PoloniusContext {
    pub(crate) fn compute_loan_liveness<'tcx>(
        &self,
        tcx: TyCtxt<'tcx>,
-        regioncx: &RegionInferenceContext<'tcx>,
+        regioncx: &mut RegionInferenceContext<'tcx>,


I'm looking at this. It might be a slightly bigger refactor, but I wonder if it makes sense to move live_loans out of RegionInferenceContext. You basically need to manually pass it anywhere you use is_loan_live_at, but there must already be some special handling wherever that is, or it would panic.

Thoughts?

Sure, I can look into it for a follow up PR. What’s the goal you’re trying to achieve?

My recollection is that only the scope precomputer accesses these, so while there’s no special handling per se, it sounds possible to pass these optional live loans when creating the Borrows data flow computation instead.

The NLL OutOfScopePrecomputer also uses the regioncx to check where a loan is live, so the structure is similar between the two analyses.)

Mostly just trying to have cleaner code. To me, tracking some meta-state by storing an Option is just a recipe for a bug in the future (though I'll concede in this case its probably pretty unlikely given how rarely this code is actually touched).

I see. A possibility could be if we were to duplicate more of the borrowck into two instead of one handling multiple codepaths, we wouldn’t have options.

in NLLs some locals are marked live at all points if one of their regions escapes the function but that doesn't work in a flow-sensitive setting like polonius

we're in in the endgame now set up the location-sensitive analysis end to end: - stop recording inflowing loans and loan liveness in liveness - replace location-insensitive liveness data with live loans computed by reachability - remove equivalence between polonius scopes and NLL scopes, and only run one scope computation

it's a bit mind-bending

this addresses review comments while: - keeping the symmetry between the NLL and Polonius out of scope precomputers - keeping the unstable `calculate_borrows_out_of_scope_at_location` function to avoid churn for consumers

lqd · 2025-01-12T07:46:39Z

(Force pushed to fix conflicts, not to push new code that needs reviewing)

compiler/rustc_borrowck/src/polonius/loan_liveness.rs

jackh726 · 2025-01-10T13:32:46Z

compiler/rustc_borrowck/src/polonius/mod.rs

@@ -98,7 +98,7 @@ impl PoloniusContext {
    pub(crate) fn compute_loan_liveness<'tcx>(
        &self,
        tcx: TyCtxt<'tcx>,
-        regioncx: &RegionInferenceContext<'tcx>,
+        regioncx: &mut RegionInferenceContext<'tcx>,


Mostly just trying to have cleaner code. To me, tracking some meta-state by storing an Option is just a recipe for a bug in the future (though I'll concede in this case its probably pretty unlikely given how rarely this code is actually touched).

jackh726 · 2025-01-17T04:16:14Z

@bors r+

bors · 2025-01-17T04:16:17Z

📌 Commit 8ac045d has been approved by jackh726

It is now in the queue for this repository.

…kh726 Location-sensitive polonius prototype: endgame This PR sets up the naive location-sensitive analysis end-to-end, and replaces the location-insensitive analysis. It's roughly all the in-progress work I wanted to land for the prototype, modulo cleanups I still want to do after the holidays, or the polonius debugger, and so on. Here, we traverse the localized constraint graph, have to deal with kills and time-traveling (👌), and record that as loan liveness for the existing scope and active loans computations. Then the near future looks like this, especially if the 2025h1 project goal is accepted: - gradually bringing it up to completion - analyzing and fixing the few remaining test failures - going over the *numerous* fixmes in this prototype (one of which is similar to a hang on one test's millions and millions of constraints) - trying to see how to lower the impact of the lack of NLL liveness optimization on diagnostics, and their categorization of local variables and temporaries (the vast majority of blessed expectations differences), as well as the couple ICEs trying to find an NLL constraint to blame for errors. - dealing with the theoretical weakness around kills, conflating reachability for the two TCS, etc that is described ad nauseam in the code. - switching the compare mode to the in-tree implementation, and blessing the diagnostics - apart from the hang, it's not catastrophically slower on our test suite, so then we can try to enable it on CI - checking crater, maybe trying to make it faster :3, etc. I've tried to gradually introduce this PR's work over 4 commits, because it's kind of subtle/annoying, and Niko/I are not completely convinced yet. That one comment explaining the situation is maybe 30% of the PR 😓. Who knew that spacetime reachability and time-traveling could be mind bending. I kinda found this late and the impact on this part of the computation was a bit unexpected to us. A bit more care/thought will be needed here. I've described my plan in the comments though. In any case, I believe we have the current implementation is a conservative approximation that shouldn't result in unsoundness but false positives at worst. So it feels fine for now. r? `@jackh726` --- Fixes rust-lang#127628 -- which was a assertion triggered for a difference in loan computation between NLLs and the location-insensitive analysis. That doesn't exist anymore so I've removed this crash test.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#131806 (Treat other items as functions for the purpose of type-based search) - rust-lang#134290 (Windows x86: Change i128 to return via the vector ABI) - rust-lang#134980 (Location-sensitive polonius prototype: endgame) - rust-lang#135558 (Detect if-else chains with a missing final else in type errors) - rust-lang#135594 (fix error for when results in a rustdoc-js test are in the wrong order) - rust-lang#135601 (Fix suggestion to convert dereference of raw pointer to ref) - rust-lang#135604 (Expand docs for `E0207` with additional example) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#131806 (Treat other items as functions for the purpose of type-based search) - rust-lang#134980 (Location-sensitive polonius prototype: endgame) - rust-lang#135558 (Detect if-else chains with a missing final else in type errors) - rust-lang#135594 (fix error for when results in a rustdoc-js test are in the wrong order) - rust-lang#135601 (Fix suggestion to convert dereference of raw pointer to ref) - rust-lang#135604 (Expand docs for `E0207` with additional example) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#134980 - lqd:polonius-next-episode-7, r=jackh726 Location-sensitive polonius prototype: endgame This PR sets up the naive location-sensitive analysis end-to-end, and replaces the location-insensitive analysis. It's roughly all the in-progress work I wanted to land for the prototype, modulo cleanups I still want to do after the holidays, or the polonius debugger, and so on. Here, we traverse the localized constraint graph, have to deal with kills and time-traveling (👌), and record that as loan liveness for the existing scope and active loans computations. Then the near future looks like this, especially if the 2025h1 project goal is accepted: - gradually bringing it up to completion - analyzing and fixing the few remaining test failures - going over the *numerous* fixmes in this prototype (one of which is similar to a hang on one test's millions and millions of constraints) - trying to see how to lower the impact of the lack of NLL liveness optimization on diagnostics, and their categorization of local variables and temporaries (the vast majority of blessed expectations differences), as well as the couple ICEs trying to find an NLL constraint to blame for errors. - dealing with the theoretical weakness around kills, conflating reachability for the two TCS, etc that is described ad nauseam in the code. - switching the compare mode to the in-tree implementation, and blessing the diagnostics - apart from the hang, it's not catastrophically slower on our test suite, so then we can try to enable it on CI - checking crater, maybe trying to make it faster :3, etc. I've tried to gradually introduce this PR's work over 4 commits, because it's kind of subtle/annoying, and Niko/I are not completely convinced yet. That one comment explaining the situation is maybe 30% of the PR 😓. Who knew that spacetime reachability and time-traveling could be mind bending. I kinda found this late and the impact on this part of the computation was a bit unexpected to us. A bit more care/thought will be needed here. I've described my plan in the comments though. In any case, I believe we have the current implementation is a conservative approximation that shouldn't result in unsoundness but false positives at worst. So it feels fine for now. r? ``@jackh726`` --- Fixes rust-lang#127628 -- which was a assertion triggered for a difference in loan computation between NLLs and the location-insensitive analysis. That doesn't exist anymore so I've removed this crash test.

Encode constraints that hold at all points as logical edges in location-sensitive polonius Currently, with the full setup in rust-lang#134980 (but is from rust-lang#134268), the polonius location-sensitive analysis converts `Locations::All` typeck constraints as edges at all points in the CFG. This was temporary. There's a FIXME about that already, and this PR implements it: we now use the constraints that hold at all points during traversal instead of eagerly materializing them as physical edges. Another easy one `@jackh726.` This fixes the slowness that was happening on the big CFG from the `saturating-float-casts` test (because of its 12M materialized edges) without, AFAICT, simply moving this overhead to traversal: materializing the logical edges is done on-demand. r? `@jackh726` (no rush either)

…kh726 Encode constraints that hold at all points as logical edges in location-sensitive polonius Currently, with the full setup in rust-lang#134980 (but is from rust-lang#134268), the polonius location-sensitive analysis converts `Locations::All` typeck constraints as edges at all points in the CFG. This was temporary. There's a FIXME about that already, and this PR implements it: we now use the constraints that hold at all points during traversal instead of eagerly materializing them as physical edges. Another easy one `@jackh726.` This fixes the slowness that was happening on the big CFG from the `saturating-float-casts` test (because of its 12M materialized edges) without, AFAICT, simply moving this overhead to traversal: materializing the logical edges is done on-demand. r? `@jackh726` (no rush either)

Rollup merge of rust-lang#135290 - lqd:polonius-next-episode-8, r=jackh726 Encode constraints that hold at all points as logical edges in location-sensitive polonius Currently, with the full setup in rust-lang#134980 (but is from rust-lang#134268), the polonius location-sensitive analysis converts `Locations::All` typeck constraints as edges at all points in the CFG. This was temporary. There's a FIXME about that already, and this PR implements it: we now use the constraints that hold at all points during traversal instead of eagerly materializing them as physical edges. Another easy one `@jackh726.` This fixes the slowness that was happening on the big CFG from the `saturating-float-casts` test (because of its 12M materialized edges) without, AFAICT, simply moving this overhead to traversal: materializing the logical edges is done on-demand. r? `@jackh726` (no rush either)

rustbot assigned jackh726 Dec 31, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 31, 2024

lqd force-pushed the polonius-next-episode-7 branch from cdc0452 to 03b6f70 Compare December 31, 2024 18:28

lqd mentioned this pull request Dec 31, 2024

Scalable Polonius support on nightly rust-lang/rust-project-goals#118

Open

18 tasks

jackh726 reviewed Jan 1, 2025

View reviewed changes

compiler/rustc_borrowck/src/polonius/mod.rs Outdated Show resolved Hide resolved

lqd force-pushed the polonius-next-episode-7 branch from 03b6f70 to 8ced309 Compare January 1, 2025 10:12

lqd force-pushed the polonius-next-episode-7 branch from 8ced309 to 1e20d93 Compare January 8, 2025 10:13

lqd marked this pull request as ready for review January 8, 2025 10:15

lqd force-pushed the polonius-next-episode-7 branch from 1e20d93 to d8b1fb6 Compare January 9, 2025 14:55

lqd mentioned this pull request Jan 9, 2025

Encode constraints that hold at all points as logical edges in location-sensitive polonius #135290

Merged

jackh726 reviewed Jan 9, 2025

View reviewed changes

lqd force-pushed the polonius-next-episode-7 branch from d8b1fb6 to 2532943 Compare January 10, 2025 08:53

This comment was marked as resolved.

Sign in to view

lqd added 7 commits January 12, 2025 07:29

disable NLL liveness optimization when using polonius

5055864

in NLLs some locals are marked live at all points if one of their regions escapes the function but that doesn't work in a flow-sensitive setting like polonius

introduce reachability to the constraint graph

0c978bc

handle kills in reachability

550cf1f

deal with naive reachability weakness

0f3dd33

it's a bit mind-bending

remove location-insensitive ICE test

735013d

move out of scope precomputer code

8ac045d

this addresses review comments while: - keeping the symmetry between the NLL and Polonius out of scope precomputers - keeping the unstable `calculate_borrows_out_of_scope_at_location` function to avoid churn for consumers

lqd force-pushed the polonius-next-episode-7 branch from 2532943 to 8ac045d Compare January 12, 2025 07:45

apiraino assigned jackh726 and unassigned jackh726 Jan 16, 2025

jackh726 reviewed Jan 17, 2025

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2025

matthiaskrgr mentioned this pull request Jan 17, 2025

Rollup of 7 pull requests #135613

Closed

matthiaskrgr mentioned this pull request Jan 17, 2025

Rollup of 6 pull requests #135615

Merged

bors merged commit dbdfa79 into rust-lang:master Jan 17, 2025
6 checks passed

rustbot added this to the 1.86.0 milestone Jan 17, 2025

lqd deleted the polonius-next-episode-7 branch January 17, 2025 11:47

matthiaskrgr mentioned this pull request Jan 17, 2025

ICE: polonius: None in region infer with unconditional panic #135646

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Location-sensitive polonius prototype: endgame #134980

Location-sensitive polonius prototype: endgame #134980

lqd commented Dec 31, 2024 •

edited

Loading

lqd commented Dec 31, 2024

jackh726 left a comment

lqd commented Jan 8, 2025

lqd commented Jan 9, 2025

jackh726 left a comment

jackh726 Jan 9, 2025

lqd Jan 9, 2025 •

edited

Loading

jackh726 Jan 10, 2025

lqd Jan 17, 2025

This comment was marked as resolved.

lqd commented Jan 12, 2025

jackh726 Jan 10, 2025

jackh726 commented Jan 17, 2025

bors commented Jan 17, 2025

Location-sensitive polonius prototype: endgame #134980

Location-sensitive polonius prototype: endgame #134980

Conversation

lqd commented Dec 31, 2024 • edited Loading

lqd commented Dec 31, 2024

jackh726 left a comment

Choose a reason for hiding this comment

lqd commented Jan 8, 2025

lqd commented Jan 9, 2025

jackh726 left a comment

Choose a reason for hiding this comment

jackh726 Jan 9, 2025

Choose a reason for hiding this comment

lqd Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

jackh726 Jan 10, 2025

Choose a reason for hiding this comment

lqd Jan 17, 2025

Choose a reason for hiding this comment

This comment was marked as resolved.

lqd commented Jan 12, 2025

jackh726 Jan 10, 2025

Choose a reason for hiding this comment

jackh726 commented Jan 17, 2025

bors commented Jan 17, 2025

lqd commented Dec 31, 2024 •

edited

Loading

lqd Jan 9, 2025 •

edited

Loading