disallow enum to enum conversion (require passing through `ord`)

[timotheecour]

proposal

when true:
  type Koo = enum k1, k2
  type Goo = enum g1, g2
  discard k2.Goo # make this illegal (currently legal)
  discard k2.ord.Goo # this stays legal

rationale: this seems error prone and it's more explicit to pass through ord (and should result in same C code)

[Araq]

"Accepted RFC" here means "let's try and see what it breaks".

[timotheecour]

"let's try and see what it breaks"

=> "only" 2 packages, more precisely 2 LOC in total.

[Araq]

Still needs a backwards compat switch but aside from that, ready to go.

[cooldome]

IMO, explicit conversions should be allowed. Implicit conversions not allowed.

[Araq]

The RFC only targets explicit conversions as there are no implicit enum conversions in Nim.

[alaviss]

May I know how this RFC was accepted without much reasoning?

I'm worried about us introducing tiny breaking changes on every minor release.

[timotheecour]

what makes you think it was done without much reasoning? the previous behavior was error prone:

when true:
  type Koo = enum k1, k2
  type Goo = enum g1, g2, g3
  var a = g3
  var b = a.Koo
  echo b

before https://github.com/nim-lang/Nim/pull/16351: nim c main # no error/warning nim r main # RangeDefect nim r -d:danger main # 2 (invalid data!)

after https://github.com/nim-lang/Nim/pull/16351: nim c main # error:

Error: type mismatch:
 got 'Goo' for 'a' [enum declared in /Users/timothee/git_clone/nim/timn/tests/nim/all/t12239.nim(7, 8)]
 but expected 'Koo = enum' [enum declared in /Users/timothee/git_clone/nim/timn/tests/nim/all/t12239.nim(6, 8)]
    var b = a.Koo

nim c -d:nimLegacyConvEnumEnum main # Warning: enum to enum conversion is now deprecated

this change (and -d:nimLegacyConvEnumEnum) is explained in details in the changelog, and the proper fix (beside the workaround -d:nimLegacyConvEnumEnum) is backward compatible: explicit conversion through ord via var b = a.ord.Koo

(and see https://github.com/nim-lang/Nim/pull/17928 which mentions -d:nimLegacyConvEnumEnum in the error message)

[Clyybber]

Copying my reply from https://github.com/nim-lang/Nim/pull/16351#issuecomment-831472160 :

I'm not sure anymore wether this change is a good idea, the user is already explicit when writing foo.Bar, having to add .ord in there is unneccessary syntax salt. I was originally positive about this change because two enums might not have any meaningful relation and this would make conversions between them more explicit, but thinking more about it I don't think it fits well with the rest of the language; conversions from distinct T to T or vice versa don't have this extra explicitness either, even though the value of a distinct T might not have any meaningful relation to it's value as T.

[timotheecour]

unneccessary syntax salt

the added syntax salt doesn't matter if this case occurs so rarely, and if it prevents illegal unintended conversions in the common case, eg where user intended conversion from (say) an int and instead got a silent conversion from another unrelated enum.

Note that nim is strict about other conversions (apart from distinct that is), this case was AFAIK an oversight/inconsistency.

when true:
  type A = object
    a0: int
  type B = object
    b0: int
  var a = A(a0: 1)
  var b = a.B # error

[Clyybber]

Note that nim is strict about other conversions (apart from distinct that is), this case was AFAIK an oversight/inconsistency.

when true:
  type A = object
    a0: int
  type B = object
    b0: int
  var a = A(a0: 1)
  var b = a.B # error

objects are the odd one out here, I think it's reasonable to argue that your snippet should actually work (if the objects are structurally equivalent, but it's not implemented yet). Regardless I think objects are not the best thing to compare this to, a better comparision is to integer types, for which narrowing conversions are allowed too, if explicit.

This isn't really about strictness IMO, the compiler was already strict, the conversion is explicit.

[Araq]

Conversions between enums are unsafe and should be done via cast, note that it can come up in generic code where you use T(x) and expect only sound conversions -- not true for enums. This RFC is justified and arguably even a language bugfix.

[Clyybber]

Conversions between enums are unsafe and should be done via cast, note that it can come up in generic code where you use T(x) and expect only sound conversions -- not true for enums. This RFC is justified and arguably even a language bugfix.

For distincts a conversion from distinct T to T might change the meaning in the same way an enum to enum conversion does. For integers a conversion from int to int8 errors, for ranges it's the same story, for enum to enum it was the same.

When we say that the conversion should be done via cast then k2.ord.Goo should be just as illegal as k2.Goo is with this RFC, which would require making integer to enum conversions illegal, which is even more of a breaking change.

[Araq]

For distincts a conversion from distinct T to T might change the meaning in the same way an enum to enum conversion does.

Not in practice, no. In theory you can argue that there should be no connection between distinct T and T but then why would .borrow exist? There is a type relation between these two, it's not a subtype relation but still. There is no such relation between unrelated enum types.

Your argument makes much more sense when you compare two different enum types to two different distinct types of the same base type, but well, then these distinct types have at least the same base type whereas an enum's base type is not int.

[Clyybber]

Your argument makes much more sense when you compare two different enum types to two different distinct types of the same base type, but well, then these distinct types have at least the same base type whereas an enum's base type is not int.

Yeah, that's a better comparison. Even though an int is not an enum's base type, it can still represent all of it's values and is convertible to and from, and this RFC forces conversions to go through k2.ord.Goo or k2.int.Goo which isn't any safer than k2.Goo. In the same way explicit conversions from distinct T to another distinct T are allowed directly, and don't have to go through T first.

[Wh1teDuke]

I disagree, too verbose

rationale: this seems error prone and it's more explicit to pass through ord (and should result in same C code)

It might seem like that, but it isn't. No more error prone than myfloat.int

[Araq]

this RFC forces conversions to go through k2.ord.Goo or k2.int.Goo which isn't any safer than k2.Goo.

But it is safer, I gave an example why that is, think about a generic T(x) conversion.

[Clyybber]

But it is safer, I gave an example why that is, think about a generic T(x) conversion.

But this isn't exclusive to enum, if x is an integer and T an enum it might raise too, when T is a smaller integer it might raise too, or if T is a range type it might raise too. So if one were to have a generic T(x) conversion before this change, after this RFC this generic code would now have to become:

when typeof(x) is enum:
  T(ord(x))
else:
  T(x)

where T(ord(x)) can raise just as T(x) could.

[Araq]

This is not about raising, the conversion to a different enum is silent and can easily produce an invalid enum value! Type conversions in Nim cannot produce invalid bit patterns otherwise, in general. This implies that cast[E](5) should be enforced if 5 is not a valid representation of an enum field from E.

[alaviss]

the conversion to a different enum is silent and can easily produce an invalid enum value!

What do you mean by silent? It's explicit in the code, and a RangeDefect is thrown if it's out-of-bounds.

Type conversions in Nim cannot produce invalid bit patterns otherwise, in general. This implies that cast[E](5) should be enforced if 5 is not a valid representation of an enum field from E.

Wouldn't it also implies that you need to cast from int64 to int32, always?