Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add final Clippy lints #704

Merged
merged 8 commits into from
Dec 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .cargo/config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,19 @@ lto = true
rustflags = [
# rustc additional warnings:
"-Wunused_crate_dependencies",
"-Wunused_lifetimes",
"-Wunused_macro_rules",
"-Wunused_tuple_struct_fields", # Will be uplifed into `dead_code` (warn-by-default) with https://github.com/rust-lang/rust/pull/118297
"-Wmeta_variable_misuse",
# Rust 2018 idioms that are not yet warn-by-default:
"-Welided_lifetimes_in_paths",
"-Wunused_extern_crates",
"-Wexplicit_outlives_requirements",
# 📎 Lints that are enabled (warn/deny) by default
"-Wclippy::all",
# Cargo
"-Wclippy::cargo", # Warn about Cargo.toml issues, except...
OmarTawfik marked this conversation as resolved.
Show resolved Hide resolved
"-Aclippy::multiple_crate_versions", # Not possible to deduplicate, should be done periodically ourselves
# Restriction (optional, neutral lints)
"-Wclippy::exit", # Prefer not `process::exit`ing directly
"-Wclippy::rest_pat_in_fully_bound_structs", # Prefer not to use `..` in fully bound structs
Expand All @@ -30,4 +37,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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.toml and outputs/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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Nursery
"-Wclippy::collection_is_never_read", # Lint against collections not used after creation
"-Wclippy::equatable_if_let", # Prefer regular `==` checks over Yoda `if let $pat = $value`
"-Wclippy::useless_let_if_seq", # Use idiomatic direct assignment of `let $val = if $cond { .. } else { ..};`
]
4 changes: 2 additions & 2 deletions crates/codegen/parser/runtime/src/support/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,14 @@ pub(crate) struct DelimiterGuard<'a, 's> {
closing_delim: TokenKind,
}

impl<'a, 's> Drop for DelimiterGuard<'a, 's> {
impl Drop for DelimiterGuard<'_, '_> {
fn drop(&mut self) {
let popped = self.input.closing_delimiters.pop();
debug_assert_eq!(popped, Some(self.closing_delim));
}
}

impl<'a, 's> DelimiterGuard<'a, 's> {
impl<'s> DelimiterGuard<'_, 's> {
pub(crate) fn ctx(&mut self) -> &mut ParserContext<'s> {
self.input
}
Expand Down
12 changes: 4 additions & 8 deletions crates/codegen/parser/runtime/src/support/precedence_helper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,14 +224,10 @@ impl PrecedenceHelper {

// 3. Until we have a single expression.

if pratt_elements.len() != 1 {
unreachable!("Expected a single element: {:#?}", pratt_elements)
}

if let Expression { nodes } = pratt_elements.pop().unwrap() {
ParserResult::r#match(nodes, vec![])
} else {
unreachable!("Expected an expression: {:#?}", pratt_elements)
match <[_; 1]>::try_from(pratt_elements) {
Ok([Expression { nodes }]) => ParserResult::r#match(nodes, vec![]),
Ok([head]) => unreachable!("Expected an expression: {:#?}", head),
Err(elems) => unreachable!("Expected a single element: {:#?}", elems),
}
}

Expand Down
4 changes: 2 additions & 2 deletions crates/codegen/parser/runtime/src/support/scanner_macros.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#[allow(unused_macros)]
macro_rules! scan_chars {
($stream:ident, $($char:literal),+) => {
if $( $stream.next() == Some($char) )&&* {
if $( $stream.next() == Some($char) )&&+ {
true
} else {
$stream.undo();
Expand All @@ -14,7 +14,7 @@ macro_rules! scan_chars {
macro_rules! scan_none_of {
($stream:ident, $($char:literal),+) => {
if let Some(c) = $stream.next() {
if $(c != $char)&&* {
if $(c != $char)&&+ {
true
} else {
$stream.undo();
Expand Down
6 changes: 2 additions & 4 deletions crates/codegen/schema/src/json_schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,8 @@ fn relax_schema(value: Value) -> Value {
.into_iter()
.filter_map(|(key, value)| {
// Remove `additionalProperties: false`
if key == "additionalProperties" {
if let Value::Bool(false) = value {
return None;
}
if key == "additionalProperties" && value == Value::Bool(false) {
return None;
}

// Replace `oneOf` and `allOf` with `anyOf`
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading