mbox series

[v2,0/5] rust: reduce pointer casts, enable related lints

Message ID 20250309-ptr-as-ptr-v2-0-25d60ad922b7@gmail.com
Headers show
Series rust: reduce pointer casts, enable related lints | expand

Message

Tamir Duberstein March 9, 2025, 4 p.m. UTC
This started with a patch that enabled `clippy::ptr_as_ptr`. Benno
Lossin suggested I also look into `clippy::ptr_cast_constness` and I
discovered `clippy::as_ptr_cast_mut`. This series now enables all 3
lints. It also enables `clippy::as_underscore` which ensures other
pointer casts weren't missed. The first commit reduces the need for
pointer casts and is shared with another series[1].

Link: https://lore.kernel.org/all/20250307-no-offset-v1-0-0c728f63b69c@gmail.com/ [1]

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
Changes in v2:
- Fixed typo in first commit message.
- Added additional patches, converted to series.
- Link to v1: https://lore.kernel.org/r/20250307-ptr-as-ptr-v1-1-582d06514c98@gmail.com

---
Tamir Duberstein (5):
      rust: retain pointer mut-ness in `container_of!`
      rust: enable `clippy::ptr_as_ptr` lint
      rust: enable `clippy::ptr_cast_constness` lint
      rust: enable `clippy::as_ptr_cast_mut` lint
      rust: enable `clippy::as_underscore` lint

 Makefile                               |  4 ++++
 rust/bindings/lib.rs                   |  1 +
 rust/kernel/alloc/allocator_test.rs    |  2 +-
 rust/kernel/alloc/kvec.rs              |  4 ++--
 rust/kernel/block/mq/operations.rs     |  2 +-
 rust/kernel/block/mq/request.rs        |  7 ++++---
 rust/kernel/device.rs                  |  5 +++--
 rust/kernel/device_id.rs               |  2 +-
 rust/kernel/devres.rs                  | 19 ++++++++++---------
 rust/kernel/error.rs                   |  2 +-
 rust/kernel/firmware.rs                |  2 +-
 rust/kernel/fs/file.rs                 |  2 +-
 rust/kernel/io.rs                      | 18 +++++++++---------
 rust/kernel/kunit.rs                   | 15 +++++++--------
 rust/kernel/lib.rs                     |  5 ++---
 rust/kernel/list/impl_list_item_mod.rs |  2 +-
 rust/kernel/miscdevice.rs              |  2 +-
 rust/kernel/of.rs                      |  6 +++---
 rust/kernel/pci.rs                     | 13 ++++++++-----
 rust/kernel/platform.rs                |  6 ++++--
 rust/kernel/print.rs                   | 11 +++++------
 rust/kernel/rbtree.rs                  | 23 ++++++++++-------------
 rust/kernel/seq_file.rs                |  3 ++-
 rust/kernel/str.rs                     | 10 +++++-----
 rust/kernel/sync/poll.rs               |  2 +-
 rust/kernel/workqueue.rs               | 12 ++++++------
 rust/uapi/lib.rs                       |  1 +
 27 files changed, 95 insertions(+), 86 deletions(-)
---
base-commit: ff64846bee0e7e3e7bc9363ebad3bab42dd27e24
change-id: 20250307-ptr-as-ptr-21b1867fc4d4

Best regards,

Comments

Benno Lossin March 12, 2025, 3:07 p.m. UTC | #1
Hi Tamir,

On Sun Mar 9, 2025 at 5:00 PM CET, Tamir Duberstein wrote:
> This started with a patch that enabled `clippy::ptr_as_ptr`. Benno
> Lossin suggested I also look into `clippy::ptr_cast_constness` and I
> discovered `clippy::as_ptr_cast_mut`. This series now enables all 3
> lints. It also enables `clippy::as_underscore` which ensures other
> pointer casts weren't missed. The first commit reduces the need for
> pointer casts and is shared with another series[1].
>
> Link: https://lore.kernel.org/all/20250307-no-offset-v1-0-0c728f63b69c@gmail.com/ [1]
>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>

Thanks for this series! Did you encounter any instances of `$val as $ty`
where you couldn't convert them due to unsizing? I remember that we had
some cases back then (maybe Alice remembers them too?). If not then no
worries :)

---
Cheers,
Benno
Tamir Duberstein March 12, 2025, 3:14 p.m. UTC | #2
On Wed, Mar 12, 2025 at 11:07 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> Hi Tamir,
>
> On Sun Mar 9, 2025 at 5:00 PM CET, Tamir Duberstein wrote:
> > This started with a patch that enabled `clippy::ptr_as_ptr`. Benno
> > Lossin suggested I also look into `clippy::ptr_cast_constness` and I
> > discovered `clippy::as_ptr_cast_mut`. This series now enables all 3
> > lints. It also enables `clippy::as_underscore` which ensures other
> > pointer casts weren't missed. The first commit reduces the need for
> > pointer casts and is shared with another series[1].
> >
> > Link: https://lore.kernel.org/all/20250307-no-offset-v1-0-0c728f63b69c@gmail.com/ [1]
> >
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
>
> Thanks for this series! Did you encounter any instances of `$val as $ty`
> where you couldn't convert them due to unsizing? I remember that we had
> some cases back then (maybe Alice remembers them too?). If not then no
> worries :)

I didn't :)

Thank you for the reviews!
Tamir Duberstein March 12, 2025, 9:04 p.m. UTC | #3
On Wed, Mar 12, 2025 at 5:01 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Wed Mar 12, 2025 at 9:41 PM CET, Tamir Duberstein wrote:
> > On Wed, Mar 12, 2025 at 4:07 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >>
> >> On Wed, Mar 12, 2025 at 3:43 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >> >
> >> > On Wed Mar 12, 2025 at 8:19 PM CET, Tamir Duberstein wrote:
> >> > > I tried using the strict provenance lints locally and I think we can't
> >> > > until we properly bump MSRV due to `clippy::incompatible_msrv`:
> >> > >
> >> > > warning: current MSRV (Minimum Supported Rust Version) is `1.78.0` but
> >> > > this item is stable since `1.84.0`
> >> > >    --> ../rust/kernel/str.rs:696:22
> >> > >     |
> >> > > 696 |             pos: pos.expose_provenance(),
> >> > >     |                      ^^^^^^^^^^^^^^^^^^^
> >> > >     |
> >> > >     = help: for further information visit
> >> > > https://rust-lang.github.io/rust-clippy/master/index.html#incompatible_msrv
> >> >
> >> > Oh this is annoying...
> >> >
> >> > > This is with `#![feature(strict_provenance)]`. I can file the issue
> >> > > but I think it's blocked on MSRV >= 1.84.0. But maybe you know of a
> >> > > path forward :)
> >> >
> >> > I think we should be able to just `allow(clippy::incompatible_msrv)`,
> >> > since Miguel & other maintainers will test with 1.78 (or at least are
> >> > supposed to :).
> >>
> >> Alright, you've sniped me. This is coming in v3.
> >
> > I just realized I only covered the kernel crate. In order to cover all
> > Rust code, I need to move the lints and the features out to the root
> > Makefile. I tried something like this:
> >
> >> diff --git a/Makefile b/Makefile
> >> index 2af40bfed9ce..10af1e44370b 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -466,13 +466,21 @@ KBUILD_USERHOSTCFLAGS := -Wall -Wmissing-prototypes -Wstrict-prototypes \
> >>  KBUILD_USERCFLAGS  := $(KBUILD_USERHOSTCFLAGS) $(USERCFLAGS)
> >>  KBUILD_USERLDFLAGS := $(USERLDFLAGS)
> >>
> >> +# Lints were moved to `strict_provenance_lints` when `strict_provenance` was stabilized.
> >> +#
> >> +# See https://github.com/rust-lang/rust/commit/56ee492a6e7a917b2b3f888e33dd52a13d3ecb64.
> >> +KBUILD_RUST_STRICT_PROVENANCE_FEATURE = $(if $(CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE),strict_provenance_lints,strict_provenance)
> >> +
> >>  # These flags apply to all Rust code in the tree, including the kernel and
> >>  # host programs.
> >>  export rust_common_flags := --edition=2021 \
> >>      -Zbinary_dep_depinfo=y \
> >> +     -Zcrate-attr="feature($(KBUILD_RUST_STRICT_PROVENANCE_FEATURE))" \
> >>      -Astable_features \
> >>      -Dnon_ascii_idents \
> >>      -Dunsafe_op_in_unsafe_fn \
> >> +     -Wfuzzy_provenance_casts \
> >> +     -Wlossy_provenance_casts \
> >>      -Wmissing_docs \
> >>      -Wrust_2018_idioms \
> >>      -Wunreachable_pub \
> >> diff --git a/rust/Makefile b/rust/Makefile
> >> index ea3849eb78f6..d7d5be741245 100644
> >> --- a/rust/Makefile
> >> +++ b/rust/Makefile
> >> @@ -435,8 +435,10 @@ $(obj)/helpers/helpers.o: $(src)/helpers/helpers.c $(recordmcount_source) FORCE
> >>  # symbol versions generated from Rust objects.
> >>  $(obj)/exports.o: private skip_gendwarfksyms = 1
> >>
> >> +KBUILD_RUST_STRICT_PROVENANCE_FEATURE := $(if $(CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE),strict_provenance_lints,strict_provenance)
> >> +
> >>  $(obj)/core.o: private skip_clippy = 1
> >> -$(obj)/core.o: private skip_flags = -Wunreachable_pub
> >> +$(obj)/core.o: private skip_flags = -Zcrate-attr="feature($(KBUILD_RUST_STRICT_PROVENANCE_FEATURE))" -Wunreachable_pub -Wfuzzy_provenance_casts -Wlossy_provenance_casts
> >>  $(obj)/core.o: private rustc_objcopy = $(foreach sym,$(redirect-intrinsics),--redefine-sym $(sym)=__rust$(sym))
> >>  $(obj)/core.o: private rustc_target_flags = $(core-cfgs)
> >>  $(obj)/core.o: $(RUST_LIB_SRC)/core/src/lib.rs \
> >
> > but this doesn't work because
> > `CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE` is not yet defined when I
> > read it in the root Makefile. I can read it lower down and then append
> > the feature flag to `KBUILD_RUSTFLAGS` but by then the rustdoc flags
> > have been copied from `rust_common_flags` and so rustdoc doesn't get
> > the feature flag, resulting in unknown lint warnings in rustdoc and
> > kunit tests.
> >
> > Any ideas?
>
> 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)`.
Tamir Duberstein March 12, 2025, 10:24 p.m. UTC | #4
On Wed, Mar 12, 2025 at 5:30 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Wed Mar 12, 2025 at 10:10 PM CET, Tamir Duberstein wrote:
> > On Wed, Mar 12, 2025 at 5:04 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >>
> >> On Wed, Mar 12, 2025 at 5:01 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >> > 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.

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

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

Thanks!
Tamir
Tamir Duberstein March 13, 2025, 5:50 p.m. UTC | #5
On Thu, Mar 13, 2025 at 10:11 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Thu Mar 13, 2025 at 11:47 AM CET, Tamir Duberstein wrote:
> > On Wed, Mar 12, 2025 at 6:38 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >>
> >> On Wed Mar 12, 2025 at 11:24 PM CET, Tamir Duberstein wrote:
> >> > On Wed, Mar 12, 2025 at 5:30 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >> >>
> >> >> On Wed Mar 12, 2025 at 10:10 PM CET, Tamir Duberstein wrote:
> >> >> > On Wed, Mar 12, 2025 at 5:04 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >> >> >>
> >> >> >> On Wed, Mar 12, 2025 at 5:01 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >> >> >> > 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.
> >
> >> >> > 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:
> >> >
> >> >> 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.

> >> 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'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.
Benno Lossin March 13, 2025, 6:12 p.m. UTC | #6
On Thu Mar 13, 2025 at 6:50 PM CET, Tamir Duberstein wrote:
> On Thu, Mar 13, 2025 at 10:11 AM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On Thu Mar 13, 2025 at 11:47 AM CET, Tamir Duberstein wrote:
>> > On Wed, Mar 12, 2025 at 6:38 PM Benno Lossin <benno.lossin@proton.me> wrote:
>> >>
>> >> On Wed Mar 12, 2025 at 11:24 PM CET, Tamir Duberstein wrote:
>> >> > On Wed, Mar 12, 2025 at 5:30 PM Benno Lossin <benno.lossin@proton.me> wrote:
>> >> >>
>> >> >> On Wed Mar 12, 2025 at 10:10 PM CET, Tamir Duberstein wrote:
>> >> >> > On Wed, Mar 12, 2025 at 5:04 PM Tamir Duberstein <tamird@gmail.com> wrote:
>> >> >> >>
>> >> >> >> On Wed, Mar 12, 2025 at 5:01 PM Benno Lossin <benno.lossin@proton.me> wrote:
>> >> >> >> > 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.
>> >
>> >> >> > 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:
>> >> >
>> >> >> 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.

>> >> 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
Tamir Duberstein March 14, 2025, 12:26 p.m. UTC | #7
On Thu, Mar 13, 2025 at 2:12 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Thu Mar 13, 2025 at 6:50 PM CET, Tamir Duberstein wrote:
> > On Thu, Mar 13, 2025 at 10:11 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >>
> >>
> >> 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.

I'd rather keep this narrowly scoped for now -- putting the genie back
in the bottle later is usually harder.

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

I got it working, but it's pretty messy. Let's discuss on v3.