Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cleanup region handling: add LateParamRegionKind #133961

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

cleanup region handling: add LateParamRegionKind #133961

wants to merge 2 commits into from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Dec 6, 2024 •
edited
Loading

The second commit is to enable a split between BoundRegionKind and LateParamRegionKind, by avoiding BoundRegionKind where it isn't necessary.

The third comment then adds LateParamRegionKind to avoid having the same late-param region for separate bound regions. This fixes #124021.

r? @compiler-errors

@rustbot 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 6, 2024
@lcnr lcnr force-pushed the borrowck-cleanup branch from e5e02ac to 6f5bf6c Compare December 6, 2024 13:26
@rustbot
Copy link
Collaborator

rustbot commented Dec 6, 2024

HIR ty lowering was modified

cc @fmease

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@lcnr
Copy link
Contributor Author

lcnr commented Dec 6, 2024

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 6, 2024
@bors
Copy link
Contributor

bors commented Dec 6, 2024

⌛ Trying commit ad6c287 with merge 82684f3...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 6, 2024
@bors
Auto merge of rust-lang#133961 - lcnr:borrowck-cleanup, r=<try>
82684f3 
some cleanup to  our region handling

The second commit is to enable a split between `BoundRegionKind` and `LateParamRegionKind`, by avoiding `BoundRegionKind` where it isn't necessary.

The third comment then adds `LateParamRegionKind` to avoid having the same late-param region for separate bound regions.

r? `@compiler-errors`
@lcnr lcnr changed the title some cleanup to our region handling cleanup region handling: add LateParamRegionKind Dec 6, 2024
@lcnr lcnr force-pushed the borrowck-cleanup branch from a52c918 to 4086a6b Compare December 6, 2024 14:03
@lcnr
Copy link
Contributor Author

lcnr commented Dec 6, 2024

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Dec 6, 2024

⌛ Trying commit 4086a6b with merge 10ac87b...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 6, 2024
@bors
Auto merge of rust-lang#133961 - lcnr:borrowck-cleanup, r=<try>
10ac87b 
cleanup region handling: add `LateParamRegionKind`

The second commit is to enable a split between `BoundRegionKind` and `LateParamRegionKind`, by avoiding `BoundRegionKind` where it isn't necessary.

The third comment then adds `LateParamRegionKind` to avoid having the same late-param region for separate bound regions. This fixes rust-lang#124021.

r? `@compiler-errors`
jackh726
jackh726 reviewed Dec 6, 2024
View reviewed changes
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just very quickly skimmed over the diff....

Why not just add a u32 to BoundRegionKind::Anon? When would you use ReLateParam vs ReBound?

compiler/rustc_trait_selection/src/error_reporting/infer/nice_region_error/util.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Dec 6, 2024

☀️ Try build successful - checks-actions
Build commit: 10ac87b (10ac87bad11840cd0a8fd34a3bb1d40a1f98ca11)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (10ac87b): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.2% [-0.2%, -0.2%] 2
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 769.713s -> 766.853s (-0.37%)
Artifact size: 330.91 MiB -> 330.88 MiB (-0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 6, 2024
@lcnr
Copy link
Contributor Author

lcnr commented Dec 9, 2024

Just very quickly skimmed over the diff....

Why not just add a u32 to BoundRegionKind::Anon? When would you use ReLateParam vs ReBound?

because we've previously removed that index in #110036 🤔 unsure whether simply adding it back causes any issues. If not, that's probably easier and i'll redo the last commit here

@jackh726
Copy link
Member

Yes I remember this - at the time, this index was only used for diagnostics, so this PR represents a shift in that. But seemingly we're in a place that we need that. I'm not sure if the def_id for Named has always been "load-bearing" or used to also only be for diagnostics.

Personally, I think that the "real" solution is probably to use the info in the binders list if we need to track info (but likely it's more complicated than that, and either way is a larger change than what you probably want to make here).

@jackh726
Copy link
Member

FWIW, the first two commits stand well enough alone for me, so if you want to split those and land them, you can r=me on that

@lcnr lcnr mentioned this pull request Dec 17, 2024
Merged
@lcnr
Copy link
Contributor Author

lcnr commented Dec 17, 2024

I've looked into changing BoundRegionKind and it feels significantly worse to me, having soundness-critical1 - unchecked - duplicate information in the type system feels quite bad, especially considering just how frequently we create new bound regions.

wip commit: https://github.com/lcnr/rust/pull/new/br-anon-index

@rustbot ready on the last 2 commits

Footnotes

  1. In theory as we may instantiate different bound vars to the same late-param. Don't think this is exploitable in practice ↩

@lcnr
Copy link
Contributor Author

lcnr commented Dec 17, 2024

r? @jackh726

@rustbot rustbot assigned jackh726 and unassigned compiler-errors Dec 17, 2024
jhpratt added a commit to jhpratt/rust that referenced this pull request Dec 17, 2024
@jhpratt
Rollup merge of rust-lang#134412 - lcnr:borrowck-cleanup-trivial, r=j…
cdd71c9 
…ackh726

small borrowck cleanup

the already approved parts of rust-lang#133909 and rust-lang#133961

r? `@jackh726`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 17, 2024
@rust-timer
Unrolled build for rust-lang#134412
f828977 
Rollup merge of rust-lang#134412 - lcnr:borrowck-cleanup-trivial, r=jackh726

small borrowck cleanup

the already approved parts of rust-lang#133909 and rust-lang#133961

r? `@jackh726`
@bors
Copy link
Contributor

bors commented Dec 17, 2024

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

@jackh726
Copy link
Member

Hmm, I think maybe I need to think about it a bit more...

Your wip commit looks mostly fine to me. Which makes me kind of go back and look at this PR: how exactly do these changes fix #124021? Presumably we compare kinds somewhere; I thought I saw that the last time I looked at this, but idk if it's just because I'm short on time looking at this today or I was imagining things last time but I don't see it.

I guess, I can only speculate without looking directly at the code where the bug is, but I imagine we either have just lost the var info, or just decided to not use it. Either way, that's the imo the right way to compare regions. (Though, we probably aren't particularly consistent about doing that in most places)

@lcnr
Copy link
Contributor Author

lcnr commented Dec 18, 2024 •
edited
Loading

The bug is that https://doc.rust-lang.org/nightly/nightly-rustc/rustc_borrowck/universal_regions/struct.UniversalRegionIndices.html#structfield.indices maps from RegionKind::LateParam to their unique nll vars. Right now two anon bound regions get mapped to the same late param, so the second bound region overwrites the entry for the first one.

We then use this map to instantiate definitions[nll-var].external_name. This means we don't add an external name for the first anon late-param. Trying to access that one then ICE's.

Imo the bug here is clearly the fact that we map two anonymous bound regions to the same RegionKInd::LateParam when liberating late-bound regions.

@jackh726
Copy link
Member

Ah yes, I see it and understand now.

r=me after rebase

@lcnr
Copy link
Contributor Author

lcnr commented Dec 18, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Dec 18, 2024

📌 Commit 4d5aaa0 has been approved by lcnr

It is now in the queue for this repository.

@bors 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 Dec 18, 2024
@jackh726
Copy link
Member

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Dec 18, 2024

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Dec 18, 2024

📌 Commit 4d5aaa0 has been approved by jackh726

It is now in the queue for this repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackh726 jackh726 jackh726 left review comments

Assignees

@jackh726 jackh726

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ICE: None in opaque types region infer
6 participants
@lcnr @rustbot @rust-timer @bors @jackh726 @compiler-errors