search

egraphs-good / egglog

egraphs + datalog!
https://egraphs-good.github.io/egglog/
MIT License
386 stars 45 forks source link

Fix Nullary Unstable Functions #382

Closed saulshanabrook closed 1 week ago

saulshanabrook commented 1 month ago

I noticed that nullary functions don't parse correctly with the unstable function sort (i.e. (sort TestNullaryFunction (UnstableFn () Math)))

That's because the second argument is now parsed as a Unit type instead of a Call arg.

I could fix it and keep the existing format, but I was thinking another option would be to just change the sort to take all its args as a list, so there is always at least one and put the return type first. This would make it parse the same as other collection types.

So instead of (sort MathFn (UnstableFn (Arg) Ret)) you would have (sort MathFn (UnstableFn (Ret Arg))) or for this nullary case (sort TestNullaryFunction (UnstableFn (Math))))

It's a bit less intuitive for anyone writing UnstableFn manually in egglog, but makes the serialization code easier in Python and will make the parsing a bit easier in Egglog. I don't think it's a big deal to make the ergonomics worse to use in pure egglog, b/c I think as is it's already pretty odd to use, and I assume if we want it in core we will add better ergonomics to it anyways, like there are in the Python bindings.

But I just wanted to check before making a PR for this change if anyone had objections, in which case I could keep the format the same as it is, but just fix the parsing for the empty args case.

oflatt commented 1 month ago

It seems like putting the return value first is a bit confusing. I think languages often put it last or after an arrow

saulshanabrook commented 1 month ago

Yeah last is fine too.

saulshanabrook commented 1 month ago

I could fix it and keep the existing format, but I was thinking another option would be to just change the sort to take all its args as a list, so there is always at least one and put the return type first. This would make it parse the same as other collection types.

I opted just to keep the existing format for now and fix how it parses functions with no args in https://github.com/egraphs-good/egglog/pull/385