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
rust-cloud-vms
Bot
force-pushed
the
2026-07-01-use-unnormalized
branch
from
de59e89 to
95ac622
Compare
| let sig = self_ty.fn_sig(tcx); | ||
| let sig = self_ty.unnormalized_fn_sig(tcx); | ||
| let output_ty = sig.map(|sig| self.infcx.enter_forall_and_leak_universe(sig.output())); | ||
| let sig = sig.skip_normalization(); |
There was a problem hiding this comment.
I don't particularly understand this skip_normalization. Is closure_trait_ref_and_return_type ok with taking an unnormalized type? Should it take Unnormalized<>? Is there something weird/interesting going on here? If so, I think adding a comment explaining why skip_normalization is OK/desired here would be nice. Or, perhaps this should be skip_norm_wip.
| let Ok(sig) = self | ||
| .deeply_normalize(ty::Unnormalized::new_wip(unnormalized_sig), term_location) | ||
| else { | ||
| return; |
There was a problem hiding this comment.
Nitpicking: It would be nice to make it explicit in code that the Err has an ErrorGuaranteed here - maybe like elsewhere where lcnr recommended let _: ErrorGuaranteed = ...;. Idk though, maybe this is a relatively common pattern in the compiler to not explicitly write out that it's a Err(ErrorGuaranteed).
| pub fn instantiate_binder_with_fresh_vars_unnormalized<T>( | ||
| &self, | ||
| span: Span, | ||
| lbrct: BoundRegionConversionTime, | ||
| value: ty::Binder<'tcx, T>, | ||
| ) -> ty::Unnormalized<'tcx, T> | ||
| where | ||
| T: TypeFoldable<TyCtxt<'tcx>> + Copy, | ||
| { | ||
| ty::Unnormalized::new(self.instantiate_binder_with_fresh_vars(span, lbrct, value)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Elsewhere (in compiler/rustc_type_ir/src/ty_kind/closure.rs), you renamed the original function, made it return Unnormalized, and then added a new function with the original name that calls the original and does a skip-norm. IMO that's the better pattern and what should be done here, but it should probably at least be consistent :3
|
|
||
| /// Extracts the signature from the closure. | ||
| pub fn sig(self) -> ty::Binder<I, ty::FnSig<I>> { | ||
| self.unnormalized_sig().skip_normalization() |
There was a problem hiding this comment.
This feels like a, "we've left the original function untouched to keep the diff manageable, but in theory, all callers should be using the unnormalized version" situation. In that case, this should probably be .skip_norm_wip() (same with the others with a similar pattern you touched)
Ideally, all callers do use the unnormalized version and have an immediate .skip_norm_wip() on the result, but that might be a lot of code churn and could probably be done in a followup (just be sure that doing so is tracked somewhere~)
| ty | ||
| } | ||
| Err(_) => ty, | ||
| Err(_) => ty.skip_normalization(), |
There was a problem hiding this comment.
I don't particularly understand this skip_normalization either, but with the FIXME, I'm not going to look too hard into trying to understand it. Perhaps this should be a skip_norm_wip though.
Oh, it's an ErrorGuaranteed. Unfortunate that it's difficult to see these things in code review. Hmm. Still not sure about this being a skip_normalization, dunno how that impacts further error reporting and whatnot. Maybe propagating and handling it in the caller is better, maybe it isn't, I'm not sure.
5 participants
View all comments
part of #155345
r? @lcnr