Delete duplicate imports.

The criteria for deletion is:
- Has an identical path, excluding whitespace, as an existing import
- No more than 1 of the matching imports have a trailing comment

The trailing comment rule is necessary because it's impossible to safely combine multiple trailing comments such that they all stay grouped to the relevant import. Leading comments are safe to combine, because OrderedImports keeps multiple lines of leading comments grouped to the next import. Rather than trying to keep the trailing comments on 1 line together or promote 1 trailing comment to a leading comment, it's safer to raise the diagnostic and keep both imports.

Author: dylansturg

Sources/SwiftFormatRules/OrderedImports.swift

@@ -191,24 +191,52 @@ public final class OrderedImports: SyntaxFormatRule {
   /// import lines should be assocaited with it, and move with the line during sorting. We also emit
   /// a linter error if an import line is discovered to be out of order.
   private func formatImports(_ imports: [Line]) -> [Line] {
-    var linesWithLeadingComments: [(Line, [Line])] = []
+    var linesWithLeadingComments: [(import: Line, comments: [Line])] = []
+    var visitedImports: [String: Int] = [:]
     var commentBuffer: [Line] = []
     var previousImport: Line? = nil
     var diagnosed = false
 
     for line in imports {
       switch line.type {
       case .regularImport, .declImport, .testableImport:
+        let fullyQualifiedImport = line.fullyQualifiedImport
+        // Check for duplicate imports and potentially remove them.
+        if let previousMatchingImportIndex = visitedImports[fullyQualifiedImport] {
+          // Even if automatically removing this import is impossible, alert the user that this is a
+          // duplicate so they can manually fix it.
+          diagnose(.removeDuplicateImport, on: line.firstToken)
+          var duplicateLine = linesWithLeadingComments[previousMatchingImportIndex]
+
+          // We can combine multiple leading comments, but it's unsafe to combine trailing comments.
+          // Any extra comments must go on a new line, and would be grouped with the next import.
+          guard !duplicateLine.import.trailingTrivia.isEmpty && !line.trailingTrivia.isEmpty else {
+            duplicateLine.comments.append(contentsOf: commentBuffer)
+            commentBuffer = []
+            // Keep the Line that has the trailing comment, if there is one.
+            if !line.trailingTrivia.isEmpty {
+              duplicateLine.import = line
+            }
+            linesWithLeadingComments[previousMatchingImportIndex] = duplicateLine
+            continue
+          }
+          // Otherwise, both lines have trailing trivia so it's not safe to automatically merge
+          // them. Leave this duplicate import.
+        }
         if let previousImport = previousImport,
           line.importName.lexicographicallyPrecedes(previousImport.importName) && !diagnosed
+            // Only warn to sort imports that shouldn't be removed.
+            && visitedImports[fullyQualifiedImport] == nil
         {
           diagnose(.sortImports, on: line.firstToken)
           diagnosed = true  // Only emit one of these errors to avoid alert fatigue.
         }
+
         // Pack the import line and its associated comments into a tuple.
         linesWithLeadingComments.append((line, commentBuffer))
         commentBuffer = []
         previousImport = line
+        visitedImports[fullyQualifiedImport] = linesWithLeadingComments.endIndex - 1
       case .comment:
         commentBuffer.append(line)
       default: ()
@@ -449,13 +477,33 @@ fileprivate class Line {
     return .blankLine
   }
 
+  /// Returns a fully qualified description of this line's import if it's an import statement,
+  /// including any attributes, modifiers, the import kind, and the import path. When this line
+  /// isn't an import statement, returns an empty string.
+  var fullyQualifiedImport: String {
+    guard let syntaxNode = syntaxNode, case .importCodeBlock(let importCodeBlock, _) = syntaxNode,
+      let importDecl = importCodeBlock.item.as(ImportDeclSyntax.self)
+    else {
+      return ""
+    }
+    // Using the description is a reliable way to include all content from the import, but
+    // description includes all leading and trailing trivia. It would be unusual to have any
+    // non-whitespace trivia on the components of the import. Trim off the leading trivia, where
+    // comments could be, and trim whitespace that might be after the import.
+    let leadingText = importDecl.leadingTrivia?.reduce(into: "") { $1.write(to: &$0) } ?? ""
+    return importDecl.description.dropFirst(leadingText.count)
+      .trimmingCharacters(in: .whitespacesAndNewlines)
+  }
+
+  /// Returns the path that is imported by this line's import statement if it's an import statement.
+  /// When this line isn't an import statement, returns an empty string.
   var importName: String {
     guard let syntaxNode = syntaxNode, case .importCodeBlock(let importCodeBlock, _) = syntaxNode,
       let importDecl = importCodeBlock.item.as(ImportDeclSyntax.self)
     else {
       return ""
     }
-    return importDecl.path.description
+    return importDecl.path.description.trimmingCharacters(in: .whitespaces)
   }
 
   /// Returns the first `TokenSyntax` in the code block(s) from this Line, or nil when this Line
@@ -535,6 +583,8 @@ extension Diagnostic.Message {
     return Diagnostic.Message(.warning, "place \(before) imports before \(after) imports")
   }
 
+  public static let removeDuplicateImport = Diagnostic.Message(.warning, "remove duplicate import")
+
   public static let sortImports =
     Diagnostic.Message(.warning, "sort import statements lexicographically")
 }

Tests/SwiftFormatRulesTests/OrderedImportsTests.swift

@@ -395,4 +395,150 @@ final class OrderedImportsTests: LintOrFormatRuleTestCase {
       checkForUnassertedDiagnostics: true
     )
   }
+
+  func testRemovesDuplicateImports() {
+    let input =
+      """
+      import CoreLocation
+      import UIKit
+      import CoreLocation
+      import ZeeFramework
+      bar()
+      """
+
+    let expected =
+      """
+      import CoreLocation
+      import UIKit
+      import ZeeFramework
+
+      bar()
+      """
+
+    XCTAssertFormatting(
+      OrderedImports.self, input: input, expected: expected, checkForUnassertedDiagnostics: true)
+    XCTAssertDiagnosed(.removeDuplicateImport, line: 3, column: 1)
+  }
+
+  func testDuplicateCommentedImports() {
+    let input =
+      """
+      import AppKit
+      // CoreLocation is necessary to get location stuff.
+      import CoreLocation  // This import must stay.
+      // UIKit does UI Stuff?
+      import UIKit
+      // This is the second CoreLocation import.
+      import CoreLocation  // The 2nd CL import has a comment here too.
+      // Comment about ZeeFramework.
+      import ZeeFramework
+      import foo
+      // Second comment about ZeeFramework.
+      import ZeeFramework  // This one has a trailing comment too.
+      foo()
+      """
+
+    let expected =
+      """
+      import AppKit
+      // CoreLocation is necessary to get location stuff.
+      import CoreLocation  // This import must stay.
+      // This is the second CoreLocation import.
+      import CoreLocation  // The 2nd CL import has a comment here too.
+      // UIKit does UI Stuff?
+      import UIKit
+      // Comment about ZeeFramework.
+      // Second comment about ZeeFramework.
+      import ZeeFramework  // This one has a trailing comment too.
+      import foo
+
+      foo()
+      """
+
+    XCTAssertFormatting(
+      OrderedImports.self, input: input, expected: expected, checkForUnassertedDiagnostics: true)
+
+    // Even though this import is technically also not sorted, that won't matter if the import is
+    // removed so there should only be a warning to remove it.
+    XCTAssertDiagnosed(.removeDuplicateImport, line: 7, column: 1)
+    XCTAssertDiagnosed(.removeDuplicateImport, line: 12, column: 1)
+  }
+
+  func testDuplicateIgnoredImports() {
+    let input =
+      """
+      import AppKit
+      // swift-format-ignore
+      import CoreLocation
+      // Second CoreLocation import here.
+      import CoreLocation
+      // Comment about ZeeFramework.
+      import ZeeFramework
+      // swift-format-ignore
+      import ZeeFramework  // trailing comment
+      foo()
+      """
+
+    let expected =
+      """
+      import AppKit
+
+      // swift-format-ignore
+      import CoreLocation
+
+      // Second CoreLocation import here.
+      import CoreLocation
+      // Comment about ZeeFramework.
+      import ZeeFramework
+
+      // swift-format-ignore
+      import ZeeFramework  // trailing comment
+
+      foo()
+      """
+
+    XCTAssertFormatting(
+      OrderedImports.self, input: input, expected: expected, checkForUnassertedDiagnostics: true)
+  }
+
+  func testDuplicateAttributedImports() {
+    let input =
+      """
+      // imports an enum
+      import enum Darwin.D.isatty
+      // this is a dup
+      import enum Darwin.D.isatty
+      import foo
+      import a
+      @testable import foo
+      // exported import of bar
+      @_exported import bar
+      @_implementationOnly import bar
+      import bar
+      // second import of foo
+      import foo
+      baz()
+      """
+
+    let expected =
+      """
+      import a
+      // exported import of bar
+      @_exported import bar
+      @_implementationOnly import bar
+      import bar
+      // second import of foo
+      import foo
+
+      // imports an enum
+      // this is a dup
+      import enum Darwin.D.isatty
+
+      @testable import foo
+
+      baz()
+      """
+
+    XCTAssertFormatting(OrderedImports.self, input: input, expected: expected)
+  }
 }

Tests/SwiftFormatRulesTests/XCTestManifests.swift

@@ -266,13 +266,17 @@ extension OrderedImportsTests {
     static let __allTests__OrderedImportsTests = [
         ("testDisableOrderedImports", testDisableOrderedImports),
         ("testDisableOrderedImportsMovingComments", testDisableOrderedImportsMovingComments),
+        ("testDuplicateAttributedImports", testDuplicateAttributedImports),
+        ("testDuplicateCommentedImports", testDuplicateCommentedImports),
+        ("testDuplicateIgnoredImports", testDuplicateIgnoredImports),
         ("testEmptyFile", testEmptyFile),
         ("testImportsOrderWithDocComment", testImportsOrderWithDocComment),
         ("testImportsOrderWithoutModuleType", testImportsOrderWithoutModuleType),
         ("testInvalidImportsOrder", testInvalidImportsOrder),
         ("testMultipleCodeBlocksPerLine", testMultipleCodeBlocksPerLine),
         ("testMultipleCodeBlocksWithImportsPerLine", testMultipleCodeBlocksWithImportsPerLine),
         ("testNonHeaderComment", testNonHeaderComment),
+        ("testRemovesDuplicateImports", testRemovesDuplicateImports),
         ("testSeparatedFileHeader", testSeparatedFileHeader),
         ("testValidOrderedImport", testValidOrderedImport),
     ]