Address reviewer comments.

8 files changed, 46 insertions, 40 deletions

diff --git a/src/dotty/tools/dotc/ast/Desugar.scala b/src/dotty/tools/dotc/ast/Desugar.scala
index 2fb1b3ef5..7a305a8b8 100644
--- a/src/dotty/tools/dotc/ast/Desugar.scala
+++ b/src/dotty/tools/dotc/ast/Desugar.scala
@@ -500,7 +500,7 @@ object desugar {
    *    val/var/lazy val p = e
    *
    *  Otherwise, in case there is exactly one variable x_1 in pattern
-   *   val/var/lazy val p = e  ==>  val/var x_1 = (e: @unchecked) match (case p => (x_1))
+   *   val/var/lazy val p = e  ==>  val/var/lazy val x_1 = (e: @unchecked) match (case p => (x_1))
    *
    *   in case there are zero or more than one variables in pattern
    *   val/var/lazy p = e  ==>  private synthetic [lazy] val t$ = (e: @unchecked) match (case p => (x_1, ..., x_N))
@@ -508,7 +508,7 @@ object desugar {
    *                   ...
    *                   val/var/def x_N = t$._N
    *  If the original pattern variable carries a type annotation, so does the corresponding
-   *  ValDef.
+   *  ValDef or DefDef.
    */
   def makePatDef(mods: Modifiers, pat: Tree, rhs: Tree)(implicit ctx: Context): Tree = pat match {
     case VarPattern(named, tpt) =>
diff --git a/src/dotty/tools/dotc/ast/NavigateAST.scala b/src/dotty/tools/dotc/ast/NavigateAST.scala
index 3753c6702..782866bad 100644
--- a/src/dotty/tools/dotc/ast/NavigateAST.scala
+++ b/src/dotty/tools/dotc/ast/NavigateAST.scala
@@ -47,7 +47,7 @@ object NavigateAST {
         }
       case _ =>
         untypedPath(tree.pos) match {
-          case path @ last :: _ if last.pos == tree.pos || !exactMatch => path
+          case (path @ last :: _) if last.pos == tree.pos || !exactMatch => path
           case _ => Nil
         }
     }
diff --git a/src/dotty/tools/dotc/ast/Positioned.scala b/src/dotty/tools/dotc/ast/Positioned.scala
index cff77c919..e7f5de591 100644
--- a/src/dotty/tools/dotc/ast/Positioned.scala
+++ b/src/dotty/tools/dotc/ast/Positioned.scala
@@ -54,14 +54,23 @@ abstract class Positioned extends DotClass with Product {
    */
   private[dotc] def setPosUnchecked(pos: Position) = curPos = pos
 
-  /** If any children of this node do not have positions, set them to the given position,
+  /** If any children of this node do not have positions,
+   *  fit their positions between the positions of the known subtrees
    *  and transitively visit their children.
+   *  The method is likely time-critical because it is invoked on any node
+   *  we create, so we want to avoid object allocations in the common case.
+   *  The method is naturally expressed as two mutually (tail-)recursive
+   *  functions, one which computes the next element to consider or terminates if there
+   *  is none and the other which propagates the position information to that element.
+   *  But since mutual tail recursion is not supported in Scala, we express it instead
+   *  as a while loop with a termination by return in the middle.
    */
   private def setChildPositions(pos: Position): Unit = {
-    var n = productArity
-    var elems: List[Any] = Nil
-    var end = pos.end
-    var outstanding: List[Positioned] = Nil
+    var n = productArity                    // subnodes are analyzed right to left
+    var elems: List[Any] = Nil              // children in lists still to be considered, from right to left
+    var end = pos.end                       // the last defined offset, fill in positions up to this offset
+    var outstanding: List[Positioned] = Nil // nodes that need their positions filled once a start position
+                                            // is known, from left to right.
     def fillIn(ps: List[Positioned], start: Int, end: Int): Unit = ps match {
       case p :: ps1 =>
         p.setPos(Position(start, end))
@@ -69,20 +78,20 @@ abstract class Positioned extends DotClass with Product {
       case nil =>
     }
     while (true) {
-      var nextElem: Any = null
+      var nextChild: Any = null // the next child to be considered
       if (elems.nonEmpty) {
-        nextElem = elems.head
+        nextChild = elems.head
         elems = elems.tail
       }
       else if (n > 0) {
         n = n - 1
-        nextElem = productElement(n)
+        nextChild = productElement(n)
       }
       else {
         fillIn(outstanding, pos.start, end)
         return
       }
-      nextElem match {
+      nextChild match {
         case p: Positioned =>
           if (p.pos.exists) {
             fillIn(outstanding, p.pos.end, end)
@@ -91,8 +100,7 @@ abstract class Positioned extends DotClass with Product {
           }
           else outstanding = p :: outstanding
         case xs: List[_] =>
-          val newElems = xs.reverse
-          elems = if (elems.isEmpty) newElems else newElems ::: elems
+          elems = elems ::: xs.reverse
         case _ =>
       }
     }
diff --git a/src/dotty/tools/dotc/parsing/Parsers.scala b/src/dotty/tools/dotc/parsing/Parsers.scala
index aa2058692..6ec75a8b2 100644
--- a/src/dotty/tools/dotc/parsing/Parsers.scala
+++ b/src/dotty/tools/dotc/parsing/Parsers.scala
@@ -1763,7 +1763,9 @@ object Parsers {
      */
     def defDefOrDcl(mods: Modifiers): Tree = atPos(tokenRange) {
       def scala2ProcedureSyntax(resultTypeStr: String) = {
-        val toInsert = if (in.token == LBRACE) s"$resultTypeStr =" else ": Unit "
+        val toInsert =
+          if (in.token == LBRACE) s"$resultTypeStr ="
+          else ": Unit "  // trailing space ensures that `def f()def g()` works.
         testScala2Mode(s"Procedure syntax no longer supported; `$toInsert' should be inserted here") && {
           patch(source, Position(in.lastOffset), toInsert)
           true
diff --git a/src/dotty/tools/dotc/rewrite/Rewrites.scala b/src/dotty/tools/dotc/rewrite/Rewrites.scala
index cda9e8565..7ab0e5d59 100644
--- a/src/dotty/tools/dotc/rewrite/Rewrites.scala
+++ b/src/dotty/tools/dotc/rewrite/Rewrites.scala
@@ -51,7 +51,7 @@ object Rewrites {
 
     def writeBack(): Unit = {
       val out = source.file.output
-      val chars = apply(source.content)
+      val chars = apply(source.underlying.content)
       val bytes = new String(chars).getBytes
       out.write(bytes)
       out.close()
@@ -62,28 +62,21 @@ object Rewrites {
    *  given by `pos` in `source` by `replacement`
    */
   def patch(source: SourceFile, pos: Position, replacement: String)(implicit ctx: Context): Unit =
-    ctx.settings.rewrite.value match {
-      case Some(rewrites: Rewrites) =>
-        rewrites.patched.get(source) match {
-          case Some(ps) =>
-            ps.addPatch(pos, replacement)
-          case None =>
-            rewrites.patched(source) = new Patches(source)
-            patch(source, pos, replacement)
-        }
-      case _ =>
-    }
+    for (rewrites <- ctx.settings.rewrite.value)
+      rewrites.patched
+        .getOrElseUpdate(source, new Patches(source))
+        .addPatch(pos, replacement)
+
+  /** Patch position in `ctx.compilationUnit.source`. */
+  def patch(pos: Position, replacement: String)(implicit ctx: Context): Unit =
+    patch(ctx.compilationUnit.source, pos, replacement)
 
   /** If -rewrite is set, apply all patches and overwrite patched source files.
    */
   def writeBack()(implicit ctx: Context) =
-    ctx.settings.rewrite.value match {
-      case Some(rewrites: Rewrites) =>
-        for (source <- rewrites.patched.keys) {
-          ctx.println(s"[patched file ${source.file.path}]")
-          rewrites.patched(source).writeBack()
-        }
-      case _ =>
+    for (rewrites <- ctx.settings.rewrite.value; source <- rewrites.patched.keys) {
+      ctx.println(s"[patched file ${source.file.path}]")
+      rewrites.patched(source).writeBack()
     }
 }
 
diff --git a/src/dotty/tools/dotc/transform/LazyVals.scala b/src/dotty/tools/dotc/transform/LazyVals.scala
index 50db7b3fb..fc02e68cc 100644
--- a/src/dotty/tools/dotc/transform/LazyVals.scala
+++ b/src/dotty/tools/dotc/transform/LazyVals.scala
@@ -70,7 +70,10 @@ class LazyVals extends MiniPhaseTransform with IdentityDenotTransformer with Nee
       val isField = sym.owner.isClass
       if (isField) {
         if (sym.isVolatile ||
-           (sym.is(Flags.Module)/* || ctx.scala2Mode*/) && !sym.is(Flags.Synthetic)) // TODO assume @voliat
+           (sym.is(Flags.Module)/* || ctx.scala2Mode*/) &&
+            // TODO assume @volatile once LazyVals uses static helper constructs instead of
+            // ones in the companion object.
+           !sym.is(Flags.Synthetic))
             // module class is user-defined.
             // Should be threadsafe, to mimic safety guaranteed by global object
           transformMemberDefVolatile(tree)
diff --git a/src/dotty/tools/dotc/typer/Typer.scala b/src/dotty/tools/dotc/typer/Typer.scala
index 1a0525c6d..2069e790b 100644
--- a/src/dotty/tools/dotc/typer/Typer.scala
+++ b/src/dotty/tools/dotc/typer/Typer.scala
@@ -998,7 +998,7 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit
     if (sym.is(Lazy, butNot = Deferred | Module | Synthetic) && !sym.isVolatile &&
         ctx.scala2Mode && ctx.settings.rewrite.value.isDefined &&
         !ctx.isAfterTyper)
-      patch(ctx.compilationUnit.source, Position(toUntyped(vdef).envelope.start), "@volatile ")
+      patch(Position(toUntyped(vdef).envelope.start), "@volatile ")
   }
 
   def typedDefDef(ddef: untpd.DefDef, sym: Symbol)(implicit ctx: Context) = track("typedDefDef") {
@@ -1158,8 +1158,8 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit
       if (ctx.scala2Mode) {
         // Under -rewrite, patch `x _` to `(() => x)`
         ctx.migrationWarning(msg, tree.pos)
-        patch(ctx.compilationUnit.source, Position(tree.pos.start), "(() => ")
-        patch(ctx.compilationUnit.source, Position(qual.pos.end, tree.pos.end), ")")
+        patch(Position(tree.pos.start), "(() => ")
+        patch(Position(qual.pos.end, tree.pos.end), ")")
         res = typed(untpd.Function(Nil, untpd.TypedSplice(res)))
       }
       else ctx.error(msg, tree.pos)
diff --git a/src/dotty/tools/dotc/typer/VarianceChecker.scala b/src/dotty/tools/dotc/typer/VarianceChecker.scala
index 1769835da..274218ee3 100644
--- a/src/dotty/tools/dotc/typer/VarianceChecker.scala
+++ b/src/dotty/tools/dotc/typer/VarianceChecker.scala
@@ -114,8 +114,8 @@ class VarianceChecker()(implicit ctx: Context) {
       case Some(VarianceError(tvar, required)) =>
         def msg = i"${varianceString(tvar.flags)} $tvar occurs in ${varianceString(required)} position in type ${sym.info} of $sym"
         if (ctx.scala2Mode && sym.owner.isConstructor) {
-          ctx.migrationWarning(s"According to new variance rules, this is no longer accepted; need to annotate with @uncheckedVariance:\n$msg, pos = ${sym.pos}", sym.pos)
-          patch(ctx.compilationUnit.source, Position(pos.end), " @scala.annotation.unchecked.uncheckedVariance") // TODO use an import or shorten if possible
+          ctx.migrationWarning(s"According to new variance rules, this is no longer accepted; need to annotate with @uncheckedVariance:\n$msg", sym.pos)
+          patch(Position(pos.end), " @scala.annotation.unchecked.uncheckedVariance") // TODO use an import or shorten if possible
         }
         else ctx.error(msg, sym.pos)
       case None =>