Auto merge of #11523 - Alexendoo:used-underscore-bindings-lint-levels, r=xFrednet

bors · bors · commit c92de5869282 · 2023-09-18T09:04:44.000Z
used_underscore_bindings: respect lint levels on the binding definition Fixes #11520 Fixes #947 Also ignores usages of `PhantomData` changelog: none
diff --git 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
@@ -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<Item = HirId>) -> 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
@@ -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<T> {
+    _marker: std::marker::PhantomData<T>,
+}
+
+impl<T> std::fmt::Debug for PhantomField<T> {
+    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
@@ -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
 
