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

feat: support generics with define_language macro #318

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kayagokalp
Copy link

Description

Before this PR, it was not possible to define generic Language using define_macro because the macro itself was not parsing the generic part. So the following code piece was failing:

define_language! {
    pub enum GenericLang<T: SaturationNumber> {
        Number(T),
        "+" = Add([Id; 2]),
        "-" = Sub([Id; 2]),
        "/" = Div([Id; 2]),
        "*" = Mult([Id; 2]),
    }
}

Where SaturationNumber is a super trait of required set of traits such as FromStr, Debug, Display etc. This PR adds the required parsing patterns to the macro so that it is possible to define such language enums, using the macro easily. I added a test to showcase both that:

  1. The macro can parse generics.
  2. The macro can apply bounds (to generic type) correctly by testing an implementation of a bound that is definitely going to be satisfied for any T that the language is generic over (Display).

Docs are updated to showcase new generic language usage.

Motivation

I wanted to make my language generic over the number type. So that I can easily switch between different custom number representations to experiment with the effect on saturation performance given that some implementations are going to be faster to work with, saturation might be able to cover a larger space and I wanted to observe the effect experimentally without needing to re-implement same language. But in more general it should be useful to anyone who requires to define a language generic over a type.

@mwillsey
Copy link
Member

Cool idea! I think it would be simpler without the bounds, right? Why do you need to constrain the definition of the enum? AFAIK, that's typically not needed, it's instead preferred constrain impls.

@kayagokalp
Copy link
Author

Cool idea! I think it would be simpler without the bounds, right? Why do you need to constrain the definition of the enum? AFAIK, that's typically not needed, it's instead preferred constrain impls.

Thanks for the comment! that is a very valid point, in my experience as well, what I generally feel is that it is not required to constrain the definition itself and constraining the implementation is preferred. I have only found constraining the definition useful for pieces that will be consumed by downstream to impose a tighter contract where the type itself is not available to even "construct". For the use cases of egg this might not be relevant as i feel the consumer's of egg crate would tend to be binaries rather than library crates.

My main motivation was that to allow valid rust syntax to be accepted by the macro. So this implementation still supports bounds on definitions, and also generics without bounds. Of course if maintainability of the macro itself is preferable to this flexibility I can remove the bounds on definition support

@mwillsey
Copy link
Member

I would prefer removing the bound support to simplify the macro for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants