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.

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

Remove DefId from EarlyParamRegion #125468

Merged
merged 6 commits into from
May 27, 2024

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented May 23, 2024

Currently we represent usages of Region parameters via the ReEarlyParam or ReLateParam variants. The ReEarlyParam is effectively equivalent to TyKind::Param and ConstKind::Param (i.e. it stores a Symbol and a u32 index) however it also stores a DefId for the definition of the lifetime parameter.

This was used in roughly two places:

  • Borrowck diagnostics instead of threading the appropriate body_id down to relevant locations. Interestingly there were already some places that had to pass down a DefId manually.
  • Some opaque type checking logic was using the DefId field to track captured lifetimes

I've split this PR up into a commit for generate rote changes to diagnostics code to pass around a DefId manually everywhere, and another commit for the opaque type related changes which likely require more careful review as they might change the semantics of lints/errors.

Instead of manually passing the DefId around everywhere I previously tried to bundle it in with TypeErrCtxt but ran into issues with some call sites of infcx.err_ctxt being unable to provide a DefId, particularly places involved with trait solving and normalization. It might be worth investigating adding some new wrapper type to pass this around everywhere but I think this might be acceptable for now.

This pr also has the effect of reducing the size of EarlyParamRegion from 16 bytes -> 8 bytes. I wouldn't expect this to have any direct performance improvement however, other variants of RegionKind over 8 bytes are all because they contain a BoundRegionKind which is, as far as I know, mostly there for diagnostics. If we're ever able to remove this it would shrink the RegionKind type from 24 bytes to 12 (and with clever bit packing we might be able to get it to 8 bytes). I am curious what the performance impact would be of removing interning of Region's if we ever manage to shrink RegionKind that much.

Sidenote: by removing the DefId the Debug output for Region has gotten significantly nicer. As an example see this opaque type debug print before vs after this PR:
Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), [DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0, T, DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0])
Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), ['a/#0, T, 'a/#0])

r? @compiler-errors (I would like someone who understands the opaque type setup to atleast review the type system commit, but the rest is likely reviewable by anyone)

@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 May 23, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 23, 2024

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

HIR ty lowering was modified

cc @fmease

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@BoxyUwU BoxyUwU changed the title Remove DefId from EarlyRegionParam Remove DefId from EarlyParamRegion May 24, 2024
@BoxyUwU BoxyUwU force-pushed the remove_defid_from_regionparam branch from eafea35 to 5451787 Compare May 24, 2024 00:13
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Generally very happy with this change 👍 ✨

compiler/rustc_middle/src/ty/context.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/impl_trait_overcaptures.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/note.rs Outdated Show resolved Hide resolved
@compiler-errors
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 24, 2024
@bors
Copy link
Contributor

bors commented May 24, 2024

⌛ Trying commit 5451787 with merge 85c5dfc...

bors added a commit to rust-lang-ci/rust that referenced this pull request May 24, 2024
…m, r=<try>

Remove `DefId` from `EarlyParamRegion`

Currently we represent usages of `Region` parameters via the `ReEarlyParam` or `ReLateParam` variants. The `ReEarlyParam` is effectively equivalent to `TyKind::Param` and `ConstKind::Param` (i.e. it stores a `Symbol` and a `u32` index) however it also stores a `DefId` for the definition of the lifetime parameter.

This was used in roughly two places:
- Borrowck diagnostics instead of threading the appropriate `body_id` down to relevant locations. Interestingly there were already some places that had to pass down a `DefId` manually.
- Some opaque type checking logic was using the `DefId` field to track captured lifetimes

I've split this PR up into a commit for generate rote changes to diagnostics code to pass around a `DefId` manually everywhere, and another commit for the opaque type related changes which likely require more careful review as they might change the semantics of lints/errors.

Instead of manually passing the `DefId` around everywhere I previously tried to bundle it in with `TypeErrCtxt` but ran into issues with some call sites of `infcx.err_ctxt` being unable to provide a `DefId`, particularly places involved with trait solving and normalization. It might be worth investigating adding some new wrapper type to pass this around everywhere but I think this might be acceptable for now.

This pr also has the effect of reducing the size of `EarlyParamRegion` from 16 bytes -> 8 bytes. I wouldn't expect this to have any direct performance improvement however, other variants of `RegionKind` over `8` bytes are all because they contain a `BoundRegionKind` which is, as far as I know, mostly there for diagnostics. If we're ever able to remove this it would shrink the `RegionKind` type from `24` bytes to `12` (and with clever bit packing we might be able to get it to `8` bytes). I am curious what the performance impact would be of removing interning of `Region`'s if we ever manage to shrink `RegionKind` that much.

Sidenote: by removing the `DefId` the `Debug` output for `Region` has gotten significantly nicer. As an example see this opaque type debug print before vs after this PR:
`Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), [DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0, T, DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0])`
`Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), ['a/#0, T, 'a/#0])`

r? `@compiler-errors` (I would like someone who understands the opaque type setup to atleast review the type system commit, but the rest is likely reviewable by anyone)
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

🤔 also would like to know why those tests no longer ICE. I might be able to investigate tomorrow if you end up not being able to find out why, just lemme kno

@BoxyUwU
Copy link
Member Author

BoxyUwU commented May 24, 2024

Oh god why does this make tests no longer ICE 😭

@BoxyUwU BoxyUwU force-pushed the remove_defid_from_regionparam branch from 5451787 to 0946cb8 Compare May 24, 2024 00:58
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 24, 2024

⌛ Trying commit 0946cb8 with merge 8210bd9...

bors added a commit to rust-lang-ci/rust that referenced this pull request May 24, 2024
…m, r=<try>

Remove `DefId` from `EarlyParamRegion`

Currently we represent usages of `Region` parameters via the `ReEarlyParam` or `ReLateParam` variants. The `ReEarlyParam` is effectively equivalent to `TyKind::Param` and `ConstKind::Param` (i.e. it stores a `Symbol` and a `u32` index) however it also stores a `DefId` for the definition of the lifetime parameter.

This was used in roughly two places:
- Borrowck diagnostics instead of threading the appropriate `body_id` down to relevant locations. Interestingly there were already some places that had to pass down a `DefId` manually.
- Some opaque type checking logic was using the `DefId` field to track captured lifetimes

I've split this PR up into a commit for generate rote changes to diagnostics code to pass around a `DefId` manually everywhere, and another commit for the opaque type related changes which likely require more careful review as they might change the semantics of lints/errors.

Instead of manually passing the `DefId` around everywhere I previously tried to bundle it in with `TypeErrCtxt` but ran into issues with some call sites of `infcx.err_ctxt` being unable to provide a `DefId`, particularly places involved with trait solving and normalization. It might be worth investigating adding some new wrapper type to pass this around everywhere but I think this might be acceptable for now.

This pr also has the effect of reducing the size of `EarlyParamRegion` from 16 bytes -> 8 bytes. I wouldn't expect this to have any direct performance improvement however, other variants of `RegionKind` over `8` bytes are all because they contain a `BoundRegionKind` which is, as far as I know, mostly there for diagnostics. If we're ever able to remove this it would shrink the `RegionKind` type from `24` bytes to `12` (and with clever bit packing we might be able to get it to `8` bytes). I am curious what the performance impact would be of removing interning of `Region`'s if we ever manage to shrink `RegionKind` that much.

Sidenote: by removing the `DefId` the `Debug` output for `Region` has gotten significantly nicer. As an example see this opaque type debug print before vs after this PR:
`Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), [DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0, T, DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0])`
`Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), ['a/#0, T, 'a/#0])`

r? `@compiler-errors` (I would like someone who understands the opaque type setup to atleast review the type system commit, but the rest is likely reviewable by anyone)
@bors
Copy link
Contributor

bors commented May 24, 2024

☀️ Try build successful - checks-actions
Build commit: 8210bd9 (8210bd94475d1cbf00c0dd7bfe0d48e98259bca1)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8210bd9): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.1% [4.1%, 4.1%] 1
Improvements ✅
(primary)
-0.4% [-0.9%, -0.2%] 43
Improvements ✅
(secondary)
-0.8% [-1.3%, -0.3%] 24
All ❌✅ (primary) -0.4% [-0.9%, -0.2%] 43

Max RSS (memory usage)

Results (secondary -3.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

Results (primary -1.4%, secondary 0.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
8.0% [7.1%, 8.9%] 2
Improvements ✅
(primary)
-1.4% [-2.2%, -0.9%] 8
Improvements ✅
(secondary)
-2.1% [-2.1%, -2.0%] 5
All ❌✅ (primary) -1.4% [-2.2%, -0.9%] 8

Binary size

Results (primary -0.3%, secondary -0.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.3% [-1.3%, -0.0%] 88
Improvements ✅
(secondary)
-0.9% [-1.6%, -0.0%] 12
All ❌✅ (primary) -0.3% [-1.3%, -0.0%] 88

Bootstrap: 671.784s -> 671.214s (-0.08%)
Artifact size: 315.72 MiB -> 315.68 MiB (-0.02%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 24, 2024
@compiler-errors
Copy link
Member

TODO: I owe Boxy a detailed explanation for the "regression" (tests that ICE -> pass) which I'll have by the end of my day here hopefully. Then we can land this 🎉

@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 25, 2024
@compiler-errors
Copy link
Member

thinking emoji

@bors retry

@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 May 25, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 25, 2024
…m, r=compiler-errors

Remove `DefId` from `EarlyParamRegion`

Currently we represent usages of `Region` parameters via the `ReEarlyParam` or `ReLateParam` variants. The `ReEarlyParam` is effectively equivalent to `TyKind::Param` and `ConstKind::Param` (i.e. it stores a `Symbol` and a `u32` index) however it also stores a `DefId` for the definition of the lifetime parameter.

This was used in roughly two places:
- Borrowck diagnostics instead of threading the appropriate `body_id` down to relevant locations. Interestingly there were already some places that had to pass down a `DefId` manually.
- Some opaque type checking logic was using the `DefId` field to track captured lifetimes

I've split this PR up into a commit for generate rote changes to diagnostics code to pass around a `DefId` manually everywhere, and another commit for the opaque type related changes which likely require more careful review as they might change the semantics of lints/errors.

Instead of manually passing the `DefId` around everywhere I previously tried to bundle it in with `TypeErrCtxt` but ran into issues with some call sites of `infcx.err_ctxt` being unable to provide a `DefId`, particularly places involved with trait solving and normalization. It might be worth investigating adding some new wrapper type to pass this around everywhere but I think this might be acceptable for now.

This pr also has the effect of reducing the size of `EarlyParamRegion` from 16 bytes -> 8 bytes. I wouldn't expect this to have any direct performance improvement however, other variants of `RegionKind` over `8` bytes are all because they contain a `BoundRegionKind` which is, as far as I know, mostly there for diagnostics. If we're ever able to remove this it would shrink the `RegionKind` type from `24` bytes to `12` (and with clever bit packing we might be able to get it to `8` bytes). I am curious what the performance impact would be of removing interning of `Region`'s if we ever manage to shrink `RegionKind` that much.

Sidenote: by removing the `DefId` the `Debug` output for `Region` has gotten significantly nicer. As an example see this opaque type debug print before vs after this PR:
`Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), [DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0, T, DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0])`
`Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), ['a/#0, T, 'a/#0])`

r? `@compiler-errors` (I would like someone who understands the opaque type setup to atleast review the type system commit, but the rest is likely reviewable by anyone)
@bors
Copy link
Contributor

bors commented May 25, 2024

⌛ Testing commit ed8e436 with merge c154f01...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
error: failed retrieving file 'mingw-w64-x86_64-gdb-14.1-1-any.pkg.tar.zst' from mirror.msys2.org : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
error: failed retrieving file 'mingw-w64-x86_64-crt-git-11.0.0.r547.g4c8123efb-1-any.pkg.tar.zst' from mirror.msys2.org : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
error: failed retrieving file 'mingw-w64-x86_64-libgccjit-13.2.0-3-any.pkg.tar.zst' from mirror.msys2.org : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
warning: too many errors from mirror.msys2.org, skipping for the remainder of this transaction
error: failed retrieving file 'mingw-w64-x86_64-python-3.11.7-1-any.pkg.tar.zst.sig' from mirror.umd.edu : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
error: failed to commit transaction (unexpected error)
 mingw-w64-x86_64-gcc-13.2.0-3-any downloading...
 mingw-w64-x86_64-python-3.11.7-1-any downloading...
 mingw-w64-x86_64-gcc-ada-13.2.0-3-any downloading...

@bors
Copy link
Contributor

bors commented May 25, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 25, 2024
@BoxyUwU
Copy link
Member Author

BoxyUwU commented May 25, 2024

CI is broken, see this zulip thread. I guess we should just wait a little bit...

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 25, 2024
@compiler-errors
Copy link
Member

@bors r

@bors
Copy link
Contributor

bors commented May 26, 2024

📌 Commit ed8e436 has been approved by compiler-errors

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 26, 2024

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 26, 2024
@BoxyUwU
Copy link
Member Author

BoxyUwU commented May 26, 2024

5th times the charm lol :")

@bors
Copy link
Contributor

bors commented May 27, 2024

⌛ Testing commit ed8e436 with merge fec98b3...

@bors
Copy link
Contributor

bors commented May 27, 2024

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing fec98b3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 27, 2024
@bors bors merged commit fec98b3 into rust-lang:master May 27, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 27, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fec98b3): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.4% [-0.9%, -0.2%] 48
Improvements ✅
(secondary)
-0.8% [-2.0%, -0.3%] 19
All ❌✅ (primary) -0.4% [-0.9%, -0.2%] 48

Max RSS (memory usage)

Results (primary -0.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.1% [1.1%, 1.1%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.3% [-2.1%, -0.8%] 5
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.9% [-2.1%, 1.1%] 6

Cycles

Results (primary -1.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.4% [-1.6%, -1.3%] 5
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.4% [-1.6%, -1.3%] 5

Binary size

Results (primary -0.3%, secondary -0.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.3% [-1.3%, -0.0%] 80
Improvements ✅
(secondary)
-0.9% [-1.6%, -0.0%] 12
All ❌✅ (primary) -0.3% [-1.3%, -0.0%] 80

Bootstrap: 669.538s -> 668.124s (-0.21%)
Artifact size: 315.56 MiB -> 315.55 MiB (-0.00%)

@rustbot rustbot removed the perf-regression Performance regression. label May 27, 2024
oli-obk pushed a commit to oli-obk/rust that referenced this pull request May 27, 2024
…m, r=compiler-errors

Remove `DefId` from `EarlyParamRegion`

Currently we represent usages of `Region` parameters via the `ReEarlyParam` or `ReLateParam` variants. The `ReEarlyParam` is effectively equivalent to `TyKind::Param` and `ConstKind::Param` (i.e. it stores a `Symbol` and a `u32` index) however it also stores a `DefId` for the definition of the lifetime parameter.

This was used in roughly two places:
- Borrowck diagnostics instead of threading the appropriate `body_id` down to relevant locations. Interestingly there were already some places that had to pass down a `DefId` manually.
- Some opaque type checking logic was using the `DefId` field to track captured lifetimes

I've split this PR up into a commit for generate rote changes to diagnostics code to pass around a `DefId` manually everywhere, and another commit for the opaque type related changes which likely require more careful review as they might change the semantics of lints/errors.

Instead of manually passing the `DefId` around everywhere I previously tried to bundle it in with `TypeErrCtxt` but ran into issues with some call sites of `infcx.err_ctxt` being unable to provide a `DefId`, particularly places involved with trait solving and normalization. It might be worth investigating adding some new wrapper type to pass this around everywhere but I think this might be acceptable for now.

This pr also has the effect of reducing the size of `EarlyParamRegion` from 16 bytes -> 8 bytes. I wouldn't expect this to have any direct performance improvement however, other variants of `RegionKind` over `8` bytes are all because they contain a `BoundRegionKind` which is, as far as I know, mostly there for diagnostics. If we're ever able to remove this it would shrink the `RegionKind` type from `24` bytes to `12` (and with clever bit packing we might be able to get it to `8` bytes). I am curious what the performance impact would be of removing interning of `Region`'s if we ever manage to shrink `RegionKind` that much.

Sidenote: by removing the `DefId` the `Debug` output for `Region` has gotten significantly nicer. As an example see this opaque type debug print before vs after this PR:
`Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), [DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0, T, DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0])`
`Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), ['a/#0, T, 'a/#0])`

r? `@compiler-errors` (I would like someone who understands the opaque type setup to atleast review the type system commit, but the rest is likely reviewable by anyone)
bors added a commit to rust-lang-ci/rust that referenced this pull request May 30, 2024
…m, r=compiler-errors

Remove `DefId` from `EarlyParamRegion`

Currently we represent usages of `Region` parameters via the `ReEarlyParam` or `ReLateParam` variants. The `ReEarlyParam` is effectively equivalent to `TyKind::Param` and `ConstKind::Param` (i.e. it stores a `Symbol` and a `u32` index) however it also stores a `DefId` for the definition of the lifetime parameter.

This was used in roughly two places:
- Borrowck diagnostics instead of threading the appropriate `body_id` down to relevant locations. Interestingly there were already some places that had to pass down a `DefId` manually.
- Some opaque type checking logic was using the `DefId` field to track captured lifetimes

I've split this PR up into a commit for generate rote changes to diagnostics code to pass around a `DefId` manually everywhere, and another commit for the opaque type related changes which likely require more careful review as they might change the semantics of lints/errors.

Instead of manually passing the `DefId` around everywhere I previously tried to bundle it in with `TypeErrCtxt` but ran into issues with some call sites of `infcx.err_ctxt` being unable to provide a `DefId`, particularly places involved with trait solving and normalization. It might be worth investigating adding some new wrapper type to pass this around everywhere but I think this might be acceptable for now.

This pr also has the effect of reducing the size of `EarlyParamRegion` from 16 bytes -> 8 bytes. I wouldn't expect this to have any direct performance improvement however, other variants of `RegionKind` over `8` bytes are all because they contain a `BoundRegionKind` which is, as far as I know, mostly there for diagnostics. If we're ever able to remove this it would shrink the `RegionKind` type from `24` bytes to `12` (and with clever bit packing we might be able to get it to `8` bytes). I am curious what the performance impact would be of removing interning of `Region`'s if we ever manage to shrink `RegionKind` that much.

Sidenote: by removing the `DefId` the `Debug` output for `Region` has gotten significantly nicer. As an example see this opaque type debug print before vs after this PR:
`Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), [DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0, T, DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0])`
`Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), ['a/#0, T, 'a/#0])`

r? `@compiler-errors` (I would like someone who understands the opaque type setup to atleast review the type system commit, but the rest is likely reviewable by anyone)
jaisnan pushed a commit to jaisnan/rust-dev that referenced this pull request Jul 29, 2024
Update Rust toolchain from nightly-2024-05-27 to nightly-2024-05-28
without any other source changes.
This is an automatically generated pull request. If any of the CI checks
fail, manual intervention is required. In such a case, review the
changes at https://github.com/rust-lang/rust from
rust-lang@bdbbb6c
up to
rust-lang@84b40fc.
The log for this commit range is:
rust-lang@84b40fc908 Auto merge of
rust-lang#125628 - matthiaskrgr:rollup-3zk9v3w, r=matthiaskrgr
rust-lang@4966e1ae35 Rollup merge of
rust-lang#125625 - ChrisDenton:line-endings, r=Mark-Simulacrum
rust-lang@61f9d35798 Rollup merge of
rust-lang#125616 - RalfJung:mir-validate-downcast-projection, r=compiler-errors
rust-lang@e8dd585dd8 Rollup merge of
rust-lang#125542 - GuillaumeGomez:migrate-rustdoc-verify-output-files, r=jieyouxu
rust-lang@8bd15878eb Rollup merge of
rust-lang#125339 - tbu-:pr_tidy_ui_tests_u32, r=clubby789
rust-lang@f00b02e6bb Auto merge of
rust-lang#125599 - camelid:clarify-stability, r=notriddle,GuillaumeGomez
rust-lang@7a847fc4fb Use grep to
implement verify-line-endings
rust-lang@b0f8618938 Auto merge of
rust-lang#125413 - lcnr:ambig-drop-region-constraints, r=compiler-errors
rust-lang@7d24f87068 MIR validation:
ensure that downcast projection is followed by field projection
rust-lang@f6e4703e91 Auto merge of
rust-lang#125611 - GuillaumeGomez:rollup-dfavpgg, r=GuillaumeGomez
rust-lang@bdf3864d51 Migrate
`run-make/rustdoc-verify-output-files` to `rmake.rs`
rust-lang@f0ab814aec Add
`Rustdoc::output_format`
rust-lang@1551fd1202 Add file path in
case it cannot be read in `Diff::actual_file`
rust-lang@90fec5a087 Add `copy_dir_all`
and `recursive_diff` functions to `run-make-support`
rust-lang@7083131c92 Rollup merge of
rust-lang#125607 - GuillaumeGomez:migrate-compile-stdin, r=jieyouxu
rust-lang@a9c125f864 Rollup merge of
rust-lang#125597 - compiler-errors:early-binder, r=jackh726
rust-lang@cfa7ab474f Rollup merge of
rust-lang#125535 - onur-ozkan:remove-deprecated-field, r=clubby789
rust-lang@f50b4f5034 Rollup merge of
rust-lang#125530 - SparrowLii:expand2, r=petrochenkov
rust-lang@ad37f40355 Rollup merge of
rust-lang#125522 - spastorino:fix-lint-docs-edition-handling,
r=Urgau,michaelwoerister
rust-lang@86f2fa35a2 Rollup merge of
rust-lang#125148 - RalfJung:codegen-sh, r=scottmcm
rust-lang@6dddc888fc Rollup merge of
rust-lang#124870 - Lokathor:update-result-docs, r=dtolnay
rust-lang@a59072ec4f Auto merge of
rust-lang#125602 - RalfJung:interpret-mir-lifetime, r=oli-obk
rust-lang@e4abfaeb62 Migrate
`run-make/compile-stdin` to `rmake.rs`
rust-lang@b582f807fa Auto merge of
rust-lang#125410 - fmease:adj-lint-diag-api, r=nnethercote
rust-lang@fec98b3bbc Auto merge of
rust-lang#125468 - BoxyUwU:remove_defid_from_regionparam, r=compiler-errors
rust-lang@e8379c9598 interpret: get rid
of 'mir lifetime everywhere
rust-lang@36d36a3e1f interpret: the MIR
is actually at lifetime 'tcx
rust-lang@699d28f968 rustdoc: Show
"const" for const-unstable if also overall unstable
rust-lang@cdc509f7c0 Auto merge of
rust-lang#125580 - RalfJung:miri-sync, r=RalfJung
rust-lang@f92292978f Use EarlyBinder in
rustc_type_ir, simplify imports
rust-lang@993553ceb8 Uplift EarlyBinder
rust-lang@529bb2573a Auto merge of
rust-lang#125593 - workingjubilee:rollup-67qk7di, r=workingjubilee
rust-lang@bbcdb4fd3e Give EarlyBinder a
tcx parameter
rust-lang@4ff78692db Rollup merge of
rust-lang#125582 - scottmcm:less-from-usize, r=jieyouxu
rust-lang@45507e4304 Rollup merge of
rust-lang#125566 - camelid:notify-accepted, r=GuillaumeGomez
rust-lang@25b079a1cf Rollup merge of
rust-lang#125559 - scottmcm:simplify-shift-ubcheck, r=workingjubilee
rust-lang@c51fc1d02b Rollup merge of
rust-lang#125544 - Urgau:check-cfg-mention-cargo-specific, r=jieyouxu
rust-lang@b65b2b6ced Rollup merge of
rust-lang#125469 - compiler-errors:dont-skip-inner-const-body, r=cjgillot
rust-lang@09e75921f3 Rollup merge of
rust-lang#125466 - compiler-errors:dont-probe-for-ambig-in-sugg, r=jieyouxu
rust-lang@5860d43af3 Rollup merge of
rust-lang#125046 - bjorn3:no_mutable_static_linkage, r=cjgillot
rust-lang@866630d004 Rollup merge of
rust-lang#124048 - veera-sivarajan:bugfix-123773-c23-variadics, r=compiler-errors
rust-lang@0aad3f64e2 Auto merge of
rust-lang#125576 - lnicola:sync-from-ra, r=lnicola
rust-lang@d37f456b2a Avoid a
`FieldIdx::from_usize` in InstSimplify
rust-lang@0963353634 Auto merge of rust-lang#3631
- RalfJung:blocking-refactor, r=RalfJung
rust-lang@2e89443b93 add a macro to
declare thread unblock callbacks
rust-lang@8e861c6c4c Auto merge of rust-lang#3632
- RalfJung:readdir, r=RalfJung
rust-lang@350f5c88db unix/fs: a bit of
cleanup in macos_fbsd_readdir_r
rust-lang@e09bf5694b Auto merge of rust-lang#3633
- RalfJung:target, r=RalfJung
rust-lang@cbec1288a2 fix './miri run
--dep --target _'
rust-lang@e6bb468b53 data_race: vector
indices can be reused immediately when the thread is gone
rust-lang@a131243557 completely refactor
how we manage blocking and unblocking threads
rust-lang@f7ca8a6d66 Auto merge of rust-lang#17296
- mathew-horner:no-clone-target, r=Veykril
rust-lang@bd9cc02d10 Auto merge of rust-lang#17295
- 0xJonas:fix_passing_env_vars_to_cpptools, r=Veykril
rust-lang@5fa30f7eaa make release_clock
always work on the current thread
rust-lang@fa7a3f9049 rustdoc: Elide
const-unstable if also unstable overall
rust-lang@91b3ef5b4a Notify T-rustdoc for
beta-accepted and stable-accepted too
rust-lang@9b480da367 It seems that anchor
names are implicitly all lowercase
rust-lang@0c84361342 Simplify the
`unchecked_sh[lr]` ub-checks a bit
rust-lang@f8279b10c3 Fix URL target, it's
in the module not the type.
rust-lang@2b2f83e5ff github showed that
weird.
rust-lang@2e8f14fb37 correct for copy
paste errors when fixing wrapping.
rust-lang@22668e83f6 Resolve
rust-lang#124870 (comment)
rust-lang@939f2671a0 revert to the
inconsistent paragraph wrapping.
rust-lang@eb9894f3c9 Removed return
rust-lang@afa8dfc51f Avoid clone when
constructing runnable label.
rust-lang@09677b03dd Formatting
rust-lang@78fe45e273 Semicolon
rust-lang@2315c6b764 Use correct format
for setting environment variables when debugging with cpptools
rust-lang@331bb3f10d Auto merge of rust-lang#3630
- rust-lang:rustup-2024-05-25, r=saethlin
rust-lang@bebcb4e4b8 Also mention my-self
for check-cfg docs changes
rust-lang@c76477d909 add change entry
rust-lang@56dddd4c7e Remove deprecated
field `dist.missing-tools`
rust-lang@1d0ad04993 Merge from rustc
rust-lang@3cfcfbf083 Preparing for merge
from rustc
rust-lang@41d4a95fca Add "better" edition
handling on lint-docs tool
rust-lang@278212342e cleanup dependence
of `ExtCtxt` in transcribe when macro expansion
rust-lang@24b5466892 drop region
constraints for ambiguous goals
rust-lang@ed8e436916 move generics_of
call outside of iter
rust-lang@56d77b9048 Auto merge of rust-lang#17275
- roife:fix-issue-17012, r=Veykril
rust-lang@796cb8031d Remove failing tests
rust-lang@f856ee357c Remove `DefId` from
`EarlyParamRegion` (clippy/smir)
rust-lang@fe2d7794ca Remove `DefId` from
`EarlyParamRegion` (tedium/diagnostics)
rust-lang@bd6344d829 Remove `DefId` from
`EarlyParamRegion` (type system)
rust-lang@b7b350cff7 docs
rust-lang@008f6b3a3f Auto merge of rust-lang#3626
- devnexen:pthread_name_illumos, r=oli-obk
rust-lang@7fc41d1bdf Auto merge of rust-lang#3625
- Strophox:miri-allocation-fix, r=RalfJung
rust-lang@b84620ff17 extend comments
rust-lang@88d519f718 Auto merge of rust-lang#3628
- RalfJung:tokio, r=RalfJung
rust-lang@561bd9a5ec add back some tokio
features
rust-lang@10d414091b Auto merge of rust-lang#3627
- rust-lang:rustup-2024-05-24, r=RalfJung
rust-lang@4763eaf066 fmt
rust-lang@debf88ae1a Merge from rustc
rust-lang@9ce95c30b2 Preparing for merge
from rustc
rust-lang@c58b7c9c81 Don't skip inner
const when looking for body for suggestion
rust-lang@4bc41b91d7 Don't continue
probing for method if in suggestion and autoderef hits ambiguity
rust-lang@7f5e0aade8 solaris add suport
for threadname.
rust-lang@3c7a13d870 tests: update test
for runnables
rust-lang@c10bda5577 Update docs
rust-lang@1a37cfb703 Use cwd from
runnable.args for debugger
rust-lang@7b54c8231e Revert "Debug use
cargo workspace root as cwd. fixes rust-lang#13022"
rust-lang@d83b267bc1 Add cwd to
CargoRunnable
rust-lang@6259991f04 Auto merge of rust-lang#17287
- Veykril:sysroot-encode-empty, r=Veykril
rust-lang@f93256ca42 Allow sysroots to
only consist of the source root dir
rust-lang@1b374dfd9b differentiate
between layout and alloc_layout
rust-lang@56c363b43e fix alloc_bytes
(always allocate at least 1B)
rust-lang@ecadf37df4 Auto merge of rust-lang#17284
- Veykril:doc-links, r=Veykril
rust-lang@616fdd04bb Use correct
toolchain channel when generating builtin type doc links
rust-lang@6e8646df8b Auto merge of rust-lang#17174
- Kohei316:fix-infer-async-block-with-tail-return-expr, r=Veykril
rust-lang@425ed6a181 Update
crates/hir-ty/src/infer/expr.rs
rust-lang@68fe34a4c2 Auto merge of rust-lang#17140
- harrysarson:harry-unused-self, r=Veykril
rust-lang@6ea763b9e2 Auto merge of rust-lang#3624
- rust-lang:rustup-2024-05-23, r=RalfJung
rust-lang@400835fd11 fmt
rust-lang@f1ffb8d859 Merge from rustc
rust-lang@807a0f8c21 Preparing for merge
from rustc
rust-lang@37bf2d2dab Delay the
construction of early lint diag structs
rust-lang@9f67c50128 Remove `DelayDm`
rust-lang@06bc4fc671 Remove
`LintDiagnostic::msg`
rust-lang@366ef95407 Slightly clean up
some lint infra code
rust-lang@ac2708a347 Auto merge of rust-lang#17270
- davidbarsky:david/fix-completions-from-associated-types, r=Veykril
rust-lang@f2c3ef77b1 fix: ensure implied
bounds from associated types are considered in autocomplete
rust-lang@04a9a1a531 Auto merge of rust-lang#3614
- devnexen:illumos_time_support, r=oli-obk
rust-lang@0916e72a34 Auto merge of rust-lang#17251
- roife:fix-issue-17057, r=Veykril
rust-lang@56ce7e0e06 Auto merge of rust-lang#17252
- davidbarsky:david/refactor-standalone-bools-into-struct, r=Veykril
rust-lang@f50f8fbcb9 Simplify
rust-lang@7a21dff517 internal: refactor
`prefer_no_std`/`prefer_prelude` bools into a struct
rust-lang@4e9b12870c fix: check
pseudo-block by local_id instead of ModuleOrigin
rust-lang@ad810a51f0 Auto merge of rust-lang#17277
- Veykril:find-path-fixes, r=Veykril
rust-lang@3f638a9291 solaris/illumos
localtime_r / clock_getime support enabled.
rust-lang@d9dda8f84f Auto merge of rust-lang#17279
- Veykril:format_args-escape, r=Veykril
rust-lang@2ff9bab2eb fix: Fix format_args
lowering passing incorrect parameters to rustc_parse_format
rust-lang@39e6032445 Auto merge of rust-lang#17248
- mladedav:dm/delay-clear, r=Veykril
rust-lang@24bf53d993 Auto merge of rust-lang#17268
- Veykril:signatures, r=Veykril
rust-lang@b1830a5fe6 Update assists test
fixtures
rust-lang@b29c755572 expectify find_path
tests
rust-lang@5992af6506 fix: Fix general
find-path inconsistencies
rust-lang@7fd1429754 Auto merge of rust-lang#3623
- RalfJung:rustup, r=RalfJung
rust-lang@abbe244a81 clippy
rust-lang@a1bc030b70 Merge from rustc
rust-lang@24138f0034 Preparing for merge
from rustc
rust-lang@c8b0e5b1a4 The number of tests
does not depend on the architecture's pointer width
rust-lang@719eee2d82 test: add tests for
extern preludes resolving in local mods
rust-lang@41c006e21a Auto merge of rust-lang#3610
- marc0246:missing-error-kinds, r=RalfJung
rust-lang@37a37f6ab3 Use
`throw_unsup_format` instead of returning `ENOTSUP` in the mmap shim
rust-lang@6438554bce Show fn traits in
signature info for trait implementors
rust-lang@f42e55dfc8 Enable linked
locations for closure param inlay hints
rust-lang@4b3d7f6039 Render closure fn
trait kind in siganture help
rust-lang@7045044da3 Allow hir::Param to
refer to other entity params aside from functions
rust-lang@9ff4ffb817 Update builtin tool
list
rust-lang@ea2a16cadb fix: resolve extern
prelude for local mods in block modules
rust-lang@1287e868e9 Clear diagnostics
only after new ones were received
rust-lang@17bd43cb25 codegen:
tweak/extend shift comments
rust-lang@b468f21051 Don't use `T` with
both Result and Option, improve explanation.
rust-lang@531dae1cdf Only allow immutable
statics with #[linkage]
rust-lang@10f8d1ffef use teletype on the
attribute name
rust-lang@f94fa6bee3 Some Result
combinations work like an Option.
rust-lang@dd16cbcb4e braces around {self}
in UseTree are not unnecessary
rust-lang@39a653f632 Fix coercion of
async block
rust-lang@f005b451c2 Support C23's
Variadics Without a Named Parameter
rust-lang@62a104df98 Update Tests

Co-authored-by: qinheping <16714939 [email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

8 participants