Skip to content

Guarantee config-file search terminates on paths with redundant separators #1213

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

Open
adityasingh2400 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Guarantee config-file search terminates on paths with redundant separators #1213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a2d5ee
Guarantee config-file search loop terminates on non-progressing paths
adityasingh2400 May 29, 2026
5916203
Clarify the termination guard is defensive on current Foundation
adityasingh2400 Jun 2, 2026
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
13 changes: 13 additions & 0 deletions Sources/SwiftFormat/API/Configuration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,11 +515,24 @@ public struct Configuration: Codable, Equatable {
candidateDirectory.appendPathComponent("placeholder")
}
repeat {
let previousDirectory = candidateDirectory
candidateDirectory.deleteLastPathComponent()
let candidateFile = candidateDirectory.appendingPathComponent(Self.swiftFormatFilename)
if FileManager.default.isReadableFile(atPath: candidateFile.path) {
return candidateFile
}

// Stop if removing the last path component made no progress. `deleteLastPathComponent()` is a
// pure function of the path, so once it is a no-op it stays a no-op: there is no reachable
// parent above this point, and continuing would only spin until `isRoot` (which it never
// reaches). On the Foundation that originally exhibited #1035, a trailing redundant separator
// (e.g. `/path/to//main.swift`) stalled here; current Foundation collapses such separators as
// it ascends, so this guard is a defensive backstop rather than the load-bearing fix on that
// toolchain. Breaking here can never skip a directory the loop could otherwise have searched.
// See https://github.com/swiftlang/swift-format/issues/1035.
if candidateDirectory == previousDirectory {

@allevato allevato May 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we stop the first time we try to delete the last path component and it's a no-op, we would never find the configuration file in a directory above that; we would stop the loop too early.

If I do the following in Swift 6.3.2 on macOS Tahoe, I see the following behavior, which seems to imply that deleteLastPathComponent() does the right thing:

  7> var u = URL(filePath: "a/b///c")
u: Foundation.URL = "a/b///c"
  8> u.deleteLastPathComponent()
  9> u
$R3: Foundation.URL = "a/b/"
 10> u.deleteLastPathComponent()
 11> u
$R4: Foundation.URL = "a/"

I'm not sure this change is correct or doing what it's intended to do, and the test is passing because the behavior is already correct (or at least the test isn't exhibiting the bug in the original issue) and thus we never hit this case.

@adityasingh2400 adityasingh2400 Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right, and I verified it on the same toolchain (Swift 6.3.2, macOS Tahoe). Tracing the loop on the standardized input path:

start: /test/path/no/configuration///exists/anywhere/main.swift   (standardized keeps the ///)
 0 -> /test/path/no/configuration///exists/anywhere
 1 -> /test/path/no/configuration///exists
 2 -> /test/path/no/configuration          <- deleteLastPathComponent() collapses the ///
 ...
 6 -> /                                     <- reaches root cleanly

So on current Foundation deleteLastPathComponent() makes progress every step and the loop terminates without the guard. The guard is never reached here, and the terminate test passes because the behavior is already correct, exactly as you said. The original #1035 repro was on an older Foundation where a trailing redundant separator made deleteLastPathComponent() a no-op (it stalled at DemoSwift// and never reached root).

On your other concern, stopping too early: I do not think the guard can do that. deleteLastPathComponent() is a pure function of the path, so if it is a no-op once it is a no-op on every later iteration too. The break therefore only fires in the exact state that would otherwise loop forever, and there is never a reachable parent above that point that it skips. I pushed a commit that rewrites the comment to say this honestly: the guard is a defensive backstop, not the load-bearing fix on current Foundation.

Given that, your call on direction. Two options I am happy with: (1) close this as effectively fixed upstream in Foundation, since the live toolchains terminate, or (2) keep it as the two regression tests that lock #1035 stays fixed plus the cheap guard for any platform or Foundation version where the ascent still stalls. I lean slightly toward (2) for the regression coverage, but I will defer to your preference and can strip it to whichever shape you want.

break
}
} while !candidateDirectory.isRoot

return nil
Expand Down
39 changes: 39 additions & 0 deletions Tests/SwiftFormatTests/API/ConfigurationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//
import Foundation
import SwiftFormat
import XCTest

Expand Down Expand Up @@ -135,4 +136,42 @@ final class ConfigurationTests: XCTestCase {
XCTAssertEqual(config, expected)
#endif
}

func testMissingConfigurationFilePathWithRedundantSlashesTerminates() throws {
// A path that contains a run of redundant separators (here `///`) is not collapsed by `URL`'s
// standardization, so walking up its parent directories must not loop forever when no
// configuration file exists. See https://github.com/swiftlang/swift-format/issues/1035.
#if os(Windows)
let path = #"C:\test\path\no\configuration\\\exists\anywhere\main.swift"#
#else
let path = "/test/path/no/configuration///exists/anywhere/main.swift"
#endif
XCTAssertNil(Configuration.url(forConfigurationFileApplyingTo: URL(fileURLWithPath: path)))
}

func testFindsConfigurationFileWhenPathContainsRedundantSlashes() throws {
// The parent-directory walk must terminate *and* still locate the configuration file when the
// source file path contains redundant separators (e.g. `///`), which can happen when paths are
// composed by other tools. See https://github.com/swiftlang/swift-format/issues/1035.
#if os(Windows)
try XCTSkipIf(true, "Redundant POSIX-style `///` separators are not meaningful on Windows.")
#else
let projectDir = FileManager.default.temporaryDirectory
.appendingPathComponent("swift-format-1035-\(UUID().uuidString)", isDirectory: true)
let sourceDir = projectDir.appendingPathComponent("Sources").appendingPathComponent("MyModule")
try FileManager.default.createDirectory(at: sourceDir, withIntermediateDirectories: true)
defer { try? FileManager.default.removeItem(at: projectDir) }

let configURL = projectDir.appendingPathComponent(".swift-format")
try Data("{}".utf8).write(to: configURL)

// Build a source file path that contains redundant `///` separators below `projectDir`.
let redundantSlashPath = projectDir.path + "/Sources/MyModule///main.swift"
let sourceFile = URL(fileURLWithPath: redundantSlashPath)
XCTAssertTrue(sourceFile.path.contains("///"), "Test setup must preserve the redundant slashes.")

let foundConfigURL = Configuration.url(forConfigurationFileApplyingTo: sourceFile)
XCTAssertEqual(foundConfigURL?.standardizedFileURL, configURL.standardizedFileURL)
#endif
}
}