Merge pull request #597 from allevato/fateful-findings

Fix a bunch of FIXMEs around linter findings.

Author: allevato

Sources/SwiftFormat/PrettyPrint/WhitespaceLinter.swift

@@ -481,7 +481,10 @@ extension Finding.Message {
 
   public static let removeLineError: Finding.Message = "remove line break"
 
-  public static func addLinesError(_ lines: Int) -> Finding.Message { "add \(lines) line breaks" }
+  public static func addLinesError(_ lines: Int) -> Finding.Message {
+    let noun = lines == 1 ? "break" : "breaks"
+    return "add \(lines) line \(noun)"
+  }
 
   public static let lineLengthError: Finding.Message = "line is too long"
 }

Sources/SwiftFormat/Rules/DoNotUseSemicolons.swift

@@ -80,10 +80,18 @@ public final class DoNotUseSemicolons: SyntaxFormatRule {
           }
         }
 
-        // This discards any trailingTrivia from the semicolon. That trivia is at most some spaces,
-        // and the pretty printer adds any necessary spaces so it's safe to discard.
+        // This discards any trailing trivia from the semicolon. That trivia will only be horizontal
+        // whitespace, and the pretty printer adds any necessary spaces so it's safe to discard.
+        // TODO: When we stop using the legacy trivia transform, we need to fix this to preserve
+        // trailing comments.
         newItem = newItem.with(\.semicolon, nil)
-        if idx < node.count - 1 {
+
+        // When emitting the finding, tell the user to move the next statement down if there is
+        // another statement following this one. Otherwise, just tell them to remove the semicolon.
+        if let nextToken = semicolon.nextToken(viewMode: .sourceAccurate),
+          nextToken.tokenKind != .rightBrace && nextToken.tokenKind != .endOfFile
+            && !nextToken.leadingTrivia.containsNewlines
+        {
           diagnose(.removeSemicolonAndMove, on: semicolon)
         } else {
           diagnose(.removeSemicolon, on: semicolon)

Sources/SwiftFormat/Rules/DontRepeatTypeInStaticProperties.swift

@@ -77,7 +77,7 @@ public final class DontRepeatTypeInStaticProperties: SyntaxLintRule {
       for pattern in varDecl.identifiers {
         let varName = pattern.identifier.text
         if varName.contains(bareTypeName) {
-          diagnose(.removeTypeFromName(name: varName, type: bareTypeName), on: varDecl)
+          diagnose(.removeTypeFromName(name: varName, type: bareTypeName), on: pattern.identifier)
         }
       }
     }

Sources/SwiftFormat/Rules/FullyIndirectEnum.swift

@@ -25,12 +25,20 @@ public final class FullyIndirectEnum: SyntaxFormatRule {
   public override func visit(_ node: EnumDeclSyntax) -> DeclSyntax {
     let enumMembers = node.memberBlock.members
     guard !node.modifiers.has(modifier: "indirect"),
-      allCasesAreIndirect(in: enumMembers)
+      case let indirectModifiers = indirectModifiersIfAllCasesIndirect(in: enumMembers),
+      !indirectModifiers.isEmpty
     else {
       return DeclSyntax(node)
     }
 
-    diagnose(.moveIndirectKeywordToEnumDecl(name: node.name.text), on: node.name)
+    let notes = indirectModifiers.map { modifier in
+      Finding.Note(
+        message: .removeIndirect,
+        location: Finding.Location(
+          modifier.startLocation(converter: self.context.sourceLocationConverter)))
+    }
+    diagnose(
+      .moveIndirectKeywordToEnumDecl(name: node.name.text), on: node.enumKeyword, notes: notes)
 
     // Removes 'indirect' keyword from cases, reformats
     let newMembers = enumMembers.map {
@@ -75,17 +83,21 @@ public final class FullyIndirectEnum: SyntaxFormatRule {
   /// Returns a value indicating whether all enum cases in the given list are indirect.
   ///
   /// Note that if the enum has no cases, this returns false.
-  private func allCasesAreIndirect(in members: MemberBlockItemListSyntax) -> Bool {
-    var hadCases = false
+  private func indirectModifiersIfAllCasesIndirect(in members: MemberBlockItemListSyntax)
+    -> [DeclModifierSyntax]
+  {
+    var indirectModifiers = [DeclModifierSyntax]()
     for member in members {
       if let caseMember = member.decl.as(EnumCaseDeclSyntax.self) {
-        hadCases = true
-        guard caseMember.modifiers.has(modifier: "indirect") else {
-          return false
+        guard let indirectModifier = caseMember.modifiers.first(
+          where: { $0.name.text == "indirect" }
+        ) else {
+          return []
         }
+        indirectModifiers.append(indirectModifier)
       }
     }
-    return hadCases
+    return indirectModifiers
   }
 
   /// Transfers given leading trivia to the first token in the case declaration.
@@ -112,6 +124,8 @@ public final class FullyIndirectEnum: SyntaxFormatRule {
 extension Finding.Message {
   @_spi(Rules)
   public static func moveIndirectKeywordToEnumDecl(name: String) -> Finding.Message {
-    "move 'indirect' before the enum declaration '\(name)' when all cases are indirect"
+    "declare enum '\(name)' itself as indirect when all cases are indirect"
   }
+
+  public static let removeIndirect: Finding.Message = "remove 'indirect' here"
 }

Sources/SwiftFormat/Rules/GroupNumericLiterals.swift

@@ -40,21 +40,21 @@ public final class GroupNumericLiterals: SyntaxFormatRule {
       // Hexadecimal
       let digitsNoPrefix = String(originalDigits.dropFirst(2))
       guard digitsNoPrefix.count >= 8 else { return ExprSyntax(node) }
-      diagnose(.groupNumericLiteral(every: 4), on: node)
+      diagnose(.groupNumericLiteral(every: 4, base: "hexadecimal"), on: node)
       newDigits = "0x" + digits(digitsNoPrefix, groupedEvery: 4)
     case "0b":
       // Binary
       let digitsNoPrefix = String(originalDigits.dropFirst(2))
       guard digitsNoPrefix.count >= 10 else { return ExprSyntax(node) }
-      diagnose(.groupNumericLiteral(every: 8), on: node)
+      diagnose(.groupNumericLiteral(every: 8, base: "binary"), on: node)
       newDigits = "0b" + digits(digitsNoPrefix, groupedEvery: 8)
     case "0o":
       // Octal
       return ExprSyntax(node)
     default:
       // Decimal
       guard originalDigits.count >= 7 else { return ExprSyntax(node) }
-      diagnose(.groupNumericLiteral(every: 3), on: node)
+      diagnose(.groupNumericLiteral(every: 3, base: "decimal"), on: node)
       newDigits = digits(originalDigits, groupedEvery: 3)
     }
 
@@ -84,8 +84,7 @@ public final class GroupNumericLiterals: SyntaxFormatRule {
 
 extension Finding.Message {
   @_spi(Rules)
-  public static func groupNumericLiteral(every stride: Int) -> Finding.Message {
-    let ending = stride == 3 ? "rd" : "th"
-    return "group numeric literal using '_' every \(stride)\(ending) number"
+  public static func groupNumericLiteral(every stride: Int, base: String) -> Finding.Message {
+    return "group every \(stride) digits in this \(base) literal using a '_' separator"
   }
 }

Sources/SwiftFormat/Rules/NoAccessLevelOnExtensionDeclaration.swift

@@ -19,11 +19,8 @@ import SwiftSyntax
 /// Format: The access level is removed from the extension declaration and is added to each
 ///         declaration in the extension; declarations with redundant access levels (e.g.
 ///         `internal`, as that is the default access level) have the explicit access level removed.
-///
-/// TODO: Find a better way to access modifiers and keyword tokens besides casting each declaration
 @_spi(Rules)
 public final class NoAccessLevelOnExtensionDeclaration: SyntaxFormatRule {
-
   public override func visit(_ node: ExtensionDeclSyntax) -> DeclSyntax {
     guard !node.modifiers.isEmpty else { return DeclSyntax(node) }
     guard let accessKeyword = node.modifiers.accessLevelModifier else { return DeclSyntax(node) }
@@ -32,21 +29,26 @@ public final class NoAccessLevelOnExtensionDeclaration: SyntaxFormatRule {
     switch keywordKind {
     // Public, private, or fileprivate keywords need to be moved to members
     case .keyword(.public), .keyword(.private), .keyword(.fileprivate):
-      diagnose(.moveAccessKeyword(keyword: accessKeyword.name.text), on: accessKeyword)
-
       // The effective access level of the members of a `private` extension is `fileprivate`, so
       // we have to update the keyword to ensure that the result is correct.
       let accessKeywordToAdd: DeclModifierSyntax
+      let message: Finding.Message
       if keywordKind == .keyword(.private) {
         accessKeywordToAdd
           = accessKeyword.with(\.name, accessKeyword.name.with(\.tokenKind, .keyword(.fileprivate)))
+        message = .moveAccessKeywordAndMakeFileprivate(keyword: accessKeyword.name.text)
       } else {
         accessKeywordToAdd = accessKeyword
+        message = .moveAccessKeyword(keyword: accessKeyword.name.text)
       }
 
+      let (newMemberBlock, notes) = addMemberAccessKeywords(
+        memDeclBlock: node.memberBlock, keyword: accessKeywordToAdd)
+      diagnose(message, on: accessKeyword, notes: notes)
+
       let newMembers = MemberBlockSyntax(
         leftBrace: node.memberBlock.leftBrace,
-        members: addMemberAccessKeywords(memDeclBlock: node.memberBlock, keyword: accessKeywordToAdd),
+        members: newMemberBlock,
         rightBrace: node.memberBlock.rightBrace)
       var newKeyword = node.extensionKeyword
       newKeyword.leadingTrivia = accessKeyword.leadingTrivia
@@ -57,9 +59,7 @@ public final class NoAccessLevelOnExtensionDeclaration: SyntaxFormatRule {
 
     // Internal keyword redundant, delete
     case .keyword(.internal):
-      diagnose(
-        .removeRedundantAccessKeyword(name: node.extendedType.description),
-        on: accessKeyword)
+      diagnose(.removeRedundantAccessKeyword, on: accessKeyword)
       var newKeyword = node.extensionKeyword
       newKeyword.leadingTrivia = accessKeyword.leadingTrivia
       let result = node.with(\.modifiers, node.modifiers.remove(name: accessKeyword.name.text))
@@ -76,33 +76,56 @@ public final class NoAccessLevelOnExtensionDeclaration: SyntaxFormatRule {
   private func addMemberAccessKeywords(
     memDeclBlock: MemberBlockSyntax,
     keyword: DeclModifierSyntax
-  ) -> MemberBlockItemListSyntax {
+  ) -> (MemberBlockItemListSyntax, [Finding.Note]) {
     var newMembers: [MemberBlockItemSyntax] = []
+    var notes: [Finding.Note] = []
 
     var formattedKeyword = keyword
     formattedKeyword.leadingTrivia = []
 
     for memberItem in memDeclBlock.members {
       let member = memberItem.decl
       guard
+        let modifiers = member.asProtocol(WithModifiersSyntax.self)?.modifiers,
         // addModifier relocates trivia for any token(s) displaced by the new modifier.
         let newDecl = addModifier(declaration: member, modifierKeyword: formattedKeyword)
           .as(DeclSyntax.self)
-      else { continue }
+      else {
+        newMembers.append(memberItem)
+        continue
+      }
+
       newMembers.append(memberItem.with(\.decl, newDecl))
+
+      // If it already had an explicit access modifier, don't leave a note.
+      if modifiers.accessLevelModifier == nil {
+        notes.append(Finding.Note(
+          message: .addModifierToExtensionMember(keyword: formattedKeyword.name.text),
+          location: Finding.Location(member.startLocation(converter: context.sourceLocationConverter))
+        ))
+      }
     }
-    return MemberBlockItemListSyntax(newMembers)
+    return (MemberBlockItemListSyntax(newMembers), notes)
   }
 }
 
 extension Finding.Message {
   @_spi(Rules)
-  public static func removeRedundantAccessKeyword(name: String) -> Finding.Message {
-    "remove redundant 'internal' access keyword from '\(name)'"
-  }
+  public static let removeRedundantAccessKeyword: Finding.Message =
+    "remove this redundant 'internal' access modifier from this extension"
 
   @_spi(Rules)
   public static func moveAccessKeyword(keyword: String) -> Finding.Message {
-    "move the '\(keyword)' access keyword to precede each member inside the extension"
+    "move this '\(keyword)' access modifier to precede each member inside this extension"
+  }
+
+  @_spi(Rules)
+  public static func moveAccessKeywordAndMakeFileprivate(keyword: String) -> Finding.Message {
+    "remove this '\(keyword)' access modifier and declare each member inside this extension as 'fileprivate'"
+  }
+
+  @_spi(Rules)
+  public static func addModifierToExtensionMember(keyword: String) -> Finding.Message {
+    "add '\(keyword)' access modifier to this declaration"
   }
 }

Sources/SwiftFormat/Rules/NoCasesWithOnlyFallthrough.swift

@@ -28,7 +28,7 @@ public final class NoCasesWithOnlyFallthrough: SyntaxFormatRule {
     /// Flushes any un-collapsed violations to the new cases list.
     func flushViolations() {
       fallthroughOnlyCases.forEach {
-        newChildren.append(.switchCase(super.visit($0)))
+        newChildren.append(.switchCase(visit($0)))
       }
       fallthroughOnlyCases.removeAll()
     }
@@ -57,7 +57,8 @@ public final class NoCasesWithOnlyFallthrough: SyntaxFormatRule {
         if canMergeWithPreviousCases(switchCase) {
           // If the current case can be merged with the ones before it, merge them all, leaving no
           // `fallthrough`-only cases behind.
-          newChildren.append(.switchCase(visit(mergedCases(fallthroughOnlyCases + [switchCase]))))
+          let newSwitchCase = visit(switchCase)
+          newChildren.append(.switchCase(visit(mergedCases(fallthroughOnlyCases + [newSwitchCase]))))
         } else {
           // If the current case can't be merged with the ones before it, merge the previous ones
           // into a single `fallthrough`-only case and then append the current one. This could
@@ -171,6 +172,11 @@ public final class NoCasesWithOnlyFallthrough: SyntaxFormatRule {
     var newCaseItems: [SwitchCaseItemSyntax] = []
     let labels = cases.lazy.compactMap({ $0.label.as(SwitchCaseLabelSyntax.self) })
     for label in labels.dropLast() {
+      // Diagnose the cases being collapsed. We do this for all but the last one in the array; the
+      // last one isn't diagnosed because it will contain the body that applies to all the previous
+      // cases.
+      diagnose(.collapseCase, on: label)
+
       // We can blindly append all but the last case item because they must already have a trailing
       // comma. Then, we need to add a trailing comma to the last one, since it will be followed by
       // more items.
@@ -179,11 +185,6 @@ public final class NoCasesWithOnlyFallthrough: SyntaxFormatRule {
         label.caseItems.last!.with(
           \.trailingComma, 
           TokenSyntax.commaToken(trailingTrivia: .spaces(1))))
-
-      // Diagnose the cases being collapsed. We do this for all but the last one in the array; the
-      // last one isn't diagnosed because it will contain the body that applies to all the previous
-      // cases.
-      diagnose(.collapseCase, on: label)
     }
     newCaseItems.append(contentsOf: labels.last!.caseItems)
 

Sources/SwiftFormat/Rules/NoEmptyTrailingClosureParentheses.swift

@@ -24,16 +24,18 @@ public final class NoEmptyTrailingClosureParentheses: SyntaxFormatRule {
   public override func visit(_ node: FunctionCallExprSyntax) -> ExprSyntax {
     guard node.arguments.count == 0 else { return super.visit(node) }
 
-    guard let trailingClosure = node.trailingClosure,
-      node.arguments.isEmpty && node.leftParen != nil else
-    {
+    guard
+      let trailingClosure = node.trailingClosure,
+      let leftParen = node.leftParen,
+      node.arguments.isEmpty
+    else {
       return super.visit(node)
     }
     guard let name = node.calledExpression.lastToken(viewMode: .sourceAccurate) else {
       return super.visit(node)
     }
 
-    diagnose(.removeEmptyTrailingParentheses(name: "\(name.trimmedDescription)"), on: node)
+    diagnose(.removeEmptyTrailingParentheses(name: "\(name.trimmedDescription)"), on: leftParen)
 
     // Need to visit `calledExpression` before creating a new node so that the location data (column
     // and line numbers) is available.

Sources/SwiftFormat/Rules/NoParensAroundConditions.swift

@@ -42,7 +42,7 @@ public final class NoParensAroundConditions: SyntaxFormatRule {
       }
     }
 
-    diagnose(.removeParensAroundExpression, on: expr)
+    diagnose(.removeParensAroundExpression, on: tuple.leftParen)
 
     guard
       let visitedTuple = visit(tuple).as(TupleExprSyntax.self),
@@ -75,7 +75,6 @@ public final class NoParensAroundConditions: SyntaxFormatRule {
     return node.with(\.condition, .expression(extractExpr(tup)))
   }
 
-  /// FIXME(hbh): Parsing for SwitchExprSyntax is not implemented.
   public override func visit(_ node: SwitchExprSyntax) -> ExprSyntax {
     guard let tup = node.subject.as(TupleExprSyntax.self),
       tup.elements.firstAndOnly != nil