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

missing_docs triggers for integration tests #137561

Open
tv42 opened this issue Feb 24, 2025 · 9 comments
Open

missing_docs triggers for integration tests #137561

tv42 opened this issue Feb 24, 2025 · 9 comments
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-discussion Category: Discussion or questions that doesn't represent real issues. L-missing_docs Lint: missing_docs T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tv42
Copy link
Contributor

tv42 commented Feb 24, 2025

Summary

If you warn on missing_docs, every integration test causes a warning from clippy --tests.

I think integration tests should be exempt from missing_docs.

My editor (https://zed.dev) (and maybe rust-analyzer by default?) shows clippy --tests messages too (and not just clippy), so having any integration tests in the tree makes missing_docs practically unusable.

Lint Name

missing_docs

Reproducer

Using the fn add example from cargo init --lib

Add to Cargo.toml

[lints.rust]
missing_docs = "warn"

add new file tests/two_plus_two.rs

use rust_clippy_missing_docs_for_tests::add;

// literally just a copy-paste from the in-library test

#[test]
fn it_works() {
    let result = add(2, 2);
    assert_eq!(result, 4);
}

And now clippy --tests complains:

$ cargo clippy --tests
warning: missing documentation for the crate
 --> tests/two_plus_two.rs:1:1
  |
1 | / use rust_clippy_missing_docs_for_tests::add;
... |
8 | |     assert_eq!(result, 4);
9 | | }
  | |_^
  |
  = note: requested on the command line with `-W missing-docs`

warning: `rust-clippy-missing-docs-for-tests` (test "two_plus_two") generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.01s

Version

rustc 1.85.0 (4d91de4e4 2025-02-17)
binary: rustc
commit-hash: 4d91de4e48198da2e33413efdcd9cd2cc0c46688
commit-date: 2025-02-17
host: x86_64-unknown-linux-gnu
release: 1.85.0
LLVM version: 19.1.7

Additional Labels

No response

@tv42 tv42 added the C-bug Category: This is a bug. label Feb 24, 2025
@lapla-cogito
Copy link
Contributor

Note that missing_docs is a lint in rustc, not in clippy.

@tv42
Copy link
Contributor Author

tv42 commented Feb 24, 2025

Similar: rust-lang/rust-clippy#11024

@tv42
Copy link
Contributor Author

tv42 commented Feb 24, 2025

Note that missing_docs is a lint in rustc, not in clippy.

Yes, true. Should I move this issue elsewhere? I'm not very clear on the whole clippy-vs-rustc thing, apart from the work started in rustc and then got expanded as clippy.

@y21 y21 transferred this issue from rust-lang/rust-clippy Feb 24, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 24, 2025
@jieyouxu jieyouxu added the A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. label Feb 24, 2025
@lolbinarycat lolbinarycat added the L-missing_docs Lint: missing_docs label Feb 24, 2025
@lolbinarycat
Copy link
Contributor

Workaround is to enable it in the root module of your crate, instead of in Cargo.toml.

@ehuss
Copy link
Contributor

ehuss commented Feb 24, 2025

This looks like it changed recently in #130025. cc @Urgau

I think not honoring missing_docs on tests was intentional, as missing docs on test code is usually not something that you really want.

@lolbinarycat lolbinarycat added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 24, 2025
@ehuss
Copy link
Contributor

ehuss commented Feb 24, 2025

Though there was a related issue at #24584, where the behavior before 1.82 was a little awkward. If you are running cargo check in your editor with --profile=test (so that you get diagnostics for unit tests), the missing_docs lint would not work at all, which is not really what I wanted.

There seems to be some conflicting desires on how that behaves.

@Urgau
Copy link
Member

Urgau commented Feb 24, 2025

The change in #130025 was a bug-fix to make #[expect(missing_docs)] work when --test is passed. That bug-fix was necessary, and in general we should not silence lints based on secondary compiler flags.

The change in behavior was in the "Compatibility Notes" of 1.83.0:

The allow-by-default missing_docs lint used to disable itself when invoked through rustc --test/cargo test, resulting in #[expect(missing_docs)] emitting false positives due to the expectation being wrongly unfulfilled. This behavior #130025, which allows #[expect(missing_docs)] to be fulfilled in all scenarios, but will also report new missing_docs diagnostics for publicly reachable #[cfg(test)] items, integration test crate-level documentation, and publicly reachable items in integration tests.

I don't think there is anything (on rustc's side) to do here, the behavior is expected and IMO wanted, lints should be predictable and consistent.

I disagree that it makes "missing_docs practically unusable", IMO putting description of what tests are supposed to do is IMO a good thing.

In any case, one case always put #![allow(missing_docs)] in integration tests.

@lolbinarycat
Copy link
Contributor

I don't think there is anything (on rustc's side) to do here, the behavior is expected and IMO wanted, lints should be predictable and consistent.

I didn't realize rustc doesn't know if its running an integration test or unit test. It's easy to forget how relatively simple rustc's interface is when compared to cargo.

@lolbinarycat lolbinarycat added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed C-bug Category: This is a bug. labels Feb 24, 2025
@tv42
Copy link
Contributor Author

tv42 commented Feb 25, 2025

@lolbinarycat

Workaround is to enable it in the root module of your crate, instead of in Cargo.toml.

I have a workspace with lots of crates, so that's not a great answer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-discussion Category: Discussion or questions that doesn't represent real issues. L-missing_docs Lint: missing_docs T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants