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

Replace fresh terms by abstract values in all subterms #1280

Draft
wants to merge 3 commits into
base: next
Choose a base branch
from

Conversation

Halbaroth
Copy link
Collaborator

@Halbaroth Halbaroth commented Jan 28, 2025

This PR fixes the issue #1270. More precisely,

  1. Add a new argument to the function X.to_model_term. The purpose of to_model_term is to generate a model term from constant semantic values. Unfortunately, these model terms are not exactly the model values of the SMT-LIB standard because they can contain fresh terms. The new argument of X.to_model_term is a function of type E.t -> E.t that is used to create abstract values from fresh terms. A cache is hidden in the closure of this function to prevent from duplicating abstract values for the
    same fresh terms.
  2. Improve and update some comments.
  3. Remove the term_values field in Models. This field was only used to cache the result of model_repr_of_term in Uf.
    I replaced it by a local cache in the closure of extract_concrete_model.

Now the output of the example of #1270 is:

unknown
(
  (define-fun q ((arg_0 t)) Int
   (ite (= arg_0 (as @a2 t)) 1 (ite (= arg_0 (as @a4 t)) 0 (- 1))))
  (define-fun o ((arg_0 Int)) t
   (ite (= arg_0 1) (as @a3 t) (ite (= arg_0 0) (as @a5 t) (as @a7 t))))
) 

This field is unused and is not correctly filled in the UF module.
We do not replace all the fresh names by abstract values during the
model generation. This commit adds an argument `abstract` to [X.to_model_term].
`abstract` is a function of type `E.t -> E.t` that generates abstract
values and `X.to_model_term` calls it on appropriate leaves. `abstract`
contains a cache in its closure to prevent from duplicating abstract
values for the same name.
@bclement-ocp bclement-ocp self-requested a review January 28, 2025 17:16
@Halbaroth
Copy link
Collaborator Author

I realized that the solution we found for #1213 should fix #1270 too without modifying the signature of any modules. I tested it on
the file of Matteo, it works! I will finish your patch. I keep this PR open.

@Halbaroth Halbaroth marked this pull request as draft January 29, 2025 15:58
Halbaroth added a commit to Halbaroth/alt-ergo that referenced this pull request Jan 30, 2025
The fix consists in using `declared_ids` to guarantee that we never
define symbols that have not been declared at the current assertion
level in models. A proper fix requires a lot of work as we need to
rework the push/pop mechanism of CDCL.

This PR is rebased OCamlPro#1280 to ensure we won't mix identifiers from
different assertion levels as it could happen with string-based
identifiers.
@Halbaroth Halbaroth mentioned this pull request Jan 30, 2025
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.

1 participant