Skip to content

Commit

Permalink
Improve error outputs for Python code
Browse files Browse the repository at this point in the history
Fixes #1555.

Before:
```
>>> libtw2_huffman.decompress(b"")
Traceback (most recent call last):
[…]
libtw2_huffman.UniFFIExceptionTmpNamespace.InvalidInput: DecompressionError.InvalidInput('input is not a valid huffman compression')
```

After:
```
>>> libtw2_huffman.decompress(b"")
Traceback (most recent call last):
[…]
libtw2_huffman.DecompressionError.InvalidInput: input is not a valid huffman compression
```
  • Loading branch information
heinrich5991 committed May 25, 2023
1 parent f243005 commit d5b6eda
Show file tree
Hide file tree
Showing 8 changed files with 71 additions and 34 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
See [the documentation](https://mozilla.github.io/uniffi-rs/udl/interfaces.html#exposing-traits-as-interfaces).

- The `bytes` primitive type was added, it represents an array of bytes. It maps to `ByteArray` in Kotlin, `bytes` in Python, `String` with `Encoding::BINARY` in Ruby and `Data` in Swift.
- Shortened `str()` representations of errors in Python to align with other exceptions in Python. Use `repr()` or the `{!r}` format to get the old representation back.

## v0.23.0 (backend crates: v0.23.0) - (_2023-01-27_)

Expand Down
5 changes: 3 additions & 2 deletions fixtures/coverall/src/coverall.udl
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,9 @@ enum CoverallError {

[Error]
interface ComplexError {
OsError(i16 code, i16 extended_code);
PermissionDenied(string reason);
OsError(i16 code, i16 extended_code);
PermissionDenied(string reason);
UnknownError();
};

interface Coveralls {
Expand Down
3 changes: 3 additions & 0 deletions fixtures/coverall/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ pub enum ComplexError {
OsError { code: i16, extended_code: i16 },
#[error("PermissionDenied: {reason}")]
PermissionDenied { reason: String },
#[error("Unknown error")]
UnknownError,
}

#[derive(Clone, Debug, Default)]
Expand Down Expand Up @@ -203,6 +205,7 @@ impl Coveralls {
2 => Err(ComplexError::PermissionDenied {
reason: "Forbidden".to_owned(),
}),
3 => Err(ComplexError::UnknownError),
_ => panic!("Invalid input"),
}
}
Expand Down
13 changes: 11 additions & 2 deletions fixtures/coverall/tests/bindings/test_coverall.kts
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ Coveralls("test_complex_errors").use { coveralls ->
assert(e.code == 10.toShort())
assert(e.extendedCode == 20.toShort())
assert(e.toString() == "uniffi.coverall.ComplexException\$OsException: code=10, extendedCode=20") {
"Unexpected ComplexError.OsError.toString() value: ${e.toString()}"
"Unexpected ComplexException.OsError.toString() value: ${e.toString()}"
}
}

Expand All @@ -181,13 +181,22 @@ Coveralls("test_complex_errors").use { coveralls ->
} catch(e: ComplexException.PermissionDenied) {
assert(e.reason == "Forbidden")
assert(e.toString() == "uniffi.coverall.ComplexException\$PermissionDenied: reason=Forbidden") {
"Unexpected ComplexError.PermissionDenied.toString() value: ${e.toString()}"
"Unexpected ComplexException.PermissionDenied.toString() value: ${e.toString()}"
}
}

try {
coveralls.maybeThrowComplex(3)
throw RuntimeException("Expected method to throw exception")
} catch(e: ComplexException.UnknownException) {
assert(e.toString() == "uniffi.coverall.ComplexException\$UnknownException: ") {
"Unexpected ComplexException.UnknownException.toString() value: ${e.toString()}"
}
}

try {
coveralls.maybeThrowComplex(4)
throw RuntimeException("Expected method to throw exception")
} catch(e: InternalException) {
// Expected result
}
Expand Down
13 changes: 10 additions & 3 deletions fixtures/coverall/tests/bindings/test_coverall.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,16 +116,23 @@ def test_complex_errors(self):
coveralls.maybe_throw_complex(1)
self.assertEqual(cm.exception.code, 10)
self.assertEqual(cm.exception.extended_code, 20)
self.assertEqual(str(cm.exception), "ComplexError.OsError(code=10, extended_code=20)")
self.assertEqual(str(cm.exception), "code=10, extended_code=20")
self.assertEqual(repr(cm.exception), "ComplexError.OsError(code=10, extended_code=20)")

with self.assertRaises(ComplexError.PermissionDenied) as cm:
coveralls.maybe_throw_complex(2)
self.assertEqual(cm.exception.reason, "Forbidden")
self.assertEqual(str(cm.exception), "ComplexError.PermissionDenied(reason='Forbidden')")
self.assertEqual(str(cm.exception), "reason='Forbidden'")
self.assertEqual(repr(cm.exception), "ComplexError.PermissionDenied(reason='Forbidden')")

with self.assertRaises(ComplexError.UnknownError) as cm:
coveralls.maybe_throw_complex(3)
self.assertEqual(str(cm.exception), "")
self.assertEqual(repr(cm.exception), "ComplexError.UnknownError()")

# Test panics, which should cause InternalError to be raised
with self.assertRaises(InternalError) as cm:
coveralls.maybe_throw_complex(3)
coveralls.maybe_throw_complex(4)

def test_self_by_arc(self):
coveralls = Coveralls("test_self_by_arc")
Expand Down
10 changes: 9 additions & 1 deletion fixtures/coverall/tests/bindings/test_coverall.rb
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,16 @@ def test_complex_errors
raise 'should have thrown'
end

assert_raise Coverall::InternalError do
begin
coveralls.maybe_throw_complex(3)
rescue Coverall::ComplexError::UnknownError => err
assert_equal err.to_s, 'Coverall::ComplexError::UnknownError()'
else
raise 'should have thrown'
end

assert_raise Coverall::InternalError do
coveralls.maybe_throw_complex(4)
end
end

Expand Down
11 changes: 11 additions & 0 deletions fixtures/coverall/tests/bindings/test_coverall.swift
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,17 @@ do {
do {
let _ = try coveralls.maybeThrowComplex(input: 3)
fatalError("should have thrown")
} catch let e as ComplexError {
if case .UnknownError = e {
} else {
fatalError("wrong error variant: \(e)")
}
assert(String(describing: e) == "UnknownError", "Unexpected ComplexError.UnknownError description: \(e)")
}

do {
let _ = try coveralls.maybeThrowComplex(input: 4)
fatalError("should have thrown")
} catch {
assert(String(describing: error) == "rustPanic(\"Invalid input\")")
}
Expand Down
49 changes: 23 additions & 26 deletions uniffi_bindgen/src/bindings/python/templates/ErrorTemplate.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,47 +3,44 @@
# {{ type_name }}
# We want to define each variant as a nested class that's also a subclass,
# which is tricky in Python. To accomplish this we're going to create each
# class separated, then manually add the child classes to the base class's
# class separately, then manually add the child classes to the base class's
# __dict__. All of this happens in dummy class to avoid polluting the module
# namespace.
class UniFFIExceptionTmpNamespace:
class {{ type_name }}(Exception):
pass
{% for variant in e.variants() %}
{%- let variant_type_name = variant.name()|class_name %}
class {{ type_name }}(Exception):
pass

UniFFITemp{{ type_name }} = {{ type_name }}

class {{ type_name }}:
{%- for variant in e.variants() -%}
{%- let variant_type_name = variant.name()|class_name -%}
{%- if e.is_flat() %}
class {{ variant_type_name }}({{ type_name }}):
def __str__(self):
return "{{ type_name }}.{{ variant_type_name }}({})".format(repr(super().__str__()))
class {{ variant_type_name }}(UniFFITemp{{ type_name }}):
def __repr__(self):
return "{{ type_name }}.{{ variant_type_name }}({})".format(repr(str(self)))
{%- else %}
class {{ variant_type_name }}({{ type_name }}):
class {{ variant_type_name }}(UniFFITemp{{ type_name }}):
def __init__(self{% for field in variant.fields() %}, {{ field.name()|var_name }}{% endfor %}):
{%- if variant.has_fields() %}
super().__init__(", ".join([
{%- for field in variant.fields() %}
"{{ field.name()|var_name }}={!r}".format({{ field.name()|var_name }}),
{%- endfor %}
]))
{%- for field in variant.fields() %}
self.{{ field.name()|var_name }} = {{ field.name()|var_name }}
{%- endfor %}
{%- else %}
pass
{%- endif %}

def __str__(self):
{%- if variant.has_fields() %}
field_parts = [
{%- for field in variant.fields() %}
'{{ field.name()|var_name }}={!r}'.format(self.{{ field.name()|var_name }}),
{%- endfor %}
]
return "{{ type_name }}.{{ variant_type_name }}({})".format(', '.join(field_parts))
{%- else %}
return "{{ type_name }}.{{ variant_type_name }}()"
{%- endif %}
def __repr__(self):
return "{{ type_name }}.{{ variant_type_name }}({})".format(str(self))
{%- endif %}

{{ type_name }}.{{ variant_type_name }} = {{ variant_type_name }}
UniFFITemp{{ type_name }}.{{ variant_type_name }} = {{ variant_type_name }}
{%- endfor %}
{{ type_name }} = UniFFIExceptionTmpNamespace.{{ type_name }}
del UniFFIExceptionTmpNamespace

{{ type_name }} = UniFFITemp{{ type_name }}
del UniFFITemp{{ type_name }}


class {{ ffi_converter_name }}(FfiConverterRustBuffer):
Expand Down

0 comments on commit d5b6eda

Please sign in to comment.