Skip to content

Expand private members if necessary #517

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

Merged
merged 11 commits into from
May 4, 2015

Conversation

odersky
Copy link
Contributor

@odersky odersky commented May 1, 2015

Private members that are accessed from out of class, or that are overridden by implementation classes get mangled names and their Private flag is reset. NotJavaPrivate is eliminated.

Review by @DarkDimius

odersky added 6 commits May 1, 2015 17:22
Verified that all tests still run. This is a first step, so that
we can later on eliminate NotJavaPrivate altogether.
Resets private flag, and expands the name if necessary.
Logic moved from RefChecks to Mixin; implementation
is now by name expansion instead of setting NotJavaPrivate
flag.
A late miniphase which resets private flag of all
members that are not accessed from within same class.
Replaces logic in RefChecks. Doing this late has two
advantages

 - we can use name expansion, because references are
   symbolic, so the names of symbols and references
   to them do not need to correspond anymore.
 - we can automatically correct for symbols moved in earlier
   phases (e.g. lifted out by LambdaLift).
It is now redundant.
@xeno-by
Copy link

xeno-by commented May 1, 2015

Would we be able to use the same mechanism to allow macro expansions to access private members visible at the macro definition site?

*
* private def m(ps) = e
*
* --> private static def($this: C, ps) = [this -> $this] e
Copy link
Member

Choose a reason for hiding this comment

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

This description was copy-pasted from PrivateToStatic and does not match what this phase does.

@odersky
Copy link
Contributor Author

odersky commented May 1, 2015

You mean, make give private members accessed by a non-private macro expanded names? Yes that could work.

@DarkDimius
Copy link
Contributor

@xeno-by I guess it would work only if macro expansion happens in same compilation unit where private member is defined.

@DarkDimius
Copy link
Contributor

or that are overridden by implementation classes

How does this play with separate compilation? If you have a implementation class, that overrides a private field but is separately compiled, will you try to retroactively mangle field defined in super class?

@DarkDimius
Copy link
Contributor

After following the code I do not see how it will trigger problems.

@DarkDimius
Copy link
Contributor

@odersky, there are other private methods that need to be expanded: private deferred methods in traits, such as super accessors. Please review last commit.

@DarkDimius
Copy link
Contributor

Last fix allows dotty to pass flag verification of JVM.

@DarkDimius
Copy link
Contributor

Ok, I'm preparing a fix.

@odersky
Copy link
Contributor Author

odersky commented May 3, 2015

It seems there's more wrong with superaccessors than this. You need to fix
it there. Patches later on won't help. I am fairly close to a solution, but
the fix uncovers further errors.

On Sun, May 3, 2015 at 7:58 PM, Dmitry Petrashko [email protected]
wrote:

Ok, I'm preparing a fix.


Reply to this email directly or view it on GitHub
#517 (comment).

Martin Odersky
EPFL

@DarkDimius
Copy link
Contributor

@odersky please review last 3 commits.

@odersky
Copy link
Contributor Author

odersky commented May 3, 2015

LGTM. I had the same as your first commit, but got errors that were solved by your third commit. I am still puzzled why the scalac SuperAccessors explicitly defines symbols that are both Deferred and Private. I am also puzzled where super accessors for non traits (which are now private, but not deferred) actually get defined. But we'll figure that out, eventually.

odersky added a commit that referenced this pull request May 4, 2015
@odersky odersky merged commit 4808d1d into scala:master May 4, 2015
@odersky
Copy link
Contributor Author

odersky commented May 4, 2015

I figured it out. resolveSuper does the right thing for private super accessors. I'll add a test.

@DarkDimius
Copy link
Contributor

@odersky, I have non-commited post-condition for super-accessors that checks that private super-accessors have non-empty rhs. I'll make a PR.

@allanrenucci allanrenucci deleted the add/expand-privates branch December 14, 2017 16:57
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.

4 participants