Skip to content
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

Topic/apply ap reverse #821

Closed
wants to merge 2 commits into from

Conversation

lukewyman
Copy link
Contributor

This PR will not pass validation, because the laws won't pass. I still submitted it so that others can look at the changes and see what the issue is. I changed the signature of Apply.ap to reverse the order of the parameter lists from:

def ap[A, B](fa: F[A])(ff: F[A => B]): F[B]

to

def ap[A, B](ff: F[A => B])(fa: F[A]): F[B]

which broke a whole bunch of laws. So I switched the laws around, for example, from:

def alternativeRightAbsorption[A, B](ff: F[A => B]): IsEq[F[B]] =
    (F.empty[A] ap ff) <-> F.empty[B]

to

def alternativeRightAbsorption[A, B](ff: F[A => B]): IsEq[F[B]] =
    (ff ap F.empty[A]) <-> F.empty[B]

and the the law still errors with a message of:

cats/laws/AlternativeLaws.scala:11: Cannot prove that A => B =:= A => B.
[error]     (ff ap F.empty[A]) <-> F.empty[B]

It seems that A => B =:= A => B should be a provable thing. Is there anywhere else that test data "feeds" the laws that I also need to switch around to compensate for my switching around the parameter lists of Apply.ap?

@lukewyman
Copy link
Contributor Author

Forgot to add the waffle ticket - it's #803

@ceedubs
Copy link
Contributor

ceedubs commented Jan 26, 2016

@lukewyman thanks a lot for all the work you've done on this. Sorry that I've been unresponsive - I've been traveling.

I didn't think about how changing the order of the arguments would play with Simulacrum. This does seem a bit unfortunate. Separately, I'm not sure why the compiler wouldn't be able to prove A => B =:= A => B.

Perhaps having the arguments line up with the order of effects isn't worth the hassle. But it also seems unfortunate if we arrive at this decision because of some syntax. @mpilquist do you have any thoughts on this?

@lukewyman
Copy link
Contributor Author

Thanks, @ceedubs. I'll continue working on this IF/when the Simulacrum issue proves resolvable. Interesting, too, that there are a different number of errors for travis between Scala 2.10 and 2.11. Also, there are a couple of broken laws that clearly aren't equal and thus shouldn't be provable. I double checked my code on those, and I'm pretty confident that I did the switch-around right, but perhaps an issue there too.

@travisbrown
Copy link
Contributor

I'm pretty sure the issue here is just that the type equality constraint doesn't play as well with type inference as evidence of a subtype relationship:

scala> class Wrapper[A](a: A) {
     |   def foo[B, C](b: B)(implicit ev: A =:= (B => C)): C = ev(a)(b)
     |   def bar[B, C](b: B)(implicit ev: A <:< (B => C)): C = ev(a)(b)
     | }
defined class Wrapper

scala> val wrapped = new Wrapper((i: Int) => i.toString)
wrapped: Wrapper[Int => String] = Wrapper@3d7e2448

scala> wrapped.foo(0)
<console>:10: error: Cannot prove that Int => String =:= Int => C.
              wrapped.foo(0)
                         ^

scala> wrapped.bar(0)
res8: String = 0

I'm looking now at Simulacrum to see whether the constraint could be generally relaxed without causing other problems, but maybe @mpilquist could answer that more quickly?

@ceedubs
Copy link
Contributor

ceedubs commented Jan 30, 2016

I don't think I've ever seen ap used directly in real-world code (outside of using it to implement map2, etc). For this reason, I'd be okay with us just documenting that the effect order is reversed and not trying to fight this battle. And since ap is often used to implement higher-level methods, if we suddenly need to start allocating extra implicit evidence instances to prove A => B =:= A => B, for all of our ap calls, that could be a further reason that this might not be worth it.

Just some thoughts.

@travisbrown
Copy link
Contributor

@ceedubs In my opinion a much better solution would be to require type annotations on all uses of ap, ap2, etc., which would work right now with no changes to Simulacrum. I agree that the extra evidence instances aren't ideal, but off the top of my head I don't see any reason to use ap (or ap2, etc.) to implement map2 or other operations, which (I believe) can always be written in terms of the more primitive map and product.

I've also just confirmed that changing this one line in Simulacrum to use <:< instead of =:= and publishing locally seems to fix everything. I've rebased this PR and added some minor fixes and an update to the Simulacrum dependency (which will only be available if you publish it locally) in this branch.

I think there are three ways forward:

  1. Update Simulacrum with this one line fix and merge this PR more or less as-is once the new Simulacrum (either 0.6.2 or 0.7.0) is published.
  2. Don't worry about changing Simulacrum for now and just merge this PR after adding type annotations to every ap call (or at least the syntax ones).
  3. Just don't reverse the order of arguments to the syntax ap.

I'd prefer the first if possible, but I don't think the second is that bad.

@ceedubs
Copy link
Contributor

ceedubs commented Jan 30, 2016

@travisbrown wow thanks for the fast and thorough assessment. The proposed Simulacrum change sounds good to me. And even if we do want to use ap to derive other methods, I'm realizing that we only pay the evidence allocation if we use syntax, which we probably wouldn't do. So I vote for proposal number 1 as long as @mpilquist doesn't object.

@mpilquist
Copy link
Member

👍 for simulacrum change. Happy to merge and publish a 0.6.2 today.

@travisbrown
Copy link
Contributor

@mpilquist Great, thanks! Do you want the PR against master and then a backport?

@travisbrown
Copy link
Contributor

One question: I was just updating the docs and noticed that the name of the ap method on CartesianBuilderN isn't really consistent now, since the function comes second, although the operation is still potentially useful. Maybe it should be apWith? mapF? Other suggestions?

@lukewyman
Copy link
Contributor Author

@ceedubs, will anything further be required of me on this ticket once the simulacrum change is done?

@ceedubs
Copy link
Contributor

ceedubs commented Jan 30, 2016

@lukewyman it looks like @travisbrown has incorporated your changes into #833, so I think we are all set here. I'm going to go ahead and close this out, but let me know if I missed anything. Thank you!

@ceedubs ceedubs closed this Jan 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants