Do not panic in ty::consts::Const::try_to_target_usize() in case of size mismatch

[gurry]

Fixes #126359

This is the failing code:

struct OppOrder<const N: u8 = 3, T = u32> {
    arr: [T; N],
}

fn main() {
    let _ = OppOrder::<3, u32> { arr: [0, 0, 0] };
}

The ICE occurred while unifying the type of array [0, 0, 0] with OppOrder::<3, u32>.arr field. arr's type is malformed because it has u8 for size instead of usize. So when we try to unify usize with u8 a method in Valtree panics causing the ICE.

This PR makes ty::consts::Const::try_to_target_usize, which calls Valtree methods, check for size mismatch and gracefully return None instead of calling Valtree.

Edit

Now changed the approach to catch wrongly typed array size in FnCtxt::check_expr_array so that we never reach a point where we try to unify usize with u8.

[rustbot]

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[BoxyUwU]

We intentionally started ICEing here because it is genuinely a bug to be doing this. This PR Is effectively a partial revert of #126159. Ideally what should be happening is we should be tainting errors somewhere so that we can check for ErrorGuaranteed somewhere and avoid running this codepath

[BoxyUwU]

Looking it into it a little bit the problem is that super_relate_tys for TyKind::Array has diagnostic logic in it that unconditionally evaluates the array length to a usize. This codepath should correctly handle length arguments that are incorrectly typed instead of assuming they can be evaluated to a usize.

edit: wrote a bit more in the actual issue itself, should read that too

[BoxyUwU]

r? @BoxyUwU

[RalfJung]

This diagnostic still looks buggy.

[gurry]

Yeah, it should be suppressed. Will look into it

[RalfJung]

Yeah, as you said for most users of try_to_target_usize it is a bug if the type is not usize (the "try" refers to the fact that the constant may fail to evaluate, but it must have the right type).

The special situation here is that this is called in an error path.

[gurry]

Looking it into it a little bit the problem is that super_relate_tys for TyKind::Array has diagnostic logic in it that unconditionally evaluates the array length to a usize. This codepath should correctly handle length arguments that are incorrectly typed instead of assuming they can be evaluated to a usize.

Yeah, that's why I felt the solution was simply to prevent the panic. First I thought I should make the change here in structurally_relate_tys():

rust/compiler/rustc_type_ir/src/relate.rs

Lines 502 to 514 in 56e112a

	Err(err) => {
	// Check whether the lengths are both concrete/known values,
	// but are unequal, for better diagnostics.
	let sz_a = sz_a.try_to_target_usize(tcx);
	let sz_b = sz_b.try_to_target_usize(tcx);
	match (sz_a, sz_b) {
	(Some(sz_a_val), Some(sz_b_val)) if sz_a_val != sz_b_val => Err(
	TypeError::FixedArraySize(ExpectedFound::new(true, sz_a_val, sz_b_val)),
	),
	_ => Err(err),
	}
	}

but then it seemed easier to do it inside Const::try_to_target_size(). I can move it to the above location if you guys prefer.

[RalfJung]

I don't think try_to_target_size is the right place for this. Though the function could probably use a comment saying that it is resilient to constants that fail to compute, but not resilient to constants having the wrong type.

[gurry]

@rustbot author

[matthiaskrgr]

wooondering if its worth having a debug_assert against this kind of stuff ('note: expected X, found Y' where x.display() == y.display()) because that just seems silly from a diagnostic point of view 😅

[gurry]

I have undone my changes to rustc_middle::ty::consts::Const::try_to_target_usize and moved them to the impl of rustc_type_ir::inherent::::Const::try_to_target_usize on rustc_middle::ty::consts::Const. This leaves the semantics of rustc_middle::ty::consts::Const::try_to_target_usize intact.

As for the confusing diagnostics:

LL |     let _ = OppOrder::<3, u32> { arr: [0, 0, 0] };
   |                                       ^^^^^^^^^ expected `3`, found `3`

and

= note: expected array `[u32; 3]`
              found array `[u32; 3]`

I'll tackle them later in a separate PR.

@rustbot review

[gurry]

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 46)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 49)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 61)
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---

running 237 tests
........................................................................................  88/237
........................................................................................ 176/237
........2024-07-07T08:49:16.171323Z ERROR compiletest::runtest: fatal error, panic: "crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge."
[crashes] tests/crashes/126359.rs ... F
....................................................

failures:
failures:

---- [crashes] tests/crashes/126359.rs stdout ----

error: crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge.
thread '[crashes] tests/crashes/126359.rs' panicked at src/tools/compiletest/src/runtest.rs:377:18:
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:

Occurred due to an overdue rebase. Rebased now.

[RalfJung]

That seems potentially quite confusing if there are two try_to_target_usize methods on the same type that don't behave the same.

#126159 was written under the assumption that the compiler can reliably avoid feeding ill-typed programs to later phases. If that is not the case, maybe we should go back to having the Const::try* methods check for size mismatches...

[gurry]

That seems potentially quite confusing if there are two try_to_target_usize methods on the same type that don't behave the same.

#126159 was written under the assumption that the compiler can reliably avoid feeding ill-typed programs to later phases. If that is not the case, maybe we should go back to having the Const::try* methods check for size mismatches...

What if we rename the try_to_target_usize on the rustc_type_ir::inherent::::Const trait to something else? Perhaps to a name that suggests that it takes size mismatch into account.

[RalfJung]

Is it clear where one would want to call which function? Sounds a bit like the one on the inherent impl is just an ICE waiting to happen, if the compiler feeds ill-typed terms so deep into the type system.

Is it clear why that happens / why it cannot be prevented? Boxy wrote above

"Looking it into it a little bit the problem is that super_relate_tys for TyKind::Array has diagnostic logic in it that unconditionally evaluates the array length to a usize. This codepath should correctly handle length arguments that are incorrectly typed instead of assuming they can be evaluated to a usize."

Have you dug to the bottom of why that does not work?
How much such diagnostic logic do we have? Can we realistically expect it to be written in a way that avoids the pitfalls of ill-typed code?

[gurry]

The only place the try_to_target_usize method from trait rustc_type_ir::inherent::::Const is called is here:

rust/compiler/rustc_type_ir/src/relate.rs

Lines 499 to 517 in 89aefb9

	(ty::Array(a_t, sz_a), ty::Array(b_t, sz_b)) => {
	let t = relation.relate(a_t, b_t)?;
	match relation.relate(sz_a, sz_b) {
	Ok(sz) => Ok(Ty::new_array_with_const_len(tcx, t, sz)),
	Err(err) => {
	// Check whether the lengths are both concrete/known values,
	// but are unequal, for better diagnostics.
	let sz_a = sz_a.try_to_target_usize(tcx);
	let sz_b = sz_b.try_to_target_usize(tcx);
	match (sz_a, sz_b) {
	(Some(sz_a_val), Some(sz_b_val)) if sz_a_val != sz_b_val => Err(
	TypeError::FixedArraySize(ExpectedFound::new(true, sz_a_val, sz_b_val)),
	),
	_ => Err(err),
	}
	}
	}
	}

which is the very ICEing code path in the present issue and the one that Boxy mentions. It is the only place which is passing such ill-typed code downstream.

A simple solution would be to remove this method from the trait and expose the underlying method try_to_valtree instead which it calls. That will rid the trait of all ambiguity.

Currently try_to_target_usize's implementation looks like this:

   fn try_to_target_usize(self, interner: TyCtxt<'tcx>) -> Option<u64> {
        let scalar = self.try_to_valtree()?.try_to_scalar_int()?;
        if scalar.size() != interner.data_layout.pointer_size {
            return None;
        }
        Some(scalar.to_target_usize(interner))
    }

If we expose try_to_valtree on the trait, this whole logic can be written at the call site in relate.rs shown above. However, that will require us to import types like ty::ValTree and interpret::value::Scalar in the rustc_type_ir crate (which is where the rustc_type_ir::inherent::::Const trait lives). Doing that I felt would be a bit awkward, which is why I abandoned the idea.

[BoxyUwU]

Will try and get to this in the next couple of days, didn't realise there had been a bunch more conversation

[RalfJung]

The only place the try_to_target_usize method from trait rustc_type_ir::inherent::::Const is called is here:

Ah, there's the extra abstraction layer in rustc_type_ir in the way. Yeah that always makes things more complicated.

A simple solution would be to remove this method from the trait and expose the underlying method try_to_valtree instead which it calls. That will rid the trait of all ambiguity.

That does sound like a good plan to me, but I don't know if it is possible with the rustc_type_ir design.

[BoxyUwU]

I find the Const::try_to_target_usize method to be a bit confusing, it seems to be able to recover just fine when we have a value but its not a scalar, even though it ICEs when its not usize. If, indeed, it's not supposed to avoid panics for anything other than "the constant failed to evaluate" then I would expect us to have some Valtree::to_target_usize that ICEs under all cases where it is not a usize.

Regardless, I do not believe making try_to_target_usize "more" fallible, or using some other fallible method in relate.rs to be the correct fix here. I don't see any reason why there should be diagnostics logic in relate.rs or why that logic should be trying to acquire a u64 in the first place. The correct fix should be to update TypeError::FixedArraySize to accept ExpectedFound<ty::Const<'tcx>> instead of u64 and then update diagnostics logic elsewhere that uses FixedArraySize to handle Const instead of u64 (from a cursory look around I do not expect this to be too involved).

[RalfJung]

I find the Const::try_to_target_usize method to be a bit confusing, it seems to be able to recover just fine when we have a value but its not a scalar, even though it ICEs when its not usize. If, indeed, it's not supposed to avoid panics for anything other than "the constant failed to evaluate" then I would expect us to have some Valtree::to_target_usize that ICEs under all cases where it is not a usize.

I'm a bit confused by this comment.^^

try_to_target_usize propagates const-eval errors by returning Err but expects the caller to get the type right and ICEs if the type is wrong.

So to_target_usize, presumably equivalent to try_to_target_usize().unwrap(), would ICE when it is not usize and also ICE when const-eval fails e.g. due to the constant having UB. This is basically never what we want -- codegen is the only part of the compiler that can rely on const-eval not failing.

[RalfJung]

Argh sorry, I misremembered and what I said is wrong.

The key difference is between to_valtree and eval: the former expects the valtree to be already evaluated, the latter will evaluate it if needed. to_valtree ICEs if the valtree wasn't evaluated, try_to_valtree instead just returns None.

I am not entirely sure why the non-evaluating functions even exist, probably that's for when you don't have a ParamEnv? If you have a param_env, I think you should always call the eval* methods; the distinction between already evaluated and not-yet-evaluated Const should matter as little as possible. And then of course we also have try_eval methods, where e.g. try_eval_scalar will gracefully fail if the result is not a scalar, which seems inconsistent with ICEing if the result is not a usize...

I agree the Const methods are kind of a mess, but I don't know all its consumers well enough to know how to improve it.

[RalfJung]

Cc @oli-obk , maybe you have an idea?

[BoxyUwU]

I think it'd probably be good to open an issue or a zulip thread to talk about this properly rather than having the discussion on this PR since I don't think any progress there is actually going to solve the ICE.

[gurry]

Regardless, I do not believe making try_to_target_usize "more" fallible, or using some other fallible method in relate.rs to be the correct fix here. I don't see any reason why there should be diagnostics logic in relate.rs or why that logic should be trying to acquire a u64 in the first place. The correct fix should be to update TypeError::FixedArraySize to accept ExpectedFound<ty::Const<'tcx>> instead of u64 and then update diagnostics logic elsewhere that uses FixedArraySize to handle Const instead of u64 (from a cursory look around I do not expect this to be too involved).

FixedArraySize is used at these two locations:

rust/compiler/rustc_middle/src/ty/error.rs

Lines 62 to 69 in b5723af

	TypeError::FixedArraySize(values) => format!(
	"expected an array with a fixed size of {} element{}, found one with {} element{}",
	values.expected,
	pluralize!(values.expected),
	values.found,
	pluralize!(values.found)
	)
	.into(),

and

rust/compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs

Line 1776 in b5723af

Some(TypeErrorAdditionalDiags::ConsiderSpecifyingLength { span, length: sz.found })

If we change FixedArraySize(ExpectedFound<u64>) to FixedArraySize(ExpectedFound<ty::Const<'tcx>>) we will now need to evaluate the consts at these locations and that will cause code duplication. Therefore it might still make sense to have a method like try_to_target_usize on ty::Const which can be called from these two locations.

To prevent confusion we can give this new method a different name like try_to_target_usize_checked (because it checks for size mismatch and returns None instead of panicking) and add comments on both methods explaining the difference in their behaviour.

Would that make sense? @BoxyUwU @RalfJung

[gurry]

FixedArraySize is used at these two locations:

Actually it's used at a third location as well which is here:

rust/compiler/rustc_type_ir/src/error.rs

Line 77 in b5723af

| PolarityMismatch(_) | Mismatch | AbiMismatch(_) | FixedArraySize(_)

but no const evaluation is required there so it's not relevant.

[oli-obk]

Can we avoid even getting to this method call by some appropriate tainting elsewhere?

[gurry]

Can we avoid even getting to this method call by some appropriate tainting elsewhere?

Thanks. I have now implemented the approach to catch the error earlier in FnCtxt::check_expr_array and to taint the context there.

@rustbot review

[compiler-errors]

This isn't a sufficient fix. The expectation may always be None, typically by just separating the expression from the place that gives that expectation. For example, I believe this still ICEs:

struct OppOrder<const N: u8 = 3, T = u32> {
    arr: [T; N],
}

fn main() {
    let arr = [0, 0, 0];
    let _ = OppOrder::<3, u32> { arr };
}

[compiler-errors]

I want to echo Boxy's suggestions about making structurally_relate_tys be more agnostic to the consts of the ty::Array. I don't believe any amount of error tainting will fix this bug.

@rustbot author

[BoxyUwU]

Hi, sorry for this being kind of a bad contribution experience with a lot of back and forth on what's being asked of you :< I'm going to close this since there's not been any activity for a month but you're more than welcome to re-open this PR if you're interested in continuing this (though make sure to re-open before pushing any new commits if you do decide to)

[gurry]

No worries @BoxyUwU and @compiler-errors. Received a lot of valuable feedback here. Apologies for not being responsive. I'm a little busy with some other stuff these days. Will certainly return to it later. Thank you :)