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

Fix regression in Kotlin error/exception names. #1842

Merged
merged 1 commit into from
Nov 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Don't remove the forked trait for b/w compat reasons
  • Loading branch information
mhammond committed Nov 13, 2023
commit 63d06e7eb423f8482f1d5c1c5c74af93b9ceb118
59 changes: 58 additions & 1 deletion fixtures/coverall/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ impl From<InternalCoverallError> for CoverallError {
}
}

#[derive(Debug, thiserror::Error, PartialEq, Eq)]
#[derive(Clone, Debug, thiserror::Error, PartialEq, Eq)]
pub enum ComplexError {
#[error("OsError: {code} ({extended_code})")]
OsError { code: i16, extended_code: i16 },
Expand Down Expand Up @@ -108,6 +108,63 @@ fn throw_complex_macro_error() -> Result<(), ComplexMacroError> {
})
}

// Note: intentionally *does not* derive `uniffi::Error`, yet ends with `Error`, just to
// mess with Kotlin etc.
#[derive(Clone, Debug, uniffi::Enum)]
pub enum OtherError {
Unexpected,
}

#[derive(Clone, Debug, thiserror::Error, uniffi::Error)]
pub enum RootError {
#[error(transparent)]
// XXX - note Kotlin fails if this variant was called ComplexError
// (ie, the variant name can't match an existing type)
Complex {
#[from]
error: ComplexError,
},
#[error("Other Error")]
Other { error: OtherError },
}

// For Kotlin, we throw a variant which itself is a plain enum.
#[uniffi::export]
fn throw_root_error() -> Result<(), RootError> {
Err(RootError::Complex {
error: ComplexError::OsError {
code: 1,
extended_code: 2,
},
})
}

#[uniffi::export]
fn get_root_error() -> RootError {
RootError::Other {
error: OtherError::Unexpected,
}
}

#[uniffi::export]
fn get_complex_error(e: Option<ComplexError>) -> ComplexError {
e.unwrap_or(ComplexError::PermissionDenied {
reason: "too complex".to_string(),
})
}

#[uniffi::export]
fn get_error_dict(d: Option<ErrorDict>) -> ErrorDict {
d.unwrap_or(Default::default())
}

#[derive(Default, Debug, uniffi::Record)]
pub struct ErrorDict {
complex_error: Option<ComplexError>,
root_error: Option<RootError>,
errors: Vec<RootError>,
}

#[derive(Clone, Debug, Default)]
pub struct SimpleDict {
text: String,
Expand Down
18 changes: 18 additions & 0 deletions fixtures/coverall/tests/bindings/test_coverall.kts
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,24 @@ Coveralls("test_complex_errors").use { coveralls ->
}
}

Coveralls("test_error_values").use { _coveralls ->
try {
throwRootError()
throw RuntimeException("Expected method to throw exception")
} catch(e: RootException.Complex) {
assert(e.error is ComplexException.OsException)
}
val e = getRootError()
if (e is RootException.Other)
assert(e.error == OtherError.UNEXPECTED) {
} else {
throw RuntimeException("Unexpected error subclass")
}
val ce = getComplexError(null)
assert(ce is ComplexException.PermissionDenied)
assert(getErrorDict(null).complexError == null)
}

Coveralls("test_interfaces_in_dicts").use { coveralls ->
coveralls.addPatch(Patch(Color.RED))
coveralls.addRepair(
Expand Down
11 changes: 11 additions & 0 deletions fixtures/coverall/tests/bindings/test_coverall.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,17 @@ def test_complex_errors(self):
with self.assertRaises(InternalError) as cm:
coveralls.maybe_throw_complex(4)

def test_error_values(self):
with self.assertRaises(RootError.Complex) as cm:
throw_root_error()
self.assertEqual(cm.exception.error.code, 1)

e = get_root_error()
self.assertEqual(e.error, OtherError.UNEXPECTED)

self.assertTrue(isinstance(get_complex_error(None), ComplexError.PermissionDenied))
self.assertIsNone(get_error_dict(None).complex_error)

def test_enums(self):
e = get_simple_flat_macro_enum(0)
self.assertTrue(isinstance(e, SimpleFlatMacroEnum.FIRST))
Expand Down
32 changes: 32 additions & 0 deletions fixtures/coverall/tests/bindings/test_coverall.swift
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,38 @@ do {

}

// Test error values, including error enums with error variants.
do {
do {
let _ = try throwRootError()
fatalError("should have thrown")
} catch let e as RootError {
if case let .Complex(error) = e {
if case let .OsError(code, extendedCode) = error {
assert(code == 1)
assert(extendedCode == 2)
} else {
fatalError("wrong error variant: \(e)")
}
} else {
fatalError("wrong error variant: \(e)")
}
}
let e = getRootError();
if case let .Other(error) = e {
assert(error == OtherError.unexpected)
} else {
fatalError("wrong error variant: \(e)")
}
let e2 = getComplexError(e: nil);
if case let .PermissionDenied(error) = e2 {
assert(error == "too complex")
} else {
fatalError("wrong error variant: \(e)")
}
assert(getErrorDict(d: nil).complexError == nil)
}

// Swift GC is deterministic, `coveralls` is freed when it goes out of scope.
assert(getNumAlive() == 0);

Expand Down
4 changes: 4 additions & 0 deletions uniffi_bindgen/src/backend/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
use super::Literal;
use std::fmt::Debug;

// XXX - Note that this trait is not used internally. It exists just to avoid an unnecessary
// breaking change for external bindings which use this trait.
// It is likely to be removed some time after 0.26.x.

/// A Trait to help render types in a language specific format.
pub trait CodeType: Debug {
/// The language specific label used to reference this type. This will be used in
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

use crate::backend::{CodeType, Literal};
use super::CodeType;
use crate::ComponentInterface;

#[derive(Debug)]
pub struct CallbackInterfaceCodeType {
Expand All @@ -16,18 +17,14 @@ impl CallbackInterfaceCodeType {
}

impl CodeType for CallbackInterfaceCodeType {
fn type_label(&self) -> String {
super::KotlinCodeOracle.class_name(&self.id)
fn type_label(&self, ci: &ComponentInterface) -> String {
super::KotlinCodeOracle.class_name(ci, &self.id)
}

fn canonical_name(&self) -> String {
format!("Type{}", self.id)
}

fn literal(&self, _literal: &Literal) -> String {
unreachable!();
}

fn initialization_fn(&self) -> Option<String> {
Some(format!("uniffiCallbackInterface{}.register", self.id))
}
Expand Down
30 changes: 16 additions & 14 deletions uniffi_bindgen/src/bindings/kotlin/gen_kotlin/compounds.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,19 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

use crate::backend::{CodeType, Literal, Type};
use super::{AsCodeType, CodeType};
use crate::backend::{Literal, Type};
use crate::ComponentInterface;
use paste::paste;

fn render_literal(literal: &Literal, inner: &Type) -> String {
fn render_literal(literal: &Literal, inner: &Type, ci: &ComponentInterface) -> String {
match literal {
Literal::Null => "null".into(),
Literal::EmptySequence => "listOf()".into(),
Literal::EmptyMap => "mapOf()".into(),

// For optionals
_ => super::KotlinCodeOracle.find(inner).literal(literal),
_ => super::KotlinCodeOracle.find(inner).literal(literal, ci),
}
}

Expand All @@ -34,16 +36,16 @@ macro_rules! impl_code_type_for_compound {
}

impl CodeType for $T {
fn type_label(&self) -> String {
format!($type_label_pattern, super::KotlinCodeOracle.find(self.inner()).type_label())
fn type_label(&self, ci: &ComponentInterface) -> String {
format!($type_label_pattern, super::KotlinCodeOracle.find(self.inner()).type_label(ci))
}

fn canonical_name(&self) -> String {
format!($canonical_name_pattern, super::KotlinCodeOracle.find(self.inner()).canonical_name())
}

fn literal(&self, literal: &Literal) -> String {
render_literal(literal, self.inner())
fn literal(&self, literal: &Literal, ci: &ComponentInterface) -> String {
render_literal(literal, self.inner(), ci)
}
}
}
Expand Down Expand Up @@ -74,23 +76,23 @@ impl MapCodeType {
}

impl CodeType for MapCodeType {
fn type_label(&self) -> String {
fn type_label(&self, ci: &ComponentInterface) -> String {
format!(
"Map<{}, {}>",
super::KotlinCodeOracle.find(self.key()).type_label(),
super::KotlinCodeOracle.find(self.value()).type_label(),
super::KotlinCodeOracle.find(self.key()).type_label(ci),
super::KotlinCodeOracle.find(self.value()).type_label(ci),
)
}

fn canonical_name(&self) -> String {
format!(
"Map{}{}",
super::KotlinCodeOracle.find(self.key()).canonical_name(),
super::KotlinCodeOracle.find(self.value()).canonical_name(),
self.key().as_codetype().canonical_name(),
self.value().as_codetype().canonical_name(),
)
}

fn literal(&self, literal: &Literal) -> String {
render_literal(literal, &self.value)
fn literal(&self, literal: &Literal, ci: &ComponentInterface) -> String {
render_literal(literal, &self.value, ci)
}
}
9 changes: 3 additions & 6 deletions uniffi_bindgen/src/bindings/kotlin/gen_kotlin/custom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

use crate::backend::{CodeType, Literal};
use super::CodeType;
use crate::ComponentInterface;

#[derive(Debug)]
pub struct CustomCodeType {
Expand All @@ -16,15 +17,11 @@ impl CustomCodeType {
}

impl CodeType for CustomCodeType {
fn type_label(&self) -> String {
fn type_label(&self, _ci: &ComponentInterface) -> String {
self.name.clone()
}

fn canonical_name(&self) -> String {
format!("Type{}", self.name)
}

fn literal(&self, _literal: &Literal) -> String {
unreachable!("Can't have a literal of a custom type");
}
}
12 changes: 7 additions & 5 deletions uniffi_bindgen/src/bindings/kotlin/gen_kotlin/enum_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

use crate::backend::{CodeType, Literal};
use super::CodeType;
use crate::backend::Literal;
use crate::ComponentInterface;

#[derive(Debug)]
pub struct EnumCodeType {
Expand All @@ -16,19 +18,19 @@ impl EnumCodeType {
}

impl CodeType for EnumCodeType {
fn type_label(&self) -> String {
super::KotlinCodeOracle.class_name(&self.id)
fn type_label(&self, ci: &ComponentInterface) -> String {
super::KotlinCodeOracle.class_name(ci, &self.id)
}

fn canonical_name(&self) -> String {
format!("Type{}", self.id)
}

fn literal(&self, literal: &Literal) -> String {
fn literal(&self, literal: &Literal, ci: &ComponentInterface) -> String {
if let Literal::Enum(v, _) = literal {
format!(
"{}.{}",
self.type_label(),
self.type_label(ci),
super::KotlinCodeOracle.enum_variant_name(v)
)
} else {
Expand Down
31 changes: 0 additions & 31 deletions uniffi_bindgen/src/bindings/kotlin/gen_kotlin/error.rs

This file was deleted.

5 changes: 3 additions & 2 deletions uniffi_bindgen/src/bindings/kotlin/gen_kotlin/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

use crate::backend::CodeType;
use super::CodeType;
use crate::ComponentInterface;

#[derive(Debug)]
pub struct ForeignExecutorCodeType;

impl CodeType for ForeignExecutorCodeType {
fn type_label(&self) -> String {
fn type_label(&self, _ci: &ComponentInterface) -> String {
// Kotlin uses a CoroutineScope for ForeignExecutor
"CoroutineScope".into()
}
Expand Down
Loading