Repository: scalameta/scalameta (scala)

Issue:
ScalametaParser: pos for trailing comments on multi-line expression inconsistent from one dialect to another
```scala
import scala.meta._

val program =
"""
object Main extends App {
  val a = 1 // comment attached to Defn.Val on all dialects
  val b =
    1 // comment attached to Defn.Val on Scala 3 dialect only
}
"""

{
  println("Scala213")
  implicit val scala213: Dialect = dialects.Scala213
  println(program.parse[Stat].get.collect { case v: Defn.Val => v }.map(_.pos))
  println()
}

{
  println("Scala3")
  implicit val scala3: Dialect = dialects.Scala3
  println(program.parse[Stat].get.collect { case v: Defn.Val => v }.map(_.pos))
  println()
}
```

```diff
---Scala213
+++Scala3
 List([29..38) in Input.String("
 object Main extends App {
   val a = 1 // comment attached to Defn.Val on all dialects
   val b =
     1 // comment attached to Defn.Val on Scala 3 dialect only
 }
-"), [89..102) in Input.String("
+"), [89..158) in Input.String("
 object Main extends App {
   val a = 1 // comment attached to Defn.Val on all dialects
   val b =
     1 // comment attached to Defn.Val on Scala 3 dialect only
 }
 "))
```

This makes cross-dialect usage of `AssociatedComments` hard.

I don't have any opinion on what is the right behavior. FWIW,
- the behavior for the Scala213 dialect hasn't changed since at least 4.2.0
- the behavior for the Scala3 dialect hasn't changed since [4.4.33](https://github.com/scalameta/scalameta/releases/tag/v4.4.33) (before that, Scala3 had the same behavior as Scala213)

I found this inconsistency while trying to port a Scala2 Scalafix test to Scala3. I am not aware of any reported problem in production, so "won't fix" is definitely a reasonable outcome for this issue.

Pull request description:
Also, retain a significant-indentation block if it ends in a detached comment, as otherwise the comment's indentation would be lost. Fixes #3689.

Patch:
diff --git a/scalameta/parsers/shared/src/main/scala/scala/meta/internal/parsers/ScalametaParser.scala b/scalameta/parsers/shared/src/main/scala/scala/meta/internal/parsers/ScalametaParser.scala
index 386c8296f2..9eb0d2f17c 100644
--- a/scalameta/parsers/shared/src/main/scala/scala/meta/internal/parsers/ScalametaParser.scala
+++ b/scalameta/parsers/shared/src/main/scala/scala/meta/internal/parsers/ScalametaParser.scala
@@ -2331,9 +2331,10 @@ class ScalametaParser(input: Input)(implicit dialect: Dialect) {
   private def blockRaw(allowRepeated: Boolean = false): Term.Block =
     toBlockRaw(blockStatSeq(allowRepeated = allowRepeated))
 
-  private def blockOnIndent(keepBlock: Boolean = false): Term = autoPos {
+  private def blockOnIndent(keepBlock: Boolean = false): Term = autoPosOpt {
     indentedOnOpen(blockStatSeq() match {
-      case (t: Term) :: Nil if !keepBlock => t
+      case (t: Term) :: Nil
+          if !(keepBlock || isPrecededByDetachedComment(tokenPos, t.endTokenPos)) => t
       case stats => toBlockRaw(stats)
     })
   }
@@ -2348,7 +2349,7 @@ class ScalametaParser(input: Input)(implicit dialect: Dialect) {
       if (isOptional) blockOnOther(allowRepeated) else blockOnBrace(allowRepeated)
     }
 
-  private def blockOnOther(allowRepeated: Boolean = false): Term = autoPos {
+  private def blockOnOther(allowRepeated: Boolean = false): Term = autoPosOpt {
     blockStatSeq(allowRepeated = allowRepeated) match {
       case (term: Term) :: Nil => term
       case stats => toBlockRaw(stats)
@@ -3474,7 +3475,7 @@ class ScalametaParser(input: Input)(implicit dialect: Dialect) {
     val body: Stat = token match {
       case _: LeftBrace => blockOnBrace(getStats())
       case _: Indentation.Indent => autoPosOpt(indentedOnOpen(getStats() match {
-          case stat :: Nil => stat
+          case t :: Nil if !isPrecededByDetachedComment(tokenPos, t.endTokenPos) => t
           case stats => toBlockRaw(stats)
         }))
       case _ if isDefIntro(tokenPos) => nonLocalDefOrDcl()
diff --git a/scalameta/parsers/shared/src/main/scala/scala/meta/internal/parsers/ScannerTokens.scala b/scalameta/parsers/shared/src/main/scala/scala/meta/internal/parsers/ScannerTokens.scala
index 484629d3af..98b63c005a 100644
--- a/scalameta/parsers/shared/src/main/scala/scala/meta/internal/parsers/ScannerTokens.scala
+++ b/scalameta/parsers/shared/src/main/scala/scala/meta/internal/parsers/ScannerTokens.scala
@@ -92,6 +92,14 @@ final class ScannerTokens(val tokens: Tokens)(implicit dialect: Dialect) {
   @inline
   private def isFollowedByNL(index: Int): Boolean = tokens(getStrictNext(index)).is[AtEOLorF]
 
+  @tailrec
+  final def isPrecededByDetachedComment(idx: Int, end: Int): Boolean = idx > end &&
+    (tokens(idx) match {
+      case _: Comment => isPrecededByNL(idx)
+      case _: Whitespace => isPrecededByDetachedComment(idx - 1, end)
+      case _ => false
+    })
+
   @inline
   private def isEndMarkerIdentifier(token: Token) = soft.KwEnd(token)
 


Test patch:
diff --git a/tests/shared/src/test/scala-2.13/scala/meta/tests/parsers/dotty/Scala3PositionSuite.scala b/tests/shared/src/test/scala-2.13/scala/meta/tests/parsers/dotty/Scala3PositionSuite.scala
index 53f4a25405..32b905484e 100644
--- a/tests/shared/src/test/scala-2.13/scala/meta/tests/parsers/dotty/Scala3PositionSuite.scala
+++ b/tests/shared/src/test/scala-2.13/scala/meta/tests/parsers/dotty/Scala3PositionSuite.scala
@@ -705,9 +705,9 @@ class Scala3PositionSuite extends BasePositionSuite(dialects.Scala3) {
        |    else
        |      gx
        |      // c2
-       |Term.Name fx
+       |Term.Block fx
        |      // c1
-       |Term.Name gx
+       |Term.Block gx
        |      // c2
        |""".stripMargin
   )
@@ -738,13 +738,18 @@ class Scala3PositionSuite extends BasePositionSuite(dialects.Scala3) {
        |    else
        |      gx
        |    // c2
-       |Term.If if cond then
+       |Term.Block if cond then
        |      fx
        |      // c1
        |    else
        |      gx
        |    // c2
-       |Term.Name fx
+       |Term.If if cond then
+       |      fx
+       |      // c1
+       |    else
+       |      gx
+       |Term.Block fx
        |      // c1
        |""".stripMargin
   )
@@ -782,9 +787,9 @@ class Scala3PositionSuite extends BasePositionSuite(dialects.Scala3) {
        |    else
        |      gx
        |      // c2
-       |Term.Name fx
+       |Term.Block fx
        |      // c1
-       |Term.Name gx
+       |Term.Block gx
        |      // c2
        |""".stripMargin
   )
@@ -816,13 +821,18 @@ class Scala3PositionSuite extends BasePositionSuite(dialects.Scala3) {
        |    else
        |      gx
        |    // c2
-       |Term.If if cond then
+       |Term.Block if cond then
        |      fx
        |      // c1
        |    else
        |      gx
        |    // c2
-       |Term.Name fx
+       |Term.If if cond then
+       |      fx
+       |      // c1
+       |    else
+       |      gx
+       |Term.Block fx
        |      // c1
        |""".stripMargin
   )
@@ -854,13 +864,18 @@ class Scala3PositionSuite extends BasePositionSuite(dialects.Scala3) {
        |    else
        |      gx
        |    // c2
-       |Term.If if cond then
+       |Term.Block if cond then
        |      fx
        |      // c1
        |    else
        |      gx
        |    // c2
-       |Term.Name fx
+       |Term.If if cond then
+       |      fx
+       |      // c1
+       |    else
+       |      gx
+       |Term.Block fx
        |      // c1
        |""".stripMargin
   )
@@ -890,13 +905,18 @@ class Scala3PositionSuite extends BasePositionSuite(dialects.Scala3) {
        |    else
        |      gx
        |    /* c2 */
-       |Term.If if cond then
+       |Term.Block if cond then
        |      fx
        |      // c1
        |    else
        |      gx
        |    /* c2 */
-       |Term.Name fx
+       |Term.If if cond then
+       |      fx
+       |      // c1
+       |    else
+       |      gx
+       |Term.Block fx
        |      // c1
        |""".stripMargin
   )
@@ -1128,4 +1148,282 @@ class Scala3PositionSuite extends BasePositionSuite(dialects.Scala3) {
        |""".stripMargin
   )
 
+  // #3689 1
+  checkPositions[Term](
+    """|{
+       |  val a = 1 // comment
+       |}
+       |""".stripMargin,
+    """|Defn.Val val a = 1
+       |""".stripMargin
+  )
+
+  // #3689 2
+  checkPositions[Term](
+    """|{
+       |  val b =
+       |    1 // comment
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b =
+       |    1 // comment
+       |""".stripMargin
+  )
+
+  // #3689 2.1
+  checkPositions[Term](
+    """|{
+       |  val b = {
+       |    1 // comment
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = {
+       |    1 // comment
+       |  }
+       |Term.Block {
+       |    1 // comment
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 3
+  checkPositions[Term](
+    """|{
+       |  val b = // comment1
+       |    1 // comment2
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = // comment1
+       |    1 // comment2
+       |""".stripMargin
+  )
+
+  // #3689 3.1
+  checkPositions[Term](
+    """|{
+       |  val b = /* comment1 */ {
+       |    1 // comment2
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = /* comment1 */ {
+       |    1 // comment2
+       |  }
+       |Term.Block {
+       |    1 // comment2
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 4
+  checkPositions[Term](
+    """|{
+       |  val b =
+       |    // comment1
+       |    1 // comment2
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b =
+       |    // comment1
+       |    1 // comment2
+       |""".stripMargin
+  )
+
+  // #3689 4.1
+  checkPositions[Term](
+    """|{
+       |  val b = {
+       |    // comment1
+       |    1 // comment2
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = {
+       |    // comment1
+       |    1 // comment2
+       |  }
+       |Term.Block {
+       |    // comment1
+       |    1 // comment2
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 5
+  checkPositions[Term](
+    """|{
+       |  val b = // comment
+       |    1
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = // comment
+       |    1
+       |""".stripMargin
+  )
+
+  // #3689 5.1
+  checkPositions[Term](
+    """|{
+       |  val b = /* comment */ {
+       |    1
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = /* comment */ {
+       |    1
+       |  }
+       |Term.Block {
+       |    1
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 6
+  checkPositions[Term](
+    """|{
+       |  val b =
+       |    // comment
+       |    1
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b =
+       |    // comment
+       |    1
+       |""".stripMargin
+  )
+
+  // #3689 6.1
+  checkPositions[Term](
+    """|{
+       |  val b = {
+       |    // comment
+       |    1
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = {
+       |    // comment
+       |    1
+       |  }
+       |Term.Block {
+       |    // comment
+       |    1
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 7
+  checkPositions[Term](
+    """|{
+       |  val b =
+       |    1
+       |    // comment
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b =
+       |    1
+       |    // comment
+       |Term.Block 1
+       |    // comment
+       |""".stripMargin
+  )
+
+  // #3689 7.1
+  checkPositions[Term](
+    """|{
+       |  val b = {
+       |    1
+       |    // comment
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = {
+       |    1
+       |    // comment
+       |  }
+       |Term.Block {
+       |    1
+       |    // comment
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 8
+  checkPositions[Term](
+    """|{
+       |  val b = // comment1
+       |    1
+       |    // comment2
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = // comment1
+       |    1
+       |    // comment2
+       |Term.Block 1
+       |    // comment2
+       |""".stripMargin
+  )
+
+  // #3689 8.1
+  checkPositions[Term](
+    """|{
+       |  val b = /* comment1 */ {
+       |    1
+       |    // comment2
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = /* comment1 */ {
+       |    1
+       |    // comment2
+       |  }
+       |Term.Block {
+       |    1
+       |    // comment2
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 9
+  checkPositions[Term](
+    """|{
+       |  val b =
+       |    // comment1
+       |    1
+       |    // comment2
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b =
+       |    // comment1
+       |    1
+       |    // comment2
+       |Term.Block 1
+       |    // comment2
+       |""".stripMargin
+  )
+
+  // #3689 9.1
+  checkPositions[Term](
+    """|{
+       |  val b = {
+       |    // comment1
+       |    1
+       |    // comment2
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = {
+       |    // comment1
+       |    1
+       |    // comment2
+       |  }
+       |Term.Block {
+       |    // comment1
+       |    1
+       |    // comment2
+       |  }
+       |""".stripMargin
+  )
+
 }
diff --git a/tests/shared/src/test/scala-2.13/scala/meta/tests/tokenizers/TokensPositionSuite.scala b/tests/shared/src/test/scala-2.13/scala/meta/tests/tokenizers/TokensPositionSuite.scala
index 525d2613e4..d2df2ffd36 100644
--- a/tests/shared/src/test/scala-2.13/scala/meta/tests/tokenizers/TokensPositionSuite.scala
+++ b/tests/shared/src/test/scala-2.13/scala/meta/tests/tokenizers/TokensPositionSuite.scala
@@ -804,4 +804,274 @@ class TokensPositionSuite extends BasePositionSuite(dialects.Scala213) {
        |Type.ArgClause (10) + @@1 toInt
        |""".stripMargin
   )
+
+  // #3689 1
+  checkPositions[Term](
+    """|{
+       |  val a = 1 // comment
+       |}
+       |""".stripMargin,
+    """|Defn.Val val a = 1
+       |""".stripMargin
+  )
+
+  // #3689 2
+  checkPositions[Term](
+    """|{
+       |  val b =
+       |    1 // comment
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b =
+       |    1
+       |""".stripMargin
+  )
+
+  // #3689 2.1
+  checkPositions[Term](
+    """|{
+       |  val b = {
+       |    1 // comment
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = {
+       |    1 // comment
+       |  }
+       |Term.Block {
+       |    1 // comment
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 3
+  checkPositions[Term](
+    """|{
+       |  val b = // comment1
+       |    1 // comment2
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = // comment1
+       |    1
+       |""".stripMargin
+  )
+
+  // #3689 3.1
+  checkPositions[Term](
+    """|{
+       |  val b = /* comment1 */ {
+       |    1 // comment2
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = /* comment1 */ {
+       |    1 // comment2
+       |  }
+       |Term.Block {
+       |    1 // comment2
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 4
+  checkPositions[Term](
+    """|{
+       |  val b =
+       |    // comment1
+       |    1 // comment2
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b =
+       |    // comment1
+       |    1
+       |""".stripMargin
+  )
+
+  // #3689 4.1
+  checkPositions[Term](
+    """|{
+       |  val b = {
+       |    // comment1
+       |    1 // comment2
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = {
+       |    // comment1
+       |    1 // comment2
+       |  }
+       |Term.Block {
+       |    // comment1
+       |    1 // comment2
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 5
+  checkPositions[Term](
+    """|{
+       |  val b = // comment
+       |    1
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = // comment
+       |    1
+       |""".stripMargin
+  )
+
+  // #3689 5.1
+  checkPositions[Term](
+    """|{
+       |  val b = /* comment */ {
+       |    1
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = /* comment */ {
+       |    1
+       |  }
+       |Term.Block {
+       |    1
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 6
+  checkPositions[Term](
+    """|{
+       |  val b =
+       |    // comment
+       |    1
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b =
+       |    // comment
+       |    1
+       |""".stripMargin
+  )
+
+  // #3689 6.1
+  checkPositions[Term](
+    """|{
+       |  val b = {
+       |    // comment
+       |    1
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = {
+       |    // comment
+       |    1
+       |  }
+       |Term.Block {
+       |    // comment
+       |    1
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 7
+  checkPositions[Term](
+    """|{
+       |  val b =
+       |    1
+       |    // comment
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b =
+       |    1
+       |""".stripMargin
+  )
+
+  // #3689 7.1
+  checkPositions[Term](
+    """|{
+       |  val b = {
+       |    1
+       |    // comment
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = {
+       |    1
+       |    // comment
+       |  }
+       |Term.Block {
+       |    1
+       |    // comment
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 8
+  checkPositions[Term](
+    """|{
+       |  val b = // comment1
+       |    1
+       |    // comment2
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = // comment1
+       |    1
+       |""".stripMargin
+  )
+
+  // #3689 8.1
+  checkPositions[Term](
+    """|{
+       |  val b = /* comment1 */ {
+       |    1
+       |    // comment2
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = /* comment1 */ {
+       |    1
+       |    // comment2
+       |  }
+       |Term.Block {
+       |    1
+       |    // comment2
+       |  }
+       |""".stripMargin
+  )
+
+  // #3689 9
+  checkPositions[Term](
+    """|{
+       |  val b =
+       |    // comment1
+       |    1
+       |    // comment2
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b =
+       |    // comment1
+       |    1
+       |""".stripMargin
+  )
+
+  // #3689 9.1
+  checkPositions[Term](
+    """|{
+       |  val b = {
+       |    // comment1
+       |    1
+       |    // comment2
+       |  }
+       |}
+       |""".stripMargin,
+    """|Defn.Val val b = {
+       |    // comment1
+       |    1
+       |    // comment2
+       |  }
+       |Term.Block {
+       |    // comment1
+       |    1
+       |    // comment2
+       |  }
+       |""".stripMargin
+  )
+
 }
diff --git a/tests/shared/src/test/scala/scala/meta/tests/parsers/dotty/ExtensionMethodsSuite.scala b/tests/shared/src/test/scala/scala/meta/tests/parsers/dotty/ExtensionMethodsSuite.scala
index 862ff95b03..64ffb9f7cf 100644
--- a/tests/shared/src/test/scala/scala/meta/tests/parsers/dotty/ExtensionMethodsSuite.scala
+++ b/tests/shared/src/test/scala/scala/meta/tests/parsers/dotty/ExtensionMethodsSuite.scala
@@ -170,18 +170,20 @@ class ExtensionMethodsSuite extends BaseDottySuite {
          |    
          |    /** */
          |""".stripMargin,
-      assertLayout = Some("extension (a: Int) def double = a * 2")
+      """|extension (a: Int) {
+         |  def double = a * 2
+         |}""".stripMargin
     )(Defn.ExtensionGroup(
       Nil,
       List(List(tparam("a", "Int"))),
-      Defn.Def(
+      blk(Defn.Def(
         Nil,
         tname("double"),
         Nil,
         Nil,
         None,
         Term.ApplyInfix(tname("a"), tname("*"), Nil, List(int(2)))
-      )
+      ))
     ))
   }