Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rethink should_panic and fail_uncoverable options as global conditions #2967

Merged

Conversation

adpaco-aws
Copy link
Contributor

This PR is the next step to rework/introduce the should_panic/fail_uncoverable options as global conditions.

Until now, we haven't had a concrete proposal to do so other than the implementation in #2532. This PR presents one for each option in their respective RFCs. I'd like to agree on this design before starting the code review for #2532.

Related to #1905 #2272 #2299 #2516

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@adpaco-aws adpaco-aws requested a review from a team as a code owner January 3, 2024 23:16
@adpaco-aws adpaco-aws added the T-RFC Label RFC PRs and Issues label Jan 3, 2024
Copy link
Contributor

@zhassan-aws zhassan-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. How will this interact with the cover_or_fail macro proposed in #2792?

@adpaco-aws
Copy link
Contributor Author

adpaco-aws commented Jan 8, 2024

Looks good. How will this interact with the cover_or_fail macro proposed in #2792?

Thank you!

Good question. First of all, the interaction would depend on how the macro is implemented. I'm not aware of any specific proposal to do that, but I'm happy to discuss it considering the two options I've thought about. So please take what follows with a grain of salt because I may not be considering all options/cases.

The first option is to implement cover_or_fail through a special property class (e.g., cover_fail) which can hold a Failure status. In this case, I don't think there would be any conflicts because should_panic only cares about assertion properties and fail_uncoverable does the same for cover properties.

Another option is to have cover_or_fail enable a different global condition which checks that all cover_or_fail properties are indeed covered. At present, failing any global condition implies a verification failure. Again, I don't think there would be conflicts (because of the properties that each condition cares about) but this really depends on the implementation.

Also, I wanted to point out that, assuming fail_uncoverable worked in the way it's being described, I believe one could use it to achieve the desired outcome in #2792 (i.e., the second test would fail). That's because fail_uncoverable would essentially transform all reachable cover macros into cover_or_fail ones. But it's not enough for the general case since other cover statements may be present.

Copy link
Contributor

@zhassan-aws zhassan-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

@adpaco-aws adpaco-aws enabled auto-merge (squash) January 12, 2024 18:26
@adpaco-aws adpaco-aws merged commit fd12a28 into model-checking:main Jan 12, 2024
21 checks passed
qinheping added a commit that referenced this pull request Jan 24, 2024
These are the auto-generated release notes for comparison purposes:

## What's Changed
* Rethink `should_panic` and `fail_uncoverable` options as global
conditions by @adpaco-aws in
#2967
* Upgrade toolchain to nightly-2024-01-17 by @celinval in
#2976
* Benchcomp visualize: fix missing import by @tautschnig in
#2977
* Cargo update 2024-01-18 by @remi-delmas-3000 in
#2978

**Full Changelog**:
kani-0.44.0...kani-0.45.0

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
qinheping added a commit to qinheping/kani that referenced this pull request Jan 26, 2024
These are the auto-generated release notes for comparison purposes:

## What's Changed
* Rethink `should_panic` and `fail_uncoverable` options as global
conditions by @adpaco-aws in
model-checking#2967
* Upgrade toolchain to nightly-2024-01-17 by @celinval in
model-checking#2976
* Benchcomp visualize: fix missing import by @tautschnig in
model-checking#2977
* Cargo update 2024-01-18 by @remi-delmas-3000 in
model-checking#2978

**Full Changelog**:
model-checking/kani@kani-0.44.0...kani-0.45.0

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
@adpaco-aws adpaco-aws mentioned this pull request Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-RFC Label RFC PRs and Issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants