-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add CanEqual typeclass instances of Option, Either and Tuple for strictEquality #12419
Add CanEqual typeclass instances of Option, Either and Tuple for strictEquality #12419
Conversation
230fa78
to
bd62ef1
Compare
|
||
// The next three typeclass instances can also go into the companion objects of classes Option and Either. | ||
// For now they are here in order not to have to touch the source code of these classes | ||
given canEqualOptions[T, U](using eq: CanEqual[T, U]): CanEqual[Option[T], Option[U]] = derived |
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.
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.
Indeed. This must be for 3.1.
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.
Thanks @nicolasstucki and @sjrd for your answers. I have a few questions. I'm so sorry to ask it when everyone is busy with finalizing Scala 3 for release and I really appreciate your answer.
-
Regarding the users having no choice once it's added, could you please think about who are affected by this?
They are the ones who actually needstrictEquality
and most likely need this typeclass instance. The ones don't usestrictEquality
are not affected at all. Besides, the users do still have contol ofCanEqual[T, U]
since it's not auto-magically give themCanEqual[T, U]
as well.Can there be another use case than this one? I think it would be hard to find any other use case of
CanEqual
forOption
but I know that I could be easily wrong so I'm happy to take any advice. -
If what I mentioned above is not so important, that's fine. Could you tell me about the rest please?
a. Can we have the otherCanEqual
forOption
to solvecannot be compared with == or !=
onNone
case in pattern matching?
b. Can we haveCanEqual
forEither
?
c. Can we haveCanEqual
forTuple
?
It will be so nice if at least the ones for Tuple
are accepted for 3.0
because it's so common use case.
Finally, could you please think about what would make strictEquality
usable and more useful. Whenever users use Option
, Either
and Tuple
, they need to import their own CanEqual
typeclass instances seprately and it would be so annoying.
As far as I know, wildcard import like import my.typeclasses._
doesn't work and it has to be done seprately like.
import my.typeclasses.{canEqualOptions, canEqualOption, canEqualEither, canEqualEmptyTuple, canEqualTuple}
Thank you!
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.
Oh, I just found that 3.0.0
has been tagged already. 😅
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.
Regarding,
As far as I know, wildcard import like
import my.typeclasses._
doesn't work and it has to be done seprately like.
I didn't know that I could do import my.typeclasses.given
for that. Sorry about lack of my Scala 3 knowledge. 😅
Oh I misunderstood about Scala Do you think I should create a separate PR for just |
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.
Otherwise LGTM for 3.1.
bd62ef1
to
18971cb
Compare
18971cb
to
77fc62f
Compare
Hi @kevin-lee, would you be able to maybe rebase and add MIMA exclusions in https://github.com/lampepfl/dotty/blob/master/project/MiMaFilters.scala? |
@bishabosha I don't think adding mima exclusion makes sense here, we can only merge this PR once we've bumped our base version to 3.1.0-RC1, and at that point mima will not error out on new definitions. |
Ah ok, thank you for explaining |
Whenever I need to rebase, please let me know. 🙂 |
I recently came across this case: val ints = List(1,2,3)
ints match {
case x::_ => println(x)
case (Nil: List[Int]) => println("it's empty")
} I would suggest adding a test case for this, as it does not compile on Scala 3.0.1 with Edit: Here's the error:
|
@bbarker Thanks Brando! I'm glad that you pointed it out. I've been trying to figure out what common @smarter Could you please tell me if I should add that case to this PR or create another one? |
Hi @kevin-lee - thanks for pointing out the repository. It did seem to address the toy example above. Unfortunately, the example that inspired it still had the same issue. So, I tried adding an additional layer of indirection ( val intOpts: List[Option[Int]] = List(Some(1), None, Some(3))
intOpts match {
case x::_ => println(x)
case Nil => println("it's empty")
}
val intIOs: List[MyIO[Any, Unit, Int]] = List(
MyIO.pure(1), MyIO.fail(()), MyIO.pure(3)
)
intIOs match {
case x::_ => println(x)
case Nil => println("it's empty")
} The error:
I guess the issue is that we don't have a given for my custom type, so that probably explains it. My only question: is this a circumstance where I should expect to implement the given myself, or is there a better way? Also, a related issue occurs with
|
@bbarker Based on what you said, I guess your final case class MyIO[F[_], A, B](...) derives CanEqual
// or
class MyIO[F[_], A, B](...) derives CanEqual or if the first param is not a higher-kinded type like final case class MyIO[F, A, B](...) derives CanEqual |
@bbarker Please have a look at https://scastie.scala-lang.org/Kevin-Lee/ThWpw5D9R1qVDeAX0zUq6Q |
I would say make a new one since this one is already approved and ready to go once the version is bumped while anything new would have to be properly reviewed first. |
Thanks @kevin-lee - this is pretty neat, I didn't know about
|
@smarter Ok, make sense. I will create a new one for that. Thank you! |
@bbarker Please try it with libraryDependencies += "io.kevinlee" %% "can-equal" % "0.1.0" and import canequal.all.given It looks fine. |
@bbarker Sorry, I think I accidentally removed |
@bbarker In your case having given canEqualMyIO[R, E, A]: CanEqual[MyIO[R, E, A], MyIO[R, E, A]] = CanEqual.derived inside the MyIO.pure(1) == MyIO.pure(1) is alway |
Ah, makes sense for sure. Now, I do wonder if it makes sense that we have to have |
@kevin-lee Would it make sense to add |
- In addition to CanEqual[Option[T], Option[U]], CanEqual[Option[T], Option[T]] is also added for None case in pattern matching. - CanEqual instances for Tuple are added to Tuple companion object since Tuple is Scala 3's new type unlike Option and Either.
77fc62f
to
3e963c0
Compare
Doesn't the need for these instances go away if we can summon the trivial |
I may be misunderstanding, but it seems like |
@dwijnand Let me try to find if there's a better way or any issue with |
@bbarker My initial solution was something similar to what you asked. It was like given optionCanEqual[A]: CanEqual[None.type, Option[A]] = CanEqual.derived However it's not commutative and it doesn't make sense if equality is not commutative. None == Some(1) // fine
Some(1) == None // compile-time error So I had to add two typeclass instances like given noneOptionCanEqual[A]: CanEqual[None.type, Option[A]] = CanEqual.derived
given optionNoneCanEqual[A]: CanEqual[Option[A], None.type] = CanEqual.derived and that's why I eventually came up with a simpler solution which was just given canEqualOption[T](using eq: CanEqual[T, T]): CanEqual[Option[T], Option[T]] = CanEqual.derived If your concern is about pattern matching on
If you really want to pattern match on e.g.) https://scastie.scala-lang.org/Kevin-Lee/1qcXHY8DR3Gy2FeCK9F7hQ |
First, sorry for the delayed response, and thank you for the detailed reply.
Interesting, I didn't realize it wasn't commutative. Naively I would think equality should be made commutative in a PL whenever possible, but I suppose the workaround is easy enough as you showed below:
Hmm, this doesn't seem to work: package chapter4
import zio.*
import canequal.all.given
object Chapter4Exs:
def failWithMessageCaught(string: String): Task[Nothing] =
failWithMessageOrig(string).sandbox.mapError { cause =>
//given CanEqual[None.type, Option[?]] = CanEqual.derived // TODO - see about adding this to Scala prelude
given canEqualOption[T](using eq: CanEqual[T, T]): CanEqual[Option[T], Option[T]] = CanEqual.derived
cause.dieOption match {
case Some(thr) => Cause.Fail(thr)
case None => cause
}
}.unsandbox This results in:
So far, I've not thought of any single-given solutions to this issue that don't lose type safety, so maybe going back to the pair of symmetric givens above is the better approach?
This is a very good point - my rustiness with Scala is showing; I'll try to remember to use |
@dwijnand I've done some testing and it doesn't work. import scala.language.strictEquality
given nothingCanEqual1[A]: CanEqual[A, Nothing] = CanEqual.derived
given nothingCanEqual2[A]: CanEqual[Nothing, A] = CanEqual.derived
val optionalValue: Option[String] = Some("A")
optionalValue match {
case Some(value) => println(s"value: $value")
case None => println("None")
}
|
I'm sorry to create this PR without prior discussion but today is the 11th of May (in Sydney Australia) and the latest Scala 3 blog post says
I hope this PR is considered before the stable release and I thought that it would be good to discuss it here if there is any chance that it can be merged before that day.
Add CanEqual typeclass instances of Option, Either and Tuple for strictEquality
Additional details:
CanEqual[Option[T], Option[U]]
,CanEqual[Option[T], Option[T]]
is also added forNone
case in pattern matching.CanEqual
instances forTuple
are added toTuple
companion object since Tuple is Scala 3's new type unlikeOption
andEither
.Motivation and Reason
I recently started using Scala 3 for my personal projects and as I use more, I found
strictEquality
really useful and it makes my code much safer. In fact, I found this bug in Scala 3 I reported and fixed withstrictEquality
.However, as soon as I enable
strictEquality
, it is so inconvenient and difficult to use very common Scala types likeOption
,Either
andTuple
.For instance, the following code doesn't compile because
None and Option[Int] cannot be compared with == or !=
and there are more
Also please check out the tests I added in this PR.
Of course, we can create custom
CanEqual
typeclass instances forOption
,Either
andTuple
but then these types are from Scala so we can't put them in their companion objects meaning we have to import the custom ones whenever we want to check equality of those types. Checking their equality is so common case but it's so inconvenient to do without the typeclass instances. That's why I'm proposing these typeclass instance.What about Try?
I could not add
CanEqual
typeclass instance forTry
becauseTry
has no type parameter forFailure
case which containsThrowable
, so it is hard to make the following case fail in compile-time.