Skip to content

Clean up symlink following and recursively follow symlink chains. #947

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 26, 2025
Merged

Clean up symlink following and recursively follow symlink chains. #947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 45 additions & 27 deletions Sources/SwiftFormat/Utilities/FileIterator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,25 +71,18 @@ public struct FileIterator: Sequence, IteratorProtocol {
if dirIterator != nil {
output = nextInDirectory()
} else {
guard var next = urlIterator.next() else {
guard let 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 {
guard let (next, fileType) = fileAndType(at: next, followSymlinks: followSymlinks) else {
continue
}

switch fileType {
case .typeSymbolicLink:
guard
followSymlinks,
let destination = try? FileManager.default.destinationOfSymbolicLink(atPath: next.path)
else {
break
}
next = URL(fileURLWithPath: destination, relativeTo: next)
fallthrough
// If we got here, we encountered a symlink but didn't follow it. Skip it.
continue

case .typeDirectory:
dirIterator = FileManager.default.enumerator(
Expand Down Expand Up @@ -132,27 +125,20 @@ public struct FileIterator: Sequence, IteratorProtocol {
}
#endif

guard item.lastPathComponent.hasSuffix(fileSuffix), let fileType = fileType(at: item) else {
guard item.lastPathComponent.hasSuffix(fileSuffix),
let (item, fileType) = fileAndType(at: item, followSymlinks: followSymlinks)
else {
continue
}

var path = item.path
switch fileType {
case .typeSymbolicLink where followSymlinks:
guard
let destination = try? FileManager.default.destinationOfSymbolicLink(atPath: path)
else {
break
}
path = URL(fileURLWithPath: destination, relativeTo: item).path
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 directory, they will
// be displayed as relative paths. Otherwise, they will still be displayed as absolute
// paths.
let path = item.path
let relativePath: String
if !workingDirectory.isRoot, path.hasPrefix(workingDirectory.path) {
relativePath = String(path.dropFirst(workingDirectory.path.count).drop(while: { $0 == "/" || $0 == #"\"# }))
Expand All @@ -173,9 +159,41 @@ public struct FileIterator: Sequence, IteratorProtocol {
}
}

/// 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
/// Returns the actual URL and type of the file at the given URL, following symlinks if requested.
///
/// - Parameters:
/// - url: The URL to get the file and type of.
/// - followSymlinks: Whether to follow symlinks.
/// - Returns: The actual URL and type of the file at the given URL, or `nil` if the file does not
/// exist or is not a supported file type. If `followSymlinks` is `true`, the returned URL may be
/// different from the given URL; otherwise, it will be the same.
private func fileAndType(at url: URL, followSymlinks: Bool) -> (URL, FileAttributeType)? {
func typeOfFile(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
}

guard var fileType = typeOfFile(at: url) else {
return nil
}

// We would use `standardizedFileURL.path` here as we do in the iterator above to ensure that
// path components like `.` and `..` are resolved, but the standardized URLs returned by
// Foundation pre-Swift-6.0 resolve symlinks. This causes the file type of a URL and its
// standardized path to not match.
var visited: Set<String> = [url.absoluteString]
var url = url
while followSymlinks && fileType == .typeSymbolicLink,
let destination = try? FileManager.default.destinationOfSymbolicLink(atPath: url.path)
Comment on lines +187 to +188
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we detect circles here if you have a symlink that points to itself?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch; fixed. Symlink cycles (either a link pointing to itself, or a multi-link cycle) are silently ignored now. We could warn on this, but we'd have to plumb DiagnosticsEngine down into FileIterator, which I didn't really want to do here if we can avoid it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we don’t infinite loop, I think the behavior is OK.

Also, it only just occurs to me, why can’t we use resolvingSymlinksInPath/realpath here if we follow all symlinks?

In SourceKit-LSP, we have the following code to avoid the /tmp vs /private/tmp issue: https://github.com/swiftlang/sourcekit-lsp/blob/3e7f350246dc86072a9dfc88f25782dc86dd1c9a/Sources/SwiftExtensions/URLExtensions.swift#L40-L57

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good question. That realpath code seems to work on macOS from local testing, so let me see if it works on Linux/Windows. Assuming they both consistently resolve all symlinks deeply or stop when a cycle is reached on all platforms, then that would remove the need for our own loop.

Copy link
Member Author

@allevato allevato Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh, no good. Based on CI results and local testing, it looks like resolvingSymlinksInPath on Linux pre-5.x Foundation doesn't correctly resolve through multiple symlinks. So we'd need a special case to handle those, which would essentially be the code I had earlier anyway.

Maybe one day when we drop 5.x support, we can clean this all up.

{
url = URL(fileURLWithPath: destination, relativeTo: url)
// If this URL is in the visited set, we must have a symlink cycle. Ignore it gracefully.
guard !visited.contains(url.absoluteString), let newType = typeOfFile(at: url) else {
return nil
}
visited.insert(url.absoluteString)
fileType = newType
}
return (url, fileType)
}
40 changes: 40 additions & 0 deletions Tests/SwiftFormatTests/Utilities/FileIteratorTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ final class FileIteratorTests: XCTestCase {
try touch("project/.build/generated.swift")
try symlink("project/link.swift", to: "project/.hidden.swift")
try symlink("project/rellink.swift", relativeTo: ".hidden.swift")

#if !(os(Windows) && compiler(<5.10))
// Test both a self-cycle and a cycle between multiple symlinks.
try symlink("project/cycliclink.swift", relativeTo: "cycliclink.swift")
try symlink("project/linktolink.swift", relativeTo: "link.swift")

// Test symlinks that use nonstandardized paths.
try symlink("project/2stepcyclebegin.swift", relativeTo: "../project/2stepcycleend.swift")
try symlink("project/2stepcycleend.swift", relativeTo: "./2stepcyclebegin.swift")
#endif
}

override func tearDownWithError() throws {
Expand Down Expand Up @@ -72,6 +82,36 @@ final class FileIteratorTests: XCTestCase {
XCTAssertTrue(seen.contains { $0.path.hasSuffix("project/.hidden.swift") })
}

func testFollowSymlinksToSymlinks() throws {
#if os(Windows) && compiler(<5.10)
try XCTSkipIf(true, "Foundation does not follow symlinks on Windows")
#endif
let seen = allFilesSeen(
iteratingOver: [tmpURL("project/linktolink.swift")],
followSymlinks: true
)
// Hidden but found through the visible symlink chain.
XCTAssertTrue(seen.contains { $0.path.hasSuffix("project/.hidden.swift") })
}

func testSymlinkCyclesAreIgnored() throws {
#if os(Windows) && compiler(<5.10)
try XCTSkipIf(true, "Foundation does not follow symlinks on Windows")
#endif
let seen = allFilesSeen(
iteratingOver: [
tmpURL("project/cycliclink.swift"),
tmpURL("project/2stepcyclebegin.swift"),
tmpURL("project/link.swift"),
],
followSymlinks: true
)
// Hidden but found through the visible symlink chain.
XCTAssertTrue(seen.contains { $0.path.hasSuffix("project/.hidden.swift") })
// And the cycles were ignored.
XCTAssertEqual(seen.count, 1)
}

func testTraversesHiddenFilesIfExplicitlySpecified() throws {
#if os(Windows) && compiler(<5.10)
try XCTSkipIf(true, "Foundation does not follow symlinks on Windows")
Expand Down