Skip to content

Use a regular downcast instead of an unsafe downcast for Error to NSError conversion #5222

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use a regular downcast instead of an unsafe downcast for Error to NSError conversion #5222

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions Sources/Foundation/NSError.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -891,8 +891,8 @@ func _convertNSErrorToError(_ error: NSError?) -> Error {

public // COMPILER_INTRINSIC
func _convertErrorToNSError(_ error: Error) -> NSError {
if let object = _extractDynamicValue(error as Any) {
return unsafeDowncast(object, to: NSError.self)
if let object = _extractDynamicValue(error as Any), let asNS = object as? NSError {
Copy link
Contributor

@jmschonfeld jmschonfeld Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I suppose CI will tell us, but I don't expect this will work here. I don't know the full details, but I believe that _convertErrorToNSError is effectively the implementation of as NSError, so I suspect this is likely to cause infinite recursion. To check that, I compiled

public func foo(_ e: any Error) -> NSError {
    e as NSError
}

in GodBolt on Linux and saw that it indeed compiles down to a call to this function:

output.foo(Swift.Error) -> Foundation.NSError:
        b       ($s10Foundation22_convertErrorToNSErroryAA0E0Cs0C0_pF)

That said, I don't know the answer to what this should be instead. I'd be interested to know what the type of object is here in the failing case

Copy link
Contributor Author

@parkera parkera Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I did run unit tests on this locally, but it's possible we for some reason do not hit this path.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jmschonfeld

I'd be interested to know what the type of object is here in the failing case

The original bug report has a very short reproducer:

import Foundation

public class BotError: Error {}

print(BotError().localizedDescription)

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose CI will tell us, but I don't expect this will work here. I don't know the full details, but I believe that _convertErrorToNSError is effectively the implementation of as NSError, so I suspect this is likely to cause infinite recursion.

If you compare

public func foo(_ e: any Error) -> NSError? {
    e as? NSError
}

public func bar(_ e: any Error) -> NSError? {
    e as NSError
}

you'll see that as? NSError causes a call to swift_dynamicCast rather than a call to _convertErrorToNSError.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like with the reproducer, both error and object are the BotError instance:

(lldb) frame select 2
frame #2: 0x0000aaaae793437c TestCLI`_convertErrorToNSError(error=0x0000aaab15256780) at NSError.swift:895:16
   892 	public // COMPILER_INTRINSIC
   893 	func _convertErrorToNSError(_ error: Error) -> NSError {
   894 	    if let object = _extractDynamicValue(error as Any) {
-> 895 	        return unsafeDowncast(object, to: NSError.self)
    	               ^
   896 	    } else {
   897 	        let domain: String
   898 	        let code: Int
(lldb) frame variable
(TestCLI.BotError) error = 0x0000aaab15256780 {}
(TestCLI.BotError) object = 0x0000aaab15256780 {}

Copy link
Contributor

Choose a reason for hiding this comment

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

And when the error type is a struct or enum instead of a class, _extractDynamicValue returns nil

return asNS
} else {
let domain: String
let code: Int
Expand Down