Skip to content

mgca: Lower all const paths as ConstArgKind::Path #135186

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Mar 5, 2025

Conversation

camelid
Copy link
Member

@camelid camelid commented Jan 7, 2025

When #![feature(min_generic_const_args)] is enabled, we now lower all
const paths in generic arg position to hir::ConstArgKind::Path. We
then lower assoc const paths to ty::ConstKind::Unevaluated since we
can no longer use the anon const expression lowering machinery. In the
process of implementing this, I factored out hir_ty_lowering code that
is now shared between lowering assoc types and assoc consts.

This PR also introduces a #[type_const] attribute for trait assoc
consts that are allowed as const args. However, we still need to
implement code to check that assoc const definitions satisfy
#[type_const] if present (basically is it a const path or a
monomorphic anon const).

r? @BoxyUwU

@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 Jan 7, 2025
@rust-log-analyzer

This comment has been minimized.

@camelid camelid changed the title Start lowering multi-segment const paths as ConstArgKind::Path Lower multi-segment const paths as ConstArgKind::Path Jan 7, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@camelid camelid marked this pull request as ready for review January 11, 2025 02:51
@rustbot
Copy link
Collaborator

rustbot commented Jan 11, 2025

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

HIR ty lowering was modified

cc @fmease

@camelid camelid changed the title Lower multi-segment const paths as ConstArgKind::Path Lower multi-segment const paths as ConstArgKind::Path Jan 11, 2025
Copy link
Member

@BoxyUwU BoxyUwU left a comment

Choose a reason for hiding this comment

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

thx for working on this :3

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Jan 23, 2025

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

@BoxyUwU BoxyUwU 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 Jan 23, 2025
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 27, 2025
@camelid camelid changed the title Lower multi-segment const paths as ConstArgKind::Path Lower all trivial const paths as ConstArgKind::Path Jan 27, 2025
@bors
Copy link
Collaborator

bors commented Jan 27, 2025

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

@BoxyUwU BoxyUwU 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 Jan 28, 2025
@bors
Copy link
Collaborator

bors commented Feb 25, 2025

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

@camelid camelid force-pushed the const-path-multi branch from abe7d68 to 991f55c Compare March 4, 2025 03:59
camelid added a commit to camelid/rust that referenced this pull request Mar 4, 2025
A bit tricky to support this special case now, so might as well save it
for once we're ready to handle the general case. See
rust-lang#135186 (comment).

Co-authored-by: BoxyUwU <rust@boxyuwu.dev>
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 4, 2025
@BoxyUwU
Copy link
Member

BoxyUwU commented Mar 4, 2025

lgtm, once you've squashed the commits ill r+ (and probably should update the PR description/title)

@BoxyUwU BoxyUwU 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 Mar 4, 2025
@camelid camelid changed the title Lower all trivial const paths as ConstArgKind::Path mgca: Lower all const paths as ConstArgKind::Path Mar 4, 2025
When `#![feature(min_generic_const_args)]` is enabled, we now lower all
const paths in generic arg position to `hir::ConstArgKind::Path`. We
then lower assoc const paths to `ty::ConstKind::Unevaluated` since we
can no longer use the anon const expression lowering machinery. In the
process of implementing this, I factored out `hir_ty_lowering` code that
is now shared between lowering assoc types and assoc consts.

This PR also introduces a `#[type_const]` attribute for trait assoc
consts that are allowed as const args. However, we still need to
implement code to check that assoc const definitions satisfy
`#[type_const]` if present (basically is it a const path or a
monomorphic anon const).
@camelid camelid force-pushed the const-path-multi branch from c082ce1 to 177e7ff Compare March 4, 2025 15:11
@camelid camelid added A-const-generics Area: const generics (parameters and arguments) F-min_generic_const_args `#![feature(min_generic_const_args)]` S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 4, 2025
@BoxyUwU
Copy link
Member

BoxyUwU commented Mar 4, 2025

@bors r+ rollup=never

@bors
Copy link
Collaborator

bors commented Mar 4, 2025

📌 Commit 177e7ff has been approved by BoxyUwU

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 Mar 4, 2025
@bors
Copy link
Collaborator

bors commented Mar 4, 2025

⌛ Testing commit 177e7ff with merge 08db600...

@bors
Copy link
Collaborator

bors commented Mar 5, 2025

☀️ Test successful - checks-actions
Approved by: BoxyUwU
Pushing 08db600 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 5, 2025
@bors bors merged commit 08db600 into rust-lang:master Mar 5, 2025
7 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 5, 2025
@camelid camelid deleted the const-path-multi branch March 5, 2025 02:31
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (08db600): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -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.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary 0.6%)

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)
2.1% [1.8%, 2.4%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) - - 0

Cycles

Results (secondary -2.1%)

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)
-2.1% [-2.1%, -2.1%] 1
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 778.226s -> 775.839s (-0.31%)
Artifact size: 362.05 MiB -> 362.04 MiB (-0.00%)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-const-generics Area: const generics (parameters and arguments) F-min_generic_const_args `#![feature(min_generic_const_args)]` 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.

6 participants