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

Enable SI-2712 fix in cats / Remove unapply machinery #1583

Merged
merged 5 commits into from
Apr 21, 2017

Conversation

kailuowang
Copy link
Contributor

@kailuowang kailuowang commented Apr 1, 2017

fixes #1073
I also added an FAQ item to help people running into SI-2712

@@ -85,7 +85,7 @@ class RegressionTests extends CatsSuite {

test("#500: foldMap - traverse consistency") {
assert(
List(1,2,3).traverseU(i => Const(List(i))).getConst == List(1,2,3).foldMap(List(_))
List(1,2,3).traverse(i => Const.of[List[Int]](List(i))).getConst == List(1,2,3).foldMap(List(_))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for some reason the traverse version can no longer infer the type correctly. Thus I had to pass the type into Const's apply method. For convenience, I added an of factory method to Const. Please shout out if you know a better solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not nice. Perhaps Const needs its type params reversed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would that affect partial unification?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial unification is strictly left to right. If Const varies on the left (can't look right now), then it wouldn't work with partial unification.as a rule of thumb, think about how you would declare the type constructor in Haskell.

Copy link
Contributor Author

@kailuowang kailuowang Apr 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djspiewak it varies on the right. @edmundnoble any other suggestion in terms of what to do with this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might it have something to do with Scala inferring Nothing on the unused type parameter which can make the compiler freak out since it treats Nothing specially?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that specifying the type parameter here is an acceptable tradeoff for getting rid of the Unapply machinery, but paging @milessabin to see if he has any thoughts on this popping up.

Copy link
Contributor Author

@kailuowang kailuowang Apr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chatted with @milessabin IRT to this.
Here is the minimised version in case anyone is interested.

    def trav[F[_], A, B](f: A => F[B]): F[B] = ???
    case class C[A, B](a: A)
    val c: C[Int, Int] = trav(C(_:Int))   //compiles
    val a: Int  = trav(C(_:Int)).a    //doesn’t compile

[error] found : Int => C[Int,Nothing]
[error] required: Int => C[Int,B]
[error] val a: Int = trav(C(_:Int)).a

Our conclusion is that might have to live with this as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you make C covariant in B?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I just tried it myself. It resolves the issue.

@kailuowang kailuowang added this to the cats-1.0.0 milestone Apr 1, 2017
@codecov-io
Copy link

codecov-io commented Apr 1, 2017

Codecov Report

Merging #1583 into master will increase coverage by 0.29%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1583      +/-   ##
==========================================
+ Coverage   93.08%   93.38%   +0.29%     
==========================================
  Files         250      240      -10     
  Lines        3989     3943      -46     
  Branches      138      141       +3     
==========================================
- Hits         3713     3682      -31     
+ Misses        276      261      -15
Impacted Files Coverage Δ
free/src/main/scala/cats/free/FreeT.scala 93.58% <ø> (+1.18%) ⬆️
core/src/main/scala/cats/Traverse.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/apply.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/data/EitherT.scala 96.19% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/reducible.scala 100% <ø> (+60%) ⬆️
core/src/main/scala/cats/Foldable.scala 93.65% <ø> (-0.2%) ⬇️
core/src/main/scala/cats/syntax/flatMap.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/data/Func.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/cartesian.scala 75% <ø> (-10.72%) ⬇️
core/src/main/scala/cats/syntax/foldable.scala 100% <ø> (ø) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3267664...df8aaab. Read the comment docs.

@@ -39,6 +39,12 @@ final case class Const[A, B](getConst: A) {
object Const extends ConstInstances {
def empty[A, B](implicit A: Monoid[A]): Const[A, B] =
Const(A.empty)

class partialApply[B] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps change this to something like OfPartiallyApplied to be consistent with https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/data/EitherT.scala#L275

@kailuowang
Copy link
Contributor Author

Thanks to @djspiewak, the type inference glitch is resolved. Does any maintainer have time to give it another look?

@ceedubs
Copy link
Contributor

ceedubs commented Apr 12, 2017

I'm not sure about making B covariant in Const to get around the inference. Since it's a phantom type it could just as easily be treated as contravariant. I'm hesitant to choose one over the other in the name of (questionable) inference.

There's some more information about phantom types and variance on the typelevel blog about phantom types in scala for anyone who is interested.

@ceedubs
Copy link
Contributor

ceedubs commented Apr 12, 2017

cc @djspiewak about my last message. You had good insight that making the parameter covariant might get around the inference issue. What are your thoughts on actually doing this for Const though?

@kailuowang
Copy link
Contributor Author

kailuowang commented Apr 12, 2017

@ceedubs is it a phantom type? I'm literally not sure. It appears in both covariant and contravariant positions (as part of type constructors such as Const[A, B] and Function1).

I agree it that feels hackish when the only reason to chose this variance is for type inference. On the other hand, for Const, I am not aware of any need of subtyping a Const[A, B](since there is retag?), so I am not sure about the downside of choosing a variance either.

@djspiewak
Copy link
Member

@ceedubs It could certainly be either covariant or contravariant, and so by that argument would correctly be invariant. However, Scala's type inference biases downward, not upward and not "in place". It always tries to drive things to Nothing. For that reason, covariant is the correct default in any phantom position where inference is relevant.

Though I agree it seems awfully arbitrary.

@kailuowang As long as there's no need to subtype Const, I think it's safe to just toss the + on there and call it a day. We should make it clear in the release notes for RC1 that this was done, so people who use Const can take a crack at it and see if their own particular niche use-case is broken.

@milessabin
Copy link
Member

I share everyone's discomfort, but I think this is the right thing to do.

@ceedubs
Copy link
Contributor

ceedubs commented Apr 13, 2017

Thanks for the input @djspiewak and @milessabin. @djspiewak you raise some good points, and I don't think that I'm opposed to the B becoming covariant in Const.

@kailuowang thank you I'm very excited about this change!

I assume that kittens will have to be updated in a similar way (especially since Unapply has been removed from Cats)? I'd be really interested in seeing how much simpler the heterogeneous HList sequencing becomes without the need for Unapply. It might be nice to give that a spin against this version of Cats before merging this PR just to make sure that everything still works smoothly. I won't have time today, but I could give that a try tomorrow or this weekend if nobody beats me to it.

I think that it's outside of the scope of this PR, but I wonder if it would be helpful to have a FAQ item for "Why doesn't the code example in the Cats docs compile for me?" which could have an answer that links to both the kind-projector and si-2712 items.

@ceedubs
Copy link
Contributor

ceedubs commented Apr 13, 2017

@kailuowang sorry but it looks like there are merge conflicts :(

@kailuowang
Copy link
Contributor Author

Resolved the conflicts and squashed.
I also brought back the Unapply class. It has ueses other than overcoming SI-2712. Given the experience in kittens, I believe there is good chance that some users are using it to help resolve implicit search.
I also tried kittens with this branch of code without using Unapply though. It worked out fine. Here is the PR

@ceedubs
Copy link
Contributor

ceedubs commented Apr 15, 2017

I also brought back the Unapply class. It has ueses other than overcoming SI-2712. Given the experience in kittens, I believe there is good chance that some users are using it to help resolve implicit search.

@kailuowang I don't see any Unapply references in the current kittens code. Is there something in particular that you are referring to? I was pretty happy about the prospect of dropping Unapply from Cats :)

@kailuowang
Copy link
Contributor Author

kailuowang commented Apr 15, 2017

@ceedubs, sorry I should've been more specific, it was late when I wrote the last update. Unapply is used in a situation where you are given specific type F, but you wanna try find out if this is actually a G[T] and there is an instance of H[G[_]]. You can achieve this using a different trick like here in kittens, but I suspect it will take user some time to discover that. So I am not sure if we should simply remove it. Maybe we can deprecate it instead?

@ceedubs
Copy link
Contributor

ceedubs commented Apr 15, 2017

@kailuowang hmm, I wonder if there are many places where code outside of Cats is using the Cats Unapply. I'm a bit inclined to think that after the SI-2712 fix this isn't really something that belongs in Cats but could potentially live elsewhere if libraries really still need it. But I'm definitely open to other viewpoints. Does anyone else want to weigh in on this?

Aside from the question of whether there should still be an Unapply in Cats, 👍 on this PR.

@@ -106,7 +106,7 @@ nested.map(_ + 1)
```

The `Nested` approach, being a distinct type from its constituents, will resolve the usual way modulo
possible [SI-2712][si2712] issues, but requires syntactic and runtime overhead from wrapping and
possible [SI-F2712][si2712] issues, but requires syntactic and runtime overhead from wrapping and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this change intentional? SI-F2712 doesn't mean anything to me.

val a: Either[String, Int]= Right(42)
val b: FreeT[Option, Either[String, ?], Int] = FreeT.liftTU(a)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep a test like this but make sure that it works with the normal FreeT.liftT?

implicit val eq5 = EitherT.catsDataEqForEitherT[OptionT[SEither, ?], String, Int]
implicit val eq6 = OptionT.catsDataEqForOptionT[SEither, (Int, Int, Int)]

implicit val iso = CartesianTests.Isomorphisms.invariant[OptionT[SEither, ?]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love to see all of this go away! 😻

@ceedubs
Copy link
Contributor

ceedubs commented Apr 15, 2017

Oops I'm bad at the GitHub review system. I don't think I submitted #1583 (comment) until just now.

@johnynek
Copy link
Contributor

So, the side effect of this change is that Const[Int, Int] is going to be usable where a Const[Int, Any] is needed. This seems a bit weird to me, since the type tag has now become an upper bound tag, not the exact value.

I imagine this will break some use cases.

It is certainly the case that in principle a phantom type can be set to be covariant, but the meaning of the type is considerably changed, isn't it?

@kailuowang
Copy link
Contributor Author

@ceedubs @johnynek I reverted the change of making Const covariant on the phantom type and created a separate issue #1608. I think that is a separate issue from removing Unapply machinery.
As for the Unapply class, I also created a new ticket #1607. I am all for moving it out of cats, but creating a new repo and release take some time and this PR touches a lot of files and is blocking other PRs. So maybe we can address it in a separate PR?

@ceedubs
Copy link
Contributor

ceedubs commented Apr 17, 2017

It's been a while and there have been some changes since @adelbertc's review, so I'm going to wait for another review or two before merging.

@@ -6,6 +6,7 @@ import cats.functor.Contravariant
/**
* [[Const]] is a phantom type, it does not contain a value of its second type parameter `B`
* [[Const]] can be seen as a type level version of `Function.const[A, B]: A => B => A`
* B is set covariant to help type inference.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You meant invariant?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I saw a discussion below, I think you forgot to make B covariant in Const definition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we reverted that in this PR and created a new Issue #1608 for it, but I forgot to remove this sentence. Thanks for spotting it!

Copy link
Contributor

@edmundnoble edmundnoble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@@ -47,13 +49,22 @@ There are a few minor mismatches between `Xor` and `Either`. For example, in som

Similarly, `cats.data.XorT` has been replaced with `cats.data.EitherT`, although since this is a type defined in Cats, you don't need to import syntax or instances for it (although you may need imports for the underlying monad).

## <a id="si-2712" href="#si-2712"></a>Why is the compiler having trouble with types with more than one type parameters?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/parameters/parameter

@ceedubs
Copy link
Contributor

ceedubs commented Apr 20, 2017

👍 LGTM.

@johnynek would you want to take another look after the most recent changes?

@johnynek
Copy link
Contributor

👍

@kailuowang kailuowang merged commit 01711b0 into typelevel:master Apr 21, 2017
@kailuowang kailuowang deleted the un-unapply branch April 21, 2017 01:52
@kailuowang kailuowang modified the milestone: 1.0.0-MF Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure out how to handle SI-2712 fix
9 participants