Thread (38 messages) 38 messages, 3 authors, 2025-03-14

Re: [PATCH v2 5/5] rust: enable `clippy::as_underscore` lint

From: Benno Lossin <hidden>
Date: 2025-03-13 18:12:16
Also in: linux-devicetree, linux-kbuild, linux-kselftest, linux-pci, lkml, rust-for-linux

On Thu Mar 13, 2025 at 6:50 PM CET, Tamir Duberstein wrote:
On Thu, Mar 13, 2025 at 10:11 AM Benno Lossin [off-list ref] wrote:
quoted
On Thu Mar 13, 2025 at 11:47 AM CET, Tamir Duberstein wrote:
quoted
On Wed, Mar 12, 2025 at 6:38 PM Benno Lossin [off-list ref] wrote:
quoted
On Wed Mar 12, 2025 at 11:24 PM CET, Tamir Duberstein wrote:
quoted
On Wed, Mar 12, 2025 at 5:30 PM Benno Lossin [off-list ref] wrote:
quoted
On Wed Mar 12, 2025 at 10:10 PM CET, Tamir Duberstein wrote:
quoted
On Wed, Mar 12, 2025 at 5:04 PM Tamir Duberstein [off-list ref] wrote:
quoted
On Wed, Mar 12, 2025 at 5:01 PM Benno Lossin [off-list ref] wrote:
quoted
Always enable the features, we have `allow(stable_features)` for this
reason (then you don't have to do this dance with checking if it's
already stable or not :)
It's not so simple. In rustc < 1.84.0 the lints *and* the strict
provenance APIs are behind `feature(strict_provenance)`. In rustc >=
1.84.0 the lints are behind `feature(strict_provenance_lints)`. So you
need to read the config to learn that you need to enable
`feature(strict_provenance_lints)`.
I see... And `strict_provenance_lints` doesn't exist in <1.84? That's a
bit of a bummer...

But I guess we could have this config option (in `init/Kconfig`):

    config RUSTC_HAS_STRICT_PROVENANCE
            def_bool RUSTC_VERSION >= 108400

and then do this in `lib.rs`:

    #![cfg_attr(CONFIG_RUSTC_HAS_STRICT_PROVENANCE, feature(strict_provenance_lints))]
Yep! That's exactly what I did, but as I mentioned up-thread, the
result is that we only cover the `kernel` crate.
Ah I see, can't we just have the above line in the other crate roots?
The most difficult case is doctests. You'd have to add this to every
example AFAICT.
quoted
quoted
quoted
quoted
Actually this isn't even the only problem. It seems that
`-Astable_features` doesn't affect features enabled on the command
line at all:

error[E0725]: the feature `strict_provenance` is not in the list of
allowed features
 --> <crate attribute>:1:9
  |
1 | feature(strict_provenance)
  |         ^^^^^^^^^^^^^^^^^
That's because you need to append the feature to `rust_allowed_features`
in `scripts/Makefile.build` (AFAIK).
Thanks, that's a helpful pointer, and it solves some problems but not
all. The root Makefile contains this bit:
quoted
KBUILD_HOSTRUSTFLAGS := $(rust_common_flags) -O -Cstrip=debuginfo \
-Zallow-features= $(HOSTRUSTFLAGS)
which means we can't use the provenance lints against these host
targets (including e.g. `rustdoc_test_gen`). We can't remove this
-Zallow-features= either because then core fails to compile.

I'm at the point where I think I need more involved help. Want to take
a look at my attempt? It's here:
https://github.com/tamird/linux/tree/b4/ptr-as-ptr.
With doing `allow(clippy::incompatible_msrv)`, I meant doing that
globally, not having a module to re-export the functions :)
Yeah, but I think that's too big a hammer. It's a useful warning, and
it doesn't accept per-item configuration.
Hmm, I don't think it's as useful. We're going to be using more unstable
features until we eventually bump the minimum version when we can
disable `RUSTC_BOOTSTRAP=1`. From that point onwards, it will be very
useful, but before that I don't think that it matters too much. Maybe
the others disagree.
quoted
quoted
quoted
I'll take a look tomorrow, you're testing my knowledge of the build
system a lot here :)
We're guaranteed to learn something :)
Yep! I managed to get it working, but it is rather janky and
experimental. I don't think you should use this in your patch series
unless Miguel has commented on it.

Notable things in the diff below:
* the hostrustflags don't get the *provenance_casts lints (which is
  correct, I think, but probably not the way I did it with filter-out)
* the crates compiler_builtins, bindings, uapi, build_error, libmacros,
  ffi, etc do get them, but probably shouldn't?
Why don't we want host programs to have the same warnings applied? Why
don't we want it for all those other crates?
I have never looked at the rust hostprogs before, so I'm missing some
context here.

I didn't enable them, because they are currently being compiled without
any unstable features and I thought we might want to keep that. (though
I don't really see a reason not to compile them with unstable features
that we also use for the kernel crate)
I'd expect we want uniform diagnostics settings throughout which is
why these things are in the Makefile rather than in individual crates
in the first place.

Your patch sidesteps the problems I'm having by not applying these
lints to host crates, but I think we should.
We're probably working on some stuff that Miguel's new build system will
do entirely differently. So I wouldn't worry too much about getting it
perfect, as it will be removed in a couple cycles.

---
Cheers,
Benno
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help