RedundantBraces: improve check around infix

It appears that the check around opening brace is not important but the check around closing brace should take into account whether there's a trailing comma or if the infix continues after the right brace.
scalameta · Dec 25, 2024 · 2d13fde · 2d13fde
1 parent b4a3afc
commit 2d13fde
Show file tree

Hide file tree

Showing 7 changed files with 69 additions and 60 deletions.
diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Router.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Router.scala
@@ -2503,7 +2503,13 @@ class Router(formatOps: FormatOps) {
         CtrlBodySplits.get(body)(null)(nlSplit)
 
       // After comment
-      case FT(_: T.Comment, _, _) => Seq(Split(getMod(ft), 0))
+      case FT(_: T.Comment, rt, _) =>
+        val noSplit = leftOwner match {
+          case p: Term.Block => rt.is[T.Comma] && isSingleStatBlock(p) &&
+            !isEnclosedWithinParensOrBraces(p)
+          case _ => false
+        }
+        Seq(Split(if (noSplit) NoSplit else getMod(ft), 0))
       // Inline comment
       case FT(_, _: T.Comment, _) =>
         val forceBlankLine = hasBreak() && blankLineBeforeDocstring(ft)
@@ -2737,8 +2743,8 @@ class Router(formatOps: FormatOps) {
       case FT(_: T.BOF, _, _) => splits
       case FT(_, _: T.Comment, _) if rhsIsCommentedOutIfComment(ft) =>
         splitsAsNewlines(splits.map(_.withNoIndent))
-      case FT(_: T.Comment, _, _) =>
-        if (ft.noBreak) splits else splitsAsNewlines(splits)
+      case FT(_: T.Comment, r, _) =>
+        if (ft.noBreak || r.is[T.Comma]) splits else splitsAsNewlines(splits)
       case FT(_, _: T.Comment, _) if hasBreakAfterRightBeforeNonComment(ft) =>
         if (ft.noBreak) splits.map(_.withMod(Space))
         else splitsAsNewlines(splits)

diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/rewrite/RedundantBraces.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/rewrite/RedundantBraces.scala
@@ -533,20 +533,31 @@ class RedundantBraces(implicit val ftoks: FormatTokens)
   )(implicit ft: FT, style: ScalafmtConfig): Boolean = p match {
     case _: Member.Infix =>
       /* for infix, we will preserve the block unless the closing brace
+       * is not followed by an operator of a further infix expression, or
        * follows a non-whitespace character on the same line as we don't
        * break lines around infix expressions.
        * we shouldn't join with the previous line (which might also end
        * in a comment), and if we keep the break before the right brace
        * we are removing, that will likely invalidate the expression. */
-      def checkOpen = {
-        val nft = ftoks.next(ft)
-        nft.noBreak || style.formatInfix(p) && !nft.right.is[T.Comment]
+      val rft = ftoks.matchingRight(ft)
+      def checkAfterRight(wasNonComment: => Boolean) = {
+        val nrft = ftoks.nextNonComment(rft)
+        !nrft.right.is[T.Ident] || wasNonComment && style.formatInfix(p) ||
+        findTreeWithParent(p) { // check if infix is in parens
+          case pp: Member.ArgClause => ftoks.getClosingIfWithinParensOrBraces(pp)
+              .map(_.isRight)
+          case _: Member.Infix => None
+          case _: Tree.Block => Some(false)
+          case pp => Some(ftoks.isEnclosedWithinParens(pp))
+        }.isDefined
       }
-      def checkClose = {
-        val nft = ftoks.prev(ftoks.matchingRight(ft))
-        nft.noBreak || style.formatInfix(p) && !nft.left.is[T.Comment]
+      val prft = ftoks.prev(rft)
+      prft.left match {
+        case _: T.Comma => RewriteTrailingCommas.enabled &&
+          checkAfterRight(!ftoks.prev(prft).left.is[T.Comment])
+        case _: T.Comment => prft.noBreak || checkAfterRight(false)
+        case _ => prft.noBreak || checkAfterRight(true)
       }
-      checkOpen && checkClose
     case _ => true
   }
 

diff --git a/scalafmt-tests/shared/src/test/resources/newlines/source_classic.stat b/scalafmt-tests/shared/src/test/resources/newlines/source_classic.stat
@@ -10687,11 +10687,10 @@ def isNeverSubType(tp1: Type, tp2: Type): Boolean = /*logResult(s"isNeverSubType
   tp2.dealias
 ) match {
   case (TypeRef(_, sym1, args1), TypeRef(_, sym2, args2)) =>
-    isNeverSubClass(sym1, sym2) || {
-      (sym1 isSubClass sym2) && {
-        val tp1seen = tp1 baseType sym2
-        isNeverSubArgs(tp1seen.typeArgs, args2, sym2.typeParams)
-      }
+    isNeverSubClass(sym1, sym2) ||
+    (sym1 isSubClass sym2) && {
+      val tp1seen = tp1 baseType sym2
+      isNeverSubArgs(tp1seen.typeArgs, args2, sym2.typeParams)
     }
   case _ => false
 }
@@ -10718,11 +10717,10 @@ override def isHidden(dia: Diagnostic)(using Context): Boolean =
   }
 >>>
 override def isHidden(dia: Diagnostic)(using Context): Boolean =
-  super.isHidden(dia) || {
+  super.isHidden(dia) ||
     dia.msg.isNonSensical &&
     hasErrors && // if there are no errors yet, report even if diagnostic is non-sensical
     !ctx.settings.YshowSuppressedErrors.value
-  }
 <<< #4133 redundant braces around inner infix
 maxColumn = 78
 rewrite.rules = [RedundantBraces, RedundantParens]
@@ -10738,10 +10736,9 @@ lazy val defaultSettings: Seq[Setting[_]] = Def.settings(
 >>>
 lazy val defaultSettings: Seq[Setting[_]] = Def.settings(
   Dependencies.Versions,
-  Compile / javacOptions ++= {
+  Compile / javacOptions ++=
     DefaultJavacOptions ++
-      JdkOptions.targetJdkJavacOptions(targetSystemJdk.value)
-  },
+      JdkOptions.targetJdkJavacOptions(targetSystemJdk.value),
   resolverSettings
 )
 <<< #4133 redundant braces with overflow

diff --git a/scalafmt-tests/shared/src/test/resources/newlines/source_keep.stat b/scalafmt-tests/shared/src/test/resources/newlines/source_keep.stat
@@ -10440,11 +10440,10 @@ def isNeverSubType(tp1: Type, tp2: Type): Boolean = /*logResult(s"isNeverSubType
 def isNeverSubType(tp1: Type, tp2: Type): Boolean = /*logResult(s"isNeverSubType($tp1, $tp2)")*/
   (tp1.dealias, tp2.dealias) match {
     case (TypeRef(_, sym1, args1), TypeRef(_, sym2, args2)) =>
-      isNeverSubClass(sym1, sym2) || {
-        (sym1 isSubClass sym2) && {
-          val tp1seen = tp1 baseType sym2
-          isNeverSubArgs(tp1seen.typeArgs, args2, sym2.typeParams)
-        }
+      isNeverSubClass(sym1, sym2) ||
+      (sym1 isSubClass sym2) && {
+        val tp1seen = tp1 baseType sym2
+        isNeverSubArgs(tp1seen.typeArgs, args2, sym2.typeParams)
       }
     case _ => false
   }
@@ -10471,11 +10470,10 @@ override def isHidden(dia: Diagnostic)(using Context): Boolean =
   }
 >>>
 override def isHidden(dia: Diagnostic)(using Context): Boolean =
-  super.isHidden(dia) || {
+  super.isHidden(dia) ||
     dia.msg.isNonSensical &&
     hasErrors && // if there are no errors yet, report even if diagnostic is non-sensical
     !ctx.settings.YshowSuppressedErrors.value
-  }
 <<< #4133 redundant braces around inner infix
 maxColumn = 78
 rewrite.rules = [RedundantBraces, RedundantParens]
@@ -10491,10 +10489,9 @@ lazy val defaultSettings: Seq[Setting[_]] = Def.settings(
 >>>
 lazy val defaultSettings: Seq[Setting[_]] = Def.settings(
   Dependencies.Versions,
-  Compile / javacOptions ++= {
+  Compile / javacOptions ++=
     DefaultJavacOptions ++
-      JdkOptions.targetJdkJavacOptions(targetSystemJdk.value)
-  },
+      JdkOptions.targetJdkJavacOptions(targetSystemJdk.value),
   resolverSettings
 )
 <<< #4133 redundant braces with overflow

diff --git a/scalafmt-tests/shared/src/test/resources/rewrite/RedundantBraces-ParenLambdas.stat b/scalafmt-tests/shared/src/test/resources/rewrite/RedundantBraces-ParenLambdas.stat
@@ -279,15 +279,14 @@ object a {
   lowClass.isJavaDefined && highClass.isJavaDefined && { // skip if both are java-defined, and
     lowClass.isNonBottomSubClass(
       highClass
-    ) || { //  - low <:< high, which means they are overrides in Java and javac is doing the check; or
-      base.info.parents.tail.forall {
-        p => //  - every mixin parent is unrelated to (not a subclass of) low and high, i.e.,
-          val psym =
-            p.typeSymbol //    we're not mixing in high or low, both are coming from the superclass
-          !psym.isNonBottomSubClass(lowClass) && !psym.isNonBottomSubClass(
-            highClass
-          )
-      }
+    ) || //  - low <:< high, which means they are overrides in Java and javac is doing the check; or
+    base.info.parents.tail.forall {
+      p => //  - every mixin parent is unrelated to (not a subclass of) low and high, i.e.,
+        val psym =
+          p.typeSymbol //    we're not mixing in high or low, both are coming from the superclass
+        !psym.isNonBottomSubClass(lowClass) && !psym.isNonBottomSubClass(
+          highClass
+        )
     }
   }
 }
@@ -307,13 +306,12 @@ object a {
 >>>
 object a {
   lowClass.isJavaDefined && highClass.isJavaDefined && {
-    lowClass.isNonBottomSubClass(highClass) || {
-      base.info.parents.tail.forall { p =>
-        val psym = p.typeSymbol
-        !psym.isNonBottomSubClass(lowClass) && !psym.isNonBottomSubClass(
-          highClass
-        )
-      }
+    lowClass.isNonBottomSubClass(highClass) ||
+    base.info.parents.tail.forall { p =>
+      val psym = p.typeSymbol
+      !psym.isNonBottomSubClass(lowClass) && !psym.isNonBottomSubClass(
+        highClass
+      )
     }
   }
 }
diff --git a/scalafmt-tests/shared/src/test/resources/rewrite/RedundantBraces2.stat b/scalafmt-tests/shared/src/test/resources/rewrite/RedundantBraces2.stat
@@ -458,10 +458,9 @@ Def.settings(
 )
 >>>
 Def.settings(
-  saveForStabilityTest / artifactPath := {
+  saveForStabilityTest / artifactPath :=
     // this path intentionally survives a `clean`
-    (LocalRootProject / baseDirectory).value / "test-suite/target/test-suite-stability.js" /* c */
-  },
+    (LocalRootProject / baseDirectory).value / "test-suite/target/test-suite-stability.js" /* c */,
   saveForStabilityTest := foo
 )
 <<< braces at end of infix expressions
@@ -480,13 +479,14 @@ Switch(typeof(instance), List(
     } + 1
 ))
 >>>
-Idempotency violated
-=> Diff (- obtained, + expected)
-   List(
--    str("string") -> Return(constantClassResult(BoxedStringClass)),
--    str("boolean") -> Return(constantClassResult(BoxedBooleanClass)),
--    str("undefined") -> Return(constantClassResult(BoxedUnitClass)) + 1
-+    str("string") -> { Return(constantClassResult(BoxedStringClass)) },
-+    str("boolean") -> { Return(constantClassResult(BoxedBooleanClass)) },
-+    str("undefined") -> { Return(constantClassResult(BoxedUnitClass)) } + 1
-   )
+Switch(
+  typeof(instance),
+  List(
+    str("string") ->
+      Return(constantClassResult(BoxedStringClass)),
+    str("boolean") ->
+      Return(constantClassResult(BoxedBooleanClass)),
+    str("undefined") ->
+      Return(constantClassResult(BoxedUnitClass)) + 1
+  )
+)
diff --git a/scalafmt-tests/shared/src/test/scala/org/scalafmt/FormatTests.scala b/scalafmt-tests/shared/src/test/scala/org/scalafmt/FormatTests.scala
@@ -144,7 +144,7 @@ class FormatTests extends FunSuite with CanRunTests with FormatAssertions {
     val explored = Debug.explored.get()
     logger.debug(s"Total explored: $explored")
     if (!onlyUnit && !onlyManual)
-      assertEquals(explored, 1204857, "total explored")
+      assertEquals(explored, 1204816, "total explored")
     val results = debugResults.result()
     // TODO(olafur) don't block printing out test results.
     // I don't want to deal with scalaz's Tasks :'(