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

Allow explicit enum values #114

Merged
merged 11 commits into from
Nov 13, 2024
Merged

Conversation

harrisi
Copy link
Contributor

@harrisi harrisi commented Sep 27, 2024

This PR adds the ability to set values for enums, so this type of C code can be generated:

enum Foo {
  FOO_A = 1,
  FOO_B,
  FOO_C = 4
};

The syntax I went with is the following:

type foo :: {:a, 1} | :b | {:c, 4}

The only real alternative I could think of would be something like this:

type foo :: :a = 1 | :b | :c = 4

I don't like this, since it's nonsensical in normal Elixir.

I originally thought a list would be okay, something like this:

type foo :: [a: 1, b: 2, c: 3]

But this doesn't allow for partially explicit definitions, unless all the explicit definitions are at the end (to make it a valid keyword list), so the above definition isn't possible:

type foo :: [a: 1, :b, c: 3] # this doesn't work

I think the desugared pseudo-keyword list is a decent option. It seems less weird than writing :a = 1 to me, anyway. It could also be defined as a list (e.g., [:a, 1] | :b), but that seems confusing since [type] is meaningful elsewhere.

@mat-hek
Copy link
Member

mat-hek commented Sep 30, 2024

👋 thanks for the PR 😉 Personally I find quite unobvious that the alternative of tuples maps to an enum 🤔 What about

@type foo :: enum(a: 1, b: 2)
@type foo :: enum({:a, 1} | {:b, 2})
@type foo :: enum_value(:a, 1) | enum_value(:b, 2)

I like the last one the most, except that enum_value is extremely long. WDYT @FelonEkonom?

@harrisi
Copy link
Contributor Author

harrisi commented Sep 30, 2024

I agree it's not the best syntax, so I'm happy to see more suggestions.

Something all three of your suggestions highlight is the use of the word "enum". I think it would be helpful to change the spec dsl to use enum instead of type in general.

@FelonEkonom
Copy link
Member

FelonEkonom commented Oct 3, 2024

@type foo :: enum_value(:a, 1) | enum_value(:b, 2)

I think this option is the best of the three you mentioned.

@harrisi even if we would introduce @enum, we have to still support @type, because of the backwards compatibility.

The general disadvantage of using names with enum is that it might make an impression that it has something to do with Enum from Elixir standard library, while it doesn't.

WDYT about something like

@type my_type :: (:a :: 1) | :b | (:c :: 100)

@harrisi
Copy link
Contributor Author

harrisi commented Oct 3, 2024

The general disadvantage of using names with enum is that it might make an impression that it has something to do with Enum from Elixir standard library, while it doesn't.

I hadn't thought about this, but good point. Having two ways of defining enums (with type and enum) doesn't seem worth it.

The main issue I have with using something like enum_value(:a, 1) is what happens if I define a function called enum_value already? It probably won't break anything, but it's certainly weird.

@type my_type :: (:a :: 1) | :b | (:c :: 100)

The precedence makes it so that parsing this is either very strange, or the parentheses are mandatory, which seems weird.

Without parentheses:

{:@, [context: Elixir, imports: [{1, Kernel}]],
 [
   {:type, [context: Elixir],
    [
      {:"::", [],
       [
         {:my_type, [], Elixir},
         {:"::", [],
          [:a, {:"::", [], [{:|, [], [1, {:|, [], [:b, :c]}]}, 100]}]}
       ]}
    ]}
 ]}

with parentheses:

{:@, [context: Elixir, imports: [{1, Kernel}]],
 [
   {:type, [context: Elixir],
    [
      {:"::", [],
       [
         {:my_type, [], Elixir},
         {:|, [],
          [{:"::", [], [:a, 1]}, {:|, [], [:b, {:"::", [], [:c, 100]}]}]}
       ]}
    ]}
 ]}

@mat-hek
Copy link
Member

mat-hek commented Oct 4, 2024

Right, another idea:

@typemap a: 1, c: 100
@type my_type :: :a | :b | :c

@harrisi
Copy link
Contributor Author

harrisi commented Oct 4, 2024

Having the definitions be separate seems weird, since there's a whole new type of errors to handle. What if the typemap includes extra keys? Should the order matter? Etc.

I'm not sure how y'all feel about it, but I'm kind of coming back around to the :a = 1 | :b | :c = 4 option. Any thoughts on that?

@mat-hek
Copy link
Member

mat-hek commented Oct 4, 2024

Yeah, unfortunately, all the approaches are a bit weird, because we're trying to connect two totally different worlds :P I'd lean towards the typemap thing, as it's not using syntax that has very different semantics in regular Elixir/specs, cc @FelonEkonom

What if the typemap includes extra keys?

I think it's ok to raise an error

Should the order matter?

The order would be as in the @type, just like it is now. The order in the typemap doesn't seem to matter.

@harrisi
Copy link
Contributor Author

harrisi commented Oct 4, 2024

I think it's ok to raise an error

Yeah, I was more just bringing up that there exists a new class of error with a separate "typemap" that needs to be considered.

The order would be as in the @type, just like it is now. The order in the typemap doesn't seem to matter.

What I mean is typemap could be defined in any order, technically, which can cause issues with the enum generation if type is of a different order:

type foo :: :a | :b | :c
typemap foo :: [a: 1, c: 4]

# later on: "huh, you know, I think `b` should be defined after `c`, actually.."
type foo :: :a | :c | :b

# uh oh, the enum changed from `1, 2, or 4` to `1, 4, or 5`.

Obviously changing the order of enums requires care anyway, but having the definition be separated in two calls isn't something that's familiar in C, so it seems easier to miss and have weird things happen.

The fix for this would be to enforce typemap to be in the same order as type, which is fine, but makes implementing this more work.

I'm not too concerned about the syntax, so I'll just say my preference is either {:a, 1} or :a = 1. Happy to implement any option, though. Maybe over the weekend someone will figure out the best choice :)

@FelonEkonom
Copy link
Member

@harrisi good point about paying attention to the order of fields in enum 👍

@FelonEkonom
Copy link
Member

@harrisi after internal discussion we decided that we like the option with @type foo :: enum_value(:a, 1) | ... the most, so if you have time and will, you can implement this one. We would release it just after merging this PR.

Thanks 👍

@harrisi
Copy link
Contributor Author

harrisi commented Oct 29, 2024

How would y'all feel about using a keyword list? Either instead of a 2-arity "function", or in addition to? So,

type foo :: enum_value(a: 1) | :b | enum_value(:c, 4)

would be valid if both are allowed? I don't love having two ways to define it, but the single element keyword list kind of feels better to me. It doesn't really make parsing meaningfully more complex to allow both.

@FelonEkonom
Copy link
Member

FelonEkonom commented Oct 30, 2024

IMO it seems weird to make enum_value accept keyword list with only one argument. Usually if a function in Elixir accepts a keyword list with unrestricted keys, there is no boundary on the length of this list (e.g. struct/2, maybe creating a new map doesn't require creating a keyword list, but the syntax is kind of similar, and there is no such a boundary there as well).

Personally I would stay with enum_value(:atom, 1) only

@harrisi
Copy link
Contributor Author

harrisi commented Oct 30, 2024

Sounds good! Let me know if there's anything else to add.

@FelonEkonom
Copy link
Member

@harrisi I don't see a need for anything else right now. Is this PR ready to review?

@harrisi
Copy link
Contributor Author

harrisi commented Nov 4, 2024

Should be! :)

Copy link
Member

@FelonEkonom FelonEkonom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Fix failing tests, probably you will have to update fixture files.
  2. Fix credo, try mix deps.update credo and see if it helps.

@harrisi
Copy link
Contributor Author

harrisi commented Nov 6, 2024

Merged the credo update from #115, fixed a few errors, updated test fixtures.

Sorry, I thought I did those things already. Forgot where things were at in the few weeks break there!

Hopefully that resolves everything.

@FelonEkonom FelonEkonom self-requested a review November 7, 2024 12:52
Copy link
Member

@FelonEkonom FelonEkonom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond one comment to the docs everything looks fine

lib/unifex/specs_dsl.ex Show resolved Hide resolved
@harrisi
Copy link
Contributor Author

harrisi commented Nov 12, 2024

Don't mean to rush, but is there anything else I need to do here, for now? I don't want to hold anything up! :)

@FelonEkonom FelonEkonom self-requested a review November 13, 2024 09:16
@FelonEkonom FelonEkonom merged commit 84c563b into membraneframework:master Nov 13, 2024
3 checks passed
@FelonEkonom
Copy link
Member

Everything is OK right now. I have released your changes in the version 1.2.1 on hex.pm. Thank you very much for your contribution 🥇 🥇 🥇

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

Successfully merging this pull request may close these issues.

3 participants