Properly check that array length is valid type during built-in unsizing in index

[compiler-errors]

This results in duplicated errors, but this class of errors is not new; in general, we aren't really equipped to detect cases where a WF error due to a field type would be shadowed by the parent struct of that field also not being WF.

This also adds a note for these types of mismatches to make it clear that this is due to an array type.

Fixes #134352

r? boxyuwu

[rustbot]

changes to the core type system

cc @compiler-errors, @lcnr

[BoxyUwU]

We don't check the array is wf because we assume that field types are wf if the struct is wf right? Why do we believe that only arrays will cause issues and not arbitrary other types, because we only expect mir building to interact with builtin types in ways that would be wrong if they were not-wf?

[compiler-errors]

We don't check the array is wf because we assume that field types are wf if the struct is wf right?

No, we definitely do not do this, and in general it's kind of a non-local assumption to make, since the index operator (or a method call, for example) can't really know from where a type originated. If I have some struct like:

struct Bad {
    field: NeedsWf<NotSatisfied>,
};

and I do:

bad.field.method();

I'll get a WF error if field is not well-formed, regardless if this is conceptually implied by the struct being WF. The fac tthat we emit duplicated WF obligations kinda sucks, but seems to me to be a much bigger problem than this diagnostic regression.

Why do we believe that only arrays will cause issues and not arbitrary other types, because we only expect mir building to interact with builtin types in ways that would be wrong if they were not-wf?

This bug is specifically caused by our special casing of array types in hir typeck here in a way that side-steps the wf obligations that we would've otherwise collected by actually applying an impl like Unsize.

To me this feels like we're incompletely applying a built-in impl's where clauses that we really ought to be enforcing, and are just luckily avoiding because we can ensure that they're satisfied elsewhere.

Bad for perf (?)

We're already checking these predicates practically everywhere else -- all impls with const generics and on array types themselves. I could queue a perf run tho.

@bors try @rust-timer queue

[bors]

⌛ Trying commit 7e68422 with merge fd022b2...

[compiler-errors]

In other words, in a perfect compiler, we'd be registering the [T; N]: Unsize<[T]> goal here rather than manually matching on the array tykind during indexing. That unsize goal would have a built-in impl that looks like:

impl<T, const N: usize> Unsize<[T]> for [T; N] {}

which would have an implicit ConstArgHasType(N, usize) goal associated with it. Thus to satisfy that goal, we'd also need to satisfy the ConstArgHasType subgoal, which I'm emulating here by registering that manually.

But because we're not actually going thru that impl, we're side-stepping that predicate, which means that we don't taint the infcx, and end up getting generating malformed MIR which is built off of the assumption that array len consts are always type usize.

[BoxyUwU]

If we don't assume that field types are wf then why does the crashes test ICE?

struct Struct<const N: i128>(pub [u8; N]);

pub fn function(value: Struct<3>) -> u8 {
    value.0[0]
}

value.0 should result in a not wf type so we should error? I would expect that if we are not assuming field types are wf then we are checking them for wf somewhere

[compiler-errors]

If we don't assume that field types are wf [...]

Well so we do assume the field types are WF, but what I'm saying is that that's not an assumption we can or should rely on, because it's really just a side-effect of how we type check the indexing operation in this one case.

We don't assume it's WF in other cases such as method lookup, which is that example that I provided above.

[BoxyUwU]

If we're not assuming field types are wf why do we not just check wf(field_ty) whenever we do a.b? It sounds like instead we expect that any context you can do a.b in will indirectly check the field type for wf?

edit: tbc when I say assume I don't mean in an implied bounds sense, just a "we dont bother explicitly checking wf(typeof(a.b))`"

[BoxyUwU]

The fact that we're skipping a where clause on the builtin impl does make sense though.

[compiler-errors]

It sounds like instead we expect that any context you can do a.b in will indirectly check the field type for wf?

Yeah, kinda that's the state of things. I think changing this to always check that a.b is wf would probably be beneficial for error tainting, but bad for diagnostics, so keeping this limited is kinda my motivation here. I understand that it feels kinda inconsistent tho, but I don't have a better idea for how to get error tainting to understand this situation.

[bors]

☀️ Try build successful - checks-actions
Build commit: fd022b2 (fd022b24ef05db4770f4a73d0f539d14ee19a7e4)

[rust-timer]

Finished benchmarking commit (fd022b2): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.8%	[-1.4%, -0.3%]	2
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.8%, secondary 2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[1.1%, 3.5%]	2
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.8%	[-0.8%, -0.8%]	1

Cycles

Results (secondary -1.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 772.605s -> 771.032s (-0.20%)
Artifact size: 328.25 MiB -> 328.30 MiB (0.02%)

[BoxyUwU]

@bors r+ rollup

[bors]

📌 Commit 7e68422 has been approved by BoxyUwU

It is now in the queue for this repository.