Skip to content

Conversation

@axel22
Copy link
Contributor

@axel22 axel22 commented Aug 21, 2012

This bug is a result of a subtle interplay of the stackable modifications
mechanism and specialization.

Prior to this commit, using abstract override with specialization was
broken in the sense that specialization did not create a specialized
version of the super accessor. Observe the following code:

trait A[@specialized(Int) T] {
  def foo(t: T)
}

trait B extends A[Int] {
  def foo(t: Int) {
    println("B.foo")
  }
}

trait M extends B {
  abstract override def foo(t: Int) {
    super.foo(t)
    println("M.foo")
  }
}

object C extends B with M

During the superaccessors phase, the following stub is generated
in M:

private <artifact> <superaccessor> def super$foo(t: Int)

Note that foo is a method that will later need to be specialized.
During the specialize phase, A.foo gets a special overload
A.foo$mcI$sp, which is a bridge to A.foo.

B extends A$mcI$sp (previously A[Int]), so B.foo gets a
special override B.foo$mcI$sp, which contains the implementation.
B.foo is overridden to become a bridge to B.foo$mcI$sp.

M extends B, so M.foo gets a special override M.foo$mcI$sp,
and M.foo itself is turned into a bridge to M.foo$mcI$sp, just
as was the case with B.
This is where the first problem arises - M.foo$mcI$sp does not
get an ABSOVERRIDE flag after being created. This commit fixes
that.

As mentioned earlier, M has a super accessor super$foo.
A super accessor (naturally) does not override anything, thus,
according to the standing specialization criteria it does not
need a special override (see needsSpecialOverride).
Its type does not contain any specialized type parameters, thus,
it is not eligible to obtain a special overload either.

So, our super$foo stays the way it is, subsequently being
renamed to M$$super$foo.
Later, during mixin, it is implemented in C as a forwarder
to B$class.foo (Not B.foo, because the implementation
classes are generated in mixin).

Lets see what we have now (I omit the $this parameter
for methods in implementation classes B$class and M$class):

    class B$class
      def foo(t: Int) =     ------------\ <-\
      def foo$mcI$sp(t: Int) =          |   |
                                        |   |
    trait M$class                       |   |
      def foo(t: Int) =   -------\      |   |
                                 |      |   |
      def foo$mcI$sp(t: Int) = <-/<-\   |   |
      {                             |   |   |
  /---- M$$super$foo(t)             |   |   |
  |     ...                         |   |   |
  |   }                             |   |   |
  |                                 |   |   |
  | object C                        |   |   |
  |   def foo$mcI$sp(t: Int) = -----/ <-/   |
  \-> def M$$super$foo(t: Int) =            |
      {                                     |
        ------------------------------------/

Now, call C.foo. This call is rewritten to
C.foo$mcI$sp, which is a bridge to
M$class.foo$mcI$sp.
Follow the lines on the diagram to enter
an endless loop. Boom! Stack overflow.

The culprit is the super accessor C.M$$super$foo, which
should have forwarded to the special override
B$class.foo$mcI$sp, instead of to B$class.foo.

So, we have 2 options:

  1. Make C.M$$super$foo forward to the proper special
    override, where the implementation actually is.
  2. During specialization, create a specialized version
    of M$$super$foo called M$$super$foo$mcI$sp, and set
    its alias to the special overload foo$mcI$sp. Later,
    during mixin, this super accessor will be appropriately
    resolved in concrete classes.

Option 1) involves cluttering mixin with specialization
logic.
Furthermore, the specialization already does create
specialized versions of super accessors when the super
accessor type contains specialized type parameters
(in other words, it generates a special overload).

Thus, 2) seems an ideal solution. We cannot deduct
if a super accessor should get a special override
directly, but we can see if its alias needs a
special override. If it does, then we generate
a special override for the super accessor.

This commit introduces the following changes:

  1. The ABSOVERRIDE flag is now retained, as mentioned
    earlier.
  2. A super accessor gets a special override if its
    alias needs a special override.
  3. The super calls in the methods bodies are rewritten
    to their specialized variants if they exist.
  4. Newly generated special overrides and special overloads
    are now entered into the declaration list of the owner
    during specialization.
    Strangely, this was not done before, but it is necessary for
    mixin to detect the generated special overload/override
    which is an alias for the super accessor.

Review by @dragos, @VladUreche.

Btw, this made me discover https://issues.scala-lang.org/browse/SI-6265,
which is not a regression caused by this commit.

This bug is a result of a subtle interplay of the stackable modifications
mechanism and specialization.

Prior to this commit, using `abstract override` with specialization was
broken in the sense that specialization did not create a specialized
version of the super accessor. Observe the following code:

    trait A[@specialized(Int) T] {
      def foo(t: T)
    }

    trait B extends A[Int] {
      def foo(t: Int) {
        println("B.foo")
      }
    }

    trait M extends B {
      abstract override def foo(t: Int) {
        super.foo(t)
        println("M.foo")
      }
    }

    object C extends B with M

During the `superaccessors` phase, the following stub is generated
in `M`:

    private <artifact> <superaccessor> def super$foo(t: Int)

Note that `foo` is a method that will later need to be specialized.
During the `specialize` phase, `A.foo` gets a *special overload*
`A.foo$mcI$sp`, which is a bridge to `A.foo`.

`B` extends `A$mcI$sp` (previously `A[Int]`), so `B.foo` gets a
*special override* `B.foo$mcI$sp`, which contains the implementation.
`B.foo` is overridden to become a bridge to `B.foo$mcI$sp`.

`M` extends `B`, so `M.foo` gets a special override `M.foo$mcI$sp`,
and `M.foo` itself is turned into a bridge to `M.foo$mcI$sp`, just
as was the case with `B`.
This is where the first problem arises - `M.foo$mcI$sp` does not
get an `ABSOVERRIDE` flag after being created. This commit fixes
that.

As mentioned earlier, `M` has a super accessor `super$foo`.
A super accessor (naturally) does not override anything, thus,
according to the standing specialization criteria it does not
need a special override (see `needsSpecialOverride`).
Its type does not contain any specialized type parameters, thus,
it is not eligible to obtain a special overload.

So, our `super$foo` stays the way it is, subsequently being
renamed to `M$$super$foo`.
Later, during `mixin`, it is implemented in `C` as a forwarder
to `B$class.foo` (Not `B.foo`, because the implementation
classes are generated in `mixin`).

Lets see what we have now (I omit the `$this` parameter
for methods in implementation classes `B$class` and `M$class`):

    class B$class
      def foo(t: Int) =     ------------\ <-\
      def foo$mcI$sp(t: Int) =          |   |
                                        |   |
    trait M$class                       |   |
      def foo(t: Int) =   -------\      |   |
                                 |      |   |
      def foo$mcI$sp(t: Int) = <-/<-\   |   |
      {                             |   |   |
  /---- M$$super$foo(t)             |   |   |
  |     ...                         |   |   |
  |   }                             |   |   |
  |                                 |   |   |
  | object C                        |   |   |
  |   def foo$mcI$sp(t: Int) = -----/ <-/   |
  \-> def M$$super$foo(t: Int) =            |
      {                                     |
        ------------------------------------/

Now, call `C.foo`. This call is rewritten to
`C.foo$mcI$sp`, which is a bridge to
`M$class.foo$mcI$sp`.
Follow the lines on the diagram to enter
an endless loop. Boom! Stack overflow.

The culprit is the super accessor `C.M$$super$foo`, which
should have forwarded to the special override
`B$class.foo$mcI$sp`, instead of to `B$class.foo`.

So, we have 2 options:
1) Make `C.M$$super$foo` forward to the proper special
override, where the implementation actually is.
2) During specialization, create a specialized version
of `M$$super$foo` called `M$$super$foo$mcI$sp`, and set
its alias to the special overload `foo$mcI$sp`. Later,
during `mixin`, this super accessor will be appropriately
resolved in concrete classes.

Option 1) involves cluttering `mixin` with specialization
logic.
Furthermore, the specialization already does create
specialized versions of super accessors when the super
accessor type contains specialized type parameters
(in other words, it generates a special overload).

Thus, 2) seems an ideal solution. We cannot deduct
if a super accessor should get a special override
directly, but we can see if its alias needs a
special override. If it does, then we generate
a special override for the super accessor.

This commit introduces the following changes:

1) The `ABSOVERRIDE` flag is now retained, as mentioned
earlier.
2) A super accessor gets a special override if its
alias needs a special override.
3) The super calls in the methods bodies are rewritten
to their specialized variants if they exist.
4) Newly generated special overrides and special overloads
are now entered into the declaration list of the owner
during specialization.
Strangely, this was not done before, but it is necessary for
`mixin` to detect the generated special overload/override
which is an alias for the super accessor.
@scala-jenkins
Copy link

@scala-jenkins
Copy link

@scala-jenkins
Copy link

@scala-jenkins
Copy link

@VladUreche
Copy link
Contributor

LGTM! Thanks a lot for the work you put into this Alex, I wish all bugs were solved in such elegant ways. And also documented so well!

One quick question: Can we remove the original superaccessor? If I understand correctly, M$$super$foo should not be called at all.

@axel22
Copy link
Contributor Author

axel22 commented Aug 22, 2012

In the case where a superaccessor gets a special overload - no, because the original superaccessor may still be called there.
In the case where a superaccessor gets a special override (like here) - maybe we could, but I wouldn't do it, since its overhead is too small compared to the additional logic needed to remove it.

@VladUreche
Copy link
Contributor

Agreed. Merging it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants