Stricter override checking for protected Scala members which override Java members

[kynthus]

Currently, a protected, protected[x] or private[x] Scala member is always allowed when overriding a public or package-protected (default access) Java member. (Overriding a protected Java member is already checked correctly).

Since scalac emits all protected, protected[x] and private[x] methods as public in bytecode, the generated classfiles are valid.

This PR tightens the checks, so that

• a public Java member can only be overridden by a public Scala member
• a package-protected Java member can only be overridden by a public, protected[p] or private[p] Scala member where p is the same package

Fixes part of scala/bug#12349.

[lrytz]

Thank you @kynthus, this looks very good! I split up the change in two commits, so that the it's easy to see what's changed in the test output.

Scala 3 seems to have the same issue. Are you interested in submitting a ticket? https://github.com/lampepfl/dotty/issues.

My only reservation is: does this break codebases that currently compile? The additional restrictions are correct, but the fact that our checks are too lax doesn't manifest at runtime, because the Scala compiler emits protected, protected[X] and private[X] as public in bytecode.

@SethTisue wdyt?

[kynthus]

@lrytz Thank you for your review and various reinforcements.

Scala 3 seems to have the same issue. Are you interested in submitting a ticket? https://github.com/lampepfl/dotty/issues.

Scala 3 doesn't seem to need this fix.

https://github.com/lampepfl/dotty/blob/086d1a852a1412fb21124b5afdb1022c6d278d8b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala#L371

However, the following cases are still alive in Scala 3.

No	Modifier(JavaSuper)	Modifier(ScalaSub)	Compile	Expected
2.	Java's protected	protected	○	no
2.	Java's protected	protected[Cls]	○	no
3.	Java's protected	protected[pkg]	○	no
4.	Java's protected	protected[this]	○	no

About the bottom one, protected[this] is a will be drop(or deprecate?) in Scala 3, I think that measures are needed only in Scala 2.
Other than that Since Java's protected itself is broken, it is necessary to consider whether these are worth fixing.
Personally, I think Scala 3 is likely to stay in the status quo.

[lrytz]

Scala 3 doesn't seem to need this fix.

Right, I missed something in my previous test. Scala 3 does the same as this PR.

@SethTisue @dwijnand I think the new restrictions can be added. They are all about overriding public or package-private Java members with protected, protected[something] or private[something] in Scala. This should be rare.

[SethTisue]

over at scala/community-build#1384 , this caused two community build failures:

https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/2579/artifact/logs/scalafx-build.log

[scalafx] [error] /home/jenkins/workspace/scala-2.13.x-jdk11-integrate-community-build/target-0.9.17/project-builds/scalafx-c0d60aed391946a6ad0b9f62c6523e9a0e231694/scalafx/src/test/scala/scalafx/scene/chart/AxisSpec.scala:48:19: weaker access privileges in overriding
[scalafx] [error] def getDisplayPosition(x$1: T): Double (defined in class Axis)
[scalafx] [error]   override should be public
[scalafx] [error]     protected def getDisplayPosition(value: T) = 0.0

https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/2579/artifact/logs/blaze-build.log

[blaze] [error] /home/jenkins/workspace/scala-2.13.x-jdk11-integrate-community-build/target-0.9.17/project-builds/blaze-215dab7f1d3aad93904660179f317868eb8608a2/http/src/main/scala/org/http4s/blaze/http/http1/server/Http1ServerParser.scala:37:26: weaker access privileges in overriding
[blaze] [error] def submitRequestLine(methodString: String, path: String, scheme: String, majorVersion: Int, minorVersion: Int): Boolean (defined in class Http1ServerParser)
[blaze] [error]   override should be public
[blaze] [error]   override protected def submitRequestLine(
[blaze] [error]                          ^

this seems acceptable (there are 246 projects in the 2.13 community build!)

I will submit the necessary changes to the upstream projects (and see if they object, though I don't see why they would).

[lrytz]

👍 thanks. The required change is binary compatible, protected in Scala is always public in bytecode.

[SethTisue]

(The changes to the upstream projects were only in one place each, and don't seem to contravene any design intent; I strongly suspect the protecteds were accidental.)