On Tue, Apr 15, 2025 at 4:51 PM Boqun Feng boqun.feng@gmail.com wrote:
On Tue, Apr 15, 2025 at 04:10:01PM -0400, Tamir Duberstein wrote:
On Tue, Apr 15, 2025 at 2:18 PM Boqun Feng boqun.feng@gmail.com wrote:
On Tue, Apr 15, 2025 at 01:58:41PM -0400, Tamir Duberstein wrote:
Hi Boqun, thanks for having a look!
On Tue, Apr 15, 2025 at 1:37 PM Boqun Feng boqun.feng@gmail.com wrote:
On Wed, Apr 09, 2025 at 10:47:23AM -0400, Tamir Duberstein wrote:
In Rust 1.78.0, Clippy introduced the `ref_as_ptr` lint [1]:
> Using `as` casts may result in silently changing mutability or type.
While this doesn't eliminate unchecked `as` conversions, it makes such conversions easier to scrutinize. It also has the slight benefit of removing a degree of freedom on which to bikeshed. Thus apply the changes and enable the lint -- no functional change intended.
Link: https://rust-lang.github.io/rust-clippy/master/index.html#ref_as_ptr [1] Suggested-by: Benno Lossin benno.lossin@proton.me Link: https://lore.kernel.org/all/D8PGG7NTWB6U.3SS3A5LN4XWMN@proton.me/ Signed-off-by: Tamir Duberstein tamird@gmail.com
Makefile | 1 + rust/bindings/lib.rs | 1 + rust/kernel/device_id.rs | 3 ++- rust/kernel/fs/file.rs | 3 ++- rust/kernel/str.rs | 6 ++++-- rust/kernel/uaccess.rs | 10 ++++------ rust/uapi/lib.rs | 1 + 7 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/Makefile b/Makefile index eb5a942241a2..2a16e02f26db 100644 --- a/Makefile +++ b/Makefile @@ -485,6 +485,7 @@ export rust_common_flags := --edition=2021 \ -Wclippy::no_mangle_with_rust_abi \ -Wclippy::ptr_as_ptr \ -Wclippy::ptr_cast_constness \
-Wclippy::ref_as_ptr \ -Wclippy::undocumented_unsafe_blocks \ -Wclippy::unnecessary_safety_comment \ -Wclippy::unnecessary_safety_doc \diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs index b105a0d899cc..2b69016070c6 100644 --- a/rust/bindings/lib.rs +++ b/rust/bindings/lib.rs @@ -27,6 +27,7 @@ #[allow(dead_code)] #[allow(clippy::cast_lossless)] #[allow(clippy::ptr_as_ptr)] +#[allow(clippy::ref_as_ptr)] #[allow(clippy::undocumented_unsafe_blocks)] mod bindings_raw { // Manual definition for blocklisted types. diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs index 4063f09d76d9..37cc03d1df4c 100644 --- a/rust/kernel/device_id.rs +++ b/rust/kernel/device_id.rs @@ -136,7 +136,8 @@ impl<T: RawDeviceId, U, const N: usize> IdTable<T, U> for IdArray<T, U, N> { fn as_ptr(&self) -> *const T::RawType { // This cannot be `self.ids.as_ptr()`, as the return pointer must have correct provenance // to access the sentinel.
(self as *const Self).cast()
let this: *const Self = self;Hmm.. so this lint usually just requires to use a let statement instead of as expression when casting a reference to a pointer? Not 100% convinced this results into better code TBH..
The rationale is in the lint description and quoted in the commit message: "Using `as` casts may result in silently changing mutability or type.".
Could you show me how you can silently change the mutability or type? A simple try like below doesn't compile:
let x = &42; let ptr = x as *mut i32; // <- error let another_ptr = x as *const i64; // <- errorI think the point is that the meaning of an `as` cast can change when the type of `x` changes, which can happen at a distance. The example
So my example shows that you can only use `as` to convert a `&T` into a `*const T`, no matter how far it happens, and..
I don't think you're engaging with the point I'm making here. Suppose the type is `&mut T` initially and `as _` is being used to convert it to `*mut T`; now if the type of `&mut T` changes to `*const T`, you have completely different semantics.
shown in the clippy docs uses `as _`, which is where you get into real trouble.
... no matter whether `as _` is used or not. Of course once you have a `*const T`, using `as` can change it to a different type or mutability, but that's a different problem. Your argument still lacks convincing evidences or examples showing this is a real trouble. For example, if you have a `x` of type `&i32`, and do a `x as _` somewhere, you will have a compiler error once compilers infers a type that is not `*const i32` for `_`. If your argument being it's better do the reference-to-pointer conversion explicitly, then that makes some sense, but I still don't think we need to do it globablly.
Can you help me understand what it is I need to convince you of? There was prior discussion in https://lore.kernel.org/all/D8PGG7NTWB6U.3SS3A5LN4XWMN@proton.me/, where it was suggested to use this lint.
I suppose in any discussion of a chance, we should also enumerate the costs -- you're taking the position that the status quo is preferable, yes? Can you help me understand why?
also from the link document you shared, looks like the suggestion is to use core::ptr::from_{ref,mut}(), was this ever considered?
I considered it, but I thought it was ugly. We don't have a linter to enforce it, so I'd be surprised if people reached for it.
I think avoiding the extra line of `let` is a win, also I don't get why you feel it's *ugly*: having the extra `let` line is ugly to me ;-)
I admit it's subjective, so I'm happy to change it. But I've never seen that syntax used, and we lack enforcement for either one, so I don't find much value in arguing over this.