result: cleanups (#22)

* fix ResultError type * add/fix documentation * clean up raises (in preparation for better Defect handling) * fix toException mixin * work around compiler bug with more explicit types * fix capture exception type * fix result type on `?`
status-im · Mar 30, 2020 · d622c07 · d622c07
1 parent 4477f45
commit d622c07
Show file tree

Hide file tree

Showing 2 changed files with 94 additions and 59 deletions.
diff --git a/stew/result.nim b/stew/result.nim
@@ -7,10 +7,10 @@
 # at your option. This file may not be copied, modified, or distributed except according to those terms.
 
 type
-  ResultError*[E] = ref object of ValueError
+  ResultError*[E] = object of ValueError
     ## Error raised when trying to access value of result when error is set
     ## Note: If error is of exception type, it will be raised instead!
-    error: E
+    error*: E
 
   Result*[T, E] = object
     ## Result type that can hold either a value or an error, but not both
@@ -19,7 +19,7 @@ type
     ##
     ## ```
     ## # It's convenient to create an alias - most likely, you'll do just fine
-    ## # with strings as error!
+    ## # with strings or cstrings as error
     ##
     ## type R = Result[int, string]
     ##
@@ -49,10 +49,10 @@ type
     ##
     ## # If you provide this exception converter, this exception will be raised
     ## # on dereference
-    ## func toException(v: Error): ref CatchableException = (ref CatchableException)(msg: $v)
+    ## func toException(v: Error): ref CatchableError = (ref CatchableError)(msg: $v)
     ## try:
     ##   RE[int].err(a)[]
-    ## except CatchableException:
+    ## except CatchableError:
     ##   echo "in here!"
     ##
     ## ```
@@ -92,6 +92,8 @@ type
     ## The API visibility issue of exceptions can also be solved with
     ## `{.raises.}` annotations - as of now, the compiler doesn't remind
     ## you to do so, even though it knows what the right annotation should be.
+    ## `{.raises.}` does not participate in generic typing, making it just as
+    ## verbose but less flexible in some ways, if you want to type it out.
     ##
     ## Many system languages make a distinction between errors you want to
     ## handle and those that are simply bugs or unrealistic to deal with..
@@ -117,6 +119,21 @@ type
     ## annotation that function may throw:
     ## https://developer.apple.com/library/content/documentation/Swift/Conceptual/Swift_Programming_Language/ErrorHandling.html
     ##
+    ## # Considerations for the error type
+    ##
+    ## * Use a `string` or a `cstring` if you want to provide a diagnostic for
+    ##   the caller without an expectation that they will differentiate between
+    ##   different errors. Callers should never parse the given string!
+    ## * Use an `enum` to provide in-depth errors where the caller is expected
+    ##   to have different logic for different errors
+    ## * Use a complex type to include error-specific meta-data - or make the
+    ##   meta-data collection a visible part of your API in another way - this
+    ##   way it remains discoverable by the caller!
+    ##
+    ## A natural "error API" progression is starting with `Option[T]`, then
+    ## `Result[T, cstring]`, `Result[T, enum]` and `Result[T, object]` in
+    ## escalating order of complexity.
+    ##
     ## # Other implemenations in nim
     ##
     ## There are other implementations in nim that you might prefer:
@@ -143,6 +160,11 @@ type
     ## * Rust uses From traits to deal with result translation as the result
     ##   travels up the call stack - needs more tinkering - some implicit
     ##   conversions would be nice here
+    ## * Pattern matching in rust allows convenient extraction of value or error
+    ##   in one go.
+    ##
+    ## Relevant nim bugs:
+    ## https://github.com/nim-lang/Nim/issues/13799
 
     case o: bool
     of false:
@@ -151,40 +173,42 @@ type
       v: T
 
 func raiseResultError[T, E](self: Result[T, E]) {.noreturn.} =
+  mixin toException
+
   when E is ref Exception:
     if self.e.isNil: # for example Result.default()!
-      raise ResultError[void](msg: "Trying to access value with err (nil)")
+      raise (ref ResultError[void])(msg: "Trying to access value with err (nil)")
     raise self.e
-  elif compiles(self.e.toException()):
-    raise self.e.toException()
+  elif compiles(toException(self.e)):
+    raise toException(self.e)
   elif compiles($self.e):
-    raise ResultError[E](
+    raise (ref ResultError[E])(
       error: self.e, msg: "Trying to access value with err: " & $self.e)
   else:
-    raise ResultError[E](error: self.e)
+    raise (res ResultError[E])(msg: "Trying to access value with err", error: self.e)
 
-template ok*(R: type Result, x: auto): auto =
+template ok*[T, E](R: type Result[T, E], x: auto): R =
   ## Initialize a result with a success and value
   ## Example: `Result[int, string].ok(42)`
   R(o: true, v: x)
 
-template ok*(self: var Result, x: auto) =
+template ok*[T, E](self: var Result[T, E], x: auto) =
   ## Set the result to success and update value
   ## Example: `result.ok(42)`
   self = ok(type self, x)
 
-template err*(R: type Result, x: auto): auto =
+template err*[T, E](R: type Result[T, E], x: auto): R =
   ## Initialize the result to an error
   ## Example: `Result[int, string].err("uh-oh")`
   R(o: false, e: x)
 
-template err*(self: var Result, x: auto) =
+template err*[T, E](self: var Result[T, E], x: auto) =
   ## Set the result as an error
   ## Example: `result.err("uh-oh")`
   self = err(type self, x)
 
-template ok*(v: auto): auto = typeof(result).ok(v)
-template err*(v: auto): auto = typeof(result).err(v)
+template ok*(v: auto): auto = ok(typeof(result), v)
+template err*(v: auto): auto = err(typeof(result), v)
 
 template isOk*(self: Result): bool = self.o
 template isErr*(self: Result): bool = not self.o
@@ -220,45 +244,62 @@ func mapCast*[T0, E0](
   if self.isOk: result.ok(cast[T1](self.v))
   else: result.err(self.e)
 
-template `and`*(self: Result, other: untyped): untyped =
+template `and`*[T, E](self, other: Result[T, E]): Result[T, E] =
   ## Evaluate `other` iff self.isOk, else return error
   ## fail-fast - will not evaluate other if a is an error
+  ##
+  ## TODO: This API is unsafe due to potential multiple
+  ## evaluation of the `self` parameter.
   if self.isOk:
     other
   else:
     type R = type(other)
     R.err(self.e)
 
-template `or`*(self: Result, other: untyped): untyped =
+template `or`*[T, E](self, other: Result[T, E]): Result[T, E] =
   ## Evaluate `other` iff not self.isOk, else return self
   ## fail-fast - will not evaluate other if a is a value
+  ##
+  ## TODO: This API is unsafe due to potential multiple
+  ## evaluation of the `self` parameter.
   if self.isOk: self
   else: other
 
-template catch*(body: typed): Result[type(body), ref Exception] =
-  ## Convert a try expression into a Result
-  type R = Result[type(body), ref Exception]
+template catch*(body: typed): Result[type(body), ref CatchableError] =
+  ## Catch exceptions for body and store them in the Result
+  ##
+  ## ```
+  ## let r = catch: someFuncThatMayRaise()
+  ## ```
+  type R = Result[type(body), ref CatchableError]
 
   try:
     R.ok(body)
-  except:
-    R.err(getCurrentException())
-
-template capture*(T: type, e: ref Exception): Result[T, ref Exception] =
-  type R = Result[T, ref Exception]
+  except CatchableError as e:
+    R.err(e)
+
+template capture*[E: Exception](T: type, someExceptionExpr: ref E): Result[T, ref E] =
+  ## Evaluate someExceptionExpr and put the exception into a result, making sure
+  ## to capture a call stack at the capture site:
+  ##
+  ## ```
+  ## let e: Result[void, ValueError] = void.capture((ref ValueError)(msg: "test"))
+  ## echo e.error().getStackTrace()
+  ## ```
+  type R = Result[T, ref E]
 
   var ret: R
   try:
     # TODO is this needed? I think so, in order to grab a call stack, but
     #      haven't actually tested...
     if true:
       # I'm sure there's a nicer way - this just works :)
-      raise e
-  except:
-    ret = R.err(getCurrentException())
+      raise someExceptionExpr
+  except E as caught:
+    ret = R.err(caught)
   ret
 
-func `==`*(lhs, rhs: Result): bool {.inline.} =
+func `==`*[T0, E0, T1, E1](lhs: Result[T0, E0], rhs: Result[T1, E1]): bool {.inline.} =
   if lhs.isOk != rhs.isOk:
     false
   elif lhs.isOk:
@@ -307,7 +348,7 @@ func `$`*(self: Result): string =
   else: "Err(" & $self.e & ")"
 
 func error*[T, E](self: Result[T, E]): E =
-  if self.isOk: raise ResultError[void](msg: "Result does not contain an error")
+  if self.isOk: raise (ref ResultError[void])(msg: "Result does not contain an error")
 
   self.e
 
@@ -331,6 +372,13 @@ template ok*[E](self: var Result[void, E]) =
   ## Example: `result.ok(42)`
   self = (type self).ok()
 
+template ok*(): auto = ok(typeof(result))
+template err*(): auto = err(typeof(result))
+
+# TODO:
+# Supporting `map` and `get` operations on a `void` result is quite
+# an unusual API. We should provide some motivating examples.
+
 func map*[E, A](
     self: Result[void, E], f: proc(): A): Result[A, E] {.inline.} =
   ## Transform value using f, or return error
@@ -379,11 +427,16 @@ template value*[E](self: var Result[void, E]) = self.get()
 template `?`*[T, E](self: Result[T, E]): T =
   ## Early return - if self is an error, we will return from the current
   ## function, else we'll move on..
+  ##
+  ## ```
+  ## let v = ? funcWithResult()
+  ## echo v # prints value, not Result!
+  ## ```
   ## Experimental
   # TODO the v copy is here to prevent multiple evaluations of self - could
   #      probably avoid it with some fancy macro magic..
-  let v = self
-  if v.isErr: return v
+  let v = (self)
+  if v.isErr: return err(typeof(result), v.error)
 
   v.value
 
diff --git a/tests/test_result.nim b/tests/test_result.nim
@@ -10,7 +10,7 @@ func fails(): R = R.err("dummy")
 func fails2(): R = result.err("dummy")
 
 func raises(): int =
-  raise newException(Exception, "hello")
+  raise (ref CatchableError)(msg: "hello")
 
 # Basic usage, consumer
 let
@@ -83,13 +83,15 @@ doAssert (rOk.flatMap(
 doAssert (rErr.mapErr(func(x: string): string = x & "no!").error == (rErr.error & "no!"))
 
 # Exception interop
-let e = capture(int, newException(Exception, "test"))
+let e = capture(int, (ref ValueError)(msg: "test"))
 doAssert e.isErr
+doAssert e.error.msg == "test"
+
 try:
   discard e[]
   doAssert false, "should have raised"
-except:
-  doAssert getCurrentException().msg == "test"
+except ValueError as e:
+  doAssert e.msg == "test"
 
 # Nice way to checks
 if (let v = works(); v.isOk):
@@ -145,33 +147,13 @@ func testErr(): Result[int, string] =
 doAssert testOk()[] == 42
 doAssert testErr().error == "323"
 
-# It's also possible to use the same trick for stack capture:
-template capture*(): untyped =
-  type R = type(result)
-
-  var ret: R
-  try:
-    # TODO is this needed? I think so, in order to grab a call stack, but
-    #      haven't actually tested...
-    if true:
-      # I'm sure there's a nicer way - this just works :)
-      raise newException(Exception, "")
-  except:
-    ret = R.err(getCurrentException())
-  ret
-
-proc testCapture(): Result[int, ref Exception] =
-  return capture()
-
-doAssert testCapture().isErr
-
 func testQn(): Result[int, string] =
   let x = ?works() - ?works()
   result.ok(x)
 
 func testQn2(): Result[int, string] =
   # looks like we can even use it creatively like this
-  if ?fails() == 42: raise newException(Exception, "shouldn't happen")
+  if ?fails() == 42: raise (ref ValueError)(msg: "shouldn't happen")
 
 doAssert testQn()[] == 0
 doAssert testQn2().isErr
@@ -180,7 +162,7 @@ type
   AnEnum = enum
     anEnumA
     anEnumB
-  AnException = ref object of Exception
+  AnException = ref object of CatchableError
     v: AnEnum
 
 func toException(v: AnEnum): AnException = AnException(v: v)