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

Add CanEqual typeclass instance for Seq to match Nil case #13265

Merged
merged 1 commit into from
Aug 23, 2021

Conversation

kevin-lee
Copy link
Contributor

Add CanEqual instance for Seq to match Nil case.

This PR solves the similar issue as what #12419 solves, but it's for pattern matching on List.
With -language:strictEquality compiler option or import scala.language.strictEquality, the following case doesn't compile

List(1, 2, 3, 4, 5) match {
  case n :: ns =>
    println(s"head: $n, tail: ${ns.mkString("[", ",", "]")}")
  case Nil =>
    println("empty")
}

with this compile-time error

    case Nil =>
         ^^^
Values of types object scala.collection.immutable.Nil and List[Int] cannot be compared with == or !=.
I found:

    CanEqual.canEqualSeq[Nothing, Int](/* missing */summon[CanEqual[Nothing, Int]])

But no implicit values were found that match type CanEqual[Nothing, Int].

This PR solves it.

@kevin-lee kevin-lee marked this pull request as ready for review August 7, 2021 16:04
@nicolasstucki nicolasstucki added the needs-minor-release This PR cannot be merged until the next minor release label Aug 9, 2021
@kevin-lee kevin-lee changed the title Add CanEqual instance for Seq to match Nil case Add CanEqual typeclass instance for Seq to match Nil case Aug 9, 2021
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM. Would be nice to tweak the comments, but this shouldn't slip to 3.2.0 on that basis!

library/src/scala/CanEqual.scala Outdated Show resolved Hide resolved
library/src/scala/CanEqual.scala Outdated Show resolved Hide resolved
@dwijnand dwijnand added this to the 3.1.0 milestone Aug 19, 2021
@kevin-lee kevin-lee force-pushed the add-CanEqual-for-Seq-to-match-Nil branch from 4931e71 to ede2820 Compare August 19, 2021 09:17
@kevin-lee
Copy link
Contributor Author

@dwijnand I've rebased the upstream mater and updated the commit as suggested. Thank you!

Comment on lines -32 to +33
given canEqualSeq[T, U](using eq: CanEqual[T, U]): CanEqual[Seq[T], Seq[U]] = derived
given canEqualSeqs[T, U](using eq: CanEqual[T, U]): CanEqual[Seq[T], Seq[U]] = derived
given canEqualSeq[T](using eq: CanEqual[T, T]): CanEqual[Seq[T], Seq[T]] = derived // for `case Nil` in pattern matching
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why the original given isn't enough to match Nil /cc @odersky

Copy link
Member

Choose a reason for hiding this comment

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

It's the same logic as the Option/Either PR, you get:

    case Nil =>
         ^^^
Values of types object scala.collection.immutable.Nil and List[Int] cannot be compared with == or !=.
I found:

    CanEqual.canEqualSeq[Nothing, Int](/* missing */summon[CanEqual[Nothing, Int]])

But no implicit values were found that match type CanEqual[Nothing, Int].

@nicolasstucki
Copy link
Contributor

@kevin-lee could you rebase this PR on master? We aim merge it for 3.1

@kevin-lee
Copy link
Contributor Author

@nicolasstucki Sure, I will do it soon.

@kevin-lee kevin-lee force-pushed the add-CanEqual-for-Seq-to-match-Nil branch from ede2820 to 665df86 Compare August 20, 2021 07:30
@kevin-lee
Copy link
Contributor Author

@nicolasstucki Done!

@kevin-lee kevin-lee force-pushed the add-CanEqual-for-Seq-to-match-Nil branch from 665df86 to 32a99fd Compare August 21, 2021 03:36
@kevin-lee
Copy link
Contributor Author

and done again.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

It would be good to add a comment why the new method is needed to the code. It is explained in the comments to the PR.

@odersky odersky removed their assignment Aug 23, 2021
@dwijnand dwijnand merged commit eaf321b into scala:master Aug 23, 2021
@kevin-lee kevin-lee deleted the add-CanEqual-for-Seq-to-match-Nil branch August 23, 2021 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants