-
-
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
Enumerable (WIP) #4349
base: main
Are you sure you want to change the base?
Enumerable (WIP) #4349
Conversation
cc @zarthross |
Should |
Should |
package cats | ||
package kernel | ||
|
||
private[kernel] object ScalaVersionSpecificLazyListCompat extends LazyListCompatBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, can we simply define a type alias Stream => LazyList for Scala 2.12,
while keeping a usual LazyList for Scala 2.13+ ?
type LazyList[a] => Stream[a]
val LazyList = Stream
or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably do that, but we'd be implying that all methods used would be identical for Stream
and LazyList
. I think they are right now...and we could add exceptions if needed in the future...
So...yes? I'll try it out.
I am not sure about bringing |
@satorg yeah, I admit it is a little strange. We need something which maps to the natural numbers here, and the integer numbers (as in from negative infinity to positive infinity) is such a set, In other words, we can't write a lawful instance of I'm open to other ideas here, I just can't think of anything. |
@armanbilge @satorg I've updated the first post on this issues with an unreasonably long discussion on where we can go here. Let me know your thoughts (I don't expect any responses in the immediately 😉 ) |
This pattern is generally used to avoid ambiguous implicits. This is the same reason that |
@armanbilge ah, that makes sense (and is unfortunate). I think my general proposal still stands, just with the branching class super types being members rather than extending them. Do you guys have thoughts on it? |
Actually, let me take another deeper look at this before we proceed. I'm not satisfied with the ergonomics of the solution I proposed, if we have to encode the super classes as members for each branching parent class. It may be an unsolvable problem without coherence built into the language, but I'd like to see if something else comes to mind. |
Prototyping adding the
Enumerable
class to cats. This class is based on https://hackage.haskell.org/package/base-4.15.0.0/docs/GHC-Enum.html#v:toEnum but tries to avoid some pitfalls that the Haskell version has (e.g.toEnum
doesn't throw in our version) and relates to the other classes currently defined inEnumerable.scala
.This isn't remotely complete, but I thought I'd start a PR so we can have a central place to discuss the work as it progresses.
Open Questions
CycleNext
class for example in Add Syntax For Enumerable #4347.cycleNext
orcyclePrevious
for types which haveNext
orPrevious
be defined as aliases fornext
andprevious
?Update, Current State And Proposed Changes
I've been hacking these classes for a bit now and I think I've got a more concrete proposal about how they should line up, as well as some questions I'd like to raise to a broader audience. There's a lot going on here, so I apologize in advance for the upcoming wall of text.
In the class hierarchy diagrams below, I'm only showing the abstract methods on the class.
Current Hierarchy
This is the current class hierarchy. There are a few things worth pointing out that are suspect.
PartialNext
,PartialPrevious
,UpperBounded
andLowerBounded
compose inPartialOrder
, rather than extending it. The situation is similar forBoundedEnumerable
andUnboundedEnumerable
with respect toOrder
.PartialNextLowerBounded
andPartialPreviousUpperBounded
have no abstract members.BoundedEnumerable
andUnboundedEnumerable
only haveOrder
as an abstract member.PartialNextLowerBounded
extendsPartialNext
,LowerBounded
, andPartialPrevious
PartialPreviousUpperBounded
extendsPartialPrevious
,UpperBounded
, andPartialNext
TL;DR The current hierarchy encourages the use of incoherent instances for some behaviors, inconsistently uses certain patterns to defend against invalid behavior in the presence of incoherent instances, and has some naming issues.
Something to note is that many of these classes have a
def reverse
function which reverses the ordering, which we will dicuss a bit below.Composing PartialOrder and Order
I don't know why we are composing in
PartialOrder
andOrder
as an abstract member on these classes, rather than extending it. As a side effect of thereverse
function, composing the class as a member rather than extending it means that a function like this might lead to incorrect behaviors that are difficult to detect.There are two possible ways to make this better that come to mind.
PartialNext
extendsPartialOrder
(yes I know this isn't bincompat, we'll come back to that)reverse
in favor of something like Inverse.We could do either or both of these. They are not mutually exclusive.
PartialNextLowerBounded and PartialPreviousUpperBounded
With respect to
PartialNextLowerBounded
andPartialPreviousUpperBounded
not having any abstract members, this seems very odd to me. I expect a typeclass to introduce some abstract behavior that is then used to provide some new functionality, rather than just composing multiple already defined classes. These classes do have a non-abstract members, for exampledef membersDescending: Stream[A]
. The reason for this encoding seems to be becausemembersDescending
needs to access theUpperBounded.maxBound
. One could definemembersDescending
onPartialPrevious
like so,This is consistent with how we define methods on other classes, for example
isEmpty
onMonoid
demands anEq[A]
constraint. For example, we don't introduce a class liketrait EqMonoid[A] extends Monoid[A] with Eq[A]
just to define the methodisEmpty
.It seems the reason we don't define
membersDescending
with an implicit constraint is so that in the event of areverse
we can ensure we are using the correct constraint. For example, ifmembersDescending
was defined with a constraint and someone reverses aBoundedEnumerable
then they would likely get the wrongUpperBounded
when calling themembersDescending
on the reversedBoundedEnumerable
. Although, it may not have been as intentional as that. The PR which added them doesn't seem to discuss any of this..An alternative way we could resolve this is by discouraging the use of incoherent instances and encouraging the use of newtypes such as Inverse.
Also, as a small aside, the names seem wrong. These classes extend from both
PartialNext
andPartialPrevious
, but they are named as though they only extend from one.Proposed New Hierarchy
Here is my ideal proposed new hierarchy.
Major differences
PartialOrder
andOrder
as members on the classes, we extend them.Enumerable
class is added betweenPartialNext
/PartialPrevious
andUnboundedEnumerable
/BoundableEnumerable
.Although I elided them in this graphic, there are a great many useful concrete methods one can define on these classes. Some require related constraints, for example
PartialNext
could have this method.Bincompat
This would create bincompat issues. So it would either need to wait for cats 3.x.x or we'd have find some alternative names here and introduce a new hierarchy. I'm personally a slight fan latter, as I like to avoid bincompat breaks as long as possible.
Here are some ideas. Though, FWIW, I don't like them as much as the names we are currently using.
PartialNext
->PartialForward
PartialPrevious
->PartialBackward
Next
->Forward
Previous
->Backward
BoundedEnumerable
->BoundableEnumerable
UnboundedEnumerable
->UnboundableEnumerable
LowerBounded
->MinBounded
UpperBounded
->MaxBounded
Enumerable
is a net new class, so that can stay the same. Some classes in the current hierarchy would just be deprecated without a new class as their methods can be rolled into the other classes with constraints.Alternative Updated Hierarchy
As an alternative, we could continue the current patterns. Have
PartialOrder
andOrder
be members on classes, and introduce new classes with no abstract methods in order to define utility functions likenextOrMinBy
andmembersDescending
without using constraints.Here is what that might look like with the addition of the
Enumerable
class.In my opinion, that's quite messy.
The ergonomics of using this hierarchy are pretty bad as well. For example, if I need to write a function which uses
nextOrMinBy
andfromEnum
, in this hierarchy defined onPartialNextLowerBounded
andEnumberable
respectively, and also a comparison, then I need this signature.In this case,
PartialNextLowerBounded
provides aPartialOrder
as a member (defined in bothLowerBounded
andPartialNext
.Enumerable
provides it's own, possibly completely different,Order
as a member, and I also have the implicitOrder
in scope.Thoughts?