check private[this] members in override checking

[lrytz]

Previously, private[this] members were not considered in override checking, so a subclass could have a private[this] member x while also inheriting a member x, which leads to an IllegalAccessError at runtime

scala> class C { def f = 1 }; object D extends C { private[this] def f = 2; def g = f }
class C
object D

scala> D.f
java.lang.IllegalAccessError: tried to access method D$.f()I from class $iw

With this PR, the private[this] "overrides" are rejected ("error: weaker access privileges in overriding")

Fixes scala/bug#9334

[som-snytt]

OK, I agree that if there are matching members, the "bottom" member (and here one may need terminology from urban dictionary) cannot constrain the visibility. The spec says you inherit one member and the other is defined directly, which may have an arbitrary modifier. That is, on the "high" or "top" side, a private member should not mess with me. But from my perspective, I must understand and contend with any inherited members. private[this] does not exempt me from this responsibility.

[lrytz]

Reading through the spec, I cannot see anything that makes private different from private[this] in terms of inheritance.

https://www.scala-lang.org/files/archive/spec/2.13/05-classes-and-objects.html#private

A member is private if it is either class-private or object-private
Class-private or object-private members may not be abstract, and may not have protected or override modifiers. They are not inherited by subclasses and they may not override definitions in parent classes.

https://www.scala-lang.org/files/archive/spec/2.13/05-classes-and-objects.html#class-members

A member M of a class C that matches a non-private member M' of a base class of C is said to override that member. [...] Furthermore, the following restrictions on modifiers apply to M and M':

• M must not be private

So a private or private[this] definition M cannot override another member. I guess that doesn't say that M cannot exist, but it was ruled out for private since always, and again, I don't find anything that says private[this] should be handled differently.

[som-snytt]

Took me a minute to mentally verify the intuition and the implementation and then also try it out.

[SethTisue]

@lrytz I remember this came up at team meeting yesterday but I don't recall if any new conclusions were reached...?

[som-snytt]

I was trying to recall when visibility came to figure into inheritability; I thought Paul's thing was that you should be able to add a private or private[enough] member without worrying about conflicts. I remember he tweaked it without a SIP, but I didn't have time to time to consult the history. That's why I was skeptical; but I also think that in the face of brittle inheritance, you're not allowed to be ignorant of or isolated from your base classes or subclasses.

[lrytz]

not allowed to be ignorant of or isolated from your base classes or subclasses

private or private[this] methods don't restrict what you can define in subclasses, or..?

any new conclusions were reached...?

Not really, I think the next step is community build; we could merge this and revert if necessary.

[som-snytt]

OK, I was thinking of imports not inheritance. "Importable" changed from "not private[this]" to "accessible". I'm sure glad I cleared that up; I wouldn't have slept for a week.

[dwijnand]

we could merge this and revert if necessary.

OK, merged and let's see.

[SethTisue]

I'll run the community build soon (this week) and see if something squawks.

[SethTisue]

@lrytz the community build found the following needed changes; do they seem expected to you?

• adjust to 2.13.6 restriction on narrowing access when overriding sbt/io#317
• scalacommunitybuild/http4s@06a3720

[lrytz]

These look expected to me. Do you think the error message is good enough?

[SethTisue]

Do you think the error message is good enough?

Well... it's not great. stab at a better one: "an override cannot have more restricted access than the method it overrides; method foo is public in class C"

[som-snytt]

That's a good point. "Weaker access privilege" is what people are in the streets protesting about. As a security concept, privilege as in privileged code implies an authorization that can be acquired or not; but maybe we're too comfortable with setAccessible.

The other part of the message is that if the user did not write override then overriding is not intended. The new information for the user is that the member "matches" a member in a base class. "Oh, I didn't realize there is already a logger."

Is it too annoying:

m matches an existing public method m in class C but does not override it:
  m must not be private
  m is not marked override