remove Serializable from types inferred across companions

[bjaglin]

Follows #984, which was not enough for the previous use-case.

On master:

[wartremover:Serializable] Inferred type containing Serializable: scalapb.GeneratedMessage{def companion: java.io.Serializable}
[error]         Seq(
[error]         ^

[bjaglin]

@thesamet I verified that this is solving my use-case (no wartremover Serializable error on an inferred type when constructing a Seq of GeneratedMessage in Scala 2.x), but I cannot come up with a simple test to prevent regressions.

I explored the idea of adding the wartremover compiler plugin to the runtime project, but I couldn't find a way to add it only for Test and I am concerned this slow downs further upgrades (as it needs to be published for any new Scala 2.x patch release). I concluded that it was better to leave this untested rather than adding complexity to the build since it's a fairly advanced use-case, and this won't be relevant in a Scala 3 world.

[thesamet]

The traits changes in this PR are used to extend objects (not case classes, and not case objects), so unlike GeneratedMessage (which applies to case classes), there's no Serializable implicitly added. Can you give an example of code that makes wartremover complain?

[bjaglin]

Can you give an example of code that makes wartremover complain?

Of course, I should have done that from the beginning.

bjaglin@x1:/tmp/demo$ grep -R '' *
build.sbt:libraryDependencies += "com.thesamet.scalapb" %% "scalapb-runtime" % "0.11.3"  
build.sbt:wartremoverErrors += Wart.Serializable
project/plugins.sbt:addSbtPlugin("org.wartremover" % "sbt-wartremover" % "2.4.15")
project/build.properties:sbt.version=1.5.3
src/main/scala/Serializable.scala:object Serializable {
src/main/scala/Serializable.scala:  val _ = Seq(com.google.protobuf.timestamp.Timestamp(), com.google.protobuf.duration.Duration())
src/main/scala/Serializable.scala:}
bjaglin@x1:/tmp/demo$ sbt compile
[info] welcome to sbt 1.5.3 (Azul Systems, Inc. Java 11.0.10)
[info] loading settings for project global-plugins from graph.sbt,update.sbt ...
[info] loading global plugins from /home/bjaglin/.sbt/1.0/plugins
[info] loading settings for project demo-build from plugins.sbt ...
[info] loading project definition from /tmp/demo/project
[info] loading settings for project demo from build.sbt ...
[info] set current project to demo (in build file:/tmp/demo/)
[info] Executing in batch mode. For better performance use sbt's shell
[info] compiling 1 Scala source to /tmp/demo/target/scala-2.12/classes ...
[error] /tmp/demo/src/main/scala/Serializable.scala:2:7: [wartremover:Serializable] Inferred type containing Serializable: Seq[scalapb.GeneratedMessage with scalapb.lenses.Updatable[_ >: com.google.protobuf.duration.Duration with com.google.protobuf.timestamp.Timestamp <: scalapb.GeneratedMessage with scalapb.lenses.Updatable[_ >: com.google.protobuf.duration.Duration with com.google.protobuf.timestamp.Timestamp <: scalapb.GeneratedMessage]{def companion: Serializable}]{def companion: Serializable with scalapb.GeneratedMessageCompanion[_ >: com.google.protobuf.duration.Duration with com.google.protobuf.timestamp.Timestamp <: scalapb.GeneratedMessage] with scalapb.JavaProtoSupport[_ >: com.google.protobuf.duration.Duration with com.google.protobuf.timestamp.Timestamp <: scalapb.GeneratedMessage, _ >: com.google.protobuf.Duration with com.google.protobuf.Timestamp <: com.google.protobuf.GeneratedMessageV3]}]
[error]   val _ = Seq(com.google.protobuf.timestamp.Timestamp(), com.google.protobuf.duration.Duration())
[error]       ^
[error] /tmp/demo/src/main/scala/Serializable.scala:2:11: [wartremover:Serializable] Inferred type containing Serializable: scalapb.GeneratedMessage with scalapb.lenses.Updatable[_ >: com.google.protobuf.duration.Duration with com.google.protobuf.timestamp.Timestamp <: scalapb.GeneratedMessage with scalapb.lenses.Updatable[_ >: com.google.protobuf.duration.Duration with com.google.protobuf.timestamp.Timestamp <: scalapb.GeneratedMessage]{def companion: Serializable}]{def companion: Serializable with scalapb.GeneratedMessageCompanion[_ >: com.google.protobuf.duration.Duration with com.google.protobuf.timestamp.Timestamp <: scalapb.GeneratedMessage] with scalapb.JavaProtoSupport[_ >: com.google.protobuf.duration.Duration with com.google.protobuf.timestamp.Timestamp <: scalapb.GeneratedMessage, _ >: com.google.protobuf.Duration with com.google.protobuf.Timestamp <: com.google.protobuf.GeneratedMessageV3]}
[error]   val _ = Seq(com.google.protobuf.timestamp.Timestamp(), com.google.protobuf.duration.Duration())
[error]           ^
[error] two errors found
[error] (Compile / compileIncremental) Compilation failed
[error] Total time: 4 s, completed Jun 1, 2021, 6:14:24 PM

bjaglin@x1:/tmp/demo$ grep -R '' *
build.sbt:libraryDependencies += "com.thesamet.scalapb" %% "scalapb-runtime" % "0.11.3+12-c6de4b06"  
build.sbt:wartremoverErrors += Wart.Serializable
project/plugins.sbt:addSbtPlugin("org.wartremover" % "sbt-wartremover" % "2.4.15")
project/build.properties:sbt.version=1.5.3
src/main/scala/Serializable.scala:object Serializable {
src/main/scala/Serializable.scala:  val _ = Seq(com.google.protobuf.timestamp.Timestamp(), com.google.protobuf.duration.Duration())
src/main/scala/Serializable.scala:}
bjaglin@x1:/tmp/demo$ sbt compile
[info] welcome to sbt 1.5.3 (Azul Systems, Inc. Java 11.0.10)
[info] loading settings for project global-plugins from graph.sbt,update.sbt ...
[info] loading global plugins from /home/bjaglin/.sbt/1.0/plugins
[info] loading settings for project demo-build from plugins.sbt ...
[info] loading project definition from /tmp/demo/project
[info] loading settings for project demo from build.sbt ...
[info] set current project to demo (in build file:/tmp/demo/)
[info] Executing in batch mode. For better performance use sbt's shell
[info] compiling 1 Scala source to /tmp/demo/target/scala-2.12/classes ...
[success] Total time: 8 s, completed Jun 1, 2021, 6:17:25 PM

[bjaglin]

The traits changes in this PR are used to extend objects (not case classes, and not case objects), so unlike GeneratedMessage (which applies to case classes), there's no Serializable implicitly added

I am not sure when it comes from, but it's definitely there:

bjaglin@x1:~/git/projects/ScalaPB$ javap -c ./scalapb-runtime/jvm/target/scala-2.12/classes/com/google/protobuf/duration/Duration\$.class | grep Serializable
public final class com.google.protobuf.duration.Duration$ implements scalapb.GeneratedMessageCompanion<com.google.protobuf.duration.Duration>, scalapb.HasBuilder<com.google.protobuf.duration.Duration>, scalapb.JavaProtoSupport<com.google.protobuf.duration.Duration, com.google.protobuf.Duration>, scalapb.DurationCompanionMethods, scala.Serializable {

[thesamet]

I think that in this case, since it is unexpected to have Serializable for companions is we should first track down where it's coming from. I suspect that adding Serializable when not needed can have performance implications: Spark attempts to serialize all members of an instance that is passed to workers. We get a runtime error if anything in the object graph is not Serializable and to suppress this error for constant references (such as type mappers and companions) we use @transient. By adding Serializable unnecessarily, we may end up not controlling what gets serialized and that could lead to wasted network bandwidth and reduced performance.

[bjaglin]

this does not seem to be used anymore (so the addition is useless), should it be deprecated?

[thesamet]

Nice catch. Looks like it has never been used. Let's mark it as deprecated.

[bjaglin]

we should first track down where it's coming from

I had a look with Scala 2.12: scala.Serializable is synthetically added for companion objects of case classes (GeneratedMessage companions) and companion objects of classes extending scala.Serializable (like GeneratedEnum). I didn't check the behavior on Scala 3, but I assume it's the same?

[thesamet]

scala.Serializable is synthetically added for companion objects of case classes (GeneratedMessage companions) and companion objects of classes extending scala.Serializable (like GeneratedEnum).

Huh, interesting! Thanks for investigating this. I didn't know scalac was doing that.