From c9a84d621360f14480cd869c4dd9af392a81040e Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Tue, 29 Aug 2023 12:21:19 -0400 Subject: [PATCH] Ignore symlinks and hidden (dot) files during `--recursive`. Note that hidden files will *always* be honored if they are specified explicitly among the paths on the command line. Symlinks encountered during recursive traversal *or* passed on the command line will only be followed if the flag `--follow-symlinks` is passed. This is meant to avoid symlinks being hidden by shell expansions; for example, running `swift-format *` where the current directory contains a symlink and following that symlink might be surprising to users. This PR also adds a new test target for code in the `swift-format` binary in order to test `FileIterator` without factoring the binary's code out to a separate library target. We should use this in the future to expand coverage of other functionality like the frontends and commands. --- Package.swift | 4 + Sources/swift-format/Frontend/Frontend.swift | 9 +- .../Subcommands/LintFormatOptions.swift | 7 ++ .../swift-format/Utilities/FileIterator.swift | 115 +++++++++++++----- .../Utilities/FileIteratorTests.swift | 91 ++++++++++++++ 5 files changed, 193 insertions(+), 33 deletions(-) create mode 100644 Tests/swift-formatTests/Utilities/FileIteratorTests.swift diff --git a/Package.swift b/Package.swift index b7ec1973e..9b74aa635 100644 --- a/Package.swift +++ b/Package.swift @@ -130,6 +130,10 @@ let package = Package( .product(name: "SwiftSyntaxBuilder", package: "swift-syntax"), ] ), + .testTarget( + name: "swift-formatTests", + dependencies: ["swift-format"] + ), ] ) diff --git a/Sources/swift-format/Frontend/Frontend.swift b/Sources/swift-format/Frontend/Frontend.swift index 86930ee0b..2d5274160 100644 --- a/Sources/swift-format/Frontend/Frontend.swift +++ b/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) } } diff --git a/Sources/swift-format/Subcommands/LintFormatOptions.swift b/Sources/swift-format/Subcommands/LintFormatOptions.swift index 170ebaad2..4e98d1e14 100644 --- a/Sources/swift-format/Subcommands/LintFormatOptions.swift +++ b/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] = [] diff --git a/Sources/swift-format/Utilities/FileIterator.swift b/Sources/swift-format/Utilities/FileIterator.swift index 20b513a01..6f88cc909 100644 --- a/Sources/swift-format/Utilities/FileIterator.swift +++ b/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.Iterator + private var urlIterator: Array.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 = [] + private var visited: Set = [] /// 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 +} diff --git a/Tests/swift-formatTests/Utilities/FileIteratorTests.swift b/Tests/swift-formatTests/Utilities/FileIteratorTests.swift new file mode 100644 index 000000000..5bfe5bb18 --- /dev/null +++ b/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 + } +}