Add final Clippy lints #704
Conversation
This can be expressed more cleanly using safe code. Done when trying to limit unwraps/panics in the code but this should be done as a separate PR/push.
|
| @@ -30,4 +33,8 @@ rustflags = [ | |||
| "-Aclippy::module_name_repetitions", # It seems we prefer it this way; we'd need to discuss that | |||
| "-Aclippy::must_use_candidate", # Overzealous, we'd have to `[must_use]` a lot of things | |||
| "-Aclippy::redundant_closure_for_method_calls", # Not always clearer, let's not pepper `allow`s whenever needed | |||
There was a problem hiding this comment.
I think we already did a great push in Q4 with linting/code quality, so I'm not suggesting we spend more time on this. However, I think there are a few things remaining in #155 so I would like to keep it open for now. If you agree, we can unassign it and push it out of this sprint, or split the issue:
- panics: while we cannot prevent it 100%, especially in depenedencies, we can still prevent the majority of it (panic/unwrap/expect/etc...) in our code, which I think is more likely to have panics. I would like to try turning it on just in
outputs/cargo/crate/.cargo/config.tomlandoutputs/npm/crate/.cargo/config.toml. The rest of the codebase is internal, and panicing there is fine. - Not sure if we reviewed allowed-by-default already for anything useful?
There was a problem hiding this comment.
I suggest separating panics out, because the core issue here that we want to adress is not just about enabling select lints and fixing the code.
Rather, it's a more conscious effort to minimize panics on Rust side when used from the TS/client side, which would need more involved setup e.g. running the node with the additional flags (NomicFoundation/hardhat-vscode#523 (comment)), probably collecting the crash reports from Sentry, adding further soak/regression tests and the like, since it's impossible to statically guarantee that certain code paths are (reasonably) panic-free.
Unfortunately, we can't turn it on because it has false positives that
suggest a code that fails to compile, i.e.
```rust
enum Errors<'err> {
#[error("An item with the name '{0}' already exists.")]
ExistingItem(&'err Identifier),
#[error("A variant referencing '{0}' already exists.")]
ExistingVariant(&'err Identifier),
#[error("An expression with the name '{0}' already exists.")]
ExistingExpression(&'err Identifier),
}
```
suggests to remove the `'err` but it's impossible to define the enum
using the elided lifetime, i.e. `enum Errors<'_>`.
See rust-lang/rust#117965 for the existing bug report.
The first one is enabled in the `rustc` itself and vendored-in tools: `unused_lifetimes`. It makes sense to warn against it by default and pepper allows or change the code such that it's used, e.g. with PhantomData. The second one will be already uplifted to `dead_code` (which is warn-by-default) but since we're warning against some lints, it can't hurt.
I've added few more from the list in c721b9f, 9f6733e, and 5faa20a. The rest is either:
Some crates have prided themselves at some point as being Also Other than that, I'd say we went above and beyond and did our due dilligence here and as part of the #155 in general and the rustc/Cargo/Clippy lint list should be now exhaustive (I know I am exhausted!). @OmarTawfik could I get a rubber stamp on the rustc lints as well, as part of this PR? |
|
Awesome! Thank you for your reviews on this ❤️ |
Final Clippy part of #155
Lints
From the additional lint list mentioned by @OmarTawfik:
clippy::same_name_method- sounds useful but std traits are covered already byshould_implement_traitand this fires on wrapper functions/traits; only 4 occurences in our codebase are intentional wrappersclippy::allow_attributes_without_reason- requires a nightly rustc featureclippy::self_named_module_files- forcingmod.rsonto users seems backwards, as that's one of the key issues addressed for Rust 2018 in terms of module/file path ergonomics, see https://dev-doc.rust-lang.org/stable/edition-guide/rust-2018/path-changes.html#no-more-modrs for more contextclippy::format_push_string- alternative proposed is fallible, whereas the "offending" code is infallible, not enough value IMOclippy::str_to_string- will be better fixed withimplicit_cloneonce we bump Rust to 1.7{3,4} with a new Clippy that has less false positivesclippy::wildcard_enum_match_arm- we already havematch_wildcard_for_single_variants+ can have more false positivesclippy::use_self- unstable, false positivesclippy::collection_is_never_read✅ addedclippy::equatable_if_let✅ addedclippy::useless_let_if_seq✅ addedPanicking
The remaining items from #155 are about panicking in N-API-exposed crate.
Unfortunately, there is no way to guarantee or more broadly verify that certain code paths don't panic, as
rustconly compiles a single crate at a time and panicking is not encoded in the type system. We could brainstorm how we can make this more resilient on the Rust side but that requires more data (maybe we could plug in Sentry for the Rust side of VSCode extension?) and a more careful thinking rather than trying to tackle this as part of the "Lint Rust code" item.