Add `get(Option[T], var T): bool` procedure

[xTrayambak]

This PR adds a new procedure to std/options that takes in an Option[T] and a var T, and if the Option is not empty, then it stores its contents into the var T and returns true. If it is empty, it simply returns false. I've also added a test for this. It works a bit like this:

import std/options

var v: string

let opt = some("Nim rocks!")

# This will be true, as `opt` is not empty.
if opt.get(v):
    doAssert v == "Nim rocks!"

[xTrayambak]

I'm sorry if I'm being pushy, but is there anything else needed to be done in this PR?

[Araq]

I'm sorry if I'm being pushy, but is there anything else needed to be done in this PR?

We need to agree on whether it's var T or out T and I have the impression that the people arguing for var T don't understand the consequences.

[PMunch]

We need to agree on whether it's var T or out T and I have the impression that the people arguing for var T don't understand the consequences.

I'm very much in favour of picking the correct choice of var T and out T to match the logic of an API. However in this case I simply don't understand why you think out T is the correct choice. The whole idea behind an Option[T] is that it either has a value T or it hasn't got any value at all. So to expect unpack to always return a value T, even in the case where Option[T] doesn't have any value seems completely against the core concept of such an Option. We've even shown multiple examples of how unpack with a var T argument can be used in elegant ways to achieve things which we can't easily do currently, and which out T would explicitly forbid. The argument for out T seems to be that it is more "correct", but I just don't see the reasoning behind this, if anything I would say that var T much more closely matches the expectations of this procedure. If anything a procedure with an out T parameter should unpack into this argument or throw an exception so that it's analogous to the current get procedure. Because if we say that out T should always have a value it would be an exceptional state when we can't give it one. However if it is a var T we explicitly say that this argument might be changed, or it may not, and the return value will tell you whether in did or not. Something like this:

proc tryGet[T](opt: Option[T], value: out T) =
  ## Takes an option `opt` and sets `value` to the value stored in the option, if no option is
  ## stored an exception is throw.
  value = opt.get() # Throws an exception if it doesn't have a value

proc unpack[T](opt: Option[T], value: var T): bool =
  ## Takes on option `opt` and sets `value` to the value stored in the option and returns true.
  ## If no value is stored in the option `value` is left untouched and this returns false.
  if opt.isSome:
    value = opt.unsafeGet()
    true
  else:
    false

[Araq]

The sequence var x: T; unpack(y, x) does not compile under --experimental:strictDefs when unpack does not take an out parameter. Neither does the following snippet:

var x: T
if unpack(y, x):
  use x

The compiler simply camnot understand that x is only used here when it was assigned a value.

[ASVIEST]

The main problem with var parameters here is that the result can be a null pointer dereference; with an out parameter there is no such possibility. The main purpose of Option types is to avoid this behavior. Does anyone seriously want to use code that might cause undefined behavior ? Example of null pointer dereference:

import std/options
type
  Test = ref object
    val: int

  Obj = object
    x: Test

proc unpack(
  self: Option[Test],
  val: var Test): bool =
  if not self.isSome:
    false
  else:
    val = self.get()
    true

proc use(t: Obj) =
  echo t.x.val
  # deref of null pointer,
  # program crash                                    

var a = Obj()
if unpack(none(Test), a.x):
  discard
use a

With out params there will be no undefined behavior so out params keep code safely. Btw for ref object and ptr object should be used constructor instead of default proc

?= is just same with out param @beef331 example also works with out param yes you can't write this

var conf = Config(meaningOfLife: 42)
if getNone().unpack(conf.meaningOfLife):
  discard
echo conf.meaningOfLife # --> 0

but you can write

var conf = Config(meaningOfLife: 42)
var meaningOfLife: int
if getNone().unpack(meaningOfLife):
  conf.meaningOfLife = meaningOfLife
echo conf.meaningOfLife  #--> 42

this is not so bad, given that the code remains safety, but for this particular case ?= is more suitable So I think that unpack with out better than unpack with var. But in general I prefer ?= and maybe

proc `?~`[T](val: var T, o: Option[T]) =
  if x ?= o:
    val = x
config.mearningOfLife ?~ newMearning

[PMunch]

The sequence var x: T; unpack(y, x) does not compile under --experimental:strictDefs when unpack does not take an out parameter. Neither does the following snippet:

var x: T
if unpack(y, x):
  use x

The compiler simply camnot understand that x is only used here when it was assigned a value.

That is true, but this is more of a flaw with strictDefs than with this design. I agree it's not optimal, but I'd rather just do var x = default(T); if unpack(y, x): use x that lose compelling features and the more logical design of var T. If anything it would be really cool if you could poll Nim on compile-time if something was initialised or not. This way we could have:

proc unpack[T](opt: Option[T], value: out T): bool =
  if opt.isSome:
    value = opt.unsafeGet
    true
  else:
    when not isInitialized(value): # Imaginary function which returns whether Nim has detected that `value` is initialized
      value = default(T)
    false

The main problem with var parameters here is that the result can be a null pointer dereference; with an out parameter there is no such possibility. The main purpose of Option types is to avoid this behavior. Does anyone seriously want to use code that might cause undefined behavior ? Example of null pointer dereference:

Well this is an obvious usage error, you're looking at a field outside the if check. This is kinda solved with ?= where the symbol isn't defined outside outside the if block and is indeed the safer design. However turning it into an out T in this case wouldn't help, because the default of a pointer type is the nil pointer, so it's not even safer! If anything you'll end up with nil pointers more often because it's causing the default value instead of preserving whatever was there. If you want safer patterns for Options you can use something like this, however turning var T into out T in this PR won't be any safer in this case.

[beef331]

I'll just suggest the following signatures. The latter of which is safer than the proposed "swap to out" as it forces the user to define what happens when there is no value and it allows the use of strictdefs.

proc unpack[T](opt: Option[T], val: var T): bool template getOrDefault[T](opt: Option[T], defaultVal: T): T # template to allow lazy evaluation

[ASVIEST]

However turning it into an out T in this case wouldn't help, because the default of a pointer type is the nil pointer, so it's not even safer!.

I said that for ptr and ref objects you need to use a constructor instead of default. But this is not so important, as for me the best solution is ?=.

[xTrayambak]

There's two ways to approach this problem. One is to overhaul --experimental:strictDefs to add an {.assured.} pragma which makes the function say "hey, I'm gonna ensure that the user knows that I did not mutate the value!" and the return type must be a bool. I'm no programming language designer though, so let me know if this makes sense.

The second way is a very, very stupid one.

proc unpack*[T](self: Option[T], val: out T): bool {.inline.} =
  if not self.isSome:
    # if the value is nil/not initialized, make it the default
    # otherwise, just do a re-assignment, which sounds incredibly stupid.
    if val == nil:
        val = default(typeof(val))
    else:
        val = val # does this actually make `out` happy???
    return false

  val = self.get()
  true

[ASVIEST]

The third way ?=, devoid of all these problems

[xTrayambak]

We also have to remember that not all outputs need to be mutable. But yeah, this seems like a better idea.

[xTrayambak]

Okay, so the latest commit adds the ?= template, which in turn relies on unpack, which now uses out T instead of var T and ensures that the value is ALWAYS set, matterless of whether the Option has anything or not. If it has nothing, the value will be set to the type's default one.

[Araq]

Tests are red, please investigate.

[xTrayambak]

Fusion no longer compiles. Weird. I'll investigate that tomorrow and fix it.

[xTrayambak]

Why does this fail to compile? Fusion doesn't use this proc, obviously, and there shouldn't be any other reason for it to fail. I'll look a bit deeper.

[xTrayambak]

As a matter of fact, that file doesn't even import options or use them!

[xTrayambak]

Should work now, in theory :sweat_smile:

[xTrayambak]

Nope, broken yet again. I'll just remove ?=, seems counterintuitive anyways.

[xTrayambak]

@Araq Is there anything else left now?

[ASVIEST]

I looked at your commits with the ?= operator and the code doesn't compile due to a (pretty obvious signature mismatch):

template `?=`*[T](self: Option[T], x: untyped): bool
# but should be
template `?=`*[T](x: untyped, self: Option[T]): bool

also please do not use early return when it is not needed Instead of this:

  if not self.isSome:
    val = default(T)
    return false

  val = self.get()
  true

Should be this:

  if not self.isSome:
    val = default(T)
    false
  else:
    val = self.get()
    true

[ASVIEST]

the problem is caused because fusion uses untyped for its ?= (https://github.com/nim-lang/fusion/blob/master/tests/tmatching.nim#L212C1-L219C1), so for the option

if x ?= some(42):
  echo x

you need to use untyped{ident} to avoid conflicts

like this:

template `?=`[T](x: untyped{ident}, y: Option[T]): bool =
  var x: T
  unpack(y, x)

[Araq]

Can't we have this ?= somewhere else so that not every user of options.nim gets a new operator that previously nobody asked for when importing the module?

[xTrayambak]

What about moving it into std/sugar instead? unpack does the job but it's not the best looking way to do it.

let x = some "woops"
var y: string

if x.unpack(y):
  assert x.get() == y

if z ?= x:
  assert x.get() == z