From 32d3387c80b59ad85756aa66237aa6d2a0eca884 Mon Sep 17 00:00:00 2001 From: Alex Macleod Date: Sun, 17 Sep 2023 14:33:06 +0000 Subject: [PATCH] used_underscore_bindings: respect lint levels on the binding definition --- clippy_lints/src/misc.rs | 104 +++++++++++------------- clippy_utils/src/lib.rs | 27 ++++++ tests/ui/used_underscore_binding.rs | 28 ++++++- tests/ui/used_underscore_binding.stderr | 47 +++++++++-- 4 files changed, 140 insertions(+), 66 deletions(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 303f012569087..f8e76a5833e50 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -1,24 +1,23 @@ -use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg, span_lint_hir_and_then}; +use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg, span_lint_and_then, span_lint_hir_and_then}; use clippy_utils::source::{snippet, snippet_opt, snippet_with_context}; +use clippy_utils::sugg::Sugg; +use clippy_utils::{ + any_parent_is_automatically_derived, fulfill_or_allowed, get_parent_expr, in_constant, is_integer_literal, + is_lint_allowed, is_no_std_crate, iter_input_pats, last_path_segment, SpanlessEq, +}; use if_chain::if_chain; use rustc_errors::Applicability; +use rustc_hir::def::Res; use rustc_hir::intravisit::FnKind; use rustc_hir::{ - self as hir, def, BinOpKind, BindingAnnotation, Body, ByRef, Expr, ExprKind, FnDecl, Mutability, PatKind, Stmt, - StmtKind, TyKind, + BinOpKind, BindingAnnotation, Body, ByRef, Expr, ExprKind, FnDecl, Mutability, PatKind, QPath, Stmt, StmtKind, Ty, + TyKind, }; -use rustc_lint::{LateContext, LateLintPass}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::def_id::LocalDefId; -use rustc_span::hygiene::DesugaringKind; -use rustc_span::source_map::{ExpnKind, Span}; - -use clippy_utils::sugg::Sugg; -use clippy_utils::{ - get_parent_expr, in_constant, is_integer_literal, is_lint_allowed, is_no_std_crate, iter_input_pats, - last_path_segment, SpanlessEq, -}; +use rustc_span::source_map::Span; use crate::ref_patterns::REF_PATTERNS; @@ -257,46 +256,56 @@ impl<'tcx> LateLintPass<'tcx> for LintPass { self.check_cast(cx, expr.span, e, ty); return; } - if in_attributes_expansion(expr) || expr.span.is_desugaring(DesugaringKind::Await) { - // Don't lint things expanded by #[derive(...)], etc or `await` desugaring + if in_external_macro(cx.sess(), expr.span) + || expr.span.desugaring_kind().is_some() + || any_parent_is_automatically_derived(cx.tcx, expr.hir_id) + { return; } - let sym; - let binding = match expr.kind { - ExprKind::Path(ref qpath) if !matches!(qpath, hir::QPath::LangItem(..)) => { - let binding = last_path_segment(qpath).ident.as_str(); - if binding.starts_with('_') && - !binding.starts_with("__") && - binding != "_result" && // FIXME: #944 - is_used(cx, expr) && - // don't lint if the declaration is in a macro - non_macro_local(cx, cx.qpath_res(qpath, expr.hir_id)) + let (definition_hir_id, ident) = match expr.kind { + ExprKind::Path(ref qpath) => { + if let QPath::Resolved(None, path) = qpath + && let Res::Local(id) = path.res + && is_used(cx, expr) { - Some(binding) + (id, last_path_segment(qpath).ident) } else { - None + return; } }, - ExprKind::Field(_, ident) => { - sym = ident.name; - let name = sym.as_str(); - if name.starts_with('_') && !name.starts_with("__") { - Some(name) + ExprKind::Field(recv, ident) => { + if let Some(adt_def) = cx.typeck_results().expr_ty_adjusted(recv).ty_adt_def() + && let Some(field) = adt_def.all_fields().find(|field| field.name == ident.name) + && let Some(local_did) = field.did.as_local() + && let Some(hir_id) = cx.tcx.opt_local_def_id_to_hir_id(local_did) + && !cx.tcx.type_of(field.did).skip_binder().is_phantom_data() + { + (hir_id, ident) } else { - None + return; } }, - _ => None, + _ => return, }; - if let Some(binding) = binding { - span_lint( + + let name = ident.name.as_str(); + if name.starts_with('_') + && !name.starts_with("__") + && let definition_span = cx.tcx.hir().span(definition_hir_id) + && !definition_span.from_expansion() + && !fulfill_or_allowed(cx, USED_UNDERSCORE_BINDING, [expr.hir_id, definition_hir_id]) + { + span_lint_and_then( cx, USED_UNDERSCORE_BINDING, expr.span, &format!( - "used binding `{binding}` which is prefixed with an underscore. A leading \ + "used binding `{name}` which is prefixed with an underscore. A leading \ underscore signals that a binding will not be used" ), + |diag| { + diag.span_note(definition_span, format!("`{name}` is defined here")); + } ); } } @@ -312,29 +321,8 @@ fn is_used(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { }) } -/// Tests whether an expression is in a macro expansion (e.g., something -/// generated by `#[derive(...)]` or the like). -fn in_attributes_expansion(expr: &Expr<'_>) -> bool { - use rustc_span::hygiene::MacroKind; - if expr.span.from_expansion() { - let data = expr.span.ctxt().outer_expn_data(); - matches!(data.kind, ExpnKind::Macro(MacroKind::Attr | MacroKind::Derive, _)) - } else { - false - } -} - -/// Tests whether `res` is a variable defined outside a macro. -fn non_macro_local(cx: &LateContext<'_>, res: def::Res) -> bool { - if let def::Res::Local(id) = res { - !cx.tcx.hir().span(id).from_expansion() - } else { - false - } -} - impl LintPass { - fn check_cast(&self, cx: &LateContext<'_>, span: Span, e: &Expr<'_>, ty: &hir::Ty<'_>) { + fn check_cast(&self, cx: &LateContext<'_>, span: Span, e: &Expr<'_>, ty: &Ty<'_>) { if_chain! { if let TyKind::Ptr(ref mut_ty) = ty.kind; if is_integer_literal(e, 0); diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 4ef3ec19647e9..eb5baa98f62f3 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1785,6 +1785,33 @@ pub fn is_try<'tcx>(cx: &LateContext<'_>, expr: &'tcx Expr<'tcx>) -> Option<&'tc None } +/// Returns `true` if the lint is `#[allow]`ed or `#[expect]`ed at any of the `ids`, fulfilling all +/// of the expectations in `ids` +/// +/// This should only be used when the lint would otherwise be emitted, for a way to check if a lint +/// is allowed early to skip work see [`is_lint_allowed`] +/// +/// To emit at a lint at a different context than the one current see +/// [`span_lint_hir`](diagnostics::span_lint_hir) or +/// [`span_lint_hir_and_then`](diagnostics::span_lint_hir_and_then) +pub fn fulfill_or_allowed(cx: &LateContext<'_>, lint: &'static Lint, ids: impl IntoIterator) -> bool { + let mut suppress_lint = false; + + for id in ids { + let (level, _) = cx.tcx.lint_level_at_node(lint, id); + if let Some(expectation) = level.get_expectation_id() { + cx.fulfill_expectation(expectation); + } + + match level { + Level::Allow | Level::Expect(_) => suppress_lint = true, + Level::Warn | Level::ForceWarn(_) | Level::Deny | Level::Forbid => {}, + } + } + + suppress_lint +} + /// Returns `true` if the lint is allowed in the current context. This is useful for /// skipping long running code when it's unnecessary /// diff --git a/tests/ui/used_underscore_binding.rs b/tests/ui/used_underscore_binding.rs index c672eff1c2710..a8f404b1400c9 100644 --- a/tests/ui/used_underscore_binding.rs +++ b/tests/ui/used_underscore_binding.rs @@ -1,6 +1,5 @@ //@aux-build:proc_macro_derive.rs -#![feature(rustc_private)] -#![warn(clippy::all)] +#![feature(rustc_private, lint_reasons)] #![warn(clippy::used_underscore_binding)] #![allow(clippy::disallowed_names, clippy::eq_op, clippy::uninlined_format_args)] @@ -107,6 +106,31 @@ async fn await_desugaring() { .await } +struct PhantomField { + _marker: std::marker::PhantomData, +} + +impl std::fmt::Debug for PhantomField { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + f.debug_struct("PhantomField").field("_marker", &self._marker).finish() + } +} + +struct AllowedField { + #[allow(clippy::used_underscore_binding)] + _allowed: usize, +} + +struct ExpectedField { + #[expect(clippy::used_underscore_binding)] + _expected: usize, +} + +fn lint_levels(allowed: AllowedField, expected: ExpectedField) { + let _ = allowed._allowed; + let _ = expected._expected; +} + fn main() { let foo = 0u32; // tests of unused_underscore lint diff --git a/tests/ui/used_underscore_binding.stderr b/tests/ui/used_underscore_binding.stderr index 289519b172eeb..78d8279810c1f 100644 --- a/tests/ui/used_underscore_binding.stderr +++ b/tests/ui/used_underscore_binding.stderr @@ -1,41 +1,76 @@ error: used binding `_foo` which is prefixed with an underscore. A leading underscore signals that a binding will not be used - --> $DIR/used_underscore_binding.rs:24:5 + --> $DIR/used_underscore_binding.rs:23:5 | LL | _foo + 1 | ^^^^ | +note: `_foo` is defined here + --> $DIR/used_underscore_binding.rs:22:22 + | +LL | fn prefix_underscore(_foo: u32) -> u32 { + | ^^^^ = note: `-D clippy::used-underscore-binding` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::used_underscore_binding)]` error: used binding `_foo` which is prefixed with an underscore. A leading underscore signals that a binding will not be used - --> $DIR/used_underscore_binding.rs:29:20 + --> $DIR/used_underscore_binding.rs:28:20 | LL | println!("{}", _foo); | ^^^^ + | +note: `_foo` is defined here + --> $DIR/used_underscore_binding.rs:27:24 + | +LL | fn in_macro_or_desugar(_foo: u32) { + | ^^^^ error: used binding `_foo` which is prefixed with an underscore. A leading underscore signals that a binding will not be used - --> $DIR/used_underscore_binding.rs:30:16 + --> $DIR/used_underscore_binding.rs:29:16 | LL | assert_eq!(_foo, _foo); | ^^^^ + | +note: `_foo` is defined here + --> $DIR/used_underscore_binding.rs:27:24 + | +LL | fn in_macro_or_desugar(_foo: u32) { + | ^^^^ error: used binding `_foo` which is prefixed with an underscore. A leading underscore signals that a binding will not be used - --> $DIR/used_underscore_binding.rs:30:22 + --> $DIR/used_underscore_binding.rs:29:22 | LL | assert_eq!(_foo, _foo); | ^^^^ + | +note: `_foo` is defined here + --> $DIR/used_underscore_binding.rs:27:24 + | +LL | fn in_macro_or_desugar(_foo: u32) { + | ^^^^ error: used binding `_underscore_field` which is prefixed with an underscore. A leading underscore signals that a binding will not be used - --> $DIR/used_underscore_binding.rs:43:5 + --> $DIR/used_underscore_binding.rs:42:5 | LL | s._underscore_field += 1; | ^^^^^^^^^^^^^^^^^^^ + | +note: `_underscore_field` is defined here + --> $DIR/used_underscore_binding.rs:36:5 + | +LL | _underscore_field: u32, + | ^^^^^^^^^^^^^^^^^^^^^^ error: used binding `_i` which is prefixed with an underscore. A leading underscore signals that a binding will not be used - --> $DIR/used_underscore_binding.rs:104:16 + --> $DIR/used_underscore_binding.rs:103:16 | LL | uses_i(_i); | ^^ + | +note: `_i` is defined here + --> $DIR/used_underscore_binding.rs:102:13 + | +LL | let _i = 5; + | ^^ error: aborting due to 6 previous errors