`DeeplyNormalize` and `normalize_with_depth_to` take `Unnormalized<T>` as input by Shourya742 · Pull Request #158668 · rust-lang/rust

Shourya742 · 2026-07-01T16:51:43Z

rustbot · 2026-07-01T16:51:46Z

changes to the core type system

lcnr · 2026-07-02T08:25:48Z

        self.constraints.type_tests.push(type_test);
    }

    fn normalize_and_add_type_outlives_constraints(


Suggested change

// FIXME(trait-system-refactor-initiative#260): This function should be

// removed.

fn normalize_and_add_type_outlives_constraints(

View changes since the review

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

still relevant

lcnr · 2026-07-02T08:29:07Z

+            }) = self.infcx.fully_perform(
+                DeeplyNormalize { value: ty::Unnormalized::new_wip(outlives) },
+                span,
+            )
            else {
                self.infcx.dcx().delayed_bug(format!("could not normalize {outlives:?}"));
                return;


can you instead change this to a match and in the error branch do let _: ErrorGuarateed = guar; instead of a delayed_bug

View changes since the review

lcnr · 2026-07-02T08:33:53Z

                    |ty::ParamEnvAnd { param_env, value }| ty::ParamEnvAnd {
                        param_env,
-                        value: Normalize { value: value.value },
+                        value: Normalize { value: value.value.skip_normalization() },


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?

View changes since the review

Collapsed them here: d8f43f7

lcnr · 2026-07-02T08:37:57Z

    push_const_arg_has_type_obligation(
        tcx,
        obligations,
        &cause,
        depth + 1,
        param_env,
        term,
        def_id,
        args,
    );


and move this below the call to normalizes_with_depth_to? cc @BoxyUwU @khyperia for whether ConstArgHasType expects (or allows) the const to be normalized

View changes since the review

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

(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; ...

moving some questions here to https://rust-lang.zulipchat.com/#narrow/channel/260443-project-const-generics/topic/.60ConstArgHasType.60.20ICE.20guard/with/608022857

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.

lcnr

can you go through the calls to new_wip you've added whether we have a call to skip_norm_wip earlier in that function and whether you can restructure the code to remove both of them?

View changes since this review

lcnr · 2026-07-06T10:43:09Z

Handing over reviews in this area to @khyperia as I don't have a lot of capacity and want the reviews to actually be responsive :> I have already discussed this work with her a bunch e.g. for #156224, and she is willing to take over for me where possible 😁

r? khyperia

khyperia · 2026-07-07T07:03:51Z

-        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();


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.

View changes since the review

I changed it to skip_norm_wip, though underneath, both skip_normalization and skip_norm_wip are the same. We might get rid of both of them by the end of the migration.

I do understand the concern around getting an unnormalized value and then skipping normalization, which, tbh, doesn't really make sense. I will open a follow-up to harden this and make sure we aren't ambiguous about it.

This PR only focuses on a very small subset of these ambiguities. I will try to open a larger PR to address them more broadly, so we don't all get confused (though I am still confused 😅).

I changed it to skip_norm_wip, though underneath, both skip_normalization and skip_norm_wip are the same. We might get rid of both of them by the end of the migration.

skip_normalization should only be used in places where we want to continue doing so even by the end of the migration, either because the place wants to directly use the unnormalized value (e.g. crate_variances is computed using unnormalized types), or because we're inside of a normalization routine which is the only way to properly discard these wrappers

oh! so they do have different semantic meanings. Thanks for info, I was considering them the same.

khyperia · 2026-07-07T07:06:32Z

+                let Ok(sig) = self
+                    .deeply_normalize(ty::Unnormalized::new_wip(unnormalized_sig), term_location)
+                else {
+                    return;


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).

View changes since the review

khyperia · 2026-07-07T07:10:07Z

+    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))
+    }
+


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

View changes since the review

khyperia · 2026-07-07T07:11:28Z


    /// Extracts the signature from the closure.
    pub fn sig(self) -> ty::Binder<I, ty::FnSig<I>> {
+        self.unnormalized_sig().skip_normalization()


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~)

View changes since the review

Yup, thats the plan. In my next PR, I hope to address all of these.

khyperia · 2026-07-07T07:21:14Z

                ty
            }
-            Err(_) => ty,
+            Err(_) => ty.skip_normalization(),


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.

View changes since the review

Changed to skip_norm_wip, will get rid of it soon.

…malized<T> as input 2. add unnormalized producing API's 3. merge Deeply Normalize and Normalize 4. make normalize_with_category accept unnormalized

lcnr · 2026-07-15T08:20:34Z

        self.constraints.type_tests.push(type_test);
    }

    fn normalize_and_add_type_outlives_constraints(


still relevant

lcnr · 2026-07-15T08:29:11Z

-    // that corresponds to your use case, consider whether or not you should
-    // use [`InferCtxt::enter_forall`] instead.
-    pub fn instantiate_binder_with_fresh_vars<T>(
+    pub fn instantiate_binder_with_fresh_vars_unnormalized<T>(


this one is interesting 🤔 cc #156453 https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/Eager.20normalization.2C.20ahoy.21/near/593190304 and https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/we.20didn.27t.20have.20enough.20normalization.20threads.20yet/with/599149011 see the zulip discussion for more details.

Whether this requires a renormalization is unclear and in most places it does not. Can you remove this change from this PR and instead maybe do this in a followup, if at all.

View changes since the review

lcnr · 2026-07-15T08:29:58Z


-    let term: Term<'tcx> = if alias_term.kind.is_type() {
-        tcx.type_of(def_id).instantiate(tcx, args).skip_norm_wip().into()
+    let term: ty::Unnormalized<'tcx, Term<'tcx>> = if alias_term.kind.is_type() {


Suggested change

let term: ty::Unnormalized<'tcx, Term<'tcx>> = if alias_term.kind.is_type() {

let term = if alias_term.kind.is_type() {

why this type annotation

View changes since the review

lcnr · 2026-07-15T08:30:43Z

+    let term = if term.has_aliases() {
+        normalize_with_depth_to(selcx, param_env, cause.clone(), depth + 1, term, obligations)
    } else {
-        tcx.const_of_item(def_id).instantiate(tcx, args).skip_norm_wip().into()
+        term.skip_normalization()
    };


i feel like we should a fast path for this in normalize_with_depth_to already

if we remove the term.has_aliases() method call we also don't need the explicit type annotation anymore

View changes since the review

lcnr · 2026-07-15T08:32:27Z

                                // and confirm these obligations once again during confirmation
-                                normalize_with_depth(
+                                let mut obligations = PredicateObligations::new();
+                                normalize_with_depth_to(


why change this method call?

View changes since the review

lcnr · 2026-07-15T08:34:05Z

        )
    };
-    let coroutine_sig = args.as_coroutine().sig();
+    let coroutine_sig = args.as_coroutine().unnormalized_sig();


actually, the coroutine signature should always be normalized and if it isn't that's due to higher-ranked alias handling jank (which I'd ignore and will be fixed by higher-ranked infer vars long-term) or old solver bugs.

Can you remove fn unnormalized_sig from coroutines and do a ty::Unnormalized::new_wip here

View changes since the review

lcnr · 2026-07-15T08:34:51Z

    let self_ty = selcx.infcx.shallow_resolve(obligation.predicate.self_ty());
    let closure_sig = match *self_ty.kind() {
-        ty::Closure(_, args) => args.as_closure().sig(),
+        ty::Closure(_, args) => args.as_closure().unnormalized_sig(),


same for closures

View changes since the review

lcnr · 2026-07-15T08:35:37Z

-                // Otherwise, defer to `AsyncFnKindHelper::Upvars` to delay
-                // the projection, like the `AsyncFn*` traits do.
-                let output_ty = if let Some(_) = kind_ty.to_opt_closure_kind()
+            args.unnormalized_coroutine_closure_sig().map(|sig| {


and same here :3

View changes since the review

lcnr · 2026-07-15T08:38:19Z

+    pub fn unnormalized_sig(self) -> Unnormalized<I, ty::Binder<I, ty::FnSig<I>>> {
        match self.sig_as_fn_ptr_ty().kind() {
-            ty::FnPtr(sig_tys, hdr) => sig_tys.with(hdr),
+            ty::FnPtr(sig_tys, hdr) => Unnormalized::new(sig_tys.with(hdr)),


similar to skip_normalization, Unnormalized::new should also only be used at the boundaries as everywhere else, unnormalized things are already wrapped with Unnormalized

View changes since the review

lcnr · 2026-07-15T08:39:38Z

+    fn fold_with<F: TypeFolder<I>>(self, folder: &mut F) -> Self {
+        Unnormalized::new(self.value.fold_with(folder))
+    }
+}


what do you need this for? It's not strictly wrong, but I feel like it's often wrong to fold unnormalized types without first normalizing them/the TypeFoldable impl implicitly walks into the unnormalized type which feels easier to get wrong by accident

View changes since the review

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 1, 2026

rustbot assigned lcnr Jul 1, 2026

This comment has been minimized.

Sign in to view

rust-cloud-vms Bot force-pushed the 2026-07-01-use-unnormalized branch 2 times, most recently from 2f2feff to 8977db3 Compare July 2, 2026 03:12