Rename UnevaluatedConst to AliasConst#158115
Conversation
|
This PR changes rustc_public cc @oli-obk, @celinval, @ouz-a, @makai410 changes to the core type system cc @lcnr Some changes occurred in cc @BoxyUwU Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor Some changes occurred in match checking cc @Nadrieril Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr Some changes occurred in cc @BoxyUwU HIR ty lowering was modified cc @fmease changes to the core type system cc @lcnr |
|
Failed to set assignee to
|
73249cb to
bea205a
Compare
This comment has been minimized.
This comment has been minimized.
|
thanks so much for working on this! ❤️ there's some more stuff that we want to rename related to this change (sorry for not being more clear in the issue description!):
I discovered the above list by running Would you like to work on all this other stuff in phases, or would you rather do everything in one big PR? Up to you I think! |
Thanks for thorough review and suggestions. I would like to get them in, in this PR itself (would be scary to ping these many people again). I will make sure to add commits for each update for easier review. I will reach out, if I have some doubts. Thanks. |
I think I have addressed most of these, I have few questions which I posted side by side to the code change. Super thanks for the review. @khyperia (for some reason bors acting weird with assignments.) |
This comment has been minimized.
This comment has been minimized.
I'm not actually part of the project 😅 (yet...), so I don't have |
9f77834 to
1309d08
Compare
|
Ah!! let me try again, considering bors should do assignment based on comments. r? @khyperia yay!!! it did! |
| const HAS_TY_INHERENT = 1 << 13; | ||
| /// Does this have `ConstKind::Unevaluated`? | ||
| /// Does this have `ConstKind::Alias`? | ||
| const HAS_CT_PROJECTION = 1 << 14; |
There was a problem hiding this comment.
This is totally unrelated to your PR, but I dislike the name of this bitflag. To me, HAS_CT_PROJECTION corresponds to having a AliasConstKind::Projection, just like HAS_TY_PROJECTION does, but no, the comment is correct, HAS_CT_PROJECTION corresponds to having any ConstKind::Alias. (Pointing this out in case any other reviewer sees this and has the same "???" as me)
There was a problem hiding this comment.
Yeah this is weird :3 follow up item to change this I think, probably at minimum we should rename this to HAS_CONST_ALIASES
There was a problem hiding this comment.
Looks great to me! Feel free to fix up the little nits/questions you had, but as they're not directly relevant to the core of the change, I'm also happy as-is. Thank you so much for working on this, it's gonna make it sooo much nicer to talk about these things, and help the readability of the compiler so much.
As for someone who can actually r+, I'm guessing anyone in the Const Generics Project Group (I'm guessing Boxy probably knows what's up here the best, but she's also very swamped with reviews, so idk)
I still like the "unevalutaed" name and don't understand how this is an "alias" (a const item is not an alias for anything?), so please discuss this with wg-const-eval and explain the issues with the current names before renaming anything on the MIR side. t-types is not the only group interacting with MIR constants, and to a non-types-person, this "alias" terminology is more confusing than helpful IMO. (For type constants I have no objections to t-types picking whatever name you find most helpful, but when writing docs, please keep in mind that whatever analogy you see between type aliases and consts is unlikely to be obvious to others.) |
This comment has been minimized.
This comment has been minimized.
| const HAS_TY_INHERENT = 1 << 13; | ||
| /// Does this have `ConstKind::Unevaluated`? | ||
| /// Does this have `ConstKind::Alias`? | ||
| const HAS_CT_PROJECTION = 1 << 14; |
There was a problem hiding this comment.
Yeah this is weird :3 follow up item to change this I think, probably at minimum we should rename this to HAS_CONST_ALIASES
|
thx yeah this looks good to me, one nit & needs rebasing then can be merged. i agree with ralf that we shouldn't change the MIR constant stuff, that stuff will remain just being unevaluated consts rather than being type system consts. thanks @khyperia for reviews and @Shourya742 for working on this 💚 im really happy to see this finally happen after years of having this awkward inconsistency 😅 |
|
@rustbot author |
* Const::new_unevaluated / interner method -> new_alias * Interner::UnevaluatedConstId -> AnonConstId, since assoc type is only used for anonnymous consts
f0c6dcd to
8f9b763
Compare
|
☔ The latest upstream changes (presumably #158013) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. This pull request was unapproved. |
|
the answer was yes it will 😆 |
View all comments
refer: rust-lang/project-const-generics#115
r? @khyperia
I only renamed the type system version to
AliasConst/AliasConstKind. I did not rename MIR’sUnevaluatedConst, it currently holds direct def_id instead of kind variant we have in type-system. I also leftrustc_public’sUnevaluatedConstunchanged.