From 52ccef5962000569c0aeb84c952d3fbe1bc4a316 Mon Sep 17 00:00:00 2001 From: YushiOMOTE Date: Fri, 20 Nov 2020 22:12:19 +0900 Subject: [PATCH 1/9] Remove unsafe Sync implementation --- perde-core/src/decode/class.rs | 2 +- perde-core/src/import.rs | 77 ++++++++++++++-------------------- perde-core/src/object.rs | 38 +++++++++++++++++ perde-core/src/resolve.rs | 6 +-- perde-core/src/schema.rs | 18 ++++---- 5 files changed, 82 insertions(+), 59 deletions(-) diff --git a/perde-core/src/decode/class.rs b/perde-core/src/decode/class.rs index d25f04e..4e9b2ed 100644 --- a/perde-core/src/decode/class.rs +++ b/perde-core/src/decode/class.rs @@ -132,7 +132,7 @@ impl Class { // If default is defined, use it. // If this is optional, return `None`. if let Some(d) = s.attr.default.as_ref() { - return Ok(d.clone()); + return Ok(d.owned()); } else if let Some(d) = s.attr.default_factory.as_ref() { return d.call0(); } else if s.schema.is_optional() { diff --git a/perde-core/src/import.rs b/perde-core/src/import.rs index 0cf320e..f086004 100644 --- a/perde-core/src/import.rs +++ b/perde-core/src/import.rs @@ -1,51 +1,34 @@ -use crate::{error::Result, object::Object}; +use crate::{ + error::Result, + object::{Object, SyncObject}, +}; use pyo3::prelude::*; -use std::ops::Deref; - -#[derive(Debug)] -pub struct StaticObject(Object); - -impl Deref for StaticObject { - type Target = Object; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl From for StaticObject { - fn from(p: pyo3::PyObject) -> Self { - StaticObject(Object::new(p.into_ptr()).unwrap()) - } -} pub struct Import { - pub fields: StaticObject, - pub missing: StaticObject, - pub generic_alias: StaticObject, - pub base_generic_alias: Option, - pub union_generic_alias: Option, - pub special_generic_alias: Option, - pub type_var: StaticObject, - pub any: StaticObject, - pub union: StaticObject, - pub tuple: StaticObject, - pub empty_tuple: StaticObject, - pub optional: StaticObject, - pub dict: StaticObject, - pub list: StaticObject, - pub set: StaticObject, - pub frozenset: StaticObject, - pub enum_meta: StaticObject, - pub datetime: StaticObject, - pub date: StaticObject, - pub time: StaticObject, - pub decimal: StaticObject, - pub uuid: StaticObject, + pub fields: SyncObject, + pub missing: SyncObject, + pub generic_alias: SyncObject, + pub base_generic_alias: Option, + pub union_generic_alias: Option, + pub special_generic_alias: Option, + pub type_var: SyncObject, + pub any: SyncObject, + pub union: SyncObject, + pub tuple: SyncObject, + pub empty_tuple: SyncObject, + pub optional: SyncObject, + pub dict: SyncObject, + pub list: SyncObject, + pub set: SyncObject, + pub frozenset: SyncObject, + pub enum_meta: SyncObject, + pub datetime: SyncObject, + pub date: SyncObject, + pub time: SyncObject, + pub decimal: SyncObject, + pub uuid: SyncObject, } -unsafe impl Sync for StaticObject {} - pub fn import() -> Result<&'static Import> { STATIC_OBJECTS.as_ref().map_err(|e| err!("{}", e)) } @@ -54,7 +37,11 @@ macro_rules! getattr { ($module:expr, $name:expr) => { $module .getattr($name) - .map(|p| pyo3::PyObject::from(p).into()) + .map(|p| { + Object::new(pyo3::PyObject::from(p).into_ptr()) + .unwrap() + .into() + }) .map_err(|_| err!(concat!("couldn't find function `", $name, "`"))) }; } @@ -96,7 +83,7 @@ lazy_static::lazy_static! { let frozenset = getattr!(typing, "FrozenSet")?; let enum_meta = getattr!(enum_, "EnumMeta")?; - let empty_tuple = StaticObject(Object::new_unit()?); + let empty_tuple = SyncObject::new(Object::new_unit()?); let datetime = getattr!(datetime_, "datetime")?; let date = getattr!(datetime_, "date")?; diff --git a/perde-core/src/object.rs b/perde-core/src/object.rs index 59fd63c..d34132f 100644 --- a/perde-core/src/object.rs +++ b/perde-core/src/object.rs @@ -11,6 +11,7 @@ use std::{ ops::{Deref, DerefMut}, os::raw::c_char, ptr::NonNull, + sync::atomic::{AtomicPtr, Ordering}, }; macro_rules! objnew { @@ -805,3 +806,40 @@ impl TupleBuilder { self.0 } } + +#[derive(Debug)] +pub struct SyncObject(AtomicPtr); + +impl SyncObject { + pub fn new(obj: Object) -> Self { + Self(AtomicPtr::new(obj.into_ptr())) + } +} + +impl From for SyncObject { + fn from(obj: Object) -> Self { + Self::new(obj) + } +} + +impl Deref for SyncObject { + type Target = ObjectRef; + + fn deref(&self) -> &Self::Target { + ObjectRef::new(self.0.load(Ordering::Relaxed)).unwrap() + } +} + +impl PartialEq for SyncObject { + fn eq(&self, other: &SyncObject) -> bool { + self.as_ptr() == other.as_ptr() + } +} + +impl Eq for SyncObject {} + +impl Clone for SyncObject { + fn clone(&self) -> Self { + Self::new(self.owned()) + } +} diff --git a/perde-core/src/resolve.rs b/perde-core/src/resolve.rs index 77d4814..671ef61 100644 --- a/perde-core/src/resolve.rs +++ b/perde-core/src/resolve.rs @@ -212,7 +212,7 @@ fn to_dataclass(p: &ObjectRef, attr: &Option>) -> Resu let flatten_members = collect_flatten_members(&members); Ok(Schema::Class(Class::new( - class, + class.into(), name.into(), cattr, members, @@ -259,14 +259,14 @@ fn to_enum(p: &ObjectRef, attr: &Option>) -> Result, - pub default: Option, - pub default_factory: Option, + pub default: Option, + pub default_factory: Option, pub skip: bool, pub skip_serializing: bool, pub skip_deserializing: bool, @@ -121,8 +121,8 @@ impl FieldAttr { Ok(Self::new( field_extract_bool!(attr, "perde_flatten"), field_extract_str!(attr, "perde_rename"), - default, - default_factory, + default.map(|o| o.into()), + default_factory.map(|o| o.into()), field_extract_bool!(attr, "perde_skip"), field_extract_bool!(attr, "perde_skip_serializing"), field_extract_bool!(attr, "perde_skip_deserializing"), @@ -264,7 +264,7 @@ impl Tuple { #[derive(Debug, Clone, new, PartialEq, Eq)] pub struct Enum { pub name: String, - pub object: Object, + pub object: SyncObject, pub attr: EnumAttr, pub variants: Vec, } @@ -281,12 +281,12 @@ pub struct VariantSchema { pub sername: String, pub dename: String, pub attr: VariantAttr, - pub value: Object, + pub value: SyncObject, } #[derive(Debug, Clone, new, PartialEq, Eq)] pub struct Class { - pub ty: Object, + pub ty: SyncObject, pub name: String, pub attr: ClassAttr, pub fields: IndexMap, @@ -436,5 +436,3 @@ lazy_static::lazy_static! { } }; } - -unsafe impl Sync for StaticSchema {} From f3b4508d436d0fa46b345562833be9026a119f50 Mon Sep 17 00:00:00 2001 From: YushiOMOTE Date: Fri, 20 Nov 2020 22:12:20 +0900 Subject: [PATCH 2/9] Add argument error handling and tests --- perde-core/src/args.rs | 4 ++ perde-core/src/error.rs | 77 +++++++++++++++++++----- perde-core/src/lib.rs | 4 +- perde-core/src/methods.rs | 39 ++++++++---- perde-core/src/object.rs | 57 +++++++++++++----- perde-core/src/resolve.rs | 13 ++-- perde-json/src/lib.rs | 4 +- perde-msgpack/src/lib.rs | 4 +- perde-tests/README.md | 18 +++++- perde-tests/tests/test_error.py | 103 ++++++++++++++++++++++++++++++-- perde-tests/tests/util.py | 22 +++---- perde-toml/src/lib.rs | 4 +- perde-yaml/src/lib.rs | 4 +- 13 files changed, 278 insertions(+), 75 deletions(-) diff --git a/perde-core/src/args.rs b/perde-core/src/args.rs index a28d53b..d9dfb89 100644 --- a/perde-core/src/args.rs +++ b/perde-core/src/args.rs @@ -16,6 +16,10 @@ impl<'a> Args<'a> { }) } + pub fn len(&self) -> usize { + self.tuple.len() + } + pub fn arg(&self, index: usize) -> Result<&ObjectRef> { self.tuple.get(index) } diff --git a/perde-core/src/error.rs b/perde-core/src/error.rs index 1e03f14..d1b39bf 100644 --- a/perde-core/src/error.rs +++ b/perde-core/src/error.rs @@ -5,8 +5,9 @@ use std::fmt::{self, Display}; pub type Result = std::result::Result; #[derive(Debug)] -pub struct Error { - msg: String, +pub enum Error { + TypeError(String), + Else(String), } #[macro_export] @@ -16,6 +17,13 @@ macro_rules! err { } } +#[macro_export] +macro_rules! type_err { + ($($t:tt)*) => { + $crate::error::Error::type_error(format!($($t)*)) + } +} + #[macro_export] macro_rules! bail { ($($t:tt)*) => { @@ -23,24 +31,53 @@ macro_rules! bail { } } +#[macro_export] +macro_rules! bail_type_err { + ($($t:tt)*) => { + return Err($crate::type_err!($($t)*)) + } +} + +pub fn raise(msg: U) { + let py = unsafe { Python::assume_gil_acquired() }; + let pyerr = PyErr::new::(msg.to_string()); + pyerr.restore(py); +} + +fn clear() { + if unsafe { !pyo3::ffi::PyErr_Occurred().is_null() } { + unsafe { pyo3::ffi::PyErr_Clear() }; + } +} + impl Error { pub fn new(t: T) -> Self where T: ToString, { - let py = unsafe { Python::assume_gil_acquired() }; - - if PyErr::occurred(py) { - unsafe { pyo3::ffi::PyErr_Clear() }; - } + clear(); + Self::Else(t.to_string()) + } - Self { msg: t.to_string() } + pub fn type_error(t: T) -> Self + where + T: ToString, + { + clear(); + Self::TypeError(t.to_string()) } pub fn restore_as(self) { - let py = unsafe { Python::assume_gil_acquired() }; - let pyerr = PyErr::new::(self.msg); - pyerr.restore(py); + match self { + Error::TypeError(t) => raise::(t), + Error::Else(t) => raise::(t), + } + } + + pub fn message(&self) -> &str { + match self { + Self::TypeError(m) | Self::Else(m) => &m, + } } } @@ -55,7 +92,13 @@ where impl Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}", self.msg) + write!( + f, + "{}", + match self { + Error::TypeError(s) | Error::Else(s) => s, + } + ) } } @@ -96,8 +139,14 @@ impl Convert for Result { where C: ToString, { - self.map_err(|e| Error { - msg: format!("{}: {}", context.to_string(), e), + self.map_err(|mut e| { + let new_msg = format!("{}: {}", context.to_string(), e); + match &mut e { + Error::TypeError(m) | Error::Else(m) => { + *m = new_msg; + } + }; + e }) } } diff --git a/perde-core/src/lib.rs b/perde-core/src/lib.rs index d6f1150..9d6f38d 100644 --- a/perde-core/src/lib.rs +++ b/perde-core/src/lib.rs @@ -17,10 +17,10 @@ pub mod object; pub mod prelude { pub use crate::args::{Args, FastArgs}; - pub use crate::error::{Convert, Error, Result}; + pub use crate::error::{raise, Convert, Error, Result}; pub use crate::object::{Object, ObjectRef}; pub use crate::schema::{Schema, WithSchema}; pub use crate::{ - bail, exception, impl_default_methods, method_fastcall, method_varargs, module, + bail, exception, impl_default_methods, method_fastcall, method_varargs, module, type_err, }; } diff --git a/perde-core/src/methods.rs b/perde-core/src/methods.rs index 3507376..8a13b2c 100644 --- a/perde-core/src/methods.rs +++ b/perde-core/src/methods.rs @@ -105,6 +105,14 @@ macro_rules! impl_default_methods { let inner = || { let args = Args::new(args)?; + + if args.len() != 2 { + return Err($crate::type_err!( + "loads_as() requires 2 positional arguments but got {}", + args.len() + )); + } + $loads_as(args.arg(0)?.resolve(None)?, args.arg(1)?) }; @@ -119,15 +127,16 @@ macro_rules! impl_default_methods { pub extern "C" fn dumps( _self: *mut pyo3::ffi::PyObject, - args: *const *mut pyo3::ffi::PyObject, - nargs: pyo3::ffi::Py_ssize_t, - kwnames: *mut pyo3::ffi::PyObject, + args: *mut pyo3::ffi::PyObject, ) -> *mut pyo3::ffi::PyObject { let inner = || { - let args = FastArgs::new(args, nargs, kwnames); + let args = Args::new(args)?; - if args.num_args() != 1 { - bail!("dumps() requires 1 positional argument"); + if args.len() != 1 { + return Err($crate::type_err!( + "dumps() requires 1 positional argument but got {}", + args.len() + )); } let obj = args.arg(0)?; @@ -147,12 +156,18 @@ macro_rules! impl_default_methods { pub extern "C" fn loads( _self: *mut pyo3::ffi::PyObject, - args: *const *mut pyo3::ffi::PyObject, - nargs: pyo3::ffi::Py_ssize_t, - kwnames: *mut pyo3::ffi::PyObject, + args: *mut pyo3::ffi::PyObject, ) -> *mut pyo3::ffi::PyObject { let inner = || { - let args = FastArgs::new(args, nargs, kwnames); + let args = Args::new(args)?; + + if args.len() != 1 { + return Err($crate::type_err!( + "loads() requires 1 positional argument but got {}", + args.len() + )); + } + let obj = args.arg(0)?; $loads(obj) }; @@ -169,8 +184,8 @@ macro_rules! impl_default_methods { module!( $module_name, exception!($exception_type), - method_fastcall!(loads, ""), - method_fastcall!(dumps, ""), + method_varargs!(loads, ""), + method_varargs!(dumps, ""), method_varargs!(loads_as, "") ); }; diff --git a/perde-core/src/object.rs b/perde-core/src/object.rs index d34132f..b923a85 100644 --- a/perde-core/src/object.rs +++ b/perde-core/src/object.rs @@ -8,6 +8,7 @@ use crate::{ use pyo3::ffi::*; use std::{ collections::HashMap, + fmt::{self, Debug}, ops::{Deref, DerefMut}, os::raw::c_char, ptr::NonNull, @@ -51,7 +52,6 @@ macro_rules! is_type_opt { }; } -#[derive(Debug)] pub struct ObjectRef; impl ObjectRef { @@ -128,14 +128,14 @@ impl ObjectRef { } else if self.is(unsafe { Py_False() }) { Ok(false) } else { - bail!("object is not boolean type") + bail_type_err!("expected `bool` got `{}`: {:?}", self.typename(), self) } } pub fn as_i64(&self) -> Result { let p = unsafe { PyLong_AsLongLong(self.as_ptr()) }; if unsafe { !PyErr_Occurred().is_null() } { - bail!("object is not integer type") + bail_type_err!("expected `int` got `{}`: {:?}", self.typename(), self) } else { Ok(p) } @@ -144,7 +144,7 @@ impl ObjectRef { pub fn as_u64(&self) -> Result { let p = unsafe { PyLong_AsLongLong(self.as_ptr()) }; if unsafe { !PyErr_Occurred().is_null() } { - bail!("object is not integer type") + bail_type_err!("expected `int` got `{}`: {:?}", self.typename(), self) } else { Ok(p as u64) } @@ -153,7 +153,7 @@ impl ObjectRef { pub fn as_f64(&self) -> Result { let p = unsafe { PyFloat_AsDouble(self.as_ptr()) }; if unsafe { !PyErr_Occurred().is_null() } { - bail!("object is not double float") + bail_type_err!("expected `float` got `{}`: {:?}", self.typename(), self) } else { Ok(p) } @@ -164,7 +164,7 @@ impl ObjectRef { let p = unsafe { PyUnicode_AsUTF8AndSize(self.as_ptr(), &mut len) }; if p.is_null() { - bail!("object is not a string") + bail_type_err!("expected `str` got `{}`: {:?}", self.typename(), self) } else { unsafe { let slice = std::slice::from_raw_parts(p as *const u8, len as usize); @@ -179,7 +179,7 @@ impl ObjectRef { let p = unsafe { PyBytes_AsStringAndSize(self.as_ptr(), &mut buf, &mut len) }; if p == -1 { - bail!("object is not bytes") + bail_type_err!("expected `bytes` got `{}`: {:?}", self.typename(), self) } else { unsafe { let slice = std::slice::from_raw_parts(buf as *const u8, len as usize); @@ -193,7 +193,7 @@ impl ObjectRef { let len = unsafe { PyByteArray_Size(self.as_ptr()) }; if p.is_null() { - bail!("object is not bytearray") + bail_type_err!("expected `bytearray` got `{}`: {:?}", self.typename(), self) } else { unsafe { let slice = std::slice::from_raw_parts(p as *const u8, len as usize); @@ -215,8 +215,7 @@ impl ObjectRef { } pub fn to_str(&self) -> Result { - let strtype = ObjectRef::new(cast!(PyUnicode_Type))?; - strtype.call1(self.owned()) + Object::new(unsafe { PyObject_Str(self.as_ptr()) }) } pub fn is(&self, p: *mut PyObject) -> bool { @@ -227,8 +226,12 @@ impl ObjectRef { self.is(unsafe { Py_None() }) } + pub fn is_type(&self) -> bool { + unsafe { (*self.as_ptr()).ob_type == &mut PyType_Type } + } + pub fn is_none_type(&self) -> bool { - self.is(unsafe { (*Py_None()).ob_type as *mut PyObject }) + self.is(ptr_cast!((*Py_None()).ob_type)) } pub fn is_bool(&self) -> bool { @@ -321,12 +324,21 @@ impl ObjectRef { pub fn name(&self) -> &str { unsafe { - std::ffi::CStr::from_ptr((*(self.as_ptr() as *mut PyTypeObject)).tp_name) - .to_str() - .unwrap_or("") + if self.is_type() { + let p = (*(self.as_ptr() as *mut PyTypeObject)).tp_name; + std::ffi::CStr::from_ptr(p) + .to_str() + .unwrap_or("__unknown__") + } else { + "__unknown__" + } } } + pub fn typename(&self) -> &str { + self.get_type().map(|t| t.name()).unwrap_or("__unknown__") + } + pub fn as_ptr(&self) -> *mut PyObject { &*self as *const Self as *mut Self as *mut PyObject } @@ -377,6 +389,15 @@ impl ObjectRef { } } +impl Debug for ObjectRef { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + self.to_str() + .ok() + .and_then(|o| o.as_str().ok().map(|s| write!(f, "{}", s))) + .unwrap_or_else(|| write!(f, "")) + } +} + #[derive(Debug)] pub struct ObjectIter(Object); @@ -476,7 +497,7 @@ impl<'a> Iterator for DictIter<'a> { } } -#[derive(Debug, PartialEq, Eq)] +#[derive(PartialEq, Eq)] pub struct Object(NonNull); impl Object { @@ -660,6 +681,12 @@ impl Drop for Object { } } +impl Debug for Object { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + self.deref().fmt(f) + } +} + #[derive(Debug, Clone)] pub struct SetRef<'a>(&'a ObjectRef); diff --git a/perde-core/src/resolve.rs b/perde-core/src/resolve.rs index 671ef61..b4b12e5 100644 --- a/perde-core/src/resolve.rs +++ b/perde-core/src/resolve.rs @@ -117,12 +117,11 @@ pub fn resolve_schema<'a>( } else if p.is_enum() { to_enum(p, &attr)? } else { - bail!( - "unsupported type `{}`", - p.get_attr(&ATTR_TYPENAME) - .and_then(|o| { Ok(o.as_str()?.to_string()) }) - .unwrap_or("".into()) - ); + if !p.is_type() { + bail_type_err!("invalid argument: `{:?}` is not a type", p) + } else { + bail_type_err!("unsupported type `{:?}`", p) + } }; p.set_capsule(&SCHEMA_CACHE, s) @@ -354,7 +353,7 @@ fn to_generic(p: &ObjectRef) -> Result { } else if origin.is_frozen_set() { to_frozen_set(&args)? } else { - bail!("unsupported generic type"); + bail_type_err!("unsupported generic type"); }; Ok(s) diff --git a/perde-json/src/lib.rs b/perde-json/src/lib.rs index 948e8fe..101cd87 100644 --- a/perde-json/src/lib.rs +++ b/perde-json/src/lib.rs @@ -2,13 +2,13 @@ use perde_core::prelude::*; use serde::de::DeserializeSeed; fn loads_as_(schema: &Schema, object: &ObjectRef) -> Result { - let s = object.as_str()?; + let s = object.as_str().context("invalid argument")?; let mut de = serde_json::Deserializer::from_str(&s); Ok(schema.deserialize(&mut de)?) } fn loads_(object: &ObjectRef) -> Result { - let s = object.as_str()?; + let s = object.as_str().context("invalid argument")?; Ok(serde_json::from_str(s)?) } diff --git a/perde-msgpack/src/lib.rs b/perde-msgpack/src/lib.rs index 134678f..eebd8da 100644 --- a/perde-msgpack/src/lib.rs +++ b/perde-msgpack/src/lib.rs @@ -2,13 +2,13 @@ use perde_core::prelude::*; use serde::de::DeserializeSeed; fn loads_as_(schema: &Schema, object: &ObjectRef) -> Result { - let b = object.as_bytes()?; + let b = object.as_bytes().context("invalid argument")?; let mut de = rmp_serde::Deserializer::from_read_ref(&b); Ok(schema.deserialize(&mut de)?) } fn loads_(object: &ObjectRef) -> Result { - let b = object.as_bytes()?; + let b = object.as_bytes().context("invalid argument")?; Ok(rmp_serde::from_slice(&b)?) } diff --git a/perde-tests/README.md b/perde-tests/README.md index 5bb2fb6..264f42f 100644 --- a/perde-tests/README.md +++ b/perde-tests/README.md @@ -2,7 +2,9 @@ ## How this works? - +* `gen/datagen` parses Rust code (serde derived structs) in the comments in Python unit tests. It generates the Rust program `gen/datagen`, which dumps serialized data in each format. +* `gen/datagen` generates serialized data in each format. The output is stored in `data`. +* The Python unit tests uses the serialized data for testing. ## Types @@ -77,6 +79,20 @@ * [x] `perde_skip_deserialzing` * [x] `perde_other` +## Error testing + +* Arguments error + * [ ] `dumps` + * [ ] `loads` + * [ ] `loads_as` +* Serialization error + * [ ] Parsing invalid objects. + * [ ] Serializing skipped enum variant. + * [ ] Serializing flatten struct in msgpack. +* Deserialization error + * [ ] Parsing broken data. + * [ ] Parsing valid format but doesn't match the specified type. + ## Known issues / Constraints * Flatten for msgpack doesn't work due to [the issue](https://github.com/3Hren/msgpack-rust/issues/196) in `rmp-serde`. diff --git a/perde-tests/tests/test_error.py b/perde-tests/tests/test_error.py index bae7368..054a240 100644 --- a/perde-tests/tests/test_error.py +++ b/perde-tests/tests/test_error.py @@ -1,12 +1,103 @@ import pytest -from util import FORMATS +from util import FORMATS, FORMATS_EXCEPT, FORMATS_ONLY @pytest.mark.parametrize("m", FORMATS) -def test_error(m): - class Plain: +def test_error_no_args(m): + with pytest.raises(TypeError) as e: + m.dumps() + assert e.value.args[ + 0] == "dumps() requires 1 positional argument but got 0" + + with pytest.raises(TypeError) as e: + m.loads() + assert e.value.args[ + 0] == "loads() requires 1 positional argument but got 0" + + with pytest.raises(TypeError) as e: + m.loads_as() + assert e.value.args[ + 0] == "loads_as() requires 2 positional arguments but got 0" + + +@pytest.mark.parametrize("m", FORMATS) +def test_error_keyword_args(m): + with pytest.raises(TypeError) as e: + m.dumps(kw='a') + assert e.value.args[0] == "dumps() takes no keyword arguments" + + with pytest.raises(TypeError) as e: + m.loads(kw='a') + assert e.value.args[0] == "loads() takes no keyword arguments" + + with pytest.raises(TypeError) as e: + m.loads_as(kw='a') + assert e.value.args[0] == "loads_as() takes no keyword arguments" + + +@pytest.mark.parametrize("m", FORMATS) +def test_error_dumps_wrong_number_of_args(m): + with pytest.raises(TypeError) as e: + m.dumps('a', 'b') + assert e.value.args[ + 0] == "dumps() requires 1 positional argument but got 2" + + with pytest.raises(TypeError) as e: + m.loads('a', 'b') + assert e.value.args[ + 0] == "loads() requires 1 positional argument but got 2" + + with pytest.raises(TypeError) as e: + m.loads_as('a') + assert e.value.args[ + 0] == "loads_as() requires 2 positional arguments but got 1" + with pytest.raises(TypeError) as e: + m.loads_as('a', 'b', 'c') + assert e.value.args[ + 0] == "loads_as() requires 2 positional arguments but got 3" + + +@pytest.mark.parametrize("m", FORMATS_EXCEPT("msgpack")) +def test_error_loads_invalid_argument_type(m): + with pytest.raises(TypeError) as e: + m.loads(b"a") + assert e.value.args[ + 0] == "invalid argument: expected `str` got `bytes`: b'a'" + with pytest.raises(TypeError) as e: + m.loads(any) + assert e.value.args[0] == ("invalid argument: expected `str` " + "got `builtin_function_or_method`:" + " ") + + with pytest.raises(TypeError) as e: + m.loads_as(str, b"a") + assert e.value.args[ + 0] == "invalid argument: expected `str` got `bytes`: b'a'" + + with pytest.raises(TypeError) as e: + m.loads_as("b", "a") + assert e.value.args[0] == "invalid argument: `b` is not a type" + + with pytest.raises(TypeError) as e: + m.loads_as(any, "a") + assert e.value.args[ + 0] == "invalid argument: `` is not a type" + + +@pytest.mark.parametrize("m", FORMATS_ONLY("msgpack")) +def test_error_loads_invalid_argument_type_msgpack(m): + with pytest.raises(TypeError) as e: + m.loads("a") + assert e.value.args[0] == "invalid argument: expected `bytes` got `str`: a" + + +@pytest.mark.parametrize("m", FORMATS) +def test_error_unsupported_type(m): + class Abc: pass - with pytest.raises(Exception) as e: - m.dumps(Plain()) - print(f'{e}') + with pytest.raises(TypeError) as e: + m.dumps(Abc()) + assert e.value.args[0] == ( + "unsupported type " + "`.Abc'>`") diff --git a/perde-tests/tests/util.py b/perde-tests/tests/util.py index da3d8f6..fb8628e 100644 --- a/perde-tests/tests/util.py +++ b/perde-tests/tests/util.py @@ -18,15 +18,16 @@ class Format: fmtname: str package: Any argtype: Any + errtype: Any = None - def dumps(self, v): - return self.package.dumps(v) + def dumps(self, *args, **kwargs): + return self.package.dumps(*args, **kwargs) - def loads(self, v): - return self.package.loads(v) + def loads(self, *args, **kwargs): + return self.package.loads(*args, **kwargs) - def loads_as(self, t, v): - return self.package.loads_as(t, v) + def loads_as(self, *args, **kwargs): + return self.package.loads_as(*args, **kwargs) def repack(self, v): print(f'repacking {v}...') @@ -222,10 +223,11 @@ def mark(params): _FORMATS = [ - Format("json", "json", perde_json, str), - Format("yaml", "yaml", perde_yaml, str), - Format("msgpack", "msgpack", perde_msgpack, bytes), - Format("toml", "toml", perde_toml, str) + Format("json", "json", perde_json, str, perde_json.JsonError), + Format("yaml", "yaml", perde_yaml, str, perde_yaml.YamlError), + Format("msgpack", "msgpack", perde_msgpack, bytes, + perde_msgpack.MsgpackError), + Format("toml", "toml", perde_toml, str, perde_toml.TomlError) ] FORMATS = mark(_FORMATS) diff --git a/perde-toml/src/lib.rs b/perde-toml/src/lib.rs index 4212b16..e8a29bc 100644 --- a/perde-toml/src/lib.rs +++ b/perde-toml/src/lib.rs @@ -2,13 +2,13 @@ use perde_core::prelude::*; use serde::de::DeserializeSeed; fn loads_as_(schema: &Schema, object: &ObjectRef) -> Result { - let buf = object.as_str()?; + let buf = object.as_str().context("invalid argument")?; let mut deserializer = toml::de::Deserializer::new(&buf); Ok(schema.deserialize(&mut deserializer)?) } fn loads_(object: &ObjectRef) -> Result { - let s = object.as_str()?; + let s = object.as_str().context("invalid argument")?; Ok(toml::from_str(&s)?) } diff --git a/perde-yaml/src/lib.rs b/perde-yaml/src/lib.rs index e5a6905..278d059 100644 --- a/perde-yaml/src/lib.rs +++ b/perde-yaml/src/lib.rs @@ -1,12 +1,12 @@ use perde_core::prelude::*; fn loads_as_(schema: &Schema, object: &ObjectRef) -> Result { - let s = object.as_str()?; + let s = object.as_str().context("invalid argument")?; Ok(serde_yaml::seed::from_str_seed(s, schema)?) } fn loads_(object: &ObjectRef) -> Result { - let s = object.as_str()?; + let s = object.as_str().context("invalid argument")?; Ok(serde_yaml::from_str(s)?) } From 38a59c2f068441d018bd03f8ff9a3b1129eda528 Mon Sep 17 00:00:00 2001 From: YushiOMOTE Date: Sat, 21 Nov 2020 06:52:42 +0900 Subject: [PATCH 3/9] Add error handling for perde attributes --- perde-core/src/error.rs | 43 +++++++++--- perde-core/src/lib.rs | 3 +- perde-core/src/methods.rs | 7 +- perde-core/src/resolve.rs | 4 +- perde-core/src/schema.rs | 18 +++-- perde-tests/tests/test_error.py | 117 +++++++++++++++++++++++++++++++- perde/perde/__init__.py | 5 +- perde/src/lib.rs | 13 ++-- 8 files changed, 180 insertions(+), 30 deletions(-) diff --git a/perde-core/src/error.rs b/perde-core/src/error.rs index d1b39bf..b9c6f1d 100644 --- a/perde-core/src/error.rs +++ b/perde-core/src/error.rs @@ -7,6 +7,7 @@ pub type Result = std::result::Result; #[derive(Debug)] pub enum Error { TypeError(String), + ValueError(String), Else(String), } @@ -24,6 +25,13 @@ macro_rules! type_err { } } +#[macro_export] +macro_rules! value_err { + ($($t:tt)*) => { + $crate::error::Error::value_error(format!($($t)*)) + } +} + #[macro_export] macro_rules! bail { ($($t:tt)*) => { @@ -38,6 +46,13 @@ macro_rules! bail_type_err { } } +#[macro_export] +macro_rules! bail_value_err { + ($($t:tt)*) => { + return Err($crate::value_err!($($t)*)) + } +} + pub fn raise(msg: U) { let py = unsafe { Python::assume_gil_acquired() }; let pyerr = PyErr::new::(msg.to_string()); @@ -67,16 +82,33 @@ impl Error { Self::TypeError(t.to_string()) } + pub fn value_error(t: T) -> Self + where + T: ToString, + { + clear(); + Self::ValueError(t.to_string()) + } + pub fn restore_as(self) { match self { Error::TypeError(t) => raise::(t), + Error::ValueError(t) => raise::(t), Error::Else(t) => raise::(t), } } + pub fn set_message(&mut self, message: String) { + match self { + Error::TypeError(m) | Error::ValueError(m) | Error::Else(m) => { + *m = message; + } + } + } + pub fn message(&self) -> &str { match self { - Self::TypeError(m) | Self::Else(m) => &m, + Self::TypeError(m) | Self::ValueError(m) | Self::Else(m) => &m, } } } @@ -96,7 +128,7 @@ impl Display for Error { f, "{}", match self { - Error::TypeError(s) | Error::Else(s) => s, + Error::TypeError(m) | Error::ValueError(m) | Error::Else(m) => m, } ) } @@ -140,12 +172,7 @@ impl Convert for Result { C: ToString, { self.map_err(|mut e| { - let new_msg = format!("{}: {}", context.to_string(), e); - match &mut e { - Error::TypeError(m) | Error::Else(m) => { - *m = new_msg; - } - }; + e.set_message(format!("{}: {}", context.to_string(), e)); e }) } diff --git a/perde-core/src/lib.rs b/perde-core/src/lib.rs index 9d6f38d..548a21f 100644 --- a/perde-core/src/lib.rs +++ b/perde-core/src/lib.rs @@ -21,6 +21,7 @@ pub mod prelude { pub use crate::object::{Object, ObjectRef}; pub use crate::schema::{Schema, WithSchema}; pub use crate::{ - bail, exception, impl_default_methods, method_fastcall, method_varargs, module, type_err, + bail, bail_type_err, bail_value_err, err, exception, impl_default_methods, method_fastcall, + method_varargs, module, type_err, value_err, }; } diff --git a/perde-core/src/methods.rs b/perde-core/src/methods.rs index 8a13b2c..fa29e7f 100644 --- a/perde-core/src/methods.rs +++ b/perde-core/src/methods.rs @@ -113,7 +113,10 @@ macro_rules! impl_default_methods { )); } - $loads_as(args.arg(0)?.resolve(None)?, args.arg(1)?) + $loads_as( + args.arg(0)?.resolve(None).context("invalid argument")?, + args.arg(1)?, + ) }; match inner() { @@ -140,7 +143,7 @@ macro_rules! impl_default_methods { } let obj = args.arg(0)?; - let resolved = obj.resolved_object()?; + let resolved = obj.resolved_object().context("invalid argument")?; $dumps(resolved) }; diff --git a/perde-core/src/resolve.rs b/perde-core/src/resolve.rs index b4b12e5..f7e9ccd 100644 --- a/perde-core/src/resolve.rs +++ b/perde-core/src/resolve.rs @@ -118,7 +118,7 @@ pub fn resolve_schema<'a>( to_enum(p, &attr)? } else { if !p.is_type() { - bail_type_err!("invalid argument: `{:?}` is not a type", p) + bail_type_err!("`{:?}` is not a type", p) } else { bail_type_err!("unsupported type `{:?}`", p) } @@ -128,7 +128,7 @@ pub fn resolve_schema<'a>( } } -pub fn to_schema(p: &ObjectRef) -> Result { +fn to_schema(p: &ObjectRef) -> Result { resolve_schema(p, None).map(|s| s.clone()) } diff --git a/perde-core/src/schema.rs b/perde-core/src/schema.rs index d1db7d8..7fbb4a2 100644 --- a/perde-core/src/schema.rs +++ b/perde-core/src/schema.rs @@ -32,7 +32,7 @@ impl FromStr for StrCase { "SCREAMING_SNAKE_CASE" => Ok(StrCase::ScreamingSnake), "kebab-case" => Ok(StrCase::Kebab), "SCREAMING-KEBAB-CASE" => Ok(StrCase::ScreamingKebab), - c => bail!("Unsupported string case: {}", c), + c => bail_value_err!("invalid string case: `{}`", c), } } } @@ -43,7 +43,7 @@ macro_rules! field_extract_bool { .as_ref() .and_then(|map| map.get($field).ok().map(|v| v.as_bool())) .transpose() - .context(format!("expected `bool` in attribute `{}`", $field))? + .context(format!("invalid attribute `{}`", $field))? .unwrap_or(false) }; } @@ -58,7 +58,7 @@ macro_rules! field_extract_str { .map(|v| v.as_str().map(|v| v.to_string())) }) .transpose() - .context(format!("expected `str` in attribute `{}`", $field))? + .context(format!("invalid attribute `{}`", $field))? }; } @@ -69,13 +69,11 @@ macro_rules! extract_stringcase { .and_then(|map| { map.get($field).map(|v| { let s = v.as_str()?; - s.parse().context(format!( - "invalid string case `{}` in attribute `{}`", - s, $field - )) + s.parse() }) }) - .transpose()? + .transpose() + .context(format!("invalid attribute `{}`", $field))? }; } @@ -85,7 +83,7 @@ macro_rules! extract_bool { .as_ref() .and_then(|map| map.get($field).map(|v| v.as_bool())) .transpose() - .context(format!("expected `bool` in attribute `{}`", $field))? + .context(format!("invalid attribute `{}`", $field))? .unwrap_or(false) }; } @@ -96,7 +94,7 @@ macro_rules! extract_str { .as_ref() .and_then(|map| map.get($field).map(|v| v.as_str().map(|v| v.to_string()))) .transpose() - .context(format!("expected `str` in attribute `{}`", $field))? + .context(format!("invalid attribute `{}`", $field))? }; } diff --git a/perde-tests/tests/test_error.py b/perde-tests/tests/test_error.py index 054a240..7cab069 100644 --- a/perde-tests/tests/test_error.py +++ b/perde-tests/tests/test_error.py @@ -1,7 +1,32 @@ +import enum +from dataclasses import dataclass, field import pytest +import perde from util import FORMATS, FORMATS_EXCEPT, FORMATS_ONLY +def test_error_perde(): + with pytest.raises(TypeError) as e: + + @perde.attr + class X: + pass + + assert e.value.args[0] == ( + "unsupported type " + "`.X'>`") + + with pytest.raises(TypeError) as e: + + @perde.attr() + class Y: + pass + + assert e.value.args[0] == ( + "unsupported type " + "`.Y'>`") + + @pytest.mark.parametrize("m", FORMATS) def test_error_no_args(m): with pytest.raises(TypeError) as e: @@ -91,6 +116,96 @@ def test_error_loads_invalid_argument_type_msgpack(m): assert e.value.args[0] == "invalid argument: expected `bytes` got `str`: a" +@pytest.mark.parametrize("m", FORMATS) +def test_error_invalid_class_attribute(m): + for attr, ty in [("rename_all", "str"), ("rename_all_serialize", "str"), + ("rename_all_deserialize", "str"), ("rename", "str"), + ("deny_unknown_fields", "bool"), ("default", "bool")]: + with pytest.raises(TypeError) as e: + + @perde.attr(**{attr: 3}) + @dataclass + class A: + pass + + assert e.value.args[ + 0] == f"invalid attribute `{attr}`: expected `{ty}` got `int`: 3" + + for attr in [ + "rename_all", "rename_all_serialize", "rename_all_deserialize" + ]: + with pytest.raises(ValueError) as e: + + @perde.attr(**{attr: "hage"}) + @dataclass + class B: + pass + + assert e.value.args[ + 0] == f"invalid attribute `{attr}`: invalid string case: `hage`" + + +@pytest.mark.parametrize("m", FORMATS) +def test_error_invalid_enum_attribute(m): + for attr, ty in [("rename_all", "str"), ("rename_all_serialize", "str"), + ("rename_all_deserialize", "str"), ("rename", "str"), + ("as_value", "bool")]: + with pytest.raises(TypeError) as e: + + @perde.attr(**{attr: 3}) + class A(enum.Enum): + X = 10 + + assert e.value.args[ + 0] == f"invalid attribute `{attr}`: expected `{ty}` got `int`: 3" + + for attr in [ + "rename_all", "rename_all_serialize", "rename_all_deserialize" + ]: + with pytest.raises(ValueError) as e: + + @perde.attr(**{attr: "hage"}) + class B(enum.Enum): + X = 10 + + assert e.value.args[ + 0] == f"invalid attribute `{attr}`: invalid string case: `hage`" + + +@pytest.mark.parametrize("m", FORMATS) +def test_error_invalid_class_field_attribute(m): + for attr, ty in [("perde_flatten", "bool"), ("perde_rename", "str"), + ("perde_skip", "bool"), + ("perde_skip_serializing", "bool"), + ("perde_skip_deserializing", "bool"), + ("perde_default", "bool")]: + with pytest.raises(TypeError) as e: + + @perde.attr() + @dataclass + class A: + foo: int = field(metadata={attr: 3}) + + assert e.value.args[ + 0] == f"invalid attribute `{attr}`: expected `{ty}` got `int`: 3" + + +@pytest.mark.parametrize("m", FORMATS) +def test_error_invalid_enum_field_attribute(m): + for attr, ty in [("perde_rename", "str"), ("perde_skip", "bool"), + ("perde_skip_serializing", "bool"), + ("perde_skip_deserializing", "bool"), + ("perde_other", "bool")]: + with pytest.raises(TypeError) as e: + + @perde.attr() + class A(perde.Enum): + X = 100, {attr: 3} + + assert e.value.args[ + 0] == f"invalid attribute `{attr}`: expected `{ty}` got `int`: 3" + + @pytest.mark.parametrize("m", FORMATS) def test_error_unsupported_type(m): class Abc: @@ -99,5 +214,5 @@ class Abc: with pytest.raises(TypeError) as e: m.dumps(Abc()) assert e.value.args[0] == ( - "unsupported type " + "invalid argument: unsupported type " "`.Abc'>`") diff --git a/perde/perde/__init__.py b/perde/perde/__init__.py index 1c91168..8a1ed88 100644 --- a/perde/perde/__init__.py +++ b/perde/perde/__init__.py @@ -2,7 +2,10 @@ import enum -def attr(**kwargs): +def attr(*args, **kwargs): + if args: + return resolve(args[0]) + def func(ty): resolve(ty, **kwargs) return ty diff --git a/perde/src/lib.rs b/perde/src/lib.rs index 4ba0f50..67be162 100644 --- a/perde/src/lib.rs +++ b/perde/src/lib.rs @@ -11,15 +11,18 @@ pub extern "C" fn resolve( kwnames: *mut pyo3::ffi::PyObject, ) -> *mut pyo3::ffi::PyObject { let inner = || { - let fargs = FastArgs::new(args, nargs, kwnames); + let args = FastArgs::new(args, nargs, kwnames); - if fargs.num_args() != 1 { - bail!("resolve() requires 1 positional argument"); + if args.num_args() != 1 { + bail_type_err!( + "resolve() requires 1 positional argument but got {}", + args.num_args() + ); } - let typeobj = fargs.arg(0)?; + let typeobj = args.arg(0)?; - let attr = if let Some(iter) = fargs.iter_kwargs()? { + let attr = if let Some(iter) = args.iter_kwargs()? { let mut attr = HashMap::new(); for res in iter { let (key, value) = res?; From 63df2e4920a44cc26849da59a2e6bc2f78d9442a Mon Sep 17 00:00:00 2001 From: YushiOMOTE Date: Sat, 21 Nov 2020 07:00:21 +0900 Subject: [PATCH 4/9] Refactor logic to get default/default_factory --- perde-core/src/object.rs | 2 +- perde-core/src/resolve.rs | 23 +++++++---------------- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/perde-core/src/object.rs b/perde-core/src/object.rs index b923a85..7db76b8 100644 --- a/perde-core/src/object.rs +++ b/perde-core/src/object.rs @@ -632,7 +632,7 @@ impl Object { ptr as *mut PyObject } - pub fn none_as_optional(self) -> Option { + pub fn into_opt(self) -> Option { if self.is_none() { None } else { diff --git a/perde-core/src/resolve.rs b/perde-core/src/resolve.rs index f7e9ccd..f29b202 100644 --- a/perde-core/src/resolve.rs +++ b/perde-core/src/resolve.rs @@ -142,29 +142,20 @@ fn to_dataclass(p: &ObjectRef, attr: &Option>) -> Resu let mut ser_field_len = 0; let mut flatten_dict = None; + let missing = &import()?.missing; + for (i, field) in fields.enumerate() { let name = field.get_attr(&ATTR_NAME)?; - let ty = field.get_attr(&ATTR_TYPE)?; let default = field .get_attr(&ATTR_DEFAULT)? - .none_as_optional() - .filter(|o| { - import() - .ok() - .filter(|so| !o.is(so.missing.as_ptr())) - .is_some() - }); + .into_opt() + .filter(|o| !o.is(missing.as_ptr())); let default_factory = field .get_attr(&ATTR_DEFAULT_FACTORY)? - .none_as_optional() - .filter(|o| { - import() - .ok() - .filter(|so| !o.is(so.missing.as_ptr())) - .is_some() - }); - let metadata = field.get_attr(&ATTR_METADATA)?.none_as_optional(); + .into_opt() + .filter(|o| !o.is(missing.as_ptr())); + let metadata = field.get_attr(&ATTR_METADATA)?.into_opt(); let fattr = FieldAttr::parse(metadata, default, default_factory)?; From 485f92053568a2f58f7052daae0312be8eee7dd8 Mon Sep 17 00:00:00 2001 From: YushiOMOTE Date: Sat, 21 Nov 2020 07:16:08 +0900 Subject: [PATCH 5/9] Clean up error messages around generic type inspection --- perde-core/src/resolve.rs | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/perde-core/src/resolve.rs b/perde-core/src/resolve.rs index f29b202..3e695b6 100644 --- a/perde-core/src/resolve.rs +++ b/perde-core/src/resolve.rs @@ -1,4 +1,6 @@ -use crate::{attr::AttrStr, error::Result, import::import, object::ObjectRef, schema::*}; +use crate::{ + attr::AttrStr, error::Convert, error::Result, import::import, object::ObjectRef, schema::*, +}; use indexmap::IndexMap; use std::collections::HashMap; @@ -284,7 +286,7 @@ fn to_tuple(args: &ObjectRef) -> Result { let mut args = args.get_tuple_iter()?; if args.len() == 1 { - let p = args.next().ok_or(err!("missing tuple element"))?; + let p = args.next().ok_or(type_err!("cannot get element type"))?; if p.is(import()?.empty_tuple.as_ptr()) { return Ok(Schema::Tuple(Tuple::new(vec![]))); } @@ -304,26 +306,26 @@ fn to_tuple(args: &ObjectRef) -> Result { fn to_dict(args: &ObjectRef) -> Result { let mut args = args.get_tuple_iter()?; - let key = to_schema(args.next().ok_or(err!("missing key type in dict"))?)?; - let value = to_schema(args.next().ok_or(err!("missing value type in dict"))?)?; + let key = to_schema(args.next().ok_or(type_err!("cannot get key type"))?)?; + let value = to_schema(args.next().ok_or(type_err!("cannot get value type"))?)?; Ok(Schema::Dict(Dict::new(Box::new(key), Box::new(value)))) } fn to_list(args: &ObjectRef) -> Result { let mut args = args.get_tuple_iter()?; - let value = to_schema(args.next().ok_or(err!("missing value type in list"))?)?; + let value = to_schema(args.next().ok_or(type_err!("cannot get element type"))?)?; Ok(Schema::List(List::new(Box::new(value)))) } fn to_set(args: &ObjectRef) -> Result { let mut args = args.get_tuple_iter()?; - let value = to_schema(args.next().ok_or(err!("missing value type in set"))?)?; + let value = to_schema(args.next().ok_or(type_err!("cannot get element type"))?)?; Ok(Schema::Set(Set::new(Box::new(value)))) } fn to_frozen_set(args: &ObjectRef) -> Result { let mut args = args.get_tuple_iter()?; - let value = to_schema(args.next().ok_or(err!("missing value type in frozenset"))?)?; + let value = to_schema(args.next().ok_or(type_err!("cannot get element type"))?)?; Ok(Schema::FrozenSet(FrozenSet::new(Box::new(value)))) } @@ -332,20 +334,20 @@ fn to_generic(p: &ObjectRef) -> Result { let args = p.get_attr(&ATTR_ARGS)?; let s = if origin.is(import()?.union.as_ptr()) { - to_union(&args)? + to_union(&args) } else if origin.is_tuple() { - to_tuple(&args)? + to_tuple(&args) } else if origin.is_dict() { - to_dict(&args)? + to_dict(&args) } else if origin.is_set() { - to_set(&args)? + to_set(&args) } else if origin.is_list() { - to_list(&args)? + to_list(&args) } else if origin.is_frozen_set() { - to_frozen_set(&args)? + to_frozen_set(&args) } else { - bail_type_err!("unsupported generic type"); + bail_type_err!("unsupported generic type: {:?}", p); }; - Ok(s) + s.context(format!("cannot get generic type information: `{:?}`", p)) } From de612049c62a6d7f593185ecb2d0c0378d517e15 Mon Sep 17 00:00:00 2001 From: YushiOMOTE Date: Sat, 21 Nov 2020 07:25:22 +0900 Subject: [PATCH 6/9] Prefer explicit GIL acquisition to unsafe --- perde-core/src/error.rs | 3 ++- perde-core/src/import.rs | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/perde-core/src/error.rs b/perde-core/src/error.rs index b9c6f1d..f680996 100644 --- a/perde-core/src/error.rs +++ b/perde-core/src/error.rs @@ -54,7 +54,8 @@ macro_rules! bail_value_err { } pub fn raise(msg: U) { - let py = unsafe { Python::assume_gil_acquired() }; + let gil = Python::acquire_gil(); + let py = gil.python(); let pyerr = PyErr::new::(msg.to_string()); pyerr.restore(py); } diff --git a/perde-core/src/import.rs b/perde-core/src/import.rs index f086004..d799dfb 100644 --- a/perde-core/src/import.rs +++ b/perde-core/src/import.rs @@ -50,7 +50,8 @@ lazy_static::lazy_static! { static ref STATIC_OBJECTS: Result = { use pyo3::{Python, types::PyModule}; - let py = unsafe { Python::assume_gil_acquired() }; + let gil = Python::acquire_gil(); + let py = gil.python(); macro_rules! import { ($name:expr) => { From 7e11a0ea352ef143707f85d9d8c4abc2368244de Mon Sep 17 00:00:00 2001 From: YushiOMOTE Date: Sat, 21 Nov 2020 15:57:25 +0900 Subject: [PATCH 7/9] Forward some low level errors to users --- perde-core/src/error.rs | 29 +++++++++--------- perde-core/src/import.rs | 4 +-- perde-core/src/object.rs | 66 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 81 insertions(+), 18 deletions(-) diff --git a/perde-core/src/error.rs b/perde-core/src/error.rs index f680996..a214d55 100644 --- a/perde-core/src/error.rs +++ b/perde-core/src/error.rs @@ -1,3 +1,4 @@ +use crate::object::ErrorObject; use pyo3::{type_object::PyTypeObject, PyErr, Python}; use serde::{de, ser}; use std::fmt::{self, Display}; @@ -8,7 +9,7 @@ pub type Result = std::result::Result; pub enum Error { TypeError(String), ValueError(String), - Else(String), + Native(String, Option), } #[macro_export] @@ -60,26 +61,19 @@ pub fn raise(msg: U) { pyerr.restore(py); } -fn clear() { - if unsafe { !pyo3::ffi::PyErr_Occurred().is_null() } { - unsafe { pyo3::ffi::PyErr_Clear() }; - } -} - impl Error { pub fn new(t: T) -> Self where T: ToString, { - clear(); - Self::Else(t.to_string()) + Self::Native(t.to_string(), ErrorObject::new()) } pub fn type_error(t: T) -> Self where T: ToString, { - clear(); + ErrorObject::clear(); Self::TypeError(t.to_string()) } @@ -87,7 +81,7 @@ impl Error { where T: ToString, { - clear(); + ErrorObject::clear(); Self::ValueError(t.to_string()) } @@ -95,13 +89,18 @@ impl Error { match self { Error::TypeError(t) => raise::(t), Error::ValueError(t) => raise::(t), - Error::Else(t) => raise::(t), + Error::Native(_, Some(t)) => { + t.restore(); + } + Error::Native(s, None) => { + raise::(format!("{}", s)) + } } } pub fn set_message(&mut self, message: String) { match self { - Error::TypeError(m) | Error::ValueError(m) | Error::Else(m) => { + Error::TypeError(m) | Error::ValueError(m) | Error::Native(m, _) => { *m = message; } } @@ -109,7 +108,7 @@ impl Error { pub fn message(&self) -> &str { match self { - Self::TypeError(m) | Self::ValueError(m) | Self::Else(m) => &m, + Self::TypeError(m) | Self::ValueError(m) | Self::Native(m, _) => &m, } } } @@ -129,7 +128,7 @@ impl Display for Error { f, "{}", match self { - Error::TypeError(m) | Error::ValueError(m) | Error::Else(m) => m, + Error::TypeError(m) | Error::ValueError(m) | Error::Native(m, _) => m, } ) } diff --git a/perde-core/src/import.rs b/perde-core/src/import.rs index d799dfb..0cf08de 100644 --- a/perde-core/src/import.rs +++ b/perde-core/src/import.rs @@ -30,7 +30,7 @@ pub struct Import { } pub fn import() -> Result<&'static Import> { - STATIC_OBJECTS.as_ref().map_err(|e| err!("{}", e)) + IMPORT.as_ref().map_err(|e| err!("{}", e)) } macro_rules! getattr { @@ -47,7 +47,7 @@ macro_rules! getattr { } lazy_static::lazy_static! { - static ref STATIC_OBJECTS: Result = { + static ref IMPORT: Result = { use pyo3::{Python, types::PyModule}; let gil = Python::acquire_gil(); diff --git a/perde-core/src/object.rs b/perde-core/src/object.rs index 7db76b8..13b5998 100644 --- a/perde-core/src/object.rs +++ b/perde-core/src/object.rs @@ -834,13 +834,16 @@ impl TupleBuilder { } } -#[derive(Debug)] pub struct SyncObject(AtomicPtr); impl SyncObject { pub fn new(obj: Object) -> Self { Self(AtomicPtr::new(obj.into_ptr())) } + + pub fn into_ptr(self) -> *mut PyObject { + self.as_ptr() + } } impl From for SyncObject { @@ -870,3 +873,64 @@ impl Clone for SyncObject { Self::new(self.owned()) } } + +impl Drop for SyncObject { + fn drop(&mut self) { + let _ = Object::new(self.as_ptr()); + } +} + +impl Debug for SyncObject { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + self.deref().fmt(f) + } +} + +#[derive(Debug)] +pub struct ErrorObject { + ptype: SyncObject, + pvalue: SyncObject, + ptraceback: SyncObject, +} + +impl ErrorObject { + pub fn new() -> Option { + if unsafe { PyErr_Occurred().is_null() } { + return None; + } + + unsafe { + let mut ptype = std::ptr::null_mut(); + let mut pvalue = std::ptr::null_mut(); + let mut ptraceback = std::ptr::null_mut(); + + pyo3::ffi::PyErr_Fetch(&mut ptype, &mut pvalue, &mut ptraceback); + + let ptype = Object::new(ptype); + let pvalue = Object::new(pvalue); + let ptraceback = Object::new(ptraceback); + + Some(ErrorObject { + ptype: ptype.ok()?.into(), + pvalue: pvalue.ok()?.into(), + ptraceback: ptraceback.ok()?.into(), + }) + } + } + + pub fn restore(self) { + unsafe { + pyo3::ffi::PyErr_Restore( + self.ptype.into_ptr(), + self.pvalue.into_ptr(), + self.ptraceback.into_ptr(), + ) + } + } + + pub fn clear() { + if unsafe { !pyo3::ffi::PyErr_Occurred().is_null() } { + unsafe { pyo3::ffi::PyErr_Clear() }; + } + } +} From 7157bfae6a53f1ed19f108171bc20b62cb1e05f3 Mon Sep 17 00:00:00 2001 From: YushiOMOTE Date: Sat, 21 Nov 2020 16:18:09 +0900 Subject: [PATCH 8/9] Add decode error cases --- perde-core/src/error.rs | 4 +- perde-tests/gen/datagen/src/main.rs | 24 ++++++++++ perde-tests/tests/test_error.py | 73 +++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 3 deletions(-) diff --git a/perde-core/src/error.rs b/perde-core/src/error.rs index a214d55..5a23c81 100644 --- a/perde-core/src/error.rs +++ b/perde-core/src/error.rs @@ -92,9 +92,7 @@ impl Error { Error::Native(_, Some(t)) => { t.restore(); } - Error::Native(s, None) => { - raise::(format!("{}", s)) - } + Error::Native(s, None) => raise::(format!("{}", s)), } } diff --git a/perde-tests/gen/datagen/src/main.rs b/perde-tests/gen/datagen/src/main.rs index 6b9524b..648badc 100644 --- a/perde-tests/gen/datagen/src/main.rs +++ b/perde-tests/gen/datagen/src/main.rs @@ -83,6 +83,30 @@ struct Opt { } fn main() { + #[derive(Serialize, Debug, new)] + struct TypeMismatch { + a: String, + b: Vec, + } + + add!(TypeMismatch { "hage".into(), vec![1,2,3] }); + + #[derive(Serialize, Debug, new)] + struct MissingMember { + a: String, + } + + add!(MissingMember { "hage".into() }); + + #[derive(Serialize, Debug, new)] + struct TooManyMember { + a: String, + b: String, + c: i64, + } + + add!(TooManyMember { "hage".into(), "faa".into(), 33 }); + #[derive(Serialize, Debug, new)] struct Plain { a: String, diff --git a/perde-tests/tests/test_error.py b/perde-tests/tests/test_error.py index 7cab069..f4f7fa1 100644 --- a/perde-tests/tests/test_error.py +++ b/perde-tests/tests/test_error.py @@ -216,3 +216,76 @@ class Abc: assert e.value.args[0] == ( "invalid argument: unsupported type " "`.Abc'>`") + + +"""rust +#[derive(Serialize, Debug, new)] +struct TypeMismatch { + a: String, + b: Vec, +} + +add!(TypeMismatch { "hage".into(), vec![1,2,3] }); +""" + + +@pytest.mark.parametrize("m", FORMATS) +def test_error_decode_type_mismatch(m): + @dataclass + class TypeMismatch: + a: str + b: str + + with pytest.raises(m.errtype) as e: + m.loads_as(TypeMismatch, m.data("DecodeError")) + + print(f"{m.name}: {e}") + + +"""rust +#[derive(Serialize, Debug, new)] +struct MissingMember { + a: String, +} + +add!(MissingMember { "hage".into() }); +""" + + +@pytest.mark.parametrize("m", FORMATS) +def test_error_decode_missing_member(m): + @dataclass + class MissingMember: + a: str + b: str + + with pytest.raises(m.errtype) as e: + m.loads_as(MissingMember, m.data("MissingMember")) + + print(f"{m.name}: {e}") + + +"""rust +#[derive(Serialize, Debug, new)] +struct TooManyMember { + a: String, + b: String, + c: i64, +} + +add!(TooManyMember { "hage".into(), "faa".into(), 33 }); +""" + + +@pytest.mark.parametrize("m", FORMATS) +def test_error_decode_missing_member(m): + @perde.attr(deny_unknown_fields=True) + @dataclass + class TooManyMember: + a: str + b: str + + with pytest.raises(m.errtype) as e: + m.loads_as(TooManyMember, m.data("TooManyMember")) + + print(f"{m.name}: {e}") From d47bc070989b87a891254e9c7f6b68cb04a85388 Mon Sep 17 00:00:00 2001 From: YushiOMOTE Date: Sat, 21 Nov 2020 17:46:15 +0900 Subject: [PATCH 9/9] Add encode error cases Also check the rmp-serde flatten issue --- perde-core/src/encode.rs | 7 +- perde-core/src/resolve.rs | 35 +++++++--- perde-tests/README.md | 16 ++--- perde-tests/gen/datagen/src/main.rs | 45 ++++++++++++ perde-tests/tests/test_attrs.py | 56 +++++++++++++++ perde-tests/tests/test_error.py | 102 ++++++++++++++++++++++++++-- 6 files changed, 233 insertions(+), 28 deletions(-) diff --git a/perde-core/src/encode.rs b/perde-core/src/encode.rs index 86d1fcd..a21cd40 100644 --- a/perde-core/src/encode.rs +++ b/perde-core/src/encode.rs @@ -93,7 +93,12 @@ impl<'a> Serialize for WithSchema<'a> { map.end() } Schema::Class(c) => { - let mut map = s.serialize_map(Some(c.ser_field_len))?; + let len = if c.flatten_dict.is_some() { + None + } else { + Some(c.ser_field_len) + }; + let mut map = s.serialize_map(len)?; serialize_fields(&self.object, &c.fields, &mut map)?; map.end() } diff --git a/perde-core/src/resolve.rs b/perde-core/src/resolve.rs index 3e695b6..5de3676 100644 --- a/perde-core/src/resolve.rs +++ b/perde-core/src/resolve.rs @@ -4,8 +4,11 @@ use crate::{ use indexmap::IndexMap; use std::collections::HashMap; -fn collect_members(mems: &IndexMap) -> (IndexMap, bool) { +fn collect_members( + mems: &IndexMap, +) -> (IndexMap, bool, usize) { let mut has_flatten = false; + let mut skip_field_len = 0; let mems = mems .iter() @@ -18,6 +21,10 @@ fn collect_members(mems: &IndexMap) -> (IndexMap {} } + } else { + if field.attr.skip || field.attr.skip_serializing { + skip_field_len += 1; + } } let mut map = IndexMap::new(); map.insert(key.to_string(), field.clone()); @@ -25,15 +32,17 @@ fn collect_members(mems: &IndexMap) -> (IndexMap) -> IndexMap { - let (mems, has_flatten) = collect_members(mems); +fn collect_flatten_members( + mems: &IndexMap, +) -> (IndexMap, usize) { + let (mems, has_flatten, skip_len) = collect_members(mems); if has_flatten { - mems + (mems, skip_len) } else { - IndexMap::new() + (IndexMap::new(), 0) } } @@ -141,7 +150,7 @@ fn to_dataclass(p: &ObjectRef, attr: &Option>) -> Resu let fields = fields.get_tuple_iter()?; let mut members = IndexMap::new(); - let mut ser_field_len = 0; + let mut skip_field_len = 0; let mut flatten_dict = None; let missing = &import()?.missing; @@ -174,8 +183,8 @@ fn to_dataclass(p: &ObjectRef, attr: &Option>) -> Resu ) }; - if !fattr.skip && !fattr.skip_serializing { - ser_field_len += 1; + if (fattr.skip || fattr.skip_serializing) && !fattr.flatten { + skip_field_len += 1; } let schema = to_schema(ty.as_ref())?; @@ -201,7 +210,13 @@ fn to_dataclass(p: &ObjectRef, attr: &Option>) -> Resu let name = p.name(); let class = p.owned(); - let flatten_members = collect_flatten_members(&members); + let (flatten_members, flatten_skip_len) = collect_flatten_members(&members); + + let ser_field_len = if flatten_members.is_empty() { + members.len() - skip_field_len + } else { + flatten_members.len() - flatten_skip_len + }; Ok(Schema::Class(Class::new( class.into(), diff --git a/perde-tests/README.md b/perde-tests/README.md index 264f42f..ce82d3c 100644 --- a/perde-tests/README.md +++ b/perde-tests/README.md @@ -81,18 +81,12 @@ ## Error testing -* Arguments error - * [ ] `dumps` - * [ ] `loads` - * [ ] `loads_as` -* Serialization error - * [ ] Parsing invalid objects. - * [ ] Serializing skipped enum variant. - * [ ] Serializing flatten struct in msgpack. -* Deserialization error - * [ ] Parsing broken data. - * [ ] Parsing valid format but doesn't match the specified type. +* [x] Arguments error +* [x] Serialization error +* [x] Deserialization error ## Known issues / Constraints * Flatten for msgpack doesn't work due to [the issue](https://github.com/3Hren/msgpack-rust/issues/196) in `rmp-serde`. + * Flatten for nested classes works in perde. The Rust issue has been bypassed. + * Flatten for dict isn't yet supported in perde. diff --git a/perde-tests/gen/datagen/src/main.rs b/perde-tests/gen/datagen/src/main.rs index 648badc..469ae68 100644 --- a/perde-tests/gen/datagen/src/main.rs +++ b/perde-tests/gen/datagen/src/main.rs @@ -107,6 +107,28 @@ fn main() { add!(TooManyMember { "hage".into(), "faa".into(), 33 }); + #[derive(Serialize, Debug, new)] + struct SkipEnumError { + x: i64, + e: String, + } + + add!(SkipEnumError { 3, "A".into() }); + + #[derive(Serialize, Debug, new)] + struct DictFlattenMsgpack { + x: String, + y: i64, + pp: String, + ppp: String, + pppp: String, + } + + add!(DictFlattenMsgpack { + "hey".into(), -103223, + "q1".into(), "q2".into(), "q3".into() + }); + #[derive(Serialize, Debug, new)] struct Plain { a: String, @@ -268,6 +290,29 @@ fn main() { }} except "msgpack"); + #[derive(Serialize, Debug, new)] + struct Flatten2 { + x: String, + a: i64, + b: i64, + } + + add!(Flatten2 { "haa".into(), 11, 33 }); + + #[derive(Serialize, Debug, new)] + struct DictFlatten2 { + x: String, + y: i64, + pp: String, + ppp: String, + pppp: String, + } + + add!(DictFlatten2 { + "hey".into(), -103223, + "q1".into(), "q2".into(), "q3".into() + }); + #[derive(Serialize, Debug, new)] struct DefaultConstruct { a: String, diff --git a/perde-tests/tests/test_attrs.py b/perde-tests/tests/test_attrs.py index a003a90..937f837 100644 --- a/perde-tests/tests/test_attrs.py +++ b/perde-tests/tests/test_attrs.py @@ -349,3 +349,59 @@ class DictFlatten: z: Dict[str, str] = field(metadata={"perde_flatten": True}) m.repack_type(DictFlatten) + + +"""rust +#[derive(Serialize, Debug, new)] +struct Flatten2 { + x: String, + a: i64, + b: i64, +} + +add!(Flatten2 { "haa".into(), 11, 33 }); +""" + + +@pytest.mark.parametrize("m", FORMATS) +def test_flatten2(m): + @dataclass + class Flatten2Child: + a: int + b: int + + @dataclass + class Flatten2: + x: str + y: Flatten2Child = field(metadata={"perde_flatten": True}) + + m.repack_type(Flatten2) + + +"""rust +#[derive(Serialize, Debug, new)] +struct DictFlatten2 { + x: String, + y: i64, + pp: String, + ppp: String, + pppp: String, +} + +add!(DictFlatten2 { + "hey".into(), -103223, + "q1".into(), "q2".into(), "q3".into() + }); +""" + + +# Hopefully support msgpack. +@pytest.mark.parametrize("m", FORMATS_EXCEPT("msgpack")) +def test_dict_flatten2(m): + @dataclass + class DictFlatten2: + x: str + y: int + z: Dict[str, str] = field(metadata={"perde_flatten": True}) + + m.repack_type(DictFlatten2) diff --git a/perde-tests/tests/test_error.py b/perde-tests/tests/test_error.py index f4f7fa1..8f92879 100644 --- a/perde-tests/tests/test_error.py +++ b/perde-tests/tests/test_error.py @@ -2,6 +2,7 @@ from dataclasses import dataclass, field import pytest import perde +import typing from util import FORMATS, FORMATS_EXCEPT, FORMATS_ONLY @@ -237,7 +238,7 @@ class TypeMismatch: b: str with pytest.raises(m.errtype) as e: - m.loads_as(TypeMismatch, m.data("DecodeError")) + m.loads_as(TypeMismatch, m.data("TypeMismatch")) print(f"{m.name}: {e}") @@ -245,7 +246,7 @@ class TypeMismatch: """rust #[derive(Serialize, Debug, new)] struct MissingMember { - a: String, + a: String, } add!(MissingMember { "hage".into() }); @@ -268,9 +269,9 @@ class MissingMember: """rust #[derive(Serialize, Debug, new)] struct TooManyMember { - a: String, - b: String, - c: i64, + a: String, + b: String, + c: i64, } add!(TooManyMember { "hage".into(), "faa".into(), 33 }); @@ -278,7 +279,7 @@ class MissingMember: @pytest.mark.parametrize("m", FORMATS) -def test_error_decode_missing_member(m): +def test_error_decode_too_many_member(m): @perde.attr(deny_unknown_fields=True) @dataclass class TooManyMember: @@ -289,3 +290,92 @@ class TooManyMember: m.loads_as(TooManyMember, m.data("TooManyMember")) print(f"{m.name}: {e}") + + +"""rust +#[derive(Serialize, Debug, new)] +struct SkipEnumError { + x: i64, + e: String, +} + +add!(SkipEnumError { 3, "A".into() }); +""" + + +@pytest.mark.parametrize("m", FORMATS) +def test_error_encode_skipped_enum(m): + class E(perde.Enum): + A = 1 + B = 2, {"perde_skip": True} + C = 3 + + @dataclass + class SkipEnumError: + x: int + e: E + + assert m.data("SkipEnumError") == m.dumps(SkipEnumError(3, E.A)) + + with pytest.raises(m.errtype) as e: + m.dumps(SkipEnumError(3, E.B)) + + assert e.value.args[ + 0] == 'variant `B` is marked as `skip` and cannot be serialized' + + class E2(perde.Enum): + A = 1 + B = 2, {"perde_skip_serializing": True} + C = 3 + + @dataclass + class SkipEnumError2: + x: int + e: E2 + + assert m.data("SkipEnumError") == m.dumps(SkipEnumError2(3, E2.A)) + + with pytest.raises(m.errtype) as e: + m.dumps(SkipEnumError(3, E2.B)) + + assert e.value.args[ + 0] == 'variant `B` is marked as `skip` and cannot be serialized' + + +"""rust +#[derive(Serialize, Debug, new)] +struct DictFlattenMsgpack { + x: String, + y: i64, + pp: String, + ppp: String, + pppp: String, +} + +add!(DictFlattenMsgpack { + "hey".into(), -103223, + "q1".into(), "q2".into(), "q3".into() + }); +""" + + +@pytest.mark.parametrize("m", FORMATS) +def test_error_dict_flatten_msgpack(m): + @dataclass + class DictFlattenMsgpack: + x: str + y: int + z: typing.Dict[str, str] = field(metadata={"perde_flatten": True}) + + d = DictFlattenMsgpack("hey", -103223, { + "pp": "q1", + "ppp": "q2", + "pppp": "q3" + }) + + if m.fmtname == "msgpack": + with pytest.raises(m.errtype) as e: + m.dumps(d) + print(e) + else: + assert m.dumps(d) == m.data("DictFlattenMsgpack")