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

Matching enum #126

Open
jtrakk opened this issue Jun 10, 2021 · 6 comments
Open

Matching enum #126

jtrakk opened this issue Jun 10, 2021 · 6 comments
Labels
bug Something isn't working
Milestone

Comments

@jtrakk
Copy link

jtrakk commented Jun 10, 2021

Sometimes I want to match on an enum.

using MLStyle

@enum Var left middle right

@match right begin
    left => 0
    middle => 1
    right => 2
end

This gives the result 0, but I want 2. As documented in pattern matching for Julia enums I can define

using MLStyle.AbstractPatterns: literal
MLStyle.is_enum(::Var) = true
MLStyle.pattern_uncall(e::Var, _, _, _, _) = literal(e)

to get the right answer. But I'm nervous about using this. If I forget to define these methods, or define them in the wrong place so they're not executed yet, I'll get the wrong answer.

I would prefer to get either the right answer or an error rather than the wrong answer. What would be a good solution here?

@thautwarm
Copy link
Owner

thautwarm commented Jun 26, 2021

A possible option is to report you that the global variable left is used as a capture pattern.

using MLStyle

MLStyle_AllowShadow = false
@enum Var left middle right

@match right begin
    left => 0
    middle => 1
    right => 2
end

┌ Error: global variables used as capture patterns: left, middle, right.
└ @ Main xxx:xxx

Could this suffice?

@jtrakk
Copy link
Author

jtrakk commented Jun 26, 2021

I'm an unsophisticated user -- I don't understand why it gives 0 in my example or what MLStyle_AllowShadow means. I just want to make sure that I don't get the wrong answer accidentally.

  1. If the suggestion is that a user like me would write MLStyle_AllowShadow = false, I don't like that solution, because if I forget to write it then I will get the wrong answer. If I remembered that I need to write something, I would just copy-paste
using MLStyle.AbstractPatterns: literal
MLStyle.is_enum(::Var) = true
MLStyle.pattern_uncall(e::Var, _, _, _, _) = literal(e)

instead so I get the right answer instead of an error.

  1. On the other hand, if I could just get the error by default, I would be happy with that solution.

  2. I guess the reason I have to copy-paste those lines into my code is so that MLStyle.jl macros don't need any MLStyle.jl functions at runtime? For my use cases I wouldn't mind invoking your functions at runtime if it means I wouldn't have to copy-paste any code. But maybe that violates the MLStyle.jl philosophy and would need to be a separate package.

@thautwarm
Copy link
Owner

Thanks for this explanation. I understand your concerns, but unfortunately it's hard to achieve what you prefer.
In your example, you use left as a pattern, but MLStyle cannot distinguish whether you expect to bind a variable left or compare the global variable left with your match target.
MLStyle takes the way that all symbols in a pattern are default to be variable binding, namely capture patterns. The only exception occurs when is_enum(left) == true.
If you want to treat left directly as a pattern, it's yet another protocol so that you need tell MLStyle about it.

@jtrakk
Copy link
Author

jtrakk commented Jul 4, 2021

Could MLStyle do something like this?

MLStyle.is_enum(::T) where {T<:Enum} = true
MLStyle.pattern_uncall(e::T, _, _, _, _) where {T<:Enum} = literal(e)

It seems to work in this case

julia> @match right begin
           left => 0
           middle => 1
           right => 2
       end
2

I think I understand your explanation. For other types that aren't known to MLStyle, like SumTypes.jl, it doesn't know whether I mean the existing name or a new name, and the "AllowShadow = false` suggestion would disambiguate. I think #127 would also avoid this accident, maybe that is a solution?

@thautwarm thautwarm added this to the 0.5 milestone Jul 19, 2022
@thautwarm
Copy link
Owner

MLStyle 0.5 will support Base.Enum as a builtin pattern.

@Zentrik
Copy link

Zentrik commented Dec 19, 2023

This just bit me, I didn't realise this silently gave the wrong results for enums. If you can't make matching on an enum error, might I suggest adding the warning you have in the README to https://thautwarm.github.io/MLStyle.jl/latest/index.html as well as I went to check there first and didn't look at the README.

@thautwarm thautwarm added the bug Something isn't working label Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants