Scala 2.13: Fix weaker access privileges compile error

[rtyley]

Compiling Frontend under Scala 2.13, we see two identical compilation errors against the RssAtomModule & GModule traits, which both extend the com.sun.syndication.feed.module.Module interface from ROME (used for processing RSS). The error is this:

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 Module 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:

• scalac allows overriding protected member with weaker privileges 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 implementation that throws a 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]       ^

[ioannakok]

Nice! 👏

[AshCorr]

Awesome write up, and a great fix!

[rtyley]

Thanks!

[prout-bot]

Seen on PROD (merged by @rtyley 9 hours, 4 minutes and 35 seconds ago)

• Check your changes on www.theguardian.com ✔️
• Keep an eye on the deploy dashboard 📉