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

rust: support 1.57 #2040

Merged
merged 1 commit into from
Dec 8, 2021
Merged

rust: support 1.57 #2040

merged 1 commit into from
Dec 8, 2021

Conversation

davidhewitt
Copy link
Member

Clippy fixes for Rust 1.57.

There's a new lint which catches that our macro code generates useless Option::as_deref in certain situations. I'm not entirely sure what to do about that; I might need to poke around at things a bit to find an alternative implementation.

@davidhewitt
Copy link
Member Author

This Option::as_deref lint is proving awkward - I haven't found a place in the codegen where I can add an attribute to silence it. I've been experimenting with alternative implementations, but haven't found anything which works cleanly yet.

Suggestions welcome! Otherwise I'll just keep musing over this when I have spare moments.

@mejrs
Copy link
Member

mejrs commented Dec 6, 2021

I suggest sprinkling some #[allow(clippy::all)] over the generated code, like

fn impl_py_method_def_new(cls: &syn::Type, spec: &FnSpec) -> Result<TokenStream> {
    let wrapper_ident = syn::Ident::new("__wrap", Span::call_site());
    let wrapper = spec.get_wrapper_function(&wrapper_ident, Some(cls))?;
    Ok(quote! {
        #[allow(clippy::all)]
        impl ::pyo3::class::impl_::PyClassNewImpl<#cls> for ::pyo3::class::impl_::PyClassImplCollector<#cls> {
            fn new_impl(self) -> ::std::option::Option<::pyo3::ffi::newfunc> {
                ::std::option::Option::Some({
                    #wrapper
                    #wrapper_ident
                })
            }
        }
    })
}

or in a more granular way by putting it directly on the use site:

        Ok(quote_arg_span! {
            let #mut_ _tmp: #target_ty = #arg_value_or_default;

            #[allow(clippy::needless_option_as_deref)]
            let #arg_name = #borrow_tmp;
        })

@davidhewitt
Copy link
Member Author

or in a more granular way by putting it directly on the use site:

Ah, that's solved it nicely, thanks 👍. I was trying to place it on the generated expression for #borrow_tmp, which the compiler didn't like (attributes on expressions aren't fully supported).

@davidhewitt davidhewitt force-pushed the rust-1.57 branch 2 times, most recently from 14a6e95 to 5d19269 Compare December 7, 2021 23:30
@davidhewitt davidhewitt marked this pull request as ready for review December 8, 2021 07:10
@davidhewitt davidhewitt enabled auto-merge December 8, 2021 07:10
@davidhewitt davidhewitt merged commit 2705a92 into PyO3:main Dec 8, 2021
@davidhewitt davidhewitt deleted the rust-1.57 branch December 8, 2021 07:30
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.

2 participants