results.? compatibity

[arnetheduck]

This PR uses the facilities provided in
status-im/nim-stew#134 to support ? in async
proc's.

[zah]

Instead of enabling this only in procs that return Result, I think it would be better to enable it always, but to use a name that is less likely to clash with user-defined symbols. You can use an identifier such as #asignResult for example.

[zah]

This would allow third party libraries to plug into the same mechanism.

Furthermore, it enables the creation of templates that create control-flow abstractions in async procs. ? is just one example.

[zah]

Another alternative is to call it just asyncReturn and to include the return statement in it. This would be the most convenient option for authoring templates like the ones that I mentioned. Many input validation patterns in the REST API can be abstracted away with such templates for example.

[arnetheduck]

Many input validation patterns in the REST API can be abstracted away with such templates for example.

this sounds like overloading the mechanism for an entirely different purpose, and as such would probably require a dedicated template - ? works only because it's part of the "generally known control flow vocabulary" like break - having random custom control flow hidden in templates sounds like something that might overobfuscate the code instead (something we've avoided pretty much everywhere).. can you provide a more specific code example of how this would look?

[zah]

There are plenty of reasonable use-cases where you might want to hide a control-flow statement behind a template. Consider this authorization check that must be repeated at the beginning of every request in the Keymanager API:

https://github.com/status-im/nimbus-eth2/blob/eb6b7affee4264e4c4b642a153289ce9f439c65c/beacon_chain/rpc/rest_key_management_api.nim#L117-L120

Extracting it in a template called checkAuthorization or return40xIfNotAuthorized is perfectly reasonable way to improve the factoring of the code.

Here is another example, featuring input validation logic for the ValidatorIndex values which is duplicated in many of the REST handlers:

https://github.com/status-im/nimbus-eth2/blob/eb6b7affee4264e4c4b642a153289ce9f439c65c/beacon_chain/rpc/rest_beacon_api.nim#L252-L259

[zah]

Well, I have already provided few very specific motivating examples here:
#297 (comment)

What would constitute "more clarity"?

And can you motivate in any way the inconsistency in the language that is being created here - using a return statement within templates is allowed in general, but it's not possible to use inside async procs. You seem to be arguing that Nim should not allow return in templates at all, but this ship has already sailed.

[arnetheduck]

What would constitute "more clarity"?

code.

[arnetheduck]

it's not possible to use inside async procs.

this is perhaps the core of the problem: to enable this / make it work, we're reaching for more and more convoluted constructs whose ability to scale in a composable way is .. dubious.

ie your own surprise that if compiles(templateHere()) works is a hint that what we're doing to achieve this goal is .. dubious and perhaps short-sighted - that it doesn't work in 2.0 is another hint that it's perhaps ill advised - I'm concerned that this will turn into non-composability along an axis if we overload the mechanism, so before venturing further in this direction at all, it stands to reason to examine it at smaller scale.

[arnetheduck]

here: #297 (comment)

the question is: is increasing the complexity of interaction between components the right solution here - or is decreasing the complexity of presto etc better? the success in combining a particular set of tools lies in using the tools efficiently together: some tools simply aren't good matches for each other - the same way with the authorization example: if you're working against the offered framework, you run into friction and you might be able to force your way a bit further but sometimes, it's the framework that needs changing..

[zah]

ie your own surprise that if compiles(templateHere()) works is a hint that what we're doing to achieve this goal is .. dubious and perhaps short-sighted - that it doesn't work in 2.0 is another hint that it's perhaps ill advised - I'm concerned that this will turn into non-composability along an axis if we overload the mechanism, so before venturing further in this direction at all, it stands to reason to examine it at smaller scale.

The more general solution seems to me equal in complexity to the current solution (i.e. it would exercise the same sub-systems within the compiler).

[Menduist]

Besides the alternative I proposed here and will test out when I find some time, a cooler solution would be to open an RFC on the Nim specs to allow:

proc hey: string =
  template `return`(i: int) = return $i
  return 5

to work. It could replace the whole "rewritting return" sheningan, fix the issue with results in a nice generic way, and doesn't seem stupid to me

Do you think it would make sense to open such RFC? (Obviously, it would take a long time, so we'll need another temporary solution anyway)

[arnetheduck]

ci fails are due to nimble, not the changes

[arnetheduck]

arnetheduck/nim-results#37 is one more entry in the ? compatibility line which works out-of-the-box now that #449 is merged

[arnetheduck]

obsoleted by arnetheduck/nim-results#37