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.

Sign up for GitHub

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

Jump to bottom

Fix Nullary Unstable Functions #382

Open
saulshanabrook opened this issue May 24, 2024 · 3 comments · May be fixed by #385
Open

Fix Nullary Unstable Functions #382

saulshanabrook opened this issue May 24, 2024 · 3 comments · May be fixed by #385
Assignees
@saulshanabrook

Comments

@saulshanabrook
Copy link
Member

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.
@saulshanabrook saulshanabrook self-assigned this May 24, 2024
@oflatt
Copy link
Member

oflatt commented May 24, 2024

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

@saulshanabrook
Copy link
Member Author

Yeah last is fine too.

saulshanabrook added a commit to saulshanabrook/egg-smol that referenced this issue Jun 6, 2024
@saulshanabrook
Fix support for nullary unstable functions
0c0c6ae 
This resolves egraphs-good#382 by adding parsing support for defining sorts of 0 argument
unstable functions.

It does this by handling that input `()` as the `Unit` constructor.

If we ever want to remove the Unit constructor, we could move the return type
to the list of argument types, to make sure it always has at least one value.

This would also make parsing it more similar to other datatypes.

For now I just kept it the same to make the change minimal.
@saulshanabrook saulshanabrook linked a pull request Jun 6, 2024 that will close this issue
Open
@saulshanabrook
Copy link
Member Author

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 #385

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@saulshanabrook saulshanabrook

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix support for nullary unstable functions saulshanabrook/egg-smol
2 participants
@saulshanabrook @oflatt