Repository navigation
Avoid ICEs from ill-typed direct constants in MIR - #164058
darkraider01 wants to merge 2 commits into
Conversation
rustbot
added
S-waiting-on-review
|
r? @chenyukang rustbot has assigned @chenyukang. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
| && let Some(local_def_id) = instance.def_id().as_local() | ||
| && tcx.is_direct_const(instance.def_id()) | ||
| { | ||
| tcx.ensure_result().check_well_formed(local_def_id)?; |
There was a problem hiding this comment.
The initializer already fails WF, so I reused that result here instead of adding another validation step. This keeps MIR from evaluating the bad valtree. For trait constants, we need to resolve the impl first to find the initializer. I’d appreciate a close look at the new query dependency, especially for cycles and overhead.
| } | ||
|
|
||
| const VALUE: (u8,) = (1_u8,); | ||
| const FROM_FN: u8 = std::gca!(const { value() }); |
There was a problem hiding this comment.
I added this because checking WF can lead into const evaluation and then back into MIR. This exercises that path with valid code. The generic case below also covers an associated constant we can’t resolve yet.
| //[current]~| ERROR method `arr` has an incompatible type for trait [E0053] | ||
| //[next]~| ERROR type annotations needed | ||
| //[next]~| ERROR type mismatch resolving `<A as Array>::LEN == _` [E0271] | ||
| //[current]~^ ERROR the constant `0` is not of type `usize` |
There was a problem hiding this comment.
These were follow-on errors from the invalid constant. Once the WF failure reaches MIR, they disappear. The original constant-type error still gets reported.
|
r? khyperia |
|
This seems like the wrong approach to me. On an aside, the code you're modifying here (the lowering of |
khyperia
added
S-waiting-on-author
Yeah, I see what you mean. I chose this spot because the initializer already fails WF, and I wanted to carry that error through before MIR evaluates the bad valtree. The trait case needed the impl initializer, which is why I added try_resolve, but that doesn’t establish that MIR building is the right place for it. |
|
Huh, just want to link these two PRs, #164294 was just rewritten to not run KnownPanicsLint on tainted MIR, and the issue for this PR has its backtrace in KnownPanicsLint as well. I wonder if the new version of 164294 fixes this issue as well then. (I'm unsure if skipping KnownPanicsLint on tainted MIR is the correct fix or not, it's the weekend and I don't want to think too long on it, just wanted to link the two PRs together :P ) |
|
Just wanted to update that KnownPanicsLint already had the check khyperia described (I was sleep-deprived when I was writing that commit, oops) and so it looks like the issue in #164294 is actually that the body isn't tainted to begin with. So don't bother checking that fix for this issue (although the root cause may still be the same in any case). |
|
☔ The latest upstream changes (presumably #165270) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
Fixes #163903.
An ill-typed direct constant can already have a well-formedness error, but MIR consumers can still try to evaluate its invalid valtree and ICE.
This propagates the existing WF error when building the constant operand, so later MIR consumers see an error constant. Reusing the existing check keeps validation in one place and avoids duplicating it during aggregate lowering.
The tricky part is associated constants: a trait constant needs to resolve to its impl before we can check the actual initializer. Generic instances that can’t yet be resolved keep their existing alias.
The first commit adds tests asserting the existing ICE; the second applies the fix and updates the expected diagnostics. Coverage includes free, inherent, and trait constants under both solvers, along with valid aggregate arguments and generic associated constants.
LLM disclosure : I used LLM to review my fix and to benchmark it.
Local benchmarks used 20 paired runs per case, a pinned CPU, and single-threaded MIR emission. The trait-heavy workload showed roughly 1.7% overhead with the current solver and 3.8% with the next solver. These measurements used the earlier checkout and describe this stress workload; broader rustc-perf testing hasn’t been run.