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.

Sign up for GitHub

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

Jump to bottom

Derive Eq, PartialOrder, Order and Hash #336

Merged
merged 6 commits into from May 18, 2021
Merged

Derive Eq, PartialOrder, Order and Hash #336

merged 6 commits into from May 18, 2021

Conversation

TimWSpence
Copy link
Member

No description provided.

@TimWSpence
Copy link
Member Author

@joroKr21 this looks vaguely correct at least! Thought it would be good to get the structure in place at least

joroKr21
joroKr21 reviewed May 17, 2021
View reviewed changes
Copy link
Member

@joroKr21 joroKr21 left a comment •
edited

Choose a reason for hiding this comment

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

I think we should avoid allocations in ProductHash.
Regarding PartialOrder - not sure, but I lean towards keeping the 2.0 semantics.

core/src/main/scala-3.0.0-RC3/cats/derived/hash.scala Outdated
Comment on lines 14 to 16
val (hash, len) = inst.foldLeft[(Int, Int)](x)((MurmurHash3.productSeed, 0))(
[t] => (acc: (Int, Int), h: T[t], t0: t) => Continue((MurmurHash3.mix(acc._1, h.hash(t0)), acc._2 + 1))
)
Copy link
Member

Choose a reason for hiding this comment

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

I would rather do two iterations without allocating tuples

Copy link
Member

Choose a reason for hiding this comment

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

Oh, even better: restrict A <: Product and use productArity directly

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good call! 👍

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I gather that A <: Product doesn't quite work so we need A <:< Product.
I wonder if we can avoid the type casts somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't want to guarantee that we can't 😂 But I couldn't see how to get derived to work, where we need A <: Product only if Mirror[A] is a Product and not a Sum

core/src/main/scala-3.0.0-RC3/cats/derived/partialOrder.scala Outdated
extends PartialOrder[A]:

def partialCompare(x: A, y: A): Double =
//TODO is this correct? I _think_ it must be to be consistent with Order
Copy link
Member

Choose a reason for hiding this comment

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

Hmm in Kittens 2 we have the PartialOrder where two different subclasses are considered not comparable. I never thought about type class coherence in this case. But OTH it's way more intuitive to me that we can't compare them 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I guess coherence only really demands that partialCompare(x,y) != Nan ==> compare(x,y).toDouble == partialCompare(x,y). Or at least I would hope that that is the case.

However I can't think of a particularly good reason why we shouldn't compare ordinals when that is considered valid for deriving Order instances? Happy to trust your judgement on it though!

Copy link
Member

Choose a reason for hiding this comment

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

To me it's the other way around - why should we assume that we can compare two different branches of an ADT? We can make it work for Order via ordinal but it's kinda arbitrary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, fair enough 👍 Gives another reason to derive PartialOrder as well - if you want that behaviour instead of the arbitrary comparison of ordinals

joroKr21
joroKr21 approved these changes May 18, 2021
View reviewed changes
Copy link
Member

@joroKr21 joroKr21 left a comment

Choose a reason for hiding this comment

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

Awesome 👏

@TimWSpence
Copy link
Member Author

AFAICT Github Actions just exploded ("internal errors") rather than it being a genuine build failure? But I might have missed something

@joroKr21
Copy link
Member

Yes it has problems right now: https://www.githubstatus.com/

@joroKr21 joroKr21 merged commit cd6baa2 into typelevel:dotty May 18, 2021
@TimWSpence TimWSpence deleted the eq-hierarchy-derivation branch May 19, 2021 07:45
joroKr21 added a commit that referenced this pull request Sep 12, 2022
@joroKr21 @TimWSpence
@bplommer @andrzejressel @scala-steward @larsrh @armanbilge
Scala 3 support (#514)
014ed4a 
* Setup build for Scala 3

* Hello Scala 3 + Shapeless 3

* For now use test instead of validate

* Use the correct source directory

* Fix version specific source directories (#331)

* Derive Functor, Foldable, Traverse (#330)

* Derive Eq, PartialOrder, Order and Hash (#336)

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Derive Commutative{Semigroup, Monoid} (#341)

* Derive Show with field labels (#340)

* Derive Show with field labels

* Make show safe for empty products

* Use SAM functions to avoid anonymous class generation

* Derive MonoidK hierarchy (#342)

* Scala 3.0, Shapeless 3.0 and other dependencies (#343)

* Cleanup Const - it's provided by shapeless (#345)

Also use the constant type directly for brevity

* Derive Contravariant and Invariant (#346)

* Reducible derivation (#347)

* Deep and Coproduct Empty derivation (#349)

* Use summonFrom to prioritize DerivedEmpty instances (#355)

* Further simplify Empty derivation (#356)

* Generic Derived opaque type for given priority (#359)

* semiauto and auto imports (#353)

* Update sbt and shapeless, use project to derive Show for products (#363)

* Port functor tests (#362)

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Port FoldableSuite to Scala 3 (#374)

* Port ReducibleSuite to Scala 3 (#379)

* Port TraverseSuite to Scala 3 (#377)

* [WIP] Port TraverseSuite to Scala 3

* Update shapeless and fix TraverseSuite

* Port EmptySuite and also solve Empty for Coproducts (#387)

* Port tests for Semigroup and Monoid (#389)

* Rename files according to classes

* Derive Semigroup and Monoid

* Port tests for Semigroup and Monoid

* Port Eq and Hash plus tests (#393)

* Port NonEmptyTraverse derivation to Scala 3 (#394)

* Make derived methods final override (#395)

* Bump Scala and Cats and comment out tests that don't work (#427)

* Bump Scala and comment out tests that don't work

* Update cats

* Bump Scala 2 versions and Kind projector

* Port Show with tests (#433)

* Implement Eq derivation for Dotty

* Restore serializable

* Port Apply to Dotty (#441)

* Port Apply to Dotty

* Apply -> Or

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Make testApply inline

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Port Order to Dotty (#440)

* Port Order to Dotty

* MR comments fixes

* Port Applicative to Dotty (#439)

* Port Applicative to dotty

* Code cleanup

* Comment out Tuple

* Use Apply for deriving Applicative

* Composite Applicative can also be derived

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Merge master to dotty branch (#446)

* Update shapeless to 2.3.6 (#328)

* Update sbt to 1.5.2 (#329)

* Update kind-projector to 0.12.0

* Update sbt-mima-plugin to 0.9.1 (#333)

* Update discipline-munit to 1.0.9 (#335)

* Update alleycats-core, cats-core, ... to 2.6.1 (#334)

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Update kind-projector to 0.13.0 (#337)

* Update shapeless to 2.3.7 (#338)

* Update scala-library, scala-reflect to 2.13.6 (#339)

* Add VSCode .ignore files (#344)

* Update sbt-github-actions to 0.11.0 (#348)

* Update sbt-mima-plugin to 0.9.2 (#350)

* Update scala-library, scala-reflect to 2.12.14 (#351)

* Update sbt to 1.5.3 (#352)

* Update sbt-github-actions to 0.12.0

* Regenerate workflow with sbt-github-actions

* Update sbt-scalajs, scalajs-library, ... to 1.6.0

* Update sbt to 1.5.4 (#365)

* Update sbt-scala-native-crossproject, ... to 1.1.0 (#366)

* Update sbt-scalafmt to 2.4.3 (#367)

* Update sbt to 1.5.5 (#368)

* Update sbt-scalajs, scalajs-compiler, ... to 1.7.0 (#369)

* Update sbt-github-actions to 0.13.0 (#370)

* Update scalafmt-core to 3.0.0 (#371)

* Update scalafmt-core to 3.0.0

* Reformat with scalafmt 3.0.0

* Update sbt-mima-plugin to 1.0.0 (#372)

* Update kind-projector to 0.13.1 (#373)

* Update scalafmt-core to 3.0.1 (#378)

* Update kind-projector to 0.13.2 (#381)

* Update scalafmt-core to 3.0.2 (#380)

* Update scalafmt-core to 3.0.3 (#382)

* Update scalafmt-core to 3.0.4 (#384)

* Update scala-library, scala-reflect to 2.12.15 (#383)

* Update sbt-ci-release to 1.5.9 (#385)

* Update scalafmt-core to 3.0.5 (#386)

* Remove unnecessary instance (#388)

It was merged in cats-core.

* Update scalafmt-core to 3.0.6 (#390)

* Update sbt-mima-plugin to 1.0.1 (#391)

* Update sbt-scalajs, scalajs-compiler, ... to 1.7.1 (#392)

* Update sbt-ci-release to 1.5.10 (#396)

* Update scalafmt-core to 3.0.7 (#397)

* Update auxlib, javalib, nativelib, nscplugin, ... to 0.4.1 (#398)

* Update scalafmt-core to 3.0.8 (#399)

* Update scala-library, scala-reflect to 2.13.7 (#400)

* Update scalafmt-core to 3.1.1 (#403)

* Update scalafmt-core to 3.1.1

* Specify runner dialect explicitly

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Update sbt-scalafmt to 2.4.4 (#404)

* Update scalafmt-core to 3.1.2 (#405)

* Update alleycats-core, cats-core, ... to 2.7.0 (#406)

* Update scalafmt-core to 3.2.0 (#407)

* Update sbt-github-actions to 0.14.2 (#409)

* Update sbt-github-actions to 0.14.2

* Set Java version to Temurin 8

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Update scalafmt-core to 3.2.1 (#410)

* Update sbt-scalafmt to 2.4.5 (#411)

* Update auxlib, javalib, nativelib, nscplugin, ... to 0.4.2 (#412)

* Update sbt to 1.5.6 (#413)

* Update sbt-scalajs, scalajs-compiler, ... to 1.8.0 (#414)

* Update sbt to 1.5.7 (#415)

* Update sbt to 1.5.8 (#416)

* Update scalafmt-core to 3.2.2 (#417)

* Update scalafmt-core to 3.3.0 (#419)

* Update sbt-scalafmt to 2.4.6 (#418)

* Update sbt to 1.6.0 (#420)

* Update sbt to 1.6.1 (#421)

* Update scalafmt-core to 3.3.1 (#424)

* Update scala-library, scala-reflect to 2.13.8 (#426)

* Update scalafmt-core to 3.3.2 (#428)

* Update auxlib, javalib, nativelib, nscplugin, ... to 0.4.3 (#430)

* Update scalafmt-core to 3.3.3 (#431)

* Update scalafmt-core to 3.4.0 (#432)

* Update sbt to 1.6.2 (#434)

* Migrate to sbt-typelevel-ci-release (#429)

* Migrate to sbt-typelevel-ci-release

* Rescope publish settings

* Set tlVersionIntroduced

* Poke ci

* Update to latest sbt-typelevel

* Update scalafmt-core to 3.4.2 (#436)

* Use predefined Apache 2 license (#437)

* Enable snapshots on master (#438)

* Delete .sbtrc

* Publish snapshots from master

* Update sbt-typelevel-ci-release to 0.4.4 (#442)

* Update sbt-typelevel-ci-release to 0.4.4

* Regenerate workflow with sbt-github-actions

* Update sbt-typelevel-ci-release to 0.4.5 (#443)

* Update scalafmt-core to 3.4.3 (#444)

* Update shapeless to 2.3.8 (#445)

* Reformat

* Release from Dotty branch

Co-authored-by: Arman Bilge <armanbilge@gmail.com>

* Update Scala.js to 1.9.0

* Regenerate workflow

* Specify Munit version explicitly

* Don't override scalacOptions

* Check sbt formatting in root project

Co-authored-by: Arman Bilge <armanbilge@gmail.com>

* githubWorkflowGenerate

Co-authored-by: Scala Steward <43047562+scala-steward@users.noreply.github.com>
Co-authored-by: Scala Steward <me@scala-steward.org>
Co-authored-by: Lars Hupel <lars.hupel@mytum.de>
Co-authored-by: Arman Bilge <armanbilge@gmail.com>

* Update to Scala 3.1.2-RC3 and fix outstanding issues (#455)

* Scala 3.1.2 (#464)

* Scala 3.1.2

* Update sbt to 1.7.0-M1 and set scalaOutputVersion

* Port EmptyK to dotty (#448)

* Port EmptyK to dotty

* Fix formatting

* Support EmptyK for sum types

* Refactor EmptyKSuite

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Fix ApplicativeSuite (#465)

* Shapeless 3.1.0 (#468)

Also revert scalaOutputVersion - it's future is unclear

* Derive Pure for Scala 3 (#470)

* Add missing implicit not found messages (#471)

Unfortunately the experiece is subpar, because it doesn't work well
with type aliases which don't support type argument interpolation.

* Port Semigroupk/Monoidk to new scheme (#472)

* Port SemigroupK and MonoidK to new derivation scheme

* WIP port of SemigroupK suite to scala 3

* Nested derivations for SemigroupK

* Priority for derived SemigroupK given instances

* Port scala 2 MonoidK tests to scala 3

* Various improvements

- ImplicitNotFound error for SemigroupK/MonoidK
- Replace given priority via traits with NotGiven

* Use inline in tests

* SemigroupK/MonoidK test for derives syntax

* Port contravariant to new scheme (#475)

* Port SemigroupK and MonoidK to new derivation scheme

* WIP port of SemigroupK suite to scala 3

* Nested derivations for SemigroupK

* Priority for derived SemigroupK given instances

* Port scala 2 MonoidK tests to scala 3

* Various improvements

- ImplicitNotFound error for SemigroupK/MonoidK
- Replace given priority via traits with NotGiven

* Use inline in tests

* SemigroupK/MonoidK test for derives syntax

* Port Contravariant to new derivation scheme

* WIP port scala 2 Contravariant tests to scala 3

* Link commented out tests to issues

* derives syntax tests for Contravariant

* derives syntax tests for Contravariant

* Scala 3 import syntax

* Port invariant to new scheme (#478)

* Port invariant derivation to new scheme

* Port scala 2 invariant tests to scala 3

* Consistent test naming

* WIP extra Invariant instances

* Tests for invariant derivation syntax

* Port partial order to new scheme (#480)

* Port invariant derivation to new scheme

* Port scala 2 invariant tests to scala 3

* Consistent test naming

* WIP extra Invariant instances

* Tests for invariant derivation syntax

* Rename partial order derivation

* Port partial order derivation to new scheme

* Rename to PartialOrderSuite

* Port scala 2 partial order suite to scala 3

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Require Or instances for givens (#486)

* Scala 3 all the tests (#485)

* Replace all usages of implicit with given for Scala 3

Also use braceless syntax in more places.

* Scala 3 all the tests

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Remove obsolete Instances (#487)

* Derive ShowPretty (#490)

* WIP derive ShowPretty

* Derive ShowPretty

* Derive ShowPretty WIP

* Fix show pretty derivations

* Fix ShowPretty tests

Define companion object as scala/scala3#15391
isn't released yet

* Nicer syntax

* Test scala 3 enums (#498)

* Test with scala 3 enums

* More enum tests

* Fix typo

* Add other branches to test enums

* Simplify recursive Gen logic

* Cross-build for Native (#501)

* Cross-build for Native

* Bump munit version

* Bump cats

* Update auto instance implicit error messages

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Upgrade to Scala 3.2 (#513)

* Upgrade to Scala 3.2

Contains a load of fixes that we need, particularly for Mirror synthesis

* Regenerate GH workflow

* Update the rest of dependencies

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Docs (#509)

* Scala 3 docs

* Typo

* Clarify type constructor field limitations

* Extra clarification on the combination of auto and semiauto

* Apply suggestions from code review

Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com>

* Fix and simplify ApplySuite

Co-authored-by: Tim Spence <timothywspence@gmail.com>
Co-authored-by: Ben Plommer <ben.plommer@gmail.com>
Co-authored-by: ​Andrzej Ressel <green.hope9220@fastmail.com>
Co-authored-by: Scala Steward <43047562+scala-steward@users.noreply.github.com>
Co-authored-by: Scala Steward <me@scala-steward.org>
Co-authored-by: Lars Hupel <lars.hupel@mytum.de>
Co-authored-by: Arman Bilge <armanbilge@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joroKr21 joroKr21 joroKr21 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@TimWSpence @joroKr21