Implement soundness checking & corresponding diagnostics in lsp#103
Implement soundness checking & corresponding diagnostics in lsp#103umutdural wants to merge 6 commits intomoves-rwth:mainfrom
Conversation
Philipp15b
left a comment
There was a problem hiding this comment.
Thanks! I left a bunch of comments.
| .into_iter() | ||
| .flat_map(|item| { | ||
| item.flat_map(|unit| CoreVerifyTask::from_source_unit(unit, &mut depgraph)) | ||
| let soundness_blame = soundness_map.get(item.name()).cloned().unwrap_or_default(); |
There was a problem hiding this comment.
How does this map work? Is it really ever not a bug if an item has no associated SoundnessBlame?
src/driver/commands/verify.rs
Outdated
| // (depending on options). | ||
| let vc_is_valid = lower_quant_prove_task(options, &limits_ref, &mut tcx, name, vc_expr)?; | ||
|
|
||
| let soundness_blame = verify_unit.soundness_blame.take().unwrap_or_default(); |
There was a problem hiding this comment.
Why the .take() here? Do we really want to modify the VerifyUnit here?
src/driver/core_verify.rs
Outdated
| pub deps: Dependencies, | ||
| pub direction: Direction, | ||
| pub block: Block, | ||
| pub soundness_blame: Option<SoundnessBlame>, |
There was a problem hiding this comment.
If this is Option, it needs a comment for explanation.
| #[derive(Debug, Clone)] | ||
| pub struct ApproximationListEntry { | ||
| pub span: Span, | ||
| pub is_loop: bool, |
There was a problem hiding this comment.
To avoid the infamous Boolean blindness, I'd suggest creating something like a StmtKindName enum here, with two values: Stmt and Loop. This type should implement Display accordingly.
| } | ||
| pub type ApproximationList = Vec<ApproximationListEntry>; | ||
|
|
||
| // pub type SoundnessBlame = (Soundness, ApproximationList); |
src/proof_rules/mod.rs
Outdated
| if let Some(ident) = proc.calculus.as_ref() { | ||
| match tcx.get(*ident) { | ||
| Some(decl) => { | ||
| // If the declaration is a calculus annotation, return it |
src/vc/explain.rs
Outdated
| stmt_span, | ||
| call_span, | ||
| direction, | ||
| calculus: None, |
|
|
||
| // pub type SoundnessBlame = (Soundness, ApproximationList); | ||
|
|
||
| #[derive(Debug, Clone, Default)] |
There was a problem hiding this comment.
I think we might be able to simplify the type design here. SoundnessBlame is a very open type, and can be invalidated by everyone easily. I propose a slight refactoring, making a lot of the computations internal to one type.
- Unify
SoundnessBlameandSoundness, new nameProcSoundness. - The
soundnessfield is replaced by two fieldssound_proofs,sound_refutationsof type bool. - All fields are private by default, with accessor methods.
- Add methods to this type to generate diagnostics (c.f. https://github.com/moves-rwth/caesar/pull/103/files#r2527526668)
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] | ||
| pub enum ApproximationKind { |
There was a problem hiding this comment.
A possible idea: change this to be a struct with two fields, under, and over of type bool. The infimum and supremum operations become field-wise && and || operations, and you can even overload the built-in (bitwise) operators instead (e.g. https://doc.rust-lang.org/core/ops/trait.BitAnd.html).
(that would be wrong!)partial_cmp can now be derived automatically by the compiler!
You can still have constants like ApproximationKind::EXACT, ApproximationKind::UNKNOWN declared.
Also, you can implement the Sum trait, and have that do the infimum operation. I think it's intuitive that that operation does the right thing here (i.e. folding with infimum).
| "There must not be any statements after this annotated statement (and the annotated statement must not be nested in a block).", | ||
| )) | ||
| } | ||
| AnnotationUnsoundnessError::CalculusEncodingMismatch{direction, span, calculus_name, enc_name } => { |
There was a problem hiding this comment.
Do we still have a corresponding test now for this case?
Philipp15b
left a comment
There was a problem hiding this comment.
Left a bunch of comments.
| } | ||
| } | ||
|
|
||
| impl std::ops::Deref for ApproximationList { |
There was a problem hiding this comment.
I consider these Deref and DerefMut impls a code smell. Either we abstract from the underlying data type and implement necessary operations like push on ApproximationList itself, OR we have Deref and DerefMut impls.
| calculus: Option<Calculus>, | ||
| ) -> Self { | ||
| let approx = approximations.infimum(); | ||
| // The mapping between approximation kinds and soundness is as follows: |
There was a problem hiding this comment.
It cannot be this complicated. In particular, the XORs are impossible to read and explain. I suggest explicit if-then-else chains that make the code overall readable.
| (_, ApproximationKind::Unknown) => Some(std::cmp::Ordering::Greater), | ||
| (ApproximationKind::Under, ApproximationKind::Over) | ||
| | (ApproximationKind::Over, ApproximationKind::Under) => None, | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] |
There was a problem hiding this comment.
This needs a very detailed doc comment explaining what the fields mean. Those fields can also be public, with proper comments like "if under is true, then the vp semantics under-approximates the original programs semantics".
| over: bool, | ||
| } | ||
|
|
||
| // Note that the operations are reversed because the exact approximation should be the top element in the lattice but we want it to be (false,false). |
There was a problem hiding this comment.
I don't understand at all why it's this way. over = true should mean that e.g. vp[S] >= wp[S]. And the bitwise and implementation must be the obvious one, i.e. pairwise conjunction.
As I understand it now, ApproximationKind seems to be the flipped lattice of the one you put in the PR description.
| Refutation, | ||
| Unknown, | ||
| impl ApproximationKind { | ||
| pub const EXACT: Self = Self { |
There was a problem hiding this comment.
These constants need doc comments as well.
| )); | ||
| acc | ||
| }) | ||
| block |
There was a problem hiding this comment.
I think the cleaner version of this would be to do a map first, and then flatten and then collect the result into the ApproximationList, no?
At this point, since ApproximationList doesn't seem to carry any guarantees, a no-newtype simple Vec type alias for ApproximationList would make this easier.
| // Composite statements are handled recursively to collect approximations from sub-statements | ||
| StmtKind::Seq(stmts) => stmts | ||
| .iter() | ||
| .fold(ApproximationList::default(), |mut acc, stmt| { |
There was a problem hiding this comment.
Same comment about map and flatten here
|
|
||
| let approx_list = track_approximation_in_block(block, direction, calculus, tcx); | ||
| let infimum_approx = infimum_approximation_list(&approx_list); | ||
| let infimum_approx = approx_list.infimum(); |
There was a problem hiding this comment.
This needs a comment on what happens with an empty approximation list (also on that function's doc comment).
| soundness: Soundness::from_approximation(infimum_approx, direction), | ||
| return Some(ProcSoundness { | ||
| sound_proofs: match direction { | ||
| Direction::Down => infimum_approx == ApproximationKind::UNDER, |
There was a problem hiding this comment.
This can't be right. What if it's ApproximationKind::EXACT?
| Some(std::cmp::Ordering::Greater) => self, | ||
| None => ApproximationKind::Exact, // If they are incomparable, return Exact (which is the top element) | ||
| impl ApproximationList { | ||
| pub fn infimum(&self) -> ApproximationKind { |
There was a problem hiding this comment.
I suggest adding Rustdoc here, and also a bunch of Documentation tests to explain this, esp. with the list of zero items, one item, and two/three.
…d ProcSoundness logic
| pub kind: ApproximationKind, | ||
| } | ||
| // #[derive(Debug, Clone, Default)] | ||
| // pub struct ApproximationList(pub Vec<ApproximationRecord>); |
| calculus, | ||
| } | ||
| } | ||
| /// Get whether proofs for this procedure are sound. |
| /// See also [`infer_fixpoint_semantics_kind`] for more details on how the semantics kind is inferred. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] | ||
| pub struct ApproximationKind { | ||
| /// True if the vc semantics under-approximates the original program semantics |
| } | ||
|
|
||
| impl ApproximationKind { | ||
| /// vc is both under- and over-approximating the original program semantics |
| /// The original program semantics is based on the calculus annotation or the default fixpoint semantics for loops based on the direction and the encoding used. | ||
| /// | ||
| /// See also [`infer_fixpoint_semantics_kind`] for more details on how the semantics kind is inferred. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] |
This PR implements the soundness checking described in #75.
Approximation kinds form a complete lattice, illustrated by the following diagram:
graph TD; Exact-->Over; Exact-->Under; Over-->Unknown; Under-->Unknown;Each encoding annotation may approximate the actual program behavior using one of the approximation kinds shown above. The overall approximation of a block of statements is the infimum of the approximations of all statements in the block. For example if there are both under- and over-approximating statements in a block, the block has the approximation kind
Unknown. But if every statement is under-approximating then the block also under-approximates.To determine how an encoding annotation approximates, the (optional) calculus annotation of the surrounding procedure, the direction, and the encoding itself are important. Depending on these three aspects, the fixed-point semantics of the annotated loop are determined. Using a function attached to each encoding annotation, we can then decide how each encoding approximates based on the chosen fixed-point semantics and the approximation of its body.
Finally by looking the direction of the proc and the approximation kind of its body we choose a soundness type.
Soundness::Exactmeans that there is no approximation in the proc so any verification result or counter example is sound.Soundness::Proofmeans that the verification result is sound when it is a proof, i.e., if the proc verifies. If the result is a counter-example then this counter-example might be spurious.Soundness::Refutationmeans that the verification result is sound when it is a counter-example. If the program verifies it might be spurious and the original program might not actually verify.Soundness::Unknownmeans that we can not give guarantees about the soundness of both proofs and refutations.Closes #75.