Open llogiq opened 5 years ago
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).
@QuietMisdreavus @GuillaumeGomez any idea how we could coax rustdoc into extracting the doctests for us to lint?
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?
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).
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?
That's not really useful for tooling, tooling shouldn't have to add new files to the tree to be able to run tests.
I don't have other ideas to do this nicely. But if you have any, please tell so! :)
@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.
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.
Fine by me!
@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.
Vaguely related to #73314
I think the proper way to do this is something like this:
--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 https://github.com/rust-lang/rust/pull/73314#issuecomment-793767733 that the output looks like a test suite instead of a tool lint. compile_fail
tests will not emit any output.cargo clippy --doc
flag to cargo which passes --check-tests --test-runner clippy-driver
to rustdoc.
cargo check --doc
; hmm maybe now I'm in favor of calling it --doc
instead of --doctest
after all (https://github.com/rust-lang/cargo/issues/10025#issuecomment-980370681). 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 ...
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.
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.
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 ?
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
How are people not suffering from the lack of this feature
Building on https://github.com/rust-lang/rust/issues/56232#issuecomment-2046615285
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
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.
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:
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