address review: part 1

Author: bishabosha

compiler/src/dotty/tools/dotc/CompilationUnit.scala

@@ -59,8 +59,8 @@ class CompilationUnit protected (val source: SourceFile, val info: CompilationUn
 
   var hasMacroAnnotations: Boolean = false
 
-  def hasUnrollDefs: Boolean = unrolledClasses != null
-  var unrolledClasses: Set[Symbol] | Null = null
+  def hasUnrollDefs: Boolean = unrolledClasses.nonEmpty
+  var unrolledClasses: Set[Symbol] = Set.empty
 
   /** Set to `true` if inliner added anonymous mirrors that need to be completed */
   var needsMirrorSupport: Boolean = false

compiler/src/dotty/tools/dotc/core/Denotations.scala

@@ -1074,7 +1074,9 @@ object Denotations {
     def filterDisjoint(denots: PreDenotation)(using Context): SingleDenotation =
       if (denots.exists && denots.matches(this)) NoDenotation else this
     def filterWithFlags(required: FlagSet, excluded: FlagSet)(using Context): SingleDenotation =
-      val realExcluded = if ctx.isAfterTyper then excluded else excluded | (if ctx.mode.is(Mode.ResolveFromTASTy) then EmptyFlags else Invisible)
+      val realExcluded =
+        if ctx.isAfterTyper || ctx.mode.is(Mode.ResolveFromTASTy) then excluded
+        else excluded | Invisible
       def symd: SymDenotation = this match
         case symd: SymDenotation => symd
         case _ => symbol.denot

compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala

@@ -20,7 +20,6 @@ import collection.mutable
 import reporting.{Profile, NoProfile}
 import dotty.tools.tasty.TastyFormat.ASTsSection
 import quoted.QuotePatterns
-import dotty.tools.dotc.config.Feature
 
 object TreePickler:
   class StackSizeExceeded(val mdef: tpd.MemberDef) extends Exception
@@ -475,16 +474,15 @@ class TreePickler(pickler: TastyPickler, attributes: Attributes) {
             case _ =>
               if passesConditionForErroringBestEffortCode(tree.hasType) then
                 // #19951 The signature of a constructor of a Java annotation is irrelevant
-                val sym = tree.symbol
                 val sig =
-                  if name == nme.CONSTRUCTOR && sym.exists && sym.owner.is(JavaAnnotation) then Signature.NotAMethod
+                  if name == nme.CONSTRUCTOR && tree.symbol.exists && tree.symbol.owner.is(JavaAnnotation) then Signature.NotAMethod
                   else tree.tpe.signature
-                var ename = sym.targetName
+                var ename = tree.symbol.targetName
                 val selectFromQualifier =
                   name.isTypeName
                   || qual.isInstanceOf[Hole] // holes have no symbol
                   || sig == Signature.NotAMethod // no overload resolution necessary
-                  || !sym.exists // polymorphic function type
+                  || !tree.denot.symbol.exists // polymorphic function type
                   || tree.denot.asSingleDenotation.isRefinedMethod // refined methods have no defining class symbol
                 if selectFromQualifier then
                   writeByte(if name.isTypeName then SELECTtpt else SELECT)
@@ -493,9 +491,9 @@ class TreePickler(pickler: TastyPickler, attributes: Attributes) {
                 else // select from owner
                   writeByte(SELECTin)
                   withLength {
-                    pickleNameAndSig(name, sym.signature, ename)
+                    pickleNameAndSig(name, tree.symbol.signature, ename)
                     pickleTree(qual)
-                    pickleType(sym.owner.typeRef)
+                    pickleType(tree.symbol.owner.typeRef)
                   }
               else
                 writeByte(if name.isTypeName then SELECTtpt else SELECT)

compiler/src/dotty/tools/dotc/reporting/messages.scala

@@ -3361,7 +3361,7 @@ extends DeclarationMsg(IllegalUnrollPlacementID):
     case Some(method) =>
       val isCtor = method.isConstructor
       def what = if isCtor then i"a ${if method.owner.is(Trait) then "trait" else "class"} constructor" else i"method ${method.name}"
-      val prefix = s"Can not unroll parameters of $what"
+      val prefix = s"Cannot unroll parameters of $what"
       if method.is(Deferred) then
         i"$prefix: it must not be abstract"
       else if isCtor && method.owner.is(Trait) then

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

@@ -120,20 +120,12 @@ class PostTyper extends MacroTransform with InfoTransformer { thisPhase =>
 
     private var inJavaAnnot: Boolean = false
 
-    private var seenUnrolledMethods: util.EqHashMap[Symbol, Boolean] | Null = null
+    private val seenUnrolledMethods: util.EqHashMap[Symbol, Boolean] = new util.EqHashMap[Symbol, Boolean]
 
     private var noCheckNews: Set[New] = Set()
 
     def isValidUnrolledMethod(method: Symbol, origin: SrcPos)(using Context): Boolean =
-      val seenMethods =
-        val local = seenUnrolledMethods
-        if local == null then
-          val map = new util.EqHashMap[Symbol, Boolean]
-          seenUnrolledMethods = map
-          map
-        else
-          local
-      seenMethods.getOrElseUpdate(method, {
+      seenUnrolledMethods.getOrElseUpdate(method, {
         val isCtor = method.isConstructor
         if
           method.name.is(DefaultGetterName)
@@ -208,12 +200,8 @@ class PostTyper extends MacroTransform with InfoTransformer { thisPhase =>
     private def registerIfUnrolledParam(sym: Symbol)(using Context): Unit =
       if sym.hasAnnotation(defn.UnrollAnnot) && isValidUnrolledMethod(sym.owner, sym.sourcePos) then
         val cls = sym.enclosingClass
-        val classes = ctx.compilationUnit.unrolledClasses
         val additions = Array(cls, cls.linkedClass).filter(_ != NoSymbol)
-        if classes == null then
-          ctx.compilationUnit.unrolledClasses = Set.from(additions)
-        else
-          ctx.compilationUnit.unrolledClasses = classes ++ additions
+        ctx.compilationUnit.unrolledClasses ++= additions
 
     private def processValOrDefDef(tree: Tree)(using Context): tree.type =
       val sym = tree.symbol

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

@@ -22,6 +22,7 @@ import scala.collection.mutable
 import scala.util.boundary, boundary.break
 import dotty.tools.dotc.core.StdNames.nme
 import dotty.tools.unreachable
+import dotty.tools.dotc.util.Spans.Span
 
 /**Implementation of SIP-61.
  * Runs when `@unroll` annotations are found in a compilation unit, installing new definitions
@@ -33,16 +34,10 @@ class UnrollDefinitions extends MacroTransform, IdentityDenotTransformer {
 
   import tpd.*
 
-  private var _unrolledDefs: util.EqHashMap[Symbol, ComputedIndicies] | Null = null
-  private def initializeUnrolledDefs(): util.EqHashMap[Symbol, ComputedIndicies] =
-    val local = _unrolledDefs
-    if local == null then
-      val map = new util.EqHashMap[Symbol, ComputedIndicies]
-      _unrolledDefs = map
-      map
-    else
-      local.clear()
-      local
+  private val _unrolledDefs: util.EqHashMap[Symbol, ComputedIndices] = new util.EqHashMap[Symbol, ComputedIndices]
+  private def initializeUnrolledDefs(): util.EqHashMap[Symbol, ComputedIndices] =
+    _unrolledDefs.clear()
+    _unrolledDefs
 
   override def phaseName: String = UnrollDefinitions.name
 
@@ -55,15 +50,15 @@ class UnrollDefinitions extends MacroTransform, IdentityDenotTransformer {
       super.run // create and run the transformer on the current compilation unit
 
   def newTransformer(using Context): Transformer =
-    UnrollingTransformer(ctx.compilationUnit.unrolledClasses.nn)
+    UnrollingTransformer(ctx.compilationUnit.unrolledClasses)
 
-  type ComputedIndicies = List[(Int, List[Int])]
-  type ComputeIndicies = Context ?=> Symbol => ComputedIndicies
+  type ComputedIndices = List[(Int, List[Int])]
+  type ComputeIndices = Context ?=> Symbol => ComputedIndices
 
-  private class UnrollingTransformer(classes: Set[Symbol]) extends Transformer {
+  private class UnrollingTransformer(unrolledClasses: Set[Symbol]) extends Transformer {
     private val unrolledDefs = initializeUnrolledDefs()
 
-    def computeIndices(annotated: Symbol)(using Context): ComputedIndicies =
+    def computeIndices(annotated: Symbol)(using Context): ComputedIndices =
       unrolledDefs.getOrElseUpdate(annotated, {
         if annotated.name.is(DefaultGetterName) then
           Nil // happens in curried methods where more than one parameter list has @unroll
@@ -84,25 +79,25 @@ class UnrollDefinitions extends MacroTransform, IdentityDenotTransformer {
     end computeIndices
 
     override def transform(tree: tpd.Tree)(using Context): tpd.Tree = tree match
-      case tree @ TypeDef(_, impl: Template) if classes(tree.symbol) =>
+      case tree @ TypeDef(_, impl: Template) if unrolledClasses(tree.symbol) =>
         super.transform(cpy.TypeDef(tree)(rhs = unrollTemplate(impl, computeIndices)))
       case tree =>
         super.transform(tree)
   }
 
-  def copyParamSym(sym: Symbol, parent: Symbol)(using Context): (Symbol, Symbol) =
+  private def copyParamSym(sym: Symbol, parent: Symbol)(using Context): (Symbol, Symbol) =
     val copied = sym.copy(owner = parent, flags = (sym.flags &~ HasDefault), coord = sym.coord)
     sym -> copied
 
-  def symLocation(sym: Symbol)(using Context) = {
+  private def symLocation(sym: Symbol)(using Context) = {
     val lineDesc =
       if (sym.span.exists && sym.span != sym.owner.span)
         s" at line ${sym.srcPos.line + 1}"
       else ""
     i"in ${sym.owner}${lineDesc}"
   }
 
-  def findUnrollAnnotations(params: List[Symbol])(using Context): List[Int] = {
+  private def findUnrollAnnotations(params: List[Symbol])(using Context): List[Int] = {
     params
       .zipWithIndex
       .collect {
@@ -111,16 +106,25 @@ class UnrollDefinitions extends MacroTransform, IdentityDenotTransformer {
       }
   }
 
-  def isTypeClause(p: ParamClause) = p.headOption.exists(_.isInstanceOf[TypeDef])
-
-  def generateSingleForwarder(defdef: DefDef,
-                              prevMethodType: Type,
+  private def isTypeClause(p: ParamClause) = p.headOption.exists(_.isInstanceOf[TypeDef])
+
+  /** Generate a forwarder that calls the next one in a "chain" of forwarders
+   *
+   * @param defdef the original unrolled def that the forwarder is derived from
+   * @param paramIndex index of the unrolled parameter (in the parameter list) that we stop at
+   * @param paramCount number of parameters in the annotated parameter list
+   * @param nextParamIndex index of next unrolled parameter - to fetch default argument
+   * @param nextSpan span of next forwarder - used to ensure the span is not identical by shifting (TODO remove)
+   * @param annotatedParamListIndex index of the parameter list that contains unrolled parameters
+   * @param isCaseApply if `defdef` is a case class apply/constructor - used for selection of default arguments
+   */
+  private def generateSingleForwarder(defdef: DefDef,
                               paramIndex: Int,
                               paramCount: Int,
                               nextParamIndex: Int,
-                              nextSymbol: Symbol,
+                              nextSpan: Span,
                               annotatedParamListIndex: Int,
-                              isCaseApply: Boolean)(using Context) = {
+                              isCaseApply: Boolean)(using Context): DefDef = {
 
     def initNewForwarder()(using Context): (TermSymbol, List[List[Symbol]]) = {
       val forwarderDefSymbol0 = Symbols.newSymbol(
@@ -129,15 +133,15 @@ class UnrollDefinitions extends MacroTransform, IdentityDenotTransformer {
         defdef.symbol.flags &~ HasDefaultParams |
         Invisible | Synthetic,
         NoType, // fill in later
-        coord = nextSymbol.span.shift(1) // shift by 1 to avoid "secondary constructor must call preceding" error
+        coord = nextSpan.shift(1) // shift by 1 to avoid "secondary constructor must call preceding" error
       ).entered
 
       val newParamSymMappings = extractParamSymss(copyParamSym(_, forwarderDefSymbol0))
       val (oldParams, newParams) = newParamSymMappings.flatten.unzip
 
       val newParamSymLists0 =
-        newParamSymMappings.map: pairss =>
-          pairss.map: (oldSym, newSym) =>
+        newParamSymMappings.map: pairs =>
+          pairs.map: (oldSym, newSym) =>
             newSym.info = oldSym.info.substSym(oldParams, newParams)
             newSym
 
@@ -153,8 +157,6 @@ class UnrollDefinitions extends MacroTransform, IdentityDenotTransformer {
         else ps.map(p => onSymbol(p.symbol))
       }
 
-    val paramCount = defdef.symbol.paramSymss(annotatedParamListIndex).size
-
     val (forwarderDefSymbol, newParamSymLists) = initNewForwarder()
 
     def forwarderRhs(): tpd.Tree = {
@@ -224,10 +226,10 @@ class UnrollDefinitions extends MacroTransform, IdentityDenotTransformer {
     val forwarderDef =
       tpd.DefDef(forwarderDefSymbol, rhs = forwarderRhs())
 
-    forwarderDef.withSpan(nextSymbol.span.shift(1))
+    forwarderDef.withSpan(nextSpan.shift(1))
   }
 
-  def generateFromProduct(startParamIndices: List[Int], paramCount: Int, defdef: DefDef)(using Context) = {
+  private def generateFromProduct(startParamIndices: List[Int], paramCount: Int, defdef: DefDef)(using Context) = {
     cpy.DefDef(defdef)(
       name = defdef.name,
       paramss = defdef.paramss,
@@ -248,28 +250,35 @@ class UnrollDefinitions extends MacroTransform, IdentityDenotTransformer {
                 )
             )
           )
-        } ++ Seq(
-          CaseDef(
-            Underscore(defn.IntType),
-            EmptyTree,
-            defdef.rhs
-          )
+        } :+ CaseDef(
+          Underscore(defn.IntType),
+          EmptyTree,
+          defdef.rhs
         )
       )
     ).setDefTree
   }
 
-  def generateSyntheticDefs(tree: Tree, compute: ComputeIndicies)(using Context): Option[(Symbol, Option[Symbol], Seq[DefDef])] = tree match {
+  private enum Gen:
+    case Substitute(origin: Symbol, newDef: DefDef)
+    case Forwarders(origin: Symbol, forwarders: Seq[DefDef])
+
+    def origin: Symbol
+    def extras: Seq[DefDef] = this match
+      case Substitute(_, d) => d :: Nil
+      case Forwarders(_, ds) => ds
+
+  private def generateSyntheticDefs(tree: Tree, compute: ComputeIndices)(using Context): Option[Gen] = tree match {
     case defdef: DefDef if defdef.paramss.nonEmpty =>
       import dotty.tools.dotc.core.NameOps.isConstructorName
 
       val isCaseCopy =
-        defdef.name.toString == "copy" && defdef.symbol.owner.is(CaseClass)
+        defdef.name == nme.copy && defdef.symbol.owner.is(CaseClass)
 
       val isCaseApply =
-        defdef.name.toString == "apply" && defdef.symbol.owner.companionClass.is(CaseClass)
+        defdef.name == nme.apply && defdef.symbol.owner.companionClass.is(CaseClass)
 
-      val isCaseFromProduct = defdef.name.toString == "fromProduct" && defdef.symbol.owner.companionClass.is(CaseClass)
+      val isCaseFromProduct = defdef.name == nme.fromProduct && defdef.symbol.owner.companionClass.is(CaseClass)
 
       val annotated =
         if (isCaseCopy) defdef.symbol.owner.primaryConstructor
@@ -282,25 +291,27 @@ class UnrollDefinitions extends MacroTransform, IdentityDenotTransformer {
         case Seq((paramClauseIndex, annotationIndices)) =>
           val paramCount = annotated.paramSymss(paramClauseIndex).size
           if isCaseFromProduct then
-            Some((defdef.symbol, Some(defdef.symbol), Seq(generateFromProduct(annotationIndices, paramCount, defdef))))
+            Some(Gen.Substitute(
+              origin = defdef.symbol,
+              newDef = generateFromProduct(annotationIndices, paramCount, defdef)
+            ))
           else
             val (generatedDefs, _) =
               val indices = (annotationIndices :+ paramCount).sliding(2).toList.reverse
-              indices.foldLeft((Seq.empty[DefDef], defdef.symbol)):
-                case ((defdefs, nextSymbol), Seq(paramIndex, nextParamIndex)) =>
+              indices.foldLeft((Seq.empty[DefDef], defdef.symbol.span)):
+                case ((defdefs, nextSpan), Seq(paramIndex, nextParamIndex)) =>
                   val forwarder = generateSingleForwarder(
                     defdef,
-                    defdef.symbol.info,
                     paramIndex,
                     paramCount,
                     nextParamIndex,
-                    nextSymbol,
+                    nextSpan,
                     paramClauseIndex,
                     isCaseApply
                   )
-                  (forwarder +: defdefs, forwarder.symbol)
+                  (forwarder +: defdefs, forwarder.symbol.span)
                 case _ => unreachable("sliding with at least 2 elements")
-            Some((defdef.symbol, None, generatedDefs))
+            Some(Gen.Forwarders(origin = defdef.symbol, forwarders = generatedDefs))
 
         case multiple =>
           report.error("Cannot have multiple parameter lists containing `@unroll` annotation", defdef.srcPos)
@@ -310,12 +321,12 @@ class UnrollDefinitions extends MacroTransform, IdentityDenotTransformer {
     case _ => None
   }
 
-  def unrollTemplate(tmpl: tpd.Template, compute: ComputeIndicies)(using Context): tpd.Tree = {
+  private def unrollTemplate(tmpl: tpd.Template, compute: ComputeIndices)(using Context): tpd.Tree = {
 
     val generatedBody = tmpl.body.flatMap(generateSyntheticDefs(_, compute))
     val generatedConstr0 = generateSyntheticDefs(tmpl.constr, compute)
     val allGenerated = generatedBody ++ generatedConstr0
-    val bodySubs = generatedBody.flatMap((_, maybeSub, _) => maybeSub).toSet
+    val bodySubs = generatedBody.collect({ case s: Gen.Substitute => s.origin }).toSet
     val otherDecls = tmpl.body.filterNot(d => d.symbol.exists && bodySubs(d.symbol))
 
     /** inlined from compiler/src/dotty/tools/dotc/typer/Checking.scala */
@@ -326,28 +337,26 @@ class UnrollDefinitions extends MacroTransform, IdentityDenotTransformer {
     if allGenerated.nonEmpty then
       val byName = (tmpl.constr :: otherDecls).groupMap(_.symbol.name.toString)(_.symbol)
       for
-        (src, _, dcls) <- allGenerated
-        dcl <- dcls
+        syntheticDefs <- allGenerated
+        dcl <- syntheticDefs.extras
       do
         val replaced = dcl.symbol
         byName.get(dcl.name.toString).foreach { syms =>
           val clashes = syms.filter(checkClash(replaced, _))
           for existing <- clashes do
+            val src = syntheticDefs.origin
             report.error(i"""Unrolled $replaced clashes with existing declaration.
               |Please remove the clashing definition, or the @unroll annotation.
               |Unrolled from ${hl(src.showDcl)} ${symLocation(src)}""".stripMargin, existing.srcPos)
         }
     end if
 
-    val generatedDefs = generatedBody.flatMap((_, _, gens) => gens)
-    val generatedConstr = generatedConstr0.toList.flatMap((_, _, gens) => gens)
-
     cpy.Template(tmpl)(
       tmpl.constr,
       tmpl.parents,
       tmpl.derived,
       tmpl.self,
-      otherDecls ++ generatedDefs ++ generatedConstr
+      otherDecls ++ allGenerated.flatMap(_.extras)
     )
   }