From 0895a9c065800285095a70f9e9ea10449adba56a Mon Sep 17 00:00:00 2001
From: Mark Spanbroek <mark@spanbroek.net>
Date: Mon, 20 Jun 2022 17:36:24 +0200
Subject: [PATCH] Fix: `without` statement with error works in generic code

---
 questionable/binding.nim           | 21 +++++++++++++------
 questionable/private/binderror.nim | 19 +++++++++++++++++
 questionable/withoutresult.nim     | 33 +++---------------------------
 testmodules/result/test.nim        | 31 ++++++++++++++++++++++++----
 4 files changed, 64 insertions(+), 40 deletions(-)
 create mode 100644 questionable/private/binderror.nim

diff --git a/questionable/binding.nim b/questionable/binding.nim
index da01187..488caa2 100644
--- a/questionable/binding.nim
+++ b/questionable/binding.nim
@@ -1,5 +1,6 @@
 import std/options
 import std/macros
+import ./private/binderror
 
 proc option[T](option: Option[T]): Option[T] =
   option
@@ -8,17 +9,25 @@ proc placeholder(T: type): T =
   discard
 
 template bindLet(name, expression): bool =
-  let option = expression.option
+  let evaluated = expression
+  let option = evaluated.option
   type T = typeof(option.unsafeGet())
-  let name {.used.} = if option.isSome: option.unsafeGet() else: placeholder(T)
+  let name {.used.} = if option.isSome:
+    option.unsafeGet()
+  else:
+    bindFailed(evaluated)
+    placeholder(T)
   option.isSome
 
 template bindVar(name, expression): bool =
-  let option = expression.option
+  let evaluated = expression
+  let option = evaluated.option
   type T = typeof(option.unsafeGet())
-  var name {.used.} : T = placeholder(T)
-  if option.isSome:
-    name = option.unsafeGet()
+  var name {.used.} = if option.isSome:
+    option.unsafeGet()
+  else:
+    bindFailed(evaluated)
+    placeholder(T)
   option.isSome
 
 macro `=?`*(name, expression): bool =
diff --git a/questionable/private/binderror.nim b/questionable/private/binderror.nim
new file mode 100644
index 0000000..bf23195
--- /dev/null
+++ b/questionable/private/binderror.nim
@@ -0,0 +1,19 @@
+import std/options
+
+var captureEnabled {.global, compiletime.}: bool
+var errorVariable: ptr ref CatchableError
+
+template captureBindError*(error: var ref CatchableError, expression): auto =
+  static: captureEnabled = true
+  errorVariable = addr error
+  let evaluated = expression
+  static: captureEnabled = false
+  evaluated
+
+func error[T](option: Option[T]): ref CatchableError =
+  newException(ValueError, "Option is set to `none`")
+
+template bindFailed*(expression) =
+  when captureEnabled:
+    mixin error
+    errorVariable[] = expression.error
diff --git a/questionable/withoutresult.nim b/questionable/withoutresult.nim
index 1188db2..7872a80 100644
--- a/questionable/withoutresult.nim
+++ b/questionable/withoutresult.nim
@@ -1,39 +1,12 @@
-import std/macros
-import ./binding
 import ./without
+import ./private/binderror
 
-macro replaceInfix(expression, operator, replacement): untyped =
-  ## Replaces an infix operator in an expression. The AST of the expression is
-  ## traversed to find and replace all instances of the operator.
-
-  proc replace(expression, operator, replacement: NimNode): NimNode =
-    if expression.kind == nnkInfix and eqIdent(expression[0], operator):
-      expression[0] = replacement
-      expression[2] = replace(expression[2], operator, replacement)
-    else:
-      for i in 0..<expression.len:
-        expression[i] = replace(expression[i], operator, replacement)
-    expression
-
-  replace(expression, operator, replacement)
-
-template without*(condition, errorname, body) =
+template without*(condition, errorname, body): untyped =
   ## Used to place guards that ensure that a Result contains a value.
   ## Exposes error when Result does not contain a value.
 
   var error: ref CatchableError
 
-  # override =? operator such that it stores the error if there is one
-  template `override=?`(name, expression): bool {.gensym, used.} =
-    let optional = expression
-    when optional is Result:
-      if optional.isFailure:
-        error = optional.error
-    when optional is Option:
-      if optional.isNone:
-        error = newException(ValueError, "Option is set to `none`")
-    name =? optional
-
-  without replaceInfix(condition, `=?`, `override=?`):
+  without captureBindError(error, condition):
     template errorname: ref CatchableError = error
     body
diff --git a/testmodules/result/test.nim b/testmodules/result/test.nim
index 869b27d..a6689e1 100644
--- a/testmodules/result/test.nim
+++ b/testmodules/result/test.nim
@@ -265,6 +265,22 @@ suite "result":
 
     test()
 
+  test "without statement with error works in generic code":
+    proc test(_: type) =
+      without a =? int.failure "error", e:
+        check e.msg == "error"
+        return
+      fail
+
+    test(int)
+
+  test "without statements with error can be nested":
+    without a =? int.failure "error1", e1:
+      without b =? int.failure "error2", e2:
+        check e1.msg == "error1"
+        check e2.msg == "error2"
+      check e1.msg == "error1"
+
   test "catch can be used to convert exceptions to results":
     check parseInt("42").catch == 42.success
     check parseInt("foo").catch.error of ValueError
@@ -387,11 +403,11 @@ import pkg/questionable/resultsbase
 
 suite "result compatibility":
 
-  test "|?, =? and .option work on other types of Result":
-    type R = Result[int, string]
-    let good = R.ok 42
-    let bad = R.err "error"
+  type R = Result[int, string]
+  let good = R.ok 42
+  let bad = R.err "error"
 
+  test "|?, =? and .option work on other types of Result":
     check bad |? 43 == 43
 
     if value =? good:
@@ -400,3 +416,10 @@ suite "result compatibility":
       fail
 
     check good.option == 42.some
+
+  test "=? works on other type of Result after without statement with error":
+    without a =? 42.success, error:
+      discard error # fixes warning about unused variable "error"
+      fail
+    without b =? good:
+      fail