Skip to content

Represent type-level consts with new-and-improved hir::ConstArg#125915

Merged
bors merged 10 commits intorust-lang:masterfrom
camelid:const-arg-refactor
Jul 19, 2024
Merged

Represent type-level consts with new-and-improved hir::ConstArg#125915
bors merged 10 commits intorust-lang:masterfrom
camelid:const-arg-refactor

Conversation

@camelid
Copy link
Member

@camelid camelid commented Jun 3, 2024

Summary

This is a step toward min_generic_const_exprs. We now represent all const
generic arguments using an enum that differentiates between const paths
(temporarily just bare const params) and arbitrary anon consts that may perform
computations. This will enable us to cleanly implement the min_generic_const_args
plan of allowing the use of generics in paths used as const args, while
disallowing their use in arbitrary anon consts. Here is a summary of the salient
aspects of this change:

  • Add current_def_id_parent to LoweringContext

    This is needed to track anon const parents properly once we implement
    ConstArgKind::Path (which requires moving anon const def-creation
    outside of DefCollector).

  • Create hir::ConstArgKind enum with Path and Anon variants. Use it in the
    existing hir::ConstArg struct, replacing the previous hir::AnonConst field.

  • Use ConstArg for all instances of const args. Specifically, use it instead
    of AnonConst for assoc item constraints, array lengths, and const param
    defaults.

  • Some ast::AnonConsts now have their DefIds created in
    rustc_ast_lowering rather than DefCollector. This is because in some
    cases they will end up becoming a ConstArgKind::Path instead, which
    has no DefId. We have to solve this in a hacky way where we guess
    whether the AnonConst could end up as a path const since we can't
    know for sure until after name resolution (N could refer to a free
    const or a nullary struct). If it has no chance as being a const
    param, then we create a DefId in DefCollector -- otherwise we
    decide during ast_lowering. This will have to be updated once all path
    consts use ConstArgKind::Path.

  • We explicitly use ConstArgHasType for array lengths, rather than
    implicitly relying on anon const type feeding -- this is due to the
    addition of ConstArgKind::Path.

  • Some tests have their outputs changed, but the changes are for the
    most part minor (including removing duplicate or almost-duplicate
    errors). One test now ICEs, but it is for an incomplete, unstable
    feature and is now tracked at ICE(non_lifetime_binders): const bound params not handled correctly #127009.

Followup items post-merge

r? @BoxyUwU

@camelid camelid added A-const-generics Area: const generics (parameters and arguments) F-generic_const_exprs `#![feature(generic_const_exprs)]` labels Jun 3, 2024
@camelid camelid marked this pull request as draft June 3, 2024 06:27
@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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 3, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

HIR ty lowering was modified

cc @fmease

@camelid camelid removed the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jun 3, 2024
@rust-log-analyzer

This comment has been minimized.

@camelid camelid force-pushed the const-arg-refactor branch from 5819f98 to 9d6aa97 Compare June 3, 2024 06:35
@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 Jun 3, 2024
@camelid camelid force-pushed the const-arg-refactor branch 5 times, most recently from d9dc960 to 98df547 Compare June 4, 2024 20:05
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@camelid camelid changed the title [WIP] Lower const params in generics to a dedicated ConstArgKind variant [WIP] Represent type-level consts with new-and-improved hir::ConstArg Jun 8, 2024
@camelid camelid force-pushed the const-arg-refactor branch from 1755fdf to 4bea486 Compare June 9, 2024 05:00
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@camelid camelid force-pushed the const-arg-refactor branch from 486af6c to 405f7de Compare June 14, 2024 09:17
@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@camelid
Copy link
Member Author

camelid commented Jul 19, 2024

Let's re-approve this so it enters the queue once the tree is re-opened. @bors r=BoxyUwU

@bors
Copy link
Collaborator

bors commented Jul 19, 2024

📌 Commit c8457e6 has been approved by BoxyUwU

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 19, 2024

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

@bors
Copy link
Collaborator

bors commented Jul 19, 2024

⌛ Testing commit c8457e6 with merge 8c3a94a...

@bors
Copy link
Collaborator

bors commented Jul 19, 2024

☀️ Test successful - checks-actions
Approved by: BoxyUwU
Pushing 8c3a94a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 19, 2024
@bors bors merged commit 8c3a94a into rust-lang:master Jul 19, 2024
@rustbot rustbot added this to the 1.81.0 milestone Jul 19, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8c3a94a): 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.6% [-0.8%, -0.5%] 6
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.6% [-0.8%, -0.5%] 6

Max RSS (memory usage)

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

Cycles

Results (primary 2.2%, secondary 2.2%)

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)
2.2% [2.2%, 2.2%] 1
Regressions ❌
(secondary)
2.2% [1.5%, 2.7%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.2% [2.2%, 2.2%] 1

Binary size

Results (primary -0.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.2% [0.0%, 0.6%] 36
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-1.1%, -0.0%] 23
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.1% [-1.1%, 0.6%] 59

Bootstrap: 768.827s -> 770.782s (0.25%)
Artifact size: 328.75 MiB -> 328.81 MiB (0.02%)

@rustbot rustbot removed the perf-regression Performance regression. label Jul 19, 2024
@camelid camelid deleted the const-arg-refactor branch July 19, 2024 18:31
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 25, 2024
Represent type-level consts with new-and-improved `hir::ConstArg`

### Summary

This is a step toward `min_generic_const_exprs`. We now represent all const
generic arguments using an enum that differentiates between const *paths*
(temporarily just bare const params) and arbitrary anon consts that may perform
computations. This will enable us to cleanly implement the `min_generic_const_args`
plan of allowing the use of generics in paths used as const args, while
disallowing their use in arbitrary anon consts. Here is a summary of the salient
aspects of this change:

- Add `current_def_id_parent` to `LoweringContext`

  This is needed to track anon const parents properly once we implement
  `ConstArgKind::Path` (which requires moving anon const def-creation
  outside of `DefCollector`).

- Create `hir::ConstArgKind` enum with `Path` and `Anon` variants. Use it in the
  existing `hir::ConstArg` struct, replacing the previous `hir::AnonConst` field.

- Use `ConstArg` for all instances of const args. Specifically, use it instead
  of `AnonConst` for assoc item constraints, array lengths, and const param
  defaults.

- Some `ast::AnonConst`s now have their `DefId`s created in
  rustc_ast_lowering rather than `DefCollector`. This is because in some
  cases they will end up becoming a `ConstArgKind::Path` instead, which
  has no `DefId`. We have to solve this in a hacky way where we guess
  whether the `AnonConst` could end up as a path const since we can't
  know for sure until after name resolution (`N` could refer to a free
  const or a nullary struct). If it has no chance as being a const
  param, then we create a `DefId` in `DefCollector` -- otherwise we
  decide during ast_lowering. This will have to be updated once all path
  consts use `ConstArgKind::Path`.

- We explicitly use `ConstArgHasType` for array lengths, rather than
  implicitly relying on anon const type feeding -- this is due to the
  addition of `ConstArgKind::Path`.

- Some tests have their outputs changed, but the changes are for the
  most part minor (including removing duplicate or almost-duplicate
  errors). One test now ICEs, but it is for an incomplete, unstable
  feature and is now tracked at rust-lang#127009.

### Followup items post-merge

- Use `ConstArgKind::Path` for all const paths, not just const params.
- Fix (no github dont close this issue) rust-lang#127009
- If a path in generic args doesn't resolve as a type, try to resolve as a const
  instead (do this in rustc_resolve). Then remove the special-casing from
  `rustc_ast_lowering`, so that all params will automatically be lowered as
  `ConstArgKind::Path`.
- (?) Consider making `const_evaluatable_unchecked` a hard error, or at least
  trying it in crater

r? `@BoxyUwU`
BoxyUwU added a commit to BoxyUwU/rust that referenced this pull request Aug 6, 2024
…r=BoxyUwU"

This reverts commit 8c3a94a, reversing
changes made to 3d68afc.
@BoxyUwU BoxyUwU mentioned this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-const-generics Area: const generics (parameters and arguments) 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.

9 participants