Preserve more Cryptol numeric predicates in SAWCore types - #3202
Preserve more Cryptol numeric predicates in SAWCore types#3202brianhuffman wants to merge 17 commits into
Conversation
a91522a to
dd64670
Compare
|
The SAW-exercises test is failing on a call to |
6bdb5ae to
48c5597
Compare
| Ltac solveUnsafeAssumePGEq := | ||
| reflexivity. | ||
|
|
||
| Ltac solveUnsafeAssumePNeq := | ||
| reflexivity. |
There was a problem hiding this comment.
I'm somewhat surprised that reflexivity is enough to solver some of the trickier type-level inequality constraints that appear in Cryptol type signatures. I would have figured that something more sophisticated (e.g., lia) would have been required!
There was a problem hiding this comment.
It's not. I only expect reflexivity to be able to discharge constraints on concrete type arguments. It appears that the SAW test suite doesn't require anything more than that at the moment, but as soon as we exercise it a bit harder, I'm sure we'll need to replace it with something more sophisticated.
There was a problem hiding this comment.
Also, at that point I'll need to recruit someone with much more experience with Ltac programming than I have.
There was a problem hiding this comment.
I'm a bit surprised that there's nothing in the test suite currently that requires anything more sophisticated than just reflexivity. Surely it shouldn't be that difficult to cook up a test case where this falls over?
In any case, I'm still worried about this, as I think that this could technically constitute a regression in saw-core-rocq's ability to translate definitions involving type-level equality/inequality constraints. One possible solution here: since the names of these tactics involve the word "unsafe", perhaps they should just fall back on the admit tactic if reflexivity/lia/etc. isn't enough to solve the current goal?
There was a problem hiding this comment.
Changing these to try contradiction; try discriminate; auto; try lia would increase the set of things they work on automatically without costing much.
Some things are never going to work automatically, like exponents, and if I understand properly the idea is to have ltac generate the translated term in the middle of a bunch of other stuff, in which case using admit is the only simple way to make it not just croak when loading it.
There was a problem hiding this comment.
I think that the current implementation of solveUnsafeAssert may actually suffice for solveUnsafeAssume{PFin,PGEq,PNeq} as well. I suspect that most {PFin,PGEq,PNeq} proof goals will involve equality proofs over type-level numbers, which is exactly what solveUnsafeAssert is designed to solve. The only additions that we might need to make are to teach solveUnsafeAssertStep how to "unfold" certain operations when applied to TCNum arguments. For instance, you'll note the following cases in solveUnsafeAssertStep:
These exist so that we can unfold proof goals about Nums to proof goals about nats, the latter of which the lia tactic is excellent at solving. We may want to add an additional equalNat_eqb case to help simplify PNeq proof goals:
Theorem equalNat_eqb : forall x y, equalNat x y = Nat.eqb x y.And an additional ltNat_ltb case to help simplify PGeq proof goals:
Theorem ltNat_ltb : forall x y, ltNat x y = Nat.ltb x y.There was a problem hiding this comment.
Note that there is more work that could be done in order to teach solveUnsafeAssert how to use PFin/PGeq/PNeq as assumptions, not just as goals to prove. Doing so would be necessary in order to make the generated Rocq code for zext typecheck, for instance (see #3340). That being said, I don't want to scope-creep this PR more, so I'm happy leave that as future work.
There was a problem hiding this comment.
My vote for the moment would be to merge this with minimal changes in the Rocq stuff and then fix it separately, if only to minimize conflicts with what I'm already doing in there.
|
It looks like the only remaining CI failures are caused by #3254. As soon as that's fixed, I should be able to rebase and mark this PR as ready. |
435f2d9 to
6fb9c2d
Compare
| FalseProp_elim : (a : sort 1) -> FalseProp -> a; | ||
| FalseProp_elim a = | ||
| Eq__rec Bool True | ||
| (\ (y : Bool) (_ : Eq Bool True y) -> | ||
| Bool#rec2 (\ (_ : Bool) -> sort 1) TrueProp a y) | ||
| TrueI | ||
| False; |
There was a problem hiding this comment.
Perhaps this definition should be moved to saw-core/prelude/Prelude.sawcore, alongside the definition of FalseProp itself?
| isErasedPC :: C.PC -> Bool | ||
| isErasedPC pc = |
There was a problem hiding this comment.
Is your eventual goal here to remove all of the True cases in this function? If so, would it be worth filing follow-up issues for the remaining True cases (e.g., PPrime)?
There was a problem hiding this comment.
Yes, that's a good idea. I'd like to preserve PPrime and PValidFloat eventually. I'll have to check with @yav, but I suspect that we should never see PHas, PAnd, or PTrue for fully type-checked Cryptol modules; I think Cryptol guarantees that these are always gone by the time type checking finishes. So it's probably reasonable for cryptol-saw-core to panic on those.
There was a problem hiding this comment.
PHas should not appear after type checking, as we should report a type error if we can't simplify it.
However, I think there might be situations where And and True end up in the code. They are generated when we apply substitutions to constraints, and that ends up simplifying the constraints. The reason is that when we do this on already generalized code, it is important that we don't change the number of constraints, as this would change the "API" to the function (i.e., we'd have to also update all call sites). So if a constraint is completely simplified it stays there but becomes true. Otoh, if a constraint ends up simplifying to multiple "simpler" constraints", those are joined with "and", so we still have a single composite constraint.
I haven't tried it, but I think it might happen if you have a functor with a type parameter and then instantiate it with some concrete type.
| Definition maxNat := Nat.max. | ||
| Definition Nat__rec := nat_rect. | ||
|
|
||
| Definition ltNat_0_right (n : nat) : ltNat n Zero = false := eq_refl. |
There was a problem hiding this comment.
Out of curiosity, why is it beneficial to manually define this in Rocq?
There was a problem hiding this comment.
We can't translate the SAW definition of ltNat_0_right directly to Rocq, because it's proved by induction on Nat (i.e. using Nat#ind) and Rocq's nat type isn't defined the same way. Providing a special translation for ltNat_0_right was much easier than writing a Rocq translation rule for Nat#ind. (As mentioned in the comment thread for #1606, the Rocq backend doesn't even have hooks yet for adding translation rules for recursors.)
There was a problem hiding this comment.
I would have thought it would translate to N and not nat, and then it would mostly align...
There was a problem hiding this comment.
Ah, OK! It might be worth leaving a comment explaining why the choice to special-case this was made, as it wasn't obvious to me until reading this explanation.
There was a problem hiding this comment.
Translating Nat to N instead of nat seems like it would be the right thing to do. I think the main thing in the way is our translation of the Vec type: We are currently using a Rocq library with a vector type indexed by nat, so we'd need to find a replacement with a vector type indexed by N.
There was a problem hiding this comment.
There are not a whole lot of bitvector libraries, unfortunately. I have one of my own half done (ongoing personal project since before the pandemic) but it's not really done enough to post in public, let alone use.
There was a problem hiding this comment.
We should be translating to N rather than nat, but apparently we don't. That's going to need to be its own big breaking change.
There was a problem hiding this comment.
I agree. Perhaps we should open an issue about moving from nat to N. (I'd do so myself, although I'm not sure I've fully thought through all of the implications of what this change would bring about.)
| Ltac solveUnsafeAssumePGEq := | ||
| reflexivity. | ||
|
|
||
| Ltac solveUnsafeAssumePNeq := | ||
| reflexivity. |
There was a problem hiding this comment.
I'm a bit surprised that there's nothing in the test suite currently that requires anything more sophisticated than just reflexivity. Surely it shouldn't be that difficult to cook up a test case where this falls over?
In any case, I'm still worried about this, as I think that this could technically constitute a regression in saw-core-rocq's ability to translate definitions involving type-level equality/inequality constraints. One possible solution here: since the names of these tactics involve the word "unsafe", perhaps they should just fall back on the admit tactic if reflexivity/lia/etc. isn't enough to solve the current goal?
6fb9c2d to
7176fa6
Compare
| Definition maxNat := Nat.max. | ||
| Definition Nat__rec := nat_rect. | ||
|
|
||
| Definition ltNat_0_right (n : nat) : ltNat n Zero = false := eq_refl. |
There was a problem hiding this comment.
I would have thought it would translate to N and not nat, and then it would mostly align...
| Ltac solveUnsafeAssumePGEq := | ||
| reflexivity. | ||
|
|
||
| Ltac solveUnsafeAssumePNeq := | ||
| reflexivity. |
There was a problem hiding this comment.
Changing these to try contradiction; try discriminate; auto; try lia would increase the set of things they work on automatically without costing much.
Some things are never going to work automatically, like exponents, and if I understand properly the idea is to have ltac generate the translated term in the middle of a bunch of other stuff, in which case using admit is the only simple way to make it not just croak when loading it.
|
|
||
|
|
||
| Definition sext (m : CryptolPrimitivesForSAWCore.Num) (n : CryptolPrimitivesForSAWCore.Num) (x : CryptolPrimitivesForSAWCore.seq n Init.Datatypes.bool) : CryptolPrimitivesForSAWCore.seq m Init.Datatypes.bool := | ||
| Definition sext (m : CryptolPrimitivesForSAWCore.Num) (n : CryptolPrimitivesForSAWCore.Num) (_P : CryptolPrimitivesForSAWCore.PFin m) (_P1 : CryptolPrimitivesForSAWCore.PGeq m n) (_P2 : CryptolPrimitivesForSAWCore.PGeq n (CryptolPrimitivesForSAWCore.TCNum (Stdlib.PArith.BinPos.Pos.to_nat Stdlib.PArith.BinPos.xH))) (x : CryptolPrimitivesForSAWCore.seq n Init.Datatypes.bool) : CryptolPrimitivesForSAWCore.seq m Init.Datatypes.bool := |
7176fa6 to
1f5cc3f
Compare
c3624e2 to
a7725fc
Compare
`PEqual` is no longer one of the "erased" proposition types in the Cryptol importer, which means that any Cryptol function whose type schema includes a numeric equality constraint will now have that constraint as an argument in its SAWCore type.
This is just for completeness, as `PTrue` should never actually show up in any type-checked Cryptol terms.
`PGeq` is no longer one of the "erased" proposition types in the Cryptol importer, which means that any Cryptol function whose type schema includes a numeric inequality constraint will now have that constraint as an argument in its SAWCore type.
Any Cryptol function whose type schema includes a numeric inequality constraint will now have that constraint as an argument in its SAWCore type.
Any cryptol function whose type includes a `fin` constraint will now have that constraint as an argument in its SAWCore type.
This exercises the import of all Cryptol numeric sequence primitives.
It is replaced by three proposition-specific constants that have actual definitions: * `unsafeAssumePFin` * `unsafeAssumePGeq` * `unsafeAssumePNeq` When evaluated, each of these will force evaluation of its numeric arguments and call `error` if the proposition does not evaluate to true. This ensures that uninterpreted functions will work properly when they have `fin` or inequality constraints.
Also support this lemma in the saw-core-rocq backend.
Rocq did not allow the old `PFin` definition (as an indexed datatype) to be eliminated to a non-Prop type.
If `PFin n` is defined as `Eq Num n PFin -> FalseProp`, then this turns any function with a `PFin` constraint into a higher-order function, causing problems with uninterpreted functions.
Now they are handled consistently with the other `unsafeAssert` primitives already present in the SAWCore prelude.
Courtesy of Ryan Scott.
Cryptol.sawcore has changed.
a7725fc to
544f692
Compare
|
This PR will need to resolve the merge conflicts that came about as a result of |
Previously the Cryptol-to-SAWCore translation erased all Cryptol numeric type constraints (
fin,==,>=,!=,prime) during translation to SAWCore. With this PR, the type-level numeric constraintsfin,==,>=and!=are preserved as SAWCore predicates.This offers multiple benefits:
errorfunctions withinCryptol.sawcoreerror, we rely less on "inhabited sort" constraints in the Rocq backend4 >= nby enumeration of cases.