Fix ICE on non-literal cover/assert/check message expressions - #4711
Fix ICE on non-literal cover/assert/check message expressions#4711ivmat wants to merge 4 commits into
cover/assert/check message expressions#4711Conversation
`kani-compiler`'s codegen hooks for `kani::cover`, `kani::assert`, `kani::check` and the internal safety-check/unsupported-check hooks all called `gcx.extract_const_message(&msg).unwrap()` to recover the message string. `extract_const_message` returns `None` whenever the message operand does not codegen down to a string-literal constant -- for example when it is a function parameter -- which turned the `.unwrap()` into an internal compiler error instead of a normal diagnostic. Add `extract_msg_or_err`, mirroring the existing `utils::span_err` + `abort_if_errors` pattern already used by neighbouring intrinsic codegen in this file, and use it at all six call sites in `hooks.rs` (Cover, Assert, UnsupportedCheck, SafetyCheck, SafetyCheckNoAssume, Check). Each now emits a spanned "`<construct>` message must be a string literal" error and aborts compilation cleanly instead of panicking. Add UI regression tests for the two publicly reachable constructs, `kani::cover` and `kani::assert`, modelled on the existing `tests/ui/ice-size-overflow` test. `kani::check` is `pub(crate)` with no public re-export, so user code cannot invoke it directly.
72f4f5a to
4fa8466
Compare
There was a problem hiding this comment.
Pull request overview
This PR prevents internal compiler errors in kani-compiler when kani::cover, kani::assert, kani::check, and related internal hooks are given a non-literal message expression by emitting a proper diagnostic and aborting compilation cleanly instead of panicking.
Changes:
- Added
extract_msg_or_errhelper inkani-compilerhook codegen to replaceextract_const_message(...).unwrap()at the six affected hook sites. - Updated the hook implementations to produce a spanned error:
`<construct>` message must be a string literal. - Added UI regression tests for the
kani::coverandkani::assertnon-literal message cases.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| kani-compiler/src/codegen_cprover_gotoc/overrides/hooks.rs | Replaces unwrap() on non-literal hook messages with a diagnostic + abort path via extract_msg_or_err. |
| tests/ui/cover-non-literal-message/main.rs | New UI test ensuring kani::cover with a non-literal message produces a clean compiler error (no ICE). |
| tests/ui/cover-non-literal-message/expected | Expected diagnostic output for the new cover UI test. |
| tests/ui/assert-non-literal-message/main.rs | New UI test ensuring kani::assert with a non-literal message produces a clean compiler error (no ICE). |
| tests/ui/assert-non-literal-message/expected | Expected diagnostic output for the new assert UI test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`extract_msg_or_err` documents itself as aborting compilation when a hook
message is not a string literal, but the `unwrap_or_else` fallback returned
`String::new()` after `abort_if_errors()`. `abort_if_errors()` returns `()`,
not `!`, so nothing in the types stopped codegen from continuing with an empty
message if it ever failed to fire.
Return `unreachable!("Rustc should have aborted already")` instead, matching
the `abort_if_errors()` + `unreachable!()` pairing already used in
`codegen/intrinsic.rs`, `context/goto_ctx.rs` and
`kani_middle/transform/contracts.rs`.
The `unreachable!` is not reachable in practice: `DiagCtxtHandle::span_err`
returns `ErrorGuaranteed` and pushes onto `err_guars`, so the following
`abort_if_errors()` always finds an error and unwinds with `FatalError`. Both
UI tests added by this PR still produce a clean `error:` diagnostic and no ICE.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/ui/cover-non-literal-message/main.rs:21
- This UI test relies on passing the message through a helper function parameter to make it “non-literal at the call site”. Because
cover_with_msgis tiny, rustc/MIR optimizations could inline it and turn this back into a directkani::cover(true, "...")call, which would stop exercising the non-literal-message path and weaken the regression test.
fn cover_with_msg(cond: bool, msg: &'static str) {
kani::cover(cond, msg);
}
tests/ui/assert-non-literal-message/main.rs:21
- This UI test depends on the message being a function parameter at the
kani::assertcall site. Sinceassert_with_msgis small, it may be inlined by optimizations, turning the call back into a direct literal and no longer covering the intended non-literal-message behavior.
fn assert_with_msg(cond: bool, msg: &'static str) {
kani::assert(cond, msg);
}
| gcx.extract_const_message(msg_expr).unwrap_or_else(|| { | ||
| utils::span_err(gcx.tcx, span, format!("`{construct}` message must be a string literal")); | ||
| gcx.tcx.dcx().abort_if_errors(); | ||
| unreachable!("Rustc should have aborted already") |
There was a problem hiding this comment.
This message could be way more descriptive to tell the user when it sees this message why rustc should have aborted already.
Both tests make the hook message non-literal by passing it through a helper function parameter. Kani passes no `-C opt-level` or `-Z mir-opt-level`, so the MIR inliner is off and the helpers survive today, but nothing in the tests said so. If the helpers were ever inlined the message would fold back into a literal and the tests would stop exercising the path they exist to cover. `#[inline(never)]` pins that down. Verified: with `#[inline(always)]` and `-Zmir-opt-level=4 -Zinline-mir=yes` the diagnostic disappears and codegen proceeds; with `#[inline(never)]` it is still emitted under the same flags.
|
@ivmat could you also tackle the two suppressed Copliot comments? Almost there. |
…sage The fallback in `extract_msg_or_err` runs only if `abort_if_errors` returns after an error has been emitted, which cannot happen: `span_err` emits a hard error, so `abort_if_errors` observes it and raises a fatal error first. State that invariant in the code rather than asserting it. The panic message now says what must have gone wrong if it ever does fire -- the error was not counted by the diagnostic context -- and interpolates `construct` so it names the hook being codegen'd.
@feliperodri i think i covered everyting now. i also could not rerrun all same CI tests locally but i dont think there should be any issues |
Problem
kani-compiler's codegen hooks forkani::cover,kani::assert,kani::checkand the internalsafety-check/unsupported-check hooks all call
gcx.extract_const_message(&msg).unwrap()to recover themessage string.
extract_const_messagereturnsNonewhenever the message operand does not codegen down to astring-literal constant — for example when it is a function parameter — so the
.unwrap()produces aninternal compiler error rather than a normal diagnostic.
Reproducer (
kani::covercase):This still reproduces on
main: all sixextract_const_message(&msg).unwrap()call sites are presentin
kani-compiler/src/codegen_cprover_gotoc/overrides/hooks.rs.Fix
Add
extract_msg_or_err, which mirrors the existingutils::span_err+abort_if_errorspatternalready used by neighbouring intrinsic codegen in the same file, and use it at all six call sites
(
Cover,Assert,UnsupportedCheck,SafetyCheck,SafetyCheckNoAssume,Check).Each now emits a spanned
`<construct>` message must be a string literalerror and abortscompilation cleanly instead of panicking.
Tests
UI regression tests modelled on the existing
tests/ui/ice-size-overflowtest (a prior"ICE → clean error" regression test):
tests/ui/cover-non-literal-message/tests/ui/assert-non-literal-message/kani::checkispub(crate)inlibrary/kani_core/src/lib.rswith no public re-export, so user codecannot invoke it directly (it fails earlier with
E0425). The remaining hooks(
safety_check,safety_check_no_assume,unsupported_check) are compiler-generated and notreachable from user code either, so neither is given a UI test.
Testing performed
cargo build -p kani-compilerandcargo check -p kani-compileron this branch — pass.--force-rerun, all suites + unit tests): 1472 passed,3 failed — all 3 reproduce identically on a clean
maincheckout (2b7972b7c), so they arepre-existing environment issues (one requires z3, which is not installed here), not caused by this
change.
cargo clippyon the pinned nightly — clean.scripts/kani-fmt.sh --checkand the copyright/format checks — clean.Happy to adjust the diagnostic wording or the test placement if you'd prefer something different.