Closed hemangandhi closed 5 years ago
Bug found in the PR:
Applicative (also Monad) example is failing due to following issue
error[E0277]: cannot add `{integer}` to `std::option::Option<{integer}>`
--> fp-examples/src/applicative_example.rs:6:47
|
6 | let x = Option::of(Some(1)).ap(Some(|x| x + 1));
| ^ no implementation for `std::option::Option<{integer}> + {integer}`
|
= help: the trait `std::ops::Add<{integer}>` is not implemented for `std::option::Option<{integer}>`
I think it's because in below impl
of Option
impl<A, F, B> Apply<F, B> for Option<A>
where
F: FnOnce(A) -> B,
{
fn ap(self, f: Self::Target2) -> Self::Target {
self.and_then(|v| {
f.map(|z| z(v))
})
}
}
A
somehow forces Option<A>
to become a nested option. So even if you self.and_then
you get another option back. I feel like we should revert Self::HKT::Current
into A
and make it easier to fix the issue (also easier to read)
Looks good now, I've mentioned follow up tasks in our telegram chat. I am going to work on them ;)
@hemangandhi congrats on your 2nd PR to fp-core! this is a massive one!
Specifically, I did the following:
HKT
not have theCurrent
type as an extra generic parameter.HKT
.impl Pure<A> for Option<A>
andPure<A>
itself.Vec
is aHKT
now, so there's a concrete instance for us to play with).What was wrong with:
B
inHKT<B>
). The names make a lot more sense and implementing it is very straight-forward. The only current quirk is documented in a comment onFoldable
: in essence, we don't neatly just say "this type contains another type": we still insist that "this type contains someCurrent
and can be converted toTarget
". We now have what I might call the "A B B A idiom" dotting the code:impl<A, B> ___<B> for ___<A>
(where___<A>: HKT<B>
). This arises so that we can access both the type we want to convert to (HKT<B>::Target
) and the type we're starting with (HKT<B>::Current
which seems to always beA
).Self<A> -> Self<A>
was apparently what the type implied, but we really wanted aA -> Self<A>
. This might be becauseHKT
was harder to reason about. The discrepancy should be made clear in the instance forOption
: originally, there was nothing especially default-y about it, we just had an identity (None.pure()
, for example, should not exist, butOption::pure(3)
should beSome(3)
).fold_map
made a big mess, so I moved it out of the trait (it's easy to just implement for anyFoldable<M> where M: Monoid
). Also, adding an instance forVec
was eye-opening:FnOnce
is very wrong for folding since folding expressly calls the closure more than once. This will have to be fixed everywhere aFnOnce
is used: we might as well just use aFn
or discuss the correct trait further.