status-im / nim-stew

stew is collection of utilities, std library extensions and budding libraries that are frequently used at Status, but are too small to deserve their own git repository.
133 stars 18 forks source link

results: collections integration #179

Closed arnetheduck closed 1 year ago

arnetheduck commented 1 year ago

This set of helpers allows treating Result and Opt as collections of 0 or 1 item, allowing iterating over them and checking "membership" - such integration is useful in generic code which can then be generalised to handle more complex cases - the integration is most useful with Opt.

One design tradeoff here is the "explicitness" of items vs values for Result - technically error and value are "equal" and therefore we shouldn't give preference to the value, but there exists a convenience argument to treat the value as the "default" and therefore define items / contains for Result as well - this PR chooses the more conservative and explicit approach - a more liberal version can easily be added later should motivating examples emerge.

zah commented 1 year ago

There are some dangers of overly generic code.

Consider the situation where a function foo returns Opt[seq[T]]. The users might accidentally write: for x in foo(): expecting to iterate over the sequence, but instead they will trigger the items iterator over the Opt.

Granted, this is likely to produce another error within the body of the loop, but with sufficient use of type inference the error message may turn out to be quite distant from the root cause.

Due to the way how generics are supposed to mixin symbols from their caller scope, there is an alternative way to provide this genericity as an opt-in. You can place the definitions in a separate module that users who are interested in this generic treatment of Results as collections can import in their code which calls the generic algorithms they want to use. Unfortunately, due to current issues such as the generic sandwitch, a solution like this would still be problematic in practice.

arnetheduck commented 1 year ago

There are some dangers of overly generic code.

That generic code is not in Result however? Ie the code presented here is quite simple.

That said, "generic" here means converters, sequtils and other "functional"-style api:s that might become more popular now that iterators finally can be combined more naturally - these helpers in Result simply integrate Result with more parts of the language.

zah commented 1 year ago

That generic code is not in Result however? Ie the code presented here is quite simple.

I don't understand this remark. Clearly the problematic example that I have provided will be possible with the current code.

arnetheduck commented 1 year ago

You can place the definitions in a separate module

Yeah, the sandwitch issue is messy so for a central type like this, I would probably avoid it. I would probably use separate modules for anything that brings in additional imports (say .. std/options integration or I/O) but not for things that depend only on the Result type itself - from a UX perspective, this is often what you want as a developer, ie you don't want something to pull in deps you end up not using, but the dead-code-elimination feature of nim means the small helpers are "harmless" and convenient to always have available.

arnetheduck commented 1 year ago

I don't understand this remark. Clearly the problematic example that I have provided will be possible with the current code.

sure - what I meant was that the judgement of "what kind" of code to use with these features is on the user, the code inside the Result module is not "overly generic", ie it doesn't contain too-open generics that would accidentally have negative effects on surrounding code.

and yeah, if someone forgets to dereference in the for loop, that should quickly become apparent - it's like when usesing seq[seq[.. which would cause the same kind of error messages.

combining functional helpers is an incredibly powerful technique in languages that support it well - it "fits" Result that way, though the lack of good functional support in Nim in general has so far hampered progress on this front.

zah commented 1 year ago

Opt[seq] is likely to be more common than seq[seq], but my comment didn't meant to block this PR from merging - merely to raise awareness of the potential risk. We can give it a try and see how things go in practice.