-
-
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
Topic/apply ap reverse #821
Conversation
Forgot to add the waffle ticket - it's #803 |
@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 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? |
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. |
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? |
I don't think I've ever seen Just some thoughts. |
@ceedubs In my opinion a much better solution would be to require type annotations on all uses of I've also just confirmed that changing this one line in Simulacrum to use I think there are three ways forward:
I'd prefer the first if possible, but I don't think the second is that bad. |
@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 |
👍 for simulacrum change. Happy to merge and publish a 0.6.2 today. |
@mpilquist Great, thanks! Do you want the PR against master and then a backport? |
One question: I was just updating the docs and noticed that the name of the |
@ceedubs, will anything further be required of me on this ticket once the simulacrum change is done? |
@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! |
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:
to
which broke a whole bunch of laws. So I switched the laws around, for example, from:
to
and the the law still errors with a message of:
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?