-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fixes SI-4996. #1173
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
Fixes SI-4996. #1173
Conversation
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.
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/256/ |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/963/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/963/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/256/ |
|
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, |
|
In the case where a superaccessor gets a special overload - no, because the original superaccessor may still be called there. |
|
Agreed. Merging it! |
This bug is a result of a subtle interplay of the stackable modifications
mechanism and specialization.
Prior to this commit, using
abstract overridewith specialization wasbroken in the sense that specialization did not create a specialized
version of the super accessor. Observe the following code:
During the
superaccessorsphase, the following stub is generatedin
M:Note that
foois a method that will later need to be specialized.During the
specializephase,A.foogets a special overloadA.foo$mcI$sp, which is a bridge toA.foo.BextendsA$mcI$sp(previouslyA[Int]), soB.foogets aspecial override
B.foo$mcI$sp, which contains the implementation.B.foois overridden to become a bridge toB.foo$mcI$sp.MextendsB, soM.foogets a special overrideM.foo$mcI$sp,and
M.fooitself is turned into a bridge toM.foo$mcI$sp, justas was the case with
B.This is where the first problem arises -
M.foo$mcI$spdoes notget an
ABSOVERRIDEflag after being created. This commit fixesthat.
As mentioned earlier,
Mhas a super accessorsuper$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$foostays the way it is, subsequently beingrenamed to
M$$super$foo.Later, during
mixin, it is implemented inCas a forwarderto
B$class.foo(NotB.foo, because the implementationclasses are generated in
mixin).Lets see what we have now (I omit the
$thisparameterfor methods in implementation classes
B$classandM$class):Now, call
C.foo. This call is rewritten toC.foo$mcI$sp, which is a bridge toM$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, whichshould have forwarded to the special override
B$class.foo$mcI$sp, instead of toB$class.foo.So, we have 2 options:
C.M$$super$fooforward to the proper specialoverride, where the implementation actually is.
of
M$$super$foocalledM$$super$foo$mcI$sp, and setits alias to the special overload
foo$mcI$sp. Later,during
mixin, this super accessor will be appropriatelyresolved in concrete classes.
Option 1) involves cluttering
mixinwith specializationlogic.
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:
ABSOVERRIDEflag is now retained, as mentionedearlier.
alias needs a special override.
to their specialized variants if they exist.
are now entered into the declaration list of the owner
during specialization.
Strangely, this was not done before, but it is necessary for
mixinto detect the generated special overload/overridewhich 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.