Enable some non-default clippy lints

[jplatte]

No description provided.

[gnunicorn]

mind elaborating a bit more on the need for this? in particular I would like to know:

1. why use the rather hidden cargo-config instead of a more explicit .clippy.toml file?
2. the rational behind the specific lints, why we need and want them?

I generally trust, that default clippy-lints are pretty well tested, but non-defaults might have more false-positives, like that very first branches_sharing_code lint you are activating.

I'd really like to avoid clippy --fix to be something that I have to carefully review to make sure, it doesn't change the behavior of the code. Thus I'd rather go for a smaller, more reliable set of lints than have more false positives or unexpected changes of behavior.

(lastly, there seem to be a bunch of unrelated appservice-changes in here...)

[poljar]

Looks good, I'm not familiar with the specific lints, once you and @gnunicorn agree to a set of lints and CI is fixed feel free to merge.

[jplatte]

1. why use the rather hidden cargo-config instead of a more explicit .clippy.toml file?

That file is for configuring what lints do (e.g. which types disallowed-types warns / errors on), not whether they run.

2. the rational behind the specific lints, why we need and want them?

I just took Ruma's list and filtered out ones that clearly didn't make sense here.

I generally trust, that default clippy-lints are pretty well tested, but non-defaults might have more false-positives, like that very first branches_sharing_code lint you are activating.

Happy to remove that one again, but unless we actually see issues, why not? A few of these are also in the restriction category, i.e. not active by default because they are about style choices that aren't unanimously accepted in the Rust community (for example in Ruma we enable a lint that disallows mod.rs files) rather than having known false-positives.

(lastly, there seem to be a bunch of unrelated appservice-changes in here...)

So did you just review the whole-PR diff? I generally go commit-by-commit if there are meaningfully separate commits. I can move these into a separate PR though, if you prefer.

I'm not familiar with the specific lints

I'll remove a few again since they now seem to be active by default or have problematic false-positives, here's an overview of the rest:

cloned_instead_of_copied

Checks for usages of cloned() on an Iterator or Option where copied() could be used instead.

More obvious that it's a cheap copy.

dbg_macro / todo

Checks for leftover dbg! / todo! macros that should only present temporarily during development / debugging.

inefficient_to_string

Checks for usage of .to_string() on an &&T where T implements ToString directly (like &&str or &&String).

(bypasses specialized impls from std)

macro_use_imports

Checks for #[macro_use] which should not be necessary in modern Rust codebases.

mut_mut

Checks for &mut &mut T.

nonstandard_macro_braces

Checks for uncommon brace style for well-known macro invocations (e.g. vec!(1, 2, 3) instead of vec![1, 2, 3]).

str_to_string

Checks for <&str>::to_string.

[gnunicorn]

That file is for configuring what lints do (e.g. which types disallowed-types warns / errors on), not whether they run.

gotcha. But I wonder if the more common (and expected) way to enable them isn't over a #![warn(clippy::.. in the crates lib.rs-file ... I guess you've picked the cargo-config in order to have that work for the entire repo rather than having to stick it to each crate? Not in favor, but if you prefer that, fine with me.

So did you just review the whole-PR diff? I generally go commit-by-commit if there are meaningfully separate commits. I can move these into a separate PR though, if you prefer.

yes ... How would a reviewer know there are meaningful separate commits? And why would they be bundled in a single PR under an unrelated name ... I mean, they will be merged as whole, so they have to be reviewed at their last full state either way. So, yes, please, atomic commits about a single thing at a time.

I'll remove a few again since they now seem to be active by default or have problematic false-positives, here's an overview of the rest:

Thanks for clarifying. That list sounds fine by me. I wonder how we keep that information available for the future, maybe just link this PR in the config as a comment?

[gnunicorn]

please reduce the changes to the ones related to the topic and remove changes unrelated to it. Then merge at will.

[jplatte]

gotcha. But I wonder if the more common (and expected) way to enable them isn't over a #![warn(clippy::.. in the crates lib.rs-file ... I guess you've picked the cargo-config in order to have that work for the entire repo rather than having to stick it to each crate?

Yes, this is very much a hack but it's a somewhat well-known one that you will find referenced in various issues asking about a first-class lints configuration thing to be added to cargo. I consider the alternative of manually keeping enabled lints in sync across various crate roots to be worse.

So, yes, please, atomic commits about a single thing at a time.

I guess you meant to say PRs? Because the commits are already atomic.

[gnunicorn]

yes, atomic PRs, of course.

[jplatte]

Do you want me to move the (now) first commit into its own PR too? I found it while working on the clippy stuff but it's not strictly related.

[gnunicorn]

nah, that's fine. thanks heaps.