Enforce selected clippy lints, and small style fixes

[samueltardieu]

Right now, we only use the basic Clippy lints. While many software opt to use clippy::pedantic, with sometimes individually disabled restrictions, this would be a bit extreme to do in one step (or to do at all, some lints might be intrusive or opiniated).

This PR proposes the activation of selected Clippy lints. Those I have tried so far are:

• explicit_iter_loop: prefer iterating over &collection rather than collection.iter(), this is more compact and idiomatic
• flat_map_option: using .flat_map(f) when f returns an Option is equivalent to using .filter_map(f), except that the latter better underlines the filtering aspect
• implicit_clone: using .to_vec() on a Vec, or .to_owned() on an owned object can be replaced by .clone(), which makes it clearer that we already have something of the right type; at least one .clone() call is even superfluous after doing this transformation
• needless_for_each: for simple loops (one occurrence currently in the code), using an explicit for loop is clearer than using .for_each()
• redundant_else: we learn very early that using else after a block which returns early is useless. This might be the most contentious lint in this list, as it changes the style a bit. (removed after discussion, see below)
• semicolon_if_nothing_returned: using ; at the end of expressions returning () when we do not explicitly need this unit value better expresses that we have a statement, not just an expression. It increases style consistency.
• unlined_format_args: inline variable names in {} when interpolating, also for consistency as this is done in 90%+ of the code.

Other lints might be of interest of course, but I gave it a first run in order to test the idea, and find some lints that other developers might feel comfortable with.

If I'm not wrong, all those lints are available in the stable compiler used to run Clippy in the CI.

[ilyagr]

This is not necessarily related to this PR, but cargo +nightly clippy currently has numerous errors of the type https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph . I ended up using cargo cranky to ignore it, but if you feel like fixing some lints :)

[ilyagr]

I like most of these, except for the redundant_else commit (as you foresaw). I highlighted the two that seemed to get a lot worse. The others in that commit seem OKish to change, though I often am not sure about them and I'm not sure it's good to force those changes.

So, I'd prefer dropping that entire commit, but I don't insist on it (especially if the two cases I highlighted can be avoided)

I'm happy with the others, though I'd wait for others to take a look. Thank you!

[ilyagr]

Another thing to consider is that this will add to headache of us being affected by clippy false-positive bugs. I imagine these checks are less well-tested than the default checks.

On the bright side, these used to occur previously, and haven't for the past ~3 months, so perhaps Clippy changed something about their testing. But again, they might be more lax about less frequently used checks.

I don't think this is a blocker; if it becomes a problem, we can revert parts of this PR.

[samueltardieu]

This is not necessarily related to this PR, but cargo +nightly clippy currently has numerous errors of the type https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph . I ended up using cargo cranky to ignore it, but if you feel like fixing some lints :)

Agreed. The first version of this PR had a #![expect(clippy::too_long_first_doc_paragraph)] in several of our crates, but this is not supported by older rustc unfortunately. This would have been a relief, as it conveys that we expect this warning (and prevents it from being displayed) and hope to have it fixed in the future.

[samueltardieu]

Another thing to consider is that this will add to headache of us being affected by clippy false-positive bugs. I imagine these checks are less well-tested than the default checks.

They are well-tested nontheless: many projects use #![deny(clippy::pedantic)] and #[allow(clippy::A_PARTICULAR_LINT)].

On the bright side, these used to occur previously, and haven't for the past ~3 months, so perhaps Clippy changed something about their testing. But again, they might be more lax about less frequently used checks.

New lints must now go (since a few weeks ago) through a FCP (final comment period) before being integrated into Clippy, and must be explicitly approved by one extra member of the Clippy team (in addition to the one approving the PR), and any member of the Clippy team may use a veto. This should lead to an even higher lint quality.

I don't think this is a blocker; if it becomes a problem, we can revert parts of this PR.

Yes, removing a denying lint is done by removing the corresponding line in the top Cargo.toml, as lints are inherited by the various crates.

[samueltardieu]

I like most of these, except for the redundant_else commit (as you foresaw). I highlighted the two that seemed to get a lot worse. The others in that commit seem OKish to change, though I often am not sure about them and I'm not sure it's good to force those changes.

So, I'd prefer dropping that entire commit, but I don't insist on it (especially if the two cases I highlighted can be avoided)

Agreed. As I expected, this was the most controversial lint to enable. I'll remove it.

[PhilipMetzger]

I don't have a problem with doing this, although some discussion (on GitHub or if you're in the Discord) beforehand would've been valuable, since this affects all pending PRs. This is similar in style to Matt's one import per line PR.

[martinvonz]

I don't have a problem with doing this, although some discussion in the Discord beforehand would've been valuable, since this affects all pending PRs. This is similar in style to Matt's one import per line PR.

That's fair. A heads-up on Discord would have been good, but I also don't think contributors should have to join Discord, so I think it's more important that the PR stays or for at least a day or two to get people a chance to comment. Does 24 hours seem like a reasonable time to you too or do you think we should require a longer time?

[samueltardieu]

I don't have a problem with doing this, although some discussion in the Discord beforehand would've been valuable, since this affects all pending PRs. This is similar in style to Matt's one import per line PR.

Many files are touched by this PR, but changes are very sparse. I am not sure there will be many conflicts. Moreover, I'm of course ready to wait until some sensible or complex PRs are merged and do the job of rebasing, with jj it's a breeze. I also wanted to get a chance to get it in at the beginning of a cycle, to avoid proposing such a change just before a release in case urgent PRs need to get in, to avoid complicating things if things don't go as smoothly as I expect them to be.

[martinvonz]

I think @PhilipMetzger's concern was not with merge conflicts but that we should give people a chance to chime in on style guide changes.

[PhilipMetzger]

That's fair. A heads-up on Discord would have been good, but I also don't think contributors should have to join Discord, so I think it's more important that the PR stays or for at least a day or two to get people a chance to comment. Does 24 hours seem like a reasonable time to you too or do you think we should require a longer time?

I think we have a implicit three day period on RFC style changes, so that should be fine.

I don't have a problem with doing this, although some discussion in the Discord beforehand would've been valuable, since this affects all pending PRs. This is similar in style to Matt's one import per line PR.

Many files are touched by this PR, but changes are very sparse. I am not sure there will be many conflicts.

Its not about conflicts, as you essentially made a LSC without informing anyone and getting appropriate buy-ins, even if the technical steering for the project isn't defined yet. It also makes landing currently approved PRs harder, as we now enforce new clippy checks, which weren't enforced before.

Edit: I actually meant this link here https://chromium.googlesource.com/chromium/src/+/HEAD/docs/process/lsc/large_scale_changes.md for LSC, my bad.

[martinvonz]

I think we have a implicit three day period on RFC style changes, so that should be fine.

Sounds good. I wasn't actually aware of the 3-day period on RFC-style changes (same as design docs?), but I think you're saying that 24 hours is fine for a style change like this PR? So let's wait at least 5 hours more then, @samueltardieu.

[PhilipMetzger]

I wasn't actually aware of the 3-day period on RFC-style changes (same as design docs?), but I think you're saying that 24 hours is fine for a style change like this PR?

Sorry, it seems I was unclear. I meant that usually Authors let their RFC like proposals open for discussion for three days, before a decision was made (again, I am recalling this).

The no unsafe in cli change was discussed beforehand and the import change was also brought up before.

I'd like to wait another day, so we give everyone time to react though.

[ilyagr]

(Not really specific to this PR) As an experiment, I added a label for this.

I've occasionally missed having a way to say that a PR LGTM but I want to give others a chance to look at it. Perhaps a label could work for this.

We could also have a convention for how long the label should stay on a PR before it's merged. We could mention this in the label description. I think this could work, but better names or better ideas are of course very welcome.

For now, I'll leave the timing to everyone's judgement with the author having the final say (just like we did before without this label).

Update: One imperfection in this is that it's unclear whether the PR is waiting for a technical review from some small and specific group of people, or whether it affects everybody. We could additionally have an affects-many-developers tag or the opposite to distinguish, or it might be fine as is (if a reviewer thinks the PR would benefit from a technical review from some small and specific group of people, they could just say that and not approve the PR for a bit of time).