Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FormatOps: improve binpacking parentConstructors #3094

Merged
merged 2 commits into from
Feb 4, 2022
Merged

FormatOps: improve binpacking parentConstructors #3094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef63982
Add tests with early inits and parentConstructors
kitbellew Feb 1, 2022
62aab09
FormatOps: improve binpacking parentConstructors
kitbellew Feb 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion docs/configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4119,7 +4119,7 @@ val secret: List[Bit] = List(0, 0, 1, 1, 1, 0, 1, 0, 1, 1, 1, 0, 0, 1, 1, 0, 1,

### `binPack.parentConstructors`

Parent constructors are `C` and `D` in `class A extends B with C and D`. Changed
Parent constructors are `B` (since 3.4.1), `C` and `D` in `class A extends B with C and D`. Changed
from a boolean to a wider set of options in v2.6.0.

```scala mdoc:defaults
Expand All @@ -4138,6 +4138,7 @@ object A {
trait Foo
extends Bar
with Baz
trait Foo extends Bar with Baz
}
```

Expand Down Expand Up @@ -4192,6 +4193,12 @@ object A {
b
) with Baz
with Qux
class Foo(a: Int, b: Int)
extends Bar(
a,
b
) with Baz
with Qux
}
```

Expand Down
12 changes: 10 additions & 2 deletions scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1083,7 +1083,7 @@ class FormatOps(
.withIndent(indent)
.withSingleLine(
slbEnd,
exclude = insideBlock[T.LeftParen](ft, slbEnd),
exclude = insideBracesBlock(ft, slbEnd, true),
noSyntaxNL = extendsThenWith
),
Split(nlMod, 1).withIndent(indent)
Expand All @@ -1100,9 +1100,17 @@ class FormatOps(
case _ =>
Right(style.newlines.source eq Newlines.fold)
}
val exclude = style.binPack.parentConstructors match {
case BinPack.ParentCtors.Always
if ft.noBreak || style.newlines.sourceIgnored =>
insideBracesBlock(ft, lastToken, true)
case _ => TokenRanges.empty
}
val noSyntaxNL = extendsThenWith
Seq(
Split(Space, 0).withSingleLine(lastToken, noSyntaxNL = noSyntaxNL),
Split(Space, 0)
.withSingleLine(lastToken, exclude = exclude, noSyntaxNL = noSyntaxNL)
.withIndent(indent),
Split(nlMod, 0)
.onlyIf(nlOnelineTag != Right(false))
.preActivateFor(nlOnelineTag.left.toOption)
Expand Down
137 changes: 137 additions & 0 deletions scalafmt-tests/src/test/resources/binPack/ParentConstructors.stat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,3 +114,140 @@ def method[T <: ColType](): DTDataset[
with FCol with GCol with HCol
with ICol
]
<<< #3091 always
maxColumn = 100
runner.parser = source
binPack.parentConstructors = always
===
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection(
u,
mirror
)
with scala.meta.internal.tokens.Reflection
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection
>>>
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
} with scala.meta.internal.trees.Reflection(
u,
mirror
) with scala.meta.internal.tokens.Reflection
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
} with scala.meta.internal.trees.Reflection
<<< #3091 never
maxColumn = 100
runner.parser = source
binPack.parentConstructors = never
===
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection(
u,
mirror
)
with scala.meta.internal.tokens.Reflection
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection
>>>
object CoreReflection
extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection(
u,
mirror
)
with scala.meta.internal.tokens.Reflection
object CoreReflection
extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection
<<< #3091 oneline
maxColumn = 100
runner.parser = source
binPack.parentConstructors = oneline
===
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection(
u,
mirror
)
with scala.meta.internal.tokens.Reflection
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection
>>>
object CoreReflection
extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection(
u,
mirror
)
with scala.meta.internal.tokens.Reflection
object CoreReflection
extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection
<<< #3091 keep
maxColumn = 100
runner.parser = source
binPack.parentConstructors = keep
===
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection(
u,
mirror
)
with scala.meta.internal.tokens.Reflection
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection
>>>
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection(
u,
mirror
)
with scala.meta.internal.tokens.Reflection
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
}
with scala.meta.internal.trees.Reflection
5 changes: 2 additions & 3 deletions scalafmt-tests/src/test/resources/scala3/Enum.stat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,8 @@ enum Foo(val x: Int) {
}
>>>
enum Foo(val x: Int) {
case Bar
extends Foo(1) with X
with Y with Z
case Bar extends Foo(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong now. With binPack.parentConstructors = Always wasn't the previous state correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new approach allows space if source is ignored (i.e. fold/unfold) or space was used in the source. in this example (line 128), there was no break here, so extends is also binpacked.

with X with Y with Z
}
<<< enum case with parent constructors `oneline`
binPack.parentConstructors = Oneline
Expand Down
3 changes: 1 addition & 2 deletions scalafmt-tests/src/test/resources/scalajs/Class.stat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ private class Encoder extends CharsetEncoder(
}
>>>
object a {
private class Encoder
extends CharsetEncoder(UTF_16_Common.this, 2.0f, 2.0f,
private class Encoder extends CharsetEncoder(UTF_16_Common.this, 2.0f, 2.0f,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, same explanation is above. always attempts to binpack as much as possible unless there was an explicit break.

// Character 0xfffd
if (endianness == LittleEndian) Array(-3, -1) else Array(-1, -3))
}