Do not format blank lines in multiline string literals

[kishikawakatsumi]

Fixes #1959

For an example, see the following code

assertionFailure("""
  First line

  Second line

  """)

There are blank lines in line 3 and 5.

The conditions for a blank line are that StringSegments is only a newline character with no trivia (line 3), or that StringSegments is a zero-length string and the trailing trivia is a newline (line 5).

However, this condition would also match empty StringSegments immediately after string interpolation in the following code. (This case is covered by testStringInterpolationAtStartOfMultiLineStringLiteral.)

assertionFailure("""
  ABC
  \(myVar)
  """)

Therefore, I also added the condition that the previous token must be ended by a newline.

[Matejkob]

I've been reviewing your PR to learn more about the swift-syntax project, and I've come across a few points of interest. I've left a few in-line comments with some minor suggestions. 💙

[Matejkob]

I'm wondering whether it would be more beneficial to use isNewline instead of "\n". What do you think about this?

This can be achieved in a variety of ways. For example, we could use text.allSatisfy(\.isNewline). However, if we are prioritizing performance, we could aim for O(1) complexity with an approach like the following:

extension String {
  func isNewline() -> Bool {
    (first?.isNewline ?? false) && dropFirst().isEmpty
  }
}

[ahoppen]

I just wanted to say thanks for your review, @Matejkob. Those are valuable comments, they only got overridden by the fact that I found bigger design-ish improvements in the PR.

[Matejkob]

I noticed some minor formatting differences in this code compared to other code in the repo. Would you mind runing swift-format?

[ahoppen]

Thanks for the PR @kishikawakatsumi.

I have just investigated and it looks like the root causes of the issue are that

1. We are setting an anchorPoint for empty lines. Anchor points are meant to be starting points at which the user didn’t provide any manual indentation relative to which we should format the rest of the tree. But for empty lines it’s not like the user didn’t provide any indentation. There simply wasn’t anything to indent.
2. leadingTrivia.trimmingTrailingWhitespaceBeforeNewline was not considering whether the token text itself contained a newline in isBeforeNewline.

To fix these root causes, what I would suggest is to

• Add a new variable behind combinedTrailingTrivia

/// Whether the leading trivia of the token is folloed by a newline.
let leadingTriviaIsFollowedByNewline: Bool = {
  if token.text.isEmpty && token.trailingTrivia.startsWithNewline {
    return true
  } else if token.text.first?.isNewline ?? false {
    return true
  } else if token.text.isEmpty && token.trailingTrivia.isEmpty && nextTokenWillStartWithNewline {
    return true
  } else {
    return false
  }
}()

• Change the anchor point computation to the following

let isEmptyLine = token.leadingTrivia.isEmpty && leadingTriviaIsFollowedByNewline
if leadingTrivia.indentation(isOnNewline: isInitialToken || previousTokenWillEndWithNewline) == [] && !isEmptyLine {
  // If the token starts on a new line and does not have indentation, this
  // is the last non-indented token. Store its indentation level.
  // Do not
  anchorPoints[token] = currentIndentationLevel
}

• And finally, also pass leadingTriviaIsFollowedByNewline to trimmingTrailingWhitepsaceBeforeNewline

leadingTrivia = leadingTrivia.trimmingTrailingWhitespaceBeforeNewline(isBeforeNewline: leadingTriviaIsFollowedByNewline)

[kishikawakatsumi]

@ahoppen Thanks for the guide. The formatting problem for blank lines in multi-line string literals has been fixed. However, it seems to affect other test cases. The following three test-cases fail.

• DeclarationTests.testProtocolParsing()
• EnumTests.testEnum21a()
• OperatorDeclTests.testOperatorDecl15()

The diff for each is as follows. Perhaps a space that should be inserted has not been inserted?

// DeclarationTests.testProtocolParsing()
–protocol P{{}case <#identifier#>
+protocol P{{}case<#identifier#>
 }

// EnumTests.testEnum21a()
 enum Recovery1 {
–  case <#identifier#>:
+  case<#identifier#>:
 }

// OperatorDeclTests.testOperatorDecl15()
 precedencegroup E {
–  higherThan: <#identifier#>
+  higherThan:<#identifier#>
 }

[ahoppen]

Hi @kishikawakatsumi,

Oh, I should have totally checked whether my changes still formatted all the remaining diagnostics correctly, I’m sorry. The issue is that in the parsed tree, we represent the missing identifiers by identifiers with empty text as well and they only get transformed to the <#identifier#> placeholders in a later PresentMaker pass. Thus, all my rules of checking whether the text is empty also applied to these identifiers, which was causing the issues.

Looking into it further, I realized that the proper fix required some more, slightly more invasive changes that I, myself, would like to get a review on. I have opened #1964 with these changes and hope you don’t mind me closing this PR in favor of that one.

[kishikawakatsumi]

Thanks!