-
Notifications
You must be signed in to change notification settings - Fork 23
Add a translator from SMT.Term to SMTDDM.Term #177
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
base: main
Are you sure you want to change the base?
Conversation
This patch adds a translator from SMT.Term to SMTDDM.Term, and also has a small code snippet that prints the SMTDDM.Term into the format. There are missing features, which will be addressed in the following commits. - Whitespaces must exist between the printed subterms(!) - Option types, triggers, real-number/bit-vector constants are left as unimplemented.
| // This collides with the ':named' term attribute. This file has the example. | ||
| // op info_flag_name () : InfoFlag => ":name"; | ||
| op info_flag_reasonu () : InfoFlag => ":reason-unknown"; | ||
| op info_flag_version () : InfoFlag => ":version"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and below are updates for the requested option related SMT commands.
…cimal_neg and sc_numeral_neg to separately treat negative numerals Note that negative integers like '-1231' are symbols in Std! (Sec 3.1. Lexicon) The only way to create a unary symbol is through idenitifers, but this makes its DDM format wrapped with pipes, like '|-1231|`. Since such representation cannot be recognized by Z3, make a workaround which is to have separate `*_neg` categories for sc_numeral.
| | bool : Bool → TermPrim | ||
| | int : Int → TermPrim | ||
| | real : String → TermPrim | ||
| | real : Decimal → TermPrim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This finally changes the argument of SMT .real term from String to Decimal!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hurray!
…qX syntax categories to SMT DDM
MikaelMayer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to be able to parse ":pattern" as well for quantifiers, I think that will be necessary to convert the B3 dialect.
| // sign is not a part of the standard, but it seems CVC5 and Z3 | ||
| // support this for convenience. | ||
| // Note that negative integers like '-1231' are symbols in Std! (Sec 3.1. Lexicon) | ||
| // The only way to create a unary symbol is through idenitifers, but this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // The only way to create a unary symbol is through idenitifers, but this | |
| // The only way to create a unary symbol is through identifiers, but this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, thanks!
| // The only way to create a unary symbol is through idenitifers, but this | ||
| // makes its DDM format wrapped with pipes, like '|-1231|`. Since such | ||
| // representation cannot be recognized by Z3, make a workaround which is to have | ||
| // separate `*_neg` categories for sc_numeral. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, same as for the + operator. Thanks for the clarification.
| op se_keyword (s:Keyword) : SExpr => s; | ||
|
|
||
| op se_ls (s:Seq SExpr) : SExpr => "(" s ")"; | ||
| category SeqPSExpr; // For spacing, P for "+" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, excellent workaround until we have a SpaceSepBy type one day (which I would approve)
|
|
||
|
|
||
| // 3. Identifier | ||
| // 3. Identifier. Use 'SMTIdentifier' because the 'Identifier' category is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this possible to move the declaration of category SMTIdentifier up so that it's just below this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this in the new commit.
| op iden_simple (s:Symbol) : Identifier => s; | ||
| op iden_indexed (s:Symbol, i0:Index, il:Seq Index) : Identifier => | ||
| "(" "_" s i0 il ")"; | ||
| op iden_indexed (s:Symbol, si:SeqPIndex) : SMTIdentifier => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between SeqPSExpr and SeqPIndex? Since it's the concrete syntax tree, can we use only one of both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the existing syntax rule of Identifier is using <index>+ like this (page 102):
⟨identifier ⟩ ::= ⟨symbol ⟩ | ( _ ⟨symbol ⟩ ⟨index ⟩+ )
Probably SeqPSExpr will be a bit more general than SeqPIndex.
Strata/DL/SMT/Term.lean
Outdated
| | .bool b₁, .bool b₂ => b₁ < b₂ | ||
| | .int i₁, .int i₂ => i₁ < i₂ | ||
| | .real r₁, .real r₂ => r₁ < r₂ -- TODO | ||
| | .real r₁, .real r₂ => r₁.toRat < r₂.toRat -- TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the TODO still applicable? If so, can you tell what the TODO is about, if not can you remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
| op exists_smt (svs: SeqPSortedVar, t:Term) : Term => | ||
| "(" "exists" "(" svs ")" t ")"; | ||
| op bang (t:Term, attrs:SeqPAttribute) : Term => | ||
| "(" "!" t " " attrs ")"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting how we can give parsing errors directly on these constructs rather than decoding general S-Expressions. I like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also follows the definition of <term> category in the doc (link) . :) Page 103 has this.
I added an example that has |
| op declare_datatype (s:Symbol, so:SMTSort) : Command => | ||
| "(" "declare-datatype" s so ")"; | ||
| "(" "declare-datatype " s so ")"; | ||
| // TODO: declare-datatypes; what is ^(n+1)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not directly related to this PR, but I think it just means that there are k declare-datatypes syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense..! I will fix this in the following commit.
| return .smtsort_param srnone (mkIdentifier id) argtys | ||
|
|
||
| -- List of SortedVar to SeqPSortedVar. | ||
| -- Hope this could be elided away later. :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by this being elided away? Maybe separately, this and the next function are basically the same thing, so they could be combined into something like the following:
def list_to_ne_gen {nelist : Type → Type} (single : α → nelist α) (cons: α → nelist α → nelist α) (l: List α) (Hl: ¬l.isEmpty = true) : nelist α :=
match h: l with
| [] => by grind
| [x] => single x
| x :: y :: t => cons x (list_to_ne_gen single cons (y :: t) (by grind))
def list_to_optlist {nelist : Type → Type} (single : α → nelist α) (cons: α → nelist α → nelist α) (l: List α) : Option (nelist α) :=
if h: l.isEmpty then .none else .some (list_to_ne_gen single cons l h)
And then this can be instantiated for both functions. Not sure which is better, don't feel strongly one way or the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant I wanted to have remove these separate definitions of SeqP* to describe nonempty lists in DDM. :)
That being said, I think the 'SeqP' prefix doesn't seem straightforward to understand; I will fix SeqPSortedVar to SortedVarList.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me think that it might be worth it to have a more convenient way to work with nonempty lists in the DDM. This shows up all over the place, including for datatype constructors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to tackle this in a subsequent PR, but may I request you to add some file-level comments that describe what's going on in general in the file?
https://github.com/strata-org/Strata/blob/main/CONTRIBUTING.md#file-organization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I added one. :) I must have added this in advance to avoid making approvals stale :)
This patch adds
get-optionSMT command and relevant syntax categories (mentioned in the previous pull request)..realconstructor of the old SMT Term, fromStringtoDecimal.After 3, I checked the generated .smt2 files at
vcs/dir, and confirmed that they are equivalent modulo spaces! :)There are a few features in the string converters that are unimplemented; they will be added later when Encoder.lean uses the DDM string converter more.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.