generic_const_args: allow paths to non type consts

[khyperia]

View all comments

tracking issue: #151972

Non type consts should be usable in the type system in feature(generic_const_args). These are directly plugged into the constant evaluator, unlike type consts, which are attempted to be reasoned about by the type system.

Inherent associated constants are not supported at this time, due to complications around how generic arguments are represented for them (it's currently a mess). The mess is being cleaned up (e.g. #154758), so instead of trying to hack support in before the refactoring is done, let's just wait to be able to implement it more cleanly.

r? @BoxyUwU

[rustbot]

changes to the core type system

cc @lcnr

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in engine.rs, potentially modifying the public API of ObligationCtxt.

cc @lcnr

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

HIR ty lowering was modified

cc @fmease

changes to the core type system

cc @lcnr

[BoxyUwU]

cool ✨ two big picture things:

1. you've changed evaluate_const to take an unevaluated const instead of a ty::Const. iirc we talked about doing this to make IACs work but since we've dropped support for them can those changes be reverted?
2. if it would be kinda chill for you to do can you make the change of adding fields to AliasTermKind's variants be in a separate commit so it's easier to review just the "meaningful" stuff? If it's non-trivial/difficult it's fine to keep it as is 🤔

View changes since this review

[khyperia]

• you've changed evaluate_const to take an unevaluated const instead of a ty::Const. iirc we talked about doing this to make IACs work but since we've dropped support for them can those changes be reverted?

yeah I guess, it just makes me sad to revert, the change seems nice :c :P - I've stashed the work so can maybe done sometime in the future

• if it would be kinda chill for you to do can you make the change of adding fields to AliasTermKind's variants be in a separate commit so it's easier to review just the "meaningful" stuff? If it's non-trivial/difficult it's fine to keep it as is 🤔

done!

edit: ack, the split wasn't fully clean, there's a swap from is_type to pattern matching in the first commit that should be in the second commit, in normalize.rs, sowwy

edit: ok, pushed, moving the change to the other commit

cc @WaffleLapkin when AliasTermKind has been updated to store DefIds like you've done for AliasTyKind this can be removed and we can just call tcx.is_type_const(def) everywhere instead

I think the DefId is already available in all the relevant places - I don't think adding a is_type_const field is strictly necessary, I just thought it was cleaner/nicer, especially with the exhaustive pattern matching nice things. Totally reasonable for me to use tcx.is_type_const though, let me know what style you'd prefer!

[BoxyUwU]

I'm not super fussy about the is_type_const field I guess. It probably doesn't really change much about how much effort it'll be for one of you or waffle to fix things up between this and #155392.

I think this PR basically just needs some more tests at this point. I'm thinking off the top of my head.

• generic uses of free consts, e.g. FREE<N> so it can't just be immediately evaluated
• equality rules of non-type consts, so for both free and associated consts we should have tests that:
  • ALIAS1 and ALIAS2 are equal if they evalaute to the same value (and aren't if they don't).
  • ALIAS<N> is only equal to itself and not other aliases with the same body

then it should be good to go

View changes since this review

[BoxyUwU]

oh also can you link to the tracking issue for gca in the PR description

[rustbot]

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]

@BoxyUwU this is annoying and gross (see HACK comment), resolve_vars_if_possible is called twice on target_args. I could maybe clean this up in a follow-up PR? Or I could gut/refactor in this PR. I would slightly prefer doing it in a follow-up, but, let me know!

View changes since the review

[BoxyUwU]

we probably should split EvaluateConstErr in two eventually yeah.

Can you move this resolve_vars_if_possible into evaluate_const_and_instantiate_normalizes_to_term. Right now it kind of detracts from understanding the high level logic of how normalization works

[khyperia]

done!

[BoxyUwU]

thx this looks good ✨

View changes since this review