Guarantee config-file search terminates on paths with redundant separators#1213
Guarantee config-file search terminates on paths with redundant separators#1213adityasingh2400 wants to merge 2 commits into
Conversation
The parent-directory walk in Configuration.url(forConfigurationFileApplyingTo:) relied solely on reaching the filesystem root to terminate. On toolchains where URL.deleteLastPathComponent() stops making progress for paths containing redundant separators (e.g. /path/to///main.swift), the loop could spin forever. Add a guard that breaks the loop when removing the last path component yields no change, so termination no longer depends on Foundation collapsing redundant separators. Fixes swiftlang#1035
| // contains redundant separators (e.g. `/path/to///main.swift`), which `standardized` does not | ||
| // collapse; without this guard the loop would never reach the root and would spin forever. | ||
| // See https://github.com/swiftlang/swift-format/issues/1035. | ||
| if candidateDirectory == previousDirectory { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
deleteLastPathComponent() collapses redundant separators as it ascends on the current toolchain, so the loop already reaches root; the guard backstops the Foundation that exhibited swiftlang#1035, where a trailing redundant separator stalled the ascent. Note that breaking on a non-progressing path can never skip a searchable parent, since the operation is a pure function of the path.
Problem
Configuration.url(forConfigurationFileApplyingTo:)walks up the parent directories of a source file looking for a.swift-formatfile, loopingwhile !candidateDirectory.isRoot. When the source-file path contains a run of redundant separators (e.g./path/to///main.swift),URL's standardization does not collapse them, so repeatedly callingdeleteLastPathComponent()can stop making progress before reaching the root — and the loop spins forever. This is the hang reported in #1035.Change
Track the previous candidate directory each iteration and
breakifdeleteLastPathComponent()produced no change. This guarantees termination regardless of redundant separators while preserving the existing search behavior (it still finds a configuration file and still returnsnilwhen none exists).Verification
swift buildsucceeds andswift test --filter ConfigurationTestspasses, including two added regression tests:testMissingConfigurationFilePathWithRedundantSlashesTerminates— confirms a///-containing path with no config terminates and returnsnil(previously hung).testFindsConfigurationFileWhenPathContainsRedundantSlashes— confirms a config file is still located when the path contains redundant separators.Fixes #1035.