Add Arbitrary implementation for std::ascii::EscapeDefault - #4782
Conversation
|
@Tianshu-Huang @CYJ904 @wodex1nhaoIeng @srivatsansamraj Opened the Kani PR for the |
There was a problem hiding this comment.
🟡 Changes recommended
The proof must cover observable front- and back-consumed states to protect the core two-ended generation logic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Arbitrary support for std::ascii::EscapeDefault, enabling Autoharness verification for functions using this iterator.
Changes:
- Generates fresh, partially consumed, and exhausted iterator states.
- Adds Kani proof and Autoharness regression coverage.
- Requires direction-specific coverage for front and back consumption.
File summaries
| File | Description |
|---|---|
tests/script-based-pre/autoharness_ascii_escape_default/src/lib.rs |
Adds the Autoharness target function. |
tests/script-based-pre/autoharness_ascii_escape_default/escape_default.sh |
Runs and filters Autoharness output. |
tests/script-based-pre/autoharness_ascii_escape_default/escape_default.expected |
Defines expected successful output. |
tests/script-based-pre/autoharness_ascii_escape_default/config.yml |
Configures the regression test. |
tests/script-based-pre/autoharness_ascii_escape_default/Cargo.toml |
Defines the test crate. |
tests/kani/ascii_escape_default.rs |
Tests remaining lengths, but lacks direction-specific state coverage. |
library/kani/src/arbitrary.rs |
Implements Arbitrary for EscapeDefault. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Verified locally on CBMC 6.11.0 — matches the PR description (VERIFICATION:- SUCCESSFUL, 5/5 cover properties, ~0.78s). Two suggestions, building on Copilot's comment:
|
feliperodri
left a comment
There was a problem hiding this comment.
Logic LGTM — correctly generates states from both ends since EscapeDefault is double-ended. One thing before merge: format-check is failing in CI, please run ./scripts/kani-fmt.sh and push the fix. Approving on the assumption that's just a formatting nit.
|
@acearyanarun could you tackle the merge conflicts and the CI failures so we get this ready to merge? |
|
@acearyanarun @Tianshu-Huang what is the status of this PR? |
Hello Felipe, currently looking at this - will let you know status soon |
Merge model-checking/kani main into arbitrary-ascii-escape-default. - library/kani/src/arbitrary.rs: upstream (model-checking#4784) added an Arbitrary impl for std::char::EscapeUnicode at the same insertion point as this PR's std::ascii::EscapeDefault impl. Keep both; EscapeDefault impl unchanged. - escape_default.sh: drop blank line between shebang and copyright header so scripts/ci/copyright_check.py (run by the format-check job) passes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TnvQ2C8B2KNqKS4SkN3TG3
Head branch was pushed to by a user without write access
|
Hello @feliperodri , I resolved the arbitrary.rs conflict against current main, fixed the EscapeDefault script formatting issue, ran ./scripts/kani-fmt.sh,and both targeted EscapeDefault tests pass locally. Please take a look when you get the chance. |
feliperodri
left a comment
There was a problem hiding this comment.
Re-checked after the merge commit: the implementation is unchanged, and it still verifies.
One gap in the test. tests/kani mode doesn't fail on an unsatisfied kani::cover!, so the direction-specific covers can't catch the regression they were added for. With the whole next_back() block deleted from any(), 6 of 8 covers are satisfied, but the harness still reports SUCCESSFUL and the test passes. Moving the proof to tests/expected/arbitrary/escape_default.rs, with an .expected file that pins 8 of 8 cover properties satisfied (as duration.expected does), makes it fail on exactly that mutation. I checked both ways. tests/kani/char_escape_unicode.rs has the same gap, so no need to fix that here.
…lling Address review feedback on model-checking#4782: - Move tests/kani/ascii_escape_default.rs to tests/expected/arbitrary/escape_default.rs with an .expected file that requires all 8 cover properties to be satisfied, so unsatisfied covers (e.g. if next_back() generation is removed) fail the test. - Add a comment explaining that front/back consumption is unrolled to avoid requiring loop unwinding when generating an arbitrary value.
Head branch was pushed to by a user without write access
|
@feliperodri Addressed. I moved the EscapeDefault test to the tests/expected suite and added an .expected file that requires all 8 cover properties to be satisfied hence removing the next_back() generation now causes the test to fail. I also added the comment regarding why the front/back steps are intentionally unrolled to avoid loop unwinding. Also, noted, didn't touch char_escape_unicode.rs |
9ff2722
Summary
Adds an
Arbitraryimplementation forstd::ascii::EscapeDefault.EscapeDefaultwas previously unsupported by Autoharness because Kani could not generate arbitrary values for the type. This implementation constructs a validEscapeDefaultand nondeterministically consumes elements from the front and back so that Kani can represent fresh, partially consumed, and exhausted iterator states.This allows Autoharness to automatically generate and verify harnesses for functions that take
std::ascii::EscapeDefaultas an argument.Testing
Added a Kani proof that checks generated
EscapeDefaultvalues have at most four remaining elements and confirms that all remaining lengths from 0 through 4 are reachable.Local verification with CBMC 6.11.0:
VERIFICATION:- SUCCESSFULAlso added a script-based Autoharness regression test confirming that:
consume_escape_default(std::ascii::EscapeDefault)is selected by Autoharness and successfully verified.
The
script-based-preregression suite was also run successfully during development.Motivation
The Autoharness analyzer reports functions being skipped when argument types do not implement
Arbitrary. Adding support forEscapeDefaultremoves that limitation for this type and increases the set of functions eligible for automatic verification.