-
Notifications
You must be signed in to change notification settings - Fork 54
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
Support recursive datatypes in egglog #432
Conversation
src/ast/mod.rs
Outdated
Err((head, args)) => list!("sort", name, list!(head, ++ args)), | ||
}) | ||
.collect(); | ||
list!("datatypes", ++ datatypes) |
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.
Should this be datatype*
? Hm, I wonder if there are no tests for these?
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.
Good catch! For NCommand
, cases like this would be caught by resugared tests (which re-run same tests over programs converted from ASTs), but we don't have tests for Command (which is immediately desugared into NCommand
)
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.
Looks good overall, had one question about turning to s-expr. Thanks!
src/ast/mod.rs
Outdated
@@ -450,6 +450,8 @@ pub type Command = GenericCommand<Symbol, Symbol>; | |||
|
|||
pub type Subsume = bool; | |||
|
|||
pub type Subdatatypes = Result<Vec<Variant>, (Symbol, Vec<Expr>)>; |
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.
Are you using Result
to separate sorts vs datatypes? I think it might be clearer if you have a custom enum instead? Unless I'm missing something, just when trying to follow the code.
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, good point. I have changed it to use a custom enum.
…g-smol into yihozhang-recursive-datatype
Co-authored-by: Saul Shanabrook <[email protected]>
I actually accidentally pushed this to
main
... which is bad (now reverted). I have updated repo settings so that no one can directly push tomain