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

Simple ADT test with different behavior with or without push #1008

Open
Stevendeo opened this issue Dec 11, 2023 · 5 comments · May be fixed by #1095
Open

Simple ADT test with different behavior with or without push #1008

Stevendeo opened this issue Dec 11, 2023 · 5 comments · May be fixed by #1095
Assignees
Labels
adt Algebraic data types backlog bug completeness This issue is about completeness of theories instantiation this issue is related to the instantiation mechanism
Milestone

Comments

@Stevendeo
Copy link
Collaborator

Stevendeo commented Dec 11, 2023

The following test have a different behavior given if the push instruction is here or not:

File test_adt.smt2

(set-logic ALL)
(declare-datatype t ((A) (B) (C (i Int))))

(declare-const e t)

(push 1)
(assert (not ((_ is A) e)))
(assert (not ((_ is B) e)))
(assert (not (exists ((n Int)) (= e (C n)))))
(check-sat)

Command:
alt-ergo test_adt.smt2 --enable-assertions --output smtlib2 --sat-solver CDCL --no-minimal-bj

Without the push, alt-ergo returns unsat (as expected); with the push, it never returns.

The culplrit may be the presence of the existential quantifier and --no-minimal-bj; remove the option and everything works fine. This test is a translation of goal g_valid_5_1 of tests/adts/simple_1.ae.

@bclement-ocp
Copy link
Collaborator

The following test have a different behavior given if the push instruction is here or not:

Can you clarify what behavior is different and if either behavior is incorrect/unsound?

@Stevendeo
Copy link
Collaborator Author

Oh you're right I forgot about it: with the push it does not return, while without it returns unsat (as expected). So it's not an unsoundness issue.

Stevendeo added a commit to Stevendeo/alt-ergo that referenced this issue Dec 12, 2023
This test fails when wrapped in a push/pop instruction. Issue OCamlPro#1008 discusses
this as an independent issue.
@bclement-ocp
Copy link
Collaborator

Interesting. I actually don't understand how we prove this in the first place…

Here is a simpler example that we are able to prove:

(set-logic ALL)
(declare-datatype t ((A) (B (i Int))))

(declare-const e t)

(assert ((_ is B) e))
(assert (forall ((n Int)) (distinct e (B n))))
(check-sat)

Here is an even simpler example that we are not able to prove anymore, because the type gets converted to a record:

(set-logic ALL)
(declare-datatype t ((B (i Int))))

(declare-const e t)

(assert ((_ is B) e))
(assert (forall ((n Int)) (distinct e (B n))))
(check-sat)

So the ADT theory must be doing something weird… Pinging @Halbaroth who has been looking at ADTs.

Stevendeo added a commit to Stevendeo/alt-ergo that referenced this issue Dec 12, 2023
This test fails when wrapped in a push/pop instruction. Issue OCamlPro#1008 discusses
this as an independent issue.
@Halbaroth Halbaroth self-assigned this Dec 12, 2023
@Halbaroth Halbaroth added the bug label Dec 12, 2023
Stevendeo added a commit to Stevendeo/alt-ergo that referenced this issue Dec 14, 2023
This test fails when wrapped in a push/pop instruction. Issue OCamlPro#1008 discusses
this as an independent issue.
Stevendeo added a commit to Stevendeo/alt-ergo that referenced this issue Dec 21, 2023
This test fails when wrapped in a push/pop instruction. Issue OCamlPro#1008 discusses
this as an independent issue.
@Halbaroth Halbaroth added this to the 2.6.0 milestone Jun 6, 2024
@Halbaroth
Copy link
Collaborator

Halbaroth commented Jun 17, 2024

I believe there is no soundness bug here. For instance, for the following input file:

(set-logic ALL)
(declare-datatype t ((A) (B (i Int))))

(declare-const e t)

(assert ((_ is B) e))
(assert (forall ((n Int)) (distinct e (B n))))
(check-sat)

Alt-Ergo reaches a contradiction as folllows:

  1. After asserting ((_ is B) e), Alt-Ergo notices that the domain of e is a singleton and so Adt_rel asserts an equation of the form (= (e (B .k0)) where .k0 denotes a fresh integer name. The associated explanation is empty because the equation is asserted by the user.
  2. The instantiation engines matches (B .k0) with the unique trigger (B n) of the axiom (forall ((n Int)) (distinct e (B n))).
  3. The SAT solver learns (distinct e (B .k0)), we got a contradiction!

Now, if we consider the input:

(set-logic ALL)
(declare-datatype t ((B (i Int))))

(declare-const e t)

(assert ((_ is B) e))
(assert (forall ((n Int)) (distinct e (B n))))
(check-sat)

The type t is a record and (_ is B) e is immediately replaced by true in D_cnf. The current implementation of Matching cannot match e with (B n) because the union-find ignores the equality e = (B (i e)).

There are several solutions:

  1. Do nothing as the issue will be solved after merging Remove the Record theory #1095.
  2. Using Record_rel to generate a definition of e of the form (B .k0) as we do in Adt_rel and use Rel.new_terms to send these terms to the matching module.
  3. Allow to match e with (B (i e)) in the matching module.

The last solution is probably the best one because we avoid to add fresh terms for each field of a record.

In the current state of the code, we do not reach a contradiction with the second example, even if we turn on the model generation, because the generated terms by Uf.next_assign are not sent to the matching module:

let do_case_split env origin =
try
let acts = theory_slice env in
let tenv, _terms = Th.do_case_split ~acts env.tenv origin in
(* TODO: terms not added to matching !!! *)
env.tenv <- tenv;
C_none
with Ex.Inconsistent (expl, _) ->
C_theory expl

I am not sure it is a good idea to add these terms to the matching environment for two reasons:

  1. We want to match e with (B (i e)) before reaching the model generation phase
  2. While implementing get-value in Support get-value statement #1032, I didn't find new contradiction by asserting all the model and restart all the solver on this new problem. I believe that it means we don't find a lot of new contradiction by performing extra matching rounds during the model generation.

@bclement-ocp bclement-ocp added backlog completeness This issue is about completeness of theories instantiation this issue is related to the instantiation mechanism labels Jul 11, 2024
@bclement-ocp bclement-ocp removed this from the 2.6.0 milestone Jul 11, 2024
@bclement-ocp bclement-ocp added the adt Algebraic data types label Jul 11, 2024
@Halbaroth Halbaroth added this to the 2.7.0 milestone Aug 16, 2024
Halbaroth added a commit to Halbaroth/alt-ergo that referenced this issue Aug 16, 2024
This commit adds a test suggested by Basile in issue OCamlPro#1008.
@Halbaroth
Copy link
Collaborator

Finally, I think that the best solution is to solve this issue by merging #1095 after v2.6.0. I add a test #1211 and this issue will be closed after merging #1095.

@Halbaroth Halbaroth linked a pull request Aug 16, 2024 that will close this issue
@Halbaroth Halbaroth linked a pull request Aug 16, 2024 that will close this issue
Halbaroth added a commit to Halbaroth/alt-ergo that referenced this issue Aug 20, 2024
This commit adds tests suggested by Basile in issue OCamlPro#1008.
Halbaroth added a commit that referenced this issue Aug 20, 2024
* Add tests from issue 1008

This commit adds tests suggested by Basile in issue #1008.
@Halbaroth Halbaroth removed a link to a pull request Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adt Algebraic data types backlog bug completeness This issue is about completeness of theories instantiation this issue is related to the instantiation mechanism
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants