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

Jump to bottom

FormatWriter: fix alignment of multiline defns #2753

Merged
merged 4 commits into from
Sep 20, 2021
Merged

FormatWriter: fix alignment of multiline defns #2753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a1e9d9e
Add tests with param alignment
kitbellew Sep 19, 2021
816be29
FormatWriter: flush alignment after a blank line
kitbellew Sep 20, 2021
57c610b
FormatWriter: fix alignment of multiline defns
kitbellew Sep 20, 2021
ccbe430
FormatWriter: use largest align owner on a line
kitbellew Sep 20, 2021
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
29 changes: 21 additions & 8 deletions scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatWriter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -980,6 +980,14 @@ class FormatWriter(formatOps: FormatOps) {
getAlignContainer(floc).foreach { container =>
if (alignContainer eq null)
alignContainer = container
else if (alignContainer ne container) {
val pos1 = alignContainer.pos
val pos2 = container.pos
if (pos2.start <= pos1.start && pos2.end >= pos1.end) {
alignContainer = container
columnCandidates.clear()
}
}
if (alignContainer eq container)
columnCandidates += new AlignStop(
getAlignColumn(floc) + columnShift,
Expand All @@ -994,12 +1002,7 @@ class FormatWriter(formatOps: FormatOps) {

implicit val location = processLine
val isBlankLine = location.state.split.modExt.mod.newlines > 1
if (alignContainer eq null) {
getBlockToFlush(
getAlignContainer(location.formatToken.meta.rightOwner),
isBlankLine
).foreach(flushAlignBlock)
} else {
if (alignContainer ne null) {
val candidates = columnCandidates.result()
val block = getOrCreateBlock(alignContainer)
def appendToBlock(line: IndexedSeq[AlignStop], matches: Int = 0) = {
Expand All @@ -1025,6 +1028,11 @@ class FormatWriter(formatOps: FormatOps) {
prevAlignContainer = alignContainer
prevBlock = block
}
if (isBlankLine || alignContainer.eq(null))
getBlockToFlush(
getAlignContainer(location.formatToken.meta.rightOwner),
isBlankLine
).foreach(flushAlignBlock)
}
blocks.valuesIterator.foreach(flushAlignBlock)
finalResult.result()
Expand Down Expand Up @@ -1078,9 +1086,14 @@ class FormatWriter(formatOps: FormatOps) {
// containers that can be traversed further if on same line
case Some(p @ (_: Case | _: Enumerator)) =>
if (isEarlierLine(p)) p else getAlignContainerParent(p)
// containers that can be traversed further if single-stat
// containers that can be traversed further if lhs single-line
case Some(p @ AlignContainer.WithBody(b)) =>
if (b.is[Term.Block]) p else getAlignContainerParent(p)
val keepGoing = (b.eq(child) || p.eq(child)) && {
val beg = tokens.after(p.tokens.head)
val end = tokens.tokenJustBefore(b)
getLineDiff(locations, beg, end) == 0
}
if (keepGoing) getAlignContainerParent(p) else p
case Some(p: Term.ForYield) if child ne p.body => p
case Some(p) => p.parent.getOrElse(p)
case _ => child
Expand Down
158 changes: 151 additions & 7 deletions scalafmt-tests/src/test/resources/align/DefaultWithAlign.stat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,15 +918,15 @@ object a {
}
>>>
object a {
def foo: Int = {
def foo: Int = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have additional align here and in the two examples below?

If I understand the intention, we should not align if the statements are not just below one another. That's what seems to be happening in #2750 4 blocks, no blanks test case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

one of things highlighted in the submitted issue is the inconsistent handling of multiline definitions with and without curly braces (blocks vs no blocks). so that was one of the fixes.

both here and below, align.preset = most is used and it enables multiline alignment. since these methods are not separated by blank lines (those should interrupt alignment), they are aligned.

the difference with the case below is that includes alignment of : (and they do not align, hence = we don't get to =), while here there are only equals.

Copy link
Contributor

Choose a reason for hiding this comment

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

and they do not align

Is it an issue that there is multiple : on the line? We do not try to align in that case?

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 way I've implemented it now is that parameter colons should only align with parameters from the same definition (that is, the align owner is the definition) whereas the align owner for the equals is the template which contains the definition.

you can only have one align owner for all tokens on a given line, so it's either colons or equals but not both.

might need to adjust the logic to figure out which should be used when both are present. give me some time, please.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ptal, added a commit to choose the largest align container.

val x = ???
42
}
def bar: Int = {
def bar: Int = {
for { x <- "hey" } yield x
42
}
def fooBig: Int = {
def fooBig: Int = {
42
}
def fooBigger: Int = {
Expand All @@ -953,8 +953,8 @@ object a {
}
>>>
object a {
def a: Int = { b % c }
def aa: Int = { bb % cc }
def a: Int = { b % c }
def aa: Int = { bb % cc }
def aaa: Int = { bbb % ccc }
}
<<< singleline val without blanks
Expand All @@ -977,8 +977,8 @@ object a {
}
>>>
object a {
val a: Int = { b % c }
val aa: Int = { bb % cc }
val a: Int = { b % c }
val aa: Int = { bb % cc }
val aaa: Int = { bbb % ccc }
}
<<< #1948
Expand Down Expand Up @@ -1388,3 +1388,147 @@ object a {
someBoolean = true // bar comment
)
}
<<< #2750 1 no blocks, blanks
maxColumn = 160
align.preset = most
align.tokens."+" = [
{ code = "=", owners=[{regex="Term.Param|Defn|Term.Assign"}] },
{ code = ":", owners = [{regex="Term.Param|Defn|Decl.Def"}] },
]
===
object Thing {
def apply(foo: FooBaz.booberry, someInt: Int, booControl: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz =
apply(policy, Some(beatBytes), booControl)

def apply(foo: FooBaz.booberry, someInt: Int)(implicit p: Parameters): FooBaz =
apply(policy, Some(beatBytes), booControl = None)

def apply(fooBaz: Int, someBooberry: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz =
apply(policy = FooBaz.booberry, Some(someInt), booControl)

def apply(fooBaz: Int)(implicit p: Parameters): TLNode =
apply(policy = FooBaz.booberry, Some(someInt), booControl = None)
}
>>>
object Thing {
def apply(foo: FooBaz.booberry, someInt: Int, booControl: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz =
apply(policy, Some(beatBytes), booControl)

def apply(foo: FooBaz.booberry, someInt: Int)(implicit p: Parameters): FooBaz =
apply(policy, Some(beatBytes), booControl = None)

def apply(fooBaz: Int, someBooberry: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz =
apply(policy = FooBaz.booberry, Some(someInt), booControl)

def apply(fooBaz: Int)(implicit p: Parameters): TLNode =
apply(policy = FooBaz.booberry, Some(someInt), booControl = None)
}
<<< #2750 2 no blocks, no blanks
maxColumn = 160
align.preset = most
align.tokens."+" = [
{ code = "=", owners=[{regex="Term.Param|Defn|Term.Assign"}] },
{ code = ":", owners = [{regex="Term.Param|Defn|Decl.Def"}] },
]
===
object Thing {
def apply(foo: FooBaz.booberry, someInt: Int, booControl: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz =
apply(policy, Some(beatBytes), booControl)
def apply(foo: FooBaz.booberry, someInt: Int)(implicit p: Parameters): FooBaz =
apply(policy, Some(beatBytes), booControl = None)
def apply(fooBaz: Int, someBooberry: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz =
apply(policy = FooBaz.booberry, Some(someInt), booControl)
def apply(fooBaz: Int)(implicit p: Parameters): TLNode =
apply(policy = FooBaz.booberry, Some(someInt), booControl = None)
}
>>>
object Thing {
def apply(foo: FooBaz.booberry, someInt: Int, booControl: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz =
apply(policy, Some(beatBytes), booControl)
def apply(foo: FooBaz.booberry, someInt: Int)(implicit p: Parameters): FooBaz =
apply(policy, Some(beatBytes), booControl = None)
def apply(fooBaz: Int, someBooberry: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz =
apply(policy = FooBaz.booberry, Some(someInt), booControl)
def apply(fooBaz: Int)(implicit p: Parameters): TLNode =
apply(policy = FooBaz.booberry, Some(someInt), booControl = None)
}
<<< #2750 3 blocks, blanks
maxColumn = 160
align.preset = most
align.tokens."+" = [
{ code = "=", owners=[{regex="Term.Param|Defn|Term.Assign"}] },
{ code = ":", owners = [{regex="Term.Param|Defn|Decl.Def"}] },
]
===
object Thing {
def apply(foo: FooBaz.booberry, someInt: Int, booControl: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz = {
apply(policy, Some(beatBytes), booControl)
}

def apply(foo: FooBaz.booberry, someInt: Int)(implicit p: Parameters): FooBaz = {
apply(policy, Some(beatBytes), booControl = None)
}

def apply(fooBaz: Int, someBooberry: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz = {
apply(policy = FooBaz.booberry, Some(someInt), booControl)
}

def apply(fooBaz: Int)(implicit p: Parameters): TLNode = {
apply(policy = FooBaz.booberry, Some(someInt), booControl = None)
}
}
>>>
object Thing {
def apply(foo: FooBaz.booberry, someInt: Int, booControl: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz = {
apply(policy, Some(beatBytes), booControl)
}

def apply(foo: FooBaz.booberry, someInt: Int)(implicit p: Parameters): FooBaz = {
apply(policy, Some(beatBytes), booControl = None)
}

def apply(fooBaz: Int, someBooberry: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz = {
apply(policy = FooBaz.booberry, Some(someInt), booControl)
}

def apply(fooBaz: Int)(implicit p: Parameters): TLNode = {
apply(policy = FooBaz.booberry, Some(someInt), booControl = None)
}
}
<<< #2750 4 blocks, no blanks
maxColumn = 160
align.preset = most
align.tokens."+" = [
{ code = "=", owners=[{regex="Term.Param|Defn|Term.Assign"}] },
{ code = ":", owners = [{regex="Term.Param|Defn|Decl.Def"}] },
]
===
object Thing {
def apply(foo: FooBaz.booberry, someInt: Int, booControl: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz = {
apply(policy, Some(beatBytes), booControl)
}
def apply(foo: FooBaz.booberry, someInt: Int)(implicit p: Parameters): FooBaz = {
apply(policy, Some(beatBytes), booControl = None)
}
def apply(fooBaz: Int, someBooberry: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz = {
apply(policy = FooBaz.booberry, Some(someInt), booControl)
}
def apply(fooBaz: Int)(implicit p: Parameters): TLNode = {
apply(policy = FooBaz.booberry, Some(someInt), booControl = None)
}
}
>>>
object Thing {
def apply(foo: FooBaz.booberry, someInt: Int, booControl: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz = {
apply(policy, Some(beatBytes), booControl)
}
def apply(foo: FooBaz.booberry, someInt: Int)(implicit p: Parameters): FooBaz = {
apply(policy, Some(beatBytes), booControl = None)
}
def apply(fooBaz: Int, someBooberry: Option[OptionalBooberryParams])(implicit p: Parameters): FooBaz = {
apply(policy = FooBaz.booberry, Some(someInt), booControl)
}
def apply(fooBaz: Int)(implicit p: Parameters): TLNode = {
apply(policy = FooBaz.booberry, Some(someInt), booControl = None)
}
}