Rust Linting #155
Assignees
Comments
github-merge-queue bot
pushed a commit
that referenced
this issue
Oct 31, 2023
...but don't error when it fails. This should help us prepare to gradually adopt the linter and fix the relevant reported lints. Part of #155 EDIT: This runs the lint command as usual and we control which lints we warn against, so that we can gradually migrate.
github-merge-queue bot
pushed a commit
that referenced
this issue
Nov 28, 2023
Part of #155 These should all be more or less obvious, maybe with the exception of: - ~`comparison_chain` - in this case it might be more noise than it's worth, I'm fine with silencing it here, instead~ EDIT: Actually, let's drop it for now - `single_char_pattern` - in our case we do less `"`-escaping, so that's good enough for me (but I know it might be considered controversial) - `clippy::upper_case_acronyms` - this helps follow the Rust naming convention (`CLI` -> `Cli`, which I'm strongly in favour; it's not a SCREAMING_CASE_CONSTANT) Some select lints are explicitly allowed on a case-by-case basis here but in general are worthwhile to be linted against, e.g. `enum_variant_names` or `manual_range_contains`. The only ones left now are: - `comparison_chain` - in this case it might be more noise than it's worth, I'm fine with silencing it here, instead - `needless_return`, which we will have *many* instances of and I'd like to have a separate PR for that (but I'm *strongly* in favour, as I find it to be one of the signature aspects of Rust) - `should_implement_trait` - needs to be silenced in one case for `#[napi] fn clone`, but `ParserContext::next` might or might not implement the `Iterator`, I'm not sure yet, so I'd like to defer it for now.
This was referenced Dec 2, 2023
|
It would be great to be able to sort the imports, however Same goes for |
github-merge-queue bot
pushed a commit
that referenced
this issue
Dec 4, 2023
Part of #155 Leaving the `needless_return` for a dedicated PR.
This was referenced Dec 4, 2023
github-merge-queue bot
pushed a commit
that referenced
this issue
Dec 7, 2023
Part of #155 Commit messages have a bit more context on the relevant changes. I've decided to drop very few lints from the EDR list like deny-by-default ones, `inconsistent_struct_constructor` (not that useful) or `implicit_clone` (a lot of false-positives/unidiomatic suggestions; let's wait for Rust 1.74 and rust-lang/rust-clippy#11281 to include it again) or irrelevant ones for us like `suboptimal_flops`.
Merged
github-merge-queue bot
pushed a commit
that referenced
this issue
Dec 8, 2023
Part of #155 This is fairly standard practice - we group the imports in the `std` > external libraries > current crate categories, separated by newlines. Some links: - rust-lang/style-team#24 - rust-lang/rustfmt#1302 - https://www.reddit.com/r/rust/comments/wwbxhw/comment/ilkid50/
This was referenced Dec 11, 2023
Merged
github-merge-queue bot
pushed a commit
that referenced
this issue
Dec 13, 2023
Final Clippy part of #155 ## Lints From the additional lint list mentioned by @OmarTawfik: - Cargo - added everything, except the duplicate packages lint, as this is not fixable and we should strive to periodically do that ourselves. In any case, it's not something we should lint against and deny in CI - Restriction: - `clippy::same_name_method` - sounds useful but std traits are covered already by `should_implement_trait` and this fires on wrapper functions/traits; only 4 occurences in our codebase are intentional wrappers - `clippy::allow_attributes_without_reason` - requires a nightly rustc feature - `clippy::self_named_module_files` - forcing `mod.rs` onto 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 context - `clippy::format_push_string` - alternative proposed is fallible, whereas the "offending" code is infallible, not enough value IMO - `clippy::str_to_string` - will be better fixed with `implicit_clone` once we bump Rust to 1.7{3,4} with a new Clippy that has less false positives - `clippy::wildcard_enum_match_arm` - we already have `match_wildcard_for_single_variants` + can have more false positives - Nursery: - `clippy::use_self` - unstable, false positives - `clippy::collection_is_never_read` ✅ added - `clippy::equatable_if_let` ✅ added - `clippy::useless_let_if_seq` ✅ added ## Panicking 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 `rustc` only 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.
github-merge-queue bot
pushed a commit
that referenced
this issue
Dec 13, 2023
Part of #155 This is going to be a fun one 😆 I've pushed two preview commits: the first one imports using the `Crate` granularity, the other one with `Module` granularity. Disclaimer: I'm biased 😃 I've used this style since I started using Rust and it's also being used by vast majority of projects in the ecosystem that I've used or looked at; it's also used by the Rust compiler, Cargo and the tools like Rustfmt or Clippy. Here's a summary of the possible options: rust-lang/rustfmt#3362. Here is an RFC discussion about setting on a variant: rust-lang/style-team#140, I highly recommend reading it. To give a brief summary, it seems that people stand behind two options: - `Module` - good trade-off between readability and conciseness - `Item` - minimizes conflicts when many people edit the same files often To bring back some of the arguments raised in favor of the `Module` and explain some of my reasoning as well: - it naturally settles on the module as the, well, module/boundary from which items can be imported... - thus, helps build the intuition how to use and structure Rust crates - less verbose than Java-style (`Item`) imports, which can often explode and take unreasonable amount of space (and we don't really benefit enough from minimized conflicts as we probably won't be a team of 50 or the like)... - but repeats enough information to quickly trace a module path rather than having to reconstruct the paths from the `crate`-style use import tree... - and already often takes less space in our case, line-wise; - quite good `grep`pability
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Review
warn-by-defaultandallow-by-defaultfrom this list, and deny anything useful.--warn unused-crate-dependenciesAllow running Clippy lints via
infra#626Finish setting up rustc/Clippy lints and fix those #695
prevent panics in Rust and NAPI parser crates, as they block clients, and terminate the calling NodeJS process, with no way to recover/inspect the errors:
clippy::cargo (entire category)
Restriction:
Nursery:
Fix doc-links and run Rustdoc as part of the
infra checksuite #694install rustfmt from nightly #696
The text was updated successfully, but these errors were encountered: