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

Derivation Works Without Derives Clause #16916

Closed
adamgfraser opened this issue Feb 14, 2023 · 3 comments · Fixed by #17414
Closed

Derivation Works Without Derives Clause #16916

adamgfraser opened this issue Feb 14, 2023 · 3 comments · Fixed by #17414

Comments

@adamgfraser
Copy link
Contributor

Compiler version

Scala 3.2.2

Minimized code

import scala.deriving.*
import scala.compiletime.{erasedValue, summonInline}

inline def summonAll[T <: Tuple]: List[Eq[_]] =
  inline erasedValue[T] match
    case _: EmptyTuple => Nil
    case _: (t *: ts) => summonInline[Eq[t]] :: summonAll[ts]

trait Eq[T]:
  def eqv(x: T, y: T): Boolean

object Eq:
  given Eq[Int] with
    def eqv(x: Int, y: Int) = x == y

  def check(elem: Eq[_])(x: Any, y: Any): Boolean =
    elem.asInstanceOf[Eq[Any]].eqv(x, y)

  def iterator[T](p: T) = p.asInstanceOf[Product].productIterator

  def eqSum[T](s: Mirror.SumOf[T], elems: => List[Eq[_]]): Eq[T] =
    new Eq[T]:
      def eqv(x: T, y: T): Boolean =
        val ordx = s.ordinal(x)
        (s.ordinal(y) == ordx) && check(elems(ordx))(x, y)

  def eqProduct[T](p: Mirror.ProductOf[T], elems: => List[Eq[_]]): Eq[T] =
    new Eq[T]:
      def eqv(x: T, y: T): Boolean =
        iterator(x).zip(iterator(y)).zip(elems.iterator).forall {
          case ((x, y), elem) => check(elem)(x, y)
        }

  inline given derived[T](using m: Mirror.Of[T]): Eq[T] =
    lazy val elemInstances = summonAll[m.MirroredElemTypes]
    inline m match
      case s: Mirror.SumOf[T]     => eqSum(s, elemInstances)
      case p: Mirror.ProductOf[T] => eqProduct(p, elemInstances)
end Eq

enum Opt[+T]:
  case Sm(t: T)
  case Nn

@main def test(): Unit =
  import Opt.*
  val eqoi = summon[Eq[Opt[Int]]]
  assert(eqoi.eqv(Sm(23), Sm(23)))
  assert(!eqoi.eqv(Sm(23), Sm(13)))
  assert(!eqoi.eqv(Sm(23), Nn))

Output

// Compiles

Expectation

The snippet above is copied from the documentation of type class derivation in Scala 3 with one change. The documentation describes that we should use Opt[+T] derives Eq. However, the code above actually works without using derives.

I'm not sure if this is a feature or a bug.

On the one hand automatic derivation of instances for sum and product types where instances exist for their elements is the dream. And it does appear that implementers of concrete data types can still provide their own instances in the companion object of the data types which have priority.

However, I can also see some people feeling that this is too powerful to globally derive instances for all data types regardless of whether they use derives to implement their intent to "opt in" to this.

Just want to understand whether this is intended behavior that will continue to be supported in the future and whether this still reflects idiomatic usage of automatic derivation.

@adamgfraser adamgfraser added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 14, 2023
@bishabosha
Copy link
Member

bishabosha commented Feb 14, 2023

I agree that the documentation should not put given on the derived method.

The idea here is that each callsite of derived is doing potentially expensive code generation, that should ideally be minimised. So the idiomatic way to define it should be to leave it a def and then the derives keyword will cache exactly one instance in the companion object - i.e. saving on code generation.

there is also another benefit: if there is recursion in "summoning" in the derived method, then the cached instance will be reused, rather than generate more code on each recursion, unbounded.

@bishabosha
Copy link
Member

fixed by #17414

@bishabosha
Copy link
Member

bishabosha commented May 5, 2023

Please review the new doc pages at

also make sure your browser didn't cache the old page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants