Merge pull request #129 from olafurpg/0.1.4-fixes

0.1.4 fixes

core/src/main/scala/org/scalafmt/internal/FormatOps.scala

@@ -6,6 +6,8 @@ import org.scalafmt.ScalaStyle
 import scala.annotation.tailrec
 import scala.meta.Tree
 import scala.meta.internal.ast.Case
+import scala.meta.internal.ast.Template
+import scala.meta.internal.ast.Term
 import scala.meta.tokens.Token
 import scala.meta.tokens.Token._
 import scala.meta.internal.ast.Defn
@@ -226,4 +228,40 @@ class FormatOps(val style: ScalaStyle,
   def getArrow(caseStat: Case): Token =
     caseStat.tokens.find(t => t.isInstanceOf[`=>`] && owners(t) == caseStat)
       .getOrElse(throw CaseMissingArrow(caseStat))
+
+  def templateCurly(owner: Tree): Token = {
+    defnTemplate(owner).flatMap(templateCurly).getOrElse(owner.tokens.last)
+  }
+
+  def templateCurly(template: Template): Option[Token] = {
+    template.tokens.find(x => x.isInstanceOf[`{`] && owners(x) == template)
+  }
+
+  /**
+    * Returns the expire token for the owner of dot.
+    *
+    * If the select is part of an apply like
+    *
+    * foo.bar { ... }
+    *
+    * the expire token is the closing }, otherwise it's bar.
+    */
+  def selectExpire(dot: `.`): Token = {
+    val owner = ownersMap(hash(dot))
+    (for {
+      parent <- owner.parent
+      (_, args) <- splitApplyIntoLhsAndArgsLifted(parent) if args.nonEmpty
+    } yield {
+      args.last.tokens.last
+    }).getOrElse(owner.tokens.last)
+  }
+  def functionExpire(function: Term.Function): Token = {
+    (for {
+      parent <- function.parent
+      blockEnd <- parent match {
+        case b: Term.Block if b.stats.length == 1 => Some(b.tokens.last)
+        case _ => None
+      }
+    } yield blockEnd).getOrElse(function.tokens.last)
+  }
 }

core/src/main/scala/org/scalafmt/internal/Router.scala

@@ -1,11 +1,10 @@
 package org.scalafmt.internal
 
-import scala.language.implicitConversions
-
 import org.scalafmt.Error.UnexpectedTree
 import org.scalafmt.internal.ScalaFmtLogger._
 
 import scala.collection.mutable
+import scala.language.implicitConversions
 import scala.meta.Tree
 import scala.meta.internal.ast.Case
 import scala.meta.internal.ast.Defn
@@ -177,11 +176,13 @@ class Router(formatOps: FormatOps) {
         )
       case FormatToken(arrow: `=>`, right, _)
           if leftOwner.isInstanceOf[Term.Function] =>
-        val endOfFunction = leftOwner.tokens.last
+        val endOfFunction = functionExpire(
+            leftOwner.asInstanceOf[Term.Function])
         Seq(
-            Split(Space, 0).withPolicy(SingleLineBlock(endOfFunction)),
+            Split(Space, 0, ignoreIf = isInlineComment(right))
+              .withPolicy(SingleLineBlock(endOfFunction)),
             Split(Newline, 1 + nestedApplies(leftOwner))
-              .withIndent(2, endOfFunction, Left)
+              .withIndent(2, endOfFunction, Right)
         )
       // Case arrow
       case tok@FormatToken(arrow: `=>`, right, between)
@@ -533,22 +534,25 @@ class Router(formatOps: FormatOps) {
         val open = next(next(tok)).right
         val close = matchingParentheses(hash(open))
         val nestedPenalty = nestedSelect(rightOwner) + nestedApplies(leftOwner)
-        // Must apply to both space and newlines, otherwise we will take
-        // the newline even if it doesn't prevent a newline inside the apply.
-        val penalizeNewlinesInApply = penalizeAllNewlines(close, 2)
         val chain = getSelectChain(Some(rightOwner))
-        val names = chain.map(_.name)
         val lastToken = lastTokenInChain(chain)
-//        logger.elem(lastTokenInChain, next(tok), names, chain.length)
         val breakOnEveryDot = Policy({
           case Decision(t@FormatToken(_, dot2: `.`, _), s)
               if chain.contains(owners(dot2)) =>
-//            logger.elem(s)
-            Decision(t, Seq(Split(Newline, 0).withIndent(2, dot, Left)))
+            val expire = selectExpire(dot2)
+            val newlineSplits = s.filter(_.modification.isNewline)
+            // A plain select with no apply has no newline splits.
+            val newSplits =
+              if (newlineSplits.nonEmpty) newlineSplits
+              else Seq(Split(Newline, 1).withIndent(2, expire, Left))
+            Decision(t, newSplits)
         }, lastToken.end)
         val exclude =
           insideBlock(tok, close, _.isInstanceOf[`{`]).map(parensRange)
         val expire = Math.max(lastToken.end, close.end)
+        // This policy will apply to both the space and newline splits, otherwise
+        // the newline is too cheap even it doesn't actually prevent other newlines.
+        val penalizeNewlinesInApply = penalizeAllNewlines(close, 2)
         val noSplitPolicy =
           SingleLineBlock(lastToken, exclude, disallowInlineComments = false)
             .andThen(penalizeNewlinesInApply.f).copy(expire = expire)
@@ -769,12 +773,13 @@ class Router(formatOps: FormatOps) {
         )
       case tok@FormatToken(_, cond: `if`, _)
           if rightOwner.isInstanceOf[Case] =>
-        val owner = rightOwner.asInstanceOf[Case]
-        val lastToken = getArrow(owner)
-        val penalizeNewlines = penalizeNewlineByNesting(cond, lastToken)
+        val arrow = getArrow(rightOwner.asInstanceOf[Case])
+        val exclude =
+          insideBlock(tok, arrow, _.isInstanceOf[`{`]).map(parensRange)
+        val singleLine = SingleLineBlock(arrow, exclude = exclude)
         Seq(
-            Split(Space, 0, policy = penalizeNewlines),
-            Split(Newline, 1, policy = penalizeNewlines)
+            Split(Space, 0, policy = singleLine),
+            Split(Newline, 1)
         )
       // Inline comment
       case FormatToken(_, c: Comment, between) =>

core/src/main/scala/org/scalafmt/internal/TreeOps.scala

@@ -187,9 +187,6 @@ trait TreeOps extends TokenOps {
     if (tree.children.isEmpty) 0
     else 1 + tree.children.map(treeDepth).max
 
-  def templateCurly(owner: Tree): Token = {
-    defnTemplate(owner).flatMap(templateCurly).getOrElse(owner.tokens.last)
-  }
 
   @tailrec
   final def lastLambda(first: Term.Function): Term.Function =
@@ -200,8 +197,4 @@ trait TreeOps extends TokenOps {
         lastLambda(block.stats.head.asInstanceOf[Term.Function])
       case _ => first
     }
-
-  def templateCurly(template: Template): Option[Token] = {
-    template.tokens.find(_.isInstanceOf[`{`])
-  }
 }

core/src/test/resources/default/Case.stat

@@ -51,7 +51,8 @@ x match {
 }
 >>>
 x match {
-  case tok if // TODO(olafur) DRY.
+  case tok
+      if // TODO(olafur) DRY.
       (leftOwner.isInstanceOf[Term.Interpolate] &&
           rightOwner.isInstanceOf[Term.Interpolate]) ||
       (leftOwner.isInstanceOf[Pat.Interpolate] &&

core/src/test/resources/default/Select.stat

@@ -125,3 +125,21 @@ private def extractRhino(e: js.Dynamic): js.Array[String] = {
 >>>
 new ReflectiveDynamicAccess(classLoader)
   .createInstanceFor[ScalaFmtLike]("org.scalafmt.ScalaFmt210", Seq.empty)
+<<< indent until closing }
+abb.b(c).j {
+x + 1
+}
+>>>
+abb
+  .b(c)
+  .j {
+    x + 1
+  }
+<<< Comment indent
+lst.map { x =>
+    // complicated stuff
+}
+>>>
+lst.map { x =>
+  // complicated stuff
+}

core/src/test/resources/unit/TermApply.stat

@@ -8,8 +8,8 @@ a(b)
   .g(h)
   .i(j)
   .k {
-  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
-}
+    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+  }
 <<< avoid dead ends
 function(
     firstCall(a, b, c, d, e, f, g, h),

core/src/test/scala/org/scalafmt/FormatTests.scala

@@ -24,28 +24,36 @@ import scala.concurrent.Await
 import scala.concurrent.ExecutionContext.Implicits.global
 import scala.concurrent.Future
 import scala.language.postfixOps
+import scala.meta.Tree
+import scala.meta.parsers.common.Parse
 import scala.meta.parsers.common.ParseException
 
 // TODO(olafur) property test: same solution without optimization or timeout.
 
 class FormatTests
-    extends FunSuite with Timeouts with BeforeAndAfterAll
-    with HasTests with FormatAssertions with DiffAssertions {
+    extends FunSuite with Timeouts with BeforeAndAfterAll with HasTests
+    with FormatAssertions with DiffAssertions {
   lazy val onlyUnit = UnitTests.tests.exists(_.only)
   lazy val onlyManual = !onlyUnit && ManualTests.tests.exists(_.only)
   lazy val onlyOne = tests.exists(_.only)
   lazy val debugResults = mutable.ArrayBuilder.make[Result]
 
-  def ignore(t: DiffTest): Boolean = {
-    false
-  }
+  override def ignore(t: DiffTest): Boolean = false
 
   override val tests = {
     if (onlyManual) ManualTests.tests
     else UnitTests.tests
   }
 
-  tests.sortWith(bySpecThenName).withFilter(testShouldRun).foreach(runTest)
+  tests.sortWith(bySpecThenName).withFilter(testShouldRun).foreach(runTest(
+      run))
+
+  def run(t: DiffTest, parse: Parse[_ <: Tree]): Unit = {
+    val obtained = ScalaFmt.format_!(t.original, t.style)(parse)
+    debugResults += saveResult(t, obtained, onlyOne)
+    assertFormatPreservesAst(t.original, obtained)(parse)
+    assertNoDiff(obtained, t.expected)
+  }
 
   def testShouldRun(t: DiffTest): Boolean = !onlyOne || t.only
 
@@ -54,60 +62,6 @@ class FormatTests
     (left.spec, left.name).compare(right.spec -> right.name) < 0
   }
 
-  def runTest(t: DiffTest): Unit = {
-    val paddedName = f"${t.fullName}%-70s|"
-
-    if (ignore(t)) {
-      // Not even ignore(t), save console space.
-    } else if (t.skip) {
-      ignore(paddedName) {}
-    } else {
-      test(paddedName) {
-        Debug.newTest()
-        filename2parse(t.filename) match {
-          case Some(parse) =>
-            try {
-              val obtained = ScalaFmt.format_!(t.original, t.style)(parse)
-              saveResult(t, obtained)
-              assertFormatPreservesAst(t.original, obtained)(parse)
-              assertNoDiff(obtained, t.expected)
-            } catch {
-              case e: ParseException =>
-                fail("test does not parse" + parseException2Message(
-                    e, t.original))
-            }
-          case None =>
-            logger.warn(s"Found no parse for filename ${t.filename}")
-        }
-      }
-    }
-  }
-
-  def saveResult(t: DiffTest, obtained: String): Unit = {
-    val visitedStates = Debug.exploredInTest
-    //    logger.debug(f"$visitedStates%-4s ${t.fullName}")
-    val output = getFormatOutput(t.style)
-    val obtainedHtml = Report.mkHtml(output, t.style)
-    debugResults += Result(t,
-                           obtained,
-                           obtainedHtml,
-                           output,
-                           Debug.maxVisitedToken,
-                           visitedStates,
-                           Debug.elapsedNs)
-  }
-
-  def getFormatOutput(style: ScalaStyle): Array[FormatOutput] = {
-    val builder = mutable.ArrayBuilder.make[FormatOutput]()
-    State.reconstructPath(
-        Debug.tokens, Debug.state.splits, style, debug = onlyOne) {
-      case (_, token, whitespace) =>
-        builder += FormatOutput(
-            token.left.code, whitespace, Debug.formatTokenExplored(token))
-    }
-    builder.result()
-  }
-
   override def afterAll(configMap: ConfigMap): Unit = {
     val splits =
       Debug.enqueuedSplits.groupBy(_.line.value).toVector.sortBy(-_._2.size)

core/src/test/scala/org/scalafmt/StripMarginTest.scala

@@ -1,9 +1,13 @@
 package org.scalafmt
 
 import org.scalafmt.util.DiffAssertions
+import org.scalafmt.util.DiffTest
 import org.scalafmt.util.HasTests
 import org.scalatest.FunSuite
 
+import scala.meta.Tree
+import scala.meta.parsers.common.Parse
+
 class StripMarginTest extends FunSuite with HasTests with DiffAssertions {
 
   val rawStrings = """
@@ -45,6 +49,27 @@ val x =
   """.replace("'''", "\"\"\"")
 
   val interpolatedStrings = """
+<<< tricky tempalate curly
+case class FormatterChangedAST(diff: String, output: String)
+    extends Error(s'''Formatter changed AST
+        |=====================
+        |$diff
+        |=====================
+        |${output.lines.toVector.mkString("\n")}
+        |=====================
+        |Formatter changed AST
+    '''.stripMargin)
+>>>
+case class FormatterChangedAST(diff: String, output: String)
+    extends Error(s'''Formatter changed AST
+                     |=====================
+                     |$diff
+                     |=====================
+                     |${output.lines.toVector.mkString("\n")}
+                     |=====================
+                     |Formatter changed AST
+    '''.stripMargin)
+
 <<< break indentation
 val msg =
 
@@ -82,12 +107,13 @@ val msg = s'''AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
     parseDiffTests(rawStrings, "stripMargin/String.stat") ++
     (parseDiffTests(interpolatedStrings, "stripMargin/Interpolate.stat"))
 
-  testsToRun.foreach { t =>
-    test(t.fullName) {
-      val parse = filename2parse(t.filename).get
-      val formatted =
-        ScalaFmt.format_!(t.original, ScalaStyle.StripMarginTest)(parse)
-      assertNoDiff(formatted, t.expected)
-    }
+  testsToRun.foreach(runTest(run))
+
+  def run(t: DiffTest, parse: Parse[_ <: Tree]): Unit = {
+    val parse = filename2parse(t.filename).get
+    val formatted =
+      ScalaFmt.format_!(t.original, ScalaStyle.StripMarginTest)(parse)
+    saveResult(t, formatted, t.only)
+    assertNoDiff(formatted, t.expected)
   }
 }