Enable some Clippy lints

[nickdrozd]

Hello! This PR enables some Clippy lints and fixes all the warnings. There is one lint enabled per commit (except for the pyo3-macros) one, so I would suggest reviewing the commits one at a time. The changes all seem reasonable, and hopefully not too intrusive. It doesn't do pyo3-macros-backend, but I can add that too if that's wanted.

[LilyFoote]

Hi! Thanks for having a look at this.

I've added some comments and questions for some lints. I'm also not sure that we'll want to enable all of these. For example, the missing_const_for_fn lint is explicitly in development and has a bunch of caveats. I think it might be better to open a PR for each lint. That would make it easier to say "yes we want this" or "no we don't want this" on a case-by-case basis.

Finally, for this to land, we'll need to fix the lints for all CI builds.

[LilyFoote]

This lint looks like a false positive to me.

Something we could consider changing here would be to remove the return statements and use implicit return. But with the two cfg options, I'm not certain that'd be more readable.

[nickdrozd]

I had tried that initially, but explicit return seems to be required:

697 |                 #[cfg(any(Py_LIMITED_API, PyPy, GraalPy))]
    |                 ------------------------------------------ only `;` terminated statements or tail expressions are allowed after this attribute
698 |                 Ok(($(t.get_borrowed_item($n)?.extract::<$T>()?,)+))
    |                                                                     ^ expected `;` here

[LilyFoote]

I think a better solution here would be to use payload.downcast::<&str>. I think downcast_ref introduces the extra reference clippy is complaining about.

[nickdrozd]

Something like this?

        if let Some(string) = payload.downcast_ref::<String>() {
            Self::new_err((string.clone(),))
        } else if let Ok(s) = payload.downcast::<&str>() {  // <-- downcast / Ok
            Self::new_err((s.to_string(),))  // <-- no ref
        } else {
            Self::new_err(("panic from Rust code",))
        }

Edit: I pushed this change in.

[LilyFoote]

I'm not sure about this one - maybe to_owned is more readable?

[adamreichold]

I think this is an artefact from refactoring the initial implementation and clone seems appropriate, i.e. we have a PyBytes and just want another reference to it, not change from a borrowed to an owned type.

[nickdrozd]

The lint description says that there is no difference, and to_owned is misleading because there is no change in ownership.

[nickdrozd]

I think it might be better to open a PR for each lint. That would make it easier to say "yes we want this" or "no we don't want this" on a case-by-case basis.

Finally, for this to land, we'll need to fix the lints for all CI builds.

I dropped the const and semicolon lints, since those changed a lot of lines and were both failing in CI (hard to check locally because of all the various config flags). I can still break up the remaining commits if you like, but I think it should be a lot more manageable now.

[nickdrozd]

I didn't realize there was such deviation between a local run and a CI run 😵‍💫

[adamreichold]

I didn't realize there was such deviation between a local run and a CI run 😵‍💫

You can reproduce most of that locally using the Nox sessions which e.g. enumerate feature sets.

[birkenfeld]

For example, the missing_const_for_fn lint is explicitly in development and has a bunch of caveats.

Especially since what could be const today may not be able to be const tomorrow. IMO it's a bit irresponsible of Clippy to integrate such lints that encourage API breaking or major-version inflation, seeing as how people tend to apply its suggestions without much critical thinking (which, in many cases, is justified).

Edit: I see it's allow-by-default. That makes more sense, and once it's out of the nursery it will probably go to a lint group that we definitely don't want to have in a sweeping #[deny].

[LilyFoote]

I'm still not convinced we necessarily want all of these, so I'd prefer to have them as separate PRs.

[davidhewitt]

Sorry that this PR got stuck in limbo so long.

I agree with the above sentiments that there are a number of lints added here which probably are too extreme for what we really want; there's been quite a few discussions where we've decided not to enable too many stylistic lints (e.g. the clippy::pedantic group) as the stricter the linting the harder it gets to appease our already complex CI pipeline.

I will close this PR, though thank you for the contribution and I am always sorry to see work not merged.

For what it's worth, there may be merit in enabling more lints, but:

• We should probably propose and discuss them in issues first to avoid too much work lost
• New lint configuration should go in the Cargo.toml lints tables rather than in source files