Skip to content

De-prioritize intWitnessAs implicit on Scala 3#1317

Merged
fthomas merged 3 commits intofthomas:masterfrom
armanbilge:fix/witness-as-priority
Dec 27, 2024
Merged

De-prioritize intWitnessAs implicit on Scala 3#1317
fthomas merged 3 commits intofthomas:masterfrom
armanbilge:fix/witness-as-priority

Conversation

@armanbilge
Copy link
Contributor

@armanbilge armanbilge commented Dec 19, 2024

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
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
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