Verify generic functions' bodies against requires/ensures contracts - #105
Conversation
Generic functions carrying #[thrust_macros::requires]/#[thrust_macros::ensures] had their bodies skipped during verification: the contract was trusted rather than checked against the implementation. Non-generic functions were verified correctly, so wrong contracts (or bodies with trivially-violated assertions) on generic functions were silently accepted. This was unsound and also made higher-order functions' contracts trusted summaries. The requires/ensures macro routes the contract through an #[thrust::extern_spec_fn] wrapper whose target is the annotated function. For a generic function this wrapper was registered as a deferred def with NoAnalyze, so the target's body was never run through the analyzer. The wrapper carries the contract while the target carries the body, so the two roles must be decoupled. Replace DeferredDefMode with body_to_analyze: Option<LocalDefId> on DeferredDefTy: local_def_id remains the spec source (expected_ty), while body_to_analyze names the def whose body is verified against that spec. For a generic extern_spec_fn wrapper targeting a local function, register the target's body to be analyzed per-instantiation against the wrapper's contract; using the call site's concrete generic args sidesteps the constrained-type-parameter limitation of placeholder_generic_args. Targets without an analyzable MIR body (genuine extern fns, bodyless trait method declarations) and trusted/ignored targets keep body_to_analyze = None and stay unchecked. Add fail tests covering the issue's assert!(false) repro and a wrong-body generic contract that is consistent with its caller.
The body to verify is always the def keyed in defs (the extern_spec_fn target, or the function itself), so storing it as a separate LocalDefId duplicated the key. Replace body_to_analyze: Option<LocalDefId> with analyze_body: bool and derive the body from the keyed def_id in def_ty_with_args.
The body-analysis flag is a two-state mode, identical to the original DeferredDefMode, so restore that enum rather than a synonymous bool. The behavioral fix stays the same: def_ty_with_args verifies the keyed target def's body (not the spec source), and generic extern_spec_fn targets with a local, analyzable, non-trusted body are registered with Analyze.
There was a problem hiding this comment.
Pull request overview
Fixes an unsoundness in Thrust’s verifier where generic functions annotated with #[thrust_macros::requires] / #[thrust_macros::ensures] were not having their bodies checked, causing their contracts to be treated as trusted summaries (including for higher-order/generic-over-closure functions). The change updates deferred-definition registration and instantiation-time typing so that, when appropriate, the target function body is analyzed against the spec source’s expected type.
Changes:
- Update deferred-def registration for generic functions so extern-spec wrapper targets are eligible for
Analyzewhen local MIR is available and not skipped. - Change
def_ty_with_argssoAnalyzemode verifies the keyed def’s body (the actual target) against the spec-providing wrapper’sexpected_ty. - Add UI tests covering generic
requires/ensureswhere the body does and does not satisfy the contract.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/ui/pass/fn_poly_annot_2.rs | New passing UI test ensuring a generic function body is checked against its contract. |
| tests/ui/fail/fn_poly_annot_2.rs | New failing UI test proving a wrong generic body now triggers Unsat. |
| src/analyze/crate_.rs | Registers deferred defs for generic extern-spec targets with Analyze when the target is local/analyzable. |
| src/analyze.rs | Ensures deferred instantiations in Analyze mode run body analysis on the keyed def rather than the spec wrapper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Fixes#103.
Problem
Generic functions carrying
#[thrust_macros::requires]/#[thrust_macros::ensures]had their bodies skipped during verification — the declared contract was trusted rather than checked against the implementation. Non-generic functions were verified correctly, so a wrong contract on a generic function was silently accepted. This was unsound, and it also made higher-order functions' contracts (generic over the closure type) trusted summaries.Root cause
The
requires/ensuresmacro routes the contract through an#[thrust::extern_spec_fn]wrapper whose tail-call target is the annotated function. For a generic function, this wrapper was registered as a deferred def withNoAnalyze, so the target's body was never run through the analyzer (def_ty_with_argsgates body analysis onmode.should_analyze()).Two roles were also conflated in the deferred def: the spec source (the wrapper, which carries the contract via
expected_ty) and the body to verify (the wrapper's target). They are different defs for an extern-spec wrapper.Non-generic functions were unaffected because they go through
register_def(Concrete) +analyze_local_defs, which runs the body against the contract.Fix
The change is behavioral, not a data-model change —
DeferredDefTy { local_def_id, cache, mode }is unchanged, wherelocal_def_idis the spec source andmodeisAnalyze/NoAnalyze:def_ty_with_args: whenmodeisAnalyze, verify the body of the keyed def (def_id— i.e. the extern-spec target, or the function itself for a plain generic fn) against the spec, instead of runninglocal_def_id(the spec source / wrapper). The analysis runs per-instantiation using the call site's concrete generic args, which sidesteps the constrained-type-parameter limitation ofplaceholder_generic_args.refine_fn_def:register_deferred_defnow takes the target key explicitly, and a generic extern-spec wrapper whose target is local, has MIR, and isn't inskip_analysisis registeredAnalyze(was unconditionallyNoAnalyze). Targets that must stay trusted keepNoAnalyze: non-local extern fns (e.g.std::mem::take), bodyless trait method declarations (no MIR), and trusted/ignored targets (#[thrust::trusted], whose macro expansion marks the original#[thrust::ignored]).Known limitation: an uncalled generic function is never instantiated, so its body is never analyzed — a wrong contract on an uncalled generic function still wouldn't be caught, consistent with the per-instantiation approach. (A
TODOnotes that the criteria for which extern-spec'd target is analyzed could be made clearer.)Tests
tests/ui/pass/fn_poly_annot_2.rs— a genericid<T>(x, _t) -> i32whose body (x * 1) satisfiesensures(result == x); verifies.tests/ui/fail/fn_poly_annot_2.rs— the same shape with a wrong body (x + 1) and a caller that asserts nothing about the result. Before the fix the body was never checked so this was silently accepted; now it'sUnsat.Verification
annot_struct_impl,annot_preds_trait*,adt_poly_fn_poly) still pass once their bodies are actually verified.cargo fmt --checkandcargo clippy -- -D warningsare clean.https://claude.ai/code/session_01VTACey3vnA23ug9Pm5hMSF