Skip to content

[Windows] Standardize slashes before path processing #731

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 3 commits into from
Jul 15, 2024
Merged

[Windows] Standardize slashes before path processing #731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d72791a
Standardize backslashes before string path processing on Windows
jmschonfeld Jul 12, 2024
a04873a
Call _standardizingSlashes() from normalizedPath(with:)
jmschonfeld Jul 15, 2024
75e39c5
Return self when possible on non-Windows
jmschonfeld Jul 15, 2024
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
73 changes: 58 additions & 15 deletions Sources/FoundationEssentials/String/String+Path.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,28 @@ import WinSDK

internal import _FoundationCShims

extension StringProtocol {
fileprivate func _standardizingSlashes() -> String {
#if os(Windows)
// The string functions below all assume that the path separator is a forward slash
// Standardize the path to use forward slashes before processing for consistency
return self.replacing(._backslash, with: ._slash)
#else
if let str = _specializingCast(self, to: String.self) {
return str
} else {
return String(self)
}
#endif
}
}

extension String {
internal func deletingLastPathComponent() -> String {
_standardizingSlashes()._deletingLastPathComponent()
}

private func _deletingLastPathComponent() -> String {
let lastSlash = self.lastIndex { $0 == "/" }
guard let lastSlash else {
// No slash
Expand All @@ -50,6 +70,10 @@ extension String {
}

internal func appendingPathComponent(_ component: String) -> String {
_standardizingSlashes()._appendingPathComponent(component)
}

private func _appendingPathComponent(_ component: String) -> String {
var result = self
if !component.isEmpty {
var needsSlash = true
Expand Down Expand Up @@ -103,6 +127,10 @@ extension String {
}

internal var lastPathComponent: String {
_standardizingSlashes()._lastPathComponent
}

private var _lastPathComponent: String {
let lastSlash = self.lastIndex { $0 == "/" }
guard let lastSlash else {
// No slash, just return self
Expand Down Expand Up @@ -170,11 +198,11 @@ extension String {
return false
}
if let lastDot = pathExtension.utf8.lastIndex(of: UInt8(ascii: ".")) {
let beforeDot = pathExtension[..<lastDot].unicodeScalars
let afterDot = pathExtension[pathExtension.index(after: lastDot)...].unicodeScalars
let beforeDot = pathExtension[..<lastDot]._standardizingSlashes().unicodeScalars
let afterDot = pathExtension[pathExtension.index(after: lastDot)...]._standardizingSlashes().unicodeScalars
Copy link
Member

Choose a reason for hiding this comment

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

Where do we expect an arc separator after the extension separator? That is, the extension should be on the final arc, and so there would be no slashes right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't expect it to occur, but this is to ensure that we validate that it doesn't. I can't come up with any scenarios where we would currently end up in this situation, but invalidExtensionScalars contains a / and so to ensure that POSIX and Windows behaviors are consistent, I added this standardization here so that if we do somehow end up with a \ in the path extension that we mark it as invalid on Windows

return beforeDot.allSatisfy { $0 != "/" } && afterDot.allSatisfy { !String.invalidExtensionScalars.contains($0) }
} else {
return pathExtension.unicodeScalars.allSatisfy { !String.invalidExtensionScalars.contains($0) }
return pathExtension._standardizingSlashes().unicodeScalars.allSatisfy { !String.invalidExtensionScalars.contains($0) }
}
}

Expand Down Expand Up @@ -202,6 +230,10 @@ extension String {
}

internal func merging(relativePath: String) -> String {
_standardizingSlashes()._merging(relativePath: relativePath)
}

private func _merging(relativePath: String) -> String {
guard relativePath.utf8.first != UInt8(ascii: "/") else {
return relativePath
}
Expand All @@ -212,6 +244,10 @@ extension String {
}

internal var removingDotSegments: String {
_standardizingSlashes()._removingDotSegments
}

private var _removingDotSegments: String {
let input = self.utf8
guard !input.isEmpty else {
return ""
Expand Down Expand Up @@ -440,18 +476,12 @@ extension String {

// From swift-corelibs-foundation's NSTemporaryDirectory. Internal for now, pending a better public API.
internal static var temporaryDirectoryPath: String {
#if os(Windows)
let validPathSeps: [Character] = ["\\", "/"]
#else
let validPathSeps: [Character] = ["/"]
#endif

func normalizedPath(with path: String) -> String {
if validPathSeps.contains(where: { path.hasSuffix(String($0)) }) {
return path
} else {
return path + String(validPathSeps.last!)
var result = path._standardizingSlashes()
guard result.utf8.last != ._slash else {
return result
}
return result + "/"
}
#if os(Windows)
let cchLength: DWORD = GetTempPathW(0, nil)
Expand Down Expand Up @@ -547,7 +577,7 @@ extension String {
static var NETWORK_PREFIX: String { #"\\"# }

private var _standardizingPath: String {
var result = _transmutingCompressingSlashes()._droppingTrailingSlashes
var result = _standardizingSlashes()._transmutingCompressingSlashes()._droppingTrailingSlashes
let postNetStart = if result.starts(with: String.NETWORK_PREFIX) {
result.firstIndex(of: "/") ?? result.endIndex
} else {
Expand All @@ -558,7 +588,7 @@ extension String {
result = resolved
}

result = result.removingDotSegments
result = result._removingDotSegments

// Automounted paths need to be stripped for various flavors of paths
let exclusionList = ["/Applications", "/Library", "/System", "/Users", "/Volumes", "/bin", "/cores", "/dev", "/opt", "/private", "/sbin", "/usr"]
Expand All @@ -584,6 +614,10 @@ extension String {

// _NSPathComponents
var pathComponents: [String] {
_standardizingSlashes()._pathComponents
}

private var _pathComponents: [String] {
var components = self.components(separatedBy: "/").filter { !$0.isEmpty }
if self.first == "/" {
components.insert("/", at: 0)
Expand All @@ -596,6 +630,10 @@ extension String {

#if !NO_FILESYSTEM
var abbreviatingWithTildeInPath: String {
_standardizingSlashes()._abbreviatingWithTildeInPath
}

private var _abbreviatingWithTildeInPath: String {
guard !self.isEmpty && self != "/" else { return self }
let homeDir = String.homeDirectoryPath()
guard self.starts(with: homeDir) else { return self }
Expand All @@ -605,6 +643,10 @@ extension String {
}

var expandingTildeInPath: String {
_standardizingSlashes()._expandingTildeInPath
}

private var _expandingTildeInPath: String {
guard self.first == "~" else { return self }
var user: String? = nil
let firstSlash = self.firstIndex(of: "/") ?? self.endIndex
Expand Down Expand Up @@ -781,6 +823,7 @@ extension StringProtocol {
}
}

// Internal for testing purposes
internal func _hasDotDotComponent() -> Bool {
let input = self.utf8
guard input.count >= 2 else {
Expand Down
8 changes: 7 additions & 1 deletion Tests/FoundationEssentialsTests/FileManager/FileManagerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,16 @@ final class FileManagerTests : XCTestCase {
try $0.createDirectory(atPath: "create_dir_test2/nested2", withIntermediateDirectories: true)
XCTAssertEqual(try $0.contentsOfDirectory(atPath: "create_dir_test2").sorted(), ["nested", "nested2"])
XCTAssertNoThrow(try $0.createDirectory(atPath: "create_dir_test2/nested2", withIntermediateDirectories: true))

#if os(Windows)
try $0.createDirectory(atPath: "create_dir_test3\\nested", withIntermediateDirectories: true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an example of a previous API that used to fail without this change. Previously, this API called deletingLastPathComponent on the provided path, which would return an empty string and therefore fail to create the parent create_dir_test3 directory. Now it correctly creates create_dir_test3 followed by create_dir_test3/nested

XCTAssertEqual(try $0.contentsOfDirectory(atPath: "create_dir_test3"), ["nested"])
#endif

XCTAssertThrowsError(try $0.createDirectory(atPath: "create_dir_test", withIntermediateDirectories: false)) {
XCTAssertEqual(($0 as? CocoaError)?.code, .fileWriteFileExists)
}
XCTAssertThrowsError(try $0.createDirectory(atPath: "create_dir_test3/nested", withIntermediateDirectories: false)) {
XCTAssertThrowsError(try $0.createDirectory(atPath: "create_dir_test4/nested", withIntermediateDirectories: false)) {
XCTAssertEqual(($0 as? CocoaError)?.code, .fileNoSuchFile)
}
XCTAssertThrowsError(try $0.createDirectory(atPath: "preexisting_file", withIntermediateDirectories: false)) {
Expand Down