Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 14 commits into
base: devel
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0e216b0
(add) add proc
xTrayambak Jan 28, 2024
1d051c4
(rename) rename get(Option[T], var T) to unpack(...)
xTrayambak Jan 31, 2024
55a298c
(fix) fix tests to use unpack instead of get
xTrayambak Jan 31, 2024
1a8fd60
(fix) update runnableExamples
xTrayambak Jan 31, 2024
2ea2a82
(add) add ?= keyword, make use instead of
xTrayambak Feb 24, 2024
53c9194
(fix) variable name mismatch
xTrayambak Mar 4, 2024
e070324
(fix) yet another error
xTrayambak Mar 4, 2024
4f09e51
(fix) just remove ?=
xTrayambak Mar 4, 2024
7b2417a
(add) try to re-add ?= operator
xTrayambak Mar 10, 2024
f0b3665
(fix) tiny error
xTrayambak Mar 10, 2024
9d7ee3c
(fix) use boolean returned by unpack()
xTrayambak Mar 10, 2024
0382544
(fix) add proper runnable example
xTrayambak Mar 10, 2024
6c8304b
(fix) compilation error with fusion
xTrayambak Mar 10, 2024
c935b20
(fix) fix ?= keyword
xTrayambak Mar 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions lib/pure/options.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,21 @@ proc get*[T](self: Option[T], otherwise: T): T {.inline.} =
else:
otherwise

proc get*[T](self: Option[T], val: var T): bool {.inline.} =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But shouldn't that be val: out T?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Araq Araq Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@PMunch PMunch Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Araq Araq Feb 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 marked this conversation as resolved.
Show resolved Hide resolved
## Gets the content of the `Option`, stores it in `val` and returns true if the `Option` has a value.
## Otherwise, it simply returns false.
runnableExamples:
var storage: int

if some(1337).get(storage):
xTrayambak marked this conversation as resolved.
Show resolved Hide resolved
assert storage == 1337

if not self.isSome:
return false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var x: T

val = self.get()
true
xTrayambak marked this conversation as resolved.
Show resolved Hide resolved

proc get*[T](self: var Option[T]): var T {.inline.} =
## Returns the content of the `var Option` mutably. If it has no value,
## an `UnpackDefect` exception is raised.
Expand Down
13 changes: 13 additions & 0 deletions tests/stdlib/toptions.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,19 @@ proc main() =
let x = none(cstring)
doAssert x.isNone
doAssert $x == "none(cstring)"

# Store option's value in a variable, and return a bool
block:
let
x = some(1984)
y = none(int)

var a, b: int

doAssert x.get(a)
xTrayambak marked this conversation as resolved.
Show resolved Hide resolved
doAssert a == 1984

doAssert not y.get(b)
xTrayambak marked this conversation as resolved.
Show resolved Hide resolved

static: main()
main()
Expand Down
Loading