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

scalac allows overriding protected member with weaker privileges #3946

Closed
scabug opened this issue Oct 21, 2010 · 8 comments
Closed

scalac allows overriding protected member with weaker privileges #3946

scabug opened this issue Oct 21, 2010 · 8 comments
Assignees

Comments

@scabug
Copy link

scabug commented Oct 21, 2010

Given the the following files:

p/J.java

package p;
public class J {
    protected void m() { return; }
}

p/S.scala

package p
class S extends J {
  override protected def m { }
}

Compiling them together with scalac gives the correct error message:

lamppc11:bug1 luc$$ ../../target/pack/bin/scalac p/J.java p/S.scala
p/S.scala:3: error: overriding method m in class J of type ()Unit;
 method m has weaker access privileges; it should be at least protected[p]
  override protected def m { }
                         ^
one error found

However, when compiling first the Java file, then the Scala file there's no error message:

lamppc11:bug1 luc$$ javac p/J.java 
lamppc11:bug1 luc$$ ../../target/pack/bin/scalac p/S.scala 
lamppc11:bug1 luc$$
@scabug
Copy link
Author

scabug commented Oct 21, 2010

Imported From: https://issues.scala-lang.org/browse/SI-3946?orig=1
Reporter: @lrytz

@scabug
Copy link
Author

scabug commented Dec 1, 2010

@lrytz said:
fixed in r23627

@scabug
Copy link
Author

scabug commented Dec 1, 2010

@lrytz said:
that was the wrong one...

@scabug
Copy link
Author

scabug commented Dec 3, 2010

@lrytz said:
(In r23673) close #3946. protected in java means access boundary is the package. review by dragos.

@scabug
Copy link
Author

scabug commented Dec 7, 2010

@lrytz said:
(In r23704) follow up on fix #3946. relaxed access boundry check for overriding protected java member.

@scabug
Copy link
Author

scabug commented Dec 7, 2010

@lrytz said:
By Paul: I find experimentally that javac lets access tighten such that whether
you can call a method depends on the static type of the argument, which
would seem like an unfortunate thing to emulate, but possibly not the
worst thing being considered.

// J1.java
package bip;

public class J {
 // package[bip] + protected f()
 protected void f() { }
}
// J2.java
package notbip;

public class J2 extends bip.J {
 // this override "should" not be allowed, it excludes package bip
 // now package[notbip] + protected f()
 @Override
 protected void f() { }
}
// J3.java
package bip;

public class J3 {
 public void g(bip.J x) {
   x.f();  // ok
   notbip.J2 y = (notbip.J2)x;
   y.f();  // fails
   // J3.java:7: f() has protected access in notbip.J2
   //     y.f();
   //      ^
   // 1 error
 }
}

@scabug
Copy link
Author

scabug commented Dec 7, 2010

@lrytz said:
We discussed this in today's meeting. Martin was surprised how broken
"protected" in Java is (see Paul's example).

Summing up. Given the Java class:

package p;
public class A {
    protected void f() {}
}

This indeed means, in Scala terms, "f" is "protected[p]", i.e. we should set
the "privateWithin" flag to "p". Otherwise, the following Scala code would
not be legal:

package p {
  object T {
    val a = new A()
    a.f()
  }
}

This has been fixed in #3946, but with the unfortunate consequence on
overriding, which invalidates the following (previously valid) Scala code:

package q {
  class B extends p.A {
    override protected def f() { }
  }
}

Note that the same situation is much more unlikely to happen in
Scala-only code, because when one writes "class A" in Scala, writing
"protected def f", would mean "protected[A]", not "protected[p]".

We concluded in our discussion that

  • Most Scala programmers are not aware that "protected" in Java actually means accessible in the package ("protected[p]")
  • Changing override rules now would be too confusing, and also invalidate a non-acceptable amount of existing Scala code

The easiest solution seems to be a special case when checking the
accessibility of an override. If the overridden symbol is java and protected,
ignore the "privateWithin".

@scabug
Copy link
Author

scabug commented Dec 8, 2010

@SethTisue said:
fwiw, this notorious Scala/Java mixer agrees

@scabug scabug closed this as completed May 18, 2011
rtyley added a commit to guardian/frontend that referenced this issue Jul 5, 2022
Compiling under Scala 2.13, we see two identical compilation
errors for traits `RssAtomModule` & `GModule`, which both
extend the `com.sun.syndication.feed.module.Module` interface from
ROME (used for processing RSS). The message is of the form:

> weaker access privileges in overriding def clone(): Object (defined in trait Module)
> override should be public;
> (note that def clone(): Object (defined in trait Module) is abstract, and is therefore
> overridden by concrete protected[package lang] def clone(): Object (defined in class Object))

`com.sun.syndication.feed.module.Module` is a Java interface that specifies
`clone()` should be *public*:

```
public Object clone() throws CloneNotSupportedException;
```

...but it's just an interface, not a class - and so the `java.lang.Object.clone()` method,
which is assigned `protected` as an access modifier, is more protected than the interface
dictates - but it's the only `clone()` method available on our _trait_, whether that's
`RssAtomModule` or `GModule`.

This is a horrible little problem and various people have encountered it over the years:

* scala/bug#3946
* https://bugs.openjdk.org/browse/JDK-6946211

How to fix?
-----------

There is a fairly good suggestion at https://stackoverflow.com/a/8619704/438886 - just
define a concrete `clone()` method in the trait, with a guard implmentation that just
throws `CloneNotSupportedException` - your concrete subclasses can override the method
with whatever implementation they like. It's a bit of a hack, but it works.

However, looking at the code of these two traits and their solitary implementations:

* `RssAtomModule` with its implementation `RssAtomModuleImpl`
* `GModule` with `GModuleImpl`

...there's no reason to separate the functionality here into a separate trait and class.
It's not being used to facilitate testing, and each trait has only a single implementation.
Sometimes a trait can be a nice way to decide exactly what limited public API you want
people to think about when they use your code, but I don't think that's the case here.
Totally fine to just unite each trait & class into a simple class, reducing unnecessary
boilerplate, and removing the need to have a dummy `public` clone method!

The compilation errors in full:

```
[error] /Users/roberto/guardian/frontend/common/app/common/ShowcaseRSSModules.scala:16:7: weaker access privileges in overriding
[error] def clone(): Object (defined in trait Module)
[error]   override should be public;
[error]   (note that def clone(): Object (defined in trait Module) is abstract,
[error]   and is therefore overridden by concrete protected[package lang] def clone(): Object (defined in class Object))
[error] trait RssAtomModule extends com.sun.syndication.feed.module.Module with Serializable with Cloneable {
[error]       ^
[error] /Users/roberto/guardian/frontend/common/app/common/ShowcaseRSSModules.scala:52:7: weaker access privileges in overriding
[error] def clone(): Object (defined in trait Module)
[error]   override should be public;
[error]   (note that def clone(): Object (defined in trait Module) is abstract,
[error]   and is therefore overridden by concrete protected[package lang] def clone(): Object (defined in class Object))
[error] trait GModule extends com.sun.syndication.feed.module.Module with Serializable with Cloneable {
[error]       ^
```
rtyley added a commit to guardian/frontend that referenced this issue Jul 6, 2022
Compiling under Scala 2.13, we see two identical compilation
errors for traits `RssAtomModule` & `GModule`, which both
extend the `com.sun.syndication.feed.module.Module` interface from
ROME (used for processing RSS). The message is of the form:

> weaker access privileges in overriding def clone(): Object (defined in trait Module)
> override should be public;
> (note that def clone(): Object (defined in trait Module) is abstract, and is therefore
> overridden by concrete protected[package lang] def clone(): Object (defined in class Object))

`com.sun.syndication.feed.module.Module` is a Java interface that specifies
`clone()` should be *public*:

```
public Object clone() throws CloneNotSupportedException;
```

...but it's just an interface, not a class - and so the `java.lang.Object.clone()` method,
which is assigned `protected` as an access modifier, is more protected than the interface
dictates - but it's the only `clone()` method available on our _trait_, whether that's
`RssAtomModule` or `GModule`.

This is a horrible little problem and various people have encountered it over the years:

* scala/bug#3946
* https://bugs.openjdk.org/browse/JDK-6946211

How to fix?
-----------

There is a fairly good suggestion at https://stackoverflow.com/a/8619704/438886 - just
define a concrete `clone()` method in the trait, with a guard implmentation that just
throws `CloneNotSupportedException` - your concrete subclasses can override the method
with whatever implementation they like. It's a bit of a hack, but it works.

However, looking at the code of these two traits and their solitary implementations:

* `RssAtomModule` with its implementation `RssAtomModuleImpl`
* `GModule` with `GModuleImpl`

...there's no reason to separate the functionality here into a separate trait and class.
It's not being used to facilitate testing, and each trait has only a single implementation.
Sometimes a trait can be a nice way to decide exactly what limited public API you want
people to think about when they use your code, but I don't think that's the case here.
Totally fine to just unite each trait & class into a simple class, reducing unnecessary
boilerplate, and removing the need to have a dummy `public` clone method!

The compilation errors in full:

```
[error] /Users/roberto/guardian/frontend/common/app/common/ShowcaseRSSModules.scala:16:7: weaker access privileges in overriding
[error] def clone(): Object (defined in trait Module)
[error]   override should be public;
[error]   (note that def clone(): Object (defined in trait Module) is abstract,
[error]   and is therefore overridden by concrete protected[package lang] def clone(): Object (defined in class Object))
[error] trait RssAtomModule extends com.sun.syndication.feed.module.Module with Serializable with Cloneable {
[error]       ^
[error] /Users/roberto/guardian/frontend/common/app/common/ShowcaseRSSModules.scala:52:7: weaker access privileges in overriding
[error] def clone(): Object (defined in trait Module)
[error]   override should be public;
[error]   (note that def clone(): Object (defined in trait Module) is abstract,
[error]   and is therefore overridden by concrete protected[package lang] def clone(): Object (defined in class Object))
[error] trait GModule extends com.sun.syndication.feed.module.Module with Serializable with Cloneable {
[error]       ^
```
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

No branches or pull requests

2 participants