Name clash for $outer pointers of two different nesting levels #2382

sjrd · 2016-05-04T10:13:42Z

object HelloWorld extends js.JSApp {
  def assertEquals(expect: Int, actual: Int, field: String): Unit =
    assert(expect == actual, s"Expected $field to be <$expect> but was <$actual>")

  def main() {
    val b = new B { }
    val y = new b.Y
    val z = new y.Z

    assertEquals(1, b.a, "b.a")
    assertEquals(1, z.z1, "z.z1")
    assertEquals(2, y.y, "y.y")

    assertEquals(2, z.z2, "z.z2")
    //  assertEquals(2, z.z3, "z.z3") // for variant

  }
}

trait A {
  val a: Int = 1
  class X {
    def x: Int = a
  }
}

trait B extends A {
  class Y {
    def y: Int = 2
    class Z extends X {
      def z1: Int = a
      def z2: Int = x
    }
  }
}

//// Variant that also fails
//trait B extends A {
//  class Y {
//    def y: Int = 2
//    class Z extends X {
//      def z1: Int = a
//      def z3: Int = y
//    }
//  }
//}

nicolasstucki · 2016-05-04T12:43:56Z

Found out that the second variant can be minimized further to:

trait A {
  val a: Int = 1
  class X
}

trait B extends A {
  class Y {
    def y: Int = 2
    class Z extends X {
      def z1: Int = a
      def z3: Int = y
    }
  }
}

nicolasstucki · 2016-05-06T07:44:27Z

Further minimization to:

trait A {
  val a: Int = 1
  class X
  class Y {
    class Z extends X {
      def b: Int = a
    }
  }
}

And the issue does not manifest itself with the $outer, it is actually on accessing a directly from an instance of A.

  def main() {
    val b = new A { }
    val y = new b.Y
    val z = new y.Z

    assertEquals(1, b.a, "b.a")
    assertEquals(1, z.b, "z.b")

  }

will produce

$c_Lhelloworld_HelloWorld$.prototype.main__V = (function() {
  var b = new $c_Lhelloworld_HelloWorld$$anon$1().init___();
  var y = new $c_Lhelloworld_A$Y().init___Lhelloworld_A(b);
  var z = new $c_Lhelloworld_A$Y$Z().init___Lhelloworld_A$Y(y);
  this.assertEquals__I__I__T__V(1, b.a$1, "b.a");
  this.assertEquals__I__I__T__V(1, z.b__I(), "z.b")
});

which fails with

org.mozilla.javascript.EcmaError: TypeError: Cannot read property "a$1" from undefined (Lhelloworld_A$Y$Z.sjsir#23)

nicolasstucki · 2016-05-06T08:58:53Z

With the optimizer disabled the failure shows itself on a missing $outer method

org.mozilla.javascript.EcmaError: TypeError: Cannot find function helloworld$A$Y$$$outer__Lhelloworld_A in object helloworld.A@1. (Lhelloworld_A$Y$Z.sjsir#14)

As a deserialization hack, we mangle all Scala fields that are called `$$outer$f`, i.e., those that were `protected val $outer` in scalac. We mangle them with their type, which is always locally known (in `FieldDef`s and `Select`s, which are the only two IR nodes dealing with Scala fields). This almost fixes the name clashes. It is still possible to have a clash for classes that are nested in the same class, but for those, all the clashing `$outer` pointers must have the same value, and so the clash is not a problem in practice. For `$outer` pointers at different levels of nesting, which are the ones that were causing scala-js#2382, the type of the `$outer` pointers must be different, by construction. The mangling by type therefore always distinguishes them.

sjrd · 2016-05-26T07:27:49Z

So the fix in PR #2384 proved to be binary incompatible, so that doesn't work. Consequently, I imagined a link-time hack to disambiguate $outer field at deserialization time based on their type:
master...sjrd:fix-outer-at-link-time
That works well for Scala classes, but it does not work for Scala.js-defined JS classes, because the type of their instances is any. So if the outer class of a class is a Scala.js-defined JS class, it would still expose the bug. Even though that fixes the scala-compiler use case, it's both too hacky and not good enough to justify its inclusion.

So basically, we're stuck. This fix will have to wait for a binary breaking release, i.e., 1.0.0.

The root cause of the bug scala-js#2625 which surfaces in 2.12.0 is actually scala-js#2382. We cannot fix scala-js#2382 without breaking backwards binary compatibility in general. However, we can fix it only for 2.12.0-RC2 onwards, as RCs of Scala (and the upcoming final) have separate binary ecosystems anyway. So this commit fixes scala-js#2382 but only for 2.12.0-RC2 onwards. The fix is trivial: name mangle all outer pointers with the fully qualified enclosing class.

The root cause of the bug scala-js#2625 which surfaces in 2.12.0 is actually scala-js#2382. We cannot fix scala-js#2382 without breaking backwards binary compatibility in general. However, we can fix it only for 2.12.0-RC2 onwards, as RCs of Scala (and the upcoming final) have separate binary ecosystems anyway. So this commit fixes scala-js#2382 but only for 2.12.0-RC2 onwards. The fix is trivial: name mangle all outer pointers in the same way as if they were private, i.e., with the number of class ancestors of its owner.

sjrd · 2016-12-04T16:17:25Z

This issue closed automatically, but in fact it's still not fixed for Scala 2.10.x and 2.11.x, so reopening.

We simply remove the guard that prevented the fix from being applied in 2.10 and 2.11. It was necessary in 0.6.x not to break backward binary compatibility, but we can now fix it for good.

Fix #2382 for good, on all Scala versions.

sjrd added the bug Confirmed bug. Needs to be fixed. label May 4, 2016

sjrd added this to the v0.6.10 milestone May 4, 2016

nicolasstucki self-assigned this May 6, 2016

nicolasstucki added a commit to nicolasstucki/scala-js that referenced this issue May 6, 2016

Fix scala-js#2382: add suffix to to avoid clashes.

7a0b11e

nicolasstucki added a commit to nicolasstucki/scala-js that referenced this issue May 6, 2016

Fix scala-js#2382: Add suffix to $outer to avoid clashes.

36049d0

nicolasstucki added a commit to nicolasstucki/scala-js that referenced this issue May 6, 2016

Fix scala-js#2382: Add suffix to $outer to avoid clashes.

a290d6f

nicolasstucki added a commit to nicolasstucki/scala-js that referenced this issue May 9, 2016

Fix scala-js#2382: Add suffix to $outer to avoid clashes.

da74806

nicolasstucki added a commit to nicolasstucki/scala-js that referenced this issue May 10, 2016

Fix scala-js#2382: Add suffix to $outer to avoid clashes.

3d2c866

nicolasstucki added the has-pr label May 11, 2016

nicolasstucki added a commit to nicolasstucki/scala-js that referenced this issue May 25, 2016

Fix scala-js#2382: Add suffix to $outer to avoid clashes.

1d48272

sjrd removed the has-pr label May 25, 2016

sjrd assigned sjrd and unassigned nicolasstucki May 25, 2016

sjrd modified the milestones: v1.0.0, v0.6.10 May 26, 2016

This was referenced Oct 11, 2016

Outer pointer checks fail in 2.12 #2625

Closed

IR Checker does not check for member name collisions #2627

Closed

sjrd mentioned this issue Oct 12, 2016

Fix #2625: Fix clash of outer pointers in 2.12.0-RC2 onwards. #2628

Merged

sjrd closed this as completed in #2628 Oct 12, 2016

sjrd reopened this Dec 4, 2016

sjrd added the has-pr label Jun 6, 2017

gzm0 closed this as completed in 0fc2dc0 Jun 6, 2017

gzm0 added a commit that referenced this issue Jun 6, 2017

Merge pull request #2990 from sjrd/fix-outer-pointer-bug-in-all-versions

f3e3990

Fix #2382 for good, on all Scala versions.

sjrd removed the has-pr label Jun 7, 2017

sjrd mentioned this issue Jul 12, 2017

Derived Inner Class Not Calling the Right Outer Function #3056

Closed

sjrd mentioned this issue Nov 10, 2017

Outer instance conflict for classes with more than one outer class #3177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Name clash for $outer pointers of two different nesting levels #2382

Name clash for $outer pointers of two different nesting levels #2382

sjrd commented May 4, 2016 •

edited by nicolasstucki

Loading

nicolasstucki commented May 4, 2016

nicolasstucki commented May 6, 2016 •

edited

Loading

nicolasstucki commented May 6, 2016

sjrd commented May 26, 2016

sjrd commented Dec 4, 2016

Name clash for $outer pointers of two different nesting levels #2382

Name clash for $outer pointers of two different nesting levels #2382

Comments

sjrd commented May 4, 2016 • edited by nicolasstucki Loading

nicolasstucki commented May 4, 2016

nicolasstucki commented May 6, 2016 • edited Loading

nicolasstucki commented May 6, 2016

sjrd commented May 26, 2016

sjrd commented Dec 4, 2016

sjrd commented May 4, 2016 •

edited by nicolasstucki

Loading

nicolasstucki commented May 6, 2016 •

edited

Loading