Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

silence deprecation warning for enum with custom __eq__ #4692

Merged
merged 4 commits into from
Nov 9, 2024

Conversation

davidhewitt
Copy link
Member

Fixes #4687

This adjusts the deprecation warning so that it's conditionally emitted using good old auto ref specialization, so that if the user provides __eq__ (or any other __richcmp__ component) then the deprecation warning is not emitted.

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.

Ah, very clever! This seems much nicer than an opt-out option. (But I'm still happy once we can remove all of this again 🙃)

It doesn't work however with a manual __richcmp__ implementation directly. From a quick test we could do something like to following to catch that as well:

diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs
index 1254a8d51..a5993c981 100644
--- a/pyo3-macros-backend/src/pymethod.rs
+++ b/pyo3-macros-backend/src/pymethod.rs
@@ -1348,6 +1348,13 @@ impl SlotDef {
         )?;
         let name = spec.name;
         let holders = holders.init_holders(ctx);
+
+        let dep = if method_name == "__richcmp__" {
+            quote! { impl #pyo3_path::impl_::pyclass::HasCustomRichCmp for #cls {} }
+        } else {
+            TokenStream::default()
+        };
+
         let associated_method = quote! {
             #[allow(non_snake_case)]
             unsafe fn #wrapper_ident(
@@ -1355,6 +1362,7 @@ impl SlotDef {
                 _raw_slf: *mut #pyo3_path::ffi::PyObject,
                 #(#arg_idents: #arg_types),*
             ) -> #pyo3_path::PyResult<#ret_ty> {
+                #dep
                 let function = #cls::#name; // Shadow the method name to avoid #3017
                 let _slf = _raw_slf;
                 #holders

@davidhewitt
Copy link
Member Author

davidhewitt commented Nov 8, 2024

Ah, good catch, thanks👍. Pushed a commit with that patch.

(But I'm still happy once we can remove all of this again 🙃)

Haha yes indeed, we can probably delete this next week after the 0.23 branch is cut and released!

@Icxolu
Copy link
Contributor

Icxolu commented Nov 9, 2024

Just pushed a commit to fix ci (allow non_local_definitions and bless ui tests)

@Icxolu Icxolu enabled auto-merge November 9, 2024 14:43
@Icxolu Icxolu added this pull request to the merge queue Nov 9, 2024
Merged via the queue into PyO3:main with commit 3a6296e Nov 9, 2024
46 checks passed
@davidhewitt
Copy link
Member Author

Thanks 🙏

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no way to disable implicit equality warning for enum
2 participants