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

Cats typeclass priority hierarchy #647

Merged
merged 4 commits into from
May 12, 2023
Merged

Cats typeclass priority hierarchy #647

merged 4 commits into from
May 12, 2023

Conversation

RustedBones
Copy link
Contributor

Respect cats typeclass hierarchy for implicit resolution as defined here

In practice, this was not creating any issue due to the usage of shapeless.LowPriority but intelliJ linter was showing conflicts in resolution.

This fixes the lint errors and with scala 3, we'll have to define implicit priority properly anyway.
Tests were not run on the desired instance

@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

Merging #647 (12eacf7) into main (686964e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #647   +/-   ##
=======================================
  Coverage   94.98%   94.98%           
=======================================
  Files          52       52           
  Lines        1793     1793           
  Branches      165      165           
=======================================
  Hits         1703     1703           
  Misses         90       90           

@RustedBones RustedBones changed the title Cats typeclass priority hierarch Cats typeclass priority hierarchy Nov 14, 2022

class ScopeTest extends FunSuite {

test("auto implicit will give most powerful abstraction") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how scala solves implicit conflicts.
In our case we could have only defined those as the other implicits methods can't be called explicitly due to the shapeless LowPriority

Comment on lines +24 to +26
// use shapeless.LowPriority so the
// provided cats type classes are always preferred
// triggers derivation as last resort
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we unfortunately have to keep shapeless.LowPriority to avoid conflicts with implicits provided in cats._

// triggers derivation as last resort
package object auto extends LowPriority0Implicits

trait LowPriority0Implicits extends LowPriority2Implicits {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how this worked before the fix. Wrong type class was picked up by compiler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the shapeless.LowPriority got our back and was giving one of the implicit depending on the resolution.

implicitly[Group[Numbers]]
implicitly[CommutativeGroup[Numbers]]
implicitly[Show[Numbers]]
val s: Show[Numbers] = implicitly
Copy link
Contributor

@shnapz shnapz May 10, 2023

Choose a reason for hiding this comment

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

is this a matter of style?
upd: Oh OK. I see it below

@RustedBones RustedBones merged commit 1deca14 into main May 12, 2023
@RustedBones RustedBones deleted the cats-cleanup branch May 12, 2023 07:27
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

Successfully merging this pull request may close these issues.

2 participants