diff --git a/CHANGELOG.md b/CHANGELOG.md index 37f5eb09e524..591bc09e98f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7790,6 +7790,7 @@ Released 2018-09-13 [`allow-exact-repetitions`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-exact-repetitions [`allow-expect-in-consts`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-expect-in-consts [`allow-expect-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-expect-in-tests +[`allow-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-in-tests [`allow-indexing-slicing-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-indexing-slicing-in-tests [`allow-large-stack-frames-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-large-stack-frames-in-tests [`allow-mixed-uninlined-format-args`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-mixed-uninlined-format-args diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index f323d7040d26..0042d9e37227 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -64,6 +64,9 @@ Don't lint when comparing the result of a modulo operation to zero. ## `allow-dbg-in-tests` Whether `dbg!` should be allowed in test functions or `#[cfg(test)]` +Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write +`allow-in-tests = ["dbg_macro"]` instead. This option still works. + **Default Value:** `false` --- @@ -94,6 +97,9 @@ Whether `expect` should be allowed in code always evaluated at compile time ## `allow-expect-in-tests` Whether `expect` should be allowed in test functions or `#[cfg(test)]` +Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write +`allow-in-tests = ["expect_used"]` instead. This option still works. + **Default Value:** `false` --- @@ -101,9 +107,55 @@ Whether `expect` should be allowed in test functions or `#[cfg(test)]` * [`expect_used`](https://rust-lang.github.io/rust-clippy/main/index.html#expect_used) +## `allow-in-tests` +A list of Clippy lints to suppress in test functions and `#[cfg(test)]` items. + +Entries are **lint names**, and only the lints named here are affected. This is not a +blanket "allow everything in tests" switch: a lint you don't list keeps firing in test +code, and lints added to Clippy in future releases are never included unless you add +them. + +```toml +# `.expect()` is allowed in tests; `dbg!` is still reported there. +allow-in-tests = ["expect_used"] +``` + +#### Replaces the per-lint options + +The older options are deprecated but still honored. Note that an option's name does not +always match the lint's, so the replacements are: + +| deprecated option | write instead | +| --- | --- | +| `allow-dbg-in-tests = true` | `allow-in-tests = ["dbg_macro"]` | +| `allow-expect-in-tests = true` | `allow-in-tests = ["expect_used"]` | +| `allow-indexing-slicing-in-tests = true` | `allow-in-tests = ["indexing_slicing"]` | +| `allow-panic-in-tests = true` | `allow-in-tests = ["panic"]` | +| `allow-print-in-tests = true` | `allow-in-tests = ["print_stderr", "print_stdout"]` | +| `allow-unwrap-in-tests = true` | `allow-in-tests = ["unwrap_used"]` | +| `allow-useless-vec-in-tests = true` | `allow-in-tests = ["useless_vec"]` | + +The two combine permissively: a lint is suppressed in test code if it is listed here +**or** its own option is set to `true`. Setting that option to `false` does not cancel a +listing here. + +#### Noteworthy + +- This only suppresses lints. It cannot make a lint fire in test code that would not + fire otherwise. +- Suppressing a lint leaves an `#[expect]` for it in test code unfulfilled, so such an + attribute will report `unfulfilled_lint_expectations`. This matches how the older + per-lint options have always behaved. + +**Default Value:** `[]` + + ## `allow-indexing-slicing-in-tests` Whether `indexing_slicing` should be allowed in test functions or `#[cfg(test)]` +Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write +`allow-in-tests = ["indexing_slicing"]` instead. This option still works. + **Default Value:** `false` --- @@ -144,6 +196,9 @@ Whether to allow `r#""#` when `r""` can be used ## `allow-panic-in-tests` Whether `panic` should be allowed in test functions or `#[cfg(test)]` +Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write +`allow-in-tests = ["panic"]` instead. This option still works. + **Default Value:** `false` --- @@ -154,6 +209,9 @@ Whether `panic` should be allowed in test functions or `#[cfg(test)]` ## `allow-print-in-tests` Whether print macros (ex. `println!`) should be allowed in test functions or `#[cfg(test)]` +Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write +`allow-in-tests = ["print_stderr", "print_stdout"]` instead. This option still works. + **Default Value:** `false` --- @@ -207,6 +265,9 @@ Whether `unwrap` should be allowed in code always evaluated at compile time ## `allow-unwrap-in-tests` Whether `unwrap` should be allowed in test functions or `#[cfg(test)]` +Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write +`allow-in-tests = ["unwrap_used"]` instead. This option still works. + **Default Value:** `false` --- @@ -234,6 +295,9 @@ allow-unwrap-types = [ "std::sync::LockResult" ] ## `allow-useless-vec-in-tests` Whether `useless_vec` should ignore test functions or `#[cfg(test)]` +Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write +`allow-in-tests = ["useless_vec"]` instead. This option still works. + **Default Value:** `false` --- diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 7b2a7e0010f7..d2d3d585fcf3 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -10,7 +10,7 @@ use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; use rustc_hir::attrs::RustcVersion; use rustc_session::Session; -use rustc_span::{Pos as _, SourceFile, Symbol}; +use rustc_span::{Pos as _, SourceFile, Spanned, Symbol}; use std::path::PathBuf; use std::sync::{Arc, OnceLock}; use std::{env, fs, io}; @@ -77,6 +77,16 @@ macro_rules! define_Conf { $(#[doc = $doc:literal])* $(#[default_text = $default_text:literal])? $(#[rename = $new_name:ident])? + // Marks a `bool` field superseded by `allow-in-tests`, recording the lints its + // replacement should list. Documentation only for now: the option keeps working and + // says so in its docs, but setting it does not warn. + // + // TODO: once `allow-in-tests` has shipped in a stable release, emit a deprecation + // warning from here when the field is set to `true`. Setting one to `false` is the + // default and should stay silent, as `allow-in-tests` has no equivalent for it. + // + // Must precede `#[lints]`, which `cargo dev fmt` always re-emits last. + $(#[replaced_by_allow_in_tests($($replacement_lints:ident),* $(,)?)])? $(#[lints($($for_lints:ident),* $(,)?)])? // The type must exist for regular fields and shouldn't exist for deprecated ones. $name:ident($name_str:literal) $(: $ty:ty $(= $default:expr)?)?, @@ -215,6 +225,25 @@ macro_rules! define_Conf { fn check_conf_names() {$( assert_eq!(stringify!($name).replace('_', "-"), $name_str); )*} + + /// `#[replaced_by_allow_in_tests]` records the replacement in machine-readable form, but + /// until it emits a diagnostic the text users actually see lives in the doc comment. Keep + /// the two from drifting apart. + #[test] + fn check_replaced_by_allow_in_tests_docs() {$( + let _doc = concat!($($doc, '\n',)*); + $( + let expected = format!( + "`allow-in-tests = [{}]`", + [$(concat!("\"", stringify!($replacement_lints), "\"")),*].join(", "), + ); + assert!( + _doc.contains(&expected), + "`{}` is marked `#[replaced_by_allow_in_tests]` but its docs don't mention {expected}", + $name_str, + ); + )? + )*} }; } @@ -236,6 +265,10 @@ define_Conf! { #[lints(modulo_arithmetic)] allow_comparison_to_zero("allow-comparison-to-zero"): bool = true, /// Whether `dbg!` should be allowed in test functions or `#[cfg(test)]` + /// + /// Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write + /// `allow-in-tests = ["dbg_macro"]` instead. This option still works. + #[replaced_by_allow_in_tests(dbg_macro)] #[lints(dbg_macro)] allow_dbg_in_tests("allow-dbg-in-tests"): bool = false, /// Whether an item should be allowed to have the same name as its containing module @@ -245,9 +278,56 @@ define_Conf! { #[lints(expect_used)] allow_expect_in_consts("allow-expect-in-consts"): bool = true, /// Whether `expect` should be allowed in test functions or `#[cfg(test)]` + /// + /// Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write + /// `allow-in-tests = ["expect_used"]` instead. This option still works. + #[replaced_by_allow_in_tests(expect_used)] #[lints(expect_used)] allow_expect_in_tests("allow-expect-in-tests"): bool = false, + /// A list of Clippy lints to suppress in test functions and `#[cfg(test)]` items. + /// + /// Entries are **lint names**, and only the lints named here are affected. This is not a + /// blanket "allow everything in tests" switch: a lint you don't list keeps firing in test + /// code, and lints added to Clippy in future releases are never included unless you add + /// them. + /// + /// ```toml + /// # `.expect()` is allowed in tests; `dbg!` is still reported there. + /// allow-in-tests = ["expect_used"] + /// ``` + /// + /// #### Replaces the per-lint options + /// + /// The older options are deprecated but still honored. Note that an option's name does not + /// always match the lint's, so the replacements are: + /// + /// | deprecated option | write instead | + /// | --- | --- | + /// | `allow-dbg-in-tests = true` | `allow-in-tests = ["dbg_macro"]` | + /// | `allow-expect-in-tests = true` | `allow-in-tests = ["expect_used"]` | + /// | `allow-indexing-slicing-in-tests = true` | `allow-in-tests = ["indexing_slicing"]` | + /// | `allow-panic-in-tests = true` | `allow-in-tests = ["panic"]` | + /// | `allow-print-in-tests = true` | `allow-in-tests = ["print_stderr", "print_stdout"]` | + /// | `allow-unwrap-in-tests = true` | `allow-in-tests = ["unwrap_used"]` | + /// | `allow-useless-vec-in-tests = true` | `allow-in-tests = ["useless_vec"]` | + /// + /// The two combine permissively: a lint is suppressed in test code if it is listed here + /// **or** its own option is set to `true`. Setting that option to `false` does not cancel a + /// listing here. + /// + /// #### Noteworthy + /// + /// - This only suppresses lints. It cannot make a lint fire in test code that would not + /// fire otherwise. + /// - Suppressing a lint leaves an `#[expect]` for it in test code unfulfilled, so such an + /// attribute will report `unfulfilled_lint_expectations`. This matches how the older + /// per-lint options have always behaved. + allow_in_tests("allow-in-tests"): Vec>, /// Whether `indexing_slicing` should be allowed in test functions or `#[cfg(test)]` + /// + /// Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write + /// `allow-in-tests = ["indexing_slicing"]` instead. This option still works. + #[replaced_by_allow_in_tests(indexing_slicing)] #[lints(indexing_slicing)] allow_indexing_slicing_in_tests("allow-indexing-slicing-in-tests"): bool = false, /// Whether functions inside `#[cfg(test)]` modules or test functions should be checked. @@ -260,9 +340,17 @@ define_Conf! { #[lints(needless_raw_string_hashes)] allow_one_hash_in_raw_strings("allow-one-hash-in-raw-strings"): bool = false, /// Whether `panic` should be allowed in test functions or `#[cfg(test)]` + /// + /// Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write + /// `allow-in-tests = ["panic"]` instead. This option still works. + #[replaced_by_allow_in_tests(panic)] #[lints(panic)] allow_panic_in_tests("allow-panic-in-tests"): bool = false, /// Whether print macros (ex. `println!`) should be allowed in test functions or `#[cfg(test)]` + /// + /// Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write + /// `allow-in-tests = ["print_stderr", "print_stdout"]` instead. This option still works. + #[replaced_by_allow_in_tests(print_stderr, print_stdout)] #[lints(print_stderr, print_stdout)] allow_print_in_tests("allow-print-in-tests"): bool = false, /// Whether to allow module inception if it's not public. @@ -287,6 +375,10 @@ define_Conf! { #[lints(unwrap_used)] allow_unwrap_in_consts("allow-unwrap-in-consts"): bool = true, /// Whether `unwrap` should be allowed in test functions or `#[cfg(test)]` + /// + /// Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write + /// `allow-in-tests = ["unwrap_used"]` instead. This option still works. + #[replaced_by_allow_in_tests(unwrap_used)] #[lints(unwrap_used)] allow_unwrap_in_tests("allow-unwrap-in-tests"): bool = false, /// List of types to allow `unwrap()` and `expect()` on. @@ -299,6 +391,10 @@ define_Conf! { #[lints(expect_used, unwrap_used)] allow_unwrap_types("allow-unwrap-types"): Vec, /// Whether `useless_vec` should ignore test functions or `#[cfg(test)]` + /// + /// Deprecated in favor of [`allow-in-tests`](#allow-in-tests): write + /// `allow-in-tests = ["useless_vec"]` instead. This option still works. + #[replaced_by_allow_in_tests(useless_vec)] #[lints(useless_vec)] allow_useless_vec_in_tests("allow-useless-vec-in-tests"): bool = false, /// Additional dotfiles (files or directories starting with a dot) to allow diff --git a/clippy_config/src/metadata.rs b/clippy_config/src/metadata.rs index 415a7738011f..648f825c8555 100644 --- a/clippy_config/src/metadata.rs +++ b/clippy_config/src/metadata.rs @@ -26,17 +26,25 @@ impl ConfMetadata { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!( f, - "## `{}`\n{}\n\n**Default Value:** `{}`\n\n---\n**Affected lints:**\n{}\n\n", + "## `{}`\n{}\n\n**Default Value:** `{}`\n\n", self.0.name, self.0 .doc .lines() .format_with("\n", |doc, f| f(&doc.strip_prefix(" ").unwrap_or(doc))), self.0.default, - self.0.lints.iter().format_with("\n", |name, f| f(&format_args!( - "* [`{name}`](https://rust-lang.github.io/rust-clippy/main/index.html#{name})" - ))), - ) + )?; + // Options such as `allow-in-tests` apply to any lint, so they have no list to show. + if !self.0.lints.is_empty() { + write!( + f, + "---\n**Affected lints:**\n{}\n\n", + self.0.lints.iter().format_with("\n", |name, f| f(&format_args!( + "* [`{name}`](https://rust-lang.github.io/rust-clippy/main/index.html#{name})" + ))), + )?; + } + Ok(()) } } S(self) diff --git a/clippy_lints/src/arbitrary_source_item_ordering.rs b/clippy_lints/src/arbitrary_source_item_ordering.rs index 65ef6f63e977..f7e6f288ad7b 100644 --- a/clippy_lints/src/arbitrary_source_item_ordering.rs +++ b/clippy_lints/src/arbitrary_source_item_ordering.rs @@ -4,14 +4,14 @@ use clippy_config::types::{ SourceItemOrderingTraitAssocItemKind, SourceItemOrderingTraitAssocItemKinds, SourceItemOrderingWithinModuleItemGroupings, TraitImplItemOrder, }; -use clippy_utils::diagnostics::span_lint_and_note; +use clippy_utils::diagnostics::{ClippyLintContext, span_lint_and_note}; use clippy_utils::is_cfg_test; use rustc_hir::attrs::AttributeKind; use rustc_hir::{ Attribute, FieldDef, HirId, ImplItemId, IsAuto, Item, ItemKind, Mod, OwnerId, QPath, TraitItemId, TyKind, Variant, VariantData, }; -use rustc_lint::{LateContext, LateLintPass, LintContext, impl_lint_pass}; +use rustc_lint::{LateContext, LateLintPass, LintContext as _, impl_lint_pass}; use rustc_middle::ty::{AssocKind, TyCtxt}; use rustc_span::{Ident, Symbol}; @@ -226,7 +226,7 @@ impl ArbitrarySourceItemOrdering { } /// Produces a linting warning for incorrectly ordered item members. - fn lint_member_name(cx: &T, ident: Ident, before_ident: Ident) { + fn lint_member_name(cx: &T, ident: Ident, before_ident: Ident) { span_lint_and_note( cx, ARBITRARY_SOURCE_ITEM_ORDERING, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index cbecd381349c..73c9e790fb1e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -418,8 +418,11 @@ mod zombie_processes; use clippy_config::{Conf, sanitize_explanation}; use clippy_utils::macros::FormatArgsStorage; use rustc_data_structures::fx::FxHashSet; -use rustc_lint::is_lint_pass_required; +use rustc_errors::{Applicability, DiagCtxtHandle}; +use rustc_lint::{Lint, is_lint_pass_required}; use rustc_middle::ty::TyCtxt; +use rustc_span::edit_distance::find_best_match_for_name; +use rustc_span::{Spanned, Symbol}; use utils::attr_collector::AttrStorage; pub fn explain(name: &str) -> i32 { @@ -443,10 +446,70 @@ pub fn explain(name: &str) -> i32 { } } +/// Resolves one lint name from the `allow-in-tests` configuration. +/// +/// Accepts `unwrap_used`, `clippy::unwrap_used` and `unwrap-used` alike. A name which doesn't +/// refer to a Clippy lint is reported and ignored. +fn resolve_configured_lint(dcx: DiagCtxtHandle<'_>, name: &Spanned) -> Option<&'static Lint> { + let bare_name = name.node.strip_prefix("clippy::").unwrap_or(&name.node); + let lint_name = format!("clippy::{}", bare_name.replace('-', "_").to_ascii_uppercase()); + + if let Some(info) = declared_lints::LINTS.iter().find(|info| info.lint.name == lint_name) { + return Some(info.lint); + } + + let mut diag = dcx.struct_span_warn(name.span, format!("unknown lint: `{}`", name.node)); + diag.note("`allow-in-tests` only accepts Clippy lints"); + let renamed = deprecated_lints::RENAMED + .iter() + .find(|(old_name, _)| old_name.eq_ignore_ascii_case(&lint_name)); + if let Some((_, new_name)) = renamed { + let new_name = new_name.strip_prefix("clippy::").unwrap_or(new_name); + diag.span_suggestion( + name.span, + format!("`{}` has been renamed", name.node), + format!("\"{new_name}\""), + Applicability::MachineApplicable, + ); + } else if let Some(sugg) = find_best_match_for_name(&lint_symbols(), Symbol::intern(bare_name), None) { + diag.span_suggestion( + name.span, + "did you mean", + format!("\"{sugg}\""), + Applicability::MaybeIncorrect, + ); + } + diag.emit(); + None +} + +/// Resolves the lint names given in the `allow-in-tests` configuration and hands them to +/// `clippy_utils`, which drops their diagnostics when they are emitted from test code. +/// +/// Names which don't refer to a Clippy lint are reported and ignored. +fn register_lints_allowed_in_tests(dcx: DiagCtxtHandle<'_>, conf: &'static Conf) { + let allowed: Vec<_> = conf + .allow_in_tests + .iter() + .filter_map(|name| Some(resolve_configured_lint(dcx, name)?.name)) + .collect(); + clippy_utils::diagnostics::set_lints_allowed_in_tests(allowed); +} + +/// The names of all Clippy lints, without the `clippy::` prefix, for use in suggestions. +fn lint_symbols() -> Vec { + declared_lints::LINTS + .iter() + .map(|info| Symbol::intern(info.lint.name_lower().strip_prefix("clippy::").unwrap())) + .collect() +} + /// Register all lints and lint groups with the rustc lint store /// /// Used in `./src/driver.rs`. -pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Conf) { +pub fn register_lint_passes(dcx: DiagCtxtHandle<'_>, store: &mut rustc_lint::LintStore, conf: &'static Conf) { + register_lints_allowed_in_tests(dcx, conf); + for (old_name, new_name) in deprecated_lints::RENAMED { store.register_renamed(old_name, new_name); } @@ -492,6 +555,8 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co rustc_lint::early_lint_methods!( crate::combined_early_lint_pass, [CombinedEarlyLintPass, (conf: &'static Conf, format_args: FormatArgsStorage, attrs: AttrStorage), [ + // Must stay first: later passes consult the spans it records via `span_lint_and_then`. + TestSpanCollector: utils::test_span_collector::TestSpanCollector = utils::test_span_collector::TestSpanCollector, FormatArgsCollector: utils::format_args_collector::FormatArgsCollector = utils::format_args_collector::FormatArgsCollector::new(format_args.clone()), AttrCollector: utils::attr_collector::AttrCollector = utils::attr_collector::AttrCollector::new(attrs.clone()), PostExpansionEarlyAttributes: attrs::PostExpansionEarlyAttributes = attrs::PostExpansionEarlyAttributes::new(conf), diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 16066dd96c0a..8435151cc389 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -2,3 +2,4 @@ pub mod attr_collector; pub mod author; pub mod dump_hir; pub mod format_args_collector; +pub mod test_span_collector; diff --git a/clippy_lints/src/utils/test_span_collector.rs b/clippy_lints/src/utils/test_span_collector.rs new file mode 100644 index 000000000000..ebf9d11f5673 --- /dev/null +++ b/clippy_lints/src/utils/test_span_collector.rs @@ -0,0 +1,94 @@ +//! Records the spans of test code so that `allow-in-tests` covers early lint passes. +//! +//! A late pass resolves "is this test code?" by walking a node's HIR ancestors. Early passes have +//! no equivalent: `EarlyContext` carries only lint levels, not a parent chain. So instead of +//! asking the question per node, the answer is computed up front by walking the expanded AST once +//! and recording the span of every `#[cfg(test)]` item and `#[test]` function; emission then +//! matches a lint's span against those ranges. +//! +//! `check_crate` runs on the combined early pass before rustc visits any node, so the table is +//! always populated before the first lint can fire. + +use clippy_utils::diagnostics::{any_lint_allowed_in_tests, set_test_code_spans}; +use rustc_ast::attr::data_structures::CfgEntry; +use rustc_ast::visit::{self, Visitor}; +use rustc_ast::{AttrKind, Attribute, Crate, Item, ItemKind, ModKind, SyntheticAttr}; +use rustc_lint::{EarlyContext, EarlyLintPass, declare_lint_pass}; +use rustc_span::{Span, Symbol, sym}; + +declare_lint_pass!(TestSpanCollector => []); + +impl EarlyLintPass for TestSpanCollector { + fn check_crate(&mut self, _: &EarlyContext<'_>, krate: &Crate) { + // Nothing consults the spans unless the configuration names a lint, so don't pay for the + // walk in the common case. + if !any_lint_allowed_in_tests() { + return; + } + let mut visitor = TestSpans { spans: Vec::new() }; + visitor.collect_test_fns(&krate.items); + visit::walk_crate(&mut visitor, krate); + set_test_code_spans(visitor.spans); + } +} + +struct TestSpans { + spans: Vec, +} + +impl TestSpans { + /// Records the `#[test]` functions among `items`. + /// + /// Expansion consumes the `#[test]` attribute, leaving behind a `TestDescAndFn` constant + /// named after the function and marked `#[rustc_test_marker]`. Matching the function to that + /// constant by name is what `clippy_utils::is_in_test_function` does for late passes. + fn collect_test_fns(&mut self, items: &[Box]) { + let names: Vec = items + .iter() + .filter_map(|item| match &item.kind { + ItemKind::Const(konst) if item.attrs.iter().any(is_test_marker) => Some(konst.ident.name), + _ => None, + }) + .collect(); + if names.is_empty() { + return; + } + for item in items { + if let ItemKind::Fn(func) = &item.kind + && names.contains(&func.ident.name) + { + self.spans.push(item.span); + } + } + } +} + +impl<'ast> Visitor<'ast> for TestSpans { + fn visit_item(&mut self, item: &'ast Item) { + if item.attrs.iter().any(is_cfg_test) { + // Anything nested inside is covered by containment, so there's no need to descend. + self.spans.push(item.span); + return; + } + if let ItemKind::Mod(_, _, ModKind::Loaded(items, ..)) = &item.kind { + self.collect_test_fns(items); + } + visit::walk_item(self, item); + } +} + +/// Whether `attr` is the `#[cfg(test)]` left behind by expansion. +/// +/// Only a top-level `test` is recognized, matching `clippy_utils::is_cfg_test`. +fn is_cfg_test(attr: &Attribute) -> bool { + matches!( + &attr.kind, + AttrKind::Synthetic(synthetic) + if matches!(&**synthetic, SyntheticAttr::CfgTrace(CfgEntry::NameValue { name: sym::test, .. })) + ) +} + +/// Whether `attr` is the `#[rustc_test_marker]` expansion puts on a test's descriptor constant. +fn is_test_marker(attr: &Attribute) -> bool { + matches!(attr.kind, AttrKind::Normal(_)) && attr.has_name(sym::rustc_test_marker) +} diff --git a/clippy_utils/src/diagnostics.rs b/clippy_utils/src/diagnostics.rs index 39c0e424b658..ac81d81190c3 100644 --- a/clippy_utils/src/diagnostics.rs +++ b/clippy_utils/src/diagnostics.rs @@ -8,13 +8,117 @@ //! Thank you! //! ~The `INTERNAL_METADATA_COLLECTOR` lint +use crate::{is_in_integration_test_file, is_in_test}; +use rustc_data_structures::fx::FxHashSet; use rustc_errors::{Applicability, Diag, DiagCtxtHandle, DiagMessage, Diagnostic, Level, MultiSpan}; #[cfg(debug_assertions)] use rustc_errors::{EmissionGuarantee, SubstitutionPart, Suggestions}; use rustc_hir::HirId; -use rustc_lint::{LateContext, Lint, LintContext}; -use rustc_span::Span; +use rustc_lint::{EarlyContext, LateContext, Lint, LintContext}; +use rustc_span::{BytePos, Span}; use std::env; +use std::sync::OnceLock; + +/// The lints which the `allow-in-tests` configuration suppresses in test code, stored as +/// [`Lint::name`] values (e.g. `"clippy::UNWRAP_USED"`). +static ALLOWED_IN_TESTS: OnceLock> = OnceLock::new(); + +/// Sets the lints which the `allow-in-tests` configuration suppresses in test code. +/// +/// This must be called before any lint pass runs; only the first call has an effect. +pub fn set_lints_allowed_in_tests(lints: impl IntoIterator) { + let _ = ALLOWED_IN_TESTS.set(lints.into_iter().collect()); +} + +fn is_allowed_in_tests(lint: &'static Lint) -> bool { + // This runs for every emitted lint, so check for the common case of an unset configuration + // before hashing the lint's name. + ALLOWED_IN_TESTS + .get() + .is_some_and(|lints| !lints.is_empty() && lints.contains(lint.name)) +} + +/// Whether `allow-in-tests` names any lint at all. +/// +/// Lets the work behind [`set_test_code_spans`] be skipped entirely when the option is unset, +/// which is the overwhelmingly common case. +pub fn any_lint_allowed_in_tests() -> bool { + ALLOWED_IN_TESTS.get().is_some_and(|lints| !lints.is_empty()) +} + +/// The source ranges covered by test code, sorted and non-overlapping. +/// +/// Only populated when `allow-in-tests` is set; see [`set_test_code_spans`]. +static TEST_CODE_SPANS: OnceLock> = OnceLock::new(); + +/// Records the spans of test code for the benefit of early lint passes. +/// +/// A late pass answers "is this test code?" by walking a node's HIR ancestors, but early passes +/// have no such chain: [`EarlyContext`] carries only lint levels. So the spans of `#[cfg(test)]` +/// items and `#[test]` functions are collected up front and matched by containment instead. +/// +/// This must be called before any lint fires; only the first call has an effect. +pub fn set_test_code_spans(spans: impl IntoIterator) { + let mut ranges: Vec<_> = spans + .into_iter() + .filter(|sp| !sp.is_dummy()) + .map(|sp| { + let sp = sp.source_callsite(); + (sp.lo(), sp.hi()) + }) + .collect(); + ranges.sort_unstable(); + // Merge overlapping ranges so a plain binary search can answer containment. + ranges.dedup_by(|&mut (lo, hi), &mut (prev_lo, ref mut prev_hi)| { + debug_assert!(prev_lo <= lo); + if lo <= *prev_hi { + *prev_hi = (*prev_hi).max(hi); + true + } else { + false + } + }); + let _ = TEST_CODE_SPANS.set(ranges); +} + +/// Whether `span` falls inside code recorded by [`set_test_code_spans`]. +fn is_span_in_test_code(span: Span) -> bool { + let Some(ranges) = TEST_CODE_SPANS.get() else { + return false; + }; + // Lints fired from a macro expansion belong to wherever the macro was written. + let pos = span.source_callsite().lo(); + // The ranges are sorted and disjoint, so the only candidate is the last one starting at or + // before `pos`. + match ranges.binary_search_by_key(&pos, |&(lo, _)| lo) { + Ok(_) => true, + Err(0) => false, + Err(i) => pos < ranges[i - 1].1, + } +} + +/// Extends [`LintContext`] with the information clippy's diagnostic functions need to apply the +/// `allow-in-tests` configuration. +pub trait ClippyLintContext: LintContext { + /// Whether the lint being emitted is in test code, i.e. inside a `#[test]` function or a + /// `#[cfg(test)]` item. + fn is_in_test_code(&self, span: &MultiSpan) -> bool; +} + +impl ClippyLintContext for LateContext<'_> { + fn is_in_test_code(&self, _: &MultiSpan) -> bool { + // Resolve against the same node rustc resolves `#[allow]` against, so that + // `allow-in-tests = ["foo"]` matches `#[allow(clippy::foo)]` by construction. + is_in_test(self.tcx, self.last_node_with_lint_attrs) + } +} + +impl ClippyLintContext for EarlyContext<'_> { + fn is_in_test_code(&self, span: &MultiSpan) -> bool { + // Mirrors the three cases `is_in_test` covers for late passes. + is_in_integration_test_file(self.sess()) || span.primary_span().is_some_and(is_span_in_test_code) + } +} fn docs_link(diag: &mut Diag<'_, ()>, lint: &'static Lint) { if env::var("CLIPPY_DISABLE_DOCS_LINKS").is_err() @@ -103,7 +207,12 @@ fn validate_diag(diag: &Diag<'_, impl EmissionGuarantee>) { /// | ^^^^^^^^^^^^^^^^^^^^^^^ /// ``` #[track_caller] -pub fn span_lint(cx: &T, lint: &'static Lint, sp: impl Into, msg: impl Into) { +pub fn span_lint( + cx: &T, + lint: &'static Lint, + sp: impl Into, + msg: impl Into, +) { span_lint_and_then(cx, lint, sp, msg, |_| {}); } @@ -142,7 +251,7 @@ pub fn span_lint(cx: &T, lint: &'static Lint, sp: impl Into( +pub fn span_lint_and_help( cx: &T, lint: &'static Lint, span: impl Into, @@ -197,7 +306,7 @@ pub fn span_lint_and_help( /// | ^^^^^^^^^^^ /// ``` #[track_caller] -pub fn span_lint_and_note( +pub fn span_lint_and_note( cx: &T, lint: &'static Lint, span: impl Into, @@ -235,7 +344,7 @@ pub fn span_lint_and_note( #[track_caller] pub fn span_lint_and_then(cx: &C, lint: &'static Lint, sp: S, msg: M, f: F) where - C: LintContext, + C: ClippyLintContext, S: Into, M: Into, F: FnOnce(&mut Diag<'_, ()>), @@ -251,6 +360,11 @@ where } let sp = sp.into(); + + if is_allowed_in_tests(lint) && cx.is_in_test_code(&sp) { + return; + } + #[expect(clippy::disallowed_methods)] cx.emit_span_lint( lint, @@ -329,6 +443,10 @@ pub fn span_lint_hir_and_then( msg: impl Into, f: impl FnOnce(&mut Diag<'_, ()>), ) { + if is_allowed_in_tests(lint) && is_in_test(cx.tcx, hir_id) { + return; + } + #[expect(clippy::disallowed_methods)] cx.tcx.emit_node_span_lint( lint, @@ -379,7 +497,7 @@ pub fn span_lint_hir_and_then( /// = note: `-D fold-any` implied by `-D warnings` /// ``` #[track_caller] -pub fn span_lint_and_sugg( +pub fn span_lint_and_sugg( cx: &T, lint: &'static Lint, sp: Span, diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index b8e0e8f400c2..6dca0ddd88d3 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -111,6 +111,7 @@ use rustc_middle::ty::{ self as rustc_ty, Binder, BorrowKind, ClosureKind, EarlyBinder, GenericArgKind, GenericArgsRef, IntTy, Ty, TyCtxt, TypeFlags, TypeVisitableExt as _, TypeckResults, UintTy, UpvarCapture, }; +use rustc_session::Session; use rustc_session::config::Input; use rustc_span::hygiene::{ExpnKind, MacroKind}; use rustc_span::source_map::SourceMap; @@ -2446,13 +2447,16 @@ pub fn is_in_cfg_test(tcx: TyCtxt<'_>, id: HirId) -> bool { /// Checks if the node is in a `#[test]` function or has any parent node marked `#[cfg(test)]` 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) || is_in_integration_test_file(tcx) + is_in_test_function(tcx, hir_id) || is_in_cfg_test(tcx, hir_id) || is_in_integration_test_file(tcx.sess) } -/// Check if the node is in an integration test file (i.e. under `tests/`). -fn is_in_integration_test_file(tcx: TyCtxt<'_>) -> bool { - if let Input::File(ref path) = tcx.sess.io.input - && !tcx.sess.opts.unstable_opts.ui_testing +/// Check if the crate being compiled is an integration test file (i.e. under `tests/`). +/// +/// Takes a [`Session`] rather than a [`TyCtxt`] so that early lint passes, which run before the +/// HIR exists, can answer the same question as late ones. +pub fn is_in_integration_test_file(sess: &Session) -> bool { + if let Input::File(ref path) = sess.io.input + && !sess.opts.unstable_opts.ui_testing { path.starts_with("tests") } else { diff --git a/src/driver.rs b/src/driver.rs index 78b9b2cd8de3..d9a69b0e3f5e 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -163,7 +163,7 @@ impl rustc_driver::Callbacks for ClippyCallbacks { list_builder.register(lint_store); let conf = clippy_config::Conf::load(sess); - clippy_lints::register_lint_passes(lint_store, conf); + clippy_lints::register_lint_passes(sess.dcx(), lint_store, conf); #[cfg(feature = "internal")] clippy_lints_internal::register_lints(lint_store); diff --git a/tests/config-metadata.rs b/tests/config-metadata.rs index 0b978ce3d8a6..1a48cd158ece 100644 --- a/tests/config-metadata.rs +++ b/tests/config-metadata.rs @@ -13,7 +13,6 @@ fn metadata() -> impl Iterator { Conf::get_metadata() .into_iter() .filter(|config| config.renamed_to.is_none()) - .filter(|config| !config.lints.is_empty()) } #[test] diff --git a/tests/ui-toml/allow_in_tests/allow_in_tests.rs b/tests/ui-toml/allow_in_tests/allow_in_tests.rs new file mode 100644 index 000000000000..ccb7e094d4ad --- /dev/null +++ b/tests/ui-toml/allow_in_tests/allow_in_tests.rs @@ -0,0 +1,43 @@ +//@compile-flags: --test +//@no-rustfix +#![warn(clippy::dbg_macro, clippy::indexing_slicing, clippy::panic, clippy::unwrap_used)] +#![allow(clippy::no_effect, clippy::unnecessary_operation)] + +fn main() {} + +// Outside of test code the listed lints still fire. +fn not_a_test(x: Option, s: &[u32]) -> u32 { + dbg!(x); + //~^ dbg_macro + s[0]; + //~^ indexing_slicing + x.unwrap() + //~^ unwrap_used +} + +#[test] +fn in_test_fn() { + let x: Option = "1".parse().ok(); + let s: &[u32] = &[1]; + dbg!(x); + s[0]; + x.unwrap(); + // `panic` isn't listed in `allow-in-tests`, so it is still linted. + panic!("boom"); + //~^ panic +} + +#[cfg(test)] +mod tests { + // Not a `#[test]` function, but inside a `#[cfg(test)]` module. + fn helper(x: Option, s: &[u32]) -> u32 { + dbg!(x); + s[0]; + x.unwrap() + } + + #[test] + fn uses_helper() { + helper("1".parse().ok(), &[1]); + } +} diff --git a/tests/ui-toml/allow_in_tests/allow_in_tests.stderr b/tests/ui-toml/allow_in_tests/allow_in_tests.stderr new file mode 100644 index 000000000000..480aeaf77736 --- /dev/null +++ b/tests/ui-toml/allow_in_tests/allow_in_tests.stderr @@ -0,0 +1,46 @@ +error: the `dbg!` macro is intended as a debugging tool + --> tests/ui-toml/allow_in_tests/allow_in_tests.rs:10:5 + | +LL | dbg!(x); + | ^^^^^^^ + | + = note: `-D clippy::dbg-macro` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::dbg_macro)]` +help: remove the invocation before committing it to a version control system + | +LL - dbg!(x); +LL + x; + | + +error: indexing may panic + --> tests/ui-toml/allow_in_tests/allow_in_tests.rs:12:5 + | +LL | s[0]; + | ^^^^ + | + = help: consider using `.get(n)` or `.get_mut(n)` instead + = note: `-D clippy::indexing-slicing` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::indexing_slicing)]` + +error: used `unwrap()` on an `Option` value + --> tests/ui-toml/allow_in_tests/allow_in_tests.rs:14:5 + | +LL | x.unwrap() + | ^^^^^^^^^^ + | + = note: if this value is `None`, it will panic + = help: consider using `expect()` to provide a better panic message + = note: `-D clippy::unwrap-used` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unwrap_used)]` + +error: `panic` should not be present in production code + --> tests/ui-toml/allow_in_tests/allow_in_tests.rs:26:5 + | +LL | panic!("boom"); + | ^^^^^^^^^^^^^^ + | + = note: `-D clippy::panic` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::panic)]` + +error: aborting due to 4 previous errors + diff --git a/tests/ui-toml/allow_in_tests/clippy.toml b/tests/ui-toml/allow_in_tests/clippy.toml new file mode 100644 index 000000000000..ad55942ed3d7 --- /dev/null +++ b/tests/ui-toml/allow_in_tests/clippy.toml @@ -0,0 +1 @@ +allow-in-tests = ["dbg_macro", "clippy::unwrap_used", "indexing-slicing"] diff --git a/tests/ui-toml/allow_in_tests_early/allow_in_tests_early.rs b/tests/ui-toml/allow_in_tests_early/allow_in_tests_early.rs new file mode 100644 index 000000000000..0478fdb6217c --- /dev/null +++ b/tests/ui-toml/allow_in_tests_early/allow_in_tests_early.rs @@ -0,0 +1,42 @@ +//@compile-flags: --test +//@no-rustfix +//! `allow-in-tests` applies to early lint passes too, even though they run before the HIR that +//! late passes use to recognize test code. +#![warn( + clippy::needless_raw_strings, + clippy::single_char_lifetime_names, + clippy::unseparated_literal_suffix +)] +#![allow(clippy::needless_lifetimes)] + +fn main() {} + +// Outside of test code the listed lints still fire. +fn not_a_test<'a>(x: &'a u32) -> &'a u32 { + //~^ single_char_lifetime_names + let _ = r"no escapes here"; + //~^ needless_raw_strings + x +} + +#[test] +fn in_test_fn() { + let _ = r"no escapes here"; + // `unseparated_literal_suffix` isn't listed, so it is still linted. + let _ = 123i32; + //~^ unseparated_literal_suffix +} + +#[cfg(test)] +mod tests { + // Not a `#[test]` function, but inside a `#[cfg(test)]` module. + fn helper<'a>(x: &'a u32) -> &'a u32 { + let _ = r"no escapes here"; + x + } + + #[test] + fn uses_helper() { + helper(&1); + } +} diff --git a/tests/ui-toml/allow_in_tests_early/allow_in_tests_early.stderr b/tests/ui-toml/allow_in_tests_early/allow_in_tests_early.stderr new file mode 100644 index 000000000000..0fc90e44f9a1 --- /dev/null +++ b/tests/ui-toml/allow_in_tests_early/allow_in_tests_early.stderr @@ -0,0 +1,35 @@ +error: single-character lifetime names are likely uninformative + --> tests/ui-toml/allow_in_tests_early/allow_in_tests_early.rs:15:15 + | +LL | fn not_a_test<'a>(x: &'a u32) -> &'a u32 { + | ^^ + | + = help: use a more informative name + = note: `-D clippy::single-char-lifetime-names` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::single_char_lifetime_names)]` + +error: unnecessary raw string literal + --> tests/ui-toml/allow_in_tests_early/allow_in_tests_early.rs:17:13 + | +LL | let _ = r"no escapes here"; + | ^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::needless-raw-strings` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::needless_raw_strings)]` +help: use a plain string literal instead + | +LL - let _ = r"no escapes here"; +LL + let _ = "no escapes here"; + | + +error: integer type suffix should be separated by an underscore + --> tests/ui-toml/allow_in_tests_early/allow_in_tests_early.rs:26:13 + | +LL | let _ = 123i32; + | ^^^^^^ help: add an underscore: `123_i32` + | + = note: `-D clippy::unseparated-literal-suffix` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unseparated_literal_suffix)]` + +error: aborting due to 3 previous errors + diff --git a/tests/ui-toml/allow_in_tests_early/clippy.toml b/tests/ui-toml/allow_in_tests_early/clippy.toml new file mode 100644 index 000000000000..aeddd048970e --- /dev/null +++ b/tests/ui-toml/allow_in_tests_early/clippy.toml @@ -0,0 +1 @@ +allow-in-tests = ["needless_raw_strings", "single_char_lifetime_names"] diff --git a/tests/ui-toml/allow_in_tests_unknown/allow_in_tests_unknown.rs b/tests/ui-toml/allow_in_tests_unknown/allow_in_tests_unknown.rs new file mode 100644 index 000000000000..a09164c02df6 --- /dev/null +++ b/tests/ui-toml/allow_in_tests_unknown/allow_in_tests_unknown.rs @@ -0,0 +1,6 @@ +//@no-rustfix +//@error-in-other-file: unknown lint: `unwrap_use` +//@error-in-other-file: unknown lint: `clippy::stutter` +//@error-in-other-file: unknown lint: `unused_variables` + +fn main() {} diff --git a/tests/ui-toml/allow_in_tests_unknown/allow_in_tests_unknown.stderr b/tests/ui-toml/allow_in_tests_unknown/allow_in_tests_unknown.stderr new file mode 100644 index 000000000000..f8e5abd446f7 --- /dev/null +++ b/tests/ui-toml/allow_in_tests_unknown/allow_in_tests_unknown.stderr @@ -0,0 +1,26 @@ +warning: unknown lint: `unwrap_use` + --> $DIR/tests/ui-toml/allow_in_tests_unknown/clippy.toml:3:5 + | +LL | "unwrap_use", + | ^^^^^^^^^^^^ help: did you mean: `"unwrap_used"` + | + = note: `allow-in-tests` only accepts Clippy lints + +warning: unknown lint: `clippy::stutter` + --> $DIR/tests/ui-toml/allow_in_tests_unknown/clippy.toml:5:5 + | +LL | "clippy::stutter", + | ^^^^^^^^^^^^^^^^^ help: `clippy::stutter` has been renamed: `"module_name_repetitions"` + | + = note: `allow-in-tests` only accepts Clippy lints + +warning: unknown lint: `unused_variables` + --> $DIR/tests/ui-toml/allow_in_tests_unknown/clippy.toml:7:5 + | +LL | "unused_variables", + | ^^^^^^^^^^^^^^^^^^ help: did you mean: `"unused_peekable"` + | + = note: `allow-in-tests` only accepts Clippy lints + +warning: 3 warnings emitted + diff --git a/tests/ui-toml/allow_in_tests_unknown/clippy.toml b/tests/ui-toml/allow_in_tests_unknown/clippy.toml new file mode 100644 index 000000000000..4cf5669a7b24 --- /dev/null +++ b/tests/ui-toml/allow_in_tests_unknown/clippy.toml @@ -0,0 +1,8 @@ +allow-in-tests = [ + # not a lint at all + "unwrap_use", + # a renamed lint + "clippy::stutter", + # a rustc lint + "unused_variables", +] diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index bee9e63ef742..588dadfb2124 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -14,6 +14,7 @@ LL | foobar = 42 allow-exact-repetitions allow-expect-in-consts allow-expect-in-tests + allow-in-tests allow-indexing-slicing-in-tests allow-large-stack-frames-in-tests allow-mixed-uninlined-format-args