Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking issue for removing unnecessary ty::Const::{normalize,eval} calls from the type system #130704

Open
5 of 7 tasks
compiler-errors opened this issue Sep 22, 2024 · 6 comments
Assignees
Labels
A-const-generics Area: const generics (parameters and arguments) T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@compiler-errors
Copy link
Member

compiler-errors commented Sep 22, 2024

We want to remove calls to ty::Const::{normalize,eval,eval_to_*} because they won't work correctly with future reformulations of GCE, and because they're unnecessary with non-GCE consts. This issue tracks doing that so I won't forget. Boxy can probably write more motivation here idk

Boxy rationale:

with min_generic_const_args and associated_const_equality featuers normalization of type system constants will be behaving much more like types. They'll return nested goals, access in scope whjere clauses such as T: Trait<ASSOC = 10> in order to normalize instead of simply "evaluating".

This means that the only correct way to normalize a ty::Const will be to use the "normal" normalization routines such as normalize_erasing_regions or infcx/ocx/fcx.normalize. With that in mind all of the eval_x and normalize methods on ty::Const become massive footguns as they are never correct to use.

More:

cc @BoxyUwU

@compiler-errors compiler-errors self-assigned this Sep 22, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 22, 2024
@compiler-errors compiler-errors added A-const-generics Area: const generics (parameters and arguments) T-types Relevant to the types team, which will review and decide on the PR/issue. totally-not-a-tracking-issue and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. totally-not-a-tracking-issue labels Sep 22, 2024
@RalfJung
Copy link
Member

RalfJung commented Sep 28, 2024

and because they're unnecessary with non-GCE consts

Are they? The role of ty::Const::normalize is to bring this constant into the shape ConstKind::Value where possible (so that e.g. it can be turned into a concrete integer without any further interpreter invocations). This is entirely orthogonal to GCE.

@RalfJung
Copy link
Member

As a meta remark here, I think @rust-lang/wg-const-eval should generally be kept in the loop when t-types is fundamentally changing the API of ty::Const, as that API is clearly in the responsibility of both teams/groups. :)

In particular, we did a bunch of work to make that API somewhat consistent with mir::Const, for good reasons. (Type systems consts can be used as MIR values, so every operation on mir::Const has to be supported by ty::Const.)

@BoxyUwU
Copy link
Member

BoxyUwU commented Sep 28, 2024

Nothing is fundamentally changing about being able to normalize type system constants, this has zero impact on const evaluation. This is a refactoring to make ty::Const's API be less error prone when const aliases are involved, not to remove functionality.

@RalfJung
Copy link
Member

RalfJung commented Sep 28, 2024

I didn't say any functionality was being removed. Just APIs are being (re)moved, for what I assume are good reasons. But surely wg-const-eval needs to understand why this is done. You can't say that the ty::Const API is not relevant for wg-const-eval...

@BoxyUwU
Copy link
Member

BoxyUwU commented Sep 28, 2024

I am absolutely saying that in this case it has nothing to do with this group. Nothing has fundamentally changed with this refactoring, it has no impact on const eval whatsoever. This is primarily a focus about how the API should be structured for normalizing type system constants not any of the actual const eval logic.

If we were introducing new constraints on how ctfe must work for const generics or what circumstances ctfe must be able to run in then sure wg-const-eval should be included. But this refactoring does not affect const eval/ctfe in any real way.

@fee1-dead
Copy link
Member

Both of you may be interested in moving extended discussion into a Zulip topic :)

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 21, 2024
Continue to get rid of `ty::Const::{try_}eval*`

This PR mostly does:

* Removes all of the `try_eval_*` and `eval_*` helpers from `ty::Const`, and replace their usages with `try_to_*`.
* Remove `ty::Const::eval`.
* Rename `ty::Const::normalize` to `ty::Const::normalize_internal`. This function is still used in the normalization code itself.
* Fix some weirdness around the `TransmuteFrom` goal.

I'm happy to split it out further; for example, I could probably land the first part which removes the helpers, or the changes to codegen which are more obvious than the changes to tools.

r? BoxyUwU

Part of rust-lang#130704
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Oct 22, 2024
Continue to get rid of `ty::Const::{try_}eval*`

This PR mostly does:

* Removes all of the `try_eval_*` and `eval_*` helpers from `ty::Const`, and replace their usages with `try_to_*`.
* Remove `ty::Const::eval`.
* Rename `ty::Const::normalize` to `ty::Const::normalize_internal`. This function is still used in the normalization code itself.
* Fix some weirdness around the `TransmuteFrom` goal.

I'm happy to split it out further; for example, I could probably land the first part which removes the helpers, or the changes to codegen which are more obvious than the changes to tools.

r? BoxyUwU

Part of rust-lang/rust#130704
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Oct 22, 2024
Continue to get rid of `ty::Const::{try_}eval*`

This PR mostly does:

* Removes all of the `try_eval_*` and `eval_*` helpers from `ty::Const`, and replace their usages with `try_to_*`.
* Remove `ty::Const::eval`.
* Rename `ty::Const::normalize` to `ty::Const::normalize_internal`. This function is still used in the normalization code itself.
* Fix some weirdness around the `TransmuteFrom` goal.

I'm happy to split it out further; for example, I could probably land the first part which removes the helpers, or the changes to codegen which are more obvious than the changes to tools.

r? BoxyUwU

Part of rust-lang/rust#130704
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 12, 2024
…nst_eval, r=compiler-errors

Consolidate type system const evaluation under `traits::evaluate_const`

Part of rust-lang#130704

Fixes rust-lang#128232
Fixes rust-lang#118545

Removes `ty::Const::{normalize_internal, eval_valtree}` and `InferCtxt::(try_)const_eval_resolve`, consolidating the associated logic into `evaluate_const` in `rustc_trait_selection`. This results in an API for `ty::Const` that is free of any normalization/evaluation functions that would be incorrect to use under `min_generic_const_args`/`associated_const_equality`/`generic_const_exprs` or, more generally, that would be incorrect to use in the presence of generic type system constants.

Moving this logic to `rustc_trait_selection` and out of `rustc_middle` is also a pre-requisite for ensuring that we do not evaluate constants whose where clauses do not hold.

From this point it should be relatively simple (hah) to implement more complex normalization of type system constants such as: checking wf'ness before invoking CTFE machinery, or being able to normalize const aliases that still refer to generic parameters.

r? `@compiler-errors`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 12, 2024
Rollup merge of rust-lang#132927 - BoxyUwU:consolidate_type_system_const_eval, r=compiler-errors

Consolidate type system const evaluation under `traits::evaluate_const`

Part of rust-lang#130704

Fixes rust-lang#128232
Fixes rust-lang#118545

Removes `ty::Const::{normalize_internal, eval_valtree}` and `InferCtxt::(try_)const_eval_resolve`, consolidating the associated logic into `evaluate_const` in `rustc_trait_selection`. This results in an API for `ty::Const` that is free of any normalization/evaluation functions that would be incorrect to use under `min_generic_const_args`/`associated_const_equality`/`generic_const_exprs` or, more generally, that would be incorrect to use in the presence of generic type system constants.

Moving this logic to `rustc_trait_selection` and out of `rustc_middle` is also a pre-requisite for ensuring that we do not evaluate constants whose where clauses do not hold.

From this point it should be relatively simple (hah) to implement more complex normalization of type system constants such as: checking wf'ness before invoking CTFE machinery, or being able to normalize const aliases that still refer to generic parameters.

r? `@compiler-errors`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants