Merge pull request swiftlang#257 from google/format-blank-lines

dabelknap · web-flow · commit b99c6e7f7efa · 2019-06-14T16:11:17.000-07:00
Fix false diagnostics in BlankLinesBetweenMembers.
diff --git a/Sources/SwiftFormatCore/Syntax+Convenience.swift b/Sources/SwiftFormatCore/Syntax+Convenience.swift
@@ -27,6 +27,45 @@ extension Syntax {
     }
     return nil
   }
+
+  /// Returns true if the node occupies a single line.
+  ///
+  /// - Parameters:
+  ///   - includingLeadingComment: If true, factor any possible leading comments into the
+  ///     determination of whether the node occupies a single line.
+  ///   - sourceLocationConverter: Used to convert source positions to line/column locations.
+  /// - Returns: True if the node occupies a single line.
+  public func isSingleLine(
+    includingLeadingComment: Bool,
+    sourceLocationConverter: SourceLocationConverter
+  ) -> Bool {
+    guard let firstToken = firstToken, let lastToken = lastToken else { return true }
+
+    let startPosition: AbsolutePosition
+    if includingLeadingComment {
+      // Iterate over the trivia, stopping at the first comment, and using that as the start
+      // position.
+      var currentPosition = firstToken.position
+      for piece in firstToken.leadingTrivia {
+        switch piece {
+        case .lineComment, .blockComment, .docLineComment, .docBlockComment:
+          break
+        default:
+          currentPosition += piece.sourceLength
+        }
+      }
+      startPosition = currentPosition
+    }
+    else {
+      startPosition = firstToken.positionAfterSkippingLeadingTrivia
+    }
+
+    let startLocation = sourceLocationConverter.location(for: startPosition)
+    let endLocation = sourceLocationConverter.location(
+      for: lastToken.endPositionBeforeTrailingTrivia)
+
+    return startLocation.line == endLocation.line
+  }
 }
 
 extension SyntaxCollection {
diff --git a/Sources/SwiftFormatCore/Trivia+Convenience.swift b/Sources/SwiftFormatCore/Trivia+Convenience.swift
@@ -13,7 +13,7 @@
 import SwiftSyntax
 
 extension Trivia {
-  /// Returns the number of whitespace characters after this node.
+  /// Returns the number of whitespace characters in this trivia.
   public var numberOfSpaces: Int {
     var count = 0
     for piece in self {
@@ -24,7 +24,7 @@ extension Trivia {
     return count
   }
 
-  /// Returns the number of newlines after this node.
+  /// Returns the number of newlines in this trivia.
   public var numberOfNewlines: Int {
     var count = 0
     for piece in self {
@@ -35,6 +35,20 @@ extension Trivia {
     return count
   }
 
+  /// Returns the number of leading newlines in this trivia.
+  public var numberOfLeadingNewlines: Int {
+    var count = 0
+    loop: for piece in self {
+      switch piece {
+      case .newlines(let n): count += n
+      case .carriageReturns(let n): count += n
+      case .carriageReturnLineFeeds(let n): count += n
+      default: break loop
+      }
+    }
+    return count
+  }
+
   public var numberOfComments: Int {
     var count = 0
     for piece in self {
diff --git a/Sources/SwiftFormatRules/BlankLineBetweenMembers.swift b/Sources/SwiftFormatRules/BlankLineBetweenMembers.swift
@@ -18,122 +18,62 @@ import SwiftSyntax
 ///
 /// Optionally, declarations of single-line properties can be ignored.
 ///
-/// Lint: If more than the maximum number of blank lines appear, a lint error is raised.
-///       If there are no blank lines between members, a lint error is raised.
+/// This rule does not check the maximum number of blank lines; the pretty printer clamps those
+/// as needed.
+///
+/// Lint: If there are no blank lines between members, a lint error is raised.
 ///
 /// Format: Declarations with no blank lines will have a blank line inserted.
-///         Declarations with more than the maximum number of blank lines will be reduced to the
-///         maximum number of blank lines.
 ///
-/// Configuration: maximumBlankLines, blankLineBetweenMembers.ignoreSingleLineProperties
+/// Configuration: blankLineBetweenMembers.ignoreSingleLineProperties
 ///
 /// - SeeAlso: https://google.github.io/swift#vertical-whitespace
 public final class BlankLineBetweenMembers: SyntaxFormatRule {
   public override func visit(_ node: MemberDeclBlockSyntax) -> Syntax {
-    var membersList = [MemberDeclListItemSyntax]()
-    var hasValidNumOfBlankLines = true
+    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.
+    var membersList = [visitNestedDecls(of: firstMember)]
 
     // Iterates through all the declaration of the member, to ensure that the declarations have
-    // at least on blank line and doesn't exceed the maximum number of blank lines.
-    for member in node.members {
-      let currentMember = checkForNestedMembers(member)
-      guard let memberTrivia = currentMember.leadingTrivia else { continue }
-      let triviaWithoutTrailingSpaces = memberTrivia.withoutTrailingSpaces()
-      guard let firstPiece = triviaWithoutTrailingSpaces.first else { continue }
+    // at least one blank line between them when necessary.
+    var previousMemberWasSingleLine = firstMember.isSingleLine(
+      includingLeadingComment: true,
+      sourceLocationConverter: context.sourceLocationConverter
+    )
 
-      if exceedsMaxBlankLines(triviaWithoutTrailingSpaces) {
-        let correctTrivia = removeExtraBlankLines(triviaWithoutTrailingSpaces, currentMember)
-        let newMember = replaceTrivia(
-          on: currentMember,
-          token: currentMember.firstToken!,
-          leadingTrivia: correctTrivia
-        ) as! MemberDeclListItemSyntax
-        
-        hasValidNumOfBlankLines = false
-        membersList.append(newMember)
-      }
-      // Ensures that there is at least one blank line between each member of a type.
-      // Unless is a single-line declaration and the format is configured to
-      // ignored them.
-      else if case .newlines(let numNewLines) = firstPiece,
-              !ignoreItem(item: currentMember),
-              numNewLines == 1 {
-        let numBlankLines = member.indexInParent == 0 ? 0 : 1
-        let correctTrivia = Trivia.newlines(numBlankLines) + memberTrivia
-        let newMember = replaceTrivia(
-          on: currentMember, token: currentMember.firstToken!,
+    for member in node.members.dropFirst() {
+      var memberToAdd = visitNestedDecls(of: member)
+
+      let memberIsSingleLine = memberToAdd.isSingleLine(
+        includingLeadingComment: true,
+        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
+        memberToAdd = replaceTrivia(
+          on: memberToAdd, token: memberToAdd.firstToken!,
           leadingTrivia: correctTrivia
         ) as! MemberDeclListItemSyntax
-        
-        diagnose(.addBlankLine, on: currentMember)
-        hasValidNumOfBlankLines = false
-        membersList.append(newMember)
-      }
-      else {
-        membersList.append(member)
-      }
-    }
-    
-    return hasValidNumOfBlankLines ? node :
-      node.withMembers(SyntaxFactory.makeMemberDeclList(membersList))
-  }
-  
-  /// Indicates if the given trivia has more than
-  /// the maximum number of blank lines.
-  func exceedsMaxBlankLines(_ trivia: Trivia) -> Bool {
-    let maxBlankLines = context.configuration.maximumBlankLines
 
-    for piece in trivia {
-      if case .newlines(let num) = piece,
-        num - 1 > maxBlankLines {
-        return true
+        diagnose(.addBlankLine, on: memberToAdd)
       }
-    }
-    return false
-  }
-  
-  /// Returns the given trivia without any set of consecutive blank lines
-  /// that exceeds the maximumBlankLines.
-  func removeExtraBlankLines(_ trivia: Trivia, _ member: MemberDeclListItemSyntax) -> Trivia {
-    let maxBlankLines = context.configuration.maximumBlankLines
-    var pieces = [TriviaPiece]()
-    
-    // Iterates through the trivia, verifying that the number of blank
-    // lines in the file do not exceed the maximumBlankLines. If it does
-    // a lint error is raised.
-    for piece in trivia {
-      if case .newlines(let num) = piece,
-         num - 1 > maxBlankLines {
-        pieces.append(.newlines(maxBlankLines + 1))
-        diagnose(.removeBlankLines(count: num - maxBlankLines), on: member)
-      }
-      else {
-        pieces.append(piece)
-      }
-    }
-    return Trivia(pieces: pieces)
-  }
-
-  /// Indicates if a declaration has to be ignored by checking if it's
-  /// a single line and if the format is configured to ignore single lines.
-  func ignoreItem(item: MemberDeclListItemSyntax) -> Bool {
-    guard let firstToken = item.firstToken else { return false }
-    guard let lastToken = item.lastToken else { return false }
 
-    let firstTokenLocation = context.sourceLocationConverter.location(
-      for: firstToken.positionAfterSkippingLeadingTrivia)
-    let lastTokenLocation = context.sourceLocationConverter.location(
-      for: lastToken.positionAfterSkippingLeadingTrivia)
-    let isSingleLine = firstTokenLocation.line == lastTokenLocation.line
-
-    let ignoreLine = context.configuration.blankLineBetweenMembers
-      .ignoreSingleLineProperties
+      membersList.append(memberToAdd)
+      previousMemberWasSingleLine = memberIsSingleLine
+    }
 
-    return isSingleLine && ignoreLine
+    return node.withMembers(SyntaxFactory.makeMemberDeclList(membersList))
   }
 
   /// Recursively ensures all nested member types follows the BlankLineBetweenMembers rule.
-  func checkForNestedMembers(_ member: MemberDeclListItemSyntax) -> MemberDeclListItemSyntax {
+  func visitNestedDecls(of member: MemberDeclListItemSyntax) -> MemberDeclListItemSyntax {
     switch member.decl {
     case let nestedEnum as EnumDeclSyntax:
       let nestedMembers = visit(nestedEnum.members)
@@ -157,22 +97,6 @@ public final class BlankLineBetweenMembers: SyntaxFormatRule {
   }
 }
 
-/// Indicates if the given trivia piece is any type of comment.
-func isComment(_ triviaPiece: TriviaPiece) -> Bool {
-  switch triviaPiece {
-  case .lineComment(_), .docLineComment(_),
-       .blockComment(_), .docBlockComment(_):
-    return true
-  default:
-    return false
-  }
-}
-
 extension Diagnostic.Message {
   static let addBlankLine = Diagnostic.Message(.warning, "add one blank line between declarations")
-  
-  static func removeBlankLines(count: Int) -> Diagnostic.Message {
-    let ending = count > 1 ? "s" : ""
-    return Diagnostic.Message(.warning, "remove \(count) blank line\(ending)")
-  }
 }
diff --git a/Tests/SwiftFormatRulesTests/BlankLineBetweenMembersTests.swift b/Tests/SwiftFormatRulesTests/BlankLineBetweenMembersTests.swift
@@ -5,14 +5,52 @@ import SwiftSyntax
 @testable import SwiftFormatRules
 
 public class BlankLineBetweenMembersTests: DiagnosingTestCase {
+  public func testBlankLineBeforeFirstChildOrNot() {
+    XCTAssertFormatting(
+      BlankLineBetweenMembers.self,
+      input: """
+        struct Foo {
+          /// Doc comment
+          func foo() {
+            code()
+          }
+        }
+
+        struct Bar {
+
+          /// Doc comment
+          func bar() {
+            code()
+          }
+        }
+        """,
+      expected: """
+        struct Foo {
+          /// Doc comment
+          func foo() {
+            code()
+          }
+        }
+
+        struct Bar {
+
+          /// Doc comment
+          func bar() {
+            code()
+          }
+        }
+        """
+    )
+  }
+
   public func testInvalidBlankLineBetweenMembers() {
     XCTAssertFormatting(
       BlankLineBetweenMembers.self,
       input: """
              struct foo1 {
-             
-             
-             
+
+
+
                var test1 = 13
                // Multiline
                // comment for b
@@ -21,9 +59,9 @@ public class BlankLineBetweenMembersTests: DiagnosingTestCase {
 
 
                var c = 11
-                    
 
-                      
+
+
 
                // Multiline comment
                // for d
@@ -38,15 +76,23 @@ public class BlankLineBetweenMembersTests: DiagnosingTestCase {
              """,
       expected: """
                 struct foo1 {
-                
+
+
+
                   var test1 = 13
+
                   // Multiline
                   // comment for b
                   var b = 12
+
                   /*BlockComment*/
-                
+
+
                   var c = 11
 
+
+
+
                   // Multiline comment
                   // for d
                   var d: Bool {
@@ -60,7 +106,7 @@ public class BlankLineBetweenMembersTests: DiagnosingTestCase {
                 }
                 """)
   }
-  
+
   public func testTwoMembers() {
     XCTAssertFormatting(
       BlankLineBetweenMembers.self,
@@ -94,7 +140,7 @@ public class BlankLineBetweenMembersTests: DiagnosingTestCase {
                 }
                 """)
   }
-  
+
   public func testNestedMembers() {
     XCTAssertFormatting(
       BlankLineBetweenMembers.self,
@@ -108,8 +154,6 @@ public class BlankLineBetweenMembersTests: DiagnosingTestCase {
                  case jack, queen, king, ace
                }
 
-
-
                struct secondFoo3 {
                  var a = 1
                  var e: Bool {
@@ -124,6 +168,7 @@ public class BlankLineBetweenMembersTests: DiagnosingTestCase {
                   enum Rank: Int {
                     case two = 2, three, four
 
+
                     case jack, queen, king, ace
                   }
 
