apple / swift-system

Low-level system calls and types for Swift
Apache License 2.0
1.18k stars 102 forks source link

Surprising behavior on Windows with drive roots in the middle of a path #188

Open jakepetroules opened 3 months ago

jakepetroules commented 3 months ago

Consider the following (tested with v1.3.1 tag):

let fp = FilePath("Sources/E:\\source")
print(fp.isAbsolute) // false
print(fp.isRelative) // true
print(fp.root) // nil
print(Array(fp.components)) // ["Sources", "E:", "source"]

print(FilePath("Sources").appending(fp.components)) // SystemPackage/FilePathParsing.swift:351: Assertion failed
print(FilePath("Sources").pushing(fp)) // Sources\Sources\E:\source

print(FilePath("Sources").appending(FilePath.Component("C:"))) // SystemPackage/FilePathString.swift:299: Fatal error: FilePath.Component must be created from exactly one non-root component
print(FilePath("Sources").appending("C:")) // Sources

It's not clear to me that some of these assertions are expected, or even how a seemingly bogus path like Sources/E:\\source should be treated by the APIs. Should the FilePath constructor have asserted right away on this path if containing prohibited characters per https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions? Should there be an alternative validating constructor that returns nil or throws in this case?

lorentey commented 3 months ago

Trapping in FilePath.appending does not seem right to me. Trapping in an assertion (as opposed to precondition) failure is definitely incorrect -- this operation either needs to succeed, or it must produce a guaranteed runtime error.

If Source/E:\\sources is not a valid path on Windows, then I think FilePath("Sources/E:\\source") should not successfully create a file path. FilePath.init(_: String) is a non-failable initializer, so the only way to report an error is to trap. If that isn't acceptable, then we have to make a choice between deprecating it, or coming up (and clearly documenting!) a mechanism to turn it into a valid path.

For what it's worth, Foo\0Bar is clearly an invalid path on UNIX-compatible systems, but FilePath("Foo\0Bar") appears to accept it, silently truncating the input string before the NUL character. If we take this (terrible) precedent as the intended behavior, then one potential solution on Windows would be to filter out inappropriate characters or to replace them with some bogus substitute.

The state of FilePath makes me deeply unhappy.