Oh rust doctest lints, where art þou? (Add a way to run clippy on doctests)

[llogiq]

Currently, doctests benefit from being code in numerous ways, such as being tested. However, this unfortunately does not (yet?) apply to clippy lints. For (an atmittedly contrived) example:

/// is the given number odd?
///
/// # Examples
///
/// ```rust
///# use testdoclints::is_odd;
/// let mut a = 1;
/// a = a + 1; // this should lint `clippy::assign_op_pattern`
/// assert!(!is_odd(a));
/// ```
pub fn is_odd(x: usize) -> bool {
    (x & 1) == 1
}

Running cargo clippy shows no lint.

To solve this, we'd need to be able to hook into the test code generation and present the resulting AST and HIR to our lints. I am unsure where to put this issue, but as clippy is not the only source of custom lints, I think solving it within rust/rustdoc makes sense.

cc @Manishearth @oli-obk

[Nemo157]

It seems like having cargo check able to check doctests as well would be handy (and would probably get clippy 99% of the way to working).

[llogiq]

@QuietMisdreavus @GuillaumeGomez any idea how we could coax rustdoc into extracting the doctests for us to lint?

[GuillaumeGomez]

Well, we already extract the doctest but just for compilation/run at the moment. The only question I have is: what interface do you want rustdoc to provide to do so? Running specifically clippy on the returned code samples or something more generic? And if so, then what would it look like?

[llogiq]

I think there might be interest in running both check (for quick checks) and clippy (for deeper inspection). However, we may need to somehow flag the code for compile-fail tests (& possibly avoid them in clippy runs by default, as clippy doesn't like non-compiling code very much).

[GuillaumeGomez]

So something more generic. I see a few different possibilities. Like providing a Rust file like build.rs that would be run on the different code samples provided. How does it sound?

[Manishearth]

That's not really useful for tooling, tooling shouldn't have to add new files to the tree to be able to run tests.

[GuillaumeGomez]

I don't have other ideas to do this nicely. But if you have any, please tell so! :)

[llogiq]

@GuillaumeGomez we have a custom driver (as has cargo check, IIRC), so the most elegant solution in terms of UI would be giving an expansion of the doctests with the respective spans in the code to our custom driver.

[Manishearth]

Actually I think this should wait until cargo-clippy has fully moved into Cargo, and then we can add a cargo doc --test --clippy mode that does this correctly.

Ultimately clippy behaves just like another rustc binary, and we're slowly consolidating all of the actual cargo logic in cargo itself.

[GuillaumeGomez]

Fine by me!

[Nemo157]

@Manishearth currently examples are tested by running cargo test --doc, with that as precedence it seems like cargo check --doc and cargo clippy --doc would be the appropriate flags to lint the examples as well.

Since cargo check is already built into Cargo, it seems like whatever path the clippy driver would take could already be developed as part of that and then just extended to cargo clippy once the move is complete.

[jyn514]

Vaguely related to #73314

[jyn514]

I think the proper way to do this is something like this:

• Add a new --check-tests flag to rustdoc (or we can make it --test --no-compile, but I don't know if that's any more clear) which passes --emit=metadata to rustc. In this mode rustdoc will just print the output of rustc without marking the test as passed or failed; this addresses @Nemo157's concern in Rename "--display-warnings" to "--display-doctest-warnings" #73314 (comment) that the output looks like a test suite instead of a tool lint. compile_fail tests will not emit any output.
• Add a cargo clippy --doc flag to cargo which passes --check-tests --test-runner clippy-driver to rustdoc.
  • Cargo could do something similar for cargo check --doc; hmm maybe now I'm in favor of calling it --doc instead of --doctest after all (Feature request: cargo check for documentation cargo#10025 (comment)). That's a separate question though, the basic mechanism is the same.

How does that sound?

I wonder if this is large enough to need an rfc ...

[jplatte]

This would be very very helpful for rust-analyzer as well, I think. Currently when writing non-trivial example code, I have no inline errors in my editor which reduces my productivity a bunch.

[Nemo157]

I finally got round to writing a little script that manages to run clippy against doctests using the currently available (unstable) options: https://github.com/Nemo157/dotfiles/blob/master/bin/cargo-rustdoc-clippy

I haven't tested it very widely, and it doesn't have the greatest output because you still see all the rustdoc test-runner output, but it works quite well on the few crates I've tried it on.

[IceTDrinker]

stumbled upon this while digging in rustdoc for an unrelated issue, what's the status on this ? Or how would one go about potentially contributing something ?

[ramosbugs]

I finally got round to writing a little script that manages to run clippy against doctests using the currently available (unstable) options: https://github.com/Nemo157/dotfiles/blob/master/bin/cargo-rustdoc-clippy

I haven't tested it very widely, and it doesn't have the greatest output because you still see all the rustdoc test-runner output, but it works quite well on the few crates I've tried it on.

This is great! For others who stumble on this issue, the script above has moved to https://github.com/Nemo157/dotfiles/blob/74d54412ccb705551ef0c8d928d64bc9e6de69de/packages/cargo-rustdoc-clippy

After copying it to somewhere in your PATH, just run:

cargo +nightly rustdoc-clippy

[IceTDrinker]

Building on #56232 (comment)

you can actually achieve the same effect with a plain cargo command (here less configurable, but seems like a good enough baseline, at least for us):

RUSTDOCFLAGS="--no-run --nocapture --test-builder clippy-driver -Z unstable-options" cargo +nightly test --doc

clippy emitting a warning must return a non 0 exit code, making things fails if there are some warnings in doctests

Edit: actually seems warning is not enough so there may still be a need for an intermediate script to forward the clippy flags

Edit2: simpler more "hard coded" construct:

clippy_driver.sh

#!/usr/bin/env bash

set -eu

exec clippy-driver ${CLIPPYFLAGS} $@

CLIPPYFLAGS="-D warnings" RUSTDOCFLAGS="--no-run --nocapture --test-builder ./clippy_driver.sh -Z unstable-options" cargo +nightly test --doc

[mayeul-zama]

Btw, as described in https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html#showing-warnings-in-doctests, some unused warnings are disabled by default on doctests.

With clippy, they are still disabled by default while more "sophisticated" lints are enabled.

They can be reenabled globally by using #![doc(test(attr(warn(unused))))] in the crate root.