Re: [PATCH v8 6/6] rust: enable `clippy::ref_as_ptr` lint

[Boqun Feng <boqun.feng@gmail.com>]

On Tue, Apr 15, 2025 at 04:59:01PM -0400, Tamir Duberstein wrote:
[...]
> > > > > > > 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; // <- error
> > >
> > > I 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.
> 

You're right, I had some misunderstanding, the "`_`" part of `as _`
seems to be a red herring, the problematic code snippet you meant can be
shown as (without a `as _`):

	f(x as *mut T); // f() takes a `*mut T`.

where it compiles with `x` being either a `&mut T` or `*const T`, and
`as` has different meanings in these cases.

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

In this case the status quo is not having the lint, which allows users
to convert a raw pointer from a reference with `as`. What you proposed
in patch is to do the conversion with a stand-alone let statement, and
that IMO doesn't suit all the cases: we are dealing with C code a lot,
that means dealing raw pointers a lot, it's handy and logically tight if
we have an expression that converts a Rust location into a raw pointer.
And forcing let statements every time is not really reasonble because of
this.

Also I didn't get the problematic code the lint can prevent as well
until very recent discussion in this thread.

I would not say the status quo is preferable, more like your changes in
this patch complicate some simple patterns which are reasonable to me.
And it's also weird that we use a lint but don't use its suggestion.

So in short, I'm not against this lint, but if we only use let-statement
resolution, I need to understand why and as you said, we need to
evaluate the cost.

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

If the original code use "as" for conversion purposes, I think it's good
to be consistent and using from_ref() or from_mut(): they are just
bullet-proof version of conversions, and having a separate let statement
looks like a distraction to me. If for new code, and the author has a
reason for let statement, then it's fine.

Regards,
Boqun