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!"

[Araq]

But shouldn't that be val: out T?

[xTrayambak]

I'm sorry, but what difference does that do? Is it just a naming standard or does it actually do something? Sorry, I didn't even know that prefix existed before now.

[Araq]

out vs var parameters: https://nim-lang.org/docs/manual_experimental.html#strict-definitions-and-nimout-parameters-nimout-parameters

[PMunch]

Should it? Wouldn't that require val to be set before returning? In this case you'd need a val = default(T) in the isNone case which doesn't really make much sense.

[Araq]

As opposed to what though? Otherwise you need the same val = default(T) on the client side which is worse. Keep in mind that --experimental:strictDefs is coming.

[xTrayambak]

The entire gist of this function is:

• you have an empty value, say an int
• you have an Option[int], you don't know whether it contains something or not
• you can call this function, if the Option[int] wasn't empty, true will be returned, and you can throw that into an if statement
• else, you now know that nothing was done to the empty value and also know that the Option[int] was empty

an out parameter doesn't make sense here if we're returning a bool, which we'll use for a conditional check

[PMunch]

But your unpackInto models an out parameter moreso than it models a var parameter.

Not really. The concept is that it changes the output to the value of the option only when it is set and returns whether or not it was set. As ElegantBeef showed this can be used to provide an override function where available options will override the currently assigned value or otherwise leave it alone. This isn't just the current get without a return value, this is an unpacking of the someness of the option and the value.

Not really related to this but you can also use it for a fun option walrus:

template `?=`[T](x: untyped, y: Option[T]): bool =
  var x: T
  unpack(y, x)
  
var x = some(100)

if y ?= x:
  echo y # This variable is only defined in the scope where the option is valid

[Araq]

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

[xTrayambak]

In that case, --experimental:strictDefs is deeply flawed. You can't just make 90% of code invalid in the name of "strictness" or "safety". x won't even be accessible outside the scope, and since the boolean check returns false in the case of an empty option, there's not gonna be any real problems, unless you're doing some black magic.

I do not want to sound rude or undermining, but I'd really revise strictDefs before imposing it on everything given how much flexible syntax it renders completely impossible. That was really the magic of Nim that attracted me to learn it and taking it away is a huge slap in the face for a lot of people. This seems like a great idea, but the execution needs to be revised.

[Araq]

strictDefs means that where you did var x: T you sometimes need to write var x = default(T) instead. Still seems pretty reasonable to me given that it found real bugs in my own real code and I'm not that bad at Nim programming.

For your get though it outlines some sloppy design which is why you dislike it. ;-)

[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 😅

[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)

[ASVIEST]

no copying inner option type (and runnableExamples fix)

[ASVIEST]

Suggested change

var store: int

[xTrayambak]

Can you force-push this one? I'm not on my computer now.

[ASVIEST]

Suggested change

	var x: T

[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