Repository navigation
Conversation
|
@feliperodri Not exactly. The main differences are:
So I would describe this PR as an alternative and strengthening of the Challenge 21 portion of #538, rather than a direct complement. |
feliperodri
left a comment
There was a problem hiding this comment.
Review: PR #621 — Challenge 21 (StrSearcher safety)
Verdict rationale
This PR is substantially and qualitatively different from the rejected #538. It does not repeat #538's fatal pattern, and it does real, careful verification work. However, it fails two required criteria that the author openly acknowledges in-code, so it cannot be approved as-is.
It is NOT the #538 body-swap / assume-the-conclusion failure (credit where due)
I classified all 12 #[cfg(not(kani))] blocks. None compile out TwoWaySearcher::new/next/next_back and replace them with pure nondeterministic stubs. Instead:
- Boundary-repair loop swaps — forward
Searcher::next(pattern.rsdiff L96/L177 → kani at L115-145) and reversenext_back(L334/L362/L443): the realsearcher.next::<RejectAndMatch>(...)/next_back::<...>call is preserved; only thewhile !is_char_boundary(b) { b += 1 }repair loop is summarized. - Candidate-loop swaps —
continue 'search(L807, L1093) replaced by suffix summaries (kani_stub_after_failed_forward_candidate/...reverse...). - Inner byte-scan
for-loop swaps — (L820, L859, L1101, L1152) replaced bykani_scan_forward/kani_scan_reverse, which still indexneedle[index]/haystack[position+index]at a nondeterministic representative index over the complete scan interval (diff L590-598), so memory-safety of the real indexing is genuinely checked. - Empty-needle char decode (L68) — real
.chars().next()retained in non-kani; kani models it as a nondet width 1..=4 bounded by remaining bytes plus a UTF-8 boundary assumption.
The real control flow, arithmetic (self.position += i - self.crit_pos + 1, self.position += self.period, etc.), and state transitions of the Two-Way algorithm are executed for one representative candidate. This is legitimate loop summarization, not a function-body swap.
Assume-the-conclusion check (FATAL in #538): PASS. The UTF-8 boundary facts on the Match path (assume_valid_utf8_two_way_match_boundaries, diff L1821-1855; assume_valid_utf8_two_way_reverse_match_start, L1857-1877) are gated behind assert!s that the relevant needle bytes were byte-equal to the haystack over the full final character, and the needle is valid UTF-8. Assuming the boundary then follows from "byte-equality with a valid-UTF-8 needle transfers the char boundary" — which is exactly a UTF-8 decoding fact Challenge 21 explicitly permits importing (assumptions 2 and 3). This is a defensible proof cut, not a raw assumption of the type-invariant conclusion.
Invariant is non-trivial: PASS. type_invariant_two_way_searcher (diff L2430-2482) constrains cursor bounds, char-boundaries of position/end, 1 <= period <= needle_len, crit_pos/crit_pos_back bounds, long/short-period sentinel consistency (memory/memory_back), and short-period boundary summaries. It is meaningful, not true.
Proof harnesses exist: 15 challenge #[kani::proof] harnesses (1 empty constructor + 6 empty methods + 6 Two-Way methods), plus the 2 retained small_slice_eq proofs. Absence of proof_for_contract is fine here since the challenge is invariant-based, not contract-based.
Blocking issue 1 — Verification is NOT unbounded (mandatory criterion FAILS)
The challenge states: "The verification must be unbounded—it must hold for inputs of arbitrary size." This PR hard-bounds inputs:
pattern.rsdiff L1271:const MAX_UTF8_BYTES: usize = 16;any_valid_utf8_str(L1273-1282) takes&[u8; MAX]and slices a 16-byte array; every harness buildshaystack/needlefrom[u8; MAX_UTF8_BYTES](e.g. L2752-2755).
So needle.len(), period, crit_pos, position, end are all bounded by 16. The author acknowledges this directly at diff L2748-2749 ("the concrete haystack and needle models remain bounded by MAX_UTF8_BYTES") and in the PR description ("the current harness models do not constitute an arbitrary-length input proof"). Summarizing the loops removes unwind bounds but does not make the input size unbounded. This is a clear failure of a mandatory requirement.
Blocking issue 2 — Creation does not establish C for TwoWay (success criterion 1 FAILS)
Criterion 1 requires proving that a searcher created from any valid UTF-8 haystack satisfies C. This is done for the empty needle via harness_str_searcher_empty_into_searcher (diff L2557-2565), which calls the real needle.into_searcher(haystack). But for the core Two-Way case there is no constructor harness: TwoWaySearcher::new is never invoked (grep: 0 references), and the invariant is only kani::assumed (e.g. L2772). The author explicitly disclaims this at diff L2745-2747: "Establishing it for the production preprocessing algorithm is a separate proof obligation and is not claimed by this module."
This is a real gap, not a formality: because the hand-written invariant is never cross-checked against what TwoWaySearcher::new actually produces, the preservation proofs (criterion 3) run over a hand-specified state set that is never tied to reachable states. If that set under-approximates reachable states, criterion 3 coverage is incomplete; the constructor harness is what closes this.
Non-blocking observations
- The loop-state relations (
valid_two_way_*_loop_state) are used as bothassume(loop head) andassert(loop exit) but are not independently shown to be inductive; soundness of the suffix summaries rests on them. Worth a reviewer note even after the blockers are fixed. kani::assume(false)is used to prune the continued-loop path (e.g.next_rejectdiff L271) after the per-iteration safety asserts fire — acceptable idiom, but each such site should be double-checked to confirm every concrete exit (Reject/Match/Done) is reachable via the symbolic prefix.
Direction to author
- Make inputs unbounded: drive the loop summaries/
type_invariantoff symbolic lengths rather than a fixed 16-byte array, or otherwise remove the input-size bound, so the proof holds for arbitrary-size haystack/needle. - Add a Two-Way constructor harness that runs the real
into_searcher/TwoWaySearcher::newon a valid UTF-8 (needle, haystack) and assertstype_invariant_two_way_searcher, closing criterion 1 for the core algorithm.
Once these two are addressed, the approach here (real algorithm + Challenge-21-permitted UTF-8 imports + non-trivial invariant) is a strong basis for approval.
|
@feliperodri Thank you for the detailed review. I checked the issues and non-blocking concerns as follows. Blocking issue 2 This issue is addressed by It requires a non-empty needle, checks that the resulting implementation is Non-blocking observation 1 I manually checked the preservation obligations for the These summaries are therefore supported by a documented trusted argument, and their preservation obligations have been manually checked against the production control flow. This is an informal manual justification rather than a separate Kani harness proving inductiveness for every loop transition. Non-blocking observation 2 I also checked the relevant The other Unbounded verification I agree that the current verification is bounded rather than genuinely unbounded. I experimented with a symbolic The remaining proof has an explicit bounded scope: the Two-Way constructor harness covers haystacks of up to 4 bytes and non-empty needles of up to 3 bytes. Within that domain, the constructive generator reaches every valid UTF-8 string and This is a justified bounded verification, but I am not claiming that it satisfies the challenge’s literal requirement for arbitrary-size inputs. I would appreciate any further feedback on this scope or on ways to make the summaries and harnesses more precise. I would be happy to make further changes or optimizations based on your suggestions. |
Review — changes requested (Challenge 21)Thanks for the submission. To set expectations up front: there are two Challenge 21 solutions (this PR and #538), and we give preference to #538 — it verifies the real Concerns (local review, pinned Kani
Acceptance bar (same for both PRs): the solution can only be accepted if it presents an unbounded proof that completes within CI resource limits, verifying the real algorithm rather than assuming the safety property. |
feliperodri
left a comment
There was a problem hiding this comment.
Challenge 21 requires: "The verification must be unbounded---it must hold for inputs of arbitrary size." That requirement is final. A Kani solution will only be accepted if it is unbounded in the input size for every required function. This PR still verifies at least part of the challenge with bounded harnesses (kani::unwind bounds and/or fixed-size inputs), so I'm requesting changes until every required function is verified without a bound on the input size.
Summary
This PR adds Kani verification for the substring searcher in
core::str::pattern, covering the empty-needle implementation and the forward and reverse Two-Way search methods required by Challenge 21.Verification Coverage Report (12/12 Harnesses Verified)
nextnext_matchnext_rejectnext_backnext_match_backnext_reject_backnextRejectAndMatchcandidate, summarized forward/reverse byte scans, rejection boundary repair, forward progress, valid Match/Reject ranges, and invariant preservation.next_matchMatchOnlysearch from an arbitrary failed-candidate prefix through one real candidate and a conservative suffix summary that covers a later Match or exhaustion.next_rejectnextstep, covering every Reject or Done exit while checking progress and invariant preservation.next_backRejectAndMatchpath, summarized reverse/forward byte scans, rejection boundary repair, reverse progress, valid Match/Reject ranges, and invariant preservation.next_match_backMatchOnlysearch from an arbitrary failed-candidate prefix through one real candidate and a conservative suffix summary.next_reject_backnext_backstep, covering every Reject or Done exit.Verification Approach
The verification defines a stable-state invariant
Cfor bothStrSearcherimplementations.For
EmptyNeedle,Crequires the forward and reverse cursors to remain in bounds and on UTF-8 boundaries. The constructor harness establishesC, and each method harness starts from an arbitrary state satisfyingC, checks the returned transition, and proves thatCis preserved. The Kani-only character step nondeterministically selects a width from 1 through 4, bounds it by the remaining bytes, and imports the UTF-8 boundary fact permitted by Challenge 21. Because empty-needle results strictly alternate between Match and Reject, the four filtering methods need at most two concrete calls tonextornext_back; this removes their production loops without adding a loop invariant or unwind bound.For
TwoWaySearcher,Ccaptures the safety-relevant stable state: bounded UTF-8 cursor positions, nonzero bounded periods, bounded critical positions, consistent long-period sentinels, valid short-period memory states, and UTF-8 boundary facts for the short-period cuts. Each of the six method harnesses starts from a symbolic state satisfying this invariant and proves valid output ranges, cursor progress, preservation of preprocessing fields, valid memory-state transitions, and restoration ofCbefore returning.The Two-Way candidate loops use loop stubbing. A nondeterministic loop head represents any prefix of failed candidates, one representative candidate retains the real production control flow and arithmetic, and a suffix summary over-approximates any number of later failed candidates followed by either a Match, a Reject, or exhaustion. The summaries havoc only loop-carried state and constrain it with the corresponding loop-state relation.
The forward and reverse byte-scan loops are summarized by
KaniTwoWayScan. A nondeterministic representative scan index checks the real indexing operations over the complete scan interval. The summary then conservatively chooses a mismatch position or a completed scan while retaining the first and last UTF-8 character bytes and the period probe needed to justify safe Match boundaries.Two-Way Reject results may initially stop at byte positions that are not character boundaries. The Kani-only boundary-repair summaries first prove that the raw cursor is within the haystack, execute the real increment/decrement operation at a representative non-boundary loop head, and then use the Challenge 21 UTF-8 assumption that a character has at most three continuation bytes to summarize arrival at a nearby character boundary.
Verification Tradeoffs
Directly unwinding the nested Two-Way candidate and byte-scan loops does not finish within a practical verification budget and would make the proof depend on a fixed search length. The loop summaries avoid fixed unwind bounds for those loops and over-approximate their safety-relevant behavior.
The scan and candidate summaries are proof cuts for memory safety and valid UTF-8 result boundaries. They do not prove that every reported Match is the first or semantically correct substring match, nor do they prove the full functional correctness of the Two-Way algorithm.
The concrete harness inputs are symbolic valid UTF-8 subslices of 16-byte storage arrays. The loop summaries cover arbitrary loop prefixes and suffixes without fixed unwinding, but the current harness models do not constitute an arbitrary-length input proof.
Scope Assumptions
type_invariant_two_way_searcher.#[cfg(kani)]and do not affect non-Kani builds.Verification
All added Challenge 21 harnesses pass locally with Kani.
Resolves #278
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.