A type test on an opaque type test leads to an incorrect "cannot be checked at runtime" warning

[smarter]

Given:

object A {
  opaque type Foo = Int | String
}
object B {
  def test(x: Any) = x match {
    case x: A.Foo =>
      println(x)
    case _ =>
  }
}

We get:

-- Warning: try/iso.scala:6:9 --------------------------------------------------
6 |    case x: A.Foo =>
  |         ^^^^^^^^
  |         the type test for A.Foo cannot be checked at runtime

But Dotty is being too hard on itself here: it actually was able to generate a perfectly valid type test, I suspect that the logic in https://github.com/lampepfl/dotty/blob/10091bd6aef67580eb8e786444c225568f831255/compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala#L33-L56 does not handle opaque types correctly.

[LPTK]

It's not clear whether we want to do that. If the goal of an opaque type is to cleanly abstract over some implementing type (but without paying for it at runtime), then a user should not be able to indirectly observe what the opaque type is defined as.

Concretely, if I use an opaque to hide some implementation detail of my library, I wouldn't like user code to break when I change how it's implemented — for example, going from String to List[Char] (the latter cannot be checked at runtime).

And this would give people a way to bypass the opaque type abstraction without even getting a warning for it:

def bypass(a: String): Option[NonEmptyString] = a match {
  case nes: NonEmptyString => Some(nes)
  case _ => None
}
bypass("") // violates the invariant of NonEmptyString

[smarter]

Concretely, if I use an opaque to hide some implementation detail of my library, I wouldn't like user code to break when I change how it's implemented — for example, going from String to List[Char] (the latter cannot be checked at runtime).

If you do that, you're breaking binary compatibility anyway, that's the price to pay for not wrapping things.

[smarter]

We could also choose to not support these type tests to make opaque types "more opaque", but then the message the type test for A.Foo cannot be checked at runtime is misleading.

P.S.: For context, the case I'm actually interested in is inspired by https://meta.plasm.us/posts/2019/09/23/scala-and-the-visitor-pattern/ and looks like:

sealed trait Json
case object JsonNull extends Json
private case class JsonBigDecimalNumber(value: BigDecimal) extends Json
private case class JsonLongNumber(value: Long) extends Json
//...

opaque type JsonNumber = JsonBigDecimalNumber | JsonLongNumber
object JsonNumber {
  def unapply(json: JsonNumber): Some[BigDecimal] = json match {
    case JsonBigDecimalNumber(value) => Some(value)
    case JsonLongNumber(value) => Some(BigDecimal(value))
  }
}

And then in some other file:

def countValues(json: Json): Int = json match {
  case JsonNumber(_) =>
    1
}

Gives a precise warning It would fail on pattern case: JsonNull, ..., but also gives a warning since to match the extractor JsonNumber we first have to do a type-test on JsonNumber, if we change the unapply to take a Json instead, then the result must be an Option so the exhaustiveness checking becomes less precise (we haven't eliminated any case by matching on JsonNumber(_)).

[smarter]

It's possible to work around the type-test warning by defining:

 def unapply(json: JsonNumber @unchecked): Some[BigDecimal] = ...

But that could also turn off other legitimate warnings, as in #7204 (comment)

[LPTK]

@smarter What do you make of the end of my message? Allowing to match on opaque types in the way you're proposing basically makes opaque types unusable for enforcing invariants statically (such as NonEmptyString), as is done in e.g. Haskell.

[smarter]

@LPTK Right, that's why I said:

We could also choose to not support these type tests to make opaque types "more opaque", but then the message the type test for A.Foo cannot be checked at runtime is misleading.

[bishabosha]

i don't think its because of opaque: #5494

[smarter]

ah indeed, I didn't even think to check what happened without opaque.

[LPTK]

This issue should arguably remain open until there is a proper error or warning when someone tries to match on an opaque type, or until it is explicitly decided that such a breach of abstraction is fine (but that would really weaken the usefulness of opaque types IMHO).

[smarter]

Agreed, but if I leave this open no one will understand what it's about since the conversation went into multiple directions, and I don't have the motivation to make a new issue.