Add lint unflagged_test_modules

[blyxyas]

Adds 2. from #10506.
The lint name was change from unflagged_tests_module to unflagged_test_modules to better fit lint naming conventions. This PR does not close completely #10506.

changelog: [unflagged_test_modules]: Add the lint.

[rustbot]

r? @Manishearth

(rustbot has picked a reviewer for you, use r? to override)

[bors]

☔ The latest upstream changes (presumably #10483) made this pull request unmergeable. Please resolve the merge conflicts.

[blyxyas]

I feel this lint needs to ignore items in testing directories

[Manishearth]

This has the potential to be really noisy, cc @rust-lang/clippy . Thoughts?

[Alexendoo]

#[test] fns aren't built when not compiling for tests, and if there were some helper/use/etc in the non-cfg'd test module that would already receive an unused warning e.g.

mod tests {
    fn helper() {}
    
    #[test]
    fn t() {
        helper();
    }
}

warning: function `helper` is never used
 --> src/lib.rs:2:8
  |
2 |     fn helper() {}
  |        ^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

So it wouldn't be saving compile time/space as listed in the Why is this bad?

[blyxyas]

@Alexendoo Then I'm pretty confused on the utility of the whole #[cfg(test)] attribute.
I extracted a lot of the "Why is this bad" from the book section about testing. What would be the reason to use #[cfg(test)] at all? Idiomatic conventions?

[Alexendoo]

If you had helper functions in the mod tests {} those would be compiled and give unused warning so it's still useful, same for mod tests { use some_test_dep::Foo; .. }. But those same warnings would alert you if you accidentally miss the #[cfg(tests)] already

Yeah the book does seem to miss that #[test] functions are not compiled outside of test mode

[Manishearth]

What would be the reason to use #[cfg(test)] at all? Idiomatic conventions?

Because otherwise you might need to include extra dependencies in normal mode, if your tests need them.

[blyxyas]

So... Should this lint exist at all? Are we still waiting for the Clippy team opinions about noisiness or it's just a bad idea of a lint?

[Manishearth]

I think waiting on clippy team opinions. It'll get discussed at the meeting tomorrow.

[Manishearth]

Actually, if it's pedantic I have no opposition to merging it. I'll still let people make the call tomorrow though.

Some salient points for the meeting:

• This lint triggers on modules that have test in the name: Should we be more specific? Only look for modules named test/tests? Or perhaps do something with segmenting on underscores?
• Are we sure that tcx.get_attrs() works reliably for cfgs? It seems to, but I'm not sure
• This lint doesn't look at complex things like cfg(any(test, ...)) though it's probably fine to require cfgs to be different

[matthiaskrgr]

Can someone run lintcheck against this and look if the lint warnings make sense for the crates?

[blyxyas]

After using cargo lintcheck, these were the stats:

### Stats:
| lint                                               | count |
| -------------------------------------------------- | ----- |
| clippy::cast_slice_different_sizes                 |     1 |
| clippy::checked_conversions                        |     1 |
| clippy::clone_on_copy                              |     1 |
| clippy::comparison_chain                           |     1 |
| clippy::enum_variant_names                         |     1 |
| clippy::expect_fun_call                            |     1 |
| clippy::explicit_deref_methods                     |     1 |
| clippy::extra_unused_lifetimes                     |     1 |
| clippy::field_reassign_with_default                |     1 |
| clippy::inconsistent_struct_constructor            |     1 |
| clippy::int_plus_one                               |     1 |
| clippy::manual_map                                 |     1 |
| clippy::manual_non_exhaustive                      |     1 |
| clippy::manual_saturating_arithmetic               |     1 |
| clippy::manual_str_repeat                          |     1 |
| clippy::match_on_vec_items                         |     1 |
| clippy::mut_mut                                    |     1 |
| clippy::nonminimal_bool                            |     1 |
| clippy::option_option                              |     1 |
| clippy::precedence                                 |     1 |
| clippy::range_plus_one                             |     1 |
| clippy::redundant_clone                            |     1 |
| clippy::self_named_constructors                    |     1 |
| clippy::should_implement_trait                     |     1 |
| clippy::stable_sort_primitive                      |     1 |
| clippy::unsafe_derive_deserialize                  |     1 |
| clippy::used_underscore_binding                    |     1 |
| clippy::verbose_bit_mask                           |     1 |
| clippy::borrow_as_ptr                              |     2 |
| clippy::collapsible_else_if                        |     2 |
| clippy::derived_hash_with_manual_eq                |     2 |
| clippy::from_iter_instead_of_collect               |     2 |
| clippy::from_over_into                             |     2 |
| clippy::implicit_clone                             |     2 |
| clippy::len_zero                                   |     2 |
| clippy::manual_string_new                          |     2 |
| clippy::manual_strip                               |     2 |
| clippy::option_as_ref_deref                        |     2 |
| clippy::question_mark                              |     2 |
| clippy::redundant_pattern_matching                 |     2 |
| clippy::redundant_slicing                          |     2 |
| clippy::single_char_add_str                        |     2 |
| clippy::type_complexity                            |     2 |
| clippy::unwrap_or_else_default                     |     2 |
| clippy::write_with_newline                         |     2 |
| clippy::crate_in_macro_def                         |     3 |
| clippy::get_first                                  |     3 |
| clippy::if_same_then_else                          |     3 |
| clippy::manual_is_ascii_check                      |     3 |
| clippy::multiple_crate_versions                    |     3 |
| clippy::negative_feature_names                     |     3 |
| clippy::single_char_pattern                        |     3 |
| clippy::zero_ptr                                   |     3 |
| clippy::bool_to_int_with_if                        |     4 |
| clippy::match_like_matches_macro                   |     4 |
| clippy::match_wildcard_for_single_variants         |     4 |
| clippy::needless_pass_by_value                     |     4 |
| clippy::redundant_feature_names                    |     4 |
| clippy::too_many_arguments                         |     4 |
| clippy::while_let_on_iterator                      |     4 |
| renamed_and_removed_lints                          |     4 |
| clippy::borrow_deref_ref                           |     5 |
| clippy::derivable_impls                            |     5 |
| clippy::explicit_iter_loop                         |     5 |
| clippy::identity_op                                |     5 |
| clippy::needless_return                            |     5 |
| clippy::new_without_default                        |     5 |
| clippy::single_component_path_imports              |     6 |
| clippy::transmute_float_to_int                     |     6 |
| clippy::explicit_into_iter_loop                    |     7 |
| clippy::map_clone                                  |     7 |
| clippy::mem_replace_with_default                   |     7 |
| clippy::needless_lifetimes                         |     7 |
| clippy::option_map_unit_fn                         |     7 |
| clippy::upper_case_acronyms                        |     7 |
| clippy::struct_excessive_bools                     |     8 |
| clippy::unusual_byte_groupings                     |     8 |
| clippy::len_without_is_empty                       |     9 |
| clippy::manual_range_contains                      |     9 |
| clippy::many_single_char_names                     |     9 |
| clippy::needless_doctest_main                      |    10 |
| clippy::map_unwrap_or                              |    11 |
| clippy::too_many_lines                             |    11 |
| clippy::unused_self                                |    11 |
| clippy::wrong_self_convention                      |    11 |
| clippy::unnecessary_wraps                          |    12 |
| clippy::cloned_instead_of_copied                   |    13 |
| clippy::linkedlist                                 |    14 |
| clippy::redundant_else                             |    14 |
| clippy::single_match_else                          |    15 |
| clippy::if_not_else                                |    16 |
| clippy::cargo_common_metadata                      |    19 |
| clippy::explicit_auto_deref                        |    19 |
| clippy::manual_assert                              |    19 |
| clippy::needless_borrowed_reference                |    19 |
| clippy::unnecessary_cast                           |    20 |
| clippy::unnested_or_patterns                       |    20 |
| clippy::default_trait_access                       |    21 |
| clippy::return_self_not_must_use                   |    22 |
| clippy::match_same_arms                            |    24 |
| clippy::redundant_closure_for_method_calls         |    24 |
| clippy::redundant_static_lifetimes                 |    24 |
| clippy::trivially_copy_pass_by_ref                 |    26 |
| clippy::missing_panics_doc                         |    32 |
| clippy::missing_safety_doc                         |    34 |
| clippy::enum_glob_use                              |    37 |
| clippy::cast_precision_loss                        |    39 |
| clippy::needless_borrow                            |    42 |
| clippy::cast_lossless                              |    44 |
| clippy::cast_possible_wrap                         |    45 |
| clippy::cast_sign_loss                             |    51 |
| clippy::missing_errors_doc                         |    53 |
| clippy::inline_always                              |    59 |
| clippy::manual_let_else                            |    66 |
| clippy::semicolon_if_nothing_returned              |    67 |
| clippy::similar_names                              |    71 |
| clippy::module_name_repetitions                    |    80 |
| clippy::items_after_statements                     |    86 |
| clippy::uninlined_format_args                      |    89 |
| clippy::ptr_as_ptr                                 |    93 |
| clippy::cast_possible_truncation                   |   101 |
| clippy::redundant_field_names                      |   112 |
| clippy::doc_markdown                               |   122 |
| clippy::wildcard_imports                           |   135 |
| clippy::expl_impl_clone_on_copy                    |   165 |
| clippy::must_use_candidate                         |   265 |
| clippy::unreadable_literal                         |   365 |

### ICEs:

0 instances of clippy::unflagged_test_modules in lintcheck

[Manishearth]

The general conclusion in the meeting was that this lint probably doesn't pull it's weight and is not worth merging.

There was a major concern about a lack of clarity of how cfg attributes get treaded in the AST: they've historically been sources of bugs for lints because when they get applied they get removed by the compiler, but they can still be seen from some APIs.

Another concern was this:

Implementation aside what's the benefit of the lint that wouldn't already trigger an unused warning?

You may wish to engage on Zulip (perhaps by opening a new thread) to discuss it further.

We're leaning towards closing this for now, but your work implementing and investigating this is highly appreciated! We'd like to apologize that it wasn't clear from the issue that this lint wasn't necessarily ready to be accepted.

[blyxyas]

Ok! Closing this and focusing on 1 and 3