[regression] parseEnum does not work anymore for enums with holes

[Vindaar]

Already mentioned on IRC, but obviously worth an issue.

The fix 9874981 for #14001 broke parseEnum for enums with holes, since the current implementation of parseEnum is based on iterating over the enum using low(T) .. high(T).

It may be fine to break for x in MyEnum, but imo there's no reason why parseEnum taking a string should be affected.

Example

import strutils

type
  HoledEnum = enum
    A = 1
    B = 4
    C = 8

let x = parseEnum[HoledEnum]("A")
doAssert x == A

Current Output

/tmp/broken.nim(9, 18) template/generic instantiation of `parseEnum` from here
/home/basti/src/nim/nim_git_repo/lib/pure/strutils.nim(1249, 18) template/generic instantiation of `..` from here
/home/basti/src/nim/nim_git_repo/lib/system/iterators_1.nim(85, 12) Error: type mismatch: got <HoledEnum>
but expected one of: 
proc inc[T: Ordinal](x: var T; y = 1)
  first type mismatch at position: 1
  required type for x: var T: Ordinal
  but expression 'res' is of type: HoledEnum

expression: inc(res)

Expected Output

Should compile and work.

Possible Solution

Either as mentioned by @timotheecour in #14001 by adding magics similar to the fields iterator or by adding a macro similar to:

proc listEnumImpl(enumType: NimNode,
                  returnFields: bool,
                  returnValues: bool): NimNode =
  # `getTypeInst` to unpack e.g. a generic `T: enum` from a symbol
  let impl = enumType.getTypeInst[1].getImpl[2]
  expectKind(impl, nnkEnumTy)
  result = nnkBracket.newTree()
  doAssert returnFields or returnValues
  for el in impl:
    case el.kind
    of nnkEmpty: discard # first field of enumTy
    of nnkEnumFieldDef:
      if returnFields and returnValues:
        result.add nnkPar.newTree(el[0], el[1])
      elif returnFields:
        result.add el[0]
      else:
        result.add el[1]
    else: doAssert false, "shouldn't exist in enum type"

macro listFields*(enumType: typed): untyped =
  ## Returns all fields of the given enum (with or without holes) as an array.
  result = listEnumImpl(enumType, true, false)

and using that for the loop in parseEnum instead of low(T) .. high(T).

The latter has the major disadvantage that one suddenly has to import macros in strutils / import a new module. Adding those to system.nim is also not a good idea.

Finally, by adding magics one can also support for x in HoledEnum again (even if via explicit iterators).

I tried to add a new magic, but I fail to even get that magic e.g.:

iterator fields*[T: enum](x: T): RootObj {.magic: "EnumFields", noSideEffect.}

to match an enum in the first place, because I don't understand how to match type MyEnum.

Additional Information

Worked before the aforementioned commit 9874981.

[cooldome]

There are also issues with performance of this function. Too many allocations in the loop.Let's fix it PROPERLY, we should get rid of the loop.

I see two possible solutions. First: generate case statement with string normalization using a macro. Second: case insensitive const table of string to enum.

@Araq, what would be your preference?

[Vindaar]

First: generate case statement with string normalization using a macro.

Something like this I guess?

macro genEnumStmt(n, argIdent: typed,
                  default: typed): untyped =
  let impl = getType(n)
  expectKind impl[1], nnkEnumTy
  let lower = bindSym"toLowerAscii"
  result = nnkCaseStmt.newTree(nnkDotExpr.newTree(argIdent, lower))
  for f in impl[1]:
    case f.kind
    of nnkSym, nnkIdent:
      result.add nnkOfBranch.newTree(
        nnkDotExpr.newTree(f.toStrLit,
                           lower),
        f)
    else: discard
  # finally add else branch to raise or use default
  expectKind(default, nnkSym)
  if default == newLit false:
    let raiseStmt = quote do:
      raise newException(ValueError, "Invalid value: " & $`argIdent`)
    result.add nnkElse.newTree(raiseStmt)
  else:
    result.add nnkElse.newTree(default)

proc parseEn[T: enum](x: string): T =
  genEnumStmt(T, x, default = false)

proc parseEn[T: enum](x: string, default: T): T =
  genEnumStmt(T, x, default)

But both of your proposals would mean either to import macros in strutils or some other module in such a genEnumStmt macro is defined, no?

[cooldome]

Replace

  let lower = bindSym"toLowerAscii"

with

  let lower = bindSym"normalize"

Also you need to handle four ways of defining enums:

type
  MyEnum = enum
    mxA
    mxB = "bb"
    mxC = (5, "ccc")
    mxD = 15

You will need getTypeImpl and/or getImpl to achieve this
Otherwise very looks good

[Vindaar]

  let lower = bindSym"normalize"

Ah, ok. I used toLowerAscii, because that seems more in line with the currently used cmpIgnoreCase, but I agree it makes sense to ignore _ as well.

Also you need to handle four ways of defining enums:

Yeah, good point. Didn't think about more complicated cases.

This should do:

import macros, strutils

type
  Enum = enum
    A
    B = "bb"
    C = (5, "ccc")
    D = 15

proc addOfBranch(n, field, lower: NimNode): NimNode =
  result = nnkOfBranch.newTree(
    nnkDotExpr.newTree(n,
                       lower),
    field)

macro genEnumStmt(n, argIdent: typed,
                  default: typed): untyped =
  let impl = n.getTypeInst[1].getImpl[2]
  expectKind impl, nnkEnumTy
  let lower = bindSym"normalize"
  result = nnkCaseStmt.newTree(nnkDotExpr.newTree(argIdent, lower))
  for f in impl:
    case f.kind
    of nnkEmpty: discard
    of nnkSym, nnkIdent: result.add addOfBranch(f.toStrLit, f, lower)
    of nnkEnumFieldDef:
      case f[1].kind
      of nnkStrLit: result.add addOfBranch(f[1], f[0], lower)
      of nnkTupleConstr: result.add addOfBranch(f[1][1], f[0], lower)
      of nnkIntLit: result.add addOfBranch(f[0].toStrLit, f[0], lower)
      else: error("Invalid tuple syntax!")
    else: error("Invalid node for enum type!")
  # finally add else branch to raise or use default
  expectKind(default, nnkSym)
  if default == newLit false:
    let raiseStmt = quote do:
      raise newException(ValueError, "Invalid value: " & $`argIdent`)
    result.add nnkElse.newTree(raiseStmt)
  else:
    result.add nnkElse.newTree(default)

proc parseEn[T: enum](x: string): T =
  genEnumStmt(T, x, default = false)

proc parseEn[T: enum](x: string, default: T): T =
  genEnumStmt(T, x, default)


let a = parseEn[Enum]("A")
let b = parseEn[Enum]("bb")
let c = parseEn[Enum]("ccc")
let d = parseEn[Enum]("D")

[Araq]

I see two possible solutions. First: generate case statement with string normalization using a macro. Second: case insensitive const table of string to enum. @Araq, what would be your preference?

Macro producing a case statement.

[Vindaar]

Macro producing a case statement.

So we're fine importing macros in strutils?

[cooldome]

Yes, we are importing macros from strutils

[cooldome]

@Vindaar, please submit as PR. One last very minor comment, with case branches you can do directly:

nnkOfBranch.newTree(normalize(str), enumVal)

Instead of

addOfBranch(f[1], f[0], lower)

that will do normalize but later on after macro expansion

In this case lower will be used only hence you get rid of tmp variable and write:

nnkCaseStmt.newTree(nnkDotExpr.newTree(argIdent, bindSym"normalize"))