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

Wrong destructor for mutually recursive ADT #1009

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

Halbaroth
Copy link
Collaborator

The new Dolmen frontend didn't create the correct destructor in presence of mutually recursive ADT. For instance the input:

(set-logic ALL)
(declare-datatypes ((Data1 0) (Data2 0))
  (
    ((cons1 (d1 Int) (d2 Data2)))
    (
      (cons2 (d3 Data1))
      (cons3)
    )
  )
)
(declare-const a Data1)
(declare-const b Data2)
(declare-const x Int)
(assert ((_ is cons1) a))
(assert ((_ is cons3) b))
(assert (= (d1 a) x))
(check-sat)

raised an assertion in the record theory.
The presence of a flaw in a test of d_cnf to discriminate between records and ADT produced Sy.Access destructors intead of Sy.Destructor destructors on some terms whose the type is a mutually recursive ADT. For instance d1 a was translated into mk_term Sy.Access [a] ....

The fix consists in using the cache of AE types in d_cnf to discriminate between records and ADT.

Notice that the legacy frontend parses the previous file correctly.

The new Dolmen frontend didn't create the correct destructor in presence
of mutually recursive ADT. For instance the input:
```smt2
(set-logic ALL)
(declare-datatypes ((Data1 0) (Data2 0))
  (
    ((cons1 (d1 Int) (d2 Data2)))
    (
      (cons2 (d3 Data1))
      (cons3)
    )
  )
)
(declare-const a Data1)
(declare-const b Data2)
(declare-const x Int)
(assert ((_ is cons1) a))
(assert ((_ is cons3) b))
(assert (= (d1 a) x))
(check-sat)
```
raised an assertion in the record theory.
The presence of a flaw in a test of `d_cnf` to discriminate between records
and ADT produced `Sy.Access` destructors intead of `Sy.Destructor`
destructors on some terms whose the type is a mutually recursive ADT.
For instance `d1 a` was translated into `mk_term Sy.Access [a] ...`.

The fix consists in using the cache of AE types in `d_cnf` to discriminate
between records and ADT.

Notice that the legacy frontend parses the previous file correctly.
@Halbaroth Halbaroth added this to the 2.6.0 milestone Dec 12, 2023
@Halbaroth Halbaroth changed the title [DO NOT MERGE] Wrong destructor for mutually recursive ADT Wrong destructor for mutually recursive ADT Dec 12, 2023
Copy link
Collaborator

@bclement-ocp bclement-ocp left a comment

Choose a reason for hiding this comment

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

I thought we were using the record flag to determine whether to create an ADT or a record. I guess we don't do it if they are in a mutually recursive nest because the legacy typechecker didn't either, and so the record flag is no longer correct there:

(* If there are adts in the list of type declarations then records are
converted to adts, because that's how it's done in the legacy typechecker.
But it might be more efficient not to do that. *)

I think the fix is sound but we might want to revisit this choice — it doesn't make much sense to ever use the ADT theory for types that are records (or enums, for that matter), even if they are mutually recursive with ADTs. Probably should wait until we remove the legacy frontend (#556) to avoid parity issues.

@Halbaroth
Copy link
Collaborator Author

I noticed this comment. I agree it will be simpler to modify this after removing the legacy frontend.

@Halbaroth Halbaroth merged commit 4a5e11d into OCamlPro:next Dec 12, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants