Skip to content

Implement Lift for flat errors #1847

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

Merged
merged 1 commit into from
Nov 15, 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
20 changes: 20 additions & 0 deletions fixtures/coverall/src/coverall.udl
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ namespace coverall {
void test_getters(Getters g);

sequence<string> ancestor_names(NodeTrait node);

ReturnOnlyDict output_return_only_dict();
ReturnOnlyEnum output_return_only_enum();

void try_input_return_only_dict(ReturnOnlyDict d);
};

dictionary SimpleDict {
Expand Down Expand Up @@ -49,6 +54,21 @@ dictionary SimpleDict {
NodeTrait? test_trait;
};

// Create a type that stores `CoverallFlatError` and therefore can only be lowered but not lifted.
// UniFFI should define a `Lower` implemenation but not try to define `Lift`.
dictionary ReturnOnlyDict {
CoverallFlatError e;
};

// More complicated version of the above, each variant is return-only for different reasons
[Enum]
interface ReturnOnlyEnum {
One(CoverallFlatError e);
Two(ReturnOnlyDict d);
Three(sequence<CoverallFlatError> l);
Four(record<string, CoverallFlatError> m);
};

dictionary DictWithDefaults {
string name = "default-value";
string? category = null;
Expand Down
38 changes: 38 additions & 0 deletions fixtures/coverall/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,44 @@ pub struct SimpleDict {
test_trait: Option<Arc<dyn NodeTrait>>,
}

pub struct ReturnOnlyDict {
e: CoverallFlatError,
}

pub enum ReturnOnlyEnum {
One {
e: CoverallFlatError,
},
Two {
d: ReturnOnlyDict,
},
Three {
l: Vec<CoverallFlatError>,
},
Four {
m: HashMap<String, CoverallFlatError>,
},
}

fn output_return_only_dict() -> ReturnOnlyDict {
ReturnOnlyDict {
e: CoverallFlatError::TooManyVariants { num: 1 },
}
}

fn output_return_only_enum() -> ReturnOnlyEnum {
ReturnOnlyEnum::One {
e: CoverallFlatError::TooManyVariants { num: 2 },
}
}

fn try_input_return_only_dict(_d: ReturnOnlyDict) {
// This function can't work because ReturnOnlyDict contains a flat error and therefore
// can't be lifted by Rust. There's a Python test that the UniFFI code panics before we get here.
//
// FIXME: should be a compile-time error rather than a runtime error (#1850)
}

#[derive(Debug, Clone)]
pub struct DictWithDefaults {
name: String,
Expand Down
8 changes: 8 additions & 0 deletions fixtures/coverall/tests/bindings/test_coverall.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import unittest
from datetime import datetime, timezone

from coverall import *

class TestCoverall(unittest.TestCase):
Expand Down Expand Up @@ -289,6 +290,13 @@ def test_bytes(self):
coveralls = Coveralls("test_bytes")
self.assertEqual(coveralls.reverse(b"123"), b"321")

def test_return_only_dict(self):
# try_input_return_only_dict can never work, since ReturnOnlyDict should only be returned
# from Rust not inputted. Test that an attempt raises an internal error rather than tries
# to use an invalid value.
with self.assertRaises(InternalError):
try_input_return_only_dict(ReturnOnlyDict(e=CoverallFlatError.TooManyVariants))

class PyGetters:
def get_bool(self, v, arg2):
return v ^ arg2
Expand Down
30 changes: 27 additions & 3 deletions uniffi_macros/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ fn flat_error_ffi_converter_impl(
) -> TokenStream {
let name = ident_to_string(ident);
let lower_impl_spec = tagged_impl_header("Lower", ident, udl_mode);
let lift_impl_spec = tagged_impl_header("Lift", ident, udl_mode);
let derive_ffi_traits = derive_ffi_traits(ident, udl_mode, &["ConvertError"]);
let mod_path = match mod_path() {
Ok(p) => p,
Expand Down Expand Up @@ -127,8 +128,7 @@ fn flat_error_ffi_converter_impl(
}
};

let lift_impl = implement_lift.then(|| {
let lift_impl_spec = tagged_impl_header("Lift", ident, udl_mode);
let lift_impl = if implement_lift {
let match_arms = enum_.variants.iter().enumerate().map(|(i, v)| {
let v_ident = &v.ident;
let idx = Index::from(i + 1);
Expand Down Expand Up @@ -157,7 +157,31 @@ fn flat_error_ffi_converter_impl(
}

}
});
} else {
quote! {
// Lifting flat errors is not currently supported, but we still define the trait so
// that dicts containing flat errors don't cause compile errors (see ReturnOnlyDict in
// coverall.rs).
//
// Note: it would be better to not derive `Lift` for dictionaries containing flat
// errors, but getting the trait bounds and derived impls there would be much harder.
// For now, we just fail at runtime.
#[automatically_derived]
unsafe #lift_impl_spec {
type FfiType = ::uniffi::RustBuffer;

fn try_read(buf: &mut &[::std::primitive::u8]) -> ::uniffi::deps::anyhow::Result<Self> {
panic!("Can't lift flat errors")
Copy link
Member

Choose a reason for hiding this comment

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

is it worth opening an issue around getting these errors at buildtime instead of a runtime panic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I made #1850 for this.

}

fn try_lift(v: ::uniffi::RustBuffer) -> ::uniffi::deps::anyhow::Result<Self> {
panic!("Can't lift flat errors")
}

const TYPE_ID_META: ::uniffi::MetadataBuffer = <Self as ::uniffi::Lower<crate::UniFfiTag>>::TYPE_ID_META;
}
}
};

quote! {
#lower_impl
Expand Down