From 05906092863b66479b18a714ffa53dee27edeb6f Mon Sep 17 00:00:00 2001 From: Martin Geisler Date: Wed, 24 Apr 2024 11:39:40 +0200 Subject: [PATCH] Introduce a `cargo clippy` run This might help catch stylistic problems in our Rust code. --- .github/workflows/build.yml | 30 ----------- .github/workflows/lint.yml | 53 +++++++++++++++++++ mdbook-course/src/bin/course-schedule.rs | 2 +- mdbook-course/src/bin/mdbook-course.rs | 2 +- mdbook-course/src/course.rs | 20 ++++--- mdbook-course/src/markdown.rs | 2 +- mdbook-course/src/replacements.rs | 2 +- mdbook-exerciser/src/lib.rs | 2 +- .../chat-async/src/bin/client.rs | 2 +- src/error-handling/exercise.rs | 4 +- src/iterators/exercise.rs | 4 +- src/tuples-and-arrays/exercise.rs | 2 + src/types-and-values/exercise.rs | 2 + src/unsafe-rust/exercise.rs | 3 +- .../rust-on-exercism/health-statistics.rs | 9 ++-- 15 files changed, 80 insertions(+), 59 deletions(-) create mode 100644 .github/workflows/lint.yml diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 45bb52b956e3..f041f221483b 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -10,36 +10,6 @@ env: CARGO_TERM_COLOR: always jobs: - format: - runs-on: ubuntu-latest - steps: - - name: Checkout - uses: actions/checkout@v4 - - - name: Install formatting dependencies - run: | - sudo apt update - sudo apt install gettext yapf3 - - - name: Install nightly rustfmt - run: | - rustup default nightly - rustup component add rustfmt - - - name: Check formatting - uses: dprint/check@v2.2 - - typos: - runs-on: ubuntu-latest - steps: - - name: Checkout - uses: actions/checkout@v4 - - - name: Check for typos - uses: crate-ci/typos@v1.20.9 - with: - config: ./.github/typos.toml - cargo: strategy: matrix: diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 000000000000..c101e09096a0 --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,53 @@ +name: Lint + +on: + pull_request: + push: + branches: + - main + +env: + CARGO_TERM_COLOR: always + +jobs: + format: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Install formatting dependencies + run: | + sudo apt update + sudo apt install gettext yapf3 + + - name: Install nightly rustfmt + run: | + rustup default nightly + rustup component add rustfmt + + - name: Check formatting + uses: dprint/check@v2.2 + + typos: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Check for typos + uses: crate-ci/typos@v1.20.9 + with: + config: ./.github/typos.toml + + clippy: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Setup Rust cache + uses: ./.github/workflows/setup-rust-cache + + - name: Clippy + run: cargo clippy diff --git a/mdbook-course/src/bin/course-schedule.rs b/mdbook-course/src/bin/course-schedule.rs index 073f4e73422f..6132477fab1d 100644 --- a/mdbook-course/src/bin/course-schedule.rs +++ b/mdbook-course/src/bin/course-schedule.rs @@ -48,7 +48,7 @@ fn timediff(actual: u64, target: u64, slop: u64) -> String { } else if actual + slop < target { format!("{}: ({} short)", duration(actual), duration(target - actual),) } else { - format!("{}", duration(actual)) + duration(actual).to_string() } } diff --git a/mdbook-course/src/bin/mdbook-course.rs b/mdbook-course/src/bin/mdbook-course.rs index faaeacbda137..c739a48657ec 100644 --- a/mdbook-course/src/bin/mdbook-course.rs +++ b/mdbook-course/src/bin/mdbook-course.rs @@ -29,7 +29,7 @@ fn main() { ); let matches = app.get_matches(); - if let Some(_) = matches.subcommand_matches("supports") { + if matches.subcommand_matches("supports").is_some() { // Support all renderers. process::exit(0); } diff --git a/mdbook-course/src/course.rs b/mdbook-course/src/course.rs index 354b7e2419e1..61c8485c54e6 100644 --- a/mdbook-course/src/course.rs +++ b/mdbook-course/src/course.rs @@ -156,7 +156,7 @@ impl Courses { fn course_mut(&mut self, name: impl AsRef) -> &mut Course { let name = name.as_ref(); if let Some(found_idx) = - self.courses.iter().position(|course| &course.name == name) + self.courses.iter().position(|course| course.name == name) { return &mut self.courses[found_idx]; } @@ -177,9 +177,7 @@ impl Courses { &self, chapter: &Chapter, ) -> Option<(&Course, &Session, &Segment, &Slide)> { - let Some(ref source_path) = chapter.source_path else { - return None; - }; + let source_path = chapter.source_path.as_ref()?; for course in self { for session in course { @@ -193,7 +191,7 @@ impl Courses { } } - return None; + None } } @@ -202,7 +200,7 @@ impl<'a> IntoIterator for &'a Courses { type IntoIter = std::slice::Iter<'a, Course>; fn into_iter(self) -> Self::IntoIter { - (&self.courses).into_iter() + self.courses.iter() } } @@ -216,7 +214,7 @@ impl Course { fn session_mut(&mut self, name: impl AsRef) -> &mut Session { let name = name.as_ref(); if let Some(found_idx) = - self.sessions.iter().position(|session| &session.name == name) + self.sessions.iter().position(|session| session.name == name) { return &mut self.sessions[found_idx]; } @@ -275,7 +273,7 @@ impl<'a> IntoIterator for &'a Course { type IntoIter = std::slice::Iter<'a, Session>; fn into_iter(self) -> Self::IntoIter { - (&self.sessions).into_iter() + self.sessions.iter() } } @@ -350,7 +348,7 @@ impl<'a> IntoIterator for &'a Session { type IntoIter = std::slice::Iter<'a, Segment>; fn into_iter(self) -> Self::IntoIter { - (&self.segments).into_iter() + self.segments.iter() } } @@ -403,7 +401,7 @@ impl<'a> IntoIterator for &'a Segment { type IntoIter = std::slice::Iter<'a, Slide>; fn into_iter(self) -> Self::IntoIter { - (&self.slides).into_iter() + self.slides.iter() } } @@ -451,7 +449,7 @@ impl Slide { pub fn is_sub_chapter(&self, chapter: &Chapter) -> bool { // The first `source_path` in the slide is the "parent" chapter, so anything // else is a sub-chapter. - chapter.source_path.as_ref() != self.source_paths.get(0) + chapter.source_path.as_ref() != self.source_paths.first() } /// Return the total duration of this slide. diff --git a/mdbook-course/src/markdown.rs b/mdbook-course/src/markdown.rs index d22ccc90434d..6c1657ef08e7 100644 --- a/mdbook-course/src/markdown.rs +++ b/mdbook-course/src/markdown.rs @@ -83,7 +83,7 @@ impl Table { for cell in iter { write!(f, " {} |", cell)?; } - write!(f, "\n") + writeln!(f) } } diff --git a/mdbook-course/src/replacements.rs b/mdbook-course/src/replacements.rs index 3695aa79f377..0e6587c6b041 100644 --- a/mdbook-course/src/replacements.rs +++ b/mdbook-course/src/replacements.rs @@ -50,7 +50,7 @@ pub fn replace( } ["course", "outline", course_name] => { let Some(course) = courses.find_course(course_name) else { - return format!("not found - {}", captures[0].to_string()); + return format!("not found - {}", &captures[0]); }; course.schedule() } diff --git a/mdbook-exerciser/src/lib.rs b/mdbook-exerciser/src/lib.rs index 8f6040d675d0..8a13abef4e54 100644 --- a/mdbook-exerciser/src/lib.rs +++ b/mdbook-exerciser/src/lib.rs @@ -58,7 +58,7 @@ pub fn process(output_directory: &Path, input_contents: &str) -> anyhow::Result< Event::Text(text) => { info!("Text: {:?}", text); if let Some(output_file) = &mut current_file { - output_file.write(text.as_bytes())?; + output_file.write_all(text.as_bytes())?; } } Event::End(TagEnd::CodeBlock) => { diff --git a/src/concurrency/async-exercises/chat-async/src/bin/client.rs b/src/concurrency/async-exercises/chat-async/src/bin/client.rs index bc912a28b376..a76b0db3ae25 100644 --- a/src/concurrency/async-exercises/chat-async/src/bin/client.rs +++ b/src/concurrency/async-exercises/chat-async/src/bin/client.rs @@ -41,7 +41,7 @@ async fn main() -> Result<(), tokio_websockets::Error> { println!("From server: {}", text); } }, - Some(Err(err)) => return Err(err.into()), + Some(Err(err)) => return Err(err), None => return Ok(()), } } diff --git a/src/error-handling/exercise.rs b/src/error-handling/exercise.rs index 6b4511112d8f..ceabc7f5fc99 100644 --- a/src/error-handling/exercise.rs +++ b/src/error-handling/exercise.rs @@ -101,9 +101,7 @@ enum ParserError { fn parse(input: &str) -> Result { let mut tokens = tokenize(input); - fn parse_expr<'a>( - tokens: &mut Tokenizer<'a>, - ) -> Result { + fn parse_expr(tokens: &mut Tokenizer<'_>) -> Result { let tok = tokens.next().ok_or(ParserError::UnexpectedEOF)??; let expr = match tok { Token::Number(num) => { diff --git a/src/iterators/exercise.rs b/src/iterators/exercise.rs index 2d9786073349..3f0c302c3d1d 100644 --- a/src/iterators/exercise.rs +++ b/src/iterators/exercise.rs @@ -25,8 +25,8 @@ where N: Copy + std::ops::Sub, { // ANCHOR_END: offset_differences - let a = (&values).into_iter(); - let b = (&values).into_iter().cycle().skip(offset); + let a = values.iter(); + let b = values.iter().cycle().skip(offset); a.zip(b).map(|(a, b)| *b - *a).collect() } diff --git a/src/tuples-and-arrays/exercise.rs b/src/tuples-and-arrays/exercise.rs index 43291f5fe596..84a9acdc7f7c 100644 --- a/src/tuples-and-arrays/exercise.rs +++ b/src/tuples-and-arrays/exercise.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +// Iterators are covered later. +#[allow(clippy::needless_range_loop)] // ANCHOR: solution // ANCHOR: transpose fn transpose(matrix: [[i32; 3]; 3]) -> [[i32; 3]; 3] { diff --git a/src/types-and-values/exercise.rs b/src/types-and-values/exercise.rs index 01a8ad4aed07..8c84b44cd0f3 100644 --- a/src/types-and-values/exercise.rs +++ b/src/types-and-values/exercise.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +// Omitting `return` is covered later. +#[allow(clippy::needless_return)] // ANCHOR: solution // ANCHOR: fib fn fib(n: u32) -> u32 { diff --git a/src/unsafe-rust/exercise.rs b/src/unsafe-rust/exercise.rs index 0eb0cc6c7752..618aef46952f 100644 --- a/src/unsafe-rust/exercise.rs +++ b/src/unsafe-rust/exercise.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#[allow(clippy::upper_case_acronyms)] // ANCHOR: solution // ANCHOR: ffi mod ffi { @@ -40,7 +41,7 @@ mod ffi { } // Layout according to the macOS man page for dir(5). - #[cfg(all(target_os = "macos"))] + #[cfg(target_os = "macos")] #[repr(C)] pub struct dirent { pub d_fileno: u64, diff --git a/third_party/rust-on-exercism/health-statistics.rs b/third_party/rust-on-exercism/health-statistics.rs index 52ab7ebb72e9..bb33c065a738 100644 --- a/third_party/rust-on-exercism/health-statistics.rs +++ b/third_party/rust-on-exercism/health-statistics.rs @@ -37,12 +37,9 @@ impl User { patient_name: &self.name, visit_count: self.visit_count as u32, height_change: measurements.height - self.height, - blood_pressure_change: match self.last_blood_pressure { - Some(lbp) => { - Some((bp.0 as i32 - lbp.0 as i32, bp.1 as i32 - lbp.1 as i32)) - } - None => None, - }, + blood_pressure_change: self + .last_blood_pressure + .map(|lbp| (bp.0 as i32 - lbp.0 as i32, bp.1 as i32 - lbp.1 as i32)), }; self.height = measurements.height; self.last_blood_pressure = Some(bp);