Merge pull request #644 from allevato/no-dot-files

Ignore symlinks and hidden (dot) files during `--recursive`.

Author: allevato

Package.swift

@@ -130,6 +130,10 @@ let package = Package(
         .product(name: "SwiftSyntaxBuilder", package: "swift-syntax"),
       ]
     ),
+    .testTarget(
+      name: "swift-formatTests",
+      dependencies: ["swift-format"]
+    ),
   ]
 )
 

Sources/swift-format/Frontend/Frontend.swift

@@ -130,12 +130,17 @@ class Frontend {
       "processURLs(_:) should only be called when 'urls' is non-empty.")
 
     if parallel {
-      let filesToProcess = FileIterator(urls: urls).compactMap(openAndPrepareFile)
+      let filesToProcess =
+        FileIterator(urls: urls, followSymlinks: lintFormatOptions.followSymlinks)
+        .compactMap(openAndPrepareFile)
       DispatchQueue.concurrentPerform(iterations: filesToProcess.count) { index in
         processFile(filesToProcess[index])
       }
     } else {
-      FileIterator(urls: urls).lazy.compactMap(openAndPrepareFile).forEach(processFile)
+      FileIterator(urls: urls, followSymlinks: lintFormatOptions.followSymlinks)
+        .lazy
+        .compactMap(openAndPrepareFile)
+        .forEach(processFile)
     }
   }
 

Sources/swift-format/Subcommands/LintFormatOptions.swift

@@ -71,6 +71,13 @@ struct LintFormatOptions: ParsableArguments {
       """)
   var colorDiagnostics: Bool?
 
+  /// Whether symlinks should be followed.
+  @Flag(help: """
+    Follow symbolic links passed on the command line, or found during directory traversal when \
+    using `-r/--recursive`.
+    """)
+  var followSymlinks: Bool = false
+
   /// The list of paths to Swift source files that should be formatted or linted.
   @Argument(help: "Zero or more input filenames.")
   var paths: [String] = []

Sources/swift-format/Utilities/FileIterator.swift

@@ -14,54 +14,82 @@ import Foundation
 
 /// Iterator for looping over lists of files and directories. Directories are automatically
 /// traversed recursively, and we check for files with a ".swift" extension.
-struct FileIterator: Sequence, IteratorProtocol {
+@_spi(Testing)
+public struct FileIterator: Sequence, IteratorProtocol {
 
   /// List of file and directory URLs to iterate over.
-  let urls: [URL]
+  private let urls: [URL]
+
+  /// If true, symlinks will be followed when iterating over directories and files. If not, they
+  /// will be ignored.
+  private let followSymlinks: Bool
 
   /// Iterator for the list of URLs.
-  var urlIterator: Array<URL>.Iterator
+  private var urlIterator: Array<URL>.Iterator
 
   /// Iterator for recursing through directories.
-  var dirIterator: FileManager.DirectoryEnumerator? = nil
+  private var dirIterator: FileManager.DirectoryEnumerator? = nil
 
   /// The current working directory of the process, which is used to relativize URLs of files found
   /// during iteration.
-  let workingDirectory = URL(fileURLWithPath: ".")
+  private let workingDirectory = URL(fileURLWithPath: ".")
 
   /// Keep track of the current directory we're recursing through.
-  var currentDirectory = URL(fileURLWithPath: "")
+  private var currentDirectory = URL(fileURLWithPath: "")
 
   /// Keep track of files we have visited to prevent duplicates.
-  var visited: Set<String> = []
+  private var visited: Set<String> = []
 
   /// The file extension to check for when recursing through directories.
-  let fileSuffix = ".swift"
+  private let fileSuffix = ".swift"
 
   /// 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.
-  init(urls: [URL]) {
+  public init(urls: [URL], followSymlinks: Bool) {
     self.urls = urls
     self.urlIterator = self.urls.makeIterator()
+    self.followSymlinks = followSymlinks
   }
 
   /// Iterate through the "paths" list, and emit the file paths in it. If we encounter a directory,
   /// recurse through it and emit .swift file paths.
-  mutating func next() -> URL? {
+  public mutating func next() -> URL? {
     var output: URL? = nil
     while output == nil {
       // Check if we're recursing through a directory.
       if dirIterator != nil {
         output = nextInDirectory()
       } else {
-        guard let next = urlIterator.next() else { return nil }
-        var isDir: ObjCBool = false
-        if FileManager.default.fileExists(atPath: next.path, isDirectory: &isDir), isDir.boolValue {
-          dirIterator = FileManager.default.enumerator(at: next, includingPropertiesForKeys: nil)
+        guard var next = urlIterator.next() else {
+          // If we've reached the end of all the URLs we wanted to iterate over, exit now.
+          return nil
+        }
+
+        guard let fileType = fileType(at: next) else {
+          continue
+        }
+
+        switch fileType {
+        case .typeSymbolicLink:
+          guard
+            followSymlinks,
+            let destination = try? FileManager.default.destinationOfSymbolicLink(atPath: next.path)
+          else {
+            break
+          }
+          next = URL(fileURLWithPath: destination)
+          fallthrough
+
+        case .typeDirectory:
+          dirIterator = FileManager.default.enumerator(
+            at: next,
+            includingPropertiesForKeys: nil,
+            options: [.skipsHiddenFiles])
           currentDirectory = next
-        } else {
+
+        default:
           // We'll get here if the path is a file, or if it doesn't exist. In the latter case,
           // return the path anyway; we'll turn the error we get when we try to open the file into
           // an appropriate diagnostic instead of trying to handle it here.
@@ -82,23 +110,41 @@ struct FileIterator: Sequence, IteratorProtocol {
   private mutating func nextInDirectory() -> URL? {
     var output: URL? = nil
     while output == nil {
-      if let item = dirIterator?.nextObject() as? URL {
-        if item.lastPathComponent.hasSuffix(fileSuffix) {
-          var isDir: ObjCBool = false
-          if FileManager.default.fileExists(atPath: item.path, isDirectory: &isDir)
-            && !isDir.boolValue
-          {
-            // We can't use the `.producesRelativePathURLs` enumeration option because it isn't
-            // supported yet on Linux, so we need to relativize the URL ourselves.
-            let relativePath =
-              item.path.hasPrefix(workingDirectory.path)
-              ? String(item.path.dropFirst(workingDirectory.path.count + 1))
-              : item.path
-            output =
-              URL(fileURLWithPath: relativePath, isDirectory: false, relativeTo: workingDirectory)
-          }
+      guard let item = dirIterator?.nextObject() as? URL else {
+        break
+      }
+
+      guard item.lastPathComponent.hasSuffix(fileSuffix), let fileType = fileType(at: item) else {
+        continue
+      }
+
+      var path = item.path
+      switch fileType {
+      case .typeSymbolicLink where followSymlinks:
+        guard
+          let destination = try? FileManager.default.destinationOfSymbolicLink(atPath: path)
+        else {
+          break
         }
-      } else { break }
+        path = destination
+        fallthrough
+
+      case .typeRegular:
+        // We attempt to relativize the URLs based on the current working directory, not the
+        // directory being iterated over, so that they can be displayed better in diagnostics. Thus,
+        // if the user passes paths that are relative to the current working diectory, 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)
+
+      default:
+        break
+      }
     }
     // If we've exhausted the files in the directory recursion, unset the directory iterator.
     if output == nil {
@@ -107,3 +153,10 @@ struct FileIterator: Sequence, IteratorProtocol {
     return output
   }
 }
+
+/// Returns the type of the file at the given URL.
+private func fileType(at url: URL) -> FileAttributeType? {
+  // We cannot use `URL.resourceValues(forKeys:)` here because it appears to behave incorrectly on
+  // Linux.
+  return try? FileManager.default.attributesOfItem(atPath: url.path)[.type] as? FileAttributeType
+}

Tests/swift-formatTests/Utilities/FileIteratorTests.swift

@@ -0,0 +1,91 @@
+import XCTest
+
+@_spi(Testing) import swift_format
+
+final class FileIteratorTests: XCTestCase {
+  private var tmpdir: URL!
+
+  override func setUpWithError() throws {
+    tmpdir = try FileManager.default.url(
+      for: .itemReplacementDirectory,
+      in: .userDomainMask,
+      appropriateFor: FileManager.default.temporaryDirectory,
+      create: true)
+
+    // Create a simple file tree used by the tests below.
+    try touch("project/real1.swift")
+    try touch("project/real2.swift")
+    try touch("project/.hidden.swift")
+    try touch("project/.build/generated.swift")
+    try symlink("project/link.swift", to: "project/.hidden.swift")
+  }
+
+  override func tearDownWithError() throws {
+    try FileManager.default.removeItem(at: tmpdir)
+  }
+
+  func testNoFollowSymlinks() {
+    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") })
+  }
+
+  func testFollowSymlinks() {
+    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") })
+    // Hidden but found through the visible symlink project/link.swift
+    XCTAssertTrue(seen.contains { $0.hasSuffix("project/.hidden.swift") })
+  }
+
+  func testTraversesHiddenFilesIfExplicitlySpecified() {
+    let seen = allFilesSeen(
+      iteratingOver: [tmpURL("project/.build"), tmpURL("project/.hidden.swift")],
+      followSymlinks: false)
+    XCTAssertEqual(seen.count, 2)
+    XCTAssertTrue(seen.contains { $0.hasSuffix("project/.build/generated.swift") })
+    XCTAssertTrue(seen.contains { $0.hasSuffix("project/.hidden.swift") })
+  }
+
+  func testDoesNotFollowSymlinksIfFollowSymlinksIsFalseEvenIfExplicitlySpecified() {
+    // Symlinks are not traversed even if `followSymlinks` is false even if they are explicitly
+    // passed to the iterator. This is meant to avoid situations where a symlink could be hidden by
+    // shell expansion; for example, if the user writes `swift-format --no-follow-symlinks *`, if
+    // the current directory contains a symlink, they would probably *not* expect it to be followed.
+    let seen = allFilesSeen(iteratingOver: [tmpURL("project/link.swift")], followSymlinks: false)
+    XCTAssertTrue(seen.isEmpty)
+  }
+}
+
+extension FileIteratorTests {
+  /// Returns a URL to a file or directory in the test's temporary space.
+  private func tmpURL(_ path: String) -> URL {
+    return tmpdir.appendingPathComponent(path, isDirectory: false)
+  }
+
+  /// Create an empty file at the given path in the test's temporary space.
+  private func touch(_ path: String) throws {
+    let fileURL = tmpURL(path)
+    try FileManager.default.createDirectory(
+      at: fileURL.deletingLastPathComponent(), withIntermediateDirectories: true)
+    FileManager.default.createFile(atPath: fileURL.path, contents: Data())
+  }
+
+  /// Create a symlink between files or directories in the test's temporary space.
+  private func symlink(_ source: String, to target: String) throws {
+    try FileManager.default.createSymbolicLink(
+      at: tmpURL(source), withDestinationURL: tmpURL(target))
+  }
+
+  /// 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] = []
+    for next in iterator {
+      seen.append(next.path)
+    }
+    return seen
+  }
+}