Merge pull request swiftlang#150 from dylansturg/nuke_duplicate_imports

Delete duplicate imports.

Author: allevato

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