Rename UnevaluatedConst to AliasConst

[Shourya742]

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’s UnevaluatedConst, it currently holds direct def_id instead of kind variant we have in type-system. I also left rustc_public’s UnevaluatedConst unchanged.

[rustbot]

This PR changes rustc_public

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

changes to the core type system

cc @lcnr

Some changes occurred in const_evaluatable.rs

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 rustc_ty_utils::consts.rs

cc @BoxyUwU

HIR ty lowering was modified

cc @fmease

changes to the core type system

cc @lcnr

[rustbot]

Failed to set assignee to khyperia: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

[khyperia]

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!):

• ConstKind::Unevaluated (variant that just holds a UnevaluatedConst, its name should reflect what it contains)
• Const::new_unevaluated (constructor for the previous variant)
  • (and the Interner trait for new_unevaluated)
• Interner::UnevaluatedConstId (this one is a bit sketch, I believe it's actually only used for anon consts, not all AliasConsts, so it should possibly be renamed to AnonConstId instead of AliasConstId. Please double check for me though and make a judgement call!)
• perhaps little things like AliasTerm::to_term has a local named unevaluated_const, some other places also call local variables unevaluated. This is less important though, IMO!
  • same thing that we have a relatively consistent naming that variables of type UnevaluatedConst are called uv, could possibly rename those as uv no longer makes sense. But, it's not as important, we can also clean this up over time, IMO.
• lots of comments referring to Unevaluated that could be rephrased if you'd like (it'd be nice to not have to know the legacy information that these things used to be called unevaluated consts while reading comments, but not strictly necessary to update I suppose)
• I need to remember to update the rustc dev guide (but honestly that's on me, that page is pretty out of date after my recent refactorings, I've been forgetting to update it)

I discovered the above list by running rg -i unevaluated in compiler/, and being like, "hmm, does this mention of the word 'unevaluated' have to do with the concept of UnevaluatedConst, or is it just talking about something in general being unevaluated? (and ignoring mir and rustc_public's UnevaluatedConst)". (Just letting you know so you can do the same kind of thing if you'd like! I might have missed some stuff)

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!

[Shourya742]

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.

[Shourya742]

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!):

• ConstKind::Unevaluated (variant that just holds a UnevaluatedConst, its name should reflect what it contains)

• Const::new_unevaluated (constructor for the previous variant)

  • (and the Interner trait for new_unevaluated)

• Interner::UnevaluatedConstId (this one is a bit sketch, I believe it's actually only used for anon consts, not all AliasConsts, so it should possibly be renamed to AnonConstId instead of AliasConstId. Please double check for me though and make a judgement call!)

• perhaps little things like AliasTerm::to_term has a local named unevaluated_const, some other places also call local variables unevaluated. This is less important though, IMO!

  • same thing that we have a relatively consistent naming that variables of type UnevaluatedConst are called uv, could possibly rename those as uv no longer makes sense. But, it's not as important, we can also clean this up over time, IMO.

• lots of comments referring to Unevaluated that could be rephrased if you'd like (it'd be nice to not have to know the legacy information that these things used to be called unevaluated consts while reading comments, but not strictly necessary to update I suppose)

• I need to remember to update the rustc dev guide (but honestly that's on me, that page is pretty out of date after my recent refactorings, I've been forgetting to update it)

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

[khyperia]

@khyperia (for some reason bors acting weird with assignments.)

I'm not actually part of the project 😅 (yet...), so I don't have r+ privs or stuff like that. But, I double checked with @BoxyUwU before making the issue for this change, and she (and a few other folks) are super on board with the change! (This change has been a long time coming I feel like)

[Shourya742]

Ah!! let me try again, considering bors should do assignment based on comments.

r? @khyperia

yay!!! it did!

[khyperia]

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)

View changes since the review

[BoxyUwU]

Yeah this is weird :3 follow up item to change this I think, probably at minimum we should rename this to HAS_CONST_ALIASES

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred in abstract_const.rs

cc @BoxyUwU

Some changes occurred to constck

cc @fee1-dead

Some changes occurred in need_type_info.rs

cc @lcnr

[khyperia]

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)

View changes since this review

[RalfJung]

I did not rename MIR’s UnevaluatedConst,

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

[BoxyUwU]

Yeah this is weird :3 follow up item to change this I think, probably at minimum we should rename this to HAS_CONST_ALIASES

[BoxyUwU]

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 😅

[BoxyUwU]

@bors r+

[rust-bors]

📌 Commit c502825 has been approved by BoxyUwU

It is now in the queue for this repository.

[BoxyUwU]

@bors r=BoxyUwU,khyperia

[rust-bors]

📌 Commit c502825 has been approved by BoxyUwU,khyperia

It is now in the queue for this repository.

[rust-bors]

💔 Test for 37b2cc2 failed: CI. Failed job:

• auto - x86_64-gnu-llvm-21-2 (web logs, enhanced plaintext logs)

[JonathanBrouwer]

@bors retry

[rust-bors]

💔 Test for 6948954 failed: CI. Failed job:

• auto - dist-i586-gnu-i586-i686-musl (web logs, enhanced plaintext logs)

[JonathanBrouwer]

@bors retry

[rust-log-analyzer]

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

Click to see the possible cause of the failure (guessed by this bot)

  TOOLSTATE_REPO_ACCESS_TOKEN: 
##[endgroup]
    Updating crates.io index
error: failed to get `adler2` as a dependency of package `miniz_oxide v0.8.8`
    ... which satisfies dependency `miniz_oxide = "^0.8.5"` of package `flate2 v1.1.9`
    ... which satisfies dependency `flate2 = "^1.1.9"` of package `citool v0.1.0 (/home/runner/work/rust/rust/src/ci/citool)`

Caused by:
  failed to load source for dependency `adler2`

Caused by:
  unable to update registry `crates-io`

Caused by:
  download of ad/le/adler2 failed

Caused by:
  curl failed

Caused by:

[rust-bors]

💔 Test for c5c0990 failed: CI. Failed job:

• auto - x86_64-gnu-llvm-22-3 (web logs, enhanced plaintext logs)