swiftlang / swift-foundation

The Foundation project
Apache License 2.0
2.41k stars 162 forks source link

URL.path(percentEncoded: false) is NOT Equivalent to URL.path property for File URLs #1011

Open swillits opened 1 month ago

swillits commented 1 month ago

Description

Xcode suggests that URL.path(percentEncoded: false) should be used to replace the will-be-deprecated URL.path, but for relative file URLs, this will lead to bugs in existing code. (Ask me how I know; It already did in mine.)

The documentation for .path notes: "This function resolves against the base URL." It also says: "New code should use .path(percentEncoded: false)"

The documentation for path(percentEncoded:) does not say anything about resolving against the base URL. Does it? Does it not? — It does not, which is the problem.

I raised this issue in the forums with no response: https://forums.swift.org/t/url-path-vs-path-percentencoded-for-file-urls/75358

Reproduction

The problem is something like this:

Bundle.main.url(forAuxiliaryExecutable: "tool")!.path

... which returns the expected file path (ex: "/Applications/MyApp.app/Contents/tool")

would be quickly rewritten as suggested to:

Bundle.main.url(forAuxiliaryExecutable: "tool")!.path(percentEncoded: false)

... which simply returns "tool", because the URL returned is relative to a base URL.

Widespread adoption of path(percentEncoded: false) in place of path on file URLs will lead to bugs if this undocumented difference is not known.

let a = URL(fileURLWithPath: "/System/Library")
let b = URL(string: "Library", relativeTo: URL(fileURLWithPath: "/System"))!

print(a) // file:///System/Library/
print(b) // Library -- file:///System/

// WARNING: 'path' will be deprecated in a future version of macOS: Use path(percentEncoded:) instead
print(a.path) //  /System/Library
print(b.path) //  /System/Library

print(a.path(percentEncoded: false)) //  /System/Library 
print(b.path(percentEncoded: false)) //  Library

print(a.standardizedFileURL.path(percentEncoded: false)) //  /System/Library 
print(b.standardizedFileURL.path(percentEncoded: false)) //  /System/Library

Expected behavior

Based on Xcode's deprecation warning, I expect .path(percentEncoded: false) to behave the same as .path currently does.

The fact is many users will just change .path into .path(percentEncoded: false) because that's what Xcode and the documentation tell them to do, and they will probably introduce bugs.

Environment

Apple Swift version 6.0 (swiftlang-6.0.0.9.10 clang-1600.0.26.2) Target: arm64-apple-macosx15.0

Additional information

I believe the best solution would be the addition of a new URL property specifically for getting the file path from file URLs.

I've always found it not-completely-obvious to newcomers how to get a standard file path string from a file URL and this only makes it more confusing. (.path, .relativePath, .path(), .path(percentEncoded: false) ..... there are too many choices, and the behavior difference between them is not clear unless you really dig and familiarlize yourself with URLs.

For the sake of sanity where having to go from a URL to a file path string is still very common, I'd expect something along the lines of:

url.standardizedFilePath -> String

... which makes it very clear and obvious to anyone looking at URL and asking "how do I get the [full and complete, resolved, standardized] file path string from this URL?"

This is a common task, and yet it's not obvious. The alternative is that any time code needs to go from a URL to a file path, it needs to call url.standardizedFileURL.path(percentEncoded: false) to get the correct result, which is just not convenient or obvious.

Or, since the FilePath type now exists, I'd even accept:

url.standardizedFilePath -> FilePath
jmschonfeld commented 1 month ago

@swillits I transferred this over to the swift-foundation repo where URL is implemented. @jrflat might be able to shed some light here

jrflat commented 1 month ago

Hi @swillits, I very much empathize with all the concerns you've listed here, and they're issues we're looking to fix. In fact, I'm in the process of writing a proposal to expose URL's internal var fileSystemPath as API. This property would be the preferred replacement for .path and would take into account the user's OS (for example, by stripping the leading slash and converting to backslashes on Windows). Perhaps .standardizedFilePath better describes the path being complete and standardized, but the fact that we've both arrived at a similar solution makes me confident in this approach overall.

.path() returning a relative path has also been a longstanding issue, since as you noticed, this leaves no (non-deprecated) way to get an absolute path for a URL, which can definitely lead to pitfalls. That is something we've changed with the Swift URL rewrite in swift-foundation. Now, .path() will return the absolute path.

In the future, I could imagine a set of URL path APIs to make this much clearer:

var path: String // Actually deprecated, no longer needed
var fileSystemPath: String // Returns the correct absolute path string for file system operations on the current OS
func relativePath(percentEncoded:) -> String // Replaces .relativePath, gives the option to receive percent-encoded or decoded, does not resolve against a base URL
func path(percentEncoded:) -> String // Now resolves against a base URL to get the absolute path

Of course, this is pending API review process and feedback from the community, so please let me know if you have suggestions. Along with good updated documentation, I think this would alleviate most of the issues you mentioned.

Thank you for bringing this to our attention! And I'd be very happy to hear any feedback :)

ole commented 1 month ago

That is something we've changed with the Swift URL rewrite in swift-foundation. Now, .path() will return the absolute path.

@jrflat What version are you talking about here? As @swillits says, .path() still returns the relative path in the current Apple releases, doesn't it? I tested it on macOS 15.0.1 with Xcode 16.0 in a playground.

swillits commented 1 month ago

@jrflat Those changes all sound good to me. fileSystemPath is plenty clear, and the changes to path()'s resolution makes it logical and consistent with respect to relativePath().

jrflat commented 1 month ago

@ole The URL.path() implementation in the swift-foundation repo returns the absolute path. This change is not yet present in a released Apple OS, but we intend to make this the default behavior on all platforms.

ole commented 1 month ago

@jrflat Thank you for the clarification. It would be nice to have all these details documented somewhere, i.e. a per-platform-per-version list of what APIs actually use the new Swift Foundation vs. the old Foundation (Darwin Foundation or swift-corelibs-foundation, respectively).

I understand that the overall goal of Swift Foundation is for such a list to eventually become (largely) unnecessary, but I fear the transition period is going to be long, and the official communication from Apple isn't super helpful because it's too broad and optimistic IMO ("Look, there's a new unified version of Foundation and as of late 2024 it is actually the active Foundation on all platforms").

But I should probably raise this issue on the forums. Sorry for ranting at you.