Skip to content

Unnormalized migration: assert_fully_normalized and struct_tail#156224

Open
khyperia wants to merge 2 commits intorust-lang:mainfrom
khyperia:unnormalized-migration
Open

Unnormalized migration: assert_fully_normalized and struct_tail#156224
khyperia wants to merge 2 commits intorust-lang:mainfrom
khyperia:unnormalized-migration

Conversation

@khyperia
Copy link
Copy Markdown
Contributor

@khyperia khyperia commented May 6, 2026

tracking issue: #155345 (first checkbox, and partial second checkbox, of that issue)

I'm going a bit slower than expected (less free time than I'd hope, lots of GCA work that I'm doing instead), and figured I'd just submit what I have now rather than building up a big batch of changes. Slow and steady!

r? @lcnr

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 6, 2026

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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 May 6, 2026
@rust-log-analyzer

This comment has been minimized.

@khyperia khyperia force-pushed the unnormalized-migration branch from 906e38e to d618d7a Compare May 6, 2026 07:35
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 6, 2026

The GCC codegen subtree was changed

cc @antoyo, @GuillaumeGomez

let mut normalize = |ty| self.normalize(ty, location);
let tail = tcx.struct_tail_raw(ty, &cause, &mut normalize, || {});
normalize(tail)
normalize(Unnormalized::new_wip(tail))
Copy link
Copy Markdown
Contributor

@lcnr lcnr May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that normalize call can be removed or struct_tail_raw has to return Unnormalized 🤔

View changes since the review

@@ -461,7 +461,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
let projected_ty = curr_projected_ty.projection_ty_core(
Copy link
Copy Markdown
Contributor

@lcnr lcnr May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

projection_ty_core should also have its normalization closure take Unnormalized<Ty>, unless u want to do that in a later PR

View changes since the review

Comment on lines 472 to +475
// Need to renormalize `a` as typecheck may have failed to normalize
// higher-ranked aliases if normalization was ambiguous due to inference.
let a = self.normalize(a, locations);
let ty = self.normalize(curr_projected_ty.ty, locations);
let a = self.normalize(ty::Unnormalized::new_wip(a), locations);
let ty = self.normalize(ty::Unnormalized::new_wip(curr_projected_ty.ty), locations);
Copy link
Copy Markdown
Contributor

@lcnr lcnr May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Need to renormalize `a` as typecheck may have failed to normalize
// higher-ranked aliases if normalization was ambiguous due to inference.
let a = self.normalize(a, locations);
let ty = self.normalize(curr_projected_ty.ty, locations);
let a = self.normalize(ty::Unnormalized::new_wip(a), locations);
let ty = self.normalize(ty::Unnormalized::new_wip(curr_projected_ty.ty), locations);
// Need to renormalize `a` in the old solver as typecheck may have failed
// to normalize higher-ranked aliases if normalization was ambiguous due
// to inference.
//
// We properly normalize higher-ranked aliases during writeback with the
// new solver, so this is no longer necessary.
let mut a = a;
let mut ty = curr_projected_ty.ty;
if !self.infcx.next_trait_solver() {
a = self.normalize(ty::Unnormalized::new_wip(a), locations);
ty = self.normalize(ty::Unnormalized::new_wip(curr_projected_ty.ty), locations);
}

View changes since the review

layout.ty,
&ObligationCause::dummy(),
|ty| ty,
|ty| ty.skip_normalization(),
Copy link
Copy Markdown
Contributor

@lcnr lcnr May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are we doing here 😭 that'a broken, please add a "FIXME(#155345): Missing normalization call" here

View changes since the review

Comment on lines 218 to 222
// Even if `ty` is normalized, the search for the unsized tail will project
// to fields, which can yield non-normalized types. So we need to provide a
// normalization function.
let normalize = |ty| {
self.tcx.normalize_erasing_regions(self.typing_env, Unnormalized::new_wip(ty))
};
let normalize = |ty| self.tcx.normalize_erasing_regions(self.typing_env, ty);
ty.ptr_metadata_ty(*self.tcx, normalize)
Copy link
Copy Markdown
Contributor

@lcnr lcnr May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove that comment and maybe inline the comment. I don't think that comment is useful as this holds for all uses of ptr_metadata_ty, so it should be on that function if anywhere

View changes since the review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"inline the closure" i mean

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the comment because I can never remember when one has to normalize what and we had a bunch of bugs due to that.

@@ -81,7 +81,7 @@ impl<'a, 'tcx> Expectation<'tcx> {
let cause = ObligationCause::misc(span, fcx.body_id);

// FIXME: This is not right, even in the old solver...
Copy link
Copy Markdown
Contributor

@lcnr lcnr May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// FIXME: This is not right, even in the old solver...
// FIXME(#155345): Missing normalization call

View changes since the review

tcx.types.unit
} else {
match pointee_ty.ptr_metadata_ty_or_tail(tcx, |x| x) {
match pointee_ty.ptr_metadata_ty_or_tail(tcx, |x| x.skip_normalization()) {
Copy link
Copy Markdown
Contributor

@lcnr lcnr May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aaaaaaaaaaaaaaaa xd

Suggested change
match pointee_ty.ptr_metadata_ty_or_tail(tcx, |x| x.skip_normalization()) {
// FIXME(#155345): Missing normalization call
match pointee_ty.ptr_metadata_ty_or_tail(tcx, |x| x.skip_normalization()) {

View changes since the review


ty::Alias(..) => {
let normalized = normalize(ty);
let normalized = normalize(Unnormalized::new(ty));
Copy link
Copy Markdown
Contributor

@lcnr lcnr May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let normalized = normalize(Unnormalized::new(ty));
let normalized = normalize(Unnormalized::new_wip(ty));

we should remove this as all the inputs should already be normalized

View changes since the review

pointee,
&ObligationCause::dummy(),
|ty| ty,
|ty| ty.skip_norm_wip(),
Copy link
Copy Markdown
Contributor

@lcnr lcnr May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that...

why are we not matching on try_normalize_erasing_regions inside of struct_tail_raw

ah well, not necessarily for your PR. would keep that as is for now

View changes since the review

Copy link
Copy Markdown
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few more suggestions, then r=me 😊 thank you for working on this

View changes since this review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants