Skip to content
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.

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

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

Open
swillits opened this issue Oct 24, 2024 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@swillits
Copy link

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
@swillits swillits added the bug Something isn't working label Oct 24, 2024
@jmschonfeld jmschonfeld transferred this issue from swiftlang/swift Oct 25, 2024
@jmschonfeld
Copy link
Contributor

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

@jrflat
Copy link
Contributor

jrflat commented Oct 25, 2024

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 :)

@jrflat jrflat self-assigned this Oct 25, 2024
@ole
Copy link

ole commented Oct 26, 2024

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
Copy link
Author

@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
Copy link
Contributor

jrflat commented Oct 28, 2024

@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
Copy link

ole commented Oct 29, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants