Standard lints v7 candidates (Rust 1.60)

[repi]

Planning to do a larger v7 update to our standard lints (#59) with a bunch of additions and requirement of Rust 1.60

Lint candidates

These are the candidates currently thinking of and are using in our internal main repo but will experiment with all of these in our other private and open repos and see if there are any issues with them there.

Planning to add

Quite trivial but nice ones:

• clippy::disallowed_script_idents
• clippy::equatable_if_let
• clippy::fn_to_numeric_cast_any
• clippy::index_refutable_slice
• clippy::iter_not_returning_iterator
• clippy::negative_feature_names
• clippy::nonstandard_macro_braces
• clippy::trailing_empty_array
• clippy::unnecessary_wraps
• trivial_numeric_casts

Planning to remove

These are now warn-by-default:

• clippy::disallowed_methods
• clippy::disallowed_types

Considering

More opinionated for our engine repository workflow:

• clippy::mod_module_files - the style we use and prefer
• clippy::undocumented_unsafe_blocks - this one we do have a lot of allows with for FFI crates and such, but it is a quite nice default still that we just recently activated
• unsafe_op_in_unsafe_fn - see this tracking issue and deeper discussion in RFC about the direction of getting this to be default in the language as unsafe fn and unsafe blocks are quite different things. https://github.com/rust-lang/rust/issues/71668
• rustdoc::missing_crate_level_docs - this may not be that important for many repos, but we've found it a nice quality to have for both internal and open source crates - and can just be a single-line when you add the crate.

[repi]

@MarijnS95 do you think these lints are a good fit for y'all also or do you have any exceptions or additions? Interested in getting feedback!

[MarijnS95]

@repi Adding them to our repo as we speak, let's see! I'll follow up shortly with a list of our own :)

[MarijnS95]

• clippy::equatable_if_let Really like this, glad to have some expensive op or function call on the left again, and then const thing to compare to back on the right. It's usually trivial derive(Eq) enums anyway thad somehow ended up with this rather strange syntax :thinking:;
• clippy::unnecessary_wraps Hard for me to catch in review, glad there's a lint for it!

The rest is not triggering anything for us :)

[repi]

@MarijnS95 sweet, did that include testing the ones under "Considering", surprised the unsafe ones didn't trigger anything for y'all :)

[MarijnS95]

• clippy::mod_module_files - the style we use and prefer

We use (and probably prefer) the other way around for now :grimacing:

• clippy::undocumented_unsafe_blocks - this one we do have a lot of allows with for FFI crates and such, but it is a quite nice default still that we just recently activated

Could be interesting for us too, minus our Graphics-API crates that are really heavy on the unsafe side.

• unsafe_op_in_unsafe_fn - see this tracking issue and deeper discussion in RFC about the direction of getting this to be default in the language as unsafe fn and unsafe blocks are quite different things. Tracking Issue for "unsafe blocks in unsafe fn" (RFC #2585) rust-lang/rust#71668

Yeah it is good to know the difference here. I haven't looked at the RFC discussion but this can only be about unsafe fn making a function unsafe to call, and - for now, but what this lint disallows - pretend like the entire body was wrapped in unsafe too, assuming a function that's unsafe to call must be utilizing unsafe behaviour internally too.

• rustdoc::missing_crate_level_docs - this may not be that important for many repos, but we've found it a nice quality to have for both internal and open source crates - and can just be a single-line when you add the crate.

Craving for more docs :D

[MarijnS95]

@repi I don't think we can utilize any of the opinionated extra lints, at least not across our entire project :grimacing:

[MarijnS95]

Now for what we've got going locally:

• clippy::needless_pass_by_value
• clippy::print_stderr - these seem to be "working" again since switching to config.toml
• clippy::print_stdout
• clippy::trivially_copy_pass_by_ref
• clippy::use_self
• clippy::useless_let_if_seq

Most were probably already discussed in some prior PR, or disabled because they didn't seem to lint things that should have been caught.

However, going further we've also been looking at default-allowed lints in rustc. You can find the entire list with rustc -A help, but I've only enabled a few in our workspace that were triggering active violations, and were worth fixing:

rustflags = [
    "-Wdeprecated-in-future",
    "-Wtrivial-numeric-casts", # Included in your list above 🎉
    "-Wunused-crate-dependencies",
    "-Wunused-qualifications",
]

More to come at some point, hopefully.

Would love to enable trivial-casts too at some point, already did so in https://github.com/MaikKlein/ash/pull/564. This lint seems to point out a lot of overlap with clippy::ptr_as_ptr.

[repi]

@repi I don't think we can utilize any of the opinionated extra lints, at least not across our entire project 😬

Fair enough! it took us a while to adapt to it also, esp. the crate docs. None of the ones in the "Considering" are that critical for us to enable on all of our projects at this time. clippy::mod_module_files is probably the main one I would like to do that with, but just syntax so not that important.

--

will take a look at the other ones you mention here and cross-reference with my index of tested lints and our experience with them (or if we haven't tested it yet). big thanks!

[MarijnS95]

@repi How about removing clippy::match_on_vec_items, which seems to be with us from the start? This is a really silly lint that disallows the panicking index operator, but only in match some_vec[i] because it's "supposedly" a convenient spot to wrap every match arm in Some, and add a None => at the end.

This drives developers to sometimes write None => panic!() which actually violates clippy::get_unwrap (we should add that one to the list though!) but clippy isn't even able to detect that pattern as it specifically searches for .get(i).unwrap()...

If people really want no panicking code, including indexing operators that cannot be bounds-checked at compiletime, they should simply defer to clippy::indexing_slicing.