Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 30, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2cd66da
Add Serialize to companions of serializable classes in stdlib-bootsra…
nicolasstucki Jun 15, 2023
5079be7
Cache ctx.settings.Yscala2Stdlib.value
nicolasstucki Jun 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions compiler/src/dotty/tools/dotc/ast/Desugar.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import printing.Formatting.hl
import config.Printers

import scala.annotation.internal.sharable
import scala.annotation.threadUnsafe

object desugar {
import untpd._
Expand Down Expand Up @@ -487,6 +488,7 @@ object desugar {
def isNonEnumCase = !isEnumCase && (isCaseClass || isCaseObject)
val isValueClass = parents.nonEmpty && isAnyVal(parents.head)
// This is not watertight, but `extends AnyVal` will be replaced by `inline` later.
val caseClassInScala2StdLib = isCaseClass && ctx.settings.Yscala2Stdlib.value

val originalTparams = constr1.leadingTypeParams
val originalVparamss = asTermOnly(constr1.trailingParamss)
Expand Down Expand Up @@ -662,9 +664,7 @@ object desugar {
// new C[...](p1, ..., pN)(moreParams)
val (caseClassMeths, enumScaffolding) = {
def syntheticProperty(name: TermName, tpt: Tree, rhs: Tree) =
val mods =
if ctx.settings.Yscala2Stdlib.value then synthetic | Inline
else synthetic
val mods = if caseClassInScala2StdLib then synthetic | Inline else synthetic
DefDef(name, Nil, tpt, rhs).withMods(mods)

def productElemMeths =
Expand Down Expand Up @@ -782,7 +782,7 @@ object desugar {
val unapplyParam = makeSyntheticParameter(tpt = classTypeRef)
val unapplyRHS =
if (arity == 0) Literal(Constant(true))
else if ctx.settings.Yscala2Stdlib.value then scala2LibCompatUnapplyRhs(unapplyParam.name)
else if caseClassInScala2StdLib then scala2LibCompatUnapplyRhs(unapplyParam.name)
else Ident(unapplyParam.name)
val unapplyResTp = if (arity == 0) Literal(Constant(true)) else TypeTree()

Expand Down
33 changes: 31 additions & 2 deletions compiler/src/dotty/tools/dotc/transform/PostTyper.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ object PostTyper {
* mini-phase or subfunction of a macro phase equally well. But taken by themselves
* they do not warrant their own group of miniphases before pickling.
*/
class PostTyper extends MacroTransform with IdentityDenotTransformer { thisPhase =>
class PostTyper extends MacroTransform with InfoTransformer { thisPhase =>
import tpd._

override def phaseName: String = PostTyper.name
Expand All @@ -80,6 +80,10 @@ class PostTyper extends MacroTransform with IdentityDenotTransformer { thisPhase
def newTransformer(using Context): Transformer =
new PostTyperTransformer

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

val superAcc: SuperAccessors = new SuperAccessors(thisPhase)
val synthMbr: SyntheticMembers = new SyntheticMembers(thisPhase)
val beanProps: BeanProperties = new BeanProperties(thisPhase)
Expand Down Expand Up @@ -425,7 +429,7 @@ class PostTyper extends MacroTransform with IdentityDenotTransformer { thisPhase
if sym.isOpaqueAlias then
VarianceChecker.checkLambda(rhs, TypeBounds.upper(sym.opaqueAlias))
case _ =>
processMemberDef(super.transform(tree))
processMemberDef(super.transform(scala2LibPatch(tree)))
case tree: Bind =>
if tree.symbol.isType && !tree.symbol.name.is(WildcardParamName) then
Checking.checkGoodBounds(tree.symbol)
Expand Down Expand Up @@ -534,5 +538,30 @@ class PostTyper extends MacroTransform with IdentityDenotTransformer { thisPhase
sym.addAnnotation(Annotation(defn.ExperimentalAnnot, sym.span))
sym.companionModule.addAnnotation(Annotation(defn.ExperimentalAnnot, sym.span))

private def scala2LibPatch(tree: TypeDef)(using Context) =
val sym = tree.symbol
if compilingScala2StdLib
&& sym.is(ModuleClass) && !sym.derivesFrom(defn.SerializableClass)
&& sym.companionClass.derivesFrom(defn.SerializableClass)
then
// Add Serializable to companion objects of serializable classes
tree.rhs match
case impl: Template =>
val parents1 = impl.parents :+ TypeTree(defn.SerializableType)
val impl1 = cpy.Template(impl)(parents = parents1)
cpy.TypeDef(tree)(rhs = impl1)
else tree
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest factoring that out ina @ThreadUnsafe lazy val

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

compilingScala2StdLib && sym.isAllOf(ModuleClass, butNot = Package)

def transformInfo(tp: Type, sym: Symbol)(using Context): Type = tp match
case info: ClassInfo =>
if !sym.derivesFrom(defn.SerializableClass)
&& sym.companionClass.derivesFrom(defn.SerializableClass)
then
info.derivedClassInfo(declaredParents = info.parents :+ defn.SerializableType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else tp
case _ => tp
}
6 changes: 5 additions & 1 deletion project/MiMaFilters.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ object MiMaFilters {

// Companion module class
ProblemFilters.exclude[FinalClassProblem]("scala.*$"),
ProblemFilters.exclude[MissingTypesProblem]("scala.*$"),

// Missing types {scala.runtime.AbstractFunction1}
ProblemFilters.exclude[MissingTypesProblem]("scala.ScalaReflectionException$"),
ProblemFilters.exclude[MissingTypesProblem]("scala.UninitializedFieldError$"),
ProblemFilters.exclude[MissingTypesProblem]("scala.collection.StringView$"),

// Tuples
ProblemFilters.exclude[FinalClassProblem]("scala.Tuple1"),
Expand Down
10 changes: 8 additions & 2 deletions project/TastyMiMaFilters.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,14 @@ object TastyMiMaFilters {
// Probably OK: Case class with varargs
ProblemMatcher.make(ProblemKind.IncompatibleTypeChange, "scala.StringContext.parts"), // before: scala.<repeated>[Predef.String]; after: scala.collection.immutable.Seq[Predef.String] @scala.annotation.internal.Repeated

// Problem: Missing Serializable in companions of serializable classes
ProblemMatcher.make(ProblemKind.MissingParent, "scala.*$"),
// Problem: Missing type {scala.runtime.AbstractFunction1}
ProblemMatcher.make(ProblemKind.MissingParent, "scala.collection.Searching.Found$"),
ProblemMatcher.make(ProblemKind.MissingParent, "scala.collection.Searching.InsertionPoint$"),
ProblemMatcher.make(ProblemKind.MissingParent, "scala.collection.StringView$"),
ProblemMatcher.make(ProblemKind.MissingParent, "scala.jdk.FunctionWrappers.AsJava*$"),
ProblemMatcher.make(ProblemKind.MissingParent, "scala.jdk.FunctionWrappers.FromJava*$"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ProblemMatcher.make(ProblemKind.MissingParent, "scala.ScalaReflectionException$"),
ProblemMatcher.make(ProblemKind.MissingParent, "scala.UninitializedFieldError$"),

// Problem: Class[T] or ClassTag[T] with `T` equal to wildcard `_ >: Nothing <: AnyVal` instead of a specific primitive type `T`
ProblemMatcher.make(ProblemKind.IncompatibleTypeChange, "scala.reflect.ManifestFactory.*.runtimeClass"),
Expand Down