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 Hash typeclasses to cats #1690

Closed
ctongfei opened this issue May 19, 2017 · 12 comments
Closed

Add Hash typeclasses to cats #1690

ctongfei opened this issue May 19, 2017 · 12 comments

Comments

@ctongfei
Copy link
Contributor

Hash is a special case of Eq.
These captures the Java default method hashCode and clone (in Cloneable), just like Eq for equals and Show for toString.

Example implementation:
https://github.com/ctongfei/poly-collection/blob/master/core/src/main/scala/poly/collection/typeclass/Hash.scala#L12
https://github.com/ctongfei/poly-collection/blob/master/core/src/main/scala/poly/collection/typeclass/Cloning.scala#L11

@johnynek
Copy link
Contributor

johnynek commented May 19, 2017 via email

@adelbertc
Copy link
Contributor

Related: typelevel/algebra#38

@ctongfei
Copy link
Contributor Author

OK. But Hash and/or HashFamily (for BloomFilter/CountMinSketch/etc.) should be added

@djspiewak
Copy link
Member

Ditto @johnynek. Hash would be quite useful. Clone not so much.

@kailuowang
Copy link
Contributor

Thanks for the suggestion @ctongfei . Also 👍 on adding Hash. A PR would be more than welcome.

@ctongfei
Copy link
Contributor Author

@kailuowang Thanks. I'll open a PR.

@ctongfei
Copy link
Contributor Author

ctongfei commented May 26, 2017

I propose a HashOrder[A] typeclass that extends both Hash[A] and Order[A].

Two reasons:

  • If we give an implicit instance of Hash[A] for the common As in cats.kernel.instances, every time when we require an Eq[A] we would be in a situation where implicit objects are ambiguous: There exists both a Hash[A] and an Order[A] instance. If all of those instances in cats.kernel.instances are instances of HashOrder this problem cease to exist.
  • This solves the diamond inheritance problem, especially on the method on[B](f: B => A): Hash[B]. In HashOrder we can define an overriding on that overrides both the Hash and the Order on methods.

@johnynek
Copy link
Contributor

this isn't uncommon in cats (see previous discussions #1210).

i think the best we have to offer is implicit ho: Hash[A] with Order[A] has been proposed, which means you don't have the ambiguity, or explicitly passing the Eq to break the ambiguity manually.

I personally don't like making the full cross product of possible compositions reified in types. So, I would be -1 on HashOrder. Lastly, I can't see any relationship between Hash and Order unlike Hash and Eq.

@ctongfei
Copy link
Contributor Author

@johnynek So for each typeclass instance we provide, we will override the on method manually. Is that the proposed solution?

@johnynek
Copy link
Contributor

Can you sketch the case you are worried about?

I see the issue with on returning either Hash or Order. It's not ideal. I guess you would just indeed override:

scala> trait Foo { def bar: Foo = this }
defined trait Foo

scala> trait Baz { def bar: Baz = this }
defined trait Baz

scala> new Foo with Bar { }
<console>:13: error: not found: type Bar
       new Foo with Bar { }
                    ^

scala> new Foo with Baz { }
<console>:14: error: <$anon: Foo with Baz> inherits conflicting members:
  method bar in trait Foo of type => Foo  and
  method bar in trait Baz of type => Baz
(Note: this can be resolved by declaring an override in <$anon: Foo with Baz>.)
       new Foo with Baz { }
           ^

scala> new Foo with Baz { override def bar: Foo with Baz = this }
res2: Foo with Baz{def bar: Foo with Baz} = $anon$1@8d7b252

I see your concern since basically everything has Hash and so that means everything with Order also will suffer if we do import cats.implicits._. It will basically render Eq unusable at that point, I guess.

We could use the alternate encoding for Hash:

trait Hash[A] {
  def eqInstance: Eq[A]
  def hash(a: A): Int
}

so we don't extend, but point to the instance. This would make Hash an odd duck in cats.

This keeps coming up.

@ctongfei
Copy link
Contributor Author

I would say the better route is do manual overriding on for every typeclass instance.
Or another option is not to implement the Hash#on method. Instead, use Hash$.by[A, B](f: A=>B)(implicit ev: Hash[B]): Hash[A].

Hash extends Eq has good intuitions: every map requires that there is an Eq on the type of keys: ListMap only requires Eq, TreeMap requires Order, whereas HashMap requires Hash. Very neat.

@ctongfei ctongfei changed the title Add Hash/Cloning typeclasses to cats Add Hash typeclasses to cats May 29, 2017
@ctongfei
Copy link
Contributor Author

I drafted a pull request #1712 .

LukaJCB pushed a commit to LukaJCB/cats that referenced this issue Sep 8, 2017
LukaJCB pushed a commit that referenced this issue Oct 4, 2017
* cats.kernel.Hash and related instances (#1690)

* Hash laws

* all test passed

* Hash instances for tuples

* introduce implicit precedence in KernelBoiler: Order > PartialOrder > Eq; Hash > Eq.

* Add type alias in cats._; Add contravariant instance for `Hash`

* HashFunctions extends EqFunctions

* Move contravariant instances to separate trait, in accordance with (#1659)

* revert name change

* move EitherInstances1#EitherEq out

* Optimize hash computation on case classes without allocating a new object

* fixing the problem in CI build: method catsKernelStdOrderForChar()cats.kernel.instances.CharOrder in trait cats.kernel.instances.CharInstances has a different result type in current version, where it is cats.kernel.Order rather than cats.kernel.instances.CharOrder

* Full compliance with how Scala generates hash codes on case classes; +SetHash

* Cartesian[Hash]

* ContravariantCartesian[Hash]

* ContravariantCartesian[Hash]; Hash.fromHashing

* style issues

* remove unused import

* some test cases

* some test cases

* +test for Contravariant[Hash]

* +test for Contravariant[Hash]

* Add NEL/NEV one (#1707)

`NonEmptyList.one` is analogous to `_.pure[NonEmptyList]`.

Alternative for `NonEmptyList.of(x)` where you don't pay the price of
the varargs, which isn't used.

* move instances into separate trait (#1659)

* move instances into separate trait

* adding separate objects for piecemeal imports

* Adding implicit resolution tests for piecemeal imports for hierarchy

* Removing explicit implicitly and using apply method

* cats.kernel.Hash and related instances (#1690)

* Hash laws

* all test passed

* Hash instances for tuples

* introduce implicit precedence in KernelBoiler: Order > PartialOrder > Eq; Hash > Eq.

* Add type alias in cats._; Add contravariant instance for `Hash`

* HashFunctions extends EqFunctions

* Move contravariant instances to separate trait, in accordance with (#1659)

* revert name change

* move EitherInstances1#EitherEq out

* Optimize hash computation on case classes without allocating a new object

* fixing the problem in CI build: method catsKernelStdOrderForChar()cats.kernel.instances.CharOrder in trait cats.kernel.instances.CharInstances has a different result type in current version, where it is cats.kernel.Order rather than cats.kernel.instances.CharOrder

* Full compliance with how Scala generates hash codes on case classes; +SetHash

* Cartesian[Hash]

* ContravariantCartesian[Hash]

* ContravariantCartesian[Hash]; Hash.fromHashing

* style issues

* remove unused import

* some test cases

* some test cases

* +test for Contravariant[Hash]

* +test for Contravariant[Hash]

* cats.kernel.Hash and related instances (#1690)

* Hash laws

* all test passed

* Hash instances for tuples

* introduce implicit precedence in KernelBoiler: Order > PartialOrder > Eq; Hash > Eq.

* Add type alias in cats._; Add contravariant instance for `Hash`

* HashFunctions extends EqFunctions

* Move contravariant instances to separate trait, in accordance with (#1659)

* revert name change

* move EitherInstances1#EitherEq out

* Optimize hash computation on case classes without allocating a new object

* fixing the problem in CI build: method catsKernelStdOrderForChar()cats.kernel.instances.CharOrder in trait cats.kernel.instances.CharInstances has a different result type in current version, where it is cats.kernel.Order rather than cats.kernel.instances.CharOrder

* Full compliance with how Scala generates hash codes on case classes; +SetHash

* Cartesian[Hash]

* ContravariantCartesian[Hash]

* ContravariantCartesian[Hash]; Hash.fromHashing

* style issues

* remove unused import

* some test cases

* some test cases

* +test for Contravariant[Hash]

* +test for Contravariant[Hash]

* Fix duplication error and style error

* fix merge error

* remove instance for Cartesian[Hash]: it does not satisfy associativity

* +identityHash, +`hash` postfix method

* all tests passed

* increase coverage

* accidentally removed plugin, restore it

* all tests passed

* increase coverage

* increase coverage, ## => hashCode, kernelBoiler

* suppress mimaReportBinaryIssues

* Remove cats.functor

* Remove cats.functor
@LukaJCB LukaJCB closed this as completed Nov 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants