codex-storage / questionable

Elegant optional types for Nim
Other
116 stars 5 forks source link

Upcoming Nim 1.6.16 release fails to compile tests #48

Closed elcritch closed 10 months ago

elcritch commented 10 months ago

@markspanbroek, I was trying out the (upcoming?) compiler dev-branch for Nim 1.6 series and it fails to compile questionable. Looks like there's a bug with macros and declaring variables. I created an issue on Nim's github:

https://github.com/nim-lang/Nim/issues/22897

elcritch commented 10 months ago

@markspanbroek I'm trying to reduce this issue down and I'm sorta confused by the code genderated by =? and bindVar.

It looks like option(evaluatedgensym705)is called on expression passed to=?. The type ofoptions.optionisoption*[T](val: sink T): Option[T]`, but we already have an option type. To be a bit more specific, here's the test I was working on:


  test "=? works in generic code with variable hiding":
    let value {.used.} = "ignored"

    proc toString[T](myOpt: ?T): string =
      if value =? myOpt:
        $value
      else:
        "none"

    check 42.some.toString == "42"
    check int.none.toString == "none"

I added expandMacros to the toString proc and it generates. I also renamed option to myOpt just to avoid name conflicts:

if (
  let evaluated`gensym705 = myOpt
  let option`gensym705 = option(evaluated`gensym705)
  type
    T`gensym705 = typeof(option`gensym705.unsafeGet())
  let value = if isSome(option`gensym705): unsafeGet(option`gensym705) else:
    placeholder(T`gensym705)
  isSome(option`gensym705)): $value
else:
  "none"

I took that and put it back into the test:

    proc toString[T](myOpt: ?T): string =
      when false:
        expandMacros:
          if value =? myOpt:
            $value
          else:
            "none"
      else:
        let evaluated = myOpt
        let optionGensym705 = option(evaluated)
        type
          S = typeof(optionGensym705.unsafeGet())
        let value =
            if isSome(optionGensym705): unsafeGet(optionGensym705 )
            else: default(S)

        echo "val: ", value.typeof, " ", S
        check S is T # I'm assuming this is what we want
        if (
          isSome(optionGensym705)):
            echo "issome: ": $value
            $value
        else:
          "none"

In the code above, the type of S is Option[int] where I'd expect it to be of type int? This fails the test as the result becomes "some(42)" rather than "42". Is the options.option what we really want in this bit? Or perhaps only for ref types?

elcritch commented 10 months ago

Looking into this some more, I think it's just =? with options that isn't correct.

Though, it looks like using the Result -> Option route in =? and without is a bit round about. It might also add up in a performance or compile time costs as well? Probably it'd be small but we use it so extensively in Codex it might add up.

Something like this might be a bit simpler:

        let evaluatedgensym998 = res
        let value = evaluatedgensym998.valueOr(default(evaluatedgensym998.unsafeGet().typeof()))
        if (isSuccess(evaluatedgensym998)):
          $value
        else:
          "error"

Then defining a valueOr for Option would let options use the same code. Though I forget the details of without, but I'd imagine it'd be ok.

markspanbroek commented 10 months ago

Thanks for reporting this @elcritch, I'll take a look early next week to see if we can work around it.

markspanbroek commented 10 months ago

Workaround for the compilation issue is in #49. Was easy to fix because of the discussion in the Nim issue, so thanks for filing that @elcritch!

As for valueOr, it has a different use case than without. For instance, this is a pattern we use a lot that valueOr doesn't support:

without a =? foo() and b =? a.bar() and b.isSomething:
  return
elcritch commented 10 months ago

Glad that issue helped @markspanbroek! It looks like just removing the types made the template untyped and worked around it?

As for valueOr, it has a different use case than without. For instance, this is a pattern we use a lot that valueOr doesn't support:

Oh I meant using valueOr in implementing bindVar and the others. The issue is that surprises me is how =? works at all currently since it's using the proc option() in bindVar. You've defined option for the result type, but it looks like for options it's the stdlib proc proc option*[T](val: sink T): Option[T] from std/options.

If I take this options test case and run it with expandMacros:

  test "=? works in generic code":
    expandMacros:
      proc toString[T](option: ?T): string =
        if value =? option:
          $value
        else:
          "none"

      check 42.some.toString == "42"
      check int.none.toString == "none"

The result I get after cleaning it up a bit is:


import std/options
import std/unittest

proc toString[T](option: Option[T]): string =
  if (
    let evaluated_gensym703 = option
    let option_gensym703 = evaluated_gensym703.option() ## this seems odd, it'd return `Option[Option[T]` right?
    type
      T_gensym703 = typeof(option_gensym703.unsafeGet())
    let value {.used.} =
      if option_gensym703.isSome: option_gensym703.unsafeGet()
      else: default(T_gensym703)
    option_gensym703.isSome):
    $value
  else:
    "none"

block:
  check 42.some.toString == "42"
  check int.none.toString == "none"

Running that fails with:

/Users/jaremycreechley/projs/status/sandbox/untitled.nim(20, 25): Check failed: 42.some.toString == "42"
42.some.toString was some(42)
/Users/jaremycreechley/projs/status/sandbox/untitled.nim(21, 26): Check failed: int.none.toString == "none"
int.none.toString was none(int)

However, the unit test passes just fine.

elcritch commented 10 months ago

Ah, duh! I finally found it hiding in plain site:

proc option[T](option: Option[T]): Option[T] =
  option

Ok, thought I was going crazy for a second but that makes a lot more sense.