Unify PresentMaker and MissingNodesBasicFormatter, fixing a formatting bug in string literals

The underlying issues of the malformatted multi-line string literal 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`.

Now, to detect if the token is followed by a newline, we should check if its text is empty, which is the case for empty string literals. Unfortunately, this is also the case for missing identifiers, which also have an empty text but whose empty text will be replace by a placeholder by `PresentMaker`. To distinguish between the two, I merged `PresentMaker` and `MissingNodesBasicFormatter` so that the `BasicFormat` also performs the text replacement to placeholders.

Fixes #1959

Co-Authored-By: kishikawa katsumi <[email protected]>

Author: ahoppen

Sources/SwiftBasicFormat/BasicFormat.swift

@@ -10,7 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
-import SwiftSyntax
+@_spi(RawSyntax) import SwiftSyntax
 
 /// A rewriter that performs a "basic" format of the passed tree.
 ///
@@ -271,7 +271,7 @@ open class BasicFormat: SyntaxRewriter {
       (.keyword(.`init`), .leftParen),  // init()
       (.keyword(.self), .period),  // self.someProperty
       (.keyword(.Self), .period),  // self.someProperty
-      (.keyword(.set), .leftParen),  // var mYar: Int { set(value) {} }
+      (.keyword(.set), .leftParen),  // var mVar: Int { set(value) {} }
       (.keyword(.subscript), .leftParen),  // subscript(x: Int)
       (.keyword(.super), .period),  // super.someProperty
       (.leftBrace, .rightBrace),  // {}
@@ -348,6 +348,26 @@ open class BasicFormat: SyntaxRewriter {
     return true
   }
 
+  /// Change the text of a token during formatting.
+  ///
+  /// This allows formats to e.g. replace missing tokens by placeholder tokens.
+  ///
+  /// - Parameter token: The token whose text should be changed
+  /// - Returns: The new text or `nil` if the text should not be changed
+  open func transformTokenText(_ token: TokenSyntax) -> String? {
+    return nil
+  }
+
+  /// Change the presence of a token during formatting.
+  ///
+  /// This allows formats to e.g. replace missing tokens by placeholder tokens.
+  ///
+  /// - Parameter token: The token whose presence should be changed
+  /// - Returns: The new presence or `nil` if the presence should not be changed
+  open func transformTokenPresence(_ token: TokenSyntax) -> SourcePresence? {
+    return nil
+  }
+
   // MARK: - Formatting a token
 
   open override func visit(_ token: TokenSyntax) -> TokenSyntax {
@@ -357,6 +377,8 @@ open class BasicFormat: SyntaxRewriter {
     let isInitialToken = self.previousToken == nil
     let previousToken = self.previousToken ?? token.previousToken(viewMode: viewMode)
     let nextToken = token.nextToken(viewMode: viewMode)
+    let transformedTokenText = self.transformTokenText(token)
+    let transformedTokenPresence = self.transformTokenPresence(token)
 
     /// In addition to existing trivia of `previousToken`, also considers
     /// `previousToken` as ending with whitespace if it and `token` should be
@@ -372,7 +394,7 @@ open class BasicFormat: SyntaxRewriter {
         || (requiresWhitespace(between: previousToken, and: token) && isMutable(previousToken))
     }()
 
-    /// This method does not consider any posssible mutations to `previousToken`
+    /// This method does not consider any possible mutations to `previousToken`
     /// because newlines should be added to the next token's leading trivia.
     let previousTokenWillEndWithNewline: Bool = {
       guard let previousToken = previousToken else {
@@ -416,6 +438,14 @@ open class BasicFormat: SyntaxRewriter {
       if nextToken.leadingTrivia.startsWithNewline {
         return true
       }
+      if nextToken.leadingTrivia.isEmpty {
+        if nextToken.text.first?.isNewline ?? false {
+          return true
+        }
+        if nextToken.text.isEmpty && nextToken.trailingTrivia.startsWithNewline {
+          return true
+        }
+      }
       if requiresNewline(between: token, and: nextToken),
         isMutable(nextToken),
         !token.trailingTrivia.endsWithNewline,
@@ -436,6 +466,19 @@ open class BasicFormat: SyntaxRewriter {
       return trailingTrivia + Trivia(pieces: nextTokenLeadingWhitespace)
     }()
 
+    /// Whether the leading trivia of the token is followed by a newline.
+    let leadingTriviaIsFollowedByNewline: Bool = {
+      if (transformedTokenText ?? token.text).isEmpty && token.trailingTrivia.startsWithNewline {
+        return true
+      } else if token.text.first?.isNewline ?? false {
+        return true
+      } else if (transformedTokenText ?? token.text).isEmpty && token.trailingTrivia.isEmpty && nextTokenWillStartWithNewline {
+        return true
+      } else {
+        return false
+      }
+    }()
+
     if requiresNewline(between: previousToken, and: token) {
       // Add a leading newline if the token requires it unless
       //  - it already starts with a newline or
@@ -455,7 +498,8 @@ open class BasicFormat: SyntaxRewriter {
       }
     }
 
-    if leadingTrivia.indentation(isOnNewline: isInitialToken || previousTokenWillEndWithNewline) == [] {
+    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
       anchorPoints[token] = currentIndentationLevel
@@ -501,14 +545,24 @@ open class BasicFormat: SyntaxRewriter {
     leadingTrivia = leadingTrivia.indented(indentation: leadingTriviaIndentation, isOnNewline: previousTokenIsStringLiteralEndingInNewline)
     trailingTrivia = trailingTrivia.indented(indentation: trailingTriviaIndentation, isOnNewline: false)
 
-    leadingTrivia = leadingTrivia.trimmingTrailingWhitespaceBeforeNewline(isBeforeNewline: false)
+    leadingTrivia = leadingTrivia.trimmingTrailingWhitespaceBeforeNewline(isBeforeNewline: leadingTriviaIsFollowedByNewline)
     trailingTrivia = trailingTrivia.trimmingTrailingWhitespaceBeforeNewline(isBeforeNewline: nextTokenWillStartWithNewline)
 
-    if leadingTrivia == token.leadingTrivia && trailingTrivia == token.trailingTrivia {
-      return token
+    var result = token.detached
+    if leadingTrivia != result.leadingTrivia {
+      result = result.with(\.leadingTrivia, leadingTrivia)
     }
-
-    return token.detached.with(\.leadingTrivia, leadingTrivia).with(\.trailingTrivia, trailingTrivia)
+    if trailingTrivia != result.trailingTrivia {
+      result = result.with(\.trailingTrivia, trailingTrivia)
+    }
+    if let transformedTokenText {
+      let newKind = TokenKind.fromRaw(kind: token.tokenKind.decomposeToRaw().rawKind, text: transformedTokenText)
+      result = result.with(\.tokenKind, newKind).with(\.presence, .present)
+    }
+    if let transformedTokenPresence {
+      result = result.with(\.presence, transformedTokenPresence)
+    }
+    return result
   }
 }
 

Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift

@@ -12,7 +12,7 @@
 
 import SwiftDiagnostics
 import SwiftBasicFormat
-import SwiftSyntax
+@_spi(RawSyntax) import SwiftSyntax
 
 extension FixIt {
   /// A more complex set of changes that affects multiple syntax nodes and thus
@@ -108,11 +108,46 @@ extension FixIt.MultiNodeChange {
 
 // MARK: - Make present
 
-class MissingNodesBasicFormatter: BasicFormat {
+class PresentMakingFormatter: BasicFormat {
+  init() {
+    super.init(viewMode: .fixedUp)
+  }
+
   override func isMutable(_ token: TokenSyntax) -> Bool {
     // Assume that all missing nodes will be made present by the Fix-It.
     return token.isMissing
   }
+
+  /// Change the text of all missing tokens to a placeholder with their
+  /// name for diagnostics.
+  override func transformTokenText(_ token: TokenSyntax) -> String? {
+    guard token.isMissing else {
+      return nil
+    }
+
+    let (rawKind, text) = token.tokenKind.decomposeToRaw()
+
+    guard let text = text else {
+      // The token has a default text that we cannot change.
+      return nil
+    }
+
+    if text.isEmpty && rawKind != .stringSegment {
+      // String segments are allowed to have empty text. Replace all other empty
+      // tokens (e.g. missing identifiers) by a placeholder.
+      return "<#\(token.tokenKind.nameForDiagnostics)#>"
+    }
+
+    return nil
+  }
+
+  /// Make all tokens present.
+  override func transformTokenPresence(_ token: TokenSyntax) -> SourcePresence? {
+    guard token.isMissing else {
+      return nil
+    }
+    return .present
+  }
 }
 
 extension FixIt.MultiNodeChange {
@@ -123,8 +158,7 @@ extension FixIt.MultiNodeChange {
     leadingTrivia: Trivia? = nil,
     trailingTrivia: Trivia? = nil
   ) -> Self {
-    var presentNode = MissingNodesBasicFormatter(viewMode: .fixedUp).rewrite(node, detach: true)
-    presentNode = PresentMaker().rewrite(presentNode)
+    var presentNode = PresentMakingFormatter().rewrite(node, detach: true)
 
     if let leadingTrivia {
       presentNode = presentNode.with(\.leadingTrivia, leadingTrivia)

Sources/SwiftParserDiagnostics/PresenceUtils.swift

@@ -43,29 +43,6 @@ extension SyntaxProtocol {
   }
 }
 
-/// Transforms a syntax tree by making all missing tokens present.
-class PresentMaker: SyntaxRewriter {
-  init() {
-    super.init(viewMode: .fixedUp)
-  }
-
-  override func visit(_ token: TokenSyntax) -> TokenSyntax {
-    if token.isMissing {
-      let presentToken: TokenSyntax
-      let (rawKind, text) = token.tokenKind.decomposeToRaw()
-      if let text = text, (!text.isEmpty || rawKind == .stringSegment) {  // string segments can have empty text
-        presentToken = token.with(\.presence, .present)
-      } else {
-        let newKind = TokenKind.fromRaw(kind: rawKind, text: rawKind.defaultText.map(String.init) ?? "<#\(token.tokenKind.nameForDiagnostics)#>")
-        presentToken = token.with(\.tokenKind, newKind).with(\.presence, .present)
-      }
-      return presentToken
-    } else {
-      return token
-    }
-  }
-}
-
 /// Transforms a syntax tree by making all present tokens missing.
 class MissingMaker: SyntaxRewriter {
   init() {

Tests/SwiftBasicFormatTest/BasicFormatTests.swift

@@ -250,6 +250,42 @@ final class BasicFormatTest: XCTestCase {
     )
   }
 
+  func testMultilineStringLiteralWithBlankLines() {
+    assertFormatted(
+      source: #"""
+        assertionFailure("""
+
+          First line
+
+          Second line
+
+          """)
+        """#,
+      expected: #"""
+        assertionFailure("""
+
+          First line
+
+          Second line
+
+          """)
+        """#
+    )
+
+    assertFormatted(
+      source: #"""
+        assertionFailure("""
+
+          """)
+        """#,
+      expected: #"""
+        assertionFailure("""
+
+          """)
+        """#
+    )
+  }
+
   func testNestedUserDefinedIndentation() {
     assertFormatted(
       source: """