Closed bossmc closed 5 years ago
My gut feeling (without having looked at the implementation yet) is that Result should only recurse into T
, not the error type. Support for one that does both sides could be provided as integration with the either
crate (a type specifically designed so that both type parameters play similar roles).
My gut feeling (without having looked at the implementation yet) is that Result should only recurse into
T
, not the error type. Support for one that does both sides could be provided as integration with theeither
crate (a type specifically designed so that both type parameters play similar roles).
The use case I'm looking at is for converting between types that appear in auto-generated crates, in many cases the internal types are created from the same inputs to the auto-generation so they're "identical", but because they're in different crates they're deemed different. These types get used in all kinds of containers within the autogenerated code (in particular, some get used as the T
of Result
s and some are used as the E
of Result
s) hence the desired to be able to transmogrify these objects wherever they appear in container types.
Can I see where Result
appears in your generated code? To me, Result
is a control flow construct that should seldom appear inside a type (except in something ephemeral and generic like std::iter::Peekable<std::io::Lines>
). For something that is semantically a container, one should be using Either
. (and if Result
only appears as the top-level type, I'm not sure what the issue is with using .map()
or .map_err()
unless the calls to .transmogrify()
are also in generated code)
I've had a chat with various people, and we've concluded you're correct, the Result
instances are always the top-level type (i.e. not nested in other structures). I can either remove the impl completely, or restrict it to only allowing the T to be recursed into. I suspect that, given the existence of map(|t| t.transmogrify())
you'll say remove it.
I am equally fine with either option. Having it goes well together with the Option
impl. If you feel that others may expect it to transform the error type, however, then removing it may be the safer bet.
Given a result: Result<T, Err1>
inside a fn foo -> Result<T, Err2>
, when you do let t = result?
, if there's an error, the compiler is hard-coded to do a transformation from Err1
to Err2
if there exists an impl From<Err2> for Err1
right? On the surface, it feels similar to what's being done here, albeit I can also see that transmogrification might be considered more "magical" and the semantics are a bit different...
I actually don't have a good conclusion for this trail of thought, but just thought it might help advance the discussion :)
As a pragmatic solution I'm going to remove the Result
support completely from this PR, as the rest seems uncontroversial, and raise a separate PR for Result
support (and further discussion).
I've applied the above markups (and removed the Result
support).
I've not pushed the Result
implementation as I think there's a nicer solution which I'm prototyping now.
I've not pushed the
Result
implementation as I think there's a nicer solution which I'm prototyping now.
Nice. I wonder if we can make something generic so that the work you do there Dan be used to make it possible to derive LabelledGeneric
for enum
s in the general case. Perhaps we can make use of coproduct?
Inspiration : GHC uses it for the equiv of enum there (Multi constructor)
Indeed, that's the plan, I've got the Transmogrify
impl working and I'm making good progress on the derive
enhancements. I'm currently working "out of tree" by importing frunk-proc-macro-helpers
(so I can deliver something to my coworkers while we thrash out the upstreaming).
I didn't think to use Coproduct
because I'm not super up on my Category Theory, so the current implementation uses
enum HEither<H, T> {
Head(H),
Tail(T),
}
With a common Repr
being HEither<T1, HEither<T2, HEither<T3, Void>>>
(plus labels for conversion).
Nice :) That works too; that's basically all Coproduct
really is anyway :D Sounds like you're cooking up some awesome stuff; can't wait to see it :)
Phew, that took longer than expected... I had a working code base weeks ago, then the real world got in the way.
To allow me to ship this function to teams at work, I've actually done the implementation in a separate pair of crates, which I've just open-sourced at https://github.com/Metaswitch/frunk-enum in case you're interested.
Add transmogrification through:
Box<T>
Option<T>
LinkedList<T>
VecDeque<T>
Result<T, U>
U
andT
can be recursed intoThe
Result
implementation is more complicated because it's possible for theT
to be a labelled generic while theU
isn't (or vice-versa) and hence I've added a properRepr
for it and a case for transmogrifying theRepr
s based on the internals.Unit test included for all of the above.