From 5a91b36a8e5e4a6a6a18b47d038ea34fabfaf367 Mon Sep 17 00:00:00 2001 From: heinrich5991 Date: Wed, 24 May 2023 17:56:34 +0200 Subject: [PATCH] Improve error outputs for Python code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ``` --- fixtures/coverall/src/coverall.udl | 5 +- fixtures/coverall/src/lib.rs | 3 ++ .../coverall/tests/bindings/test_coverall.kts | 13 ++++- .../coverall/tests/bindings/test_coverall.py | 13 +++-- .../coverall/tests/bindings/test_coverall.rb | 10 +++- .../tests/bindings/test_coverall.swift | 11 +++++ .../python/templates/ErrorTemplate.py | 49 +++++++++---------- 7 files changed, 70 insertions(+), 34 deletions(-) diff --git a/fixtures/coverall/src/coverall.udl b/fixtures/coverall/src/coverall.udl index 8c0c84b3e8..cce9e8088a 100644 --- a/fixtures/coverall/src/coverall.udl +++ b/fixtures/coverall/src/coverall.udl @@ -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 { diff --git a/fixtures/coverall/src/lib.rs b/fixtures/coverall/src/lib.rs index b60947f01d..b40ea324e3 100644 --- a/fixtures/coverall/src/lib.rs +++ b/fixtures/coverall/src/lib.rs @@ -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)] @@ -203,6 +205,7 @@ impl Coveralls { 2 => Err(ComplexError::PermissionDenied { reason: "Forbidden".to_owned(), }), + 3 => Err(ComplexError::UnknownError), _ => panic!("Invalid input"), } } diff --git a/fixtures/coverall/tests/bindings/test_coverall.kts b/fixtures/coverall/tests/bindings/test_coverall.kts index 252d5fec85..62fbbb5a70 100644 --- a/fixtures/coverall/tests/bindings/test_coverall.kts +++ b/fixtures/coverall/tests/bindings/test_coverall.kts @@ -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()}" } } @@ -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 } diff --git a/fixtures/coverall/tests/bindings/test_coverall.py b/fixtures/coverall/tests/bindings/test_coverall.py index fced47aa49..fc64b92f5d 100644 --- a/fixtures/coverall/tests/bindings/test_coverall.py +++ b/fixtures/coverall/tests/bindings/test_coverall.py @@ -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") diff --git a/fixtures/coverall/tests/bindings/test_coverall.rb b/fixtures/coverall/tests/bindings/test_coverall.rb index 5e540466b4..4fe1140419 100644 --- a/fixtures/coverall/tests/bindings/test_coverall.rb +++ b/fixtures/coverall/tests/bindings/test_coverall.rb @@ -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 diff --git a/fixtures/coverall/tests/bindings/test_coverall.swift b/fixtures/coverall/tests/bindings/test_coverall.swift index 50a85c7b95..d380881184 100644 --- a/fixtures/coverall/tests/bindings/test_coverall.swift +++ b/fixtures/coverall/tests/bindings/test_coverall.swift @@ -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\")") } diff --git a/uniffi_bindgen/src/bindings/python/templates/ErrorTemplate.py b/uniffi_bindgen/src/bindings/python/templates/ErrorTemplate.py index 305ae88618..c10f095a13 100644 --- a/uniffi_bindgen/src/bindings/python/templates/ErrorTemplate.py +++ b/uniffi_bindgen/src/bindings/python/templates/ErrorTemplate.py @@ -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):