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

Extensional reasoning on Values #705

Merged
merged 38 commits into from
Oct 9, 2024

Conversation

AlecsFerra
Copy link
Contributor

The extensionality flag fails when a function used as argument of a data constructor, take this as an example:

fixpoint "--rewrite"
fixpoint "--allowho"

constant f : (func(0 , [int; int; int]))
define f (x : int, y: int) : int = {(x + y)}

constant g : (func(0 , [int; int; int]))
define g (a : int, b: int) : int = {(b + a)}


data Ty 0 = [
    | Cons {mkCons : func(0 , [int; int; int])}
]

constant Cons : (func(0 , [func(0 , [int; int; int]); Ty]))

expand [1 : True; 2 : True]

constraint:
  env []
  lhs {VV1 : Tuple | true }
  rhs {VV2 : Tuple | (Cons f = Cons g) }
  id 2 tag []

it cannot prove that f and g are the same function since it cannot add the ext$... variable on the functions (and then cause their unfolding by PLE) since they would make the program ill-typed, the solution is eta-expanding the definitions of f and g to (\x:Int -> \y:Int -> f x y) (\x:Int -> \y:Int -> g x y) to let PLE then unfold the definitions in the body of the lambda and prove the equality.

DO NOT MERGE: This feature is exposing another LF bug in some tests in the liquidhaskell repo related to lambdas in the refinements.

@AlecsFerra AlecsFerra marked this pull request as draft September 13, 2024 15:16
@AlecsFerra
Copy link
Contributor Author

AlecsFerra commented Sep 13, 2024

The issue on the liquid haskell main repo is related to programs like this:

{-@ LIQUID "--reflection" @-}
{-@ LIQUID "--ple"        @-}

module Example where

{-@ reflect idu @-}
{-@ idu :: Int -> Int @-}
idu :: Int -> Int
idu x = x

{-@ proof :: {(\x:Int -> x) = idu } @-}
proof = ()

As it throws crash: SMTLIB2 respSat = Error "line 3 column 18650: unknown constant lam_arg$35$$35$1" whenever a lambda is used this way in the refinements

This error is already present is not caused by this patch

@nikivazou
Copy link
Member

Yes, right now there is no real support for lambdas in LH :)

@AlecsFerra
Copy link
Contributor Author

Uhm ok then I'm stuck

@AlecsFerra
Copy link
Contributor Author

Right now It's verifying the example but it's also making some tests in the LH repo fail because it's unfolding some types and inserting lambdas

@AlecsFerra
Copy link
Contributor Author

I think I fixed the bug regarding lambdas, I'll run the tests on the main repo to double check

@AlecsFerra
Copy link
Contributor Author

The tests on the liquidhaskell are passing!

But I still have to fix a minor bug for my tests

Copy link
Collaborator

@facundominguez facundominguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @AlecsFerra. The changes make some sense to me. I contributed some questions and comments.

src/Language/Fixpoint/Solver/Common.hs Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
@AlecsFerra
Copy link
Contributor Author

AlecsFerra commented Sep 17, 2024

Can confirm that it's passing all the test on the main LH repo, now I'm running the benchmarks

@AlecsFerra
Copy link
Contributor Author

AlecsFerra commented Sep 17, 2024

Benchmarks:
filtered
top
bot

On average we are not extremely slower than then original PLE

Copy link
Collaborator

@facundominguez facundominguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AlecsFerra. I added some comments about points I don't understand yet, but your comments helped me progress with my initial questions. Other than these, I guess only some tests would be necessary.

src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/Common.hs Show resolved Hide resolved
@AlecsFerra
Copy link
Contributor Author

I will squash the commits at the end

src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
src/Language/Fixpoint/Solver/PLE.hs Outdated Show resolved Hide resolved
@AlecsFerra
Copy link
Contributor Author

AlecsFerra commented Oct 4, 2024

Following some discussion in private I decided to remove the code relating to make PLE aware of dependent pattern matching / unification

The code of the reader monad still passes, the SKI calculus instead will require some extra lemmas

@facundominguez
Copy link
Collaborator

facundominguez commented Oct 4, 2024

The only open discussion that I could catch is about making a comment more instructive. Update: oh, I asked about a couple other things in comments later.

Regarding performance, if the verification times worsen when enabling extensionality and --etabeta I'd say it is fine to merge. If verification worsens even when not enabling the flags, then I'd prefer to understand why before merging.

-- Clearly we need the higerorder flag active as we are generating lambda in
-- the constraints.
evalApp _γ _ctx e0 es _et
| ECst (EVar _f) sortAnnotation@FFunc{} <- e0
Copy link
Collaborator

@facundominguez facundominguez Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed this is relying on ECst while the earlier equations look for the function definition in γ. What is the reason to not do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a comment in the code

@AlecsFerra
Copy link
Contributor Author

filtered
bot
top

-- get a term annoptated with type `fun(0, [a#foo, a#foo])`
-- instead of `fun(0, [@foo, @foo])` thus when substituting
-- the body in a term we could encounter type mismatches.
newE <- elaborateExpr "EvalApp unfold full: " $ substEq env eq es1
Copy link
Collaborator

@facundominguez facundominguez Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of the elaboration is going both into the new equalities and into the rewritten term that is returned. I think only the later is necessary, as it is the only place where ECst will be looked for by evalApp.

@AlecsFerra
Copy link
Contributor Author

filtered
bot
top

@AlecsFerra
Copy link
Contributor Author

Looks way better I guess the slowdowns were actually caused by the elaboration

Copy link
Collaborator

@facundominguez facundominguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlecsFerra, would you like to squash or make other edits ahead of merging?

@facundominguez
Copy link
Collaborator

Also, note the comment edits that I just contributed. Feel free to adjust them if you see anything amiss.

@AlecsFerra
Copy link
Contributor Author

Ops

@AlecsFerra
Copy link
Contributor Author

Ok now It's ok to merge

@facundominguez facundominguez merged commit 3ff2766 into ucsd-progsys:develop Oct 9, 2024
17 checks passed
@AlecsFerra AlecsFerra mentioned this pull request Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants