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

feat: Add 'ord' option for PyClass and corresponding tests #4202

Merged
merged 26 commits into from
Jun 7, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
0cd69e4
feat: Add 'ord' option for PyClass and corresponding tests
May 22, 2024
676e1a5
update: fix formatting with cargo fmt
May 22, 2024
21abf24
update: documented added feature in newsfragments
May 22, 2024
cea1677
update: updated saved errors for comparison test for invalid pyclass …
May 22, 2024
01e76bd
update: removed nested match arms and extended cases for ordering ins…
May 22, 2024
c6349c6
update: alphabetically ordered entries
May 23, 2024
ea90363
update: added section to class documentation with example for using o…
May 23, 2024
d02f65b
refactor: reduced duplication of code using closure to process tokens.
May 23, 2024
8d2a974
update: used ensure_spanned macro to emit compile time errors for use…
May 23, 2024
3fbd4b0
fix: remove errant character
May 24, 2024
be00a8f
update: added note about PartialOrd being required.
May 28, 2024
5ed0cf5
feat: implemented ordering for structs and complex enums. Retained t…
May 28, 2024
708c934
update: adjusted compile time error checks for missing PartialOrd imp…
May 28, 2024
38ab0d1
fix: updated with clippy findings
May 28, 2024
455893d
update: added not to pyclass parameters on ord (assumes that eq will …
May 28, 2024
c924dab
Merge branch 'refs/heads/main' into ordering_support_simple_enums
May 31, 2024
8c11647
Merge branch 'refs/heads/main' into ordering_support_simple_enums
Jun 3, 2024
6788067
update: rebased on main after merging of `eq` feature
Jun 3, 2024
736a4b8
update: format update
Jun 3, 2024
ff81849
update: update all test output and doc tests
Jun 3, 2024
1259901
Update guide/src/class.md
Zyell Jun 4, 2024
14b5760
Update pyo3-macros-backend/src/pyclass.rs
Zyell Jun 4, 2024
b2851c7
Update newsfragments/4202.added.md
Zyell Jun 4, 2024
f8c782d
Update guide/pyclass-parameters.md
Zyell Jun 4, 2024
102b074
update: added note about `ord` implementation with example.
Jun 4, 2024
574266d
fix doc formatting
Icxolu Jun 7, 2024
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
1 change: 1 addition & 0 deletions newsfragments/4202.added.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Support optional `ord` argument to `#[pyclass]` for simple enums in the form of `#[pyclass(ord)]`.
Zyell marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions pyo3-macros-backend/src/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ pub mod kw {
syn::custom_keyword!(transparent);
syn::custom_keyword!(unsendable);
syn::custom_keyword!(weakref);
syn::custom_keyword!(ord);
}

#[derive(Clone, Debug)]
Expand Down
69 changes: 68 additions & 1 deletion pyo3-macros-backend/src/pyclass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ pub struct PyClassPyO3Options {
pub subclass: Option<kw::subclass>,
pub unsendable: Option<kw::unsendable>,
pub weakref: Option<kw::weakref>,
pub ord: Option<kw::ord>,
}

enum PyClassPyO3Option {
Expand All @@ -90,6 +91,7 @@ enum PyClassPyO3Option {
Subclass(kw::subclass),
Unsendable(kw::unsendable),
Weakref(kw::weakref),
Ord(kw::ord),
}

impl Parse for PyClassPyO3Option {
Expand Down Expand Up @@ -125,6 +127,8 @@ impl Parse for PyClassPyO3Option {
input.parse().map(PyClassPyO3Option::Unsendable)
} else if lookahead.peek(attributes::kw::weakref) {
input.parse().map(PyClassPyO3Option::Weakref)
} else if lookahead.peek(attributes::kw::ord) {
input.parse().map(PyClassPyO3Option::Ord)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: the rest of these were alphabetical, it might be nice to move this branch up to the right place in the ordering.

This same observation applies to a few similar additions above and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Also, I have updated the documentation with this feature.

} else {
Err(lookahead.error())
}
Expand Down Expand Up @@ -179,6 +183,7 @@ impl PyClassPyO3Options {
PyClassPyO3Option::Subclass(subclass) => set_option!(subclass),
PyClassPyO3Option::Unsendable(unsendable) => set_option!(unsendable),
PyClassPyO3Option::Weakref(weakref) => set_option!(weakref),
PyClassPyO3Option::Ord(ord) => set_option!(ord),
}
Ok(())
}
Expand Down Expand Up @@ -775,6 +780,66 @@ fn impl_simple_enum(
(int_impl, int_slot)
};

// Add ordering comparisons if ord option is passed to pyclass
let richcmp_ord = match args.options.ord {
Some(_) => {
quote! {
match op {
Icxolu marked this conversation as resolved.
Show resolved Hide resolved
#pyo3_path::basic::CompareOp::Gt => {
let self_val = self.__pyo3__int__();
if let Ok(i) = other.extract::<#repr_type>() {
return Ok((self_val > i).to_object(py));
}
if let Ok(other) = other.extract::<#pyo3_path::PyRef<Self>>() {
return Ok((self_val > other.__pyo3__int__()).to_object(py));
}

return Ok(py.NotImplemented());
}
#pyo3_path::basic::CompareOp::Lt => {
let self_val = self.__pyo3__int__();
if let Ok(i) = other.extract::<#repr_type>() {
return Ok((self_val < i).to_object(py));
}
if let Ok(other) = other.extract::<#pyo3_path::PyRef<Self>>() {
return Ok((self_val < other.__pyo3__int__()).to_object(py));
}

return Ok(py.NotImplemented());
}
#pyo3_path::basic::CompareOp::Le => {
let self_val = self.__pyo3__int__();
if let Ok(i) = other.extract::<#repr_type>() {
return Ok((self_val <= i).to_object(py));
}
if let Ok(other) = other.extract::<#pyo3_path::PyRef<Self>>() {
return Ok((self_val <= other.__pyo3__int__()).to_object(py));
}

return Ok(py.NotImplemented());
}
#pyo3_path::basic::CompareOp::Ge => {
let self_val = self.__pyo3__int__();
if let Ok(i) = other.extract::<#repr_type>() {
return Ok((self_val >= i).to_object(py));
}
if let Ok(other) = other.extract::<#pyo3_path::PyRef<Self>>() {
return Ok((self_val >= other.__pyo3__int__()).to_object(py));
}

return Ok(py.NotImplemented());
}
_ => Ok(py.NotImplemented()),
}
}
}
_ => {
quote! {
Ok(py.NotImplemented())
}
}
};

let (default_richcmp, default_richcmp_slot) = {
let mut richcmp_impl: syn::ImplItemFn = syn::parse_quote! {
fn __pyo3__richcmp__(
Expand Down Expand Up @@ -809,7 +874,9 @@ fn impl_simple_enum(

return Ok(py.NotImplemented());
}
_ => Ok(py.NotImplemented()),
_ => {
#richcmp_ord
},
}
}
};
Expand Down
31 changes: 31 additions & 0 deletions tests/test_enum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@ pub enum MyEnum {
OtherVariant,
}

#[pyclass(ord)]
#[derive(Debug, PartialEq, Eq, Clone)]
Copy link
Member

Choose a reason for hiding this comment

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

A thought: should we require a PartialOrd implementation for pyclass(ord)? I see two upsides:

  • That would allow us to skip casting via __pyo3__int__.
  • I think for structs and other data classes we would need to use the PartialOrd implementation anyway as __pyo3__int__ wouldn't be possible, so again this seems good for consistency.

If we agree, as far as implementation would go, just remove the calls to convert with __pyo3__int__ from these branches.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was also one of my first thoughts, but then we should probably also require PartialEq for the already existing impls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Requiring PartialOrd seems reasonable to me. Would requiring PartialEq break anything for current implementations? If we require PartialOrd, would the ensure_spanned checks still be required?

Copy link
Member

Choose a reason for hiding this comment

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

I think requiring PartialEq would break compiles, but it'd also be trivial to fix for these simple enum cases (just add the derive). @Icxolu would you be ok if we agree that we should require PartialEq but postpone changing that to a future PR / tracking issue where we can also add #[pyclass(eq)] at the same time?

I also would love to have #[pyclass(hash)] to be implemented (again, requiring Hash).

... and then a future step is working out how to apply all of these to structs to get dataclass-like functionality :)

Copy link
Member

Choose a reason for hiding this comment

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

If we require PartialOrd, would the ensure_spanned checks still be required?

If you mean the checks for ord not being accepted on structs, we'll still need those (unless we extend this PR to make ord work for every type of #[pyclass], which I think should be possible with a bit more work).

Copy link
Contributor

Choose a reason for hiding this comment

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

@Icxolu would you be ok if we agree that we should require PartialEq but postpone changing that to a future PR / tracking issue where we can also add #[pyclass(eq)] at the same time?

I'm good with that, sounds like a nice followup to this one 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you mean the checks for ord not being accepted on structs, we'll still need those (unless we extend this PR to make ord work for every type of #[pyclass], which I think should be possible with a bit more work).

Yes, I think making ord work for all of these types would be great in this PR. I will scope this out and raise any questions along the way.

pub enum MyEnumOrd {
Variant,
OtherVariant,
}

#[test]
fn test_enum_class_attr() {
Python::with_gil(|py| {
Expand Down Expand Up @@ -73,6 +80,30 @@ fn test_enum_eq_incomparable() {
})
}

#[test]
Copy link
Member

Choose a reason for hiding this comment

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

Nice tests 👍

fn test_enum_ord_comparable_opt_in_only() {
Python::with_gil(|py| {
let var1 = Py::new(py, MyEnum::Variant).unwrap();
let var2 = Py::new(py, MyEnum::OtherVariant).unwrap();
// ordering on simple enums if opt in only, thus raising an error below
py_expect_exception!(py, var1 var2, "(var1 > var2) == False", PyTypeError);
})
}

#[test]
fn test_enum_ord_comparable() {
Python::with_gil(|py| {
let var1 = Py::new(py, MyEnumOrd::Variant).unwrap();
let var2 = Py::new(py, MyEnumOrd::OtherVariant).unwrap();
let var3 = Py::new(py, MyEnumOrd::OtherVariant).unwrap();
py_assert!(py, var1 var2, "(var1 > var2) == False");
py_assert!(py, var1 var2, "(var1 < var2) == True");
py_assert!(py, var1 var2, "(var1 >= var2) == False");
py_assert!(py, var2 var3, "(var3 >= var2) == True");
py_assert!(py, var1 var2, "(var1 <= var2) == True");
})
}

#[pyclass]
enum CustomDiscriminant {
One = 1,
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/invalid_pyclass_args.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: expected one of: `crate`, `dict`, `extends`, `freelist`, `frozen`, `get_all`, `mapping`, `module`, `name`, `rename_all`, `sequence`, `set_all`, `subclass`, `unsendable`, `weakref`
error: expected one of: `crate`, `dict`, `extends`, `freelist`, `frozen`, `get_all`, `mapping`, `module`, `name`, `rename_all`, `sequence`, `set_all`, `subclass`, `unsendable`, `weakref`, `ord`
--> tests/ui/invalid_pyclass_args.rs:3:11
|
3 | #[pyclass(extend=pyo3::types::PyDict)]
Expand Down Expand Up @@ -46,7 +46,7 @@ error: expected string literal
24 | #[pyclass(module = my_module)]
| ^^^^^^^^^

error: expected one of: `crate`, `dict`, `extends`, `freelist`, `frozen`, `get_all`, `mapping`, `module`, `name`, `rename_all`, `sequence`, `set_all`, `subclass`, `unsendable`, `weakref`
error: expected one of: `crate`, `dict`, `extends`, `freelist`, `frozen`, `get_all`, `mapping`, `module`, `name`, `rename_all`, `sequence`, `set_all`, `subclass`, `unsendable`, `weakref`, `ord`
--> tests/ui/invalid_pyclass_args.rs:27:11
|
27 | #[pyclass(weakrev)]
Expand Down
Loading