Reenable class shadowing

[Blaisorblade]

Rumor is, Martin wants it re-enabled, and we probably should at least under Scala2 mode — and then, why not in Dotty proper: scala/bug#8353 (comment)

@nafg proposed on Gitter maybe we should restrict it to use cases with an unsurprising semantics, where the shadowing class in fact overrides the superclass, such as:

trait Core {
  class Status
}
trait Ext extends Core {
  class Status extends super.Status
}

Class shadowing, in contrast, is more surprising.

[Blaisorblade]

So, naively dropping the current checks altogether (dotty-staging/dotty@630e2a7) makes shadowing work:

scala> trait Core {
           class Status
         }; trait Ext extends Core {
           class Status
         }
// defined trait Core
// defined trait Ext
scala> val a = new Core{}
val a: Core = anon$1@141d3d43
scala> val b = new a.Status
val b: a.Status = Core$Status@5f174dd2
scala> val c = new Ext{}
val c: Ext = anon$1@d257579
scala> val d = new c.Status()
val d: c.Status = Ext$Status@d16be4f
scala> val d = new c.Status
val d: c.Status = Ext$Status@630bf683

but overriding still fails:

scala> trait Core {
           class Status
         }; trait Ext extends Core {
           class Status extends super.Status
         }
4 |    class Status extends super.Status
  |                         ^^^^^^^^^^^^
  |      Cyclic inheritance: class Status extends itself
  |      (Note that inheriting a class of the same name is no longer allowed)

plus of course there's a bunch of overriding checks to steal from Scalac. Still, that that code survives most of the pipeline intact is cool. Even more cool, the typechecker already seems to distinguish the two types:

scala> d: Core#Status
1 |d: Core#Status
  |^
  |found:    c.Status(d)
  |required: Core#Status'
  |
  |where:    Status  is a class in trait Ext
  |          Status' is a class in trait Core
scala> d: a.Status
1 |d: a.Status
  |^
  |found:    c.Status(d)
  |required: a.Status'
  |
  |where:    Status  is a class in trait Ext
  |          Status' is a class in trait Core
scala> d: Ext#Status
val res0: Ext#Status = Ext$Status@630bf683

EDIT: removed leftovers from me failing at Scala

[Blaisorblade]

Dotty seems close to supporting shadowing, but supporting actual overriding is less trivial — if I disable checks stupidly, shadowing works but overriding gives cyclic reference errors (dotty-staging@b81ddbf), from an exception during Typer:

$ dotc -d out -language:Scala2 tmp/i4361.scala
-- [E046] Syntax Error: tmp/i4361.scala:5:8 ------------------------------------
5 |  class Status extends super.Status
  |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |  cyclic reference involving class Status

longer explanation available when compiling with `-explain`
one error found

[nafg]

trait Core {
 class Status
}
trait Ext extends Core {
 class Status extends super.Status
}

The problem is, at least in Scala 2, that is still shadowing, not overriding.

For instance, with type members, you could do

trait Core {
  type Status <: AnyRef
  def method: Status
}
trait Ext extends Core {
  type Status <: String
}
(null : Ext).method: String

Whereas with classes, there is still no relationship between Core#Status and Ext#Status. References in Core to Status refer to Core#Status even for an Ext.

One particular challenge that comes to mind is, what if code in Core invokes Status' constructor? How would actually overriding a class affect that?

[Blaisorblade]

I was looking at this and at scala/bug#7278, and realized that we have no evidence that shadowing is sound — it wasn't in Scala 2 (see for instance fail2 there).

The only evidence of soundness are the encodings of classes vaguely sketched in the DOT papers.
There, classes are encoded using upper bounds, so the only possible encoding in DOT of this code from scala/bug#7278:

class A { class E }
class B extends A { class E }

is such that, to the public, A has an upper-bounded member E, and B has an upper-bounded member E that can restrict the bound. That's what you get using type members. That issue continues with

def fail2() {
    val b = new B
    var x1: p.E forSome { val p: A } = new b.E // should not compile
    var x2: p.E forSome { val p: B } = new b.E
    x1 = x2 // should not compile
  }

I expect you can write this without existentials — I think def foo(p: B)(x: p.E): (p: A).E = x should certainly compile if E is a type member, which means that for type members B#E must extend A#E.

In fact, I expect def foo(p: B)(x: p.E): (p: A).E = x must compile always — it's not clear how you could have different rules for type members and nested classes, if you want nested classes to be able to implement type members:

trait A {
  type Mem
}
trait B extends A
trait C extends A {
  class Mem
}
trait D extends A with B {
  // class Mem // Must give an error, because B#E is a type member hence it must be <: A#E
  class Mem extends super.Mem // OK
}

One particular challenge that comes to mind is, what if code in Core invokes Status' constructor? How would actually overriding a class affect that?

If you want a "virtual" constructor, you must encode it by hand as a virtual method. Then you're encoding virtual classes, which is arguably the only useful usage pattern I know for all this. That encoding was used by views in Scala collections 2.8-2.12:
https://github.com/scala/scala/blob/v2.12.5/src/library/scala/collection/TraversableViewLike.scala#L276
https://github.com/scala/scala/blob/v2.12.5/src/library/scala/collection/IterableViewLike.scala#L106

What you can sensibly translate to DOT is something like:

trait Core {
 class Status
 def newStatus: Status = new Status
}

trait Ext extends Core {
 class Status extends super.Status { def foo: Int = 1 }
 override def newStatus: Status = new Status
}

Here's an attempt at translation into DOT (written in Scala syntax) freely inspired by the examples in the papers.

type Core = {
  type Status
  def newStatus: Status
}
val Core: Core = new {
  type Status = {}
  def newStatus: Status = new Status {}
}
type Ext = {
  type Status <: { def foo: Int = 1 }
  def newStatus: Status
}
// Structural typing rules ensure that Ext <: Core
val Ext: Ext = new {
  type Status = { def foo: Int = 1 }
  def newStatus: Status = new Status
}

[nafg]

If it must be written by hand, then you run into the issue I illustrated -- the type has not in fact been overridden. References to the type in the outer superclass still refer to the inner superclass.

…

On Wed, Apr 25, 2018, 6:33 AM Paolo G. Giarrusso ***@***.***> wrote: I was looking at this and at scala/bug#7278 <scala/bug#7278>, and realized that we have no evidence that shadowing is sound — it wasn't in Scala 2 (see for instance fail2 there). The only evidence of soundness are the encodings of classes vaguely sketched in the DOT papers. There, classes are encoded using upper bounds, so the only possible encoding in DOT of this code from scala/bug#7278 <scala/bug#7278>: class A { class E }class B extends A { class E } is such that, to the public, A has an upper-bounded member E, and B has an upper-bounded member E that can restrict the bound. That's what you get using type members. That issue continues with def fail2() { val b = new B var x1: p.E forSome { val p: A } = new b.E // should not compile var x2: p.E forSome { val p: B } = new b.E x1 = x2 // should not compile } I expect you can write this without existentials — I think def foo(p: B)(x: p.E): (p: A).E = x should certainly compile if E is a type member, which means that for type members B#E must extend A#E. In fact, I expect def foo(p: B)(x: p.E): (p: A).E = x must compile always — it's not clear how you could have different rules for type members and nested classes, if you want nested classes to be able to implement type members: trait A { type Mem }trait B extends Atrait C extends A { class Mem }trait D extends A with B { // class Mem // Must give an error, because B#E is a type member hence it must be <: A#E class Mem extends super.Mem // OK } One particular challenge that comes to mind is, what if code in Core invokes Status' constructor? How would actually overriding a class affect that? If you want a "virtual" constructor, you must encode it by hand as a virtual method. Then you're encoding virtual classes, which is arguably the only useful usage pattern I know for all this. That encoding was used by views in Scala collections 2.8-2.12: https://github.com/scala/scala/blob/v2.12.5/src/library/scala/collection/TraversableViewLike.scala#L276 https://github.com/scala/scala/blob/v2.12.5/src/library/scala/collection/IterableViewLike.scala#L106 What you can sensibly translate to DOT is something like: trait Core { class Status def newStatus: Status = new Status } trait Ext extends Core { class Status extends super.Status { def foo: Int = 1 } override def newStatus: Status = new Status } Here's an attempt at translation into DOT (written in Scala syntax) freely inspired by the examples in the papers. type Core = { type Status def newStatus: Status }val Core: Core = new { type Status = {} def newStatus: Status = new Status {} }type Ext = { type Status <: { def foo: Int = 1 } def newStatus: Status }// Structural typing rules ensure that Ext <: Coreval Ext: Ext = new { type Status = { def foo: Int = 1 } def newStatus: Status = new Status } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4361 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGAUBH6u02XEnvrm5k6g4MU7FFjpR85ks5tsFEXgaJpZM4TgeeA> .

[Blaisorblade]

@nafg Thanks, you're right. I suspect you could fix this properly by changing the semantics for references to the type in principle, but that wouldn't be compatible with existing Scala code.

And the encoding I have in mind for that would leak in the Java ABI: you'd have to compile class Outer { class Inner } to class Outer { class Inner$1; type Inner <: Inner$1 }, or something equivalent. Maybe with enough effort you could change the names visible in the source semantics but not in the output but that looks much harder than things we already struggle with, historically, plus a recipe for confusion and for violating expectations.

So, right now, it seems to me it's better to keep class shadowing forbidden altogether, at least in Dotty mode. I didn't manage to discuss this with Martin properly yet.

I'm not sure what we should transition-wise regarding scala/bug#7278 — @adriaanm WDYT?
I'm not sure whether this should be supported in Dotty's Scala2 mode. It'd be easier to deprecate it in 2.14 (or do we need to deprecate it in 2.13? It seems too late for that).

If you still want virtual classes

And if you're one of the people who are aware of virtual classes enough to encode them (either by hand or via additional automation), you can do all that encoding with type members by hand. Googling links to
http://lampwww.epfl.ch/~hmiller/scala2014/proceedings/p67-weiel.pdf on automating it (via annotation macros, ahem), and to http://www.scala-archive.org/scala-when-will-Scala-get-virtual-class-support-td2002555.html, which explains the pattern. BTW, the problem pointed out by Geoffrey Washburn there should be solved by Dotty's, since its checks on realizability are more lenient than Scala's restrictions on volatile types.

PS: why drop something Java does have

And just to address another argument — IIRC, today I've seen Martin argue (correctly) that inner class shadowing's allowed in Java. But that doesn't mean we should do the same in Scala, because Java has no virtual types — it's easy to have either Java shadowing for inner classes or Scala abstract types, but you can't mix the two naively. And even Java interfaces cannot require the existence of a nested class with a certain name, so no requirement there either.

[adriaanm]

I agree it's a bug that this compiles: var x1: p.E forSome { val p: A } = new b.E. Would be great to understand why this is, since rebind in TypeRef explicitly rules out classes as overridable members.

[odersky]

I think it's safest to disallow class shadowing. It looks like true overriding but isn't. So this is confusing at best, and is also an annoying special case.

[adriaanm]

By shadowing, you mean the second eponymous definition should be disallowed?

[odersky]

By shadowing, you mean the second eponymous definition should be disallowed?

Correct. Dotty currently supports it only in Scala-2 mode, but super.C does not work correctly.