missing_headers: skip linting test functions, functions in test modules, and functions with #[cfg(test)] to avoid false positives

[loxoron218]

Summary

This PR fixes unnecessary warnings for missing_headers on test code.

Changes

Add logic to skip the missing_headers lint for:

• Test functions (functions named test_* or decorated with #[test])
• Functions inside test modules (mod tests { ... })
• Functions with #[cfg(test)] attribute

This avoids triggering the lint on test code where documentation headers are often unnecessary.

The logic should affect these lints:

• missing_errors_doc
• missing_panics_doc
• missing_safety_doc
• unnecessary_safety_doc

fixes #13391 and #12265

changelog: [missing_headers]: skip linting test functions, functions in test modules, and functions with #[cfg(test)] to avoid false positives

[rustbot]

Some changes occurred in clippy_lints/src/doc

cc @notriddle

[rustbot]

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 7 candidates
• 7 candidates expanded to 7 candidates
• Random selection from Jarcho, dswij, llogiq, samueltardieu

[ralpha]

I was looking at creating a fix for this too.
My solution was:

    // Do not lint if any parent has `#[test]` attribute (#12265)
    if let Some(body_id) = body_id
        && is_in_test_function(cx.tcx, body_id.hir_id)
    {
        return;
    }

But your solution seems to cover some other cases like #[cfg(test)]
For my solution see: ralpha@0cd339f

Also make sure to add tests to the case to prevent future regressions.

Something like this will do:

/// Should not lint on test functions even with check-private-items
#[test]
fn test_with_panic() {
    let result: Result<i32, &str> = Ok(1);
    result.unwrap();
}

[ralpha]

I asked my LLM to validate your solution 😅

Issues

1. Loses nested test function coverage

is_test_function only checks if the function itself is #[test]. The current fix uses is_in_test_function, which also catches functions nested inside a #[test] function. The proposed fix silently drops that coverage.

2. #[cfg(test)] suppression is scope creep

The original bug is specifically about #[test] functions. Adding #[cfg(test)] suppression is a separate concern. A documented public function inside a #[cfg(test)] module arguably should still be linted - someone writing doc comments with panics in a test module likely wants correctness there too. This changes behavior beyond the bug fix and should be a separate, deliberate decision.

3. is_in_test already exists

clippy_utils already provides is_in_test() (clippy_utils/src/lib.rs:2442):

pub fn is_in_test(tcx: TyCtxt<'_>, hir_id: HirId) -> bool {
    is_in_test_function(tcx, hir_id) || is_in_cfg_test(tcx, hir_id)
}

If broader suppression was desired, this single function call would replace all three calls in the proposed fix.

4. Unnecessary local_def_id_to_hir_id call

is_test_function already performs tcx.local_def_id_to_hir_id(fn_def_id) internally. The let hir_id = ... line in the proposed fix is only needed for the cfg_test calls, making it partially redundant.

Maybe that helps with reviewing. I think the first case might be an uncommon by valid case to consider.

[rustbot]

⚠️ Warning ⚠️

• There are issue links (such as #123) in the commit messages of the following commits.
  Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.
  • ea198f4

[loxoron218]

Thanks for your feedback, @ralpha! Your implementation looks kind of cleaner than mine, but would miss fn create_test_app_state here:

#[cfg(test)]
mod tests {
    use std::sync::{Arc, Weak};

    use {
        anyhow::{Result, bail},
        parking_lot::RwLock,
    };

    use crate::{
        audio::{
            decoder_types::DecoderError::NoAudioTrack, engine::AudioError,
            output::OutputError::ExclusiveModeFailed,
        },
        config::settings::SettingsManager,
        error::audio_reporting::handle_exclusive_mode_error,
        state::app_state::AppState,
    };

    fn create_test_app_state() -> Result<AppState> {
        let settings_manager = SettingsManager::new()?;
        Ok(AppState::new(
            Weak::new(),
            None,
            Arc::new(RwLock::new(settings_manager)),
        ))
    }

    #[test]
    fn test_handle_exclusive_mode_error_true() -> Result<()> {
        let reason = "Device busy".to_string();
        let error = AudioError::OutputError(ExclusiveModeFailed { reason });

        let app_state = create_test_app_state()?;
        let result = handle_exclusive_mode_error(&error, &app_state);
        if !result {
            bail!("Expected true");
        }
        Ok(())
    }

    #[test]
    fn test_handle_exclusive_mode_error_false() -> Result<()> {
        let decoder_error = NoAudioTrack;
        let error = AudioError::DecoderError(decoder_error);

        let app_state = create_test_app_state()?;
        let result = handle_exclusive_mode_error(&error, &app_state);
        if result {
            bail!("Expected false");
        }
        Ok(())
    }
}

Shouldn't we also suppress this function? I've adapted your changes to avoid sending a warning for that function which also addresses all the bullet points off your LLM review btw. :)

[ralpha]

Nice, I think the current solution is_in_test is what we want. It covers both the #[test] (and sub functions) and functions inside #[cfg(test)]. So this would cover I think all the cases we want.

Thanks for fixing this. I see all the tests pass now too, even with the added test case.

Hope to see the merged soon 🙌

[loxoron218]

Any chance of this being merged soon, @llogiq? Or is there anything that is missing 😄 ?