From 425894b281b157dc683dc67267a649735fffae3d Mon Sep 17 00:00:00 2001 From: Peter Tseng Date: Wed, 7 Jul 2021 01:33:53 +0000 Subject: [PATCH] clippy: remove needless borrows This was made an error in Clippy recently, so we'll need to fix it if we want CI to work. According to https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow What it does: Checks for address of operations (`&`) that are going to be dereferenced immediately by the compiler. Why is this bad: Suggests that the receiver of the expression borrows the expression. Commentary as it applies to the Rust track: I can get behind the idea that it's better not to mislead the reader of the code about how many levels of reference the called function needs. Most of these stem from a misunderstanding of what expressions were already borrowed. They are listed below. Note carefully inconsistencies introduced in sublist and tournament, and consider whether we have a solution for this inconsistency before approving. The concern would be any confusion it may cause to those learning Rust. It is consistent from the standpoint of the type we're ultimately passing, but it's inconsistent from the standpoint of what the call sites look like. * Dominoes: The input is `&[Domino]`, and check takes `&[Domino]`. calling `check(&input)` passes `&&[Domino]` whereas we should call `check(input)` to pass `&[Domino]` * DOT DSL: `iter` iterates over `&T`. Therefore the `name` is a `&&str` (recall that string literals in Rust produce `&str`) and we are iterating over a `[&str]` producing `&&str`; since `Node::new` takes `&str`, just `name` would be better than `&name`, which would pass `&&&str` * grep: The patterns are string literals such as `"Agamemnon"`. Recall that string literals in Rust produce `&str`, and grep takes `&str`, so no additional `&` should be added. In fact, we should do this for the macro too, even though Clippy does not seem to catch those. * pangram: Recall that string literals in Rust produce `&str`, and is_pangram takes `&str`, so no additional `&` should be added. * sublist: The `v` was declared as `&[u32]`, and `sublist` takes two `&[T]`, so no additional `&` should be added. Inconsistency introduced: `sublist` is called with `&` in all other instances in this file because they are either slice literals or Vec, which do require the `&`. * tournament: Recall that string literals in Rust produce `&str`, and `tally` takes `&str`, so no additional `&` should be added. Inconsistency introduced: The first four cases use a string literal and thus do not require `&`. The next cases use `String` (recall that we decided that this was the best way to show the multi-line strings in https://github.com/exercism/rust/pull/152#discussion_r69383715), and therefore require `&`. Consider carefully whether this may be confusing to students that some require `&` and some not. --- exercises/practice/dominoes/tests/dominoes.rs | 2 +- exercises/practice/dot-dsl/tests/dot-dsl.rs | 2 +- exercises/practice/grep/tests/grep.rs | 8 ++++---- exercises/practice/pangram/tests/pangram.rs | 20 +++++++++---------- exercises/practice/sublist/tests/sublist.rs | 2 +- .../practice/tournament/tests/tournament.rs | 8 ++++---- 6 files changed, 21 insertions(+), 21 deletions(-) diff --git a/exercises/practice/dominoes/tests/dominoes.rs b/exercises/practice/dominoes/tests/dominoes.rs index 3fb41367b..5d0588c0c 100644 --- a/exercises/practice/dominoes/tests/dominoes.rs +++ b/exercises/practice/dominoes/tests/dominoes.rs @@ -65,7 +65,7 @@ fn check(input: &[Domino]) -> CheckResult { } fn assert_correct(input: &[Domino]) { - match check(&input) { + match check(input) { Correct => (), GotInvalid => panic!("Unexpectedly got invalid on input {:?}", input), ChainingFailure(output) => panic!( diff --git a/exercises/practice/dot-dsl/tests/dot-dsl.rs b/exercises/practice/dot-dsl/tests/dot-dsl.rs index 2d2ca1b4d..4811cb68e 100644 --- a/exercises/practice/dot-dsl/tests/dot-dsl.rs +++ b/exercises/practice/dot-dsl/tests/dot-dsl.rs @@ -130,7 +130,7 @@ fn test_graph_stores_attributes() { &["a", "b", "c"] .iter() .zip(attributes.iter()) - .map(|(name, &attr)| Node::new(&name).with_attrs(&[attr])) + .map(|(name, &attr)| Node::new(name).with_attrs(&[attr])) .collect::>(), ); diff --git a/exercises/practice/grep/tests/grep.rs b/exercises/practice/grep/tests/grep.rs index 87b5d2d26..f168ed4ca 100644 --- a/exercises/practice/grep/tests/grep.rs +++ b/exercises/practice/grep/tests/grep.rs @@ -127,7 +127,7 @@ macro_rules! set_up_test_case { let expected = vec![$($expected),*]; - process_grep_case(&pattern, &flags, &files, &expected); + process_grep_case(pattern, &flags, &files, &expected); } }; ($(#[$flag:meta])+ $test_case_name:ident(pattern=$pattern:expr, flags=[$($grep_flag:expr),*], files=[$($file:expr),+], prefix_expected=[$($expected:expr),*])) => { @@ -141,7 +141,7 @@ macro_rules! set_up_test_case { let expected = vec![$(concat!(stringify!($test_case_name), "_", $expected)),*]; - process_grep_case(&pattern, &flags, &files, &expected); + process_grep_case(pattern, &flags, &files, &expected); } } @@ -169,7 +169,7 @@ fn test_nonexistent_file_returns_error() { let files = vec!["test_nonexistent_file_returns_error_iliad.txt"]; - assert!(grep(&pattern, &flags, &files).is_err()); + assert!(grep(pattern, &flags, &files).is_err()); } #[test] @@ -185,7 +185,7 @@ fn test_grep_returns_result() { test_fixture.set_up(); - assert!(grep(&pattern, &flags, &files).is_ok()); + assert!(grep(pattern, &flags, &files).is_ok()); } // Test grepping a single file diff --git a/exercises/practice/pangram/tests/pangram.rs b/exercises/practice/pangram/tests/pangram.rs index 8db231d2e..27edf4cdf 100644 --- a/exercises/practice/pangram/tests/pangram.rs +++ b/exercises/practice/pangram/tests/pangram.rs @@ -3,68 +3,68 @@ use pangram::*; #[test] fn empty_strings_are_not_pangrams() { let sentence = ""; - assert!(!is_pangram(&sentence)); + assert!(!is_pangram(sentence)); } #[test] #[ignore] fn classic_pangram_is_a_pangram() { let sentence = "the quick brown fox jumps over the lazy dog"; - assert!(is_pangram(&sentence)); + assert!(is_pangram(sentence)); } #[test] #[ignore] fn pangrams_must_have_all_letters() { let sentence = "a quick movement of the enemy will jeopardize five gunboats"; - assert!(!is_pangram(&sentence)); + assert!(!is_pangram(sentence)); } #[test] #[ignore] fn pangrams_must_have_all_letters_two() { let sentence = "the quick brown fish jumps over the lazy dog"; - assert!(!is_pangram(&sentence)); + assert!(!is_pangram(sentence)); } #[test] #[ignore] fn pangrams_must_include_z() { let sentence = "the quick brown fox jumps over the lay dog"; - assert!(!is_pangram(&sentence)); + assert!(!is_pangram(sentence)); } #[test] #[ignore] fn underscores_do_not_affect_pangrams() { let sentence = "the_quick_brown_fox_jumps_over_the_lazy_dog"; - assert!(is_pangram(&sentence)); + assert!(is_pangram(sentence)); } #[test] #[ignore] fn numbers_do_not_affect_pangrams() { let sentence = "the 1 quick brown fox jumps over the 2 lazy dogs"; - assert!(is_pangram(&sentence)); + assert!(is_pangram(sentence)); } #[test] #[ignore] fn numbers_can_not_replace_letters() { let sentence = "7h3 qu1ck brown fox jumps ov3r 7h3 lazy dog"; - assert!(!is_pangram(&sentence)); + assert!(!is_pangram(sentence)); } #[test] #[ignore] fn capitals_and_punctuation_can_be_in_pangrams() { let sentence = "\"Five quacking Zephyrs jolt my wax bed.\""; - assert!(is_pangram(&sentence)); + assert!(is_pangram(sentence)); } #[test] #[ignore] fn non_ascii_characters_can_be_in_pangrams() { let sentence = "Victor jagt zwölf Boxkämpfer quer über den großen Sylter Deich."; - assert!(is_pangram(&sentence)); + assert!(is_pangram(sentence)); } diff --git a/exercises/practice/sublist/tests/sublist.rs b/exercises/practice/sublist/tests/sublist.rs index de12b46a2..b711c7424 100644 --- a/exercises/practice/sublist/tests/sublist.rs +++ b/exercises/practice/sublist/tests/sublist.rs @@ -4,7 +4,7 @@ use sublist::{sublist, Comparison}; fn empty_equals_empty() { let v: &[u32] = &[]; - assert_eq!(Comparison::Equal, sublist(&v, &v)); + assert_eq!(Comparison::Equal, sublist(v, v)); } #[test] diff --git a/exercises/practice/tournament/tests/tournament.rs b/exercises/practice/tournament/tests/tournament.rs index b969155e4..398e91807 100644 --- a/exercises/practice/tournament/tests/tournament.rs +++ b/exercises/practice/tournament/tests/tournament.rs @@ -3,7 +3,7 @@ fn just_the_header_if_no_input() { let input = ""; let expected = "Team | MP | W | D | L | P"; - assert_eq!(tournament::tally(&input), expected); + assert_eq!(tournament::tally(input), expected); } #[test] @@ -15,7 +15,7 @@ fn a_win_is_three_points_a_loss_is_zero_points() { + "Allegoric Alaskans | 1 | 1 | 0 | 0 | 3\n" + "Blithering Badgers | 1 | 0 | 0 | 1 | 0"; - assert_eq!(tournament::tally(&input), expected); + assert_eq!(tournament::tally(input), expected); } #[test] @@ -27,7 +27,7 @@ fn a_win_can_also_be_expressed_as_a_loss() { + "Allegoric Alaskans | 1 | 1 | 0 | 0 | 3\n" + "Blithering Badgers | 1 | 0 | 0 | 1 | 0"; - assert_eq!(tournament::tally(&input), expected); + assert_eq!(tournament::tally(input), expected); } #[test] @@ -39,7 +39,7 @@ fn a_different_team_can_win() { + "Blithering Badgers | 1 | 1 | 0 | 0 | 3\n" + "Allegoric Alaskans | 1 | 0 | 0 | 1 | 0"; - assert_eq!(tournament::tally(&input), expected); + assert_eq!(tournament::tally(input), expected); } #[test]