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

De-prioritize intWitnessAs implicit on Scala 3 #1317

Merged
merged 3 commits into from
Dec 27, 2024
Merged

De-prioritize intWitnessAs implicit on Scala 3 #1317

merged 3 commits into from
Dec 27, 2024

Conversation

armanbilge
Copy link
Contributor

There are already several specialized instances e.g. intWitnessAsLong. These should be higher priority than this generic instance that relies on Numeric.

Annoyingly, starting in Scala 3.6 the implicit resolution changes cause the following warning. Fixing the prioritization should address the warning.

[warn]    |Given search preference for eu.timepit.refined.internal.WitnessAs[(0 : Int), Long] between alternatives
[warn]    |  (eu.timepit.refined.internal.WitnessAs.intWitnessAsLong :
[warn]    |  [A <: Int]: eu.timepit.refined.internal.WitnessAs[A, Long])
[warn]    |and
[warn]    |  (eu.timepit.refined.internal.WitnessAs.intWitnessAs :
[warn]    |  [B, A <: Int]
[warn]    |    (implicit wa: ValueOf[A], ta: eu.timepit.refined.internal.ToInt[A], nb:
[warn]    |      Numeric[B]): eu.timepit.refined.internal.WitnessAs[A, B]
[warn]    |)
[warn]    |will change.
[warn]    |Current choice           : the first alternative
[warn]    |New choice from Scala 3.7: the second alternative

Alternatively, perhaps this implicit was ported in error. The equivalent on the Scala 2 side is for Nat, which isn't available for Scala 3, so perhaps was swapped for Int?

refined/modules/core/shared/src/main/scala-3.0-/eu/timepit/refined/internal/WitnessAs.scala

Lines 31 to 36 in 20df7cc

implicit def natWitnessAs[B, A <: Nat](implicit
wa: Witness.Aux[A],
ta: ToInt[A],
nb: Numeric[B]
): WitnessAs[A, B] =
WitnessAs(wa.value, nb.fromInt(ta.apply()))

@armanbilge armanbilge mentioned this pull request Dec 19, 2024
Merged
@codecov Codecov
Copy link

codecov bot commented Dec 19, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.78%. Comparing base (20df7cc) to head (8e9de9f).
Report is 8 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1317   +/-   ##
=======================================
  Coverage   92.78%   92.78%           
=======================================
  Files          65       65           
  Lines         845      845           
  Branches       29       29           
=======================================
  Hits          784      784           
  Misses         61       61

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fthomas
Copy link
Owner

fthomas commented Dec 19, 2024 •
edited
Loading

Alternatively, perhaps this implicit was ported in error. The equivalent on the Scala 2 side is for Nat, which isn't available for Scala 3, so perhaps was swapped for Int?

This instance was once for Nat on Scala 3:

00dcf50#diff-5b011ae4abb8c08722548566381dedd082136bc9353874ece9567afc4a681423L31-R31

If there is no test that fails without it, I'm fine with making it not implicit and deprecating it.

@fthomas fthomas merged commit 95065ae into fthomas:master Dec 27, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@armanbilge @fthomas