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

camelid · 2024-06-03T06:27:45Z

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

Use ConstArgKind::Path for all const paths, not just const params.
Fix (no github dont close this issue) ICE(non_lifetime_binders): const bound params not handled correctly #127009

r? @BoxyUwU

rustbot · 2024-06-03T06:27:54Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

HIR ty lowering was modified

cc @fmease

compiler/rustc_hir/src/hir.rs

rust-timer · 2024-07-19T13:29:53Z

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%)

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`

…r=BoxyUwU" This reverts commit 8c3a94a, reversing changes made to 3d68afc.

[beta] backports and bump stage0 - Disable jump threading of float equality rust-lang#128271 - Normalize when equating `dyn` tails in MIR borrowck rust-lang#128694 - Improve `Ord` violation help rust-lang#128273 - bump stage0 to stable 1.80.1 - Revert rust-lang#125915 on beta rust-lang#128760 - derive(SmartPointer): register helper attributes rust-lang#128925 - Fix bug in `Parser::look_ahead`. rust-lang#128994 r? cuviper

Ever since rust-lang#125915, some `ast::AnonConst`s turn into `hir::ConstArgKind::Path`s, which don't have associated `DefId`s. To deal with the fact that we don't have resolution information in `DefCollector`, we decided to implement a process where if the anon const *appeared* to be trivial (i.e., `N` or `{ N }`), we would avoid creating a def for it in `DefCollector`. If later, in AST lowering, we realized it turned out to be a unit struct literal, or we were lowering it to something that didn't use `hir::ConstArg`, we'd create its def there. However, let's say we have a macro `m!()` that expands to a reference to a free constant `FOO`. If we use `m!()` in the body of an anon const (e.g., `Foo<{ m!() }>`), then in def collection, it appears to be a nontrivial anon const and we create a def. But the macro expands to something that looks like a trivial const arg, but is not, so in AST lowering we "fix" the mistake we assumed def collection made and create a def for it. This causes a duplicate definition ICE. The long-term fix for this is to delay the creation of defs for all expression-like nodes until AST lowering (see rust-lang#128844 for an incomplete attempt at this). This would avoid issues like this one that are caused by hacky workarounds. However, doing this uncovers a pre-existing bug with opaque types that is quite involved to fix (see rust-lang#129023). In the meantime, this PR fixes the bug by delaying def creation for anon consts whose bodies are macro invocations until after we expand the macro and know what is inside it. This is accomplished by adding information to create the anon const's def to the data in `Resolver.invocation_parents`.

…h, r=cjgillot Retroactively feature gate `ConstArgKind::Path` This puts the lowering introduced by rust-lang#125915 under a feature gate until we fix the regressions introduced by it. Alternative to whole sale reverting the PR since it didn't seem like a very clean revert and I think this is generally a step in the right direction and don't want to get stuck landing and reverting the PR over and over :) cc rust-lang#129137 `@camelid,` tests taken from there. beta is branching soon so I think it makes sense to not try and rush that fix through since it wont have much time to bake and if it has issues we can't simply revert it on beta. Fixes rust-lang#128016

…h, r=cjgillot Retroactively feature gate `ConstArgKind::Path` This puts the lowering introduced by rust-lang#125915 under a feature gate until we fix the regressions introduced by it. Alternative to whole sale reverting the PR since it didn't seem like a very clean revert and I think this is generally a step in the right direction and don't want to get stuck landing and reverting the PR over and over :) cc rust-lang#129137 ``@camelid,`` tests taken from there. beta is branching soon so I think it makes sense to not try and rush that fix through since it wont have much time to bake and if it has issues we can't simply revert it on beta. Fixes rust-lang#128016

Rollup merge of rust-lang#129246 - BoxyUwU:feature_gate_const_arg_path, r=cjgillot Retroactively feature gate `ConstArgKind::Path` This puts the lowering introduced by rust-lang#125915 under a feature gate until we fix the regressions introduced by it. Alternative to whole sale reverting the PR since it didn't seem like a very clean revert and I think this is generally a step in the right direction and don't want to get stuck landing and reverting the PR over and over :) cc rust-lang#129137 ``@camelid,`` tests taken from there. beta is branching soon so I think it makes sense to not try and rush that fix through since it wont have much time to bake and if it has issues we can't simply revert it on beta. Fixes rust-lang#128016

Ever since rust-lang#125915, some `ast::AnonConst`s turn into `hir::ConstArgKind::Path`s, which don't have associated `DefId`s. To deal with the fact that we don't have resolution information in `DefCollector`, we decided to implement a process where if the anon const *appeared* to be trivial (i.e., `N` or `{ N }`), we would avoid creating a def for it in `DefCollector`. If later, in AST lowering, we realized it turned out to be a unit struct literal, or we were lowering it to something that didn't use `hir::ConstArg`, we'd create its def there. However, let's say we have a macro `m!()` that expands to a reference to a free constant `FOO`. If we use `m!()` in the body of an anon const (e.g., `Foo<{ m!() }>`), then in def collection, it appears to be a nontrivial anon const and we create a def. But the macro expands to something that looks like a trivial const arg, but is not, so in AST lowering we "fix" the mistake we assumed def collection made and create a def for it. This causes a duplicate definition ICE. The long-term fix for this is to delay the creation of defs for all expression-like nodes until AST lowering (see rust-lang#128844 for an incomplete attempt at this). This would avoid issues like this one that are caused by hacky workarounds. However, doing this uncovers a pre-existing bug with opaque types that is quite involved to fix (see rust-lang#129023). In the meantime, this PR fixes the bug by delaying def creation for anon consts whose bodies are macro invocations until after we expand the macro and know what is inside it. This is accomplished by adding information to create the anon const's def to the data in `Resolver.invocation_parents`.

Ever since rust-lang#125915, some `ast::AnonConst`s turn into `hir::ConstArgKind::Path`s, which don't have associated `DefId`s. To deal with the fact that we don't have resolution information in `DefCollector`, we decided to implement a process where if the anon const *appeared* to be trivial (i.e., `N` or `{ N }`), we would avoid creating a def for it in `DefCollector`. If later, in AST lowering, we realized it turned out to be a unit struct literal, or we were lowering it to something that didn't use `hir::ConstArg`, we'd create its def there. However, let's say we have a macro `m!()` that expands to a reference to a free constant `FOO`. If we use `m!()` in the body of an anon const (e.g., `Foo<{ m!() }>`), then in def collection, it appears to be a nontrivial anon const and we create a def. But the macro expands to something that looks like a trivial const arg, but is not, so in AST lowering we "fix" the mistake we assumed def collection made and create a def for it. This causes a duplicate definition ICE. The ideal long-term fix for this is a bit unclear. One option is to delay def creation for all expression-like nodes until AST lowering (see rust-lang#128844 for an incomplete attempt at this). This would avoid issues like this one that are caused by hacky workarounds. However, this approach has some downsides as well, and the best approach is yet to be determined. In the meantime, this PR fixes the bug by delaying def creation for anon consts whose bodies are macro invocations until after we expand the macro and know what is inside it. This is accomplished by adding information to create the anon const's def to the data in `Resolver.invocation_parents`.

Fix anon const def-creation when macros are involved Fixes rust-lang#128016. Ever since rust-lang#125915, some `ast::AnonConst`s turn into `hir::ConstArgKind::Path`s, which don't have associated `DefId`s. To deal with the fact that we don't have resolution information in `DefCollector`, we decided to implement a process where if the anon const *appeared* to be trivial (i.e., `N` or `{ N }`), we would avoid creating a def for it in `DefCollector`. If later, in AST lowering, we realized it turned out to be a unit struct literal, or we were lowering it to something that didn't use `hir::ConstArg`, we'd create its def there. However, let's say we have a macro `m!()` that expands to a reference to a free constant `FOO`. If we use `m!()` in the body of an anon const (e.g., `Foo<{ m!() }>`), then in def collection, it appears to be a nontrivial anon const and we create a def. But the macro expands to something that looks like a trivial const arg, but is not, so in AST lowering we "fix" the mistake we assumed def collection made and create a def for it. This causes a duplicate definition ICE. The long-term fix for this is to delay the creation of defs for all expression-like nodes until AST lowering (see rust-lang#128844 for an incomplete attempt at this). This would avoid issues like this one that are caused by hacky workarounds. However, doing this uncovers a pre-existing bug with opaque types that is quite involved to fix (see rust-lang#129023). In the meantime, this PR fixes the bug by delaying def creation for anon consts whose bodies are macro invocations until after we expand the macro and know what is inside it. This is accomplished by adding information to create the anon const's def to the data in `Resolver.invocation_parents`. r? `@BoxyUwU`

Ever since rust-lang#125915, some `ast::AnonConst`s turn into `hir::ConstArgKind::Path`s, which don't have associated `DefId`s. To deal with the fact that we don't have resolution information in `DefCollector`, we decided to implement a process where if the anon const *appeared* to be trivial (i.e., `N` or `{ N }`), we would avoid creating a def for it in `DefCollector`. If later, in AST lowering, we realized it turned out to be a unit struct literal, or we were lowering it to something that didn't use `hir::ConstArg`, we'd create its def there. However, let's say we have a macro `m!()` that expands to a reference to a free constant `FOO`. If we use `m!()` in the body of an anon const (e.g., `Foo<{ m!() }>`), then in def collection, it appears to be a nontrivial anon const and we create a def. But the macro expands to something that looks like a trivial const arg, but is not, so in AST lowering we "fix" the mistake we assumed def collection made and create a def for it. This causes a duplicate definition ICE. The ideal long-term fix for this is a bit unclear. One option is to delay def creation for all expression-like nodes until AST lowering (see rust-lang#128844 for an incomplete attempt at this). This would avoid issues like this one that are caused by hacky workarounds. However, this approach has some downsides as well, and the best approach is yet to be determined. In the meantime, this PR fixes the bug by delaying def creation for anon consts whose bodies are macro invocations until after we expand the macro and know what is inside it. This is accomplished by adding information to create the anon const's def to the data in `Resolver.invocation_parents`.

camelid added A-const-generics Area: const generics (parameters and arguments) F-generic_const_exprs `#![feature(generic_const_exprs)]` labels Jun 3, 2024

rustbot assigned BoxyUwU Jun 3, 2024

camelid marked this pull request as draft June 3, 2024 06:27

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

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

This comment has been minimized.

Sign in to view

camelid force-pushed the const-arg-refactor branch from 5819f98 to 9d6aa97 Compare June 3, 2024 06:35

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 force-pushed the const-arg-refactor branch 5 times, most recently from d9dc960 to 98df547 Compare June 4, 2024 20:05

This comment has been minimized.

Sign in to view

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

oli-obk reviewed Jun 8, 2024

View reviewed changes

compiler/rustc_hir/src/hir.rs Show resolved Hide resolved

camelid force-pushed the const-arg-refactor branch from 1755fdf to 4bea486 Compare June 9, 2024 05:00

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

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

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

bors mentioned this pull request Jul 19, 2024

Skip const-eval if evaluatable predicate is trivial #127242

Closed

rustbot removed the perf-regression Performance regression. label Jul 19, 2024

camelid deleted the const-arg-refactor branch July 19, 2024 18:31

camelid mentioned this pull request Jul 19, 2024

ICE. try_from_lit: received const param which shouldn't be possible #127972

Open

GrigorenkoPV mentioned this pull request Jul 20, 2024

ICE: ConstArgHasType(^0, usize) has escaping bound vars, so it cannot be wrapped in a dummy binder. #127970

Closed

matthiaskrgr mentioned this pull request Jul 20, 2024

ICE: adding a def'n for node-id NodeId(18) and def kind AnonConst but a previous def'n exists #128016

Closed

BoxyUwU added a commit to BoxyUwU/rust that referenced this pull request Aug 6, 2024

Revert "Auto merge of rust-lang#125915 - camelid:const-arg-refactor, …

a885fce

…r=BoxyUwU" This reverts commit 8c3a94a, reversing changes made to 3d68afc.

BoxyUwU mentioned this pull request Aug 6, 2024

Revert #125915 on beta #128760

Closed

cuviper pushed a commit to cuviper/rust that referenced this pull request Aug 15, 2024

Revert "Auto merge of rust-lang#125915 - camelid:const-arg-refactor, …

9fe9265

…r=BoxyUwU" This reverts commit 8c3a94a, reversing changes made to 3d68afc.

cuviper mentioned this pull request Aug 15, 2024

[beta] backports and bump stage0 #129033

Merged

camelid mentioned this pull request Aug 15, 2024

Fix anon const def-creation when macros are involved #129137

Merged

BoxyUwU mentioned this pull request Aug 18, 2024

Retroactively feature gate ConstArgKind::Path #129246

Merged

petrochenkov mentioned this pull request Aug 19, 2024

Opaque types should not be HIR items (and thus not be HIR owners) #129023

Closed

jieyouxu mentioned this pull request Sep 20, 2024

ICE when using macro to wrap const generic #130589

Closed

matthiaskrgr mentioned this pull request Oct 11, 2024

ICE: ConstArgHasType has escaping bound vars, so it cannot be wrapped in a dummy binder. #131534

Open

matthiaskrgr mentioned this pull request Oct 19, 2024

ICE: adding a def'n for node-id NodeId(13) and def kind AnonConst but a previous def'n exists: DefKey #131915

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

camelid commented Jun 3, 2024 •

edited by BoxyUwU

Loading

rustbot commented Jun 3, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment has been minimized.

This comment was marked as resolved.

rust-timer commented Jul 19, 2024

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

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

Conversation

camelid commented Jun 3, 2024 • edited by BoxyUwU Loading

Summary

Followup items post-merge

rustbot commented Jun 3, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment has been minimized.

This comment was marked as resolved.

rust-timer commented Jul 19, 2024

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

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

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

camelid commented Jun 3, 2024 •

edited by BoxyUwU

Loading