DO NOT MERGE: travis: Run Clippy

[petertseng]

As of submission time, this is an experiment and I don't know how well it will go, but we'll see.

[petertseng]

So question is whether to do anything about any of this. Whether to actually change anything as a result of the report. I think the primary focus should be on any in tests/ since they are visible to students. We can think about the examples later.

Things I think it'd be good to change soon:

• Book-store is comparing floats for equality. It'd be good to change to integer number of cents.

Things that would be fine to change, but I'm not in a rush:

• A lot of "Constants have by default a 'static lifetime"
• A lot of "it is more idiomatic to loop over references to containers instead of using explicit iteration methods"
• a few "This expression borrows a reference that is immediately dereferenced by the compiler"
• a few "this argument is passed by value, but not consumed in the function body"
• dominoes "you don't need to add & to all patterns"
• dominoes "length comparison to zero"
• nucleotide-codons and protein-translation "unneeded return statement"
• react, poker, simple-linked-list, sum-of-multiples "useless use of vec!"

Things I'm not sure how I feel about:

• A few "Writing &Vec<_> instead of &[_] involves one more reference and cannot be used with non-Vec-based slices.". Maybe we can change that but beware if it makes the types less understandable.
• A lot of "long literal lacking separators".

[petertseng]

... and also whether to merge this so results can be continually checked. Seems fine.

[coriolinus]

This is cool! Might want to expand the README as well to let new contributors know that this information is available.

I agree that using integer cents in book-store is high priority, and everything else can be touched up as people have time.

Underscore-separated integer literals seem like a clear win to me. They aren't a feature that I've seen in other languages, but they're part of idiomatic Rust: otherwise, Clippy wouldn't have a lint for them. There's no learning barrier; it's immediately obvious what number is represented. The hardest part of all this will be writing a little script to parse the clippy results and replace the digits of the appropriate files.

I'd argue that the slice type &[_] is an important part of Rust, and something we need to teach to students, probably early on. It might be worth digging through the list of exercises and finding out where a slice is first part of a function signature, and adding a note about them to the effect that "slices are primitives atop which Vecs are built, and a Vec automatically dereferences into a slice. Because other types may also dereference into slices, it's considered good form to use slices instead of Vecs as function arguments." We could also do it in two parts, introducing the notation in one exercise and supplying some depth in another. The details of the pedagogy aren't critical to sort out right now. What's important is to define the goal, and I believe that the goal to which we should aim is one in which slices are used instead of Vec references throughout the entire sequence of exercises, because that's correct usage. We want to always demonstrate good coding style in code which is presented to the students.

[petertseng]

How I'll handle this: I'll create an issue for moving to integers in book-store soon. It is not guaranteed whether I will be the one doing that, or someone else. When I figure out what to write in the README, I will add to this PR accordingly.

[petertseng]

I think the second most important thing to tackle would be using &[_] where possible. We can also probably start on that without waiting for this PR to be merged. I'm prioritising accordingly.

[petertseng]

I'm a little suspicious of this: https://travis-ci.org/exercism/rust/jobs/362332100 shows no Clippy lints on the tests. I was sure there should be some. For example, we used to get one suggesting underscores in integer literals such as in https://github.com/exercism/rust/blob/master/exercises/largest-series-product/tests/largest-series-product.rs#L49 . I will check whether that particular lint is simply allow by default these days.

[petertseng]

I will check whether that particular lint is simply allow by default these days.

Negative. It is warn. https://rust-lang-nursery.github.io/rust-clippy/v0.0.191/index.html#identity_conversion

[petertseng]

... but it was increased to >5 digits. rust-lang/rust-clippy@e9d5a31

[petertseng]

OK, well, now I know that Clippy lints from tests are definitely not showing up (whereas they used to!)

Since I care more about tests than the examples, this threatens to make this PR useless unless there is a way to get them to show.

[coriolinus]

Per rust-lang/rust-clippy#1436, we'd need to add the flags -- --cfg test to the cargo clippy command to get it to operate on tests. That's from over a year ago, so I can't confirm that it's current, but that seems like a plausible path to investigate.

[petertseng]

Last known build where Clippy was run on the tests (187, nightly = 2789b067d 2018-03-06):

https://travis-ci.org/exercism/rust/jobs/352617705

Failed attempts: Clippy no longer builds these versions as of today (clippy says it needs to be built by the same nightly version of rustc, so I could consider going to those versions...?) nightly = fb44b4c0e 2018-04-04

• 187: https://travis-ci.org/exercism/rust/builds/362785423
• 188: https://travis-ci.org/exercism/rust/jobs/362776910
• 189: https://travis-ci.org/exercism/rust/builds/362780033
• 190: https://travis-ci.org/exercism/rust/builds/362782447

Clippy demonstrably not running against tests:

• 191: https://travis-ci.org/exercism/rust/jobs/362386480

[petertseng]

confirmed that 187 is checking tests. I'll binary search between 187 and 191

[petertseng]

189: yes, does run against tests: https://travis-ci.org/exercism/rust/builds/362821482

[petertseng]

190: yes does test https://travis-ci.org/exercism/rust/builds/362833754

[petertseng]

191 did not lint tests. https://travis-ci.org/exercism/rust/builds/362854069

[petertseng]

To see what we're seeing in our tests, I'll download the log (https://api.travis-ci.org/v3/job/362921121/log.txt) and run the following on it

grep -B1 tests/ log.txt | grep warning | sort | uniq -c | sort -nr

     24 warning: long literal lacking separators
     18 warning: this expression borrows a reference that is immediately dereferenced by the compiler
      4 warning: it is more idiomatic to loop over references to containers instead of using explicit iteration methods
      4 warning: Constants have by default a `'static` lifetime
      2 warning: unneeded return statement
      2 warning: it is more idiomatic to loop over containers instead of using explicit iteration methods`
      1 warning: you should put `HashSet` between ticks in the documentation
      1 warning: you should put `CallbackRecorder` between ticks in the documentation
      1 warning: you don't need to add `&` to all patterns
      1 warning: this argument is passed by value, but not consumed in the function body
      1 warning: statement with no effect
      1 warning: length comparison to zero

Interesting. and we can do the same for our examples by looking for src/lib.rs.

     21 warning: redundant field names in struct initialization
     11 warning: it is more idiomatic to loop over references to containers instead of using explicit iteration methods
     10 warning: this expression borrows a reference that is immediately dereferenced by the compiler
     10 warning: needlessly taken reference of left operand
      8 warning: this creates an owned instance just for comparison
      8 warning: long literal lacking separators
      7 warning: using `clone` on a `Copy` type
      7 warning: this argument is passed by value, but not consumed in the function body
      7 warning: Constants have by default a `'static` lifetime
      6 warning: length comparison to zero
      4 warning: redundant closure found
      4 warning: manual implementation of an assign operation
      3 warning: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices.
      3 warning: use of `or_insert` followed by a function call
      2 warning: writing `&String` instead of `&str` involves a new object where a slice will do.
      2 warning: unneeded return statement
      2 warning: this `match` has identical arm bodies
      2 warning: this `.fold` can be written more succinctly using another method
      2 warning: single-character string constant used as pattern
      2 warning: casting u8 to u32 may become silently lossy if types change
      2 warning: called `cloned().collect()` on a slice to create a `Vec`. Calling `to_vec()` is both faster and more readable
      1 warning: you should use the `starts_with` method
      1 warning: you should put `to_base` between ticks in the documentation
      1 warning: you should put `GroupedBasket` between ticks in the documentation
      1 warning: you should put `from_base` between ticks in the documentation
      1 warning: you should consider deriving a `Default` implementation for `SimpleLinkedList<T>`
      1 warning: you should consider deriving a `Default` implementation for `School`
      1 warning: you should consider deriving a `Default` implementation for `Robot`
      1 warning: you should consider deriving a `Default` implementation for `Reactor<'a, T>`
      1 warning: you should consider deriving a `Default` implementation for `Forth`
      1 warning: you should consider deriving a `Default` implementation for `BowlingGame`
      1 warning: you seem to be using .map() to clone the contents of an _, consider using `.cloned()`
      1 warning: you are implementing `Hash` explicitly but have derived `PartialEq`
      1 warning: this pattern creates a reference to a reference
      1 warning: this `if` has identical blocks
      1 warning: this `else { if .. }` block can be collapsed
      1 warning: the loop variable `idx` is only used to index `book_groups`.
      1 warning: Suspicious use of binary operator in `Mul` impl
      1 warning: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name
      1 warning: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
      1 warning: it is more idiomatic to loop over containers instead of using explicit iteration methods`
      1 warning: item `SimpleLinkedList<T>` has a public `len` method but no corresponding `is_empty` method
      1 warning: I see you're using a LinkedList! Perhaps you meant some other data structure?
      1 warning: identical conversion
      1 warning: explicit lifetimes given in parameter types where they could be elided
      1 warning: casting u32 to u64 may become silently lossy if types change
      1 warning: casting character literal to u8. `char`s are 4 bytes wide in rust, so casting to u8 truncates them
      1 warning: called `.get().unwrap()` on a Vec. Using `[]` is more clear and more concise
      1 warning: called `.get().unwrap()` on a HashMap. Using `[]` is more clear and more concise
      1 warning: called `.get_mut().unwrap()` on a Vec. Using `[]` is more clear and more concise

[petertseng]

I am sorry to report.

I have gotten all the value I am going to get out of this PR (the IMO high priority lints).

As of my last check, if I ran this on recent versions of clippy, I would get no lint output from the tests. I was unable to solve that. It does not make sense to keep on an old version of Rust+Clippy. I will confirm one last time, and close it.

[petertseng]

I believe this would have value. It would be good if all examples and tests were clean (as defined by Clippy), and that this were enforced by CI. We would have a long road to get there, but that can be done piecemeal. But we cannot even start if our tests are not checked. I will not be the person to solve this problem. Feel free to pick up from here and try to solve it.

[petertseng]

So. This PR is Interesting because it shows what could be, and we can use it to show current clippy items in tests and example solutions.

The tests are higher priority because they are exposed to students, whereas the example solutions are lower priority.

We could do some sort of staged enforcement, where:

• things that currently have clippy suggestions will get a marker file that says that's okay like .meta/not-ready-for-clippy
• CI will ensure that everything else has no clippy suggestions
• over time, we heed clippy's suggestions and remove those marker files

By the way, here are clippy's current suggestions on the tests.

$ grep -B1 tests/ a.txt | grep warning | sort | uniq -c | sort -nr
  26 warning: long literal lacking separators
  17 warning: `cfg_attr` is deprecated for rustfmt and got replaced by tool_attributes
   4 warning: calling `as_bytes()` on a string literal
   3 warning: Constants have by default a `'static` lifetime
   2 warning: useless use of `vec!`
   2 warning: use of `expect` followed by a function call
   2 warning: unneeded return statement
   1 warning: you don't need to add `&` to all patterns
   1 warning: this argument is passed by value, but not consumed in the function body
   1 warning: this argument is passed by reference, but would be more efficient if passed by value
   1 warning: length comparison to zero

Given my available time and priorities, I'm not in the position to enact the above plan. So I will be closing this PR so that people don't give the wrong idea. However, it could be a good starting point for anyone else who might want to do the same.

[petertseng]

As of my last check, if I ran this on recent versions of clippy, I would get no lint output from the tests

Huh, I seem to have solved that by using the --all-targets suggestion from #659 .

This will be a "reopen, trigger Travis CI, generate report, close" operation. I make no guarantees about any future plans to do this, or that I will make any effort to automate it.

[petertseng]

Changes since last time I tried: It's now clippy not clippy-preview, and apparently it's not available on nightly (neither clippy nor clippy-preview) so I have to use stable

[petertseng]

We are apparently not doing so bad on our tests!

$ grep -B1 tests/ log.txt | grep warning | sort | uniq -c | sort -nr
  26 warning: long literal lacking separators
  17 warning: `cfg_attr` is deprecated for rustfmt and got replaced by tool_attributes
   4 warning: calling `as_bytes()` on a string literal
   1 warning: unneeded return statement
   1 warning: an inclusive range would be more readable

As for the example solutions (remember that example solution gets moved to src/lib.rs before clippy is run)

$ grep -B1 src/lib.rs log.txt | grep warning | sort | uniq -c | sort -nr
  21 warning: this argument is passed by reference, but would be more efficient if passed by value
  16 warning: long literal lacking separators
  14 warning: Constants have by default a `'static` lifetime
   6 warning: casting character literal to u8. `char`s are 4 bytes wide in rust, so casting to u8 truncates them
   4 warning: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices.
   2 warning: you should consider deriving a `Default` implementation for `graph::Graph`
   2 warning: you should consider deriving a `Default` implementation for `SimpleLinkedList<T>`
   2 warning: you should consider deriving a `Default` implementation for `Robot`
   2 warning: you should consider deriving a `Default` implementation for `Reactor<'a, T>`
   2 warning: you should consider deriving a `Default` implementation for `Forth`
   2 warning: you should consider deriving a `Default` implementation for `BowlingGame`
   2 warning: this `else { if .. }` block can be collapsed
   2 warning: item `SimpleLinkedList<T>` has a public `len` method but no corresponding `is_empty` method
   2 warning: called `.get().unwrap()` on a HashMap. Using `[]` is more clear and more concise
   1 warning: you should consider deriving a `Default` implementation for `School`
   1 warning: methods called `into_*` usually take self by value; consider choosing a less ambiguous name

Note that there is no provision right now that runs clippy for the stubs.

All right this was interesting to see, so back to closing this now.

[petertseng]

I'll also look for error not just warning, just have missed that.

tests:

$ grep -B1 tests/ log.txt | grep 'warning\|error' | sort | uniq -c | sort -nr
   4 warning: calling `as_bytes()` on a string literal
   4 warning: Statics have by default a `'static` lifetime
   1 error: this .into_iter() call is equivalent to .iter() and will not move the array
   1 warning: unneeded return statement

example solns:

$ grep -B1 src/lib.rs log.txt | grep 'warning\|error' | sort | uniq -c | sort -nr
  14 warning: long literal lacking separators
  14 warning: Constants have by default a `'static` lifetime
  10 warning: Statics have by default a `'static` lifetime
   8 warning: this argument (1 byte) is passed by reference, but would be more efficient if passed by value (limit: 8 byte)
   6 warning: this argument (8 byte) is passed by reference, but would be more efficient if passed by value (limit: 8 byte)
   5 warning: casting character literal to u8. `char`s are 4 bytes wide in rust, so casting to u8 truncates them
   4 warning: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices.
   4 warning: this argument (2 byte) is passed by reference, but would be more efficient if passed by value (limit: 8 byte)
   3 warning: this argument (4 byte) is passed by reference, but would be more efficient if passed by value (limit: 8 byte)
   2 error: you are implementing `Hash` explicitly but have derived `PartialEq`
   2 error: Suspicious use of binary operator in `Mul` impl
   2 warning: you should consider deriving a `Default` implementation for `graph::Graph`
   2 warning: you should consider deriving a `Default` implementation for `SimpleLinkedList<T>`
   2 warning: you should consider deriving a `Default` implementation for `School`
   2 warning: you should consider deriving a `Default` implementation for `Robot`
   2 warning: you should consider deriving a `Default` implementation for `Reactor<'a, T>`
   2 warning: you should consider deriving a `Default` implementation for `LinkedList<T>`
   2 warning: you should consider deriving a `Default` implementation for `Forth`
   2 warning: you should consider deriving a `Default` implementation for `BowlingGame`
   2 warning: this `else { if .. }` block can be collapsed
   2 warning: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
   2 warning: item `SimpleLinkedList<T>` has a public `len` method but no corresponding `is_empty` method
   2 warning: defining a method called `next` on this type; consider implementing the `std::iter::Iterator` trait or choosing a less ambiguous name
   1 error: this .into_iter() call is equivalent to .iter() and will not move the array
   1 warning: item `LinkedList<T>` has a public `len` method but no corresponding `is_empty` method

not bad at all on the tests.

[petertseng]

Need to change the line to remove the timestamp in front now that we moved to GitHub actions. New lines are:

$ grep -B1 tests/ gha-clippy-log.txt | sed 's/^20..-..-..T..:..:..........Z //' | grep 'warning\|error' | sort | uniq -c | sort -nr    
      4 warning: statics have by default a `'static` lifetime
      3 warning: Use sort_unstable instead of sort
      1 warning: unneeded `return` statement
      1 warning: this `.into_iter()` call is equivalent to `.iter()` and will not move the `array`

$ grep -B1 src/lib.rs gha-clippy-log.txt | sed 's/^20..-..-..T..:..:..........Z //' | grep 'warning\|error' | sort | uniq -c | sort -nr
     14 warning: constants have by default a `'static` lifetime
     10 warning: statics have by default a `'static` lifetime
      8 warning: `if` chain can be rewritten with `match`
      6 warning: Use sort_unstable instead of sort
      6 warning: redundant clone
      6 warning: casting a character literal to `u8` truncates
      6 warning: called `.nth(0)` on a `std::iter::Iterator`, when `.next()` is equivalent
      4 warning: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices.
      4 warning: unneeded unit return type
      2 warning: you should consider adding a `Default` implementation for `SimpleLinkedList<T>`
      2 warning: you should consider adding a `Default` implementation for `School`
      2 warning: you should consider adding a `Default` implementation for `Robot`
      2 warning: you should consider adding a `Default` implementation for `Reactor<'a, T>`
      2 warning: you should consider adding a `Default` implementation for `LinkedList<T>`
      2 warning: you should consider adding a `Default` implementation for `graph::Graph`
      2 warning: you should consider adding a `Default` implementation for `Forth`
      2 warning: you should consider adding a `Default` implementation for `BowlingGame`
      2 warning: using `Result.and_then(|x| Ok(y))`, which is more succinctly expressed as `map(|x| y)`
      2 warning: Using `.iter().next()` on an array
      2 warning: this `.into_iter()` call is equivalent to `.iter()` and will not move the `slice`
      2 warning: this `else { if .. }` block can be collapsed
      2 warning: redundant pattern matching, consider using `is_some()`
      2 warning: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
      2 warning: it looks like the same item is being pushed into this Vec
      2 warning: item `SimpleLinkedList<T>` has a public `len` method but no corresponding `is_empty` method
      2 warning: item `LinkedList<T>` has a public `len` method but no corresponding `is_empty` method
      2 warning: defining a method called `next` on this type; consider implementing the `std::iter::Iterator` trait or choosing a less ambiguous name
      2 warning: avoid using `collect()` when not needed
      2 error: you are implementing `Hash` explicitly but have derived `PartialEq`
      2 error: suspicious use of binary operator in `Mul` impl
      1 warning: this `.into_iter()` call is equivalent to `.iter()` and will not move the `array`