Fix multiple clippy issues #143423

hkBst · 2025-07-04T10:46:45Z

int_log10.rs: change top level doc comments to outer
collect.rs: remove empty line after doc comment
clippy fix: markdown indentation for indented items after line break: a markdown list item continued over multiples lines, but those following lines which are part of the same item are not indented
clippy fix: bound in one place: when there is a bound in angle brackets and another bound on the same variable in a where clause
flt2dec: fix some clippy lints: a manual index being used, and it is only used for directly indexing into the thing iterated over, so why iterate over the things directly instead of the indices and a manual is_empty impl .len() == 0.

rustbot · 2025-07-04T10:46:49Z

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-07-04T10:46:51Z

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred to the platform-builtins intrinsics. Make sure the
LLVM backend as well as portable-simd gets adapted for the changes.

cc @antoyo, @GuillaumeGomez, @bjorn3, @calebzulawski, @programmerjake

oli-obk · 2025-07-04T13:03:52Z

do we run a subset of clippy lints in CI?
Should we extend that subset as to not regress this in the future again?

Is there some overarching issue tracking this work?

jhpratt · 2025-07-05T07:54:57Z

regarding overarching issue, see #143367 (comment)

hkBst · 2025-07-05T11:36:18Z

@oli-obk I don't know if any clippy lints run in CI, but I do know that many don't, given the amount of warnings generated by clippy atm. Given the volume of warnings it is very hard to know if there is anything useful in there. Clippy can be a very useful tool in my experience, so I'd like to slowly cut down on the noise. If that succeeds then it seems a good idea to enable clippy in CI to keep the warnings down. @jhpratt has suggested that I should obtain some support/approval for this. I've started my work on core, with std and the compiler possibly after. So @rust-lang/libs, @rust-lang/compiler , can I ask that you consider this?

klensy · 2025-07-05T12:28:43Z

Clippy is run in CI https://github.com/rust-lang/rust/blob/master/src/bootstrap/src/core/build_steps/clippy.rs.

Not member, but if some documentation clean is probably ok, changes like 819d4a3 not immediately obviously (for me) better, they (mostly) miss codegen tests and fixing clippy lint can instead make code slower.

hkBst · 2025-07-05T14:09:46Z

Not member, but if some documentation clean is probably ok, changes like 819d4a3 not immediately obviously (for me) better, they (mostly) miss codegen tests and fixing clippy lint can instead make code slower.

This is true, so there is always the alternative of allowing (or expecting) the lint to silence clippy.

yotamofek · 2025-07-05T22:18:15Z

I asked about this on zulip a while back,
got some pretty negative reactions (which honestly I'm a bit more in agreement with today), might be an interesting read for anyone going down this path:
https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/Running.20clippy.20on.20the.20rustc.20repo/near/504646920

hkBst · 2025-07-06T06:42:10Z

@yotamofek thanks for the link. Personally I found the discussion there quite encouraging and I feel supported in working on this. It was very useful to learn about where clippy is already applied in CI, which gives the precise list of lints that are currently enabled.

I am not at all proposing to enable all default lints. Right now, all I really need is that it is deemed acceptable to allow lints locally. This would also prevent future bad PRs from people mindlessly following clippy, which was one of the things that were brought up as clippy negatives. This could make it possible to at some point change the global list.

Another point brought up was that the default list changes (too much) with each clippy update to enable all lints from the default list. My hope is that more interaction between clippy and rustc will make both better. One example of clippy getting better is: rust-lang/rust-clippy#15200. That change alone prevents hundreds of false positives in rustc, once the next clippy update is merged. And maybe we can get to a point where changes to the default lint list will be checked against rustc before being made. rustc could be a lithmus test for clippy, improving its quality, while at the same time we can try to take more and more advantage of clippy where it works for rustc.

Anyway, my current plan is to work through what clippy reports for core. Make changes where appropriate, but also allow lints where they seem bad or don't apply, and report false positives upstream. Along the way I hope to get a better sense of what's possible.

apiraino · 2025-07-06T09:08:30Z

@hkBst Could you please update your PR initial comment with a recap of the lints you added, possibly with a little bit of context about the reasoning? That could help casual reviewers of this patch get a quick summary. Thanks 🙂

hkBst · 2025-07-06T10:25:53Z

@apiraino I tried to separate out the different parts into separate commits, which name the thing that clippy complains about, but I will mention that in the summary. I think that would help with most of them, except for one which I will try and explain a bit more.

I ended up listing the commits with a small explanation for each.

yotamofek · 2025-07-06T10:41:32Z

I am not at all proposing to enable all default lints. Right now, all I really need is that it is deemed acceptable to allow lints locally.

Personally (and my opinion really shouldn't matter that much to anyone 😝) I don't like seeing #[allows] in code, especially if it doesn't seem likely that the lints being ignored will ever be checked in CI (see discussion about the CI runtime on that zulip thread). Certain lints that are more likely to be accepted as gates for PRs - sure, otherwise, it's just adds visual noise, the way I see it.

Other than that, yeah, I'm happy to see lints being fixed in the codebase.

hkBst · 2025-07-06T10:59:44Z

@yotamofek the allow(..) annotations do add a tiny bit of noise, but what we get in return is fewer PRs from people trying to fix what is not broken, and potentially making it possible to enable these lints later. So the question for the project is whether that is an acceptable trade-off or not, or perhaps whether they are willing to accept it for now and see where it goes.

jieyouxu

I'm not a library reviewer, but speaking as a reviewer generally. I feel like instead of shotgun-applying a bunch of clippy lints, I would strongly encourage the approach of #general > Running clippy on the rustc repo @ 💬:

fwiw when i made x clippy work i very intentionally did not try to fix any of the lints, because i think it would be a shame if we blindly applied them all.

i suspect you are right that there are maybe 5-15 lints we could turn on that would be useful and worth the time to fix. but i would be strongly against just doing RUSTFLAGS=-Dwarnings x clippy because most of them are just not that important

figuring out what those 5-15 lints are would be a good use of time though :) "i think this lint should be on because X" is much more useful than "cargo fix changed the code and i made a pr with the diff"

And if these subset of clippy lints reviewers find generally applicable and materially improves readability (or some other metric), we should include them into the ./x clippy ci subset so that they're actually CI-checked.

I don't subscribe to the argument of

the allow(..) annotations do add a tiny bit of noise, but what we get in return is fewer PRs from people trying to fix what is not broken, and potentially making it possible to enable these lints later.

I'll take changes which I find materially better, and having a lot of #[allow(clippy::*)] unchecked in CI I don't find is materially better, they just seem noisy...

Even

deemed acceptable to allow lints locally

I don't think justifies having a lot of #[allow(clippy::*)] in the codebase, especially if the allowed clippy lints are non-considerations.

jieyouxu · 2025-07-06T11:34:42Z

library/core/src/cmp.rs

+fn default_chaining_impl<T: PartialOrd<U> + PointeeSized, U: PointeeSized>(
    lhs: &T,
    rhs: &U,
    p: impl FnOnce(Ordering) -> bool,
-) -> ControlFlow<bool>
-where
-    T: PartialOrd<U>,
-{
+) -> ControlFlow<bool> {


Remark: I don't find this materially better

I think this is slightly better, but perhaps not rising to materially.

Please move everything into the where clauses instead.

I think the lint is pretty valid in that it's nicer to read all bounds in either the where or the generics, not split. 2cents: I think when there are multiple bounds on multiple generics, it usually looks nicer to do the opposite fix and move them all to where.

(whoops, race with Jubilee)

jieyouxu · 2025-07-06T11:35:04Z

library/core/src/mem/mod.rs

@@ -803,6 +803,7 @@ pub const fn swap<T>(x: &mut T, y: &mut T) {
 #[inline]
 #[stable(feature = "mem_take", since = "1.40.0")]
 pub fn take<T: Default>(dest: &mut T) -> T {
+    #[allow(clippy::mem_replace_with_default)] // exempt by being the one true definition


Remark: or having a lot of #[allow(clippy::*)] that we don't run in CI

jieyouxu · 2025-07-06T11:35:49Z

library/core/src/num/fmt.rs

+                } else if v < 10_000 {
+                    4
                } else {
-                    if v < 10_000 { 4 } else { 5 }
+                    5
                }


Remark: and debatable, but IMO this isn't materially better either.

IMO the vertical spacing is unfortunate, but eliminating extra braces seems worthwhile, no?

This is open-coding a log10 operation, cleaned this up here: #143540

@yotamofek good move! I can't wait to see perf results.

fee1-dead · 2025-07-06T11:47:12Z

if you are looking for approval to insert allow directives for clippy, consider opening a compiler MCP: https://forge.rust-lang.org/triagebot/major-changes.html

fee1-dead · 2025-07-06T11:52:31Z

.. wait these are library changes. so a library MCP or something. sorry im on mobile

workingjubilee · 2025-07-06T20:39:00Z

When doing library work, clippy admonishing me about implementing something by-hand that is available in std is useless noise. I do not wish for these lints to be enabled for std, as we often must by-hand reimplement parts of std in order to guarantee good performance.

workingjubilee

I would like to see these land:

I am not really interested in the rest or would like to see changes at least, so please separate those out into another PR and I can r+ this.

workingjubilee · 2025-07-06T20:41:17Z

library/core/src/num/f32.rs

    pub const NAN: f32 = 0.0_f32 / 0.0_f32;
    /// Infinity (∞).
    #[stable(feature = "assoc_int_consts", since = "1.43.0")]
+    #[allow(clippy::zero_divided_by_zero)]


The dividend isn't even zero. Why is clippy's "zero_divided_by_zero" lint firing on a division of not-zero by zero?

workingjubilee · 2025-07-06T20:45:43Z

library/core/src/num/fmt.rs

@@ -19,6 +19,7 @@ pub enum Part<'a> {

 impl<'a> Part<'a> {
    /// Returns the exact byte length of given part.
+    #[allow(clippy::len_without_is_empty)]


No, I do not want this lint on internals of std.

workingjubilee · 2025-07-06T20:46:22Z

library/core/src/num/flt2dec/mod.rs

            None
        }
-        None if d.len() > 0 => {
+        None if !d.is_empty() => {


I do not find !x.is_empty() easier to read than d.len() > 0, frankly.

I sympathize with that statement, but is_empty is in std, presumably because it makes it easier to express intent and thus be clearer and thus also easier to read. If the not is a problem, then this branch can always be switched with the next, I suppose.

workingjubilee · 2025-07-06T20:47:05Z

library/core/src/num/dec2flt/lemire.rs

@@ -55,7 +55,7 @@ pub fn compute_float<F: RawFloat>(q: i64, mut w: u64) -> BiasedFp {
        // <https://arxiv.org/pdf/2101.11408.pdf#section.9.1>. For detailed
        // explanations of rounding for positive exponents, see
        // <https://arxiv.org/pdf/2101.11408.pdf#section.8>.
-        let inside_safe_exponent = (q >= -27) && (q <= 55);
+        let inside_safe_exponent = (-27..=55).contains(&q);


It is extremely unlikely this is equivalent codegen in such a sensitive case.

We have #45222 as the classic example of inclusive ranges being optimized worse. I would hope that since the range bounds are constant this use gets optimized well. But I appreciate the caution because the range types are quite complicated now.

Was curious so I checked, and it seems both variations optimize down to the same 3 instructions: https://godbolt.org/z/6WP87jPK9
(that, though, doesn't necessarily mean it will codegen the same in this context, inside a larger function, obviously...)

Given the specifics, this can easily be switched to (-27..56).contains(&q). I was just trying to be conservative.

workingjubilee · 2025-07-06T20:48:46Z

library/core/src/cmp.rs

+fn default_chaining_impl<T: PartialOrd<U> + PointeeSized, U: PointeeSized>(
    lhs: &T,
    rhs: &U,
    p: impl FnOnce(Ordering) -> bool,
-) -> ControlFlow<bool>
-where
-    T: PartialOrd<U>,
-{
+) -> ControlFlow<bool> {


Please move everything into the where clauses instead.

jhpratt · 2025-07-07T01:22:49Z

While Jubilee took over assignment, I would still prefer that T-libs weigh in before there are numerous PRs like this merged. That was the reason I had explicitly not reviewed any of ones already open.

Per asking on Zulip, nominating for the general case of whether we want clippy on the standard library. If so, which ones and/or how should reviewers determine what is appropriate and what PRs to close?

workingjubilee · 2025-07-07T07:12:22Z

Yes, that's why I singled out the commits that are formatting-only changes. I do not think a few indentation fixups are worth larger discussion.

hkBst · 2025-07-08T12:19:48Z

I've abandoned adding clippy annotations, since these don't seem to have any support. Moving forwards, I will see what can be done about clippy lints, without introducing such annotations.

flt2dec/mod.rs is probably the most (and only?) contentious change of this PR.

rustbot assigned jhpratt Jul 4, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 4, 2025

rust-cloud-vms bot force-pushed the clippy-fix-1 branch 2 times, most recently from 679953e to 1e803f0 Compare July 4, 2025 10:55

rust-cloud-vms bot force-pushed the clippy-fix-1 branch from 1e803f0 to 76d9775 Compare July 6, 2025 10:42

jieyouxu reviewed Jul 6, 2025

View reviewed changes

workingjubilee requested changes Jul 6, 2025

View reviewed changes

workingjubilee assigned workingjubilee and unassigned jhpratt Jul 6, 2025

jhpratt added the I-libs-nominated Nominated for discussion during a libs team meeting. label Jul 7, 2025

int_log10.rs: change top level doc comments to outer

004478a

hkBst added 3 commits July 8, 2025 11:48

collect.rs: remove empty line after doc comment

f96d5d9

clippy fix: markdown indentation for indented items after line break

ca4a712

clippy fix: bound in one place

093831b

rust-cloud-vms bot force-pushed the clippy-fix-1 branch from 76d9775 to ce51089 Compare July 8, 2025 11:48

flt2dec: fix some clippy lints

dc23317

rust-cloud-vms bot force-pushed the clippy-fix-1 branch from ce51089 to dc23317 Compare July 8, 2025 12:12

Fix multiple clippy issues #143423

Are you sure you want to change the base?

Fix multiple clippy issues #143423

Conversation

hkBst commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 4, 2025

Uh oh!

rustbot commented Jul 4, 2025

Uh oh!

oli-obk commented Jul 4, 2025

Uh oh!

jhpratt commented Jul 5, 2025

Uh oh!

hkBst commented Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klensy commented Jul 5, 2025

Uh oh!

hkBst commented Jul 5, 2025

Uh oh!

yotamofek commented Jul 5, 2025

Uh oh!

hkBst commented Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apiraino commented Jul 6, 2025

Uh oh!

hkBst commented Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yotamofek commented Jul 6, 2025

Uh oh!

hkBst commented Jul 6, 2025

Uh oh!

jieyouxu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgross35 Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fee1-dead commented Jul 6, 2025

Uh oh!

fee1-dead commented Jul 6, 2025

Uh oh!

workingjubilee commented Jul 6, 2025

Uh oh!

workingjubilee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hkBst commented Jul 4, 2025 •

edited

Loading

hkBst commented Jul 5, 2025 •

edited

Loading

hkBst commented Jul 6, 2025 •

edited

Loading

hkBst commented Jul 6, 2025 •

edited

Loading

jieyouxu left a comment •

edited

Loading

tgross35 Jul 6, 2025 •

edited

Loading

yotamofek Jul 6, 2025 •

edited

Loading

workingjubilee commented Jul 7, 2025 •

edited

Loading