warn on enum conversions

[arnetheduck]

Converting an integer to enum is fraught with issues - often the integer is coming from an untrusted source such as a file or the network and in code reviews, we've found many instances where it leads to remote exploits crashing the application or worse.

The safe way to convert an enum is to use a case statement, converting each enum value to an integer and making a comparison - a macro can be used to generate this list (see https://forum.nim-lang.org/t/8188).

The most reasonable thing for most code out there is to avoid the MyEnum(anInteger) conversion entirely - since it can't be changed to behave safely, a compile-time warning is appropriate, educating users about the issues with the construct.

[Araq]

I think the warning should only be emitted for enums with holes.

[timotheecour]

I'm not sure a warning is a good idea, but either with or without a warning, there is a safe and efficient O(1) way to convert from int to enum (possibly with holes)

#18044 once i undraft and make public a private proc it will IMO solve this as well as the problem raised in https://forum.nim-lang.org/t/8188 as follows:

• allow a conversion from int to enum (including HoleyEnum) along with success/failure flag
• allow this in O(1) (instead of the O(n) case statement mentioned in top post + forum post)

note that this is the current behavior as of devel 1.5.1 ffce6de

type A = enum a0, a1, a2 # enum
type B = enum b0 = 10, b1, b2 # enum with offset
type C = enum c0 = 2, c1 = 4, c2 # enum with holes

proc main()=
  doAssert 2.A == a2
  doAssert not compiles(3.A)
  var a = 3
  doAssertRaises(RangeDefect): discard a.A

  doAssert 12.B == b2
  doAssert not compiles(13.B)
  var b = 13
  doAssertRaises(RangeDefect): discard b.B

  doAssert 4.C == c1
  # doAssert not compiles(13.C) # xxx should give CT error
  var c = 13
  doAssertRaises(RangeDefect): discard c.C

# static: main() # xxx doesn't pass
main()

[arnetheduck]

I think the warning should only be emitted for enums with holes.

the security issues we've found were for normal enums predominantly - ie the only time you want to convert to an enum is when you've gone through a type erasure of some sort, and that predominantly happens when you're working with external data.

This is just something people get wrong because the behavior is unintuitive and not in sync with how it's used in actual applications - nobody understands in real life that converting to an enum will crash the application - it's based on an arbitrary distinction between defects and non-defects, and inconsistent behavior around except: where sometimes it looks like it's working.

You can always claim "go read the manual" but by then the damage is already done.

allow this in O(1) (instead of the O(n) case statement mentioned in top post + forum post)

as noted by others in the O(1) pr, it's a real stretch that any enum would have so many entries that O-performance matters - the only thing that matters is access to a utility function that implements the same defect-rules as the compiler, but without raising a defect - this goes for all type conversions really, so that they reasonably can be implemented without crashes and consistently with the language rules, for those of us that care about errors. If anything, the function (and rangeof from the other PR) should really be combined into one utility without all that lookuptable stuff (which, even if it was there, I'd hesitate to use for the sheer complexity it brings to the problem)

[Araq]

it's based on an arbitrary distinction between defects and non-defects, and inconsistent behavior around except: where sometimes it looks like it's working.

File a bug report when it doesn't work. The distinction is not arbitrary either anyway.

You can always claim "go read the manual" but by then the damage is already done.

There are certain limits to how far you can write production code in any language without reading its official documentation.

[arnetheduck]

File a bug report when it doesn't work. The distinction is not arbitrary either anyway.

Well, of course it is, it's based on opinion and context and a bug report will practically turn into a bikeshedding contest - there's no right answer here because it depends on taste and use case - the language is consistent in using a Defect for this conversion since that's what it uses for all the other conversions, but at the same time, it's a common source of misunderstandings because this consistency doesn't match the context in which it most often is used / applies.

[arnetheduck]

There are certain limits to how far you can write production code in any language without reading its official documentation.

I'm pretty sure these devs have read the manual btw, and still get it wrong. That's where style guides, warnings and recommendations come in, to take the theory and turn it in to practice, even though in theory they should be the same.

[Araq]

Well, ok, but are enums then more special than object or other type conversions? Maybe you want a warning for every construct that can produce a Defect (with the exception of array indexing, I would assume).

[arnetheduck]

Well, ok, but are enums then more special than object or other type conversions? Maybe you want a warning for every construct that can produce a Defect (with the exception of array indexing, I would assume).

we've contemplated this, yes - enum in particular seem to be more difficult for people to grok, but it applies equally to any "lessening" type conversion really - what's difficult is coming up with an "alternative" spelling that de-taints the data - what we're looking for is something along the lines of a convert[U](v: T): Option[U] "standard", like so: status-im/nim-stew#34

[Araq]

Has been implemented.