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

Revert 'Eliminate top level existentials in side-condtions' #3620

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

goodlyrottenapple
Copy link
Contributor

This PR fixes #3605
The bug was introduced by #3202, which eliminated top level existentials in side-conditions. However, this was incorrect because when translated to Z3, these variables would get a universal quantification, which is what was happening in #3605

github-actions and others added 2 commits July 12, 2023 14:31
@@ -773,7 +773,7 @@ checkSimpleImplication inLeft inRight existentials =
rhsBottom <-
fmap isBottom . liftSimplifier $
SMT.Evaluator.filterMultiOr $srcLoc
=<< Pattern.simplify right
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this didn't end up being the issue, but it's not correct and should be changed nonetheless.

jberthold
jberthold previously approved these changes Jul 12, 2023
Copy link
Member

@jberthold jberthold left a comment

Choose a reason for hiding this comment

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

LGTM, but the RPC integration tests need to be adapted.

@jberthold
Copy link
Member

As it turns out, one of the RPC server implies tests is using a term that hits an error call now.
We probably just want to use a different test but it would be interesting to understand the restriction in detail.

@goodlyrottenapple
Copy link
Contributor Author

As it turns out, one of the RPC server implies tests is using a term that hits an error call now. We probably just want to use a different test but it would be interesting to understand the restriction in detail.

Does the test make sense in the first place? X and Z are not booleans but rather of sort K, so the back-end infers that Z is the term and #Not (X #Equals Z) is the predicate, hence the error.

@jberthold
Copy link
Member

As it turns out, one of the RPC server implies tests is using a term that hits an error call now. We probably just want to use a different test but it would be interesting to understand the restriction in detail.

Does the test make sense in the first place? X and Z are not booleans but rather of sort K, so the back-end infers that Z is the term and #Not (X #Equals Z) is the predicate, hence the error.

Yes, that's what happens. Just not sure why it would be such a bad thing to have an existential quantification over both term and predicate...
But I was meaning to suggest that we modify the test and move on.

@jberthold jberthold self-requested a review July 14, 2023 00:10
@jberthold jberthold dismissed their stale review July 14, 2023 00:10

Foundry proofs showed problems

@jberthold
Copy link
Member

jberthold commented Jul 14, 2023

Unfortunately, the "error somewhere else" is triggered by foundry proofs (I tested GasTest.testInfiniteGas which introduces an existential gas value).
bug-report-stripped.tar.gz is an RPC interaction log for this proof, failing in request 6.
EDIT: actually any proof in tests/foundry is failing, on the initial implies request. The consequent of that request contains an array of existentials that quantify free variables in the target state.

@geo2a geo2a removed their request for review November 25, 2024 07:48
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.

Possible implies endpoint not giving correct result
2 participants