Skip to content
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

Fix #3339: Widen access of private classes accessed from inner packages #3392

Closed
wants to merge 1 commit into from

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Oct 26, 2017

Fixes #3339.

@odersky odersky requested a review from smarter October 26, 2017 14:19

object Test extends App {
outer.inner.Test.main(Array())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be in tests/run?

@retronym
Copy link
Member

retronym commented Oct 26, 2017

This fix appears to assume joint compilation. Scalac emits the class as public regardless of a reference in the currently compilation batch.

scala> :paste -raw
// Entering paste mode (ctrl-D to finish)

package p1 { private class C }

// Exiting paste mode, now interpreting.

scala> :javap -public p1.C
Compiled from "<pastie>"
public class p1.C {
  public p1.C();
}

The impedence mismatch between Scala and the JVM-s notion of packages is really frustrating. From a security perspective, it would be great to be able to emit non-public classes. I understand the motivation (uniformity of nested modules and packages), but still, I wonder if the cost is worth it.

@smarter
Copy link
Member

smarter commented Oct 26, 2017

Yes, things get weird when we split the original test case in two files:
A.scala:

package outer {
  private class A { override def toString = "A" }
}

B.scala:

package outer {
  package inner {
    object Test {
      def main(args: Array[String]): Unit = {
        println(new A)
      }
    }
  }
}
% dotc A.scala B.scala
% dotc A.scala
% dotc B.scala
-- [E006] Unbound Identifier Error: B.scala:5:20 ---------------------
5 |        println(new A)
  |                    ^
  |                    not found: type A

I think A should not be seen from B.scala even under join compilation. That way we can still emit the class as private in the common case where we don' t have nested packages accessing private classes.

By the way, hopefully all of the mess relating to making things accessible will get easier once the JDK implements nestmates: http://openjdk.java.net/jeps/181

@retronym
Copy link
Member

Nestmates will likely help us for companions, no widening will be needed for:

object O { private def foo(o: O) = o.bar; class B { private def bar = 42; def baz = O.foo(this) }

And for nested classes (as is the intended use case in Java). Basically, it helps in intra-compilation unit cases as alternative to publicising or using accessibilty bridges.

But it won't help anything that admits separate compilation.

I think I'd prefer that the language just followed the JVM convention for packages. I'd be forced to declare a few more things public, but would have the confidence that the things that were non-public stayed that way. Classes that are really intended to be implementation details but need to be shared between a group of packages could be added to an foo.internal subpackage which would not be exported from the JPMS module.

@odersky
Copy link
Contributor Author

odersky commented Nov 11, 2017

@retronym I think agree with you. It's probably better to change the rules so that private classes cannot be accessed from inner packages.

@allanrenucci
Copy link
Contributor

Closing this until we figure out what we want to do with #3339

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