-
Notifications
You must be signed in to change notification settings - Fork 121
Add support for function contracts in cprover_bindings. #1302
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
Add support for function contracts in cprover_bindings. #1302
Conversation
Can you push this to a feature branch for now? |
I thought this was a mergeable unit, or nearly so, so I requested the PR. I could see merging this as-is, or possibly with a unit test for the irep generation added. We don't yet have many (any?) unit tests for cprover_bindings, so this might be the first, but we do actually want to get tests against this crate and have to start somewhere. :) |
We already have unit tests in this crate. It would be great if we can add some food this code. I would also suggest we tag the new functions with allow_dead with a link to the issue we're trying to solve. |
I note that as of moments ago diffblue/cbmc#6799 is merged, which means that function contracts now live in symbols of their own. |
Thanks for linking the PR, @tautschnig! I will update my code accordingly. |
Summary of changes:
|
cprover_bindings/src/goto_program/symtab_transformer/transformer.rs
Outdated
Show resolved
Hide resolved
cprover_bindings/src/goto_program/symtab_transformer/transformer.rs
Outdated
Show resolved
Hide resolved
@@ -104,6 +146,12 @@ pub struct Parameter { | |||
base_name: Option<InternedString>, | |||
} | |||
|
|||
/// Type for function contracts, loop contracts, etc. | |||
#[derive(Debug, Clone)] | |||
pub enum Contract { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be an enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it an enum to support different types of contracts. Like LoopContract(...)
in the future. Is there an alternative way to do this?
…ration for contracts.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
@@ -0,0 +1,56 @@ | |||
// Copyright Kani Contributors | |||
// SPDX-License-Identifier: Apache-2.0 OR MIT | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File comment to explain what contracts are and link to documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment + CBMC documentation link.
let name = name.into(); | ||
// Both base name and pretty name contain the name of the function that the contract is written for. | ||
let base_name: InternedString = base_name.into(); | ||
let pretty_name = base_name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be the pretty name of the function? (We might have discussed this already)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the CBMC side, the pretty_name
is still the name of the function and not the name of the contract. Should I set it to the contract's name instead?
id: IrepId::Lambda, | ||
sub: vec![ | ||
tuple_irep(binding_variables, mm) | ||
// For binding expressions (quantifies, let, lambda), the type of the `tuple_exprt` is set to just its ID in CBMC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there other cases where this is different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tuple_exprt
has a constructor with a type
argument. So, in CBMC, we can construct tuples with a specific type (and not just the ID). The binding_exprt
that sits on top of a tuple_exprt
always sets the type to be the ID.
…ng#1302) * Add type for "spec_ensures" and "spec_requires" clauses to support function contracts * Add transfomer code for code_with_contract; Fix comments; Minor implementation fixes * Revert "Add transfomer code for code_with_contract; Fix comments; Minor implementation fixes" This reverts commit f4afc15. * Revert "Add type for "spec_ensures" and "spec_requires" clauses to support function contracts" This reverts commit f7171f8. * Move function contract to Symbol * Revert "Move function contract to Symbol" This reverts commit ffd3ad2. * Add contract to SymbolValues * Update SymbolValues::Contract to have expressions * Add Contract type; Add Lambda and Tuple to Expr * WIP: Add LambdaExpression and Tuple to Expr; Add MathematicalFunction to Type; Generate Ireps * Update comment to explain pretty_name in contract symbol. * Change type to slice * Remove variables from FunctionContract type * Add type checking to the lambda_expression constructor * Change variables field in lambda expression to variables_tuple to avoid misreading the type * Remove type annotations * Remove LambdaExpression and Tuple * Remove FunctionContract type * Remove contracts * WIP:Move contracts to a separate module; Add spec to handle Irep generation for contracts. * WIP: Update spec to include location; Add comments * WIP: Add comments * WIP: Add location to spec * WIP: remove whitespace * WIP: Remove whitespace * Add file comment * Change Spec to always have Location * format Co-authored-by: Monal Narasimhamurthy <[email protected]>
…ng#1302) * Add type for "spec_ensures" and "spec_requires" clauses to support function contracts * Add transfomer code for code_with_contract; Fix comments; Minor implementation fixes * Revert "Add transfomer code for code_with_contract; Fix comments; Minor implementation fixes" This reverts commit f4afc15. * Revert "Add type for "spec_ensures" and "spec_requires" clauses to support function contracts" This reverts commit f7171f8. * Move function contract to Symbol * Revert "Move function contract to Symbol" This reverts commit ffd3ad2. * Add contract to SymbolValues * Update SymbolValues::Contract to have expressions * Add Contract type; Add Lambda and Tuple to Expr * WIP: Add LambdaExpression and Tuple to Expr; Add MathematicalFunction to Type; Generate Ireps * Update comment to explain pretty_name in contract symbol. * Change type to slice * Remove variables from FunctionContract type * Add type checking to the lambda_expression constructor * Change variables field in lambda expression to variables_tuple to avoid misreading the type * Remove type annotations * Remove LambdaExpression and Tuple * Remove FunctionContract type * Remove contracts * WIP:Move contracts to a separate module; Add spec to handle Irep generation for contracts. * WIP: Update spec to include location; Add comments * WIP: Add comments * WIP: Add location to spec * WIP: remove whitespace * WIP: Remove whitespace * Add file comment * Change Spec to always have Location * format Co-authored-by: Monal Narasimhamurthy <[email protected]>
Description of changes:
Implement
CodeWithContract
type incprover_bindings
to generate Ireps of typeCode
but with additional fields for handling function contracts. Adds the#spec_requires
and#spec_ensures
clauses to the Irep.Call-outs:
Added a new type
Spec
with fieldclauses:Vec<Expr>
to store a list of boolean expressions that would serve as pre (or post) conditions on the function. This allows us to handle required#[PartialEq]
trait for the clauses.Testing:
How is this change tested?
By running kani regression tests. As I haven't added the
#[kani::requires]
and#[kani::ensures]
macros on the Rust side yet, it is not straightforward to add additional test cases right now.Is this a refactor change?
No.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.