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

Swift errors don't provide localizedDescription #2110

Closed
mhammond opened this issue May 15, 2024 · 6 comments · Fixed by #2116
Closed

Swift errors don't provide localizedDescription #2110

mhammond opened this issue May 15, 2024 · 6 comments · Fixed by #2116

Comments

@mhammond
Copy link
Member

With the patch below, coveralls fails because e.localizedDescription is The operation couldn’t be completed. (coverall.ComplexError error 0.). We are hitting this in our production code because our swift app uses localizedDescription when reporting errors to our external error reporting frameworks etc.

I'm not sure what the right thing to do here is - a naive solution will not, by any reasonable definition, be "localized". However, it's fairly clear to me that the default message is quite useless - particularly when the error is an interface like the other part of the patch - you can't really obtain any details about the error (whereas in the enum case, the "error number" shown is apparently the zero-based index of the enum variant)

diff --git a/fixtures/coverall/tests/bindings/test_coverall.swift b/fixtures/coverall/tests/bindings/test_coverall.swift
index 91e95342cd...419b45851e 100644
--- a/fixtures/coverall/tests/bindings/test_coverall.swift
+++ b/fixtures/coverall/tests/bindings/test_coverall.swift
@@ -144,7 +144,10 @@
         } else {
             fatalError("wrong error variant: \(e)")
         }
-        assert(String(describing: e) == "OsError(code: 10, extendedCode: 20)", "Unexpected ComplexError.OsError description: \(e)")
+        let expectedString = "OsError(code: 10, extendedCode: 20)";
+        assert(String(describing: e) == expectedString)
+        print("Actual localized", e.localizedDescription)
+        assert(e.localizedDescription == expectedString)
     }
 
     do {
diff --git a/fixtures/error-types/tests/bindings/test.swift b/fixtures/error-types/tests/bindings/test.swift
index cc409c4ad0...1e13b2b021 100644
--- a/fixtures/error-types/tests/bindings/test.swift
+++ b/fixtures/error-types/tests/bindings/test.swift
@@ -20,7 +20,9 @@
 }
 
 let e = getError(message: "the error")
+print("LD:", e.localizedDescription)
+assert(e.localizedDescription == "??")
 assert(String(describing: e) == "the error")
 assert(String(reflecting: e) == "ErrorInterface { e: the error }")
 assert(Error.self is Swift.Error.Type)
 assert(Error.self != Swift.Error.self)

@linabutler
Copy link
Member

linabutler commented May 17, 2024

We talked about this at our catch-up this week, but I wanted to capture the context here, too! 😊

I think we want our Swift errors to conform to the LocalizedError protocol, and specifically implement the errorDescription property with the message.

That'll bridge transparently to NSError.localizedDescription, which should work with all error reporters; in Swift and when passed to (or through) Objective-C.

"Localized description" is a bit of a misnomer—we don't actually need to localize the strings 🎉 I think it's a Cocoa convention back from when NSErrors were intended to be shown directly to the user (that's also why "help anchor", "recovery suggestion", etc. exist) in AppKit, for document-based macOS apps.

But I can't say I've ever seen a modern app do this, and IIRC UIKit (for iOS apps) doesn't even have an API for presenting an NSError directly to the user. The name has stuck around, but the meaning and intent of it has changed.

@mhammond
Copy link
Member Author

Thanks Lina. I had a quick attempt to make this work. First, I added the test:

    // catch plain exception
    do {
        let _ = try coveralls.maybeThrow(shouldThrow: true)
        fatalError("Should have thrown")
    } catch {
        assert(String(describing: error) == "TooManyHoles(message: \"The coverall has too many holes\")")
        // Getting: The operation couldn’t be completed. (coverall.CoverallError error 0.)
        print(error.localizedDescription)
        assert(error.localizedDescription == "TooManyHoles(message: \"The coverall has too many holes\")")
    }

that first assert passes - which I think demonstrates that just converting the error to a string gives me a reasonable value I can use for localizedDescription.

So I then tried adding var localizedDescription: String { assert(false); String(describing: self) } (That assertion was a late addition to see if the code was even being called - it is not) to both the block where we extend Swift.Error, then out of desparation, to the body of each error enum - but in both cases it had no effect. Suspiciously, if I mis-spell the name as something like XlocalizedDescription I get no compile errors, which I did expect when describing the Swift.Error protocol.

Anyone have ideas?

@linabutler
Copy link
Member

@mhammond Hmmm, would you mind giving this a try?

diff --git a/uniffi_bindgen/src/bindings/swift/templates/ErrorTemplate.swift b/uniffi_bindgen/src/bindings/swift/templates/ErrorTemplate.swift
index 17aba8ca6..37655b619 100644
--- a/uniffi_bindgen/src/bindings/swift/templates/ErrorTemplate.swift
+++ b/uniffi_bindgen/src/bindings/swift/templates/ErrorTemplate.swift
@@ -83,4 +83,36 @@ public struct {{ ffi_converter_name }}: FfiConverterRustBuffer {
 {% if !contains_object_references %}
 extension {{ type_name }}: Equatable, Hashable {}
 {% endif %}
-extension {{ type_name }}: Swift.Error { }
+extension {{ type_name }}: Foundation.LocalizedError {
+    public var errorDescription: String? {
+        switch self {
+            {% if e.is_flat() %}
+            {% for variant in e.variants() %}
+            case .{{ variant.name()|class_name }}(let message):
+                return message
+            {% endfor %}
+
+            {%- else %}
+            {% for variant in e.variants() %}
+            case .{{ variant.name()|class_name }}{% if variant.has_fields() -%}(
+            {% for field in variant.fields() -%}
+                let {{ field.name()|var_name }}
+            {%- if !loop.last %}, {% endif %}
+            {% endfor -%}
+            ){% endif -%}:
+            {% if variant.has_fields() -%}
+                return """{{ variant.name()|class_name }} { \
+                {% for field in variant.fields() -%}
+                {{ field.name()|var_name }} : \(String(describing: {{ field.name()|var_name }})) \
+                {%- if !loop.last %}, {% endif %}
+                {% endfor -%}
+                }
+                """
+            {% else %}
+                return "{{ variant.name()|class_name }}"
+            {% endif %}
+            {% endfor -%}
+            {% endif %}
+        }
+    }
+}

The protocol is named Foundation.LocalizedError, and the property on that is errorDescription. Does localizedDescription pick that up?

(If String(describing: self) works, even better; I just opted for building the strings longhand in case there were any issues with infinite recursion, where String(describing:) ends up calling errorDescription...but I'm not sure if that's actually an issue).

Suspiciously, if I mis-spell the name as something like XlocalizedDescription I get no compile errors, which I did expect when describing the Swift.Error protocol.

I think Swift.Error.localizedDescription has a default, magic implementation, so XlocalizedDescription just sets a vanilla property called XlocalizedDescription on the enum, and Swift.Error.localizedDescription remains the default.

@mhammond
Copy link
Member Author

mhammond commented May 17, 2024

The protocol is named Foundation.LocalizedError, and the property on that is errorDescription.

I saw that on NSError, but then I also saw https://developer.apple.com/documentation/swift/error/localizeddescription which is very vague but implied to my naive eyes that maybe it was a property of exactly that name on the error or something as well as defined on NSError and there was some magic bridge.

But thanks, I'll give that a try a little later.

@linabutler
Copy link
Member

linabutler commented May 17, 2024

maybe it was a property of exactly that name on the error

Yeah, it's super confusing because I think Swift.Error.localizedDescription is an extension on the protocol type—it's not actually a property requirement of the protocol itself.

IOW, Swift.Error looks something like:

protocol Swift.Error {}
extension Swift.Error {
    var localizedDescription: String {
        return (self as! NSError).localizedDescription
    }
}

Whereas the LocalizedError protocol looks like:

protocol LocalizedError : Swift.Error {
    var errorDescription: String { get }
    // ...
}

Unlike Swift.Error.localizedDescription, LocalizedError.errorDescription is a property requirement. And when a LocalizedError is bridged to an NSError, LocalizedError.errorDescription becomes NSError.localizedDescription.

The confusing part about this is that any Swift.Error can be bridged to NSError (not just LocalizedError)—and when a plain Swift.Error bridges to NSError, NSError.localizedDescription uses the generic "operation couldn't be completed" message. Jordan's comment explains this in a pithier way.

@mhammond
Copy link
Member Author

The confusing part about this is that any Swift.Error can be bridged to NSError (not just LocalizedError)—and when a plain Swift.Error bridges to NSError, NSError.localizedDescription uses the generic "operation couldn't be completed" message. Jordan's comment explains this in a pithier way.

That was super helpful, thanks - it even came with receipts :) String(reflecting:) seems to work fine and is consistent with error interfaces where the Rust Debug implementation is used, so I went with that - thanks for your help.

mhammond added a commit to mhammond/uniffi-rs that referenced this issue May 18, 2024
Also some extra error string semantics tests for kotlin.

Fixes mozilla#2110
mhammond added a commit to mhammond/uniffi-rs that referenced this issue May 18, 2024
Also some extra error string semantics tests for kotlin.

Fixes mozilla#2110
mhammond added a commit to mhammond/uniffi-rs that referenced this issue May 18, 2024
Also some extra error string semantics tests for kotlin.

Fixes mozilla#2110
mhammond added a commit that referenced this issue May 18, 2024
Also some extra error string semantics tests for kotlin.

Fixes #2110
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants