crystal icon indicating copy to clipboard operation
crystal copied to clipboard

Allow calling `Enum.parse?` and `.from_value?` via `super` from overide in enum types

Open RespiteSage opened this issue 3 years ago • 5 comments

Before this PR, Enum subtypes (i.e. enums) could not override Enum#parse? or Enum#from_value? and use super in the method body. The reason was that both methods return self?, so super would call them with self as Enum, leading to the error below because unions currently can't include Enum.

enum Something
  Alphabet
  Numeric

  def self.parse?(string : String) : self?
    result = super string # => Error: can't use Enum in unions yet, use a more specific type

    if result.nil?
      case string
      when "abc"
        result = Alphabet
      when "123"
        result = Numeric
      end
    end

    result
  end
end

s = Something.parse? "abc"
t = Something.parse? "numeric"

To fix the issue, I simply removed the type restrictions on both methods. I think removing the type restrictions is the best way to fix this problem, but I do find it unsatisfying because they improve the readability of the code and the generated docs. However, the only other solution I could think of was to put Enum#parse? and Enum#from_value? inside a macro inherited, presumably allowing a user to access them using previous_def rather than super. I think that solution is jankier in general, and it certainly would mess with code and documentation readability more than this solution.

RespiteSage avatar Jan 26 '22 18:01 RespiteSage

For reference: Prior discussion in https://forum.crystal-lang.org/t/enums-methods-with-super-previous-def/4302/2

straight-shoota avatar Jan 26 '22 21:01 straight-shoota

If we allow:

struct Value
  def self.foo : self?
  end
end

struct Char
  def self.foo : self?
    super
  end
end

Char.foo # => nil

There is no reason to disallow:

struct Enum
  def self.foo : self?
  end
end

enum Foo
  X

  def self.foo : self?
    super
  end
end

Foo.foo

because those self? restrictions should not type a variable. So I don't think this PR is the fix we want to have.

HertzDevil avatar Sep 09 '22 17:09 HertzDevil

So I don't think this PR is the fix we want to have.

As I said before, I'm definitely interested in a better solution if we have one. What would be required to fix this without sacrificing the type restrictions? Would that require a type system fix or can it still be done in stdlib?

RespiteSage avatar Sep 09 '22 19:09 RespiteSage

It requires a change in the compiler. So I think this PR still makes sense as an interim solution.

straight-shoota avatar Sep 09 '22 19:09 straight-shoota

I looked into this a bit and in the first case the compiler actually forms the union between Value+ and Nil, and that doesn't produce an error for some reason. The virtual type is formed when the self restriction is resolved.

Without questioning why that union can be instantiated, the virtual type for Enum should be Enum+ instead of Enum.

HertzDevil avatar Sep 09 '22 20:09 HertzDevil