Fix: ensure that options and results are only evaluated once

questionable/chaining.nim

@@ -15,50 +15,30 @@ macro expectReturnType(identifier: untyped, expression: untyped): untyped =
     when compiles(`expression`) and not compiles(typeof `expression`):
       {.error: `message`.}
 
-template `.?`*(option: typed, identifier: untyped{nkIdent}): untyped =
-  ## The `.?` chaining operator is used to safely access fields and call procs
-  ## on Options or Results. The expression is only evaluated when the preceding
-  ## Option or Result has a value.
-
+template chain(option: typed, identifier: untyped{nkIdent}): untyped =
   # chain is of shape: option.?identifier
   expectReturnType(identifier, option.unsafeGet.identifier)
   option ->? option.unsafeGet.identifier
 
-macro `.?`*(option: typed, infix: untyped{nkInfix}): untyped =
-  ## The `.?` chaining operator is used to safely access fields and call procs
-  ## on Options or Results. The expression is only evaluated when the preceding
-  ## Option or Result has a value.
-
+macro chain(option: typed, infix: untyped{nkInfix}): untyped =
   # chain is of shape: option.?left `operator` right
   let left = infix[1]
   infix[1] = quote do: `option`.?`left`
   infix
 
-macro `.?`*(option: typed, bracket: untyped{nkBracketExpr}): untyped =
-  ## The `.?` chaining operator is used to safely access fields and call procs
-  ## on Options or Results. The expression is only evaluated when the preceding
-  ## Option or Result has a value.
-
+macro chain(option: typed, bracket: untyped{nkBracketExpr}): untyped =
   # chain is of shape: option.?left[right]
   let left = bracket[0]
   bracket[0] = quote do: `option`.?`left`
   bracket
 
-macro `.?`*(option: typed, dot: untyped{nkDotExpr}): untyped =
-  ## The `.?` chaining operator is used to safely access fields and call procs
-  ## on Options or Results. The expression is only evaluated when the preceding
-  ## Option or Result has a value.
-
+macro chain(option: typed, dot: untyped{nkDotExpr}): untyped =
   # chain is of shape: option.?left.right
   let left = dot[0]
   dot[0] = quote do: `option`.?`left`
   dot
 
-macro `.?`*(option: typed, call: untyped{nkCall}): untyped =
-  ## The `.?` chaining operator is used to safely access fields and call procs
-  ## on Options or Results. The expression is only evaluated when the preceding
-  ## Option or Result has a value.
-
+macro chain(option: typed, call: untyped{nkCall}): untyped =
   let procedure = call[0]
   if call.len == 1:
     # chain is of shape: option.?procedure()
@@ -81,11 +61,15 @@ macro `.?`*(option: typed, call: untyped{nkCall}): untyped =
       expectReturnType(`procedure`, `call`)
       `option` ->? `call`
 
-macro `.?`*(option: typed, symbol: untyped): untyped =
-  ## The `.?` chaining operator is used to safely access fields and call procs
-  ## on Options or Results. The expression is only evaluated when the preceding
-  ## Option or Result has a value.
-
+macro chain(option: typed, symbol: untyped): untyped =
   symbol.expectSym()
   let expression = ident($symbol)
   quote do: `option`.?`expression`
+
+template `.?`*(left: typed, right: untyped): untyped =
+  ## The `.?` chaining operator is used to safely access fields and call procs
+  ## on Options or Results. The expression is only evaluated when the preceding
+  ## Option or Result has a value.
+  block:
+    let evaluated = left
+    chain(evaluated, right)

questionable/operators.nim

@@ -1,12 +1,19 @@
 template liftUnary*(T: type, operator: untyped) =
 
   template `operator`*(a: T): untyped =
-    a ->? `operator`(a.unsafeGet())
+    block:
+      let evaluated = a
+      evaluated ->? `operator`(evaluated.unsafeGet())
 
 template liftBinary*(T: type, operator: untyped) =
 
   template `operator`*(a: T, b: T): untyped =
-    (a, b) ->? `operator`(a.unsafeGet, b.unsafeGet)
+    block:
+      let evalA = a
+      let evalB = b
+      (evalA, evalB) ->? `operator`(evalA.unsafeGet, evalB.unsafeGet)
 
   template `operator`*(a: T, b: typed): untyped =
-    a ->? `operator`(a.unsafeGet(), b)
+    block:
+      let evalA = a
+      evalA ->? `operator`(evalA.unsafeGet(), b)

questionable/options.nim

@@ -44,7 +44,7 @@ template `->?`*[T,U,V](options: (?T, ?U), expression: ?V): ?V =
 template `->?`*[T,U,V](options: (?T, ?U), expression: V): ?V =
   options ->? expression.some
 
-template `|?`*[T](option: ?T, fallback: T): T =
+proc `|?`*[T](option: ?T, fallback: T): T =
   ## Use the `|?` operator to supply a fallback value when an Option does not
   ## hold a value.
 
@@ -56,11 +56,13 @@ template `|?`*[T](option: ?T, fallback: T): T =
 macro `.?`*[T](option: ?T, brackets: untyped{nkBracket}): untyped =
   let index = brackets[0]
   quote do:
-    type U = typeof(`option`.unsafeGet().?[`index`].unsafeGet())
-    if `option`.isSome:
-      `option`.unsafeGet().?[`index`]
-    else:
-      U.none
+    block:
+      let evaluated = `option`
+      type U = typeof(evaluated.unsafeGet().?[`index`].unsafeGet())
+      if evaluated.isSome:
+        evaluated.unsafeGet().?[`index`]
+      else:
+        U.none
 
 Option.liftUnary(`-`)
 Option.liftUnary(`+`)

questionable/results.nim

@@ -93,7 +93,7 @@ template `->?`*[T,U,V](values: (?!T, ?!U), expression: ?!V): ?!V =
 template `->?`*[T,U,V](values: (?!T, ?!U), expression: V): ?!V =
   values ->? expression.success
 
-template `|?`*[T,E](value: Result[T,E], fallback: T): T =
+proc `|?`*[T,E](value: Result[T,E], fallback: T): T =
   ## Use the `|?` operator to supply a fallback value when a Result does not
   ## hold a value.
 

testmodules/options/test.nim

@@ -115,17 +115,6 @@ suite "optionals":
     else:
       fail
 
-  test "=? evaluates optional expression only once":
-    var count = 0
-    if a =? (inc count; 42.some):
-      let b {.used.} = a
-    check count == 1
-
-    count = 0
-    if var a =? (inc count; 42.some):
-      let b {.used.} = a
-    check count == 1
-
   test "=? works in generic code":
     proc toString[T](option: ?T): string =
       if value =? option:
@@ -274,6 +263,58 @@ suite "optionals":
 
     check a.?[1] == 42.some
 
+  test ".? chain evaluates optional expression only once":
+    var count = 0
+    discard (inc count; @[41, 42].some).?len
+    check count == 1
+
+  test "=? evaluates optional expression only once":
+    var count = 0
+    if a =? (inc count; 42.some):
+      let b {.used.} = a
+    check count == 1
+
+    count = 0
+    if var a =? (inc count; 42.some):
+      let b {.used.} = a
+    check count == 1
+
+  test "|? evaluates optional expression only once":
+    var count = 0
+    discard (inc count; 42.some) |? 43
+    check count == 1
+
+  test ".?[] evaluates optional expression only once":
+    # indexing on optional sequence:
+    block:
+      var count = 0
+      discard (inc count; @[41, 42].some).?[0]
+      check count == 1
+    # indexing on normal sequence:
+    block:
+      var count = 0
+      discard (inc count; @[41, 42]).?[0]
+      check count == 1
+
+  test "lifted unary operators evaluate optional expression only once":
+    var count = 0
+    discard -(inc count; 42.some)
+    check count == 1
+
+  test "lifted binary operators evaluate optional expressions only once":
+    # lifted operator on two options:
+    block:
+      var count1, count2 = 0
+      discard (inc count1; 40.some) + (inc count2; 2.some)
+      check count1 == 1
+      check count2 == 1
+    # lifted operator on option and value:
+    block:
+      var count1, count2 = 0
+      discard (inc count1; 40.some) + (inc count2; 2)
+      check count1 == 1
+      check count2 == 1
+
   test "examples from readme work":
 
     var x: ?int

testmodules/result/test.nim

@@ -396,6 +396,61 @@ suite "result":
 
     check (a & b) == 42.success
 
+  test ".? chain evaluates result only once":
+    var count = 0
+    discard (inc count; @[41, 42].success).?len
+    check count == 1
+
+  test "=? evaluates result only once":
+    var count = 0
+    if a =? (inc count; 42.success):
+      let b {.used.} = a
+    check count == 1
+
+    count = 0
+    if var a =? (inc count; 42.success):
+      let b {.used.} = a
+    check count == 1
+
+  test "|? evaluates result only once":
+    var count = 0
+    discard (inc count; 42.success) |? 43
+    check count == 1
+
+  test ".?[] evaluates result only once":
+    var count = 0
+    discard (inc count; @[41, 42].success).?[0]
+    check count == 1
+
+  test "lifted unary operators evaluate result only once":
+    var count = 0
+    discard -(inc count; 42.success)
+    check count == 1
+
+  test "lifted binary operators evaluate results only once":
+    # lifted operator on two options:
+    block:
+      var count1, count2 = 0
+      discard (inc count1; 40.success) + (inc count2; 2.success)
+      check count1 == 1
+      check count2 == 1
+    # lifted operator on option and value:
+    block:
+      var count1, count2 = 0
+      discard (inc count1; 40.success) + (inc count2; 2)
+      check count1 == 1
+      check count2 == 1
+
+  test "conversion to option evaluates result only once":
+    var count = 0
+    discard (inc count; 42.success).option
+    check count == 1
+
+  test "conversion to error evaluates result only once":
+    var count = 0
+    discard (inc count; int.failure(error)).errorOption
+    check count == 1
+
   test "examples from readme work":
 
     proc works: ?!seq[int] =