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

Explicit functor variance #1097

Merged
merged 1 commit into from
Jun 9, 2016
Merged

Conversation

edmundnoble
Copy link
Contributor

This uses casts to implement Functor.widen and Contravariant.narrow to take advantage of the natural variance of Functors with respect to subtyping. Functor.widen in Scalaz works similarly (though it uses Liskov and @uncheckedVariance in its implementation, it has the same underlying optimization).

@codecov-io
Copy link

codecov-io commented Jun 7, 2016

Current coverage is 88.57%

Merging #1097 into master will decrease coverage by 0.35%

  1. 2 files (not in diff) in .../main/scala/cats/std were modified. more
  2. 2 files (not in diff) in ...main/scala/cats/data were modified. more
    • Misses +1
    • Hits -1
  3. 3 files (not in diff) in .../src/main/scala/cats were modified. more
    • Misses +11
    • Hits +1
@@             master      #1097   diff @@
==========================================
  Files           226        226          
  Lines          2917       2931    +14   
  Methods        2867       2879    +12   
  Messages          0          0          
  Branches         48         49     +1   
==========================================
+ Hits           2594       2596     +2   
- Misses          323        335    +12   
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 88cbe95...4c5db8b

@edmundnoble
Copy link
Contributor Author

I think the only useful tests for widen and narrow are going to be fairly low-level. I'll add some for codecov.

@peterneyens
Copy link
Collaborator

You could probably add an sbt-doctest to solve the codecov difference ?

@julienrf
Copy link
Contributor

julienrf commented Jun 8, 2016

Can you point to a discussion about the pros and cons of this approach versus using a variance annotation?

@@ -18,6 +18,12 @@ import simulacrum.typeclass

// derived methods

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add another * so that this becomes a ScalaDoc comment?

@ceedubs
Copy link
Contributor

ceedubs commented Jun 8, 2016

@edmundnoble thanks! Looking good so far. I agree with @peterneyens that some doctest examples would be good and will help out the code coverage. Another thing that you could do is add some property-tests that widen is equivalent to map identity, but then I guess you have the question of whether you should add that for a few random functors, or add it to the laws.

@non
Copy link
Contributor

non commented Jun 8, 2016

I agree with @ceedubs specific comment syntax remarks, but other than that +1 from me. Tests and/or examples would be nice too if you felt like doing them.

@edmundnoble
Copy link
Contributor Author

@julienrf That's proving surprisingly hard to find. I can find a good argument against using a contravariance annotation on Contravariant: https://issues.scala-lang.org/browse/SI-2509.

@edmundnoble
Copy link
Contributor Author

I've made the comments into scaladoc comments, and added some doctest stuff and tests.

@ceedubs
Copy link
Contributor

ceedubs commented Jun 9, 2016

👍 thanks!

@julienrf I don't know of a discussion about this approach vs adding a variance annotation to point to. I can add some notes here though.

If you were to write an invariant InvList[A], your prepend method would look something like def ::(a: A): InvList[A] = ???. With a covariant list like the standard library's List[+A], this becomes a bit more complicated. A List[A] is a List of any supertype of A, so your method has to become something like def ::[B >: A](x: B): List[B]. This means that something that is likely an error like 1 :: List("b", "c") compiles (but returns List[Any]). There are reasons that you might want that behavior, but I think that it's also reasonable to not want that behavior.

The previous paragraph referred to putting the variance annotation on the data structure. If you were to put the variance annotation on the type class instances (like trait Functor[F[+_]]), then you would prohibit the creation of a Functor instance for any invariant structure (such as the NonEmptyList in cats). I consider that a deal-breaker for adding variance to type classes.

@non
Copy link
Contributor

non commented Jun 9, 2016

Looks good to me. 👍

@non non merged commit 8ec2caa into typelevel:master Jun 9, 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.

6 participants