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

Greater[5] Or Equal[5] results in drastic performance problems with unsafeFrom #1161

Open
toddburnside opened this issue Mar 10, 2023 · 8 comments

Comments

@toddburnside
Copy link

toddburnside commented Mar 10, 2023

Running the following takes more than a minute to complete for me:

type Crazy = Int Refined (Greater[5] Or Equal[5])
object Crazy extends RefinedTypeOps.Numeric[Crazy, Int]
val crazy = Crazy.unsafeFrom(5)

However, the following is fine:

val sane: Crazy = refineV[Greater[5] Or Equal[5]](5).toOption.get

Here is a scastie demonstrating the problem:
https://scastie.scala-lang.org/oGPpPS59RcG8ejX9dSDhhQ
It just gets closed, but when run locally it will eventually return.

I have tried refined versions 0.9.29 through 0.10.2 and scala versions 2.13.8 and 3.2.2

@fthomas
Copy link
Owner

fthomas commented Mar 10, 2023

I suspect that calculating MinValue and MaxValue of Crazy is the culprit.

@toddburnside
Copy link
Author

That was quick!
I should have mentioned in the comment, Equal[5] Or Greater[5] is fine.

@steinybot
Copy link
Contributor

It seems @fthomas suspicions are correct. This does it:

object Main extends App {
  type Crazy = Int Refined (Greater[5] Or Equal[5])
  val min: Min[Crazy] = implicitly[Min[Crazy]]
}

@steinybot
Copy link
Contributor

Oh Or is going to use Min.validateMin which will start at Int.MinValue and step up one at a time until it finds a valid value. No wonder.

Would something like this work?

  implicit def orMin[F[_, _], T, L, R](implicit
      rt: RefType[F],
      ml: Min[F[T, L]],
      mr: Min[F[T, R]],
      at: Adjacent[T],
      v: Validate[T, L Or R]
  ): Min[F[T, L Or R]] =
    Min.instance(rt.unsafeWrap(findValid(at.min(rt.unwrap(ml.min), rt.unwrap(mr.min)))))

@fthomas
Copy link
Owner

fthomas commented Mar 13, 2023

Would something like this work?

Yes, I think so. But since this is Or, the findValid call should not be needed.

@steinybot
Copy link
Contributor

Oh good point. I realised that still isn't going to cut it. There isn't one for Equal.

This seems to fix it:

  implicit def orMin[F[_, _], T, L, R](implicit
      rt: RefType[F],
      ml: Min[F[T, L]],
      mr: Min[F[T, R]],
      at: Adjacent[T],
      v: Validate[T, L Or R]
  ): Min[F[T, L Or R]] =
    Min.instance(rt.unsafeWrap(at.min(rt.unwrap(ml.min), rt.unwrap(mr.min))))
  
  implicit def equalMin[F[_, _], T, N](implicit
      rt: RefType[F],
      wn: WitnessAs[N, T]
  ): Min[F[T, Equal[N]]] =
    Min.instance(rt.unsafeWrap(wn.snd))

@fthomas
Copy link
Owner

fthomas commented Mar 13, 2023

It probably makes sense to add these two instances for Min and Max.

On a side note: The built-in GreaterEqual[N] should be preferred over Greater[N] Or Equal[N].

@toddburnside
Copy link
Author

On a side note: The built-in GreaterEqual[N] should be preferred over Greater[N] Or Equal[N].

Agreed. I found this example/problem in a different library and I'm putting in a PR to do just that. 😄

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

3 participants