-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Opaque types - selftype encoding #5300
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
Conversation
2b5dfbd
to
006752e
Compare
Replaces #4028 |
The actual implementation starts with "Parsing and pickling of opaque types" d8a6f1c. Before is just general polishing and housekeeping. It is best reviewed commit by commit. |
test performance please |
performance test scheduled: 1 job(s) in queue, 0 running. |
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/5300/ to see the changes. Benchmarks is based on merging with master (2f64743) |
Performance tests look good. No apparent degradation. |
Some explanations, since it's a subtle point. Inside Normally, this dealiasing strategy should not lose equalities but here we have an irregular situation since the module The selftype bound type T >: U | o.T <: U & o.T compensates for the loss of equalities through dealiasing by explicitly making A similar trick can be played if we rely on abstract type sealing. Here's an alternative version of an opaque encoding: object o {
trait T {
type A
}
class C extends T { this: m.type =>
type A = Int
def reveal(x: A): Int = x
def inject(x: Int): A = x
var a: A = ???
x = a // error: found: this.A required: m.A
a = x // error: found: m.A required: this.A
}
val m: T = new C
type A = m.A
var x: A = ???
} There are two errors, which show that, again, object Test {
trait T {
type A
}
class C extends T { this: m.type =>
type A >: Int | Test.A <: Int & Test.A
def reveal(x: A): Int = x
def inject(x: Int): A = x
var a: A = ???
x = a // OK!
a = x // OK!
}
val m: T = new C
type A = m.A
var x: A = ???
} Except that now C is not instantiatable because it contains a type with possibly conflicting bounds. That would need to be fixed separately by introducing a special rule. |
@odersky I'm probably making a foolish-sounding observation without enough knowledge of the subject, but I feel a little concerned about the idea of hanging a language feature off an implementation detail of the subtype checker. Is it very unlikely that the dealiasing strategy you are using now will ever change? |
I believe it is very unlikely. But the issue is really broader than that. @adriaanm's version of bringing opaque types to Scala 2 has exactly the same problem, and I showed in my comment that an alternative encoding would also have exactly the same problem. So the problem looks like its fundamental, and the solution for it works in all incarnations, it's just that the self-type encoding hides it bit better than others. |
This sort of problem is general enough that it appears to be described in the literature. IIUC, this relates to what Derek Dreyer calls this the “double vision” problem, and describes a solution in his RMC and MixML papers. As far as I understand (not yet much), he uses a first pass to collect the extra type equalities to remember for the actual typechecking pass; that’s what you’re doing here in this special case, essentially, by encoding two upper bounds and two lower bounds into one. It’s not yet clear to me whether this two-pass algorithm is unfeasible for us, or whether we could hack the first pass into namer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here’s a few initial comments on the code.
if (isOpaqueHelper) { | ||
info match { | ||
case TypeAlias(alias) => | ||
info = TypeBounds(defn.NothingType, abstractRHS(alias)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat orthogonal, but having different apparent kinds for the bounds seems surprising? I can see you’re encoding the kind-arity this way.
I expect this fulfills the invariants necessary to Dotty, but those still puzzle me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's still up for reconsideration and a possible change. There's comment in the type lambdas spec that states this.
selfType match { | ||
case selfType: TermRef => selfType.symbol | ||
case selfType: Symbol => selfType.info.asInstanceOf[TermRef].symbol | ||
def sourceOfSelf(tp: Any): Symbol = tp match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tp: TypeOrSymbol (from Types) would be more informative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
@@ -763,9 +763,10 @@ object SymDenotations { | |||
*/ | |||
def membersNeedAsSeenFrom(pre: Type)(implicit ctx: Context): Boolean = | |||
!( this.isTerm | |||
|| this.isStaticOwner | |||
|| this.isStaticOwner && !this.isOpaqueCompanion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmh, wondering if other uses of isStaticOwner need to also test isOpaqueCompanion... TODO check
Yes, this looks indeed like an instance of the double vision problem. I believe there's an advantage to solving it without requiring a special pass. And it's kind of cool that union and intersection types allow us to encode multiple aliases in one type. Anyway, Namer is not really a pass, so we could not piggyback operations to it. |
I'm at Reactive Summit this week, but I'll try to wrap up my review today or tomorrow. I haven't had a chance to solve the "double vision problem" in scalac -- it doesn't carry over directly because we don't have union types, and my naive attempt ran into a illegal cyclic ref problem (thanks @Blaisorblade for the pointer, I hadn't made that connection). For the curious, my wip is at scala/scala@2.13.x...adriaanm:opacity (will submit PR once I figure out that remaining issue) |
Another rebase necessary... |
4ef5c41
to
99fdce5
Compare
I considered an alternative design proposed by @smarter: Instead of the concept of a companion object for opaque types, make the alias visible in the scope where the opaque type is defined, and invisible outside. This can achieve some simplification, as we do not need to define the concept of a companion object for an opaque type. On the other hand, it risks being less convenient once we allow toplevel opaque types (which would be desirable). Then we run into two problems:
So, in light of these problems I still think the current spec and implementation is the right one. |
@adriaanm Any review comments? |
Don't use synthetic companion methods to achieve this. The advantages of the direct approach are: - it's overall simpler - it can be more easily extended to opaque types
resolve overload errors should be suppressed if some types are already erroenous.
Distinguish what is printed by previous knowledge whether the symbol is a term or a type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small things I noticed while reading through. Haven't made it to the docs / examples yet.
record(tp, baseTp) // typeref cannot be a GADT, so cache is stable | ||
else | ||
btrCache.remove(tp) | ||
if (inCache(superTp) && tpSym.maybeOwner.isType) record(tp, baseTp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
restore comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a talk with @smarter about this. The comment is actually misleading, and it's very subtle to argue, first that there could be a problem at all, and, second, why the problem does not arise after all. Also, the condition tpSym.maybeOwner.isType
got removed later.
@@ -744,9 +763,10 @@ object SymDenotations { | |||
*/ | |||
def membersNeedAsSeenFrom(pre: Type)(implicit ctx: Context): Boolean = | |||
!( this.isTerm | |||
|| this.isStaticOwner | |||
|| this.isStaticOwner && !this.isOpaqueCompanion | |||
|| ctx.erasedTypes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing this to scalac's implementation, I noticed we still do ASF post erasure when the prefix is Array || phase.erasedTypes && pre.typeSymbol != ArrayClass
. (See val trivial
in def asSeenFrom
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll see if I can find the corresponding bug and report back. I guess erasing a member selection on an array may need to know the element type in the prefix (the array type). Could be that we special cased this elsewhere and this is now redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially a premature optimization. It's been in there for about 13 years: scala/scala@2a5f623#diff-2731df2ea3054555efb4a23f8c628acdR158
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, could've been for correctness: that same diff also reworks transformInfo
in erasure. These days Array does not extend Seq anymore, so maybe that's why it's redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a bootstrap without the special case for Array, but then scalac crashes on compiling ArrayOps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we do compile ArrayOps, so it might be an artifact how parameters are tied to asSeenFrom (which is different in the two compilers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried as well on the 2.13 std lib and it works fine
@@ -744,9 +763,10 @@ object SymDenotations { | |||
*/ | |||
def membersNeedAsSeenFrom(pre: Type)(implicit ctx: Context): Boolean = | |||
!( this.isTerm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, couldn't a term member's info (e.g. a bound) still refer to a member of an outer class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but it's already taken care of when the type is constructed.
@adriaanm From what I understand, the main problem in Haskell is that they have things that behave like type-level functions (type families), which make it unsound to do Liskov-like substitution in general. I think we do not have the problem in Scala because we don't actually have type-level functions, and instead we use implicit programming, which soundly carries pieces of evidence around... that is, until we introduce match types, which are essentially like closed type families. Now, we can see why match types introduce unsoundness. Consider: type M[T] = T match {
case Int => String
case O => Bool
}
opaque type O = Int
object O {
def convert[X[_]]: X[O] => X[Int] = identity
// ^ inside companion we know O =:= Int
}
case class Oops[A](m: M[A])
val oopsO = Oops[O](true)
val oopsI: Oops[Int] = convert(oops) // now oopsI.m has type String!
oopsI.m.length // boom! The way they solved it in Haskell, IIRC, is using type roles to prevent substitution of types in non-parametric contexts. In Scala, the closest thing would be to have a new type kind that says we can actually interpret |
The Haskell discussion comes from the fact that in Haskell, newtype is defined by means of implicit conversions. The problem is lifting these conversions to types containing newtype occurrences. We don't have that problem since opaque types give rise to equalities, which are true congruences. One thing that Haskell can do and we can't is the One question I don't know the answer to is how Haskell's approach interacts with type inference. It requires a suitable decomposition of a type into a constructor |
@LPTK Very good example! I believe the way to solve it is to not reduce
We already have logic in place for this: When faced with a match type
and an occurrence |
@ltpk: The match type example is the only one where we consider negative information of the form "A can't possibly be a subtype of B, in all scenarios or substitution instances" to achieve a positive result. I was very nervous about that when first considering this, and your example shows beautifully the traps one can fall into with this. Haskell has several other such instances (e.g. it's also needed for typeclass coherence), so the problem with type families seems much harder in this setting. |
Note that the required logic for match types is not implemented yet (even in the absence of opaque types). I have opened #5417 to track this. |
I think @Blaisorblade pointed out that GADTs require something similar: #4029 |
GADT exhaustiveness checking, yes. But that's not such a big deal since these are warnings only. |
While we're on the topic of matching: we could use a test case that ensures patmat exhaustivity is aware of opaque types behaviour: opaque type Pos = Int
sealed trait Expr[T]
final case class PosExpr(p: Pos) extends Expr[Pos]
final case class IntExpr(i: Int) extends Expr[Int]
def eval(pe: Expr[Pos]): Pos = pe match {
case PosExpr(p) => p
// unexhaustive: IntExpr
} If opaque type NotInt = Int
type M[A] = A match {
case Int => String
}
sealed trait FunGadt[T, R]
object StrIntFun extends FunGadt[String, Int]
object StrStrFun extends FunGadt[M[NotInt], String]
def get[R](g: FunGadt[M[NotInt], R]): R = g match {
case StrStrFun => ""
// unexhaustive: StrIntFun
} Finally, as a side note, it might be worth keeping in mind that depending on how match types behave we might be able to match on opaque type O = Int
type M[A] = A match {
case Int => String
case O => Boolean
}
type MM[A] = M[A] match {
case String => String
case M[O] => Boolean
} Which should lead to similar unsoundness as in LTPK's example above. It might be easier to just reduce |
@AleksanderBG
Opaque types are just a special case of the general problem of keeping match types sound in the presence of abstract types. You don't always know what an abstract type will end up being an alias to. So you can't always say "it might be easier to just reduce I think the solution, as @odersky explained, is to not reduce matches unless we are certain. We should keep the match type "stuck" as long as we can't be sure that the current case can be ruled out. So in your last example, we can't just skip the |
I agree with @LPTK here, but let me add the rule we should keep in mind. If the companion object has externally equivalent behavior, you would also expect the runtime behavior to be the same, by parametricity, except for the known parametricity-breaking escape hatches (e.g. |
@Blaisorblade let me show an exception to your rule: 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]
def eval(e: Expr[Pos]): Pos = e match {
case PosExpr(p) => p
case IntExpr(_) => Pos.mkPos(1)
}
eval(Pos.coerce[Expr](IntExpr(-1))) (EDIT: fixed the issue from the comment below) One way or another, we will have to leak that opaque types do not create a proper new type. EDIT: Let me maybe say something more to clarify the difference between newtypes and this implementation of opaque types: newtypes truly create a new type, isomorphic to the old one but never equal to it. The isomorphism can then be exploited to zero-cost In contrast, Scala opaque types simply create a type alias which is hidden from most of typechecking. In particular, we can convert between To complete the picture, Haskell coerce :: Coercible a b => a -> b
|
No it doesn't: |
I want to talk about Opaque types on Wednesday at Scale by the Bay. So it would help if this was merged by then. |
@AleksanderBG the equality |
@odersky this PR seems good to go, but I'd like to keep the discussion regarding feature interaction with match types / GADTs / ... going somewhere. What would be the right place for that? |
He mentioned warnings redundant match warnings, but thinking about it I am not sure what the example would be, as all the examples I can write seem illegal:
|
I am merging this now. We should evaluate separately whether we want to move the new RefChecks) :: // Various checks mostly related to abstract members and overriding
List(new ElimOpaque, // Turn opaque into normal aliases
new TryCatchPatterns, // Compile cases in try/catch
new PatternMatcher, // Compile pattern matches
new ExplicitOuter, // Add accessors to outer classes from nested ones.
new ExplicitSelf, // Make references to non-trivial self types explicit as casts It would take probably not take much to move it after |
This commit implements one of the missing aspects of Match Types: an algorithm to determine when it is sound to reduce match types (see discussion in scala#5300). To understand the problem that is being solved, we can look at the motivational example from the [Haskell Role paper](https://www.seas.upenn.edu/~sweirich/papers/popl163af-weirich.pdf) (adapted to Scala). Given this class: ```scala class Foo { type Age type Type[X] = X match { case Age => Char case Int => Boolean } def method[X](x: X): Type[X] = ... } ``` What is the type of `method(1)`? On master, the compiler answers with "it depends", it could be either `Char` or `Boolean`, which is obviously unsound: ```scala val foo = new Foo { type Age = Int } foo.method(1): Char (foo: Foo).method(1): Boolean ``` The current algorithm to reduce match types is as follows: ``` foreach pattern if (scrutinee <:< pattern) return pattern's result type else continue ``` The unsoundness comes from the fact that the answer of `scrutinee <:< pattern` can change depending on the context, which can result in equivalent expressions being typed differently. The proposed solution is to extend the algorithm above with an additional intersection check: ``` foreach pattern if (scrutinee <:< pattern) return pattern's result type if (!intersecting(scrutinee, pattern)) continue else abort ``` Where `intersecting` returns `false` if there is a proof that both of its arguments are not intersecting. In the absence of such proof, the reduction is aborted. This algorithm solves the `type Age` example by preventing the reduction of `Type[X]` (with `X != Age`) when `Age is abstract. I believe this is enough to have sound type functions without the need for adding roles to the type system.
This commit implements one of the missing aspects of Match Types: an algorithm to determine when it is sound to reduce match types (see discussion in scala#5300). To understand the problem that is being solved, we can look at the motivational example from the [Haskell Role paper](https://www.seas.upenn.edu/~sweirich/papers/popl163af-weirich.pdf) (adapted to Scala). Given this class: ```scala class Foo { type Age type Type[X] = X match { case Age => Char case Int => Boolean } def method[X](x: X): Type[X] = ... } ``` What is the type of `method(1)`? On master, the compiler answers with "it depends", it could be either `Char` or `Boolean`, which is obviously unsound: ```scala val foo = new Foo { type Age = Int } foo.method(1): Char (foo: Foo).method(1): Boolean ``` The current algorithm to reduce match types is as follows: ``` foreach pattern if (scrutinee <:< pattern) return pattern's result type else continue ``` The unsoundness comes from the fact that the answer of `scrutinee <:< pattern` can change depending on the context, which can result in equivalent expressions being typed differently. The proposed solution is to extend the algorithm above with an additional intersection check: ``` foreach pattern if (scrutinee <:< pattern) return pattern's result type if (!intersecting(scrutinee, pattern)) continue else abort ``` Where `intersecting` returns `false` if there is a proof that both of its arguments are not intersecting. In the absence of such proof, the reduction is aborted. This algorithm solves the `type Age` example by preventing the reduction of `Type[X]` (with `X != Age`) when `Age is abstract. I believe this is enough to have sound type functions without the need for adding roles to the type system.
A fiendlishly clever way to encode opaque types using a combination of abstract types, self types, and union and intersection types. Refines a suggestion by @adriaanm - that guy has a really twisted mind 😎.
An opaque type / companion object combo
is represented as
The checking of the self type is omitted (objects can't officially have a self type anyway). The bounds in the self type achieve that inside object
T
, typeT.this.T
is known to be an alias of botho.T
andU
. On the other hand, outsideT
,o.T
is known to be an alias ofT.T
, but is not known to be an alias ofU
, since the self type is unavailable outside the object.