Don't override neqv in Order #2230
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Eq provides a reqsonable default implementation of `neqv`: ```scala def neqv(x: A, y: A): Boolean = !eqv(x, y) ``` Before this change, `Order` overrides this default implementation as: ```scala compare(x, y) != 0 ``` I think that this override is questionable for a few reasons: - The negation of `eqv` seems like the most straightforward implementation of `neqv` to me - It seems like an oversight that `Order` overrides `neqv` but `PartialOrder` does not. - If someone is overriding `eqv` on an `Order` instance, presumably they are doing it because their `eqv` implelentation can be faster than doing a full `compare`. If this is the case and they don't override `neqv`, it would be better for them to inherit a !eqv` implementation. As a sidenote, the current implementation leads to a lack of symmetry between `===` and `=!=` in the presence of `null`. `"foo" === null` returns `false` while `"foo" =!= null` throws an NPE. I generally agree with the decision in Cats to not perform special-handling of `null`. However, the way that I ran into this was when adding a linting rule that forbids non type-safe equality (== and !=), and it is kind of a bummer that if you are working with a third-party library that might return `null` and you want to safeguard against that you can't just check for `x =!= null` instead of `x != null` to satisfy the linting rule. You can do something like `Option(x).isDefined`, but that introduces an extra allocation and seems a little roundabout to me.
|
👍 |
|
Oh this isn't binary compatible in Scala 2.10 and 2.11 :( |
|
To get around the binary compatibility issue, I guess that I could keep the |
ceedubs
added a commit
to ceedubs/cats
that referenced
this pull request
Apr 17, 2018
Based on the discussion in typelevel#2230.
|
Replaced by #2232 |
kailuowang
pushed a commit
that referenced
this pull request
Apr 17, 2018
Based on the discussion in #2230.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Eq provides a reasonable default implementation of
neqv:Before this change,
Orderoverrides this default implementation as:I think that this override is questionable for a few reasons:
eqvseems like the most straightforward implementation ofneqvto meOrderoverridesneqvbutPartialOrderdoes not.
eqvon anOrderinstance, presumably they aredoing it because their
eqvimplelentation can be faster than doing a fullcompare. If this is the case and they don't overrideneqv, it would bebetter for them to inherit a
!eqvimplementation.As a sidenote, the current implementation leads to a lack of symmetry
between
===and=!=in the presence ofnull."foo" === nullreturns
falsewhile"foo" =!= nullthrows an NPE. I generally agreewith the decision in Cats to not perform special-handling of
null.However, the way that I ran into this was when adding a linting rule
that forbids non type-safe equality (== and !=), and it is kind of a
bummer that if you are working with a third-party library that might
return
nulland you want to safeguard against that you can't justcheck for
x =!= nullinstead ofx != nullto satisfy the lintingrule. You can do something like
Option(x).isDefined, but thatintroduces an extra allocation and seems a little roundabout to me.