diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b52a6fcd05e..b4d917f79438 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3042,6 +3042,7 @@ Released 2018-09-13 [`absurd_extreme_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#absurd_extreme_comparisons +[`allow_lint_without_reason`]: https://rust-lang.github.io/rust-clippy/master/index.html#allow_lint_without_reason [`almost_swapped`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_swapped [`approx_constant`]: https://rust-lang.github.io/rust-clippy/master/index.html#approx_constant [`as_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_conversions diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index a58d12ddd6b4..adfe65cb29b3 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -255,6 +255,36 @@ declare_clippy_lint! { "usage of `cfg(operating_system)` instead of `cfg(target_os = \"operating_system\")`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for attributes that allow lints without a reason. + /// + /// (This requires the `lint_reasons` feature) + /// + /// ### Why is this bad? + /// Allowing a lint should always have a reason. This reason should be documented to + /// ensure that others understand the reasoning + /// + /// ### Example + /// Bad: + /// ```rust + /// #![feature(lint_reasons)] + /// + /// #![allow(clippy::some_lint)] + /// ``` + /// + /// Good: + /// ```rust + /// #![feature(lint_reasons)] + /// + /// #![allow(clippy::some_lint, reason = "False positive rust-lang/rust-clippy#1002020")] + /// ``` + #[clippy::version = "1.61.0"] + pub ALLOW_LINT_WITHOUT_REASON, + restriction, + "ensures that all `allow` and `expect` attributes have a reason" +} + declare_lint_pass!(Attributes => [ INLINE_ALWAYS, DEPRECATED_SEMVER, @@ -269,6 +299,9 @@ impl<'tcx> LateLintPass<'tcx> for Attributes { if is_lint_level(ident.name) { check_clippy_lint_names(cx, ident.name, items); } + if matches!(ident.name, sym::allow | sym::expect) { + check_lint_reason(cx, ident.name, items, attr); + } if items.is_empty() || !attr.has_name(sym::deprecated) { return; } @@ -404,6 +437,30 @@ fn check_clippy_lint_names(cx: &LateContext<'_>, name: Symbol, items: &[NestedMe } } +fn check_lint_reason(cx: &LateContext<'_>, name: Symbol, items: &[NestedMetaItem], attr: &'_ Attribute) { + // Check for the feature + if !cx.tcx.sess.features_untracked().lint_reasons { + return; + } + + // Check if the reason is present + if let Some(item) = items.last().map(NestedMetaItem::meta_item).flatten() + && let MetaItemKind::NameValue(_) = &item.kind + && item.path == sym::reason + { + return; + } + + span_lint_and_help( + cx, + ALLOW_LINT_WITHOUT_REASON, + attr.span, + &format!("`{}` attribute without reason", name.as_str()), + None, + "try adding a reason at the end with `, reason = \"..\"`", + ); +} + fn is_relevant_item(cx: &LateContext<'_>, item: &Item<'_>) -> bool { if let ItemKind::Fn(_, _, eid) = item.kind { is_relevant_expr(cx, cx.tcx.typeck_body(eid), &cx.tcx.hir().body(eid).value) @@ -659,5 +716,5 @@ fn check_mismatched_target_os(cx: &EarlyContext<'_>, attr: &Attribute) { } fn is_lint_level(symbol: Symbol) -> bool { - matches!(symbol, sym::allow | sym::warn | sym::deny | sym::forbid) + matches!(symbol, sym::allow | sym::expect | sym::warn | sym::deny | sym::forbid) } diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 75ef1b0a9d51..f25b5e5109ad 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -42,6 +42,7 @@ store.register_lints(&[ assign_ops::ASSIGN_OP_PATTERN, assign_ops::MISREFACTORED_ASSIGN_OP, async_yields_async::ASYNC_YIELDS_ASYNC, + attrs::ALLOW_LINT_WITHOUT_REASON, attrs::BLANKET_CLIPPY_RESTRICTION_LINTS, attrs::DEPRECATED_CFG_ATTR, attrs::DEPRECATED_SEMVER, diff --git a/clippy_lints/src/lib.register_restriction.rs b/clippy_lints/src/lib.register_restriction.rs index f89f35b885c1..d7686187e4eb 100644 --- a/clippy_lints/src/lib.register_restriction.rs +++ b/clippy_lints/src/lib.register_restriction.rs @@ -8,6 +8,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve LintId::of(as_conversions::AS_CONVERSIONS), LintId::of(asm_syntax::INLINE_ASM_X86_ATT_SYNTAX), LintId::of(asm_syntax::INLINE_ASM_X86_INTEL_SYNTAX), + LintId::of(attrs::ALLOW_LINT_WITHOUT_REASON), LintId::of(casts::FN_TO_NUMERIC_CAST_ANY), LintId::of(create_dir::CREATE_DIR), LintId::of(dbg_macro::DBG_MACRO), diff --git a/tests/ui/allow_lint_without_reason.rs b/tests/ui/allow_lint_without_reason.rs new file mode 100644 index 000000000000..7ca806c88938 --- /dev/null +++ b/tests/ui/allow_lint_without_reason.rs @@ -0,0 +1,14 @@ +#![feature(lint_reasons)] +#![deny(clippy::allow_lint_without_reason)] + +// These should trigger the lint +#[allow(dead_code)] +#[allow(dead_code, deprecated)] +// These should be fine +#[allow(dead_code, reason = "This should be allowed")] +#[warn(dyn_drop, reason = "Warnings can also have reasons")] +#[warn(deref_nullptr)] +#[deny(deref_nullptr)] +#[forbid(deref_nullptr)] + +fn main() {} diff --git a/tests/ui/allow_lint_without_reason.stderr b/tests/ui/allow_lint_without_reason.stderr new file mode 100644 index 000000000000..718db3d23de8 --- /dev/null +++ b/tests/ui/allow_lint_without_reason.stderr @@ -0,0 +1,23 @@ +error: `allow` attribute without reason + --> $DIR/allow_lint_without_reason.rs:5:1 + | +LL | #[allow(dead_code)] + | ^^^^^^^^^^^^^^^^^^^ + | +note: the lint level is defined here + --> $DIR/allow_lint_without_reason.rs:2:9 + | +LL | #![deny(clippy::allow_lint_without_reason)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: try adding a reason at the end with `, reason = ".."` + +error: `allow` attribute without reason + --> $DIR/allow_lint_without_reason.rs:6:1 + | +LL | #[allow(dead_code, deprecated)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: try adding a reason at the end with `, reason = ".."` + +error: aborting due to 2 previous errors +