Update BlankLineBetweenMembers rule to remove false positives.

The rule was overly eager in adding blank lines around members that have comments in their leading trivia, because it didn't always correctly determine whether a given comment should trigger blank lines around the member.

I have refactored the rule using the following criteria for adding blank lines:
1. Any member whose leading trivia contains a non-blank-line-terminated comment must have a leading blank line (before the comment) and a trailing blank line (after the member).
2. Any comment that contains at least 1 blank line between the comment and the relevant syntax token is ignored entirely by the rule.

Author: dylansturg

Sources/SwiftFormatConfiguration/Configuration.swift

@@ -159,7 +159,11 @@ public class Configuration: Codable {
 /// Configuration for the BlankLineBetweenMembers rule.
 public struct BlankLineBetweenMembersConfiguration: Codable {
   /// If true, blank lines are not required between single-line properties.
-  public let ignoreSingleLineProperties = true
+  public let ignoreSingleLineProperties: Bool
+
+  public init(ignoreSingleLineProperties: Bool = true) {
+    self.ignoreSingleLineProperties = ignoreSingleLineProperties
+  }
 }
 
 /// Configuration for the NoPlaygroundLiterals rule.

Sources/SwiftFormatRules/BlankLineBetweenMembers.swift

@@ -30,8 +30,12 @@ import SwiftSyntax
 /// - SeeAlso: https://google.github.io/swift#vertical-whitespace
 public final class BlankLineBetweenMembers: SyntaxFormatRule {
   public override func visit(_ node: MemberDeclBlockSyntax) -> Syntax {
-    guard let firstMember = node.members.first else { return super.visit(node) }
+    // This rule is implemented by ensuring that all multiline member decls include a blank line
+    // before the member's leading trivia and a blank line after any members that have a preceding
+    // comment. Any comments that are separated from a member with blank lines are ignored, because
+    // those comments aren't considered to be related to a specific member.
 
+    guard let firstMember = node.members.first else { return super.visit(node) }
     let ignoreSingleLine = context.configuration.blankLineBetweenMembers.ignoreSingleLineProperties
 
     // The first member can just be added as-is; we don't force any newline before it.
@@ -40,45 +44,45 @@ public final class BlankLineBetweenMembers: SyntaxFormatRule {
     // The first comment may not be "attached" to the first member. Ignore any comments that are
     // separated from the member by a newline.
     let shouldIncludeLeadingComment = !isLeadingTriviaSeparate(from: firstMember)
-
-    // Iterates through all the declaration of the member, to ensure that the declarations have
-    // at least one blank line between them when necessary.
     var previousMemberWasSingleLine = firstMember.isSingleLine(
       includingLeadingComment: shouldIncludeLeadingComment,
       sourceLocationConverter: context.sourceLocationConverter
     )
 
+    // Iterates through all the declaration of the member, to ensure that the declarations have
+    // at least one blank line between them when necessary.
     for member in node.members.dropFirst() {
       var memberToAdd = visitNestedDecls(of: member)
 
-      let memberIsSingleLine = memberToAdd.isSingleLine(
+      // Include the comment here to ensure all comments have a preceding blank line.
+      let isMemberSingleLine = memberToAdd.isSingleLine(
         includingLeadingComment: true,
-        sourceLocationConverter: context.sourceLocationConverter
-      )
+        sourceLocationConverter: context.sourceLocationConverter)
 
-      if let memberTrivia = memberToAdd.leadingTrivia,
-        !(ignoreSingleLine && previousMemberWasSingleLine && memberIsSingleLine)
-          && memberTrivia.numberOfLeadingNewlines == 1  // Only one newline => no blank line
-      {
-        let correctTrivia = Trivia.newlines(1) + memberTrivia
+      let ignoreMember = ignoreSingleLine && isMemberSingleLine
+      if (!previousMemberWasSingleLine || !ignoreMember) && !isLeadingBlankLinePresent(on: member) {
+        let blankLinePrefacedTrivia = Trivia.newlines(1) + (member.leadingTrivia ?? [])
         memberToAdd = replaceTrivia(
-          on: memberToAdd, token: memberToAdd.firstToken!,
-          leadingTrivia: correctTrivia
+          on: memberToAdd,
+          token: memberToAdd.firstToken,
+          leadingTrivia: blankLinePrefacedTrivia
         ) as! MemberDeclListItemSyntax
-
         diagnose(.addBlankLine, on: member)
       }
 
       membersList.append(memberToAdd)
-      previousMemberWasSingleLine = memberIsSingleLine
+
+      // Consider the member single line if the trivia was separate so that non-member-specific
+      // comments won't cause blank lines between members that are otherwise single line.
+      previousMemberWasSingleLine = isMemberSingleLine || isLeadingTriviaSeparate(from: memberToAdd)
     }
 
     return node.withMembers(SyntaxFactory.makeMemberDeclList(membersList))
   }
 
   /// Returns whether any comments in the leading trivia of the given node are separated from the
   /// non-trivia tokens by at least 1 blank line.
-  func isLeadingTriviaSeparate(from node: Syntax) -> Bool {
+  private func isLeadingTriviaSeparate(from node: Syntax) -> Bool {
     guard let leadingTrivia = node.leadingTrivia else {
       return false
     }
@@ -88,6 +92,14 @@ public final class BlankLineBetweenMembers: SyntaxFormatRule {
     return false
   }
 
+  /// Returns whether there is at least 1 blank line in the leading trivia of the given node.
+  private func isLeadingBlankLinePresent(on node: Syntax) -> Bool {
+    guard let leadingTrivia = node.leadingTrivia else {
+      return false
+    }
+    return leadingTrivia.numberOfLeadingNewlines > 1
+  }
+
   /// Recursively ensures all nested member types follows the BlankLineBetweenMembers rule.
   func visitNestedDecls(of member: MemberDeclListItemSyntax) -> MemberDeclListItemSyntax {
     switch member.decl {

Tests/SwiftFormatRulesTests/BlankLineBetweenMembersTests.swift

@@ -1,6 +1,7 @@
 import Foundation
-import XCTest
+import SwiftFormatConfiguration
 import SwiftSyntax
+import XCTest
 
 @testable import SwiftFormatRules
 
@@ -192,38 +193,146 @@ public class BlankLineBetweenMembersTests: DiagnosingTestCase {
                  let baz = 2
                }
                enum Foo {
-
                  // MARK: - This is an important region of the code.
 
                  let bar = 1
                  let baz = 2
                }
                enum Foo {
+                 var quxxe = 0
+                 // MARK: - This is an important region of the code.
 
-                 // This comment is describing bar.
                  let bar = 1
                  let baz = 2
                }
+               enum Foo {
+                 let bar = 1
+                 let baz = 2
+
+                 // MARK: - This is an important region of the code.
+               }
                """,
         expected: """
                   enum Foo {
                     let bar = 1
                     let baz = 2
                   }
                   enum Foo {
+                    // MARK: - This is an important region of the code.
+
+                    let bar = 1
+                    let baz = 2
+                  }
+                  enum Foo {
+                    var quxxe = 0
 
                     // MARK: - This is an important region of the code.
 
                     let bar = 1
                     let baz = 2
                   }
+                  enum Foo {
+                    let bar = 1
+                    let baz = 2
+
+                    // MARK: - This is an important region of the code.
+                  }
+                  """)
+  }
+
+  public func testBlankLinesAroundDocumentedMembers() {
+    XCTAssertFormatting(
+           BlankLineBetweenMembers.self,
+           input: """
                   enum Foo {
 
                     // This comment is describing bar.
                     let bar = 1
+                    let baz = 2
+                    let quxxe = 3
+                  }
+                  enum Foo {
+                    var quxxe = 0
 
+                    /// bar: A property that has a Bar.
+                    let bar = 1
                     let baz = 2
+                    var car = 3
                   }
-                  """)
+                  """,
+           expected: """
+                     enum Foo {
+
+                       // This comment is describing bar.
+                       let bar = 1
+
+                       let baz = 2
+                       let quxxe = 3
+                     }
+                     enum Foo {
+                       var quxxe = 0
+
+                       /// bar: A property that has a Bar.
+                       let bar = 1
+
+                       let baz = 2
+                       var car = 3
+                     }
+                     """)
+  }
+
+  public func testBlankLineBetweenMembersIgnoreSingleLineDisabled() {
+    let config = Configuration()
+    config.blankLineBetweenMembers =
+      BlankLineBetweenMembersConfiguration(ignoreSingleLineProperties: false)
+
+    let input = """
+      enum Foo {
+        let bar = 1
+        let baz = 2
+      }
+      enum Foo {
+        // MARK: - This is an important region of the code.
+
+        let bar = 1
+        let baz = 2
+      }
+      enum Foo {
+        var quxxe = 0
+        // MARK: - This is an important region of the code.
+
+        let bar = 1
+        let baz = 2
+      }
+      """
+    let expected = """
+      enum Foo {
+        let bar = 1
+
+        let baz = 2
+      }
+      enum Foo {
+        // MARK: - This is an important region of the code.
+
+        let bar = 1
+
+        let baz = 2
+      }
+      enum Foo {
+        var quxxe = 0
+
+        // MARK: - This is an important region of the code.
+
+        let bar = 1
+
+        let baz = 2
+      }
+      """
+
+    XCTAssertFormatting(
+      BlankLineBetweenMembers.self,
+      input: input,
+      expected: expected,
+      configuration: config)
   }
 }

Tests/SwiftFormatRulesTests/DiagnosingTestCase.swift

@@ -38,9 +38,11 @@ public class DiagnosingTestCase: XCTestCase {
   }
 
   /// Creates and returns a new `Context` from the given syntax tree.
-  private func makeContext(sourceFileSyntax: SourceFileSyntax) -> Context {
+  private func makeContext(sourceFileSyntax: SourceFileSyntax, configuration: Configuration? = nil)
+    -> Context
+  {
     let context = Context(
-      configuration: Configuration(),
+      configuration: configuration ?? Configuration(),
       diagnosticEngine: DiagnosticEngine(),
       fileURL: URL(fileURLWithPath: "/tmp/test.swift"),
       sourceFileSyntax: sourceFileSyntax)
@@ -101,7 +103,8 @@ public class DiagnosingTestCase: XCTestCase {
     expected: String,
     file: StaticString = #file,
     line: UInt = #line,
-    checkForUnassertedDiagnostics: Bool = false
+    checkForUnassertedDiagnostics: Bool = false,
+    configuration: Configuration? = nil
   ) {
     let sourceFileSyntax: SourceFileSyntax
     do {
@@ -111,7 +114,7 @@ public class DiagnosingTestCase: XCTestCase {
       return
     }
 
-    context = makeContext(sourceFileSyntax: sourceFileSyntax)
+    context = makeContext(sourceFileSyntax: sourceFileSyntax, configuration: configuration)
 
     // Force the rule to be enabled while we test it.
     context!.configuration.rules[formatType.ruleName] = true

Tests/SwiftFormatRulesTests/XCTestManifests.swift

@@ -45,6 +45,8 @@ extension BlankLineBetweenMembersTests {
     // to regenerate.
     static let __allTests__BlankLineBetweenMembersTests = [
         ("testBlankLineBeforeFirstChildOrNot", testBlankLineBeforeFirstChildOrNot),
+        ("testBlankLineBetweenMembersIgnoreSingleLineDisabled", testBlankLineBetweenMembersIgnoreSingleLineDisabled),
+        ("testBlankLinesAroundDocumentedMembers", testBlankLinesAroundDocumentedMembers),
         ("testInvalidBlankLineBetweenMembers", testInvalidBlankLineBetweenMembers),
         ("testNestedMembers", testNestedMembers),
         ("testNoBlankLineBetweenSingleLineMembers", testNoBlankLineBetweenSingleLineMembers),