Skip to content

Fix ParamForwarder under non-separate compilation #2116

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 17, 2017

inheritedAccessor looks for a method in a superclass whose name
matches the param accessor name, but that method is created by
ParamForwarder itself, so this only succeeded under separate
compilation (as demonstrated by the existing paramForwarding_separate
test). We fix this by simply running inheritedAccessor at a phase
where ParamForwarder has already been run.

smarter added 2 commits March 17, 2017 15:07
The test intent is to check that ParamForwarder optimises away the
fields in SubA and B, but even if ParamForwarder fails to do that,
Constructors will do it if the accessor is only used in the constructor,
adding a method that calls the accessor is needed to make sure this does
not happen.
`inheritedAccessor` looks for a method in a superclass whose name
matches the param accessor name, but that method is created by
ParamForwarder itself, so this only succeeded under separate
compilation (as demonstrated by the existing `paramForwarding_separate`
test). We fix this by simply running `inheritedAccessor` at a phase
where `ParamForwarder` has already been run.
@smarter smarter requested a review from DarkDimius March 17, 2017 14:13
class B(member: Int) extends SubA(member) {
def getBMember = member
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not yet convinced this fixes it. Can you also add test cases where A, SubA, and B appear in different orders? Like: B, SubA, A, for instance?

Copy link
Member Author

@smarter smarter Mar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean, order of declaration in the file?

@smarter
Copy link
Member Author

smarter commented Mar 17, 2017

@odersky Ah, you're right, changing the order of declaration breaks things, I guess that makes sense since ParamForwarder is not a mini-phase.

We cannot rely on ParamForwarder having been run on the superclass even
if we're in the correct phase since ParamForwarder is not a mini-phase.
Instead, we should accept a candidate even if it's not a method.

This fixes `tests/run/paramForwarding_together_b.scala`, this also required
updating `tests/run/paramForwarding.scala` since the field in `Y` is not
actually required.
@smarter smarter force-pushed the fix/param-forwarder-nonsep branch from ffe59ef to 18cc128 Compare March 17, 2017 15:33
@smarter
Copy link
Member Author

smarter commented Mar 17, 2017

We're getting pickling differences like this:

--- compiler/before-pickling.txt    2017-03-17 17:42:45.365222146 +0100
+++ compiler/after-pickling.txt 2017-03-17 17:42:45.369222105 +0100
@@ -5985,7 +5985,7 @@
             private[this] def statement: dotty.tools.dotc.ast.untpd.ValDef = 
               <
                 <
-                  <<super:Request.this.ValOrPatHandler(ValHandler.super)>@<27630..27630>.statement:
+                  <<super:Request.this.ValOrPatHandler(ValHandler.super)>@<27630..27630>.statement:=> 
                     dotty.tools.dotc.ast.untpd.Tree
                   >@<27630..27630>
                 .asInstanceOf:([X0] => X0)>@<27630..27630>

I suspect that what's happening is that we're selecting the private[this] def statement in the superclass ValOrPatHandler instead of the val statement in the super-superclass StatementHandler. This is because the fix in #2106 intentionally allows selecting a private member when loading from a new run. @odersky How would you fix this? I think that using shadowed names in ParamForwarder would be cleaner than adding yet more custom logic when loading denotations.

@smarter
Copy link
Member Author

smarter commented Mar 20, 2017

Superceded by #2124

@smarter smarter closed this Mar 20, 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.

2 participants