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

Handles Nothing when synthesizing CanEqual #13828

Merged
merged 1 commit into from
Oct 27, 2021

Conversation

deniszo
Copy link
Contributor

@deniszo deniszo commented Oct 26, 2021

fixes #13739
fixes #13512

@SethTisue
Copy link
Member

SethTisue commented Oct 26, 2021

@dwijnand this might interest you — I saw you piped up at e.g. #12419 (comment)

@SethTisue
Copy link
Member

on #13512, @jcracknell wrote:

With this addition it appears you can remove the supplementary CanEqual.canEqual{Seq,Option} previously added to address matching Nil/None in the above-mentioned commits.

not sure if this still applies? I can't look into it right this second, but I guess we should check on it before we merge this

@jcracknell
Copy link

jcracknell commented Oct 26, 2021

This introduces a "fix" for #13512 which effectively introduces CanEqual[Any, Nothing] (and vice versa), and I just want to make sure that it was determined that this is the desired behavior.

As best I can tell it should be perfectly safe, as the given only applies in the case where one of the arguments is known to be Nothing, and there is no way to evaluate such a comparison in any case. The discussion in #12419 expressed some trepidation, although that occurred at the intersection of several issues that had not quite been teased apart.

Edit - for further clarity:

As noted by @SethTisue elsewhere, the test case for the former that I had overlooked and is missing from this PR is summon[CanEqual[1, Int]], however that appears to work already:

https://scastie.scala-lang.org/BISPPkkYSaS8qstpCNbFOg

My biggest concern with the PR is that the implementation still never actually checks for a subtype relationship between the two types - only with java.lang.Number. While this appears to cover all the cases I can think of at the moment, I suspect a more principled approach might involve boxing both sides, checking for a relationship, and then checking if both derive from java.lang.Number as a fallback.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

LGTM, strictEquality is sort of experimental (I think it should be under scala.language.experimental really) so I have no qualm merging this :).

@smarter smarter merged commit 2a49471 into scala:master Oct 27, 2021
@Kordyjan Kordyjan added this to the 3.1.2 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants