From b6b659cb949a27e29548ca5c8c5cf030bb97899b Mon Sep 17 00:00:00 2001 From: Albert Meltzer <7529386+kitbellew@users.noreply.github.com> Date: Sat, 11 May 2024 13:28:26 -0700 Subject: [PATCH] FormatOps: fix scala.js style implementation The `scalaJs` modification of `ScalafmtConfig` is, among other things, defined as having config style enabled and dangling parens disabled. Earlier, we accidentally swapped the two flag values, so let's fix it. --- docs/configuration.md | 62 ++++++++++++++----- .../org/scalafmt/config/ScalafmtConfig.scala | 6 +- .../org/scalafmt/internal/FormatOps.scala | 2 +- .../scala/org/scalafmt/internal/Router.scala | 6 +- .../test/resources/binPack/TermNameList.stat | 6 +- .../src/test/resources/default/Apply.stat | 21 ++++--- .../resources/newlines/source_classic.stat | 32 +++++----- .../src/test/resources/scalajs/Apply.stat | 58 +++++++++-------- .../src/test/resources/scalajs/Class.stat | 4 +- .../src/test/resources/scalajs/DefDef.stat | 11 +++- .../trailing-commas/trailingCommasAlways.stat | 6 +- .../trailingCommasAlwaysComments.stat | 20 +++--- .../trailingCommasMultiple.stat | 3 +- .../trailingCommasMultipleComments.stat | 20 +++--- 14 files changed, 157 insertions(+), 100 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 02ecf092f1..d132c2e0ec 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -185,9 +185,13 @@ top-level. ### Top-level presets -- `preset=default`: this preset is implicit and sets all values to their - defaults. -- `preset=IntelliJ`: this preset is defined as +#### `preset=default` + +This preset is implicit and sets all values to their defaults. + +#### `preset=IntelliJ` + +This preset is defined as ``` preset = default @@ -195,30 +199,59 @@ top-level. optIn.configStyleArguments = false ``` -- `preset=defaultWithAlign`: this preset is defined as +#### `preset=defaultWithAlign` + +This preset is defined as ``` preset = default align.preset = more ``` -- `preset=Scala.js`: this preset is defined as +#### `preset=Scala.js` + +This preset intends to approximate the +[style used for `scala.js`](https://github.com/scala-js/scala-js/blob/main/CODINGSTYLE.md). + +It uses modified detection of [config-style formatting](#newlines-config-style-formatting): + +- [according to Sébastien Doeraene](https://github.com/scala-js/scala-js/pull/4522#issuecomment-879168123), + config-style should be driven solely by presence of a dangling closing parenthesis +- to achieve that, use a combination of + [`danglingParentheses.xxxSite = false`](#danglingparenthesescallsite) and + [`newlines.configStyleXxxSite.prefer = true`](#newlinesconfigstylexxxsiteprefer) + +The preset itself is defined as: ``` preset = default binPack.preset = true - align.openParenCtrlSite = false - indent.callSite = 4 + align { + openParenCtrlSite = false + arrowEnumeratorGenerator = false + tokens = [ caseArrow ] + } + danglingParentheses { + callSite = false + defnSite = false + } docstrings.style = Asterisk importSelectors = binPack + indent.callSite = 4 newlines { - neverInResultType = true + avoidInResultType = true neverBeforeJsNative = true sometimesBeforeColonInMethodReturnType = false } - runner.optimizer.callSite { - minSpan = 500 - minCount = 5 + runner.optimizer { + callSite { + minSpan = 500 + minCount = 5 + } + defnSite { + minSpan = 500 + minCount = 5 + } } ``` @@ -4917,10 +4950,9 @@ and similarly has cross-parameter interactions: over binpacking - for `newlines.source=classic`, behaviour depends on [config-style](#newlinesconfigstylexxxsiteprefer): - - if enabled: used if [detected](#newlines-config-style-formatting), otherwise binpacked - - if disabled with both [`danglingParentheses.callSite`](#danglingparenthesescallsite) - enabled and closing parenthesis following a break: forces config-style, as described in - [scala.js](https://github.com/scala-js/scala-js/pull/4522#issuecomment-879168123) + - if enabled, config style is used if + - it is [detected](#newlines-config-style-formatting), or + - configured to use [scala.js style](#presetscalajs) - otherwise, uses binpacking - for other values of [`newlines.source`](#newlinessource), binpacking takes precedence diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/config/ScalafmtConfig.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/config/ScalafmtConfig.scala index da0e79ac75..e466bf5f76 100644 --- a/scalafmt-core/shared/src/main/scala/org/scalafmt/config/ScalafmtConfig.scala +++ b/scalafmt-core/shared/src/main/scala/org/scalafmt/config/ScalafmtConfig.scala @@ -316,11 +316,7 @@ object ScalafmtConfig { * https://github.com/scala-js/scala-js/blob/master/CODINGSTYLE.md */ val scalaJs: ScalafmtConfig = default.copy( - binPack = BinPack( - defnSite = BinPack.Site.Always, - callSite = BinPack.Site.Always, - parentConstructors = BinPack.ParentCtors.Always, - ), + binPack = BinPack.enabled, danglingParentheses = DanglingParentheses(false, false), indent = Indents(callSite = 4, defnSite = 4), importSelectors = ImportSelectors.binPack, diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala index 31d010181d..59c42a331a 100644 --- a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala +++ b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala @@ -2653,7 +2653,7 @@ class FormatOps( mustUseConfigStyle(ftAfterOpen, ftBeforeClose.hasBreak, !literalArgList) val scalaJsStyle = style.newlines.source == Newlines.classic && configStyle == ConfigStyle.None && !literalArgList && - clauseSiteFlags.dangleCloseDelim && !clauseSiteFlags.configStyle.prefer + !clauseSiteFlags.dangleCloseDelim && clauseSiteFlags.configStyle.prefer BinpackCallsiteFlags( literalArgList = literalArgList, dangleForTrailingCommas = dangleForTrailingCommas, 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 index 921d022fed..522752e2ab 100644 --- a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Router.scala +++ b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Router.scala @@ -1183,10 +1183,10 @@ class Router(formatOps: FormatOps) { val singleLineOnly = style.binPack.literalsSingleLine && flags.literalArgList - val scalaJsStyleNL = flags.scalaJsStyle && beforeClose.hasBreak val nlOpen = flags.dangleForTrailingCommas || flags.configStyle != ConfigStyle.None || - style.newlines.keepBreak(newlines) || scalaJsStyleNL || + style.newlines.keepBreak(newlines) || + flags.scalaJsStyle && beforeClose.hasBreak || tokens.isRightCommentWithBreak(ft) val nlOnly = nlOpen && !singleLineOnly @@ -1306,7 +1306,7 @@ class Router(formatOps: FormatOps) { styleMap.forcedBinPack(leftOwner) ) bothPolicies else configStylePolicy - else if (scalaJsStyleNL) configStylePolicy + else if (flags.scalaJsStyle) configStylePolicy else if ( flags.dangleForTrailingCommas || clauseSiteFlags.dangleCloseDelim && diff --git a/scalafmt-tests/src/test/resources/binPack/TermNameList.stat b/scalafmt-tests/src/test/resources/binPack/TermNameList.stat index 753bc806c1..54fbd64146 100644 --- a/scalafmt-tests/src/test/resources/binPack/TermNameList.stat +++ b/scalafmt-tests/src/test/resources/binPack/TermNameList.stat @@ -88,7 +88,8 @@ somethingVeryLong( baz( qux // c1 ) - }) + } +) <<< unsafeCallSite forced newline, !cfg + dangle maxColumn = 20 binPack.unsafeCallSite = true @@ -102,8 +103,7 @@ somethingVeryLong(bar{baz( >>> somethingVeryLong( bar { - baz( - qux // c1 + baz(qux // c1 ) } ) diff --git a/scalafmt-tests/src/test/resources/default/Apply.stat b/scalafmt-tests/src/test/resources/default/Apply.stat index 731942e712..1db7d7c898 100644 --- a/scalafmt-tests/src/test/resources/default/Apply.stat +++ b/scalafmt-tests/src/test/resources/default/Apply.stat @@ -1712,13 +1712,13 @@ object a { } >>> object a { - val cls = Select( - Select( - Select( - Select(Select(Select(Ident(nme.ROOTPKG), nme.scala_), scalajs), js), - nme.annotation, x, y, z), - internal_, a, b, c), - wasPublicBeforeTyperXxx) + val cls = + Select(Select(Select(Select(Select(Select(Ident(nme.ROOTPKG), nme.scala_), + scalajs), + js), + nme.annotation, x, y, z), + internal_, a, b, c), + wasPublicBeforeTyperXxx) } <<< #2633 with binPack always maxColumn = 70 @@ -1744,7 +1744,8 @@ object a { .getAnnotation(JSNameAnnotation).fold { sym.addAnnotation(JSNameAnnotation, Literal(Constant(jsInterop.defaultJSNameOf( - symForName)))) + symForName + )))) } { annot => sym.addAnnotation(annot) } @@ -1773,8 +1774,8 @@ object a { symForName .getAnnotation(JSNameAnnotation).fold { sym.addAnnotation(JSNameAnnotation, - Literal(Constant( - jsInterop.defaultJSNameOf(symForName)))) + Literal(Constant(jsInterop + .defaultJSNameOf(symForName)))) } { annot => sym.addAnnotation(annot) } diff --git a/scalafmt-tests/src/test/resources/newlines/source_classic.stat b/scalafmt-tests/src/test/resources/newlines/source_classic.stat index 822023457b..d3ab56f344 100644 --- a/scalafmt-tests/src/test/resources/newlines/source_classic.stat +++ b/scalafmt-tests/src/test/resources/newlines/source_classic.stat @@ -2801,7 +2801,10 @@ object a { test("foo") { a.b(c, d) shouldBe E(Seq(F(1, "v1"), F(2, "v2")), - G(Some(Seq(h, i)), Some(Seq(j, k)), a.b, c.d, e.f.g, h.i.j)).foo + G(Some(Seq(h, i)), + Some(Seq(j, k)), a.b, c.d, + e.f.g, h.i.j) + ).foo } } <<< binpack call, oneline, with syntaxNL, single arg @@ -2889,7 +2892,8 @@ object a { object a { val foo = bar.map(x => x.copy( - baz = Option.when(false)(x.qux))) + baz = Option.when(false)(x.qux) + )) } <<< binPack with named parameter values, !configStyleArguments + danglingParentheses binPack.preset = true @@ -5063,7 +5067,8 @@ object a { >>> object a { def foo(bb: BB, cc: CC, dd: DD = DDD.ddd): Bar[ - Baz] = { + Baz + ] = { // c qux } @@ -7796,18 +7801,12 @@ object Main { } >>> object Main { - val bar1 = foo1( - 10000, - 10001, - 10002 + 0 - ) val bar1 = foo1(10000, 10001, 10002 + 0) - val bar1 = foo1( - 10000, - 10001, - 10002 + 0 - ) + val bar1 = foo1(10000, + 10001, 10002 + 0) + val bar1 = foo1(10000, + 10001, 10002 + 0) val bar1 = foo1(10000, 10001, 10002 + 0) val bar2 = foo2(0, 1, 2, 3, @@ -7898,8 +7897,11 @@ object Main { ) val bar1 = foo1(10000, 10001, 10002 + 0) - val bar1 = foo1(10000, - 10001, 10002 + 0) + val bar1 = foo1( + 10000, + 10001, + 10002 + 0 + ) val bar1 = foo1(10000, 10001, 10002 + 0) val bar2 = foo2( diff --git a/scalafmt-tests/src/test/resources/scalajs/Apply.stat b/scalafmt-tests/src/test/resources/scalajs/Apply.stat index 5be3dffde3..4a66ecbdbd 100644 --- a/scalafmt-tests/src/test/resources/scalajs/Apply.stat +++ b/scalafmt-tests/src/test/resources/scalajs/Apply.stat @@ -201,8 +201,8 @@ object a { } >>> object a { - new SimpleMethodName( - validateSimpleEncodedName(name, 0, len, openAngleBracketOK = false)) + new SimpleMethodName(validateSimpleEncodedName(name, 0, len, + openAngleBracketOK = false)) } <<< #2079 avoid nested indent, always; 2 binPack.preset = true @@ -229,8 +229,8 @@ object a { } >>> object a { - new SimpleMethodName(new SimpleMethodName( - validateSimpleEncodedName(name, 0, len, openAngleBracketOK = false))) + new SimpleMethodName(new SimpleMethodName(validateSimpleEncodedName(name, 0, + len, openAngleBracketOK = false))) } <<< #2079 avoid nested indent, always; 3 binPack.preset = true @@ -269,7 +269,8 @@ object a { new SimpleMethodName(new SimpleMethodName(validateSimpleEncodedName(name, 0, len, openAngleBracketOK = false))), new SimpleMethodName(new SimpleMethodName(validateSimpleEncodedName(name, - 0, len, openAngleBracketOK = false)))) + 0, len, openAngleBracketOK = false))) + ) } <<< #2079 avoid nested indent, always; 4 binPack.preset = true @@ -284,7 +285,8 @@ object a { object a { new SimpleMethodName( new SimpleMethodName(new SimpleMethodName(validateSimpleEncodedName(name, - 0, len, openAngleBracketOK = false)))).foo + 0, len, openAngleBracketOK = false))) + ).foo } <<< #2079 avoid nested indent, oneline; 4 binPack.preset = true @@ -298,8 +300,8 @@ object a { >>> object a { new SimpleMethodName( - new SimpleMethodName(new SimpleMethodName( - validateSimpleEncodedName(name, 0, len, openAngleBracketOK = false))) + new SimpleMethodName(new SimpleMethodName(validateSimpleEncodedName(name, + 0, len, openAngleBracketOK = false))) ).foo } <<< binpack=oneline, nested, inner with multiple args @@ -359,9 +361,7 @@ object a { foo match { case Apply(appMeth, Apply(wrapRefArrayMeth, - StripCast( - arg @ ArrayValue(elemtpt, elems)) :: Nil - ) :: classTagEvidence :: Nil) + StripCast(arg @ ArrayValue(elemtpt, elems)) :: Nil) :: classTagEvidence :: Nil) if WrapArray.isClassTagBasedWrapArrayMethod(wrapRefArrayMeth.symbol) && appMeth.symbol == ArrayModule_genericApply => bar @@ -405,9 +405,7 @@ object a { object a { Apply(appMeth, Apply(wrapRefArrayMeth, - StripCast( - ArrayValue(elemtpt, elems)) :: Nil - ) :: classTagEvidence :: Nil) + StripCast(ArrayValue(elemtpt, elems)) :: Nil) :: classTagEvidence :: Nil) } <<< binpack=always, with infix maxColumn = 80 @@ -438,7 +436,8 @@ object a { object a { val call = js.JSFunctionApply( js.Select(js.This()(classType), className, fFieldIdent)(jstpe.AnyType), - actualParams) + actualParams + ) } <<< binpack=always, no break after opening, apply maxColumn = 80 @@ -452,9 +451,8 @@ object a { } >>> object a { - val call = - js.JSFunctionApply(js.Select(js.This()(classType), className, fFieldIdent)( - jstpe.AnyType), actualParams) + val call = js.JSFunctionApply(js.Select(js.This()(classType), className, + fFieldIdent)(jstpe.AnyType), actualParams) } <<< binpack=oneline, no break after opening, extract maxColumn = 80 @@ -518,7 +516,8 @@ optDef.getOrElse { abort( foo && bar, - baz) + baz + ) abort( foo && bar, @@ -526,7 +525,8 @@ optDef.getOrElse { bar, foo && // c bar, - baz) + baz + ) } <<< binpack with non-top-level-only infix, afterInfix=many maxColumn = 16 @@ -551,7 +551,8 @@ foo.bar { abort( foo && bar, - baz) + baz + ) abort( foo && bar, @@ -559,7 +560,8 @@ foo.bar { bar, foo && // c bar, - baz) + baz + ) } <<< binpack with non-top-level-only infix, afterInfix=keep, indent once indentOperator.topLevelOnly = false @@ -771,7 +773,8 @@ optDef.getOrElse { abort( fooFoo && barBar, - bazBaz) + bazBaz + ) } <<< binpack with non-top-level-only infix, fold maxColumn = 20 @@ -898,14 +901,16 @@ object a { foo + bar, foo + - bar) + bar + ) Seq(foo && bar) Seq( foo && bar, foo && - bar) + bar + ) } <<< binpack with infix rhs in parens, 1 arg maxColumn = 20 @@ -919,7 +924,8 @@ object a { >>> object a { val a = foo bar ( - baz = qux) + baz = qux + ) } <<< binpack with infix rhs in parens, 2 args maxColumn = 25 diff --git a/scalafmt-tests/src/test/resources/scalajs/Class.stat b/scalafmt-tests/src/test/resources/scalajs/Class.stat index 2876c3df7f..ed2c4413c4 100644 --- a/scalafmt-tests/src/test/resources/scalajs/Class.stat +++ b/scalafmt-tests/src/test/resources/scalajs/Class.stat @@ -43,8 +43,8 @@ class Promise[+A]( executor: js.Function2[js.Function1[A | Thenable[A], _], js.Function1[scala.Any, _], _]) >>> class Promise[+A]( - executor: js.Function2[js.Function1[A | Thenable[A], _], js.Function1[ - scala.Any, _], _]) + executor: js.Function2[js.Function1[A | Thenable[A], + _], js.Function1[scala.Any, _], _]) <<< #270 object a { private class Encoder extends CharsetEncoder( diff --git a/scalafmt-tests/src/test/resources/scalajs/DefDef.stat b/scalafmt-tests/src/test/resources/scalajs/DefDef.stat index 6a2342c720..d44e15906e 100644 --- a/scalafmt-tests/src/test/resources/scalajs/DefDef.stat +++ b/scalafmt-tests/src/test/resources/scalajs/DefDef.stat @@ -266,19 +266,24 @@ implicit def toFunction22[T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, js.Array[JSStackTraceElem], js.Array[JSStackTraceElem]]] = js.native >>> def sourceMapper: js.UndefOr[js.Function1[ // scalastyle:ignore - js.Array[JSStackTraceElem], js.Array[JSStackTraceElem]]] = js.native + js.Array[JSStackTraceElem], + js.Array[JSStackTraceElem] +]] = js.native <<< comment inside middle of type parameter #264 3 def sourceMapper: js.UndefOr[ // scalastyle:ignore js.Array[JSStackTraceElem], js.Array[JSStackTraceElem]] = js.native >>> def sourceMapper: js.UndefOr[ // scalastyle:ignore - js.Array[JSStackTraceElem], js.Array[JSStackTraceElem]] = js.native + js.Array[JSStackTraceElem], + js.Array[JSStackTraceElem] +] = js.native <<< comment inside middle of type parameter #264 4 def sourceMapper: js.UndefOr[js.Array[ // scalastyle:ignore JSStackTraceElem], js.Array[JSStackTraceElem]] = js.native >>> def sourceMapper: js.UndefOr[js.Array[ // scalastyle:ignore - JSStackTraceElem], js.Array[JSStackTraceElem]] = js.native + JSStackTraceElem + ], js.Array[JSStackTraceElem]] = js.native <<< align by =>;Case #268 def unwrapJavaScriptException(th: Throwable): Any = th match { case js.JavaScriptException(e) => e diff --git a/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasAlways.stat b/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasAlways.stat index bc17ba0c63..8dd9acbcc6 100644 --- a/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasAlways.stat +++ b/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasAlways.stat @@ -355,7 +355,8 @@ class foo( a: String, b: String, ) = ??? method( - a, b, + a, + b, ) } <<< #2755 two, no comma, no fold, binpack, !cfgStyle @@ -386,7 +387,8 @@ class foo( a: String, b: String, ) = ??? method( - a, b, + a, + b, ) } <<< #3663 enclosed literal diff --git a/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasAlwaysComments.stat b/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasAlwaysComments.stat index 0ec9df5d8d..90e2f55f08 100644 --- a/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasAlwaysComments.stat +++ b/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasAlwaysComments.stat @@ -391,10 +391,16 @@ class foo(a: String, b /* c1 */ /* c2 */ ) } >>> -Idempotency violated -=> Diff (- obtained, + expected) - method( -- a, -- b, /* c1 */ /* c2 */ -+ a, b, /* c1 */ /* c2 */ - ) +import foo.{ + a, b, /* c1 */ /* c2 */ +} +class foo( + a: String, b: String, /* c1 */ /* c2 */ +) { + def method( + a: String, b: String, /* c1 */ /* c2 */ + ) = ??? + method( + a, b, /* c1 */ /* c2 */ + ) +} diff --git a/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasMultiple.stat b/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasMultiple.stat index 1a5498aa1d..4f926529fa 100644 --- a/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasMultiple.stat +++ b/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasMultiple.stat @@ -340,7 +340,8 @@ class foo( a: String, b: String, ) = ??? method( - a, b, + a, + b, ) } <<< #2755 two, no comma, no fold, binpack, !cfgStyle diff --git a/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasMultipleComments.stat b/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasMultipleComments.stat index c043c33911..f91a30f8ab 100644 --- a/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasMultipleComments.stat +++ b/scalafmt-tests/src/test/resources/trailing-commas/trailingCommasMultipleComments.stat @@ -375,10 +375,16 @@ class foo(a: String, b /* c1 */ /* c2 */ ) } >>> -Idempotency violated -=> Diff (- obtained, + expected) - method( -- a, -- b, /* c1 */ /* c2 */ -+ a, b, /* c1 */ /* c2 */ - ) +import foo.{ + a, b, /* c1 */ /* c2 */ +} +class foo( + a: String, b: String, /* c1 */ /* c2 */ +) { + def method( + a: String, b: String, /* c1 */ /* c2 */ + ) = ??? + method( + a, b, /* c1 */ /* c2 */ + ) +}