Proposal for replacing IsXxx extractors in tasty-reflect

[smarter]

https://github.com/lampepfl/dotty/blob/master/library/src/scala/tasty/reflect/TreeOps.scala uses the following pattern:

object IsImport {
  def unapply(tree: Tree) given (ctx: Context): Option[Import] =
    internal.matchImport(tree)
}

object Import {
  // ...
  def unapply(tree: Tree) given (ctx: Context): Option[(Boolean, Term, List[ImportSelector])] =
    internal.matchImport(tree).map(x => (x.importImplied, x.expr, x.selectors))
}

Which leads to usesites like this:

tree match {
  case IsImport(tree) =>
    Import.copy(tree)(...)
  // ...
}

The use of IsImport does not feel very natural, I think the following pattern could be used instead:

object Import {
  // ...
  def unapply(tree: Import @unchecked) given (ctx: Context): Option[(Boolean, Term, List[ImportSelector])] =
    internal.matchImport(tree).map(x => (x.importImplied, x.expr, x.selectors))
}

tree match {
  case tree @ Import(_) =>
    Import.copy(tree)(...)
  // ...
}

Note the use of @unchecked at the definition site to avoid unchecked warnings at use-site, this is still safe since the pattern won't match if the tree isn't actually an Import node.

WDYT @nicolasstucki ?

[nicolasstucki]

I suspect it won't work with types that share the same runtime representation. I did try this pattern in the past and something was a miss. I will double check.

[nicolasstucki]

Unfortunately the @unchecked would allow code like this to compile without an uncheck warning and the pattern would match at runtime but it should not.

val t1: Reflection = ...
val t2: Reflection = ...
val tree: t1.Tree = ...
tree match {
  case t2.IsImport(tree) =>
}

[nicolasstucki]

The closest to a good and simple to use solution I've got is in dotty-staging@3fa4592. This is a rebase of an old PR that was closed.

[nicolasstucki]

@smarter
OK, alternatively we could consider using Typeable, apparently the Typelevel Scala compiler was able to use this typeclass to desugar case x: T: https://github.com/milessabin/shapeless/blob/bd3110d9e075b28ee1323d861e0aca130c6891b4/core/src/main/scala/shapeless/typeable.scala#L49 It seems more promising than ClassTag because the unapply method is in the companion object and looks like this:

def unapply[T: Typeable](t: Any): Option[T] = t.cast[T]

Notice that it delegates the actual unapply logic to the Typeable instance, so we could do somethng like this I think:

given patternIsTypeable(given Context): Typeable[Pattern] {
  def cast(t: Any): Option[T] = // ... operation that needs a Context
}

@milessabin WDYT ? That would require getting Typeable into the standard library. It'd be nice if we could use that to deprecate ClassTag-based pattern matching since this is known to be unsound: https://gist.github.com/Blaisorblade/a0eebb6a4f35344e48c4c60dc2a14ce6

[nicolasstucki]

Typeable would have the same shortcomings as ClassTag because it also accepts Any as an input. We would require the generalization of Typable which may look something like

trait Castable[U, T <: U] {
  def cast(t: U): Option[T]
}
trait Typable[T] extends Castable[Any, T] {
  def cast(t: Any): Option[T]
  ...
}

Then in the reflect interface we would provide instances for Castable[Tree, ValDef], Castable[Tree, Ident], ...

given ValDefIsTypeable(given Context): Castable[Tree, ValDef] {
  def cast(t: Tree): Option[ValDef] = ...
}

[milessabin]

I've no objections to Typeable going in to the standard library, especially if we have support for type arguments in patterns to go with it. I'd also be very happy to see the ClassTag based approximation to this go away.

That said, I'm not convinced that this is the right answer to the particular problem expressed in this PR ... surely there's a design out there that wouldn't require Typeable?