-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reverse the order of arguments to ap
?
#803
Comments
I'm in favor of it as correction has already been made to be consistent with Haskell version but until we reverse params order, we can't be consistent and it leads to not very logic behavior as you've experimented yourself... |
I agree, it seems that reordering |
Okay we have at least 3 votes for this. Let's consider it ready for a PR. |
And it looks like truly low-hanging fruit, so I'll take it :) |
Looking for feedback I set about making the changes as specified in this ticket with the uncurried signature of: I ran into the type inference problems that can happen with this kind of signature, so I switched to the curried version to remedy that, as follows: I was able to make all the necessary changes project-wide with this solution, except for the generated code in the ApplyArityFunctions, which always resulted in type inference compile errors. I coded up a little experiment to get to the bottom of this, and also to isolate the problem from the task of codegen, and found the following: With our original signature of def ap[A, B](fa: F[A])(ff: F[A => B]): F[B], type inference in the implementation works fine (look at a0, a1, a2): def ap3[A, B, C, Z](fa: F[A], fb: F[B], fc: F[C])(ff: F[(A, B, C) => Z]): F[Z] = But, with the new signature of def ap[A, B](ff: F[A => B])(fa: F[A]): F[B], type inference doesn't work, and the implementation needs some help, like this: def ap3[A, B, C, Z](ff: F[%28A, B, C%29 => Z])(fa: F[A], fb: F[B], fc: F[C]): F[Z] = Given all this, should I:
If we go with 1), I should be able to wrap this up in fairly short order. |
@lukewyman Gotta edit / put those bad boys into code blocks: //... i.e. def ap[A,B,C,Z](ff: F[(A, B, C)] => Z]): F[Z] vs can use |
That was my mistake on Waffle. I didn't know how to format stuff on here, so the code blocks went away. My syntax is correct in real life. The original problem is legit. |
Ok, so I've finished making all the changes to core for this item. The issue is with the laws. I'm wondering how much of this is just fixing a little something, or if this may have to do with why the parameter lists for The law (as is) is stated as follows, with
And the error for the law with the change is:
Clearly, this would be false anyway, but is there a way to change how we state the law accordingly, or is the problem deeper? |
…m lists for Apply.ap
I was kind of curious and looked into this. My guess is it might have something to do with simulacrum-generated syntax since that's the only thing that I can find is using The // …
object Apply extends scala.AnyRef {
def <init>() = {
super.<init>();
()
};
@new inline() def apply[F[_]](implicit instance: Apply[F]): Apply[F] = instance;
abstract trait Ops[F[_], C] extends scala.AnyRef {
def $init$() = {
()
};
val typeClassInstance: Apply[F];
def self: F[C];
// the =:= is being used here ($eq$colon$eq)
def ap[A, B](fa: F[A])(implicit ev$macro$2: $eq$colon$eq[C, _root_.scala.Function1[A, B]]) = typeClassInstance.ap(self.asInstanceOf[F[_root_.scala.Function1[A, B]]])(fa);
def ap2[A, B, Z](fa: F[A], fb: F[B])(implicit ev$macro$3: $eq$colon$eq[C, _root_.scala.Function2[A, B, Z]]) = typeClassInstance.ap2(self.asInstanceOf[F[_root_.scala.Function2[A, B, Z]]])(fa, fb);
def map2[B, Z](fb: F[B])(f: _root_.scala.Function2[C, B, Z]) = typeClassInstance.map2(self, fb)(f)
};
// … I think the origin of It seems you might be able to avoid the problem by not using syntax but it's probably not the right way to go in the long run. |
This was resolved by #833 |
Currently the function signature for
Apply.ap
looks like this:However, there's a requirement (an unwritten one as far as I know) that the
F
effect for the second argument (ff
) is applied before the effect of the first argument (fa
). This dependency is relied upon for things like an implementation offoldMap
in terms ofConst
.This reverse order of effects has tripped us up a few times.
While we generally prefer to put functions as the last argument to a method, I recommend that we make an exception here and put
ff
beforefa
in theap
definition to help avoid this confusion. Also, since theff
function is wrapped in anF
context, the usual arguments for putting the function last probably don't apply quite as strongly. I believe @mandubian also is in favor of this change (correct me if I'm wrong, Pascal).The proposed new signature would be:
Or we could continue to use two parameter lists, but I don't know if there's any advantage in this case.
The text was updated successfully, but these errors were encountered: