Skip to content

Stricter override checking for protected Scala members which override Java members#9525

Merged
lrytz merged 2 commits intoscala:2.13.xfrom
kynthus:2.13.x
Mar 11, 2021
Merged

Stricter override checking for protected Scala members which override Java members#9525
lrytz merged 2 commits intoscala:2.13.xfrom
kynthus:2.13.x

Conversation

@kynthus
Copy link
Contributor

@kynthus kynthus commented Feb 26, 2021 •
edited by lrytz
Loading

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.

@scala-jenkins scala-jenkins added this to the 2.13.6 milestone Feb 26, 2021
lrytz
lrytz reviewed Mar 5, 2021
View reviewed changes
Copy link
Member

@lrytz lrytz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

kynthus commented Mar 6, 2021

@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.

@SethTisue SethTisue self-requested a review March 9, 2021 18:33
@lrytz
Copy link
Member

lrytz commented Mar 10, 2021

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.

@kynthus kynthus mentioned this pull request Mar 10, 2021
Closed
@lrytz lrytz merged commit 093dec2 into scala:2.13.x Mar 11, 2021
@SethTisue
Copy link
Member

SethTisue commented Mar 13, 2021 •
edited
Loading

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).

SethTisue added a commit to SethTisue/scalafx that referenced this pull request Mar 13, 2021
@SethTisue
future-proof for Scala 2.13.6
7628ff7 
scala/scala#9525, which has been merged for inclusion in Scala 2.13.6,
makes it illegal to override a public method and specify a more
restrictive access level. (It should have been illegal all along.)

This came up in the Scala community build.
@SethTisue SethTisue mentioned this pull request Mar 13, 2021
Merged
SethTisue added a commit to scalacommunitybuild/blaze that referenced this pull request Mar 13, 2021
@SethTisue
adapt to scala/scala#9525
12a0e80
@lrytz
Copy link
Member

lrytz commented Mar 15, 2021

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

@SethTisue
Copy link
Member

SethTisue commented Mar 15, 2021 •
edited
Loading

(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.)

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Mar 15, 2021
@dwijnand dwijnand changed the title Allow only protected when overriding Java methods(Correct the test) Allow only protected when overriding Java methods Mar 29, 2021
@SethTisue SethTisue mentioned this pull request Mar 30, 2021
Merged
@lrytz lrytz mentioned this pull request May 6, 2021
Merged
@lrytz lrytz changed the title Allow only protected when overriding Java methods Stricter override checking when overriding protected Java members May 17, 2021
@lrytz lrytz changed the title Stricter override checking when overriding protected Java members Stricter override checking for protected Scala members which override Java members May 17, 2021
@lrytz lrytz mentioned this pull request Sep 8, 2021
Closed
@SethTisue SethTisue mentioned this pull request Sep 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lrytz lrytz lrytz left review comments

@dwijnand dwijnand dwijnand approved these changes

@SethTisue SethTisue Awaiting requested review from SethTisue

Assignees

No one assigned

Labels

release-notes worth highlighting in next release notes

Projects

None yet

Milestone

2.13.6

Development

Successfully merging this pull request may close these issues.

5 participants

@kynthus @lrytz @SethTisue @dwijnand @scala-jenkins