From 03c5d14beac226bf54e0f915278b2ad374b9bf79 Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Tue, 13 Aug 2024 14:40:34 +0200 Subject: [PATCH 1/4] `lent` support `lent` allows returning a (hidden) pointer to a member thus making Result slightly more efficient when working with types that are expensive to copy. `lent` is enabled by default only on recent versions as Nim is known to generate invalid code in earlier releases - how far back `lent` works reliably depends on where Result appears but versions as recent as 1.6.14 have been known to have issues. As such, `lent` support is conservatively enabled only for 2.0.8+ but can be enabled for earlier versoins as well via a compile-time define. --- results.nim | 101 +++++++++++++++++++++++++++++++++++++++++-------- results.nimble | 22 ++++++----- 2 files changed, 98 insertions(+), 25 deletions(-) diff --git a/results.nim b/results.nim index 46cdc39..bcda10a 100644 --- a/results.nim +++ b/results.nim @@ -377,6 +377,18 @@ const resultsGenericsOpenSymWorkaroundHint* {.booldefine.} = true + resultsLent {.booldefine.} = (NimMajor, NimMinor, NimPatch) >= (2, 0, 8) + ## Enable return of `lent` types - this *mostly* works in Nim 1.6+ but there + ## have been edge cases reported as late as 1.6.14 - YMMV - conservatively, + ## `lent` is therefore enabled only with the latest Nim version at the time + ## of writing, where it could be verified to work with several large + ## applications + +when resultsLent: + template maybeLent(T: untyped): untyped = lent T +else: + template maybeLent(T: untyped): untyped = T + func raiseResultOk[T, E](self: Result[T, E]) {.noreturn, noinline.} = # noinline because raising should take as little space as possible at call # site @@ -840,7 +852,14 @@ func `==`*[T0, T1](lhs: Result[T0, void], rhs: Result[T1, void]): bool {.inline. of false: true -func value*[T, E](self: Result[T, E]): T {.inline.} = +func value*[E](self: Result[void, E]) {.inline.} = + ## Fetch value of result if set, or raise Defect + ## Exception bridge mode: raise given Exception instead + ## See also: Option.get + withAssertOk(self): + discard + +func value*[T: not void, E](self: Result[T, E]): maybeLent T {.inline.} = ## Fetch value of result if set, or raise Defect ## Exception bridge mode: raise given Exception instead ## See also: Option.get @@ -882,7 +901,7 @@ template unsafeValue*[E](self: Result[void, E]) = ## See also: `unsafeError` assert self.oResultPrivate # Emulate field access defect in debug builds -func tryValue*[T, E](self: Result[T, E]): T {.inline.} = +func tryValue*[E](self: Result[void, E]) {.inline.} = ## Fetch value of result if set, or raise ## When E is an Exception, raise that exception - otherwise, raise a ResultError[E] mixin raiseResultError @@ -890,10 +909,20 @@ func tryValue*[T, E](self: Result[T, E]): T {.inline.} = of false: self.raiseResultError() of true: - when T isnot void: - self.vResultPrivate + discard -func expect*[T, E](self: Result[T, E], m: string): T = +func tryValue*[T: not void, E](self: Result[T, E]): maybeLent T {.inline.} = + ## Fetch value of result if set, or raise + ## When E is an Exception, raise that exception - otherwise, raise a ResultError[E] + mixin raiseResultError + case self.oResultPrivate + of false: + self.raiseResultError() + of true: + # TODO https://github.com/nim-lang/Nim/issues/22216 + result = self.vResultPrivate + +func expect*[E](self: Result[void, E], m: string) = ## Return value of Result, or raise a `Defect` with the given message - use ## this helper to extract the value when an error is not expected, for example ## because the program logic dictates that the operation should never fail @@ -910,8 +939,27 @@ func expect*[T, E](self: Result[T, E], m: string): T = else: raiseResultDefect(m) of true: - when T isnot void: - self.vResultPrivate + discard + +func expect*[T: not void, E](self: Result[T, E], m: string): maybeLent T = + ## Return value of Result, or raise a `Defect` with the given message - use + ## this helper to extract the value when an error is not expected, for example + ## because the program logic dictates that the operation should never fail + ## + ## ```nim + ## let r = Result[int, int].ok(42) + ## # Put here a helpful comment why you think this won't fail + ## echo r.expect("r was just set to ok(42)") + ## ``` + case self.oResultPrivate + of false: + when E isnot void: + raiseResultDefect(m, self.eResultPrivate) + else: + raiseResultDefect(m) + of true: + # TODO https://github.com/nim-lang/Nim/issues/22216 + result = self.vResultPrivate func expect*[T: not void, E](self: var Result[T, E], m: string): var T = ( @@ -939,7 +987,7 @@ func `$`*[T, E](self: Result[T, E]): string = else: "err(" & $self.eResultPrivate & ")" -func error*[T, E](self: Result[T, E]): E = +func error*[T](self: Result[T, void]) = ## Fetch error of result if set, or raise Defect case self.oResultPrivate of true: @@ -948,10 +996,21 @@ func error*[T, E](self: Result[T, E]): E = else: raiseResultDefect("Trying to access error when value is set") of false: - when E isnot void: - self.eResultPrivate + discard -func tryError*[T, E](self: Result[T, E]): E {.inline.} = +func error*[T; E: not void](self: Result[T, E]): maybeLent E = + ## Fetch error of result if set, or raise Defect + case self.oResultPrivate + of true: + when T isnot void: + raiseResultDefect("Trying to access error when value is set", self.vResultPrivate) + else: + raiseResultDefect("Trying to access error when value is set") + of false: + # TODO https://github.com/nim-lang/Nim/issues/22216 + result = self.eResultPrivate + +func tryError*[T](self: Result[T, void]) {.inline.} = ## Fetch error of result if set, or raise ## Raises a ResultError[T] mixin raiseResultOk @@ -959,8 +1018,18 @@ func tryError*[T, E](self: Result[T, E]): E {.inline.} = of true: self.raiseResultOk() of false: - when E isnot void: - self.eResultPrivate + discard + +func tryError*[T; E: not void](self: Result[T, E]): maybeLent E {.inline.} = + ## Fetch error of result if set, or raise + ## Raises a ResultError[T] + mixin raiseResultOk + case self.oResultPrivate + of true: + self.raiseResultOk() + of false: + # TODO https://github.com/nim-lang/Nim/issues/22216 + result = self.eResultPrivate template unsafeError*[T; E: not void](self: Result[T, E]): E = ## Fetch error of result if set, undefined behavior if unset @@ -1488,7 +1557,7 @@ template `?`*[T, E](self: Result[T, E]): auto = # Collection integration -iterator values*[T, E](self: Result[T, E]): T = +iterator values*[T, E](self: Result[T, E]): maybeLent T = ## Iterate over a Result as a 0/1-item collection, returning its value if set case self.oResultPrivate of true: @@ -1496,7 +1565,7 @@ iterator values*[T, E](self: Result[T, E]): T = of false: discard -iterator errors*[T, E](self: Result[T, E]): E = +iterator errors*[T, E](self: Result[T, E]): maybeLent E = ## Iterate over a Result as a 0/1-item collection, returning its error if set case self.oResultPrivate of false: @@ -1504,7 +1573,7 @@ iterator errors*[T, E](self: Result[T, E]): E = of true: discard -iterator items*[T](self: Opt[T]): T = +iterator items*[T](self: Opt[T]): maybeLent T = ## Iterate over an Opt as a 0/1-item collection, returning its value if set case self.oResultPrivate of true: diff --git a/results.nimble b/results.nimble index ab87fa2..861484d 100644 --- a/results.nimble +++ b/results.nimble @@ -1,11 +1,11 @@ # Package -version = "0.4.0" -author = "Jacek Sieka" -description = "Friendly, exception-free value-or-error returns, similar to Option[T]" -license = "MIT" -skipDirs = @["benchmarks", "tests"] -installFiles = @["results.nim"] +version = "0.4.0" +author = "Jacek Sieka" +description = "Friendly, exception-free value-or-error returns, similar to Option[T]" +license = "MIT" +skipDirs = @["benchmarks", "tests"] +installFiles = @["results.nim"] # Dependencies requires "nim >= 1.2" @@ -15,12 +15,16 @@ proc test(env, path: string) = var lang = "c" if existsEnv"TEST_LANG": lang = getEnv"TEST_LANG" - exec "nim " & lang & " " & env & - " -r " & path + exec "nim " & lang & " " & env & " -r " & path task test, "Runs the test suite": for f in ["test_results.nim", "test_results2.nim"]: - for opt in ["", "-d:resultsGenericBindingWorkaround=false"]: + for opt in [ + "-d:resultsGenericsOpenSym:false", "-d:resultsGenericsOpenSym:true", + "-d:resultsGenericsOpenSymWorkaround=false", + "-d:resultsGenericsOpenSymWorkaround=true", "-d:resultsLent=false", + "-d:resultsLent=true", + ]: test opt, "tests/" & f if (NimMajor, NimMinor) >= (2, 0): test opt & " --mm:refc", "tests/" & f From 5213da5c567a1b71231a2b80102c9d921f93e04f Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Tue, 13 Aug 2024 14:45:16 +0200 Subject: [PATCH 2/4] less tests --- results.nimble | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/results.nimble b/results.nimble index 861484d..eda8848 100644 --- a/results.nimble +++ b/results.nimble @@ -21,9 +21,7 @@ task test, "Runs the test suite": for f in ["test_results.nim", "test_results2.nim"]: for opt in [ "-d:resultsGenericsOpenSym:false", "-d:resultsGenericsOpenSym:true", - "-d:resultsGenericsOpenSymWorkaround=false", - "-d:resultsGenericsOpenSymWorkaround=true", "-d:resultsLent=false", - "-d:resultsLent=true", + "-d:resultsLent=false", "-d:resultsLent=true", ]: test opt, "tests/" & f if (NimMajor, NimMinor) >= (2, 0): From fff0655aa9cf2854849376dc8f77af216431e1c1 Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Tue, 13 Aug 2024 14:49:25 +0200 Subject: [PATCH 3/4] don't test non-default lent setting it's failing all over the place --- results.nim | 10 +++++----- results.nimble | 5 +---- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/results.nim b/results.nim index bcda10a..bb94963 100644 --- a/results.nim +++ b/results.nim @@ -378,11 +378,11 @@ const resultsGenericsOpenSymWorkaroundHint* {.booldefine.} = true resultsLent {.booldefine.} = (NimMajor, NimMinor, NimPatch) >= (2, 0, 8) - ## Enable return of `lent` types - this *mostly* works in Nim 1.6+ but there - ## have been edge cases reported as late as 1.6.14 - YMMV - conservatively, - ## `lent` is therefore enabled only with the latest Nim version at the time - ## of writing, where it could be verified to work with several large - ## applications + ## Enable return of `lent` types - this *mostly* works in Nim 1.6.18+ but + ## there have been edge cases reported as late as 1.6.14 - YMMV - + ## conservatively, `lent` is therefore enabled only with the latest Nim + ## version at the time of writing, where it could be verified to work with + ## several large applications. when resultsLent: template maybeLent(T: untyped): untyped = lent T diff --git a/results.nimble b/results.nimble index eda8848..004c5d6 100644 --- a/results.nimble +++ b/results.nimble @@ -19,10 +19,7 @@ proc test(env, path: string) = task test, "Runs the test suite": for f in ["test_results.nim", "test_results2.nim"]: - for opt in [ - "-d:resultsGenericsOpenSym:false", "-d:resultsGenericsOpenSym:true", - "-d:resultsLent=false", "-d:resultsLent=true", - ]: + for opt in ["-d:resultsGenericsOpenSym:false", "-d:resultsGenericsOpenSym:true"]: test opt, "tests/" & f if (NimMajor, NimMinor) >= (2, 0): test opt & " --mm:refc", "tests/" & f From 865e3dbde3ac5bb117d58b08045775e8c8b24611 Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Wed, 14 Aug 2024 07:39:25 +0200 Subject: [PATCH 4/4] nph --- results.nim | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/results.nim b/results.nim index bb94963..9f8445a 100644 --- a/results.nim +++ b/results.nim @@ -385,9 +385,12 @@ const ## several large applications. when resultsLent: - template maybeLent(T: untyped): untyped = lent T + template maybeLent(T: untyped): untyped = + lent T + else: - template maybeLent(T: untyped): untyped = T + template maybeLent(T: untyped): untyped = + T func raiseResultOk[T, E](self: Result[T, E]) {.noreturn, noinline.} = # noinline because raising should take as little space as possible at call