Remove strict invariant node_type on hir_type during ty privacy visit

[Shourya742]

closes: #157772

r? @BoxyUwU

[Shourya742]

I am not fully sure this change is right, but this is how I am thinking about it. This used node_type before, so visit_ty was assuming every hir_ty would already have a type entry in TypeckResults. But the comment in writeback::visit_ty makes it sound like that is not always true on some error paths, since writeback may just not write a final type entry for that hir_ty. So based on that, I changed this visit_ty to handle missing type entries and just keep walking the type tree.

I hope the thought process is sane. And I am not breaking any invariant (like I am not seeing any test fail :P)

View changes since the review

[cjgillot]

Should we instead fix writeback to ensure we always have a type recorded, even on error paths?

[Shourya742]

Not sure, seems like its an intended behavior as mentioned here in writeback visit_ty:

rust/compiler/rustc_hir_typeck/src/writeback.rs

Lines 350 to 351 in 124feda

	// If there are type checking errors, Type privacy pass will stop,
	// so we may not get the type from hir_id, see #104513

[Shourya742]

We had an issue during lowering itself, reverted this change, thanks.

[BoxyUwU]

I'm not entirely sure what's going on with this ICE, so I have a few questions hopefully you can look into/answer for me 😅

1. what's the Ty we're visiting that hasn't got a node_type entry? Is it the T in T::ASSOC?
2. If this code doesn't error where would we normally insert the node_type at and why doesn't that happen when the code errors
3. Are there similar ICEIng cases not using the tuple syntax, e.g. when using struct expressions or associated constants. If not why not :3

View changes since this review

[BoxyUwU]

delaying a bug if node_type_opt returns None feels "safer" to me than implicitly doing nothing, we only expect to not have a type here if there were errors right?

[BoxyUwU]

@rustbot author

[Shourya742]

I'm not entirely sure what's going on with this ICE, so I have a few questions hopefully you can look into/answer for me 😅

1. what's the Ty we're visiting that hasn't got a node_type entry? Is it the T in T::ASSOC?
2. If this code doesn't error where would we normally insert the node_type at and why doesn't that happen when the code errors
3. Are there similar ICEIng cases not using the tuple syntax, e.g. when using struct expressions or associated constants. If not why not :3

1. Yes, the missing node_type is for the T in T::ASSOC.
2. Because we didn't lowered the caller tuple elements, so we never assigned an node_type to those hir_nodes.
3. I didn't see ICE for struct expressions or associated constants. This bug is specific to tuple const args.

After looking deeply and many debugs logs, I think I found the real issue, which is in lower_const_arg_tup:

The current code uses zip, in lower_const_arg_tup:

let exprs = exprs
    .iter()
    .zip(tys.iter())
    .map(|(expr, ty)| self.lower_const_arg(expr, ty))
    .collect::<Vec<_>>();

code snippet:

fn lower_const_arg_tup(
        &self,
        exprs: &'tcx [&'tcx hir::ConstArg<'tcx>],
        ty: Ty<'tcx>,
        span: Span,
    ) -> Const<'tcx> {
        let tcx = self.tcx();

        let tys = match ty.kind() {
            ty::Tuple(tys) => tys,
            ty::Error(e) => return Const::new_error(tcx, *e),
            _ => {
                let e = tcx.dcx().span_err(span, format!("expected `{}`, found const tuple", ty));
                return Const::new_error(tcx, e);
            }
        };

        let exprs = exprs
            .iter()
            .zip(tys.iter())
            .map(|(expr, ty)| self.lower_const_arg(expr, ty))
            .collect::<Vec<_>>();

        let valtree = ty::ValTree::from_branches(tcx, exprs);
        ty::Const::new_value(tcx, valtree, ty)
    }

This is the problem. zip stops when either side ends. So if the user gives too many tuple elements, the extra elements are silently ignored.

For example, this can compile even though it should not:

#![feature(min_generic_const_args, adt_const_params)]

fn takes_tuple<const N: (u32,)>() {}

fn generic_caller<const N: u32>() {
    takes_tuple::<{ (1, 1) }>;
}

fn main() {}

The ICE happens in the same way. For this code:

fn takes_tuple<const N: ()>() {}

fn generic_caller<T, const N: u32>() {
    takes_tuple::<{ (N, T::ASSOC) }>;
}

the expected tuple type is (), which has zero elements. So zip lowers zero elements. That means T::ASSOC is skipped. Later, privacy still walks the HIR and sees the T, but there is no node_type for it, so we ICE.

I added a check the tuple lengths before using zip. If the number of provided tuple elements does not match the expected tuple type, we emit an error and return an error const instead of silently ignoring elements.

[rustbot]

HIR ty lowering was modified

cc @fmease

[BoxyUwU]

ah sick yeah that makes a lot of sense, thanks for looking into that for me :)

@bors r+

[rust-bors]

📌 Commit cd8c4d0 has been approved by BoxyUwU

It is now in the queue for this repository.

[Shourya742]

ah sick yeah that makes a lot of sense, thanks for looking into that for me :)

@bors r+

Ah!! Thanks for asking those super awesome questions, really helped me to think deeper and find actual bug.

[JonathanBrouwer]

@rust-timer build c8be92a
Perf run for #158342

[rust-timer]

Finished benchmarking commit (c8be92a): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.0%	[0.8%, 1.1%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 5.8%, secondary -9.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	5.8%	[5.8%, 5.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-9.8%	[-9.8%, -9.8%]	1
All ❌✅ (primary)	5.8%	[5.8%, 5.8%]	1

Cycles

Results (secondary 2.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 503.101s -> 505.582s (0.49%)
Artifact size: 353.51 MiB -> 353.44 MiB (-0.02%)