Merge pull request swiftlang#81 from dylansturg/fewer_line_breaks_in_properties

Update BlankLineBetweenMembers rule to remove false positives.

Author: allevato

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),