Merge pull request #865 from macshome/main

Fix support for `FileIterator` when working directory is `/`

Author: ahoppen

.github/workflows/pull_request.yml

@@ -16,3 +16,4 @@ jobs:
     with:
       license_header_check_enabled: false
       license_header_check_project_name: "Swift.org"
+      api_breakage_check_allowlist_path: "api-breakages.txt"

Sources/SwiftFormat/API/Configuration.swift

@@ -486,17 +486,3 @@ public struct NoAssignmentInExpressionsConfiguration: Codable, Equatable {
 
   public init() {}
 }
-
-fileprivate extension URL {
-  var isRoot: Bool {
-    #if os(Windows)
-    // FIXME: We should call into Windows' native check to check if this path is a root once https://github.com/swiftlang/swift-foundation/issues/976 is fixed.
-    // https://github.com/swiftlang/swift-format/issues/844
-    return self.pathComponents.count <= 1
-    #else
-    // On Linux, we may end up with an string for the path due to https://github.com/swiftlang/swift-foundation/issues/980
-    // TODO: Remove the check for "" once https://github.com/swiftlang/swift-foundation/issues/980 is fixed.
-    return self.path == "/" || self.path == ""
-    #endif
-  }
-}

Sources/SwiftFormat/CMakeLists.txt

@@ -97,7 +97,8 @@ add_library(SwiftFormat
   Rules/UseTripleSlashForDocumentationComments.swift
   Rules/UseWhereClausesInForLoops.swift
   Rules/ValidateDocumentationComments.swift
-  Utilities/FileIterator.swift)
+  Utilities/FileIterator.swift
+  Utilities/URL+isRoot.swift)
 target_link_libraries(SwiftFormat PUBLIC
   SwiftMarkdown::Markdown
   SwiftSyntax::SwiftSyntax

Sources/SwiftFormat/Utilities/FileIterator.swift

@@ -12,6 +12,10 @@
 
 import Foundation
 
+#if os(Windows)
+import WinSDK
+#endif
+
 /// Iterator for looping over lists of files and directories. Directories are automatically
 /// traversed recursively, and we check for files with a ".swift" extension.
 @_spi(Internal)
@@ -32,7 +36,7 @@ public struct FileIterator: Sequence, IteratorProtocol {
 
   /// The current working directory of the process, which is used to relativize URLs of files found
   /// during iteration.
-  private let workingDirectory = URL(fileURLWithPath: ".")
+  private let workingDirectory: URL
 
   /// Keep track of the current directory we're recursing through.
   private var currentDirectory = URL(fileURLWithPath: "")
@@ -46,8 +50,13 @@ public struct FileIterator: Sequence, IteratorProtocol {
   /// Create a new file iterator over the given list of file URLs.
   ///
   /// The given URLs may be files or directories. If they are directories, the iterator will recurse
-  /// into them.
-  public init(urls: [URL], followSymlinks: Bool) {
+  /// into them. Symlinks are never followed on Windows platforms as Foundation doesn't support it.
+  /// - Parameters:
+  ///   - urls: `Array` of files or directories to iterate.
+  ///   - followSymlinks: `Bool` to indicate if symbolic links should be followed when iterating.
+  ///   - workingDirectory: `URL` that indicates the current working directory. Used for testing.
+  public init(urls: [URL], followSymlinks: Bool, workingDirectory: URL = URL(fileURLWithPath: ".")) {
+    self.workingDirectory = workingDirectory
     self.urls = urls
     self.urlIterator = self.urls.makeIterator()
     self.followSymlinks = followSymlinks
@@ -144,12 +153,13 @@ public struct FileIterator: Sequence, IteratorProtocol {
         // if the user passes paths that are relative to the current working directory, they will
         // be displayed as relative paths. Otherwise, they will still be displayed as absolute
         // paths.
-        let relativePath =
-          path.hasPrefix(workingDirectory.path)
-          ? String(path.dropFirst(workingDirectory.path.count + 1))
-          : path
-        output =
-          URL(fileURLWithPath: relativePath, isDirectory: false, relativeTo: workingDirectory)
+        let relativePath: String
+        if !workingDirectory.isRoot, path.hasPrefix(workingDirectory.path) {
+          relativePath = String(path.dropFirst(workingDirectory.path.count).drop(while: { $0 == "/" || $0 == #"\"# }))
+        } else {
+          relativePath = path
+        }
+        output = URL(fileURLWithPath: relativePath, isDirectory: false, relativeTo: workingDirectory)
 
       default:
         break

Sources/SwiftFormat/Utilities/URL+isRoot.swift

@@ -0,0 +1,32 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2023 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import Foundation
+
+extension URL {
+  @_spi(Testing) public var isRoot: Bool {
+    #if os(Windows)
+    // FIXME: We should call into Windows' native check to check if this path is a root once https://github.com/swiftlang/swift-foundation/issues/976 is fixed.
+    // https://github.com/swiftlang/swift-format/issues/844
+    var pathComponents = self.pathComponents
+    if pathComponents.first == "/" {
+      // Canonicalize `/C:/` to `C:/`.
+      pathComponents = Array(pathComponents.dropFirst())
+    }
+    return pathComponents.count <= 1
+    #else
+    // On Linux, we may end up with an string for the path due to https://github.com/swiftlang/swift-foundation/issues/980
+    // TODO: Remove the check for "" once https://github.com/swiftlang/swift-foundation/issues/980 is fixed.
+    return self.path == "/" || self.path == ""
+    #endif
+  }
+}

Tests/SwiftFormatTests/Utilities/FileIteratorTests.swift

@@ -1,6 +1,31 @@
-@_spi(Internal) import SwiftFormat
+@_spi(Internal) @_spi(Testing) import SwiftFormat
 import XCTest
 
+extension URL {
+  /// Assuming this is a file URL, resolves all symlinks in the path.
+  ///
+  /// - Note: We need this because `URL.resolvingSymlinksInPath()` not only resolves symlinks but also standardizes the
+  ///   path by stripping away `private` prefixes. Since sourcekitd is not performing this standardization, using
+  ///   `resolvingSymlinksInPath` can lead to slightly mismatched URLs between the sourcekit-lsp response and the test
+  ///   assertion.
+  fileprivate var realpath: URL {
+    #if canImport(Darwin)
+    return self.path.withCString { path in
+      guard let realpath = Darwin.realpath(path, nil) else {
+        return self
+      }
+      let result = URL(fileURLWithPath: String(cString: realpath))
+      free(realpath)
+      return result
+    }
+    #else
+    // Non-Darwin platforms don't have the `/private` stripping issue, so we can just use `self.resolvingSymlinksInPath`
+    // here.
+    return self.resolvingSymlinksInPath()
+    #endif
+  }
+}
+
 final class FileIteratorTests: XCTestCase {
   private var tmpdir: URL!
 
@@ -10,7 +35,7 @@ final class FileIteratorTests: XCTestCase {
       in: .userDomainMask,
       appropriateFor: FileManager.default.temporaryDirectory,
       create: true
-    )
+    ).realpath
 
     // Create a simple file tree used by the tests below.
     try touch("project/real1.swift")
@@ -31,8 +56,8 @@ final class FileIteratorTests: XCTestCase {
     #endif
     let seen = allFilesSeen(iteratingOver: [tmpdir], followSymlinks: false)
     XCTAssertEqual(seen.count, 2)
-    XCTAssertTrue(seen.contains { $0.hasSuffix("project/real1.swift") })
-    XCTAssertTrue(seen.contains { $0.hasSuffix("project/real2.swift") })
+    XCTAssertTrue(seen.contains { $0.path.hasSuffix("project/real1.swift") })
+    XCTAssertTrue(seen.contains { $0.path.hasSuffix("project/real2.swift") })
   }
 
   func testFollowSymlinks() throws {
@@ -41,10 +66,10 @@ final class FileIteratorTests: XCTestCase {
     #endif
     let seen = allFilesSeen(iteratingOver: [tmpdir], followSymlinks: true)
     XCTAssertEqual(seen.count, 3)
-    XCTAssertTrue(seen.contains { $0.hasSuffix("project/real1.swift") })
-    XCTAssertTrue(seen.contains { $0.hasSuffix("project/real2.swift") })
+    XCTAssertTrue(seen.contains { $0.path.hasSuffix("project/real1.swift") })
+    XCTAssertTrue(seen.contains { $0.path.hasSuffix("project/real2.swift") })
     // Hidden but found through the visible symlink project/link.swift
-    XCTAssertTrue(seen.contains { $0.hasSuffix("project/.hidden.swift") })
+    XCTAssertTrue(seen.contains { $0.path.hasSuffix("project/.hidden.swift") })
   }
 
   func testTraversesHiddenFilesIfExplicitlySpecified() throws {
@@ -56,8 +81,8 @@ final class FileIteratorTests: XCTestCase {
       followSymlinks: false
     )
     XCTAssertEqual(seen.count, 2)
-    XCTAssertTrue(seen.contains { $0.hasSuffix("project/.build/generated.swift") })
-    XCTAssertTrue(seen.contains { $0.hasSuffix("project/.hidden.swift") })
+    XCTAssertTrue(seen.contains { $0.path.hasSuffix("project/.build/generated.swift") })
+    XCTAssertTrue(seen.contains { $0.path.hasSuffix("project/.hidden.swift") })
   }
 
   func testDoesNotFollowSymlinksIfFollowSymlinksIsFalseEvenIfExplicitlySpecified() {
@@ -71,6 +96,32 @@ final class FileIteratorTests: XCTestCase {
     )
     XCTAssertTrue(seen.isEmpty)
   }
+
+  func testDoesNotTrimFirstCharacterOfPathIfRunningInRoot() throws {
+    // Find the root of tmpdir. On Unix systems, this is always `/`. On Windows it is the drive.
+    var root = tmpdir!
+    while !root.isRoot {
+      root.deleteLastPathComponent()
+    }
+    var rootPath = root.path
+    #if os(Windows) && compiler(<6.1)
+    if rootPath.hasPrefix("/") {
+      // Canonicalize /C: to C:
+      rootPath = String(rootPath.dropFirst())
+    }
+    #endif
+    // Make sure that we don't drop the beginning of the path if we are running in root.
+    // https://github.com/swiftlang/swift-format/issues/862
+    let seen = allFilesSeen(iteratingOver: [tmpdir], followSymlinks: false, workingDirectory: root).map(\.relativePath)
+    XCTAssertTrue(seen.allSatisfy { $0.hasPrefix(rootPath) }, "\(seen) does not contain root directory '\(rootPath)'")
+  }
+
+  func testShowsRelativePaths() throws {
+    // Make sure that we still show the relative path if using them.
+    // https://github.com/swiftlang/swift-format/issues/862
+    let seen = allFilesSeen(iteratingOver: [tmpdir], followSymlinks: false, workingDirectory: tmpdir)
+    XCTAssertEqual(Set(seen.map(\.relativePath)), ["project/real1.swift", "project/real2.swift"])
+  }
 }
 
 extension FileIteratorTests {
@@ -111,11 +162,15 @@ extension FileIteratorTests {
   }
 
   /// Computes the list of all files seen by using `FileIterator` to iterate over the given URLs.
-  private func allFilesSeen(iteratingOver urls: [URL], followSymlinks: Bool) -> [String] {
-    let iterator = FileIterator(urls: urls, followSymlinks: followSymlinks)
-    var seen: [String] = []
+  private func allFilesSeen(
+    iteratingOver urls: [URL],
+    followSymlinks: Bool,
+    workingDirectory: URL = URL(fileURLWithPath: ".")
+  ) -> [URL] {
+    let iterator = FileIterator(urls: urls, followSymlinks: followSymlinks, workingDirectory: workingDirectory)
+    var seen: [URL] = []
     for next in iterator {
-      seen.append(next.path)
+      seen.append(next)
     }
     return seen
   }

api-breakages.txt

@@ -0,0 +1,4 @@
+6.2
+---
+
+API breakage: constructor FileIterator.init(urls:followSymlinks:) has been removed