Add monoid instances for uncurried EffectFns

[masaeedu]

No description provided.

[garyb]

I think this looks reasonable from a convenience point of view, but it's kinda defeating one of the major reasons EffectFn is useful - that the calls are inlined as uncurried when run as fully saturated.

[masaeedu]

@garyb I could write these using the FFI if you prefer. The reason I need these is because there's web ui related APIs that work heavily with records consisting almost entirely of EffectFns (usually with Unit output), and it's handy to be able to mash together these records. Without a monoid instance for EffectFns (and sadly I can't add an orphan instance myself), the records are not themselves monoids.

[garyb]

I'm not sure resorting to the FFI will help, but lemme try something out a minute, I have an idea that might work.

[garyb]

Ok, the good news is: it works! The bad news is: it only works when you do it outside of this module 😭. I'd consider that something that should be fixed in the compiler though.

So my idea is just: just use full saturation when defining the instances too. For example:

instance semigroupEffectFn2 :: Semigroup r => Semigroup (EffectFn2 a b r) where
  append f1 f2 = mkEffectFn2 \x y -> runEffectFn2 f1 x y <> runEffectFn2 f2 x y

If you write a function that does this outside of the module you can see that when compiled it'll produce this JS:

var foo = function (dictSemigroup) {
    return function (f1) {
        return function (f2) {
            return function (x, y) {
                return Data_Semigroup.append(Effect.semigroupEffect(dictSemigroup))(function () {
                    return f1(x, y);
                })(function () {
                    return f2(x, y);
                })();
            };
        };
    };
};

Rather than:

var foo = function (dictSemigroup) {
    return function (f1) {
        return function (f2) {
            return Effect_Uncurried.mkEffectFn2(Data_Semigroup.append(Data_Semigroup.semigroupFn(Data_Semigroup.semigroupFn(Effect.semigroupEffect(dictSemigroup))))(Effect_Uncurried.runEffectFn2(f1))(Effect_Uncurried.runEffectFn2(f2)));
        };
    };
};

[masaeedu]

The thing that breaks my heart is I actually tediously wrote all this stuff expanded out like that a couple days ago, then I was like AHA, I AM SO SMART, and deleted all the explicit parameters.

Should I do the same thing for all the monoid instances (mkEffectFn2 \_ _ -> mempty)?

[garyb]

😄 in any other situation eta-reduction is completely fine, it's just the EffectFn and Fn modules that are handled specially.

Yeah, I think doing the monoid instances similarly makes sense.

[thomashoneyman]

Oof, I didn’t realize this would remove the uncurrying — which is the primary reason I reach for these functions. Thanks for the explanation @garyb

[masaeedu]

Ok, I've eta-expanded all of them.

[thomashoneyman]

I think they could use a documentation comment (internal, not for Pursuit), so that future people working on this understand the reason for eta-expansion.

[masaeedu]

@thomashoneyman How's that?

[thomashoneyman]

Uncurrying is also good for JS compatibility for when you specifically want to be able to call functions as f(a, b, c), which is more idiomatic in JS, instead of f(a)(b)(c).

Plus, EffectFn removes the thunk so you can just call f(a, b) instead of f(a)(b)().

I’m not totally sure how much of that belongs in a comment here, but just for some extra context. So long as the comment helps ensure future edits don’t attempt to clean things up by removing eta expansion I think it’s good.

[masaeedu]

@thomashoneyman I think the PR is set to allow commits from maintainers, so if you want to flesh out the comment so it captures this information please feel free.

[thomashoneyman]

I am not a maintainer, unfortunately 😄, but I think omitting (which are more efficient) would be the best move, as then you're simply noting that this formulation causes the compiler to emit uncurried functions, which is all that really needs to be said.

[thomashoneyman]

Speaking of, @garyb I'm not able to merge this.

[hdgarrood]

Uncurrying is also good for JS compatibility for when you specifically want to be able to call functions as f(a, b, c), which is more idiomatic in JS, instead of f(a)(b)(c).

Plus, EffectFn removes the thunk so you can just call f(a, b) instead of f(a)(b)().

These are both covered by the existing doc comments surely?

[hdgarrood]

I am not a maintainer, unfortunately 😄, but I think omitting (which are more efficient) would be the best move, as then you're simply noting that this formulation causes the compiler to emit uncurried functions, which is all that really needs to be said.

I don’t think this is quite right. The comment is talking about the emitted code for the instances themselves, not the objects you end up with when you use these instances. Both variants (eta-expanded or not) will look the same from outside of this module. The only difference between them is that eta-expanding means that we end up with fewer function objects being allocated in the implementation - i.e. it is a matter of efficiency.

[thomashoneyman]

These are both covered by the existing doc comments surely?

Yes; the module has a lengthy comment explaining this in detail. It doesn't need to be repeated.

The comment is talking about the emitted code for the instances themselves, not the objects you end up with when you use these instances. Both variants (eta-expanded or not) will look the same from outside of this module. The only difference between them is that eta-expanding means that we end up with fewer function objects being allocated in the implementation - i.e. it is a matter of efficiency.

In that case I misunderstood. Thanks for clarifying, and the comment seems fine as-is with that in mind.

[masaeedu]

I am not a maintainer, unfortunately

Ah, whoops. I wish Github would make it easier for conversation participants to contribute tweaks to a PR directly, it would make the process so much faster.

[hdgarrood]

I think this is good as-is; I think the only remaining question is whether we want to wait until the optimizer is fixed so that it fires on these instances or not. Personally I think it’s probably fine to merge this now.

[hdgarrood]

Thanks!