Enable some Clippy lints #4120
Enable some Clippy lints #4120
Conversation
There was a problem hiding this comment.
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.
| if t.len() == $length { | ||
| if t.len() != $length { | ||
| Err(wrong_tuple_length(t, $length)) | ||
| } else { | ||
| #[cfg(any(Py_LIMITED_API, PyPy, GraalPy))] | ||
| return Ok(($(t.get_borrowed_item($n)?.extract::<$T>()?,)+)); | ||
|
|
||
| #[cfg(not(any(Py_LIMITED_API, PyPy, GraalPy)))] | ||
| unsafe {return Ok(($(t.get_borrowed_item_unchecked($n).extract::<$T>()?,)+));} | ||
| } else { | ||
| Err(wrong_tuple_length(t, $length)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
src/panic.rs
Outdated
| } else if let Some(s) = payload.downcast_ref::<&str>() { | ||
| Self::new_err((s.to_string(),)) | ||
| Self::new_err(((*s).to_string(),)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -152,7 +152,7 @@ impl From<Bound<'_, PyBytes>> for PyBackedBytes { | |||
| let b = py_bytes.as_bytes(); | |||
| let data = NonNull::from(b); | |||
| Self { | |||
| storage: PyBackedBytesStorage::Python(py_bytes.to_owned().unbind()), | |||
| storage: PyBackedBytesStorage::Python(py_bytes.clone().unbind()), | |||
There was a problem hiding this comment.
I'm not sure about this one - maybe to_owned is more readable?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The lint description says that there is no difference, and to_owned is misleading because there is no change in ownership.
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. |
|
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. |
Especially since what could be 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 |
|
I'm still not convinced we necessarily want all of these, so I'd prefer to have them as separate PRs. |
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 dopyo3-macros-backend, but I can add that too if that's wanted.