Break after annotations iff it is a block-level expression

[nreid260]

These breaks are meaningful, since adding or removing them can change which parts of the expression the annotation applies to.

Fixes #247

[facebook-github-bot]

@cgrushko has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cgrushko]

this effectively preserves all whitespace around annotations, right?
feels like a shame, but I guess we don't have any other choice?
(in particular, formatting of annotations is no longer independent of the input)

[nreid260]

I don't think it preserved all whitespace, just newlines. It normalizes weird combos of newlines into a one-annotation-per-line form, and collapses adjacent spaces into single spaces.

If there's an example you're worried about, let's add it to the tests. I tried to cover all the ones I could think of, but it's a surprisingly tricky problem, since formatting the expression might also insert newlines that change the annotation target.

[cgrushko]

I think the space() means there won't be a line break there. So, if there's a \n, it'll be a forcedBreak, otherwise a non-breaking space. That's what I meant by preserve whitespace.
I don't know enough about annotations in this context to suggest a better solution, though.

[nreid260]

Right. If there was a line break between any two annotations, or before the expression, then we'll force a break between all annotations and the expression. If there were no line breaks to begin with, all whitespace becomes non-breaking spaces.

[cgrushko]

@strulovich: "Can we make this so we do the fallback only for complex expressions?
If the annotated expression is a function call we're good - this only affects binary expressions (and maybe a few other cases)"

[nreid260]

To what end? Is there some case where this approach gives a bad formatting?

Because this part of the formatter has the potential to change the behaviour of code, I'm reluctant to give it a lot of cases. The more paths it has, the more likely it is to behave incorrectly, or surprisingly.

At the very least, I would want it to do the "safe" thing by default, with perhaps some exceptions for provably identical cases.

[strulovich]

To what end? Is there some case where this approach gives a bad formatting?

Our basic premise for this tool is:

ktfmt ignores most existing formatting. It respects existing newlines in some places, but in general, its output is deterministic and is independent of the input code.

This makes Ktfmt less deterministic. If I'm understanding the code right, both of these will remain the same:

@Magic
doiIt()

@Magic doIt()

This makes the formatter do less of what we need it to, and can result in people having to discuss or manually modify the style of their code more.

Sadly, by now the "one rule fits all" issues of the formatter have been solved, so it's more and more likely we need to dig into issues with more branches. Associativity issues should only be arising from binary expressions, so I don't think this is complicated to solve with minimal degradation of our promises.

[nreid260]

Do we know for certain that it really is just binary expressions where behaviour can change? For example, what about infix functions, or as which has a type as the right operand.

[strulovich]

Do we know for certain that it really is just binary expressions where behaviour can change? For example, what about infix functions, or as which has a type as the right operand.

Nope, I agree it's worth some testing. I think this can be done by annotating a bunch of stuff, loading the AST, and just seeing what annotation is for what. Once we understand that we can probably use the AST information to avoid mistakes while formatting. (I assume the annotations are attached in the AST correctly, otherwise it's a really weird problem)

[nreid260]

How about this version: if the annotated expression is the child of a block, force a break after the annotations.

It should hit all our requirements:

• never changes semantics
• exactly one correct formatting
• easy to verify

Testing against our code base, only 0.2% of files were affected. It also seems to improve formatting of annotations on lambdas, which is a bonus I can't explain.

[strulovich]

Sorry for the delay, was out.

This seems like a good approach.
From looking at all cases, all I could find is KtBinaryExpression/KtBinaryExpressionWithTypeRHS as a way to make this happen.

(Assignments, as and all the usual +-*/... are all KtBinaryExpression or KtBinaryExpressionWithTypeRHS)

I think we can therefore only use this behavior in a block, and for these expressions.

I'm fine with adding the type checks myself, but let's add a specific test (both options next to each other, and the same thing outside a block being formatted)

Thanks for digging into this nasty bug.

[nreid260]

It also definitely happens for infix functions. Are those KtBinaryExpression/KtBinaryExpressionWithTypeRHS?

Example https://pl.kotl.in/zyHiVpo_g

[strulovich]

Yes, infix functions are KtBinaryExpression.

Here's a test case example I used (with the tree printing to check that):

  @Test
  fun `annotations for expressions`() =
      assertFormatted(
          """
      |fun restore() {
      |  var b
      |
      |  @Suppress("UNCHECKED_CAST") val a = f(1) as Int
      |  @Suppress("UNCHECKED_CAST")
      |  val a = f(1) as Int
      |
      |  @Suppress("UNCHECKED_CAST") b = f(1) as Int
      |  @Suppress("UNCHECKED_CAST")
      |  b = f(1) as Int
      |  
      |  @Suppress("UNCHECKED_CAST") b = f(1) to 5
      |  @Suppress("UNCHECKED_CAST")
      |  b = f(1) to 5
      |}
      |""".trimMargin())

I was not able to find an example that causes this with ::, and other expressions (or property declarations) seemed to not spawn the warning as well.

[nreid260]

Ok, at this point, it seems like you know the changes you'd like to make. Do you mind merging them internally? I'm fine as long as the bug is fixed.

[facebook-github-bot]

@strulovich has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.