Add Missing parents to companions classes in stdlib-bootsrapped #17978

nicolasstucki · 2023-06-15T08:09:33Z

No description provided.

sjrd · 2023-06-19T08:49:30Z

compiler/src/dotty/tools/dotc/transform/PostTyper.scala

+        && sym.companionClass.derivesFrom(defn.SerializableClass)
+      then
+        // Add Serializable to companion objects of serializable classes
+        sym.info = sym.info match


Setting sym.info like this does not seem right to me. Normally changes to the info of symbols must be done in an InfoTransform.

I removed the info update. It should be fine not to patch those infos while compiling the stdlib as we do not really use those parents. We also only pickle the tree, which means that the unpickled info will have the correct info.

Er, that seems worse to me. Now the symbol table and the trees are not consistent with each other. That's something that should turn up in Ycheck.

Now I added an InfoTransformer to PostTyper.

sjrd · 2023-06-19T08:52:15Z

project/TastyMiMaFilters.scala

+    ProblemMatcher.make(ProblemKind.MissingParent, "scala.collection.Searching.Found$"),
+    ProblemMatcher.make(ProblemKind.MissingParent, "scala.collection.Searching.InsertionPoint$"),
+    ProblemMatcher.make(ProblemKind.MissingParent, "scala.jdk.FunctionWrappers.AsJava*$"),
+    ProblemMatcher.make(ProblemKind.MissingParent, "scala.jdk.FunctionWrappers.FromJava*$"),


Looks like these come from the fact that their companion class is a case class and therefore automatically serializable as well. Perhaps that means that Serializable is added to case classes later? If yes, it would probably make sense to make the objects Serializable in the same phase.

Those where actually missing AbstractFunction1. I miscategorized them back when I could not see which parents where missing in the TASTy-MiMa error message.

compiler/src/dotty/tools/dotc/transform/PostTyper.scala

sjrd

LGTM, but perhaps double-check on Monday at the dotty meeting whether it is acceptable to make PostTyper an InfoTransformer.

odersky · 2023-06-23T15:49:10Z

compiler/src/dotty/tools/dotc/transform/PostTyper.scala

  }
+
+  protected override def infoMayChange(sym: Symbol)(using Context): Boolean =


I suggest factoring that out ina @ThreadUnsafe lazy val

@threadunsafe lazy val compiledStdLib = ctx.settings.Yscala2Stdlib.value

…pped

odersky · 2023-06-26T17:14:34Z

compiler/src/dotty/tools/dotc/ast/Desugar.scala

@@ -440,6 +441,8 @@ object desugar {

  /** The expansion of a class definition. See inline comments for what is involved */
  def classDef(cdef: TypeDef)(using Context): Tree = {
+    @threadUnsafe lazy val compilingScala2StdLib = ctx.settings.Yscala2Stdlib.value


I don't think that will gain you anything to make it a lazy val, since that also adds inefficiency. If you want maximal performance, I'd do this:

val caseClassInScala2StdLib = isClaseClass && ctx.settings.Yscala2Stdlib.value

after isCaseCass is defined. But it's probably overkill, and we can just test the settings repeatedly.

odersky · 2023-06-26T17:15:31Z

compiler/src/dotty/tools/dotc/transform/PostTyper.scala

-  class PostTyperTransformer extends Transformer {
+  class PostTyperTransformer(@constructorOnly ictx: Context) extends Transformer {
+
+    @threadUnsafe private val compilingScala2StdLib = ictx.settings.Yscala2Stdlib.value(using ictx)


no @threadUnsafe needed here

Why not move the val outside PostTyperTransformer? Then you don't need to pass a context.

I did not have access to any context outside of PostTyperTransformer. I could have moved it into newTransformer. But now there is a better solution.

odersky · 2023-06-26T17:19:44Z

compiler/src/dotty/tools/dotc/transform/PostTyper.scala

+      if !sym.derivesFrom(defn.SerializableClass)
+        && sym.companionClass.derivesFrom(defn.SerializableClass)
+      then
+        info.derivedClassInfo(declaredParents = info.parents :+ defn.SerializableType)


Question: Should we do that as well in Scala 3? (during desugar or Typer). I mean, if a class is Serializable should we automatically make the companion Serializable as well?

Not sure. We should check if this change was intentional. @sjrd @smarter do you have more details about this?

Every object defined in Scala 3 extends Serializable in the bytecode, even if the user didn't ask for it: https://github.com/lampepfl/dotty/blob/76505f8017db9bd3409947869988ff352e17a40a/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala#L251-L269

I didn't realize Scala 2 had special logic for making the companion extend Serializable in Typer if the class itself does, I guess we could do the same for better source compatibility but otherwise it wouldn't matter.

odersky · 2023-06-26T17:25:53Z

compiler/src/dotty/tools/dotc/transform/PostTyper.scala

  }
+
+  protected override def infoMayChange(sym: Symbol)(using Context): Boolean =
+    ctx.settings.Yscala2Stdlib.value && sym.isAllOf(ModuleClass, butNot = Package)


Here you do need the cache since infoMayChange is much hotter than the other code where you check for compilingScala2StdLib. But moving compilingScala2StdLib to be a field of PostTyper is sufficient. The problem is that PostTyper does not take a context. So you probably need something like this:

private var myCompilingScala2StdLib: Boolean = false private var compilingScala2StdLibKnown = false def compilingScala2StdLib(using Context): Boolean = if !compilingScala2StdLibKnown then myCompilingScala2StdLib = ctx.settings.Yscala2Stdlib.value compilingScala2StdLibKnown = true myCompilingScala2StdLib

That did not work. The issue is that infoMayChange may be called with contexts that have different values of ctx.settings.Yscala2Stdlib.value. The instance of PostTyper is reused across compilation runs.

I found a simpler and more performant solution using

private var compilingScala2StdLib = false override def initContext(ctx: FreshContext): Unit = compilingScala2StdLib = ctx.settings.Yscala2Stdlib.value(using ctx)

The tricky part was to find when a context could have a different value of ctx.settings.Yscala2Stdlib.value. The initContext seems to be called once before we run the compilation pipeline runPhases. The setting should be known at that time and it seems to be the option that changes this field the minimum amount of times.

Ah, I forgot about initContext! Agreed, this is the best solution.

nicolasstucki self-assigned this Jun 15, 2023

nicolasstucki force-pushed the add-serializable-to-objects-in-stdlib-bootsrapped branch 4 times, most recently from 4c2b322 to 18d9e48 Compare June 15, 2023 13:03

nicolasstucki changed the title ~~Add Serialize to companions of serializable classes in stdlib-bootsrapped~~ Add Missing parents to companions classes in stdlib-bootsrapped Jun 15, 2023

nicolasstucki force-pushed the add-serializable-to-objects-in-stdlib-bootsrapped branch 2 times, most recently from 18d9e48 to 4d7789d Compare June 16, 2023 07:44

nicolasstucki marked this pull request as ready for review June 19, 2023 07:51

nicolasstucki requested a review from sjrd June 19, 2023 07:53

nicolasstucki assigned sjrd and unassigned nicolasstucki Jun 19, 2023

sjrd requested changes Jun 19, 2023

View reviewed changes

nicolasstucki force-pushed the add-serializable-to-objects-in-stdlib-bootsrapped branch 2 times, most recently from 9584c5e to a880f0b Compare June 20, 2023 13:38

sjrd requested changes Jun 23, 2023

View reviewed changes

compiler/src/dotty/tools/dotc/transform/PostTyper.scala Outdated Show resolved Hide resolved

compiler/src/dotty/tools/dotc/transform/PostTyper.scala Outdated Show resolved Hide resolved

nicolasstucki force-pushed the add-serializable-to-objects-in-stdlib-bootsrapped branch from a880f0b to 94ff3f2 Compare June 23, 2023 13:45

sjrd approved these changes Jun 23, 2023

View reviewed changes

nicolasstucki self-assigned this Jun 23, 2023

odersky reviewed Jun 23, 2023

View reviewed changes

nicolasstucki force-pushed the add-serializable-to-objects-in-stdlib-bootsrapped branch from 1e81135 to 5ffa8ff Compare June 26, 2023 12:57

Add Serialize to companions of serializable classes in stdlib-bootsra…

2cd66da

…pped

nicolasstucki force-pushed the add-serializable-to-objects-in-stdlib-bootsrapped branch from 5ffa8ff to cc749e6 Compare June 26, 2023 13:00

nicolasstucki assigned odersky and unassigned sjrd and nicolasstucki Jun 26, 2023

nicolasstucki requested a review from odersky June 26, 2023 14:52

odersky requested changes Jun 26, 2023

View reviewed changes

odersky assigned nicolasstucki and unassigned odersky Jun 27, 2023

nicolasstucki force-pushed the add-serializable-to-objects-in-stdlib-bootsrapped branch 2 times, most recently from 2163be7 to 8b9d06f Compare June 28, 2023 08:32

Cache ctx.settings.Yscala2Stdlib.value

5079be7

nicolasstucki force-pushed the add-serializable-to-objects-in-stdlib-bootsrapped branch from 8b9d06f to 5079be7 Compare June 28, 2023 08:36

nicolasstucki assigned odersky and unassigned nicolasstucki Jun 28, 2023

odersky approved these changes Jun 30, 2023

View reviewed changes

odersky assigned nicolasstucki and unassigned odersky Jun 30, 2023

nicolasstucki merged commit 9e06db7 into scala:main Jun 30, 2023

nicolasstucki deleted the add-serializable-to-objects-in-stdlib-bootsrapped branch June 30, 2023 09:56

		}

		protected override def infoMayChange(sym: Symbol)(using Context): Boolean =

Add Missing parents to companions classes in stdlib-bootsrapped #17978

Add Missing parents to companions classes in stdlib-bootsrapped #17978

Uh oh!

Conversation

nicolasstucki commented Jun 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sjrd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!