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.

Sign up for GitHub

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

Jump to bottom

Enable some Clippy lints #4120

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Enable some Clippy lints #4120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d32ef64
Enable lint: inefficient_to_string
nickdrozd Apr 19, 2024
7d7c4a7
Enable lint: type_repetition_in_bounds
nickdrozd Apr 21, 2024
96026dd
Enable lint: map_unwrap_or
nickdrozd Apr 21, 2024
80782ca
Enable lint: implicit_clone
nickdrozd Apr 23, 2024
cab28df
Enable lint: ignored_unit_patterns
nickdrozd Apr 24, 2024
9d8cc59
Enable lint: redundant_else
nickdrozd Apr 25, 2024
ee0a9ad
Enable clippy for pyo3-macros
nickdrozd Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions pyo3-macros/src/lib.rs
View file
Open in desktop
@@ -1,6 +1,7 @@
//! This crate declares only the proc macro attributes, as a crate defining proc macro attributes
//! must not contain any other public items.

#![deny(clippy::all, clippy::pedantic, clippy::nursery)]
#![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))]
use proc_macro::TokenStream;
use proc_macro2::TokenStream as TokenStream2;
Expand Down Expand Up @@ -204,7 +205,7 @@ fn pymethods_impl(
.into()
}

fn methods_type() -> PyClassMethodsType {
const fn methods_type() -> PyClassMethodsType {
if cfg!(feature = "multiple-pymethods") {
PyClassMethodsType::Inventory
} else {
Expand All @@ -218,6 +219,6 @@ trait UnwrapOrCompileError {

impl UnwrapOrCompileError for syn::Result<TokenStream2> {
fn unwrap_or_compile_error(self) -> TokenStream2 {
self.unwrap_or_else(|e| e.into_compile_error())
self.unwrap_or_else(syn::Error::into_compile_error)
}
}
6 changes: 3 additions & 3 deletions src/conversions/chrono.rs
View file
Open in desktop
Expand Up @@ -1194,7 +1194,7 @@ mod tests {
let py_time = CatchWarnings::enter(py, |_| Ok(time.to_object(py))).unwrap();
let roundtripped: NaiveTime = py_time.extract(py).expect("Round trip");
// Leap seconds are not roundtripped
let expected_roundtrip_time = micro.checked_sub(1_000_000).map(|micro| NaiveTime::from_hms_micro_opt(hour, min, sec, micro).unwrap()).unwrap_or(time);
let expected_roundtrip_time = micro.checked_sub(1_000_000).map_or(time, |micro| NaiveTime::from_hms_micro_opt(hour, min, sec, micro).unwrap());
assert_eq!(expected_roundtrip_time, roundtripped);
}
})
Expand Down Expand Up @@ -1241,7 +1241,7 @@ mod tests {
let py_dt = CatchWarnings::enter(py, |_| Ok(dt.into_py(py))).unwrap();
let roundtripped: DateTime<Utc> = py_dt.extract(py).expect("Round trip");
// Leap seconds are not roundtripped
let expected_roundtrip_time = micro.checked_sub(1_000_000).map(|micro| NaiveTime::from_hms_micro_opt(hour, min, sec, micro).unwrap()).unwrap_or(time);
let expected_roundtrip_time = micro.checked_sub(1_000_000).map_or(time, |micro| NaiveTime::from_hms_micro_opt(hour, min, sec, micro).unwrap());
let expected_roundtrip_dt: DateTime<Utc> = NaiveDateTime::new(date, expected_roundtrip_time).and_utc();
assert_eq!(expected_roundtrip_dt, roundtripped);
}
Expand Down Expand Up @@ -1269,7 +1269,7 @@ mod tests {
let py_dt = CatchWarnings::enter(py, |_| Ok(dt.into_py(py))).unwrap();
let roundtripped: DateTime<FixedOffset> = py_dt.extract(py).expect("Round trip");
// Leap seconds are not roundtripped
let expected_roundtrip_time = micro.checked_sub(1_000_000).map(|micro| NaiveTime::from_hms_micro_opt(hour, min, sec, micro).unwrap()).unwrap_or(time);
let expected_roundtrip_time = micro.checked_sub(1_000_000).map_or(time, |micro| NaiveTime::from_hms_micro_opt(hour, min, sec, micro).unwrap());
let expected_roundtrip_dt: DateTime<FixedOffset> = NaiveDateTime::new(date, expected_roundtrip_time).and_local_timezone(offset).unwrap();
assert_eq!(expected_roundtrip_dt, roundtripped);
}
Expand Down
15 changes: 8 additions & 7 deletions src/err/mod.rs
View file
Open in desktop
Expand Up @@ -428,9 +428,10 @@ impl PyErr {
let msg = pvalue
.as_ref()
.and_then(|obj| obj.bind(py).str().ok())
.map(|py_str| py_str.to_string_lossy().into())
.unwrap_or_else(|| String::from("Unwrapped panic from Python code"));

.map_or_else(
|| String::from("Unwrapped panic from Python code"),
|py_str| py_str.to_string_lossy().into(),
);
let state = PyErrState::FfiTuple {
ptype,
pvalue,
Expand All @@ -451,10 +452,10 @@ impl PyErr {
let state = PyErrStateNormalized::take(py)?;
let pvalue = state.pvalue.bind(py);
if pvalue.get_type().as_ptr() == PanicException::type_object_raw(py).cast() {
let msg: String = pvalue
.str()
.map(|py_str| py_str.to_string_lossy().into())
.unwrap_or_else(|_| String::from("Unwrapped panic from Python code"));
let msg: String = pvalue.str().map_or_else(
|_| String::from("Unwrapped panic from Python code"),
|py_str| py_str.to_string_lossy().into(),
);
Self::print_panic_and_unwind(py, PyErrState::Normalized(state), msg)
}

Expand Down
14 changes: 4 additions & 10 deletions src/impl_/extract_argument.rs
View file
Open in desktop
Expand Up @@ -32,15 +32,12 @@ where
type Holder = ();

#[inline]
fn extract(obj: &'a Bound<'py, PyAny>, _: &'a mut ()) -> PyResult<Self> {
fn extract(obj: &'a Bound<'py, PyAny>, (): &'a mut ()) -> PyResult<Self> {
obj.extract()
}
}

impl<'a, 'py, T: 'py> PyFunctionArgument<'a, 'py> for &'a Bound<'py, T>
where
T: PyTypeCheck,
{
impl<'a, 'py, T: 'py + PyTypeCheck> PyFunctionArgument<'a, 'py> for &'a Bound<'py, T> {
type Holder = Option<()>;

#[inline]
Expand All @@ -49,14 +46,11 @@ where
}
}

impl<'a, 'py, T: 'py> PyFunctionArgument<'a, 'py> for Option<&'a Bound<'py, T>>
where
T: PyTypeCheck,
{
impl<'a, 'py, T: 'py + PyTypeCheck> PyFunctionArgument<'a, 'py> for Option<&'a Bound<'py, T>> {
type Holder = ();

#[inline]
fn extract(obj: &'a Bound<'py, PyAny>, _: &'a mut ()) -> PyResult<Self> {
fn extract(obj: &'a Bound<'py, PyAny>, (): &'a mut ()) -> PyResult<Self> {
if obj.is_none() {
Ok(None)
} else {
Expand Down
8 changes: 8 additions & 0 deletions src/lib.rs
View file
Open in desktop
Expand Up @@ -19,6 +19,14 @@
non_local_definitions,
)
)))]
#![deny(
clippy::ignored_unit_patterns,
clippy::implicit_clone,
clippy::inefficient_to_string,
clippy::map_unwrap_or,
clippy::redundant_else,
clippy::type_repetition_in_bounds
)]

//! Rust bindings to the Python interpreter.
//!
Expand Down
6 changes: 2 additions & 4 deletions src/marker.rs
View file
Open in desktop
Expand Up @@ -688,10 +688,8 @@ impl<'py> Python<'py> {
return Err(PyErr::fetch(self));
}

let globals = globals
.map(|dict| dict.as_ptr())
.unwrap_or_else(|| ffi::PyModule_GetDict(mptr));
let locals = locals.map(|dict| dict.as_ptr()).unwrap_or(globals);
let globals = globals.map_or_else(|| ffi::PyModule_GetDict(mptr), |dict| dict.as_ptr());
let locals = locals.map_or(globals, |dict| dict.as_ptr());

// If `globals` don't provide `__builtins__`, most of the code will fail if Python
// version is <3.10. That's probably not what user intended, so insert `__builtins__`
Expand Down
2 changes: 1 addition & 1 deletion src/panic.rs
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@ impl PanicException {
pub(crate) fn from_panic_payload(payload: Box<dyn Any + Send + 'static>) -> PyErr {
if let Some(string) = payload.downcast_ref::<String>() {
Self::new_err((string.clone(),))
} else if let Some(s) = payload.downcast_ref::<&str>() {
} else if let Ok(s) = payload.downcast::<&str>() {
Self::new_err((s.to_string(),))
} else {
Self::new_err(("panic from Rust code",))
Expand Down
2 changes: 1 addition & 1 deletion src/pybacked.rs
View file
Open in desktop
Expand Up @@ -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()),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 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.

Copy link
Author

Choose a reason for hiding this comment

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

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

data,
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/pycell.rs
View file
Open in desktop
Expand Up @@ -404,7 +404,7 @@ impl<T: PyClass> PyCell<T> {
self.0
.borrow_checker()
.try_borrow_unguarded()
.map(|_: ()| &*self.0.get_ptr())
.map(|()| &*self.0.get_ptr())
}

/// Provide an immutable borrow of the value `T` without acquiring the GIL.
Expand Down Expand Up @@ -662,15 +662,15 @@ impl<'py, T: PyClass> PyRef<'py, T> {
cell.ensure_threadsafe();
cell.borrow_checker()
.try_borrow()
.map(|_| Self { inner: obj.clone() })
.map(|()| Self { inner: obj.clone() })
}

pub(crate) fn try_borrow_threadsafe(obj: &Bound<'py, T>) -> Result<Self, PyBorrowError> {
let cell = obj.get_class_object();
cell.check_threadsafe()?;
cell.borrow_checker()
.try_borrow()
.map(|_| Self { inner: obj.clone() })
.map(|()| Self { inner: obj.clone() })
}
}

Expand Down Expand Up @@ -859,7 +859,7 @@ impl<'py, T: PyClass<Frozen = False>> PyRefMut<'py, T> {
cell.ensure_threadsafe();
cell.borrow_checker()
.try_borrow_mut()
.map(|_| Self { inner: obj.clone() })
.map(|()| Self { inner: obj.clone() })
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/types/bytearray.rs
View file
Open in desktop
Expand Up @@ -95,7 +95,7 @@ impl PyByteArray {
std::ptr::write_bytes(buffer, 0u8, len);
// (Further) Initialise the bytearray in init
// If init returns an Err, pypybytearray will automatically deallocate the buffer
init(std::slice::from_raw_parts_mut(buffer, len)).map(|_| pybytearray)
init(std::slice::from_raw_parts_mut(buffer, len)).map(|()| pybytearray)
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/types/bytes.rs
View file
Open in desktop
Expand Up @@ -91,7 +91,7 @@ impl PyBytes {
std::ptr::write_bytes(buffer, 0u8, len);
// (Further) Initialise the bytestring in init
// If init returns an Err, pypybytearray will automatically deallocate the buffer
init(std::slice::from_raw_parts_mut(buffer, len)).map(|_| pybytes)
init(std::slice::from_raw_parts_mut(buffer, len)).map(|()| pybytes)
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/types/tuple.rs
View file
Open in desktop
Expand Up @@ -691,14 +691,14 @@ fn type_output() -> TypeInfo {
fn extract_bound(obj: &Bound<'py, PyAny>) -> PyResult<Self>
{
let t = obj.downcast::<PyTuple>()?;
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))
Comment on lines -694 to -701
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

}
}

Expand Down