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

Conversation

Zyell
Copy link
Contributor

@Zyell Zyell commented May 22, 2024

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.

Michael Gilbert added 4 commits May 22, 2024 13:05
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.
Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines 130 to 131
} 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.

Comment on lines 16 to 17
#[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.

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

Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

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 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
@Icxolu Icxolu mentioned this pull request May 25, 2024
@davidhewitt davidhewitt mentioned this pull request May 26, 2024
5 tasks
Michael Gilbert added 3 commits May 28, 2024 07:12
…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
Copy link
Contributor Author

Zyell commented May 28, 2024

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.

@Icxolu Icxolu mentioned this pull request May 28, 2024
# 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
Copy link
Contributor Author

Zyell commented May 31, 2024

@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
Copy link
Contributor

Icxolu commented May 31, 2024

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 😇

Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

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 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
Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me now 💯

guide/pyclass-parameters.md Outdated Show resolved Hide resolved
@Icxolu Icxolu enabled auto-merge June 7, 2024 18:48
@Icxolu 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 Zyell deleted the ordering_support_simple_enums branch June 7, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants