Skip to content

Fix #2099: Make param forwarders override the param they forward to #2102

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

Closed

Conversation

smarter
Copy link
Member

@smarter smarter commented Mar 14, 2017

Previously, in the following code:

class A(val member: Int)
class SubA(member: Int) extends A(member)

We created a forwarder in SubA like this:

  private[this] def member: Int = super.member

Since a private method cannot have the same name as an inherited
non-private one, we subsequently name-mangled the forwarder in
ExpandPrivate. However, this was only done in the TreeTransformer, not
in the DenotTransformer, this lead to separate compilation issues (see
added testcase).

Since we cannot detect that a ParamAccessor is a forwarder in a
DenotTransformer, we cannot fix the problem locally in ExpandPrivate.
Instead, we change ParamForwarding so that the forwarder now looks like:

  override def member: Int = super.member

To make this work we restrict ParamForwarding to only create forwarders
for non-final fields.

Previously, in the following code:

class A(val member: Int)
class SubA(member: Int) extends A(member)

We created a forwarder in SubA like this:
  private[this] def member: Int = super.member
Since a private method cannot have the same name as an inherited
non-private one, we subsequently name-mangled the forwarder in
ExpandPrivate. However, this was only done in the TreeTransformer, not
in the DenotTransformer, this lead to separate compilation issues (see
added testcase).

Since we cannot detect that a ParamAccessor is a forwarder in a
DenotTransformer, we cannot fix the problem locally in ExpandPrivate.
Instead, we change ParamForwarding so that the forwarder now looks like:

  override def member: Int = super.member

To make this work we restrict ParamForwarding to only create forwarders
for non-final fields.
@smarter smarter requested a review from odersky March 14, 2017 15:05
@smarter
Copy link
Member Author

smarter commented Mar 14, 2017

Hah! I am smitten by my own diabolical tests. Here's a minimization:

class A(val theValue: Int)
class NonVal(theValue: Int) extends A(theValue) {
  def getTheValueInNonVal = theValue
}
class X(override val theValue: Int) extends NonVal(0)


object Test {
  def main(args: Array[String]): Unit = {
    val x14 = new X(14)

    assert(x14.getTheValueInNonVal == 0)
  }
}

In both scalac and dotty before this PR, the assertion succeeds. But it now fails because calling theValue in NonVal#getTheValueInNonVal is now a virtual call that is overridden in X. @odersky : What do you think of this change?

@odersky
Copy link
Contributor

odersky commented Mar 14, 2017

Yes, I guess 0 is right.

@smarter
Copy link
Member Author

smarter commented Mar 14, 2017

@odersky Then I don't think the proposed approach of making forwarders override the param they forward to can work.

@DarkDimius
Copy link
Contributor

@smarter, I've proposed an approach to name-mangle forwarders while keeping them public.

@smarter
Copy link
Member Author

smarter commented Mar 14, 2017

@DarkDimius This makes things more complicated because we look for an inherited accessor by name in ParamForwarding (see inheritedAccessor), if we name-mangle forwarders we need to look for two names: the name-mangled one and the regular one. It does avoid other problems so this might be the best solution we can have.

Making param forwarders override the param they forward like we did in
the previous commit changes semantics. Instead, we do the same thing
that ExpandPrivate previously did, but early in ParamForwarding, so
separate compilation still works.
@smarter smarter force-pushed the fix/param-forwarder-access branch from ec762e3 to 5f9d66b Compare March 14, 2017 19:50
@smarter
Copy link
Member Author

smarter commented Mar 14, 2017

@DarkDimius I give up, I've tried various things but I can't get .ensureNotPrivate to work correctly before pickling, too many things break. Could you give it a try?

@DarkDimius
Copy link
Contributor

@smarter, I'll have a look.

@DarkDimius DarkDimius self-assigned this Mar 14, 2017
@DarkDimius DarkDimius removed the request for review from odersky March 15, 2017 09:03
@smarter
Copy link
Member Author

smarter commented Mar 15, 2017

Closed in favor of #2106

@smarter smarter closed this Mar 15, 2017
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