-
Notifications
You must be signed in to change notification settings - Fork 92
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
RFC Function Contracts #2620
RFC Function Contracts #2620
Conversation
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.
I like this structure much better! Thanks.
I haven't finished reviewing it yet. I'll keep looking at it later today.
I do have two things I would like to discuss:
- Our previous contract implementation uses
#[kani::modifies]
instead ofassigns
. I like that better. - I think it should be up to the user to verify their contracts. We shouldn't enforce it inside Kani, especially for contracts defined in a dependency.
Co-authored-by: Celina G. Val <celinval@amazon.com>
Co-authored-by: Celina G. Val <celinval@amazon.com>
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.
Please don't forget to change the summary.md file
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 "User Experience" section is structured in a way that makes it a little hard-to-follow. It seems to me that you're trying to tackle three problems at once: specification, checking and replacement. Maybe each could have their own subsection instead of being itemized?
There's a similar issue with "Rationale and alternatives" and "Future possibilities", although I haven't carefully read them yet.
Co-authored-by: Celina G. Val <celinval@amazon.com> Co-authored-by: Adrian Palacios <73246657+adpaco-aws@users.noreply.github.com>
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.
Thanks @JustusAdam , it's looking great! Only a couple technical comments, the rest are more related to writing.
Co-authored-by: Celina G. Val <celinval@amazon.com> Co-authored-by: Adrian Palacios <73246657+adpaco-aws@users.noreply.github.com>
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.
Almost there. :)
Co-authored-by: Celina G. Val <celinval@amazon.com>
BTW, please wait for a second approval before merging. Thanks! |
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.
Thanks @JustusAdam ! Please take care of unresolved comments before merging.
I'm going to wait for @remi-delmas-3000 to give it another look because there are some unresolved comments before merging. |
Co-authored-by: Celina G. Val <celinval@amazon.com> Co-authored-by: Adrian Palacios <73246657+adpaco-aws@users.noreply.github.com>
Rendered
Proposes the addition of function contracts and mechanisms to check them and use them as modular abstractions.