Skip to content

Separate compilation issue because PrivateParamAccessor are only made non-private in TreeTransformer, not DenotTransformer #2099

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.

Sign up for GitHub

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

Jump to bottom
Closed
smarter opened this issue Mar 14, 2017 · 4 comments
Closed

Separate compilation issue because PrivateParamAccessor are only made non-private in TreeTransformer, not DenotTransformer #2099

smarter opened this issue Mar 14, 2017 · 4 comments
Labels
area:transform exp:advanced itype:bug prio:high

Comments

@smarter
Copy link
Member

smarter commented Mar 14, 2017

A.scala:

class A(val member: Int)

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

B.scala:

class B(member: Int) extends SubA(member)

If you try to compile these two files together everything works fine, but if you first compile A.scala and then B.scala you get:

Exception in thread "main" java.lang.AssertionError: assertion failed: private method member in class SubA in try/exp/A.scala accessed from method member in class B in try/exp/B.scala
        at scala.Predef$.assert(Predef.scala:165)
        at dotty.tools.dotc.transform.ExpandPrivate.ensurePrivateAccessible(ExpandPrivate.scala:84)

The issue is that ExpandPrivate only transforms PrivateParamAccesor in trees:

  override def transformDefDef(tree: DefDef)(implicit ctx: Context, info: TransformerInfo) = {
    val sym = tree.symbol
    tree.rhs match {
      case Apply(sel @ Select(_: Super, _), _)
      if sym.is(PrivateParamAccessor) && sel.symbol.is(ParamAccessor) && sym.name == sel.symbol.name =>
        sym.ensureNotPrivate.installAfter(thisTransform)
      case _ =>
        if (isVCPrivateParamAccessor(sym))
          sym.ensureNotPrivate.installAfter(thisTransform)
    }
    tree
  }

So when we compile A.scala and B.scala together, the denotation for SubA#member gets transformed as expected, but when we do separate compilation, we see SubA#member as private, so ExpandPrivate explodes.
Note: see the comment at the top of ExpandPrivate if you're wondering why private param forwarders need to be made non-private.

I'm unsure how to fix this since it seems that we need access to the tree to decide whether or not the param forwarder should be made non-private. Maybe we need an extra flag to signal that a param forwarder forwards to an inherited param accessor with the same name?

This issue is blocking the Super Bootstrap
@DarkDimius
Copy link
Contributor

A possible fix would be not make paramaccessors private, but name-mangle them instead while keeping them public.

@smarter
Copy link
Member Author

smarter commented Mar 14, 2017 •
edited
Loading

@DarkDimius I'm not sure I understand what you mean. This is what the ensureNotPrivate in transformDefDef does already. Do you mean that we could make paramaccessors never private, even if they don't forward to a same-named param accessor? I agree that this would fix the issue.

@odersky
Copy link
Contributor

odersky commented Mar 14, 2017

Yes, I think making them not private to start with is the way to go.

@smarter
Copy link
Member Author

smarter commented Mar 14, 2017

@odersky OK, should EnsureNotPrivate be changed to always make them public, or should this be done before?

smarter added a commit to dotty-staging/dotty that referenced this issue Mar 14, 2017
@smarter
Fix scala#2099: Make param forwarders override the param they forward to
11254b3 
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 added a commit to dotty-staging/dotty that referenced this issue Mar 14, 2017
@smarter
Alternative fix for scala#2099: Expand param forwarders early
ec762e3 
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 added a commit to dotty-staging/dotty that referenced this issue Mar 14, 2017
@smarter
Alternative fix for scala#2099: Expand param forwarders early
5f9d66b 
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 added a commit to dotty-staging/dotty that referenced this issue Mar 14, 2017
@smarter
Fix scala#2099: Make param forwarders override the param they forward to
6056b0e 
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 added a commit to dotty-staging/dotty that referenced this issue Mar 14, 2017
@smarter
Alternative fix for scala#2099: Expand param forwarders early
1b688a1 
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 added a commit to dotty-staging/dotty that referenced this issue Mar 15, 2017
@smarter
Fix scala#2099: avoid loading a private member when recomputing a Nam…
c3339ff 
…edType denot

ParamForwarding creates the following forwarder in B:
  private[this] def member: Int = super.member
Where the type for `super.member` is `TermRef(SubA, member)` and the
  symbol is the `val member` in `A`.
So far this is correct, but in later phases we might call `loadDenot` on
this `TermRef` which will end up calling `asMemberOf`, which before this
commit just did:
  prefix.member(name)
This is incorrect in our case because `SubA` also happens to have a
private `def member`, which means that our forwarder in B now forwards
to a private method in a superclass, this subsequently crashes in
`ExpandPrivate`.
(Note: in the bytecode, a private method cannot have the same name as an
overriden method, but this is already worked around in EnsurePrivate.)

The fix is simple: when we recompute a member, we should only look at
private members if the previous denotation was private.
smarter added a commit to dotty-staging/dotty that referenced this issue Mar 15, 2017
@smarter
Fix scala#2099: avoid loading a private member when recomputing a Nam…
9686651 
…edType denot

ParamForwarding creates the following forwarder in B:
  private[this] def member: Int = super.member
Where the type for `super.member` is `TermRef(SubA, member)` and the
  symbol is the `val member` in `A`.
So far this is correct, but in later phases we might call `loadDenot` on
this `TermRef` which will end up calling `asMemberOf`, which before this
commit just did:
  prefix.member(name)
This is incorrect in our case because `SubA` also happens to have a
private `def member`, which means that our forwarder in B now forwards
to a private method in a superclass, this subsequently crashes in
`ExpandPrivate`.
(Note: in the bytecode, a private method cannot have the same name as an
overriden method, but this is already worked around in EnsurePrivate.)

The fix is simple: when we recompute a member, we should only look at
private members if the previous denotation was private.
smarter added a commit to dotty-staging/dotty that referenced this issue Mar 15, 2017
@smarter
Fix scala#2099: avoid loading a private member when recomputing a Nam…
008d450 
…edType denot

ParamForwarding creates the following forwarder in B:
  private[this] def member: Int = super.member
Where the type for `super.member` is `TermRef(SubA, member)` and the
  symbol is the `val member` in `A`.
So far this is correct, but in later phases we might call `loadDenot` on
this `TermRef` which will end up calling `asMemberOf`, which before this
commit just did:
  prefix.member(name)
This is incorrect in our case because `SubA` also happens to have a
private `def member`, which means that our forwarder in B now forwards
to a private method in a superclass, this subsequently crashes in
`ExpandPrivate`.
(Note: in the bytecode, a private method cannot have the same name as an
overriden method, but this is already worked around in EnsurePrivate.)

The fix is simple: when we recompute a member, we should only look at
private members if the previous denotation was private.
smarter added a commit to dotty-staging/dotty that referenced this issue Mar 15, 2017
@smarter
Fix scala#2099: avoid loading a private member when recomputing a Nam…
11f43fb 
…edType denot

ParamForwarding creates the following forwarder in B:
  private[this] def member: Int = super.member
Where the type for `super.member` is `TermRef(SubA, member)` and the
  symbol is the `val member` in `A`.
So far this is correct, but in later phases we might call `loadDenot` on
this `TermRef` which will end up calling `asMemberOf`, which before this
commit just did:
  prefix.member(name)
This is incorrect in our case because `SubA` also happens to have a
private `def member`, which means that our forwarder in B now forwards
to a private method in a superclass, this subsequently crashes in
`ExpandPrivate`.
(Note: in the bytecode, a private method cannot have the same name as an
overriden method, but this is already worked around in EnsurePrivate.)

The fix is simple: when we recompute a member, we should only look at
private members if the previous denotation was private.
odersky added a commit that referenced this issue Mar 16, 2017
@odersky
Merge pull request #2106 from dotty-staging/fix/param-forwarder-acces…
2325863 
…s-take-2

Alternative fix for #2099: avoid loading a private member when recomputing a NamedType denot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area:transform exp:advanced itype:bug prio:high
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@smarter @odersky @DarkDimius