OptManifest not resolved

[Sciss]

I'm currently adding Dotty support to ScalaSTM. Everything went smooth except that OptManifest implicit compiler resolution seems broken (works through Scala 2.11–13).

Minimized code

We use the following API

object Ref {
  def make[A: OptManifest]: Ref[A] = ???
}
trait Ref[A]

which is needed to distinguish between primitive types and reference types, and create arrays.

This works in the cases where the type is concrete, such as Ref.make[String] or Ref.make[Int], but also when it is abstract such as

trait Foo[A] {
  val bar = Ref.make[Int]
  val baz: Ref[A] = Ref.make
}

Output

This no longer works in Dotty

[error] -- Error: /data/temp/dotty-optmanifest/src/main/scala/Foo.scala:2:25 -----------
[error] 2 |  val bar = Ref.make[Int]
[error]   |                         ^
[error]   |no implicit argument of type OptManifest[Int] was found for an implicit parameter of method make in object Ref
[error] -- Error: /data/temp/dotty-optmanifest/src/main/scala/Foo.scala:3:28 -----------
[error] 3 |  val baz: Ref[A] = Ref.make
[error]   |                            ^
[error]   |no implicit argument of type OptManifest[A] was found for an implicit parameter of method make in object Ref
[error] two errors found

Expectation

It should work, like it did in Scala 2.13, i.e. resolve to either ClassManifest or NoManifest. E.g.:

https://travis-ci.org/github/scala-stm/scala-stm/jobs/714280130

[smarter]

We're not planning to implement support for Manifest in Dotty, because they're all but deprecated and known to have issues (I wanted to remove them from 2.13 but it didn't go through), if you want to make arrays, use a ClassTag instead, if you need something more advanced, you'll probably need macros: http://dotty.epfl.ch/docs/reference/metaprogramming/macros.html

[Sciss]

Ok, but can I get an OptClassTag somehow?

[smarter]

I think that's doable without compiler support:

import scala.reflect.ClassTag

case class OptClassTag[T](tag: Option[ClassTag[T]])
object OptClassTag extends OptClassTagLowPriorityImplicits {
  implicit def optClassTagFound[T](implicit tag: ClassTag[T]): OptClassTag[T] = OptClassTag(Some(tag))
}
trait OptClassTagLowPriorityImplicits {
  implicit def optClassTagNotFound[T]: OptClassTag[T] = OptClassTag(None)
}

object Test {
  type A
  implicitly[OptClassTag[A]]
  implicitly[OptClassTag[String]]
}

[Sciss]

I see, but this is more expensive - two times wrapping instead of zero. What I don't understand is why we cannot have the same implicit OptManifest resolution as in 2.13? Because ClassTag already extends OptManifest, and obviously ClassTag still exists in Dotty. If you want to get rid of "manifest", would it not suffice to use ClassTag extends OptClassTag in Dotty instead of ClassTag extends OptManifest in 2.13?

[Sciss]

I mean, unless IntelliJ is fooling me, I still have the 2.13.2 class library on the path when using Dotty (0.24.0-RC1)? So the only thing missing is the implicit resolution. ?

[smarter]

I see, but this is more expensive - two times wrapping instead of zero.

You can reduce it to one wrapper easily by defining OptClassTagSome/OptClassTagNone or something. You should be able to reduce it done to zero wrappers using a trick similar to https://github.com/sjrd/scala-unboxed-option, but that's probably not worth it.

What I don't understand is why we cannot have the same implicit OptManifest resolution as in 2.13?

We could yes, but Manifest is effectively deprecated and we'd rather have people stop using it since it can be misleading (I would even prefer if people refrained from using ClassTag at all since it's so easy to misuse: zio/zio#3136)

f you want to get rid of "manifest", would it not suffice to use ClassTag extends OptClassTag in Dotty instead of ClassTag extends OptManifest in 2.13?

Dotty reuses the Scala 2.13 standard library to stay binary compatibility so we can't do anything like that.

[smarter]

To ease migration, I think we'd accept a PR to support Manifests in implicit search under the Scala 2 compatibiltiy mode (-source:3.0-migration).

[Sciss]

Dotty reuses the Scala 2.13 standard library

then what is the problem with supporting implicitly[OptManifest[X]] ?

I think it's a bit contradictory to say one the one hand the library is the 2.13 library, and on the other hand to say we want to deprecate things and remove implicit resolution even though those classes are still around and functional. Then I think the normal Scala path is to put @deprecated on Manifest and that's it, and it stil works in 3.0.x...

[smarter]

Yeah, it's a bit awkward. I tried removing Manifest from 2.13 but that was rejected since there's some usecases which can only be replaced by TypeTag, which is Scala 2 specific and requires scala-reflect: scala/scala#6745.

[Sciss]

So I think the best path might be to reenable the implicit resolution in 3.0.x to stay compatible with 2.13.x, add @deprecated and in 3.1.x introduce OptClassTag instead as a more light-weight means to be able to create arrays or fall back to AnyRef arrays. (In any case it's weird, as in 2.13 the implicit resolution is not visible in Predef or elsewhere but seems to be automagically injected by the compiler).

[smarter]

in 3.1.x introduce OptClassTag instead as a more light-weight means to be able to create arrays or fall back to AnyRef arrays

Does your fallback involve casting the AnyRef array to an array of your input type? We shouldn't encourage people to do that becaue that can lead to ClassCastException:

object Foo {
  opaque type A = Int
  def get: A = 1
  def put(x: Array[A]): Unit = {}
}
object Test {
  def singletonArray[T](x: T): Array[T] =
    Array[AnyRef](x.asInstanceOf[AnyRef]).asInstanceOf[Array[T]]

  def main(args: Array[String]): Unit = {
    val arr = singletonArray(Foo.get)
    Foo.put(arr) // java.lang.ClassCastException: [Ljava.lang.Object; cannot be cast to [I
  }
}

(opaque types are an easy way to demonstrate the problem, I don't know if it can also manifest itself using only Scala 2 features).

Anyway, I'll reopen this issue but I still recommend leaving anything related to Manifest under the Scala 2 compatibility mode.

[Sciss]

Does your fallback involve casting the AnyRef array to an array of your input type?

No; it simply specialises for all primitives and AnyRef. The involved constructor method looks like this:

  def make[A]()(implicit om: OptManifest[A]): Ref[A] = (om match {
    case m: ClassTag[_] => m.newArray(0).asInstanceOf[AnyRef] match {
      // these can be reordered, so long as Unit comes before AnyRef
      case _: Array[Boolean] => apply(false)
      case _: Array[Byte]    => apply(0 : Byte)
      case _: Array[Short]   => apply(0 : Short)
      case _: Array[Char]    => apply(0 : Char)
      case _: Array[Int]     => apply(0 : Int)
      case _: Array[Float]   => apply(0 : Float)
      case _: Array[Long]    => apply(0 : Long)
      case _: Array[Double]  => apply(0 : Double)
      case _: Array[Unit]    => apply(())
      case _: Array[AnyRef]  => factory.newRef(null.asInstanceOf[A])(m.asInstanceOf[ClassTag[A]])
    }
    case _ => factory.newRef(null.asInstanceOf[Any])(implicitly[ClassTag[Any]])
  }).asInstanceOf[Ref[A]]

(this is from Nathan Bronson's original code). The apply methods are then all overloaded. So the problem here is that make is not having a value of the element type to check, and it cannot be overloaded. I don't know how expensive newArray(0) is, I imagine there is even a cheaper way to figure out which element type we have.

[Sciss]

We could also just drop this optimisation in Scala-STM, as Ref.make[Int] is rather rare, and one could recommend to the user to use Ref(0) instead.

[smarter]

No; it simply specialises for all primitives and AnyRef. The involved constructor method looks like this:

I see a bunch of casts in this code snippet so I'd say there's a good chance it is in fact unsound, but it's hard to say without the full code.

[Sciss]

Scala-STM is highly optimised; it has an extensive test suite, and I can vouch that we don't run into any class cast exceptions anywhere.

[smarter]

The ClassTag casting looked very suspicious to me, it appears to be OK in practice because the ClassTag is only passed to factory.newRef and none of the implementations of that method actually use the ClassTag for anything (but if one day someone decides to write their own implementation of RefFactory, they could break that assumption, so it'd be best to change the signature of newRef to not take a ClassTag).

[smarter]

By the way, it should be possible to implement make in dotty using an inline match on an erasedValue instead of passing a ClassTag:

 inline def make[A](): Ref[A] = inline erasedValue[A] match {
    case _: Boolean => apply(false)
    case _: Byte => apply(0 : Byte)
    // ...
    case _ => apply(null.asInstanceOf[A])
  }

See http://dotty.epfl.ch/docs/reference/metaprogramming/inline.html

[LPTK]

@smarter

(opaque types are an easy way to demonstrate the problem, I don't know if it can also manifest itself using only Scala 2 features).

Opaque types are nothing special and can be encoded in Scala 2, so yes:

trait Foo {
  type A
  def get: A
  def put(x: Array[A]): Unit
}
object Foo extends Foo {
  type A = Int
  def get = 1
  def put(x: Array[Int]): Unit = {}
}
object Test {
  def singletonArray[T](x: T): Array[T] =
    Array[AnyRef](x.asInstanceOf[AnyRef]).asInstanceOf[Array[T]]

  def main(args: Array[String]): Unit = {
    val arr = singletonArray(Foo.get)
    Foo.put(arr) // java.lang.ClassCastException: [Ljava.lang.Object; cannot be cast to [I
    println("k")
  }
}

[smarter]

scala.compiletime: https://github.com/lampepfl/dotty/blob/master/library/src/scala/compiletime/package.scala