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

ruleOfProp is a bit clunky #1261

Open
robdockins opened this issue Mar 11, 2021 · 8 comments
Open

ruleOfProp is a bit clunky #1261

robdockins opened this issue Mar 11, 2021 · 8 comments
Labels
subsystem: saw-core Issues related to the saw-core representation or the saw-core subsystem type: bug Issues reporting bugs or unexpected/unwanted behavior unsoundness Issues that can lead to unsoundness or false verification
Milestone

Comments

@robdockins
Copy link
Contributor

The ruleOfProp operation examines the statement of a proposition and attempts to determine if it can be used as a rewrite rule. However, it is has a number of problems we should find some way to address.

  1. It is unsound. For example, if it encounters a term vecEq n a f x y it will assume this proves the equality of x and y regardless of what function f is supplied. Similar for pairEq.
  2. It is incomplete. Some of the primitive equality-testing functions are missing, (e.g. intEq, intModEq), so these will not be recognized as rewrites.
  3. It is error-prone. Item 2 seems pretty directly related to the fact that ruleOfProp must be updated as we add new equality predicates on new types, and that hasn't been done yet.

Possible solution:
I think we would be better off restricting the class of theorems that are accepted as rewrite rules to address soundness. E.g., we should only accept statements of the form Eq a x y or ecEq a peq x y. Then, add axioms for converting Eq a into the various primitive equality tests, and apply them when proving the equations to start with. E.g.

axiom boolEq_Eq : (x y : Bool) -> EqTrue (boolEq x y) -> Eq Bool x y
axiom intEq_Eq : (x y : Integer) -> EqTrue (intEq x y) -> Eq Integer x y
axiom vecEq_eq: (n : Nat) -> (a : sort 0) -> (f : a -> a Bool) ->
   ( (x y : a) -> EqTrue (f x y) -> Eq a x y) ->
   ( x y : Vec n a ) -> EqTrue (vecEq n a f x y) -> Eq (Vec n a) x y
@brianhuffman
Copy link
Contributor

My comment from #110 is relevant: #110 (comment)

Another thing that bothers me about ruleOfProp is that it has the names of several equality operators like boolEq, bvEq, and vecEq hard-coded into it. It would be better if the SharedContext could refer to a set of rules/axioms for turning propositions into equations, and then ruleOfProp could work generically over this set. For example, there would be a rule like (n : Nat) -> (x y : Vec n Bool) -> EqTrue (bvEq n x y) -> Eq (Vec n Bool) x y for bvEq, and if ruleOfProp was given a theorem matching the pattern EqTrue (bvEq _ _ _) then it could be composed with this rule to produce an Eq theorem that could work directly as a rewrite rule.

If ruleOfProp was implemented this way, then we could easily extend ruleOfProp to handle new forms of theorems later, even at runtime.

This is exactly the same idea proposed above, so yes, of course I support it completely. To facilitate implementation, it would be nice to have some combinators for composing theorems with resolution/modus ponens, sort of like the OF and COMP operators in Isabelle.

Also I would not hard-code a special exception for ecEq; we can add some definitions and rules like these to Cryptol.sawcore

ValidPEq : (a : sort 0) -> PEq a -> Prop;
ValidPEq a pa = (x y : a) -> ecEq a pa x y -> Eq a x y;

ecEq_Eq : (a : sort 0) -> (pa : PEq a) -> (x y : a) -> ValidPEq a pa -> EqTrue (ecEq a pa x y) -> Eq a x y;
ecEq_Eq a pa x y valid e = ...

along with some axioms (or proved theorems) stating that the various PEq dictionary combinators preserve the ValidPEq property.

@robdockins
Copy link
Contributor Author

I missed that discussion before. Yeah, it sounds like we are largely in agreement.

We could have the PEq class carry its correctness property, which would make it pretty easy to access.

@brianhuffman
Copy link
Contributor

Putting the correctness property inside the dictionary would be pretty handy. Are there any potential drawbacks to doing that? The only thing I can think of is that putting a dependent type like that inside PEq might make it harder to export certain terms to certain backends? And I guess we'd have to define PEq as a saw-core datatype instead of a (non-dependent) record type (not a big deal).

@robdockins
Copy link
Contributor Author

I suppose that might be a problem if we wanted to target HOL or something, although I think we're already in pretty big trouble if we want to do that.

I forgot that saw-core records are necessarily non-dependent. That might be a little inconvenient. I think there are a few places where Verifier.SAW.Cryptol assumes dictionaries are records, but we can probably work around that.

@robdockins
Copy link
Contributor Author

robdockins commented Mar 12, 2021

I guess the other problem is that we wouldn't be able to have any law-breaking PEq instances. The problematic issues with Float equality would be an issue here. Edit: on second thought, maybe it isn't an issue for floats, as long as we phrase it as an implication instead of an equivalence.

@brianhuffman
Copy link
Contributor

I think it actually is an issue for floats, isn't it? We have +0 == -0 for floats, but those values are not logically equal, so the implication doesn't hold. We should probably just make a separate predicate for law-abiding Eq instances, then.

@brianhuffman
Copy link
Contributor

I had a go at implementing some validity rules for equality predicates, which you can see on the ValidEq branch (af4fe14186235a6387ae8ad73e4c703140d5c18d).

@robdockins
Copy link
Contributor Author

I think it actually is an issue for floats, isn't it? We have +0 == -0 for floats, but those values are not logically equal, so the implication doesn't hold. We should probably just make a separate predicate for law-abiding Eq instances, then.

Yes, you're right. I was only thinking about NaN, but issue with signed 0 goes the other way.

@brianhuffman brianhuffman transferred this issue from GaloisInc/saw-core Apr 27, 2021
@brianhuffman brianhuffman added the subsystem: saw-core Issues related to the saw-core representation or the saw-core subsystem label Apr 27, 2021
@brianhuffman brianhuffman self-assigned this May 7, 2021
brianhuffman pushed a commit that referenced this issue May 17, 2021
This is a band-aid pending a proper solution for #1261.
brianhuffman pushed a commit that referenced this issue Aug 26, 2021
This is a band-aid pending a proper solution for #1261.
@brianhuffman brianhuffman removed their assignment Apr 28, 2022
@robdockins robdockins self-assigned this Apr 28, 2022
@robdockins robdockins removed their assignment Aug 30, 2022
@sauclovian-g sauclovian-g added type: bug Issues reporting bugs or unexpected/unwanted behavior unsoundness Issues that can lead to unsoundness or false verification labels Oct 30, 2024
@sauclovian-g sauclovian-g added this to the 2025T1 milestone Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
subsystem: saw-core Issues related to the saw-core representation or the saw-core subsystem type: bug Issues reporting bugs or unexpected/unwanted behavior unsoundness Issues that can lead to unsoundness or false verification
Projects
None yet
Development

No branches or pull requests

3 participants