Skip to content

clippy: remove needless borrows#1283

Merged
petertseng merged 1 commit intoexercism:mainfrom
petertseng:clippy
Jul 7, 2021
Merged

clippy: remove needless borrows#1283
petertseng merged 1 commit intoexercism:mainfrom
petertseng:clippy

Conversation

@petertseng
Copy link
Copy Markdown
Member

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 tournament: Put inputs/expectations inline, not in files #152 (comment)),
    and therefore require &. Consider carefully whether this may be
    confusing to students that some require & and some not.

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 #152 (comment)),
  and therefore require `&`. Consider carefully whether this may be
  confusing to students that some require `&` and some not.
@petertseng
Copy link
Copy Markdown
Member Author

I have determined that if we want to have some consistency in tournament, I originally wanted to avoid multiline string literals having indentation similar to:

fn test() {
    let scores = "header
line1
line2";
}

Perhaps https://doc.rust-lang.org/std/macro.concat.html could be an alternative.

@petertseng petertseng merged commit 056716e into exercism:main Jul 7, 2021
@petertseng petertseng deleted the clippy branch July 7, 2021 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants