remove AliasTerm::def_id() by khyperia · Pull Request #157653 · rust-lang/rust

khyperia · 2026-06-09T08:52:08Z

View all comments

this is part of #156181

as well as part of #152245

fyi @lcnr

r? @BoxyUwU

rustbot · 2026-06-09T08:52:11Z

HIR ty lowering was modified

cc @fmease

changes to the core type system

cc @lcnr

This PR changes rustc_public

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

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

rustbot · 2026-06-09T08:52:14Z

BoxyUwU is currently at their maximum review capacity.
They may take a while to respond.

BoxyUwU · 2026-06-10T23:32:40Z

        let cx = self.cx();
        let free_alias = goal.predicate.alias;

+        let def_id = match free_alias.kind {


thoughts on mirroring the expect_projection_def_id setup for free aliases and inherent aliases and then dropping all these explicit matches? i feel like these kinda matches detract from the surrounding code and a method would flow better :3

View changes since the review

sure! I could see it either way, explicit for clarity vs. concise for quick readability. Probably good to add expect_free_def_id and friends, which would then make it clear in the future where ty::Alias<ty::FreeAliasTermKind> should be introduced by find-all-refs on the expect methods. (just explaining my logic, I kept expect_projection_def_id because it's used a bunch, but free/inherent matching a bit less often)

BoxyUwU · 2026-06-10T23:33:17Z

    ) -> Result<Candidate<I>, NoSolutionOrRerunNonErased> {
        let cx = ecx.cx();
-        let def_id = goal.predicate.def_id().try_into().unwrap();
+        let def_id = match goal.predicate.alias.kind {


could also have an expect_projecftion_ty_def_id 🤔

View changes since the review

BoxyUwU · 2026-06-10T23:35:51Z

    selcx: &'a mut SelectionContext<'b, 'tcx>,
    param_env: ty::ParamEnv<'tcx>,
    alias_term: ty::AliasTerm<'tcx>,
+    alias_def_id: DefId,


feels vaguely unfortunate to have to take a defid here even though its part of alias_term. feels like a bit of a code smell because the latter argument is implied by the former argument.

do you think it matters for perf to not match on the alias kind inside of here.

separately, kinda makes me wonder whether we want to make ProjectionAliasTerm FreeAliasTerm and InherentAliasTerm types so we can track this in the type system? might be too much effort... I want pattern types 😭...

View changes since the review

separately, kinda makes me wonder whether we want to make ProjectionAliasTerm FreeAliasTerm and InherentAliasTerm types so we can track this in the type system? might be too much effort... I want pattern types 😭...

my long-term hot take is that after #156538 lands, all the unwraps here could be replaced by narrowing the type to ty::Alias<ty::ProjectionAliasTermKind> instead of the current ty::Alias<ty::AliasTermKind> (perhaps with a fn def_id impl on ty::Alias<ty::ProjectionAliasTermKind> that would replace expect_projection_def_id)

that requires waiting for that PR to land though, and would also make this PR even bigger than it currently is, so IMO I'll do it in a followup!

feels like a bit of a code smell. do you think it matters for perf

but yeah there's parts that are pretty gross. it probably doesn't matter for perf, was just trying to make code style decisions, there's not really a clear good answer for many of these places.

BoxyUwU · 2026-06-10T23:36:55Z

-    /// into `self.out`.
-    fn add_wf_preds_for_alias_term(&mut self, data: ty::AliasTerm<'tcx>) {
+    /// Pushes the obligations required for a projection to be WF into `self.out`.
+    fn add_wf_preds_for_projection_term(&mut self, data: ty::AliasTerm<'tcx>) {


this is a weird method lmao, it has a single call site and isnt used by half the places in this module which add wf predicates for projection terms (e.g. handling of projection types in visit_ty)

kinda makes me wonder if we should inline it into its one caller, but that is orthogonal to your PR

View changes since the review

yeaah. add_wf_preds_for_projection_args also has only a single callsite, and it's this method, so the two could probably be combined into a single method.

at least the method is no longer mis-named, before my rename, add_wf_preds_for_alias_term called add_wf_preds_for_projection_args which is like, ???

the caller (clause_obligations) is currently cute tho, just invokes out to helper methods for each case of the match, so keeping this a separate method is cute IMO :3

BoxyUwU · 2026-06-11T00:27:04Z

@rustbot author

rustbot · 2026-06-11T00:27:10Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

khyperia · 2026-06-11T07:46:30Z

@rustbot ready

I added a FIXME for my long-term hot take around adding impls for Alias<XyzAliasTermKind>, totally ok to say "no don't put that in the codebase, only have that be issue instead" - will file an issue, probably self-assigned, once this PR is ready/merged.

One ambient thought I had for expect_projection_ty_def_id is that the caller should kinda be Alias<I::TraitAssocTyId>. However, outside rustc_next_trait_solver, that's just Alias<DefId> and we lose type safety, being able to convert between all the other Alias<DefId> types. Kind of unfortunate.

Shourya742

Why do we want to be explicit about Aliasterm variants when retrieving their defIds? Earlier, these methods relied on the caller to pass the right kind of Aliasterm. Is the intent now to make that requirement explicit and fail if an unexpected variant is passed?

PS: Super awesome PR, really easy to follow, helping me understand better. These are some newbie questions..

View changes since this review

Shourya742 · 2026-06-11T07:36:01Z

+            let def_id = match kind {
+                AliasTermKind::ProjectionTy { def_id } => def_id.into(),
+                AliasTermKind::InherentTy { def_id } => def_id.into(),
+                AliasTermKind::OpaqueTy { def_id } => def_id.into(),
+                AliasTermKind::FreeTy { def_id } => def_id.into(),
+                AliasTermKind::AnonConst { def_id } => def_id.into(),
+                AliasTermKind::ProjectionConst { def_id } => def_id.into(),
+                AliasTermKind::FreeConst { def_id } => def_id.into(),
+                AliasTermKind::InherentConst { def_id } => def_id.into(),
+            };


Would it make sense to keep an abstraction for cases like this? Since all variants are handled uniformly here.

The point of this change is to allow non-def_id containing variants to be easily added. In this case, a new non-def_id containing variant will presumably have its own version of debug_assert_args_compatible that will be implemented here (or perhaps debug_assert_args_compatible will take a AliasTermKind) - i.e. the explicit match here forces you to look at this piece of code when adding a variant. So I agree with you that this looks very silly right now, because this PR doesn't actually have any use cases for non-def_id variants, but, yeah.

Ah!! makes sense. Thanks.

Shourya742 · 2026-06-11T07:41:39Z

+        alias_term: ty::AliasTerm<'tcx>,
+        alias_def_id: DefId,


Shouldn't we get the alias_def_if from alias_term, and match explicitly like other places?

woop, thank you! update pushed

khyperia · 2026-06-11T08:12:18Z

Why do we want to be explicit about Aliasterm variants when retrieving their defIds? Earlier, these methods relied on the caller to pass the right kind of Aliasterm. Is the intent now to make that requirement explicit and fail if an unexpected variant is passed?

Ah, whoops, yeah, saw your file comment before I saw this review comment. Yeah, the point is to add variants to AliasTermKind/AliasTyKind/AliasConstKind that do not contain a DefId, so a blanket def_id() method will no longer be possible. For example (the following text is copied from here):

merge GCE ConstKind::Expr into UnevaluatedConst (perhaps soon called AliasConst), as GCE ExprKind sometimes doesn't have DefId AFAIK (tracked by either [Work Item]: merge ConstKind::Expr into ConstKind::Unevaluated project-const-generics#82 or [Goal]: unify representations of ConstKind::Unevaluated and TyKind::Alias and allow non defid/subst pairs project-const-generics#98 idk)
add AliasConstKind::Literal [Work Item]: AliasConstKind::Literal project-const-generics#84
add AliasTy/ConstKind::Unresolved [Work Item]: resolve IATs during trait solving not lowering project-const-generics#88

Shourya742 · 2026-06-11T08:42:45Z

Ah, whoops, yeah, saw your file comment before I saw this review comment. Yeah, the point is to add variants to AliasTermKind/AliasTyKind/AliasConstKind that do not contain a DefId, so a blanket def_id() method will no longer be possible.

Thanks for explaining. Much clear now!!!

rustbot · 2026-06-11T11:31:23Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

khyperia · 2026-06-11T11:49:11Z

sorry for all the force pushes, bit of a headache to rebase this

rust-log-analyzer · 2026-06-11T11:58:28Z

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)

rustbot assigned BoxyUwU Jun 9, 2026

This was referenced Jun 10, 2026

[Work Item]: Rename UnevaluatedConst to AliasConst rust-lang/project-const-generics#115

Open

Refactor AliasTy & AliasTerm to use Alias #156538

Open

BoxyUwU reviewed Jun 10, 2026

View reviewed changes

rustbot 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 11, 2026

khyperia force-pushed the AliasTerm-def_id branch from c893b93 to acdf4ec Compare June 11, 2026 07:36

rustbot 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 Jun 11, 2026

Shourya742 reviewed Jun 11, 2026

View reviewed changes

khyperia force-pushed the AliasTerm-def_id branch from acdf4ec to dc0b517 Compare June 11, 2026 08:13

Shourya742 approved these changes Jun 11, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

khyperia force-pushed the AliasTerm-def_id branch 2 times, most recently from cea948b to 5faf7ac Compare June 11, 2026 11:31

remove AliasTerm::def_id()

a93777a

This comment has been minimized.

Sign in to view

khyperia force-pushed the AliasTerm-def_id branch from 5faf7ac to a93777a Compare June 11, 2026 11:48

Kivooeo closed this Jun 11, 2026

Kivooeo reopened this Jun 11, 2026

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2026

Uh oh!

Conversation

khyperia commented Jun 9, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 9, 2026

Uh oh!

rustbot commented Jun 9, 2026

Uh oh!

BoxyUwU Jun 10, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BoxyUwU Jun 10, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BoxyUwU Jun 10, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BoxyUwU Jun 10, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BoxyUwU commented Jun 11, 2026

Uh oh!

rustbot commented Jun 11, 2026

Uh oh!

khyperia commented Jun 11, 2026

Uh oh!

Shourya742 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khyperia commented Jun 11, 2026

Uh oh!

Shourya742 commented Jun 11, 2026

Uh oh!

This comment has been minimized.

rustbot commented Jun 11, 2026

Uh oh!

This comment has been minimized.

khyperia commented Jun 11, 2026

Uh oh!

rust-log-analyzer commented Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

khyperia commented Jun 9, 2026 •

edited by rustbot

Loading

BoxyUwU Jun 10, 2026 •

edited by rustbot

Loading

BoxyUwU Jun 10, 2026 •

edited by rustbot

Loading

BoxyUwU Jun 10, 2026 •

edited by rustbot

Loading

BoxyUwU Jun 10, 2026 •

edited by rustbot

Loading

Shourya742 left a comment •

edited by rustbot

Loading