Embark's standard lints

[repi]

Background

This issue tracks which Rust and Clippy lints we currently use as default for all of our internal and external crates. We have a common base of lints that we configure the same everywhere that is more opinionated than the Rust and Clippy defaults and geared towards how we prefer to (currently) write & maintain Rust code.

Individual crates are able to, and often do override some of these defaults, either on a crate level, or for specific modules or functions. But we still find it helpful to have a standard across everything to keep the code high quality and generally working the same way.

Lint configuration

We currently have two ways to enable our configured lints:

1. Through copy'n'pasting https://github.com/EmbarkStudios/rust-ecosystem/blob/main/lints.rs into every crate
2. Through a .cargo/config.toml section: https://github.com/EmbarkStudios/rust-ecosystem/blob/main/lints.toml

Traditionally we've used option 1, but are testing and converting more to option 2 as it is much easier to just have a single configuration file with everything.

We hope to later in 2021 have proper first-class support for a lints.toml or [lints] section in Cargo and switch to that, but progress for it has been slow. Tracked in:

• Be able to disable/enable Clippy lints globally #22
• Support defining enabled and disabled lints in a configuration file rust-lang/cargo#5034
• [Roadmap] Configuration file for lints rust-lang/rust-clippy#6625.

Lints of interests

Testing / being discussed

• Add: clippy::needless_pass_by_value - helped us catch a lot of unnecessary : Vec<T> arguments and replace them with : &[T], saving a lot of calls to .clone() (very common trap for rust beginners to fall into). quite intrusive change for lots of crates though so safer to do separtely
• Add: rustdoc::missing_crate_level_docs

Blocked

List of Clippy lints we are interested in using, but that have issues or are not mature / specific enough yet for inclusion.

• clippy::use_self - blocked on clippy::use_self has too many false positives #53
• clippy::unnecessary_wraps
• clippy::option_if_let_else - blocked on:
  • option_if_let_else suggests map_or instead of map_or_else even if else branch contains a function call rust-lang/rust-clippy#5821
  • option_if_let_else suggestion inapplicable when mutable borrow or owned value is used in both branches rust-lang/rust-clippy#5822
  • Invalid suggestion of option_if_let_else causes a compilation error error[E0382]: Borrow of moved value rust-lang/rust-clippy#6737
  • option_if_let_else suggestion does not compile rust-lang/rust-clippy#6137
• clippy::wildcard_enum_match_arm - blocked on wildcard_enum_match_arm fail with non-exhaustive enums and OR patterns rust-lang/rust-clippy#6862
• clippy::print_stdout / clippy::print_stderr - ineffective due to EarlyLintPass::check_mac() is not called in non-toplevel modules rust-lang/rust-clippy#5721

[repi]

Filed EmbarkStudios/opensource#35 to start using the template in all of our current open source repos

[repi]

#60 is a PR draft and discussion about potential additions and removals for v0.3

[repi]

We have a v0.3 of the lints now also that adds the following ones that we've verified across multiple of our codebases 🎉

• Add: map_err_ignore - prevents loosing error context and works well in Ark, easy to ignore when needed
• Add: match_same_arms - seems to work well and simplifies matching code to reduce complexity, with some exceptions.
• Add: large_types_passed_by_value - seems like a good idea, could catch some things should be boxed. no warns in Ark.
• Add: string_add - cleaner & more explicit, only 1 easy fixed warn in Ark.
• Add: string_add_assign - cleaner & more explicit, only 1 easy fixed warn in Ark.
• Add: explicit_iter_loop - we already use explicit_into_iter_loop in v0.2, should enable or disable both.
• Add: unimplemented - good for stubbing out WIP code but want to avoid on main our crates & systems are live at head

[repi]

We've released v0.4 with significant amount of smaller pedantic lints added, and a couple removals. Details in PR #66

[Jake-Shadle]

I think we should remove disallowed_type(s) and disallowed_method(s) from the lint list, these lints only apply if they are actually configured for the repo, which a majority don't do, and it's also complicated because they were renamed in 1.59.0 to be plural, which means updating the list could make running clippy on older versions fail and require an update to all users of the lints since the renamed lints are now a warning.

[MarijnS95]

@Jake-Shadle There was a similar Rust 1.55+ requirement for v5, seems like we can do the same for a v6 release?

In any case, I fixed these in #73, we at traverse are actively using this setup. Looking at it again I'll remove them in that PR, as they're already warn-by-default and thus provide no extra benefit being configured explicit (if they weren't, clippy.toml config would be a useless no-op...).

[Jake-Shadle]

Yes, since they are warn by default I don't think it makes sense to keep them in the list at all, technically it's a breaking change to keep them in the list due to the clippy change, but it will be fine to do that in a 0.6 version, I just wanted to post here for when that does happen because I'm in the process of removing them from all of the crates I maintain.

[repi]

Oh they are warn-by-default now, that's great and yes then we shall simply remove them. Will do a v6 release now soon requires 1.59 with them removed. And follow up with another release soon with new additional lints that we've been using and testing.

[repi]

Released v6 now with the renamed warn-by-default lints removed. (#74)

[MarijnS95]

Well, guess I am too late to update that PR 😬

Any other lints in the list that are now default-warn?

With additional lints pending, shall we submit some of ours again too (after deduplicating with what Embark has been testing with)? We've been finding some interesting new flags as well.

[repi]

@MarijnS95 just wanted to get out v6 quickly to unblock Rust 1.59 clippy compile errors.

created #75 to discuss v7 additions and removals and good idea also to go through and see if there are more warn-by-default lints we can remove. and great if you have any other lints that you've been testing with and that works well, so let's discuss in that!

[epage]

I've created an RFC for package-level lint configuration: rust-lang/rfcs#3389

[epage]

I feel like the above RFC has shaped up quite well. As Embark has a large lint configuration that gets copied from project to project, I'm curious how well you feel the current design works out from the "large user" perspective.

In particular

• The priority field for controlling lint order (so you can enable a lint group like all and then override lints within it)
• This being focused more on linters and not targeting rustfmt and other tools
• The format being <lint> = "<level>" in the short form and <lint> = { level = "<level>", priority = 0 } in the long form
• Using workspace inheritance for explicitly sharing across the workspace
• Not allowing overriding of the workspace-inherited config in the initial release
• Not supporting lint configuration in the initial release

(I think that covered all the more controversial items)

[epage]

@repi there is one particular point the cargo team is hoping Embark can spare some time to provide feedback on regarding your experience with your lints lists.

Problem: RUSTFLAGS has baked in an ordering so users can have lints override groups. We are exploring how to support this within the [lints] table. A particularly challenging point is that there are no guarantees today around lint groups, e.g. two rustc lint groups intersect which makes it so we can't perfectly order lint groups on the users behalf.

We are particularly concerned about the experience managing large lists of lints, not just that some experts can maintain it but with an eye of empathy towards what problems other users may have.

In a recent post, I've laid out the options we are considering. "large lists of lints" came up in two parts of the conversation within the cargo team

• One option more directly imitates RUSTFLAGS (Option 4, particularly variant 3), so you have direct experience with this. What are trying to get a feel for is wow likely are people to not explicitly track the groups within the list and sort across lints and groups, rather than just within lints and groups only if its safe
• If we make the format warn = [ "lint" ], we are wondering how likely might a user lose track of which list they are in (we've seen this with dependency tables)
  • The RUSTFLAGS one is what motivated for me to reach out, I'm mostly including this for completeness of discussing large lint list problems