result: add result assignment extenstion point for ?
#134
Conversation
This PR uses the facilities provided in status-im/nim-stew#134 to support `?` in `async` proc's.
arnetheduck
force-pushed
the
assign-result
branch
from
d8cda64
to
4577da4
Compare
stew/results.nim
Outdated
| else: | ||
| when E is void: | ||
| return err(typeof(result)) | ||
| when declared(assignResult): |
There was a problem hiding this comment.
I'm not sure this declared check would actually work for the Chronos use-case. Have you tested it?
There was a problem hiding this comment.
well, there's a test in status-im/nim-chronos#297 - not sure about more complex cases, anything in particular you're worried about?
There was a problem hiding this comment.
Ah, OK, I didn't realize the Chronos PR was tested precisely with this version of Result (there was the possibility that assignResult existed a globally defined template that is overloaded in Chronos).
It would be good to test additional use cases such as generic async procs or async closures defined in generic functions as these trigger substantially different symbol lookup paths in the compiler (which may trip the declared check here).
There was a problem hiding this comment.
added a generic test which seems to work for now - needed an extra mixin inside assignResult to not complain about the result symbol
stew/results.nim
Outdated
| @@ -953,25 +953,52 @@ template isNone*(o: Opt): bool = | |||
| # Syntactic convenience | |||
|
|
|||
| template `?`*[T, E](self: Result[T, E]): auto = | |||
| mixin assignResult | |||
There was a problem hiding this comment.
Since this is a very low-level integration mechanism between advanced Nim libraries, I suggest relying on the Nim `backticks` feature and use an identifier that's extremely unlike to appear in user code (e.g. `#assignResult`). It's also worth re-reading the discussion we had in the Chronos tracker:
There was a problem hiding this comment.
#assignResult doesn't compile - assignResult? is fine however, switched
arnetheduck
force-pushed
the
assign-result
branch
from
4577da4
to
f5dc106
Compare
This PR uses the facilities provided in status-im/nim-stew#134 to support `?` in `async` proc's.
|
marking draft for now - failing in 2.0 |
arnetheduck
force-pushed
the
assign-result
branch
from
f5dc106
to
cad8cdb
Compare
This PR uses the facilities provided in status-im/nim-stew#134 to support `?` in `async` proc's.
This PR uses the facilities provided in status-im/nim-stew#134 to support `?` in `async` proc's.
This PR uses the facilities provided in status-im/nim-stew#134 to support `?` in `async` proc's.
This PR uses the facilities provided in status-im/nim-stew#134 to support `?` in `async` proc's.
arnetheduck
force-pushed
the
assign-result
branch
from
d118fff
to
e68ca83
Compare
This PR uses the facilities provided in status-im/nim-stew#134 to support `?` in `async` proc's.
When `?` exits early, it does so using variations `return v` - however, in frameworks such as `chronos`, we need to change `return v` to `future.complete(v)` - this feature allows chronos to inject a template that performs this assignment instead of using the "ordinary" flow. Risk: `assignResult` might already be taken as a name, and multiple frameworks might compete for the functionality. Potential alternative: Instead of `assignResult`, this construct could be called `returnResult`, and it would be responsible for breaking the control flow (performing the `return` in addition to assigning the result). Other interactions: #34 proposes a general-purpose conversion framework - this could be used to automatically convert the error based on existing conversions - the use case is that oftentimes, one error needs to be converted to another in a call chain and injecting a converter simplifies this flow - this might however create an problematic interaction with an `assignResult` template in the future.
arnetheduck
force-pushed
the
assign-result
branch
from
e68ca83
to
59c7673
Compare
as an alternative to status-im/nim-stew#134, this PR provides an accidental workaround to the underlying issue, which is that macros are generally unable to rewrite `template` contents since the macro rewriting happens before template expansion. splitting up `result` assignment and `return` allows binding to the injected `result` and `return` without argument ends up being "implicitly" compatible with the way `async` code is turned into a closure iterator.
as an alternative to status-im/nim-stew#134, this PR provides an accidental workaround to the underlying issue, which is that macros are generally unable to rewrite `template` contents since the macro rewriting happens before template expansion. splitting up `result` assignment and `return` allows binding to the injected `result` and `return` without argument ends up being "implicitly" compatible with the way `async` code is turned into a closure iterator.
|
obsoleted by arnetheduck/nim-results#37 |
When
?exits early, it does so using variationsreturn v- however,in frameworks such as
chronos, we need to changereturn vtofuture.complete(v)- this feature allows chronos to inject a templatethat performs this assignment instead of using the "ordinary" flow.
Risk:
assignResultmight already be taken as a name, and multiple frameworksmight compete for the functionality.
Potential alternative:
Instead of
assignResult, this construct could be calledreturnResult, and it would be responsible for breaking the controlflow (performing the
returnin addition to assigning the result).Other interactions:
#34 proposes a general-purpose
conversion framework - this could be used to automatically convert the
error based on existing conversions - the use case is that oftentimes,
one error needs to be converted to another in a call chain and injecting
a converter simplifies this flow - this might however create an
problematic interaction with an
assignResulttemplate in the future.