DeeplyNormalize and normalize_with_depth_to take Unnormalized<T> as input#158668
DeeplyNormalize and normalize_with_depth_to take Unnormalized<T> as input#158668Shourya742 wants to merge 4 commits into
DeeplyNormalize and normalize_with_depth_to take Unnormalized<T> as input#158668Conversation
|
changes to the core type system cc @lcnr |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
rust-cloud-vms
Bot
force-pushed
the
2026-07-01-use-unnormalized
branch
2 times, most recently
from
2f2feff to
8977db3
Compare
| ConstraintCategory::Boring, | ||
| self.infcx.param_env.and(type_op::normalize::DeeplyNormalize { value }), | ||
| ); | ||
| result.unwrap_or(value) |
There was a problem hiding this comment.
can you instead change the signature of this function to propagate the ErrorGuaranteed and handle it in its one call site?
| self.constraints.type_tests.push(type_test); | ||
| } | ||
|
|
||
| fn normalize_and_add_type_outlives_constraints( |
There was a problem hiding this comment.
| // FIXME(trait-system-refactor-initiative#260): This function should be | |
| // removed. | |
| fn normalize_and_add_type_outlives_constraints( |
There was a problem hiding this comment.
I feel like "needs to" has a slightly bad vibe and I care enough that I would like you to change this to "should be"
It's hard to explain. To me "needs to" has a stronger expectation that this is something that somebody has to do while "should" is a lot more things would be better if this happens.
In a sense, needs to feels like it introduces an obligation for someone to resolve this in the near future while should is mainly just a note that this is something worth dealing with
| }) = self.infcx.fully_perform( | ||
| DeeplyNormalize { value: ty::Unnormalized::new_wip(outlives) }, | ||
| span, | ||
| ) | ||
| else { | ||
| self.infcx.dcx().delayed_bug(format!("could not normalize {outlives:?}")); | ||
| return; |
There was a problem hiding this comment.
can you instead change this to a match and in the error branch do let _: ErrorGuarateed = guar; instead of a delayed_bug
| |ty::ParamEnvAnd { param_env, value }| ty::ParamEnvAnd { | ||
| param_env, | ||
| value: Normalize { value: value.value }, | ||
| value: Normalize { value: value.value.skip_normalization() }, |
There was a problem hiding this comment.
idk how much effort, so could leave for future work, but Normalize should also take Unnormalized. Or actually, we should merge NormalizeandDeeplyNormalize`. They are now the same thing afaict?
| push_const_arg_has_type_obligation( | ||
| tcx, | ||
| obligations, | ||
| &cause, | ||
| depth + 1, | ||
| param_env, | ||
| term, | ||
| def_id, | ||
| args, | ||
| ); |
There was a problem hiding this comment.
afaik this call to push_const_arg_has_type_obligation is checking "hey, make sure unwrapping this single alias layer didn't change the type":
type const FurtherAlias: bool = 5_usize;
type const Alias: usize = FurtherAlias;
... [(); Alias] ...when on a projection goal for Alias in [(); Alias], this should check that resolving Alias to FurtherAlias (which is what const_of_item does) has the resulting value still retain the original type of Alias, which is usize.
In other words, it checks :
typeof(const_of_item(Alias)) == typeof(Alias)=>typeof(FurtherAlias) == typeof(Alias)=>bool == usize, ❌ compiler error.
If we eager norm before doing that, we resolve FurtherAlias to 5_usize, and then check "hey, does 5_usize still have the original type of Alias, which is usize", we go "oh yeah yep we gucci", missing the fact that we hit a bool in the middle.
In other words,
typeof(normalize(const_of_item(Alias))) == typeof(Alias)=>typeof(5_usize) == typeof(Alias)=>usize == usize✔️ we good (we should not be good!!).
see also, this PR #154853
That's my naiive understanding based on a quick reading of the code though, I would defer to boxy here
There was a problem hiding this comment.
(type aliases don't have this issue, because they're all just kind *, so resolving a type alias never changes its kind. As a hypothetical, I think we'd have to do the same thing for types if we ever supported something like this:)
type List: * -> * = Vec;
// ... let _: List<u32>; ...
type BadAlias: * = List;
// ... let _: BadAlias; ...There was a problem hiding this comment.
There was a problem hiding this comment.
after discussion, seems it's fine that we only check push_const_arg_has_type_obligation on the fully normalized const (i.e. it doesn't really matter either way).
From a type theory perspective, this check is unnecessary, and only exists to prevent CTFE from seeing non-wfchecked things. So, if the type of the value of the const happens to be "accidentally correct", that's fine, CTFE won't explode. The fact it's not WF will report an error in regular wfcheck.
rust-cloud-vms
Bot
force-pushed
the
2026-07-01-use-unnormalized
branch
from
8977db3 to
d8f43f7
Compare
This comment has been minimized.
This comment has been minimized.
lcnr
changed the title
DeeplyNormalize and normalize_with_depth_to takes Unnormalized<T> as input
lcnr
changed the title
DeeplyNormalize and normalize_with_depth_to takes Unnormalized<T> as inputDeeplyNormalize and normalize_with_depth_to take Unnormalized<T> as input
| param_env.and(type_op::normalize::Normalize { value }), | ||
| ); | ||
| result.unwrap_or(value) | ||
| result.unwrap_or(value.skip_normalization()) |
There was a problem hiding this comment.
This is also dubious and something we shouldn't support :>
can keep it as wip for now
| result.unwrap_or(value.skip_normalization()) | |
| result.unwrap_or(value.skip_norm_wip()) |
5 participants
part of #155345
r? @lcnr