-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add generic allow-in-tests configuration
#17520
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,10 @@ 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`, listing the lints it enables in | ||
| // tests. Setting such a field to `true` warns and points at the replacement. | ||
| // 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)?)?, | ||
|
|
@@ -196,6 +200,26 @@ macro_rules! define_Conf { | |
| } | ||
| } | ||
|
|
||
| // Nudge the per-lint `allow-*-in-tests` options towards `allow-in-tests`. Only | ||
| // enabling one is worth warning about; disabling it is the default and has no | ||
| // equivalent in the replacement. | ||
| $($( | ||
| if $name == Some(true) | ||
| && let Some((key, _)) = table.get_key_value($name_str) | ||
| { | ||
| dcx.inner | ||
| .struct_span_warn( | ||
| dcx.make_sp(key.span()), | ||
| concat!("`", $name_str, "` is deprecated"), | ||
| ) | ||
| .with_help(format!( | ||
| "use `allow-in-tests = [{}]` instead, which works for any lint", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but it doesn't work for "any lint", only for late pass lints |
||
| [$(concat!("\"", stringify!($replacement_lints), "\"")),*].join(", "), | ||
| )) | ||
| .emit(); | ||
| } | ||
| )?)* | ||
|
|
||
| Self {$($( | ||
| $name: $name.unwrap_or_else( | ||
| || <$ty as FromDefault<_>>::from_default(first_expr!($($default,)? ())) | ||
|
|
@@ -236,6 +260,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), which works for any | ||
| /// lint. This option still works, but new configurations should use `allow-in-tests`. | ||
| #[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 +273,34 @@ 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), which works for any | ||
| /// lint. This option still works, but new configurations should use `allow-in-tests`. | ||
| #[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. | ||
| /// | ||
| /// This supersedes the per-lint `allow-<lint>-in-tests` options, which are deprecated but | ||
| /// still honored: a lint is suppressed in test code if it is listed here or its own | ||
| /// `allow-<lint>-in-tests` option is set. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if the |
||
| /// | ||
| /// #### Example | ||
| /// | ||
| /// ```toml | ||
| /// allow-in-tests = ["dbg_macro", "unwrap_used"] | ||
| /// ``` | ||
| /// | ||
| /// #### Noteworthy | ||
| /// | ||
| /// - This applies to late lint passes only, as test code becomes recognizable once the HIR | ||
| /// has been built. | ||
| allow_in_tests("allow-in-tests"): Vec<Spanned<String>>, | ||
| /// Whether `indexing_slicing` should be allowed in test functions or `#[cfg(test)]` | ||
| /// | ||
| /// Deprecated in favor of [`allow-in-tests`](#allow-in-tests), which works for any | ||
| /// lint. This option still works, but new configurations should use `allow-in-tests`. | ||
| #[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 +313,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), which works for any | ||
| /// lint. This option still works, but new configurations should use `allow-in-tests`. | ||
| #[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), which works for any | ||
| /// lint. This option still works, but new configurations should use `allow-in-tests`. | ||
| #[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 +348,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), which works for any | ||
| /// lint. This option still works, but new configurations should use `allow-in-tests`. | ||
| #[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 +364,10 @@ define_Conf! { | |
| #[lints(expect_used, unwrap_used)] | ||
| allow_unwrap_types("allow-unwrap-types"): Vec<String>, | ||
| /// Whether `useless_vec` should ignore test functions or `#[cfg(test)]` | ||
| /// | ||
| /// Deprecated in favor of [`allow-in-tests`](#allow-in-tests), which works for any | ||
| /// lint. This option still works, but new configurations should use `allow-in-tests`. | ||
| #[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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -416,8 +416,12 @@ mod zombie_processes; | |
| use clippy_config::{Conf, sanitize_explanation}; | ||
| use clippy_utils::macros::FormatArgsStorage; | ||
| use rustc_data_structures::fx::FxHashSet; | ||
| use rustc_errors::Applicability; | ||
| use rustc_lint::is_lint_pass_required; | ||
| use rustc_middle::ty::TyCtxt; | ||
| use rustc_session::Session; | ||
| use rustc_span::Symbol; | ||
| use rustc_span::edit_distance::find_best_match_for_name; | ||
| use utils::attr_collector::AttrStorage; | ||
|
|
||
| pub fn explain(name: &str) -> i32 { | ||
|
|
@@ -441,10 +445,64 @@ pub fn explain(name: &str) -> i32 { | |
| } | ||
| } | ||
|
|
||
| /// 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about non-late-pass lints - can they be reported too? |
||
| fn register_lints_allowed_in_tests(sess: &Session, conf: &'static Conf) { | ||
| let mut allowed = Vec::with_capacity(conf.allow_in_tests.len()); | ||
| for name in &conf.allow_in_tests { | ||
| // Accept `unwrap_used`, `clippy::unwrap_used` and `unwrap-used` alike. | ||
| 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) { | ||
| allowed.push(info.lint.name); | ||
| continue; | ||
| } | ||
|
|
||
| let mut diag = sess | ||
| .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(); | ||
| } | ||
| 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<Symbol> { | ||
| 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(sess: &Session, store: &mut rustc_lint::LintStore, conf: &'static Conf) { | ||
| register_lints_allowed_in_tests(sess, conf); | ||
|
|
||
| for (old_name, new_name) in deprecated_lints::RENAMED { | ||
| store.register_renamed(old_name, new_name); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Community review:
I am not sure that this is a good idea.
I think it is not uncommon to want to allow
.expect, but notdbg!in tests.Therefore this seems like a lot of churn for some users and not an 100% pure win at that.
View changes since the review