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

Zyell · 2024-05-22T20:13:28Z

Updated the macros back-end to include 'ord' as an option for PyClass allowing for Python-style ordering comparison of enum variants. Additionally, test cases to verify the proper functioning of this new feature have been introduced.

Thank you for contributing to PyO3!

By submitting these contributions you agree for them to be dual-licensed under PyO3's MIT OR Apache-2.0 license.

Please consider adding the following to your pull request:

an entry for this PR in newsfragments - see [https://pyo3.rs/main/contributing.html#documenting-changes]
- or start the PR title with docs: if this is a docs-only change to skip the check
docs to all new functions and / or detail in the guide
tests for all new or changed functions

PyO3's CI pipeline will check your pull request, thus make sure you have checked the Contributing.md guidelines. To run most of its tests
locally, you can run nox. See nox --list-sessions
for a list of supported actions.

Updated the macros back-end to include 'ord' as an option for PyClass allowing for Python-style ordering comparison of enum variants. Additionally, test cases to verify the proper functioning of this new feature have been introduced.

…args

Icxolu

Given that this can be effectively archived today using

int(SimpleEnum.VariantA) >= int(SimpleEnum.VariantB)

I wonder whether there is a case for just providing these unconditionally 🤔. Let's see what others think about that.

If we decide to keep this conditionally, we would also need error messages for the
#[pyclass]es for which this option is not supported (structs, complex enums) and corresponding tests.

pyo3-macros-backend/src/pyclass.rs

…tead

davidhewitt

Thanks! Indeed it was my suggestion to be opt-in; as well as it being good for consistency with supporting this on structs I felt that this way we avoid silently breaking users.

We should probably add some documentation somewhere in the enum section of class.md for this too.

davidhewitt · 2024-05-23T19:05:35Z

pyo3-macros-backend/src/pyclass.rs

+        } else if lookahead.peek(attributes::kw::ord) {
+            input.parse().map(PyClassPyO3Option::Ord)


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.

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

davidhewitt · 2024-05-23T19:19:17Z

tests/test_enum.rs

+#[pyclass(ord)]
+#[derive(Debug, PartialEq, Eq, Clone)]


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.

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

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?

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 :)

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).

@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 👍

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.

davidhewitt · 2024-05-23T19:21:21Z

tests/test_enum.rs

@@ -73,6 +80,30 @@ fn test_enum_eq_incomparable() {
    })
 }

+#[test]


Nice tests 👍

…rd argument.

…s of ord on complex enums or structs, updated test errors for bad compile cases

Icxolu

Nice, the refactor with the closure makes this much nicer to read! I added a hint for the PartialOrd requirement and I think we should also add the new ord attribute in pyclass-parameters.md, so it's documented in the guide and on docs.rs (it's a shared file)

guide/src/class.md

pyo3-macros-backend/src/pyclass.rs

…e equality logic for simple enums until PartialEq is deprecated.

…lementations. Refactored growing set of comparison tests for simple and complex enums and structs into separate test file.

Zyell · 2024-05-28T14:28:00Z

I just pushed the changes I worked on over the weekend (sorry for the delay! It was a holiday weekend here, didn't get around to finalizing things until this morning) to support ordering for all enums and structs. I've updated tests and placed a note in the documentation. Compile time errors are also validated for when ParialOrd is not implemented and the ord argument is passed to the pyclass macro. I have made notes that additional checks for equality will be added when that is done. @Icxolu I will take a closer look at your PR to merge that. Edit: Found pyclass parameters documentation section, but let me know if additional updates should be made elsewhere.

…be implemented and merged first)

# Conflicts: # pyo3-macros-backend/src/pyclass.rs # tests/test_enum.rs # tests/ui/invalid_pyclass_args.rs # tests/ui/invalid_pyclass_args.stderr # tests/ui/invalid_pyclass_enum.rs # tests/ui/invalid_pyclass_enum.stderr

Zyell · 2024-05-31T21:19:41Z

@Icxolu I am working on integrating ord now that eq has been merged. I was working on the tests and came across this case:

#[pyclass(eq_int)]
#[derive(Debug, Clone)]
pub enum MyEnumOrd {
    Variant,
    OtherVariant,
}

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

This currently fails, but adding eq causes it to pass. There was a test in the test suite before with a similar test for which this was passing without eq or eq_int. If the goal is to retain the old behavior while issuing the deprecation warning, shouldn't the above test pass? Is this the expected behavior if only eq_int is passed? I ran this test on main to verify it wasn't some regression I introduced. The only difference I can see is in that no match arms are added, just the catchall remains, and the slot is generated differently here.

Icxolu · 2024-05-31T21:40:33Z

Oops, yeah that's a bug. Thanks for catching! It seem like I forgot the eq_int case in pyclass_richcmp_arms, I'll open a PR to fix 😇

# Conflicts: # tests/ui/invalid_pyclass_args.rs # tests/ui/invalid_pyclass_enum.rs

Icxolu

Looks good to me 🚀! I just found two tiny stylistic suggestions. Additionally we maybe want to add some documentation of this to object.md#comparisons.

guide/src/class.md

pyo3-macros-backend/src/pyclass.rs

newsfragments/4202.added.md

guide/pyclass-parameters.md

Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>

Icxolu

Thanks! This looks good to me now 💯

guide/pyclass-parameters.md

Michael Gilbert added 4 commits May 22, 2024 13:05

update: fix formatting with cargo fmt

676e1a5

update: documented added feature in newsfragments

21abf24

update: updated saved errors for comparison test for invalid pyclass …

cea1677

…args

Icxolu reviewed May 22, 2024

View reviewed changes

pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved

update: removed nested match arms and extended cases for ordering ins…

01e76bd

…tead

davidhewitt reviewed May 23, 2024

View reviewed changes

Michael Gilbert added 4 commits May 23, 2024 16:55

update: alphabetically ordered entries

c6349c6

update: added section to class documentation with example for using o…

ea90363

…rd argument.

refactor: reduced duplication of code using closure to process tokens.

d02f65b

update: used ensure_spanned macro to emit compile time errors for use…

8d2a974

…s of ord on complex enums or structs, updated test errors for bad compile cases

Icxolu reviewed May 24, 2024

View reviewed changes

guide/src/class.md Outdated Show resolved Hide resolved

pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved

pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved

fix: remove errant character

3fbd4b0

Icxolu mentioned this pull request May 25, 2024

add pyclass hash option #4206

Merged

davidhewitt mentioned this pull request May 26, 2024

#[pyclass] trait ergonomics #4207

Open

5 tasks

Michael Gilbert added 3 commits May 28, 2024 07:12

update: added note about PartialOrd being required.

be00a8f

feat: implemented ordering for structs and complex enums. Retained th…

5ed0cf5

…e equality logic for simple enums until PartialEq is deprecated.

update: adjusted compile time error checks for missing PartialOrd imp…

708c934

…lementations. Refactored growing set of comparison tests for simple and complex enums and structs into separate test file.

Michael Gilbert added 2 commits May 28, 2024 07:45

fix: updated with clippy findings

38ab0d1

update: added not to pyclass parameters on ord (assumes that eq will …

455893d

…be implemented and merged first)

Icxolu mentioned this pull request May 28, 2024

add pyclass eq option #4210

Merged

Merge branch 'refs/heads/main' into ordering_support_simple_enums

c924dab

# Conflicts: # pyo3-macros-backend/src/pyclass.rs # tests/test_enum.rs # tests/ui/invalid_pyclass_args.rs # tests/ui/invalid_pyclass_args.stderr # tests/ui/invalid_pyclass_enum.rs # tests/ui/invalid_pyclass_enum.stderr

Icxolu mentioned this pull request May 31, 2024

fix incorrect __richcmp__ for eq_int only simple enums #4224

Merged

Michael Gilbert added 4 commits June 3, 2024 16:02

Merge branch 'refs/heads/main' into ordering_support_simple_enums

8c11647

# Conflicts: # tests/ui/invalid_pyclass_args.rs # tests/ui/invalid_pyclass_enum.rs

update: rebased on main after merging of eq feature

6788067

update: format update

736a4b8

update: update all test output and doc tests

ff81849

Icxolu reviewed Jun 4, 2024

View reviewed changes

guide/src/class.md Outdated Show resolved Hide resolved

pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved

newsfragments/4202.added.md Outdated Show resolved Hide resolved

guide/pyclass-parameters.md Outdated Show resolved Hide resolved

Zyell and others added 5 commits June 4, 2024 10:42

Update guide/src/class.md

1259901

Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>

Update pyo3-macros-backend/src/pyclass.rs

14b5760

Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>

Update newsfragments/4202.added.md

b2851c7

Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>

Update guide/pyclass-parameters.md

f8c782d

Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>

update: added note about ord implementation with example.

102b074

Icxolu approved these changes Jun 7, 2024

View reviewed changes

guide/pyclass-parameters.md Outdated Show resolved Hide resolved

fix doc formatting

574266d

Icxolu enabled auto-merge June 7, 2024 18:48

Icxolu added this pull request to the merge queue Jun 7, 2024

Merged via the queue into PyO3:main with commit b8fb367 Jun 7, 2024
41 checks passed

Zyell deleted the ordering_support_simple_enums branch June 7, 2024 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Zyell commented May 22, 2024

Icxolu left a comment

davidhewitt left a comment

davidhewitt May 23, 2024

Zyell May 24, 2024

davidhewitt May 23, 2024

Icxolu May 23, 2024

Zyell May 24, 2024

davidhewitt May 24, 2024

davidhewitt May 24, 2024

Icxolu May 24, 2024

Zyell May 24, 2024

davidhewitt May 23, 2024

Icxolu left a comment

Zyell commented May 28, 2024 •

edited

Loading

Zyell commented May 31, 2024

Icxolu commented May 31, 2024

Icxolu left a comment

Icxolu left a comment

		} else if lookahead.peek(attributes::kw::ord) {
		input.parse().map(PyClassPyO3Option::Ord)

@@ @@ -73,6 +80,30 @@ fn test_enum_eq_incomparable() { @@
                   })
               }
+              #[test]

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

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

Conversation

Zyell commented May 22, 2024

Icxolu left a comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Icxolu left a comment

Choose a reason for hiding this comment

Zyell commented May 28, 2024 • edited Loading

Zyell commented May 31, 2024

Icxolu commented May 31, 2024

Icxolu left a comment

Choose a reason for hiding this comment

Icxolu left a comment

Choose a reason for hiding this comment

Zyell commented May 28, 2024 •

edited

Loading