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

Case Enum when Symbol #6592

Closed
vladfaust opened this issue Aug 23, 2018 · 22 comments
Closed

Case Enum when Symbol #6592

vladfaust opened this issue Aug 23, 2018 · 22 comments

Comments

@vladfaust
Copy link
Contributor

enum Foo
  Bar
end

foo = Foo::Bar
pp! foo === :bar

case foo
when :bar then puts "true"
else           puts "false"
end

# => foo === (:bar) # => false
# => false

Okay, looks like Enum#=== doesn't work with symbols (although I thought that #6074 would allow so). No problem, let's extend Enum with #===(Symbol), because as stated at docs, case expands to === internally:

abstract struct Enum
  def ===(sym : Symbol)
    self.class.names.any? { |n| n.underscore == sym.to_s }
  end
end

enum Foo
  Bar
end

foo = Foo::Bar
pp! foo === :bar

case foo
when :bar then puts "true"
else           puts "false"
end

# => foo === (:bar) # => true
# => false

That's a kind of strange behavior 🤔

@asterite
Copy link
Member

To make it work you need to define Symbol#===, case rewrites it the other way around.

The reason why it doesn't work out of the box with symbol is because === is defined at Object to return nil, so a symbol already matches that signature and always fails.

By the way, you can do when .bar?, which is almost the same effort as writing a symbol.

@vladfaust
Copy link
Contributor Author

vladfaust commented Aug 23, 2018

I prefer when :bar over when .bar? 🤷‍♂️ It also allows to do when nil:

foo = rand > 0.5 ? Foo::Bar : nil

case foo
when .bar? # undefined method 'bar?' for Nil

case foo
when :bar # ok
when nil  # ok

Could this code be included into Crystal? I mean, I could finally contribute myself:

abstract struct Enum
  def symbol
    {% begin %}
      case value
      {% for member, index in @type.constants %}
      when {{index}}
        {{member.symbolize.underscore}}
      {% end %}
      end
    {% end %}
  end
end

struct Symbol
  def ===(enum _enum : Enum)
    self == _enum.symbol
  end
end

enum Foo
  Bar
end

foo = Foo::Bar
pp! :bar === foo

case foo
when :bar then puts "true"
else           puts "false"
end

# => (:bar) === foo # => true
# => true

@asterite
Copy link
Member

I just tried it and that implementation, compiled with --release, is pretty efficient.

That said, using underscore might not be ideal. I know the .bar? way always uses underscore, but at least that's type-safe. If you write :Bar because you think that's how it works, it will compile but it won't behave well. Additionally, that logic won't work for a @[Flags] enum, whereas it works for .bar?.

I'd like to have this in the language, but all the above reasons say to me not to do it.

In fact, I'd like to remove symbols from the language. They were blindly copied from Ruby and have no real purpose in Crystal, and they make modular compilation impossible, if one ever wants to implement that.

@Sija
Copy link
Contributor

Sija commented Aug 23, 2018

Even tho' I love symbols (coming from Ruby background), in this case I'd agree with all of the points mentioned by @asterite (maybe aside of removing 'em, I'd be sad to see them go, unless the gain would be worth it - like modular compilation). Adding to that :bar creates a new Symbol, whereas .bar? translates to a method call, which if I understand correctly is bit more efficient. If that's a matter of preference than we lose more than we gain.

@vladfaust
Copy link
Contributor Author

vladfaust commented Aug 23, 2018

abstract struct Enum
  def ===(symbol : Symbol)
    {% raise "Cannot call #===(Symbol) on flag enum" if @type.has_attribute?("Flags") %}
    {% begin %}
      case value
      {% for member, index in @type.constants %}
      when {{index}}
        {{member.symbolize.downcase}} == symbol || {{member.symbolize}} == symbol
      {% end %}
      end
    {% end %}
  end
end

struct Symbol
  def ===(enum _enum : Enum)
    _enum === self
  end
end

This solves issues mentioned by @asterite.

Regarding to efficiency, I don't think creating symbols leads to drastic performance issues... Calling .bar? on nil enum really bothers me.

I'm sure that casing enums with symbols is very intuitive.

@vladfaust
Copy link
Contributor Author

vladfaust commented Aug 23, 2018

I've benched it:

 === 433.65M (  2.31ns) (± 2.24%)  0 B/op   1.00× slower
bar? 433.82M (  2.31ns) (± 1.82%)  0 B/op        fastest

With .? methods:

  • Have to check against nil at first
  • If foo is type other than enum, would raise
case foo
when nil   then puts "nil"
when .bar? then puts "bar"
when .baz? then puts "baz"
else            puts "unknown"
end

With symbols - nice and clean:

case foo
when :bar then puts "bar"
when :baz then puts "baz"
else           puts "nil or unknown"
end

Let me create a PR, please

@Sija
Copy link
Contributor

Sija commented Aug 23, 2018

This solves issues mentioned by @asterite.

No, it doesn't. raising is not any sensible solution. It wasn't the only problem, lost compile-time checks are another one.

I'm sure that casing enums with symbols is very intuitive.

And I'm sure I'm seeing this for the first time as a feature-proposal so I reckon not that intuitive, btw what's not intuitive about .bar??

@asterite
Copy link
Member

I'm fine with adding this to the standard library. I would even add Enum#==(Symbol) and the other way around.

@vladfaust
Copy link
Contributor Author

@Sija,

I could wrap my head around implementing such a functionality with flags.

Regarding intuitivity - when casing, I'm usually asking the code whether this variable is something:

case x
when Int32 # IS it Int32? Note, no question mark in expression
when String # IS it String? Note, no question mark in expression

We're missing such functionality for enums. I can't ask if it is Bar, instead I'm asking if it responds to .bar?, also typing that question mark. We don't usually type question marks when asking its type in cases, right?

However, checking if it's a specific enum by it's full name (e.g. when Foo::Bar) may get too dirty when working with long paths. That's why symbols are ideal solution for that.

@Sija
Copy link
Contributor

Sija commented Aug 23, 2018

@vladfaust Quoting from the docs:

Enums are a type-safe alternative to Symbol. For example, an API's method can specify a type restriction using an enum type:

Your proposal will mix two concepts (enums vs symbols), blurring the line (type-safe-ness) between those.

We're missing such functionality for enums. I can't ask if it is Bar, instead I'm asking if it responds to .bar?, also typing that question mark. We don't usually type question marks when asking its type in cases, right?

Wrong, in this case the type is an Enum, it's the value you're talking about.

@vladfaust
Copy link
Contributor Author

@Sija,

blurring the line (type-safe-ness) between those.

I don't understand how this would blur anything. It's an alternative to .bar?, which is more pleasant to use, subjectively. It doesn't affect Enums in any way.

I've stated my thoughts, though. If there is anyone other than @asterite (which is not in core team IIRC) to support this proposal, let me know and I'll create a PR.

@Sija
Copy link
Contributor

Sija commented Aug 23, 2018

I don't understand how this would blur anything. It's an alternative to .bar?, which is more pleasant to use, subjectively. It doesn't affect Enums in any way.

enum Foo
  A
  B
end

foo = Foo::A

# compile-time safety
case foo
when .c? then # compile-time error
end

# no safety
case foo
when :c then # ouch
end

@vladfaust
Copy link
Contributor Author

@Sija why "ouch"? This wouldn't lead to an exception, instead a developer checks for a possible Foo::C value, I see no harm here, it's more like pre-caution. The developer could also forget to check .b? the same way he could forget that C doesn't exist, see?

@asterite
Copy link
Member

@vladfaust Imagine you have code like this:

enum Color
  Red
  Gray
end

color = ...
case color
when :gray
  # do sometihng
end

Now your company gets bought by a UK company and they tell you to not use "gray" but "grey". When you change the enum value from Gray to Grey and compile the code... it compiles! Whereas if you had used Color::Gray or .gray?, you would get a compile error. Of course you could search for ":gray"`, but you have to remember to do this, the compiler won't help you.

Given that in Crystal we care about providing a language that's type-safe, I think accepting the code suggested in this issue to allow comparing enums and symbols like that goes against the philosophy of the language.

@vladfaust
Copy link
Contributor Author

I tried to find alternative ways to Enum#=== with symbols in compile time, but got no luck. I agree on type issues it would raise, thanks @asterite for excellent example. However, gonna use this extension explicitly in my projects anyway. 😈

@RX14
Copy link
Contributor

RX14 commented Aug 24, 2018

Implementing this would also have added two ways of doing the exact same thing to crystal. This has all the same issues as aliases.

If we implement this syntax, we should remove symbols so that we know that :bar is always a shorthand for an enum member. Then this can be made type safe. I'm still not sure it's at all worth it though.

@asterite the first step to removing symbols is making namedtuples use string literals instead of symbols I guess?

@vladfaust
Copy link
Contributor Author

However, after #6074 a developer is able to

enum Color
  Red
  Gray
end

def foo(_enum : Color= :red)
end

foo(:gray)

Now your company gets bought by a UK company and they tell you to not use "gray" but "grey". When you change the enum value from Gray to Grey and compile the code... it does not compile! Perfect!

So why not implemeting the same functionality for when :gray? Is that too hard, @asterite (it's a honest question because I really don't know if it is)?

@RX14
Copy link
Contributor

RX14 commented Aug 25, 2018

@vladfaust you'd have to remove symbols to do that. :gray can't have a dual meaning as a symbol and as a shorthand for an enum member

@asterite
Copy link
Member

@vladfaust Good question! The reason is that it's easier to implement it when there's a restriction in the method argument that says it accepts an enum. Then the compiler can tell "the user is passing a symbol to a method that accepts an enum, I will apply this special rule".

In a case there's nothing to tell the compiler the tested value is an enum. Well, it will eventually infer it but the logic is a bit different and it's not that direct to do.

Actually... Maybe at the end of compilation the compiler can check it, and if the case expression is turns out to be an enum it could try to apply the rule...

But then... I think we'll have to do the same thing when you use ==, and then I don't know what do to, because comparing enums with symbols, even if they don't match, is valid.

And maybe a variable can be a symbol in one instantiation, or an enum in another instantiation. You don't want compilation to fail in that case.

The solution is to remove symbols, which are pretty useless in Crystal (because we have enums and immutable strings). Then comparing something to a symbol can only mean it's a shorthand for an enum name.

@vladfaust
Copy link
Contributor Author

If you need my opinion, I support that kind of change, it would not cause any harm to any of my projects, @asterite. And the idea of

comparing something to a symbol can only mean it's a shorthand for an enum name

seems like a feature of a clean language for me.

@al6x
Copy link

al6x commented Jan 5, 2020

Would like to add a comment about the type safety:

Imagine you have code like this:

enum Color
  Red
  Gray
end

color = ...
case color
when :gray
  # do sometihng
end

Now your company gets bought by a UK company and they tell you to not use "gray" but "grey". When you change the enum value from Gray to Grey and compile the code... it compiles!

In TypeScript such bug would be caught, and it would highlighted all the places that needed to be updated, as compiler won't allow you to use if or case conditions that are never going to be true.

P.S.

Literal types alias Color = :red | :blue would solve the equality and other issues natively and would provide pretty much the same level of type safety. With the literal types there will be no need for Enums at all.

@straight-shoota
Copy link
Member

It's true that there can be some mistakes the compiler can't warn you about.

In Crystal, case can receive any argument and it won't complain if a condition could never match because the types don't fit. The case comparison operator === is implemented for each combination of types and returns false by default. If color was some kind of literal type, this wouldn't change any behaviour. The compiler would still not complain about matching an completely incompatible data type.

That being said, there are a few ideas to improve the scenario you described. Exhaustive case statement is going to come eventually (see #8424). That would catch the spelling error (assuming the case is supposed to cover all enum values). The future of plain symbols is still unsettled, but I think there's a strong case for removing them from the language, which allow to fit the rules for symbol literals strictly to referencing enum values. How exactly that could work is yet to be determined, though.

Anyway, the type-safe way to implement a case expression matching enum values is to use the respective predicate methods:

enum Color
  Red
  Gray
end

color = ...
case color
when .gray?
  # do sometihng
end

Any spelling mistake would immediately result in a compiler error.

With the literal types there will be no need for Enums at all.

That's not true. Enums are way more versatile than literal types. You can't implement flags or assign specific values.

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

No branches or pull requests

6 participants