Skip to content

Test interaction between opaque and match #5436

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 14, 2018
Merged

Test interaction between opaque and match #5436

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions tests/pos/opaques-patmat.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
object OpaquePatmat {
opaque type Pos = Int
object Pos {
def mkPos(i: Int): Pos = {
require(i > 0)
i
}
def coerce[F[_]](fa: F[Int]): F[Pos] = fa
}

sealed trait Expr[T]
final case class PosExpr(p: Pos) extends Expr[Pos]
final case class IntExpr(i: Int) extends Expr[Int]
final case class StrExpr(s: String) extends Expr[String]

def eval(e: Expr[Pos]): Pos = e match {
case PosExpr(p) => p
// both of the patterns need to be well-typed here,
// since Pos is potentially equal to any other type
case IntExpr(_) => Pos.mkPos(1)
case StrExpr(_) => ???
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a shame this is not seen as dead code. Expr[T] is sealed, and there are only three valid values for T: Pos, Int and String, each of which is fully determined by the subclass.

Would be nice if case PosExpr(p) => p was seen as a total match here.

Copy link
Contributor

@LPTK LPTK Nov 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can have that (seeing case StrExpr(_) as dead) by moving the match to the companion of the opaque type. You should not have that otherwise, as it would be making the abstraction leak, which is very bad.

Also, case PosExpr(p) is not "total" as any IntExpr(_) can also be given type Expr[Pos].

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I neglected the existence of coerce here, which can translate F[Int] to F[Pos]. I see your point: for an opaque type, we have to inspect and understand all possible functions like this in the companion, and it may be pretty non-trivial to rule out certain branches.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @LPTK.
@johnynek There are two different problems — checking whether StringExpr is dead and whether IntExpr is dead. Checking those scenarios would break guarantees you expect from a type system. For both, it'd only be fine in whole-program static analysis tools, but you have different expectations. This is a tradeoff currently under discussion.

  • As @LPTK said, it's easy to figure out that StrExpr is dead once you know that Pos is implemented as Int — ignoring eval(StrExpr.asInstanceOf[Expr[Pos]) (which we should). But we shall not do it to avoid abstraction leaks. To be more precise on the guarantee: If client code compiles and you change Pos to be implemented as String, the client code should keep compiling.
  • It's hard to figure out whether IntExpr is dead, by checking whether coerce-like functions exist. It might be possible by static analysis of the companion object, but again you change warnings in client code depending on implementation details that (it seems) should be hidden (and are not visible in types). That breaks separate compilation/separate typechecking. Type systems usually don't do that, because if they did it'd be much harder to keep a library source-compatible. Especially with -Xfatal-warnings.

}

eval(Pos.coerce[Expr](IntExpr(-1)))
}