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

iter_nth_zero lint didn't display 'next' in recommendation #5783

Closed
warner opened this issue Jul 10, 2020 · 5 comments · Fixed by #5793
Closed

iter_nth_zero lint didn't display 'next' in recommendation #5783

warner opened this issue Jul 10, 2020 · 5 comments · Fixed by #5793
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy

Comments

@warner
Copy link
Contributor

warner commented Jul 10, 2020

I passed the following code to clippy:

        let adventure: Option<Direction> = directions()
            .iter()
            .filter(|d| match room.walls[wallnum(**d)] {
                Wall::Unknown => true,
                _ => false,
            })
            .copied()
            .nth(0);

It correctly identified that I could replace the nth(0) with a simple next, but the way it described what I should do was confusing, because it elided the final line:

$ cargo clippy
warning: called `.nth(0)` on a `std::iter::Iterator`
   --> src/day15/graph.rs:485:44
    |
485 |           let adventure: Option<Direction> = directions()
    |  ____________________________________________^
486 | |             .iter()
487 | |             .filter(|d| match room.walls[wallnum(**d)] {
488 | |                 Wall::Unknown => true,
...   |
491 | |             .copied()
492 | |             .nth(0);
    | |___________________^
    |
    = note: `#[warn(clippy::iter_nth_zero)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero
help: try calling
    |
485 |         let adventure: Option<Direction> = directions()
486 |             .iter()
487 |             .filter(|d| match room.walls[wallnum(**d)] {
488 |                 Wall::Unknown => true,
489 |                 _ => false,
490 |             })
  ...

warning: 1 warning emitted

    Finished dev [unoptimized + debuginfo] target(s) in 0.03s

I'm guessing that clippy has to trim lines when the code block is above some size threshold, but in this case the line it trimmed was the critical one; without it the recommendation doesn't make much sense to someone who doesn't remember the low-level Iterator API.

If the help: try calling ... output said help: try calling 'next()' instead of 'nth(0)', then the recommendation wouldn't depend quite so much upon the code sample.

Meta

$ cargo clippy -V
clippy 0.0.212 (bb37a0f 2020-06-16)

$ rustc -Vv
rustc 1.44.1 (c7087fe00 2020-06-17)
binary: rustc
commit-hash: c7087fe00d2ba919df1d813c040a5d47e43b0fe7
commit-date: 2020-06-17
host: x86_64-unknown-linux-gnu
release: 1.44.1
LLVM version: 9.0
@warner warner added the C-bug Category: Clippy is not doing the correct thing label Jul 10, 2020
@flip1995
Copy link
Member

We only use the rustc interface for diagnostics, so we can't change the truncation on our end.

But we could improve the message before the suggestion to: try calling .next()

@flip1995 flip1995 added the good-first-issue These issues are a good way to get started with Clippy label Jul 13, 2020
@warner
Copy link
Contributor Author

warner commented Jul 13, 2020

that sounds good.. if you could point me at the file where the message is created, I could try to write a patch.

@flip1995
Copy link
Member

Thanks, the message is generated here:

@warner
Copy link
Contributor Author

warner commented Jul 13, 2020

thanks, I'm on it

@warner
Copy link
Contributor Author

warner commented Jul 13, 2020

PR #5793 filed. I wasn't able to run cargo fmt, which might point to something stale about the getting-started docs. I don't usually work with anything except rustc stable. I did:

  • ./setup-toolchain.sh, I see that rustup toolchain list now has one named master, and my rust-clippy directory has an override set to use it
  • after making my changes and getting cargo test to work, I tried plain cargo fmt like I'm used to. That gave me an error:
error: 'cargo-fmt' is not installed for the toolchain 'master'
To install, run `rustup component add rustfmt --toolchain master
  • The advice it gave me failed:
% rustup component add rustfmt --toolchain master
error: master is a custom toolchain
  • The advice in doc/adding_lints.md failed:
% rustup component add rustfmt --toolchain=nightly
error: component 'rustfmt' for target 'x86_64-apple-darwin' is unavailable for download for channel nightly
Sometimes not all components are available in any given nightly.

That might be a transient error (tomorrow's nightly might have rustfmt available), but I was suspicious of the fact that the setup script is using a release named "master", while the adding_lints.md advice points at "nightly".

Anyways, I only changed a few literal strings, so I doubt I broke anything that rustfmt would complain about, and I'm sure CI will check it. But If some of the new-contributor advice is stale, then consider this a bug report on the docs :).

bors added a commit that referenced this issue Jul 14, 2020
improve advice in iter_nth_zero

fixes #5783

*Please keep the line below*
changelog:  For iter_nth_zero, the "use .next()" replacement advice is on the last line of the code snippet, where it is vulnerable to truncation. Display that advice at the beginning instead.
@bors bors closed this as completed in 201999c Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants