Pattern matcher needs to check for outer

[odersky]

When testing against an inner class, the pattern matcher needs to test outer pointers. Consider the following program:

class Outer {

  case class Inner()

  val inner: Inner = new Inner

  def checkInstance(o: Outer) =
    o.inner.isInstanceOf[this.Inner]

  def checkPattern1(i: Any) =
    i match {
      case _: Inner => true
      case _ => false
    }

  def checkPattern2(i: Any) =
    i match {
      case Inner() => true
      case _ => false
    }

  def checkEquals(o: Outer) =
    o.inner == inner
}

object Test {

  def main(args: Array[String]) = {
    val o1 = new Outer
    val o2 = new Outer
    assert(o1.checkInstance(o2)) // ok
    assert(!o1.checkPattern1(o2.inner)) // ok under scalac, fails for dotc-compiled code
    assert(!o1.checkPattern2(o2.inner))  // ok under scalac, fails for dotc-compiled code
    assert(!o1.checkEquals(o2))  // ok under scalac, fails for dotc-compiled code
  }
}

We expect the isInstanceOf test in checkInstance to return true but the other three tests should return false. Yes, I know it's contorted, but these Scala rules were a compromise between
not losing performance and being precise. Essentially we accept that we lose precision in isInstanceOf because otherwise there would be no way to avoid the "outer test tax". But we demand that pattern matchers test outer pointers. Indeed you will find specific code doing that after patmat when you compile the code with scalac:

    if (x1.isInstanceOf[Outer.this.Inner].&&
    ((x1.asInstanceOf[Outer.this.Inner]: Outer.this.Inner).<outer>.eq(Outer.this)))

But such code is currently not emitted by dotc which is why the last three asserts fail.

The discrepancy is the reason why the semantic names PR fails the bootstrap.

[DarkDimius]

I'm having a look at this

[smarter]

The discrepancy is the reason why the semantic names PR fails the bootstrap.

Hah, that must have been pretty hard to find. How is this feature used for semantic names exactly?

[DarkDimius]

@smarter, Martin's design uses inner classes and pattern-matches over them.

[DarkDimius]

Have a fix, running full test suite to verify it.

[DarkDimius]

The bug has specifically to do with the outer being this.

[DarkDimius]

I've made a proper fix for this, unfortunatelly: it broke bootstrap

in https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/io/ClassPath.scala#L245
we have a similar pattern match:

abstract class ClassPath {
 ...
 case class ClassRep(..){ ... }

 rep map {
          case x: ClassRep  => x
          case x            => throw new FatalError("Unexpected ClassRep '%s' found searching for name '%s'".format(x, name))
 }
}

For some reason, Scalac doesn't emmit an outer-class check here. @odersky do you know what's the difference between those two examples?

[odersky]

I think it's a bug in scalac. I played with it a little. First, with the code the map expands to:

    rep map ((x:ClassPath # ClassRep) => x match {
      case x: ClassRep  => x
      case x            => throw new FatalError("Unexpected ClassRep '%s' found searching for name '%s'".format(x, name))
    })

Here the outer check is omitted. But now change the parameter's type to Any:

    rep map ((x:Any) => x match {
      case x: ClassRep  => x
      case x            => throw new FatalError("Unexpected ClassRep '%s' found searching for name '%s'".format(x, name))
    })

The outer check reappears! This makes no sense. A selector of type ClassPath # ClassRep has exactly as much information about the outer pointer as a selector of type Any (i.e. none). So there should not be a difference in the generated code.

My advice would be to delete the whole map, which seems to have never done the right thing.

[lrytz]

Note that scalac omits the outer check for final nested classes

scala> class C { class I; final class J }
defined class C

scala> val c1, c2 = new C
c1: C = C@14f3c6fc
c2: C = C@3cd59ef5

scala> (new c1.I: C#I) match { case _: c2.I => 0; case _ => 1 }
res0: Int = 1

scala> (new c1.J: C#J) match { case _: c2.J => 0; case _ => 1 }
warning: there was one unchecked warning; for details, enable `:setting -unchecked' or `:replay -unchecked'
res1: Int = 0

[lrytz]

@odersky did you try Scala 2.11? It seems to be fixed in 2.12. Here's a reduced example:

class C {
  case class I(x: Int)

  type AnyI = C#I

  def l: List[C] = List(this)

  def f: Option[AnyI] = {
    val r = l.headOption.flatMap(_.f)
    r map {
      case x: I => x
      case _ => ???
    }
  }
}

With 2.11.8

$ scalac11 Test.scala && cfr-decompiler C.class
public class C {
...
    public Option<I> f() {
...
        return r.map((Function1)new Serializable(this){
...
            public final I apply(I x0$1) {
                I i = x0$1;
                if (i instanceof I) {
                    I i2;
                    I i3 = i2 = i;
                    return i3;
                }
                throw Predef..MODULE$.$qmark$qmark$qmark();
            }
...

With 2.12.1:

public class C {
...
    public Option<I> f() {
...
        return r.map(x0$1 -> {
            I i;
            I i2 = x0$1;
            if (!(i2 instanceof I) || i2.C$I$$$outer() != this) {
                throw Predef..MODULE$.$qmark$qmark$qmark();
            }
            I i3 = i = i2;
            return i3;
        }
        );
    }
...

[odersky]

@lrytz I tried an earlier 2.12. Good that it's fixed in the meantime. I also see that ClassPath has changed. It cannot find anymore the problematic test. We should adopt that also for dotty.

But another question: What's the reason for omitting the test for final classes?

[lrytz]

I don't know the full history, but a final nested class doesn't have an outer field. I assume the goal was to avoid capturing the outer instance when serializing a lambda.

[lrytz]

https://issues.scala-lang.org/browse/SI-4440, https://issues.scala-lang.org/browse/SI-1419

[lrytz]

We rely on this trick in 2.12 for local classes defined within closures. If you have

class A {
  def f = () => { class C; serialize(new C) }
}

In 2.11.x, the class C would get an outer pointer to the closure class ($anonfun). In 2.12, with delambdafy:method, the desugaring is

class A {
  def $anonfun { class C; serialize(new C) }
}

so C would get an outer pointer to A, which is problematic - it captures more, and also A might not be serializable. So there's an analysis to find out whether C is has no subclasses, in which case we add the final modifier. This eliminates the outer pointer. scala/scala#4652

[odersky]

@lrytz: What I don't understand: Why would it be safe to special case for final classes? What's different for them?

[lrytz]

What I don't understand: Why would it be safe to special case for final classes?

It's not safe, as shown by my example above. I guess it's another example of re-using the final flag for some use case (similar to compile-time constants).

For final classes the outer field can be elided if it's not otherwise used within the class. In SI-1419, @adriaanm says

When the nested class is final, you can reason locally about whether the outer accessor is needed (except for the pattern matching type test, which is weakened in the absence of the outer field). If the class is not final, a subclass (potentially defined in a separate compilation unit/run) may cause an outer accessor to be needed.

I'm not actually sure when this is the case, example welcome

[DarkDimius]

If the class is not final, a subclass (potentially defined in a separate compilation unit/run) may cause an outer accessor to be needed.

this makes it sound like there may not need more then one outer-accessor. Here's a counter-example

class A{ class B }
class C extends A {self => 
  class D {
     final class E extends self.B  // needs two outer-accessors to support all patter-matchings
  }
}

[smarter]

To quote paulp, "These are semantics out of a nightmare" (https://groups.google.com/forum/#!msg/scala-internals/vw8Kek4zlZ8/LAeakfeR3RoJ). There's a bug report with critical priority open: https://issues.scala-lang.org/browse/SI-4440

[DarkDimius]

SI-4400 doesn't apply to dotty. I've created a PR with the same test to make sure it never applies.

[odersky]

@lrytz In dotty, if a subclass needs an outer accessor, it gets one. I still don't see why the (non-final) superclass would then need an outer accessor as well.

Dotty also has the distinction that some classes always need an outer accessor and others need an outer accessor only if referenced. But finality has nothing to do with it. Instead, classes need an outer accessor only when outer is referenced if the class is local to a term or private, i.e. we can check all possible references that need an outer.

Note that this would handle the anonymous functions in the same way as the finality check.

[adriaanm]

In separate compilation, how can you decide a class ever doesn't need an outer accessor? A match could be added in a future compilation run that needs the outer check.

[DarkDimius]

In separate compilation, how can you decide a class ever doesn't need an outer accessor? A match could be added in a future compilation run that needs the outer check.

That's why it should be kept :-), eliding it is unsound.

[adriaanm]

Agreed.

[odersky]

@adriaanm Inner classes that are visible in other compilation units always get an outer accessor.

[adriaanm]

As they should, but that's not the case in Scala 2 as far as I recall. (And that's a bug, I'd say.)

[odersky]

@adriaanm Yes, I think so as well. I am not sure anymore what was the motivation to do it the way scalac does it. Performance? Is there a common use case where a publicily visible class should not get an outer accessor? I can't think of one right now.

[adriaanm]

I think it was memory footprint.

…

On Fri, Mar 31, 2017 at 12:32 odersky ***@***.***> wrote: @adriaanm <https://github.com/adriaanm> Yes, I think so as well. I am not sure anymore what was the motivation to do it the way scalac does it. Performance? Is there a common use case where a publicily visible class should not get an outer accessor? I can't think of one right now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2156 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFjyxSsPiN1oJoBYJEseNOiPzqBp1xTks5rrVTEgaJpZM4MuwyP> .

[lrytz]

class C {
  val f1 = (x: Int) => x

  val f2 = {
    final class anonfun extends Function1[Int, Int] with Serializable { def apply(x: Int) = x }
    new anonfun
  }

  val f3 = {
    class anonfun extends Function1[Int, Int] with Serializable { def apply(x: Int) = x }
    new anonfun
  }
}

object Test {
  def serializeDeserialize[T <: AnyRef](obj: T): T = {
    import java.io._
    val buffer = new ByteArrayOutputStream
    val out = new ObjectOutputStream(buffer)
    out.writeObject(obj)
    val in = new ObjectInputStream(new ByteArrayInputStream(buffer.toByteArray))
    in.readObject.asInstanceOf[T]
  }

  def main(args: Array[String]): Unit = {
    val c = new C
    import c._
    println(serializeDeserialize(f1)(1))
    println(serializeDeserialize(f2)(2))
    println(serializeDeserialize(f3)(3))
  }
}

With 2.11.8:

$ scalac11 Test.scala && scala11 Test
1
2
java.io.NotSerializableException: C
	at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1184)

[DarkDimius]

Those aren't named inner classes. In dotty they won't get outer accessors if they don't need them.

…

On 4 April 2017 21:43:05 Lukas Rytz ***@***.***> wrote: ```scala class C { val f1 = (x: Int) => x val f2 = { final class anonfun extends Function1[Int, Int] with Serializable { def apply(x: Int) = x } new anonfun } val f3 = { class anonfun extends Function1[Int, Int] with Serializable { def apply(x: Int) = x } new anonfun } } object Test { def serializeDeserialize[T <: AnyRef](obj: T): T = { import java.io._ val buffer = new ByteArrayOutputStream val out = new ObjectOutputStream(buffer) out.writeObject(obj) val in = new ObjectInputStream(new ByteArrayInputStream(buffer.toByteArray)) in.readObject.asInstanceOf[T] } def main(args: Array[String]): Unit = { val c = new C import c._ println(serializeDeserialize(f1)(1)) println(serializeDeserialize(f2)(2)) println(serializeDeserialize(f3)(3)) } } ``` With 2.11.8: ``` $ scalac11 Test.scala && scala11 Test 1 2 java.io.NotSerializableException: C at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1184) ``` -- You are receiving this because you were assigned. Reply to this email directly or view it on GitHub: #2156 (comment)