Auto merge of #13464 - y21:issue13458, r=flip1995

Don't warn on proc macro generated code in `needless_return` Fixes #13458 Fixes #13457 Fixes #13467 Fixes #13479 Fixes #13481 Fixes #13526 Fixes #13486 The fix is unfortunately a little more convoluted than just simply adding a `is_from_proc_macro`. That check *does* fix the issue, however it also introduces a bunch of false negatives in the tests, specifically when the returned expression is in a different syntax context, e.g. `return format!(..)`. The proc macro check builds up a start and end pattern based on the HIR nodes and compares it to a snippet of the span, however that would currently fail for `return format!(..)` because we would have the patterns `("return", <something inside of the format macro>)`, which doesn't compare equal. So we now return an empty string pattern for when it's in a different syntax context. "Hide whitespace" helps a bit for reviewing the proc macro detection change changelog: none
rust-lang · Oct 10, 2024 · a21a9fe · a21a9fe
1 parent df1b5d3
commit a21a9fe
Show file tree

Hide file tree

Showing 7 changed files with 154 additions and 111 deletions.
diff --git a/src/tools/clippy/clippy_lints/src/returns.rs b/src/tools/clippy/clippy_lints/src/returns.rs
@@ -407,7 +407,7 @@ fn check_final_expr<'tcx>(
                 }
             }
 
-            if ret_span.from_expansion() {
+            if ret_span.from_expansion() || is_from_proc_macro(cx, expr) {
                 return;
             }
 

diff --git a/src/tools/clippy/clippy_utils/src/check_proc_macro.rs b/src/tools/clippy/clippy_utils/src/check_proc_macro.rs
@@ -141,62 +141,89 @@ fn path_search_pat(path: &Path<'_>) -> (Pat, Pat) {
 
 /// Get the search patterns to use for the given expression
 fn expr_search_pat(tcx: TyCtxt<'_>, e: &Expr<'_>) -> (Pat, Pat) {
-    match e.kind {
-        ExprKind::ConstBlock(_) => (Pat::Str("const"), Pat::Str("}")),
-        // Parenthesis are trimmed from the text before the search patterns are matched.
-        // See: `span_matches_pat`
-        ExprKind::Tup([]) => (Pat::Str(")"), Pat::Str("(")),
-        ExprKind::Unary(UnOp::Deref, e) => (Pat::Str("*"), expr_search_pat(tcx, e).1),
-        ExprKind::Unary(UnOp::Not, e) => (Pat::Str("!"), expr_search_pat(tcx, e).1),
-        ExprKind::Unary(UnOp::Neg, e) => (Pat::Str("-"), expr_search_pat(tcx, e).1),
-        ExprKind::Lit(lit) => lit_search_pat(&lit.node),
-        ExprKind::Array(_) | ExprKind::Repeat(..) => (Pat::Str("["), Pat::Str("]")),
-        ExprKind::Call(e, []) | ExprKind::MethodCall(_, e, [], _) => (expr_search_pat(tcx, e).0, Pat::Str("(")),
-        ExprKind::Call(first, [.., last])
-        | ExprKind::MethodCall(_, first, [.., last], _)
-        | ExprKind::Binary(_, first, last)
-        | ExprKind::Tup([first, .., last])
-        | ExprKind::Assign(first, last, _)
-        | ExprKind::AssignOp(_, first, last) => (expr_search_pat(tcx, first).0, expr_search_pat(tcx, last).1),
-        ExprKind::Tup([e]) | ExprKind::DropTemps(e) => expr_search_pat(tcx, e),
-        ExprKind::Cast(e, _) | ExprKind::Type(e, _) => (expr_search_pat(tcx, e).0, Pat::Str("")),
-        ExprKind::Let(let_expr) => (Pat::Str("let"), expr_search_pat(tcx, let_expr.init).1),
-        ExprKind::If(..) => (Pat::Str("if"), Pat::Str("}")),
-        ExprKind::Loop(_, Some(_), _, _) | ExprKind::Block(_, Some(_)) => (Pat::Str("'"), Pat::Str("}")),
-        ExprKind::Loop(_, None, LoopSource::Loop, _) => (Pat::Str("loop"), Pat::Str("}")),
-        ExprKind::Loop(_, None, LoopSource::While, _) => (Pat::Str("while"), Pat::Str("}")),
-        ExprKind::Loop(_, None, LoopSource::ForLoop, _) | ExprKind::Match(_, _, MatchSource::ForLoopDesugar) => {
-            (Pat::Str("for"), Pat::Str("}"))
-        },
-        ExprKind::Match(_, _, MatchSource::Normal) => (Pat::Str("match"), Pat::Str("}")),
-        ExprKind::Match(e, _, MatchSource::TryDesugar(_)) => (expr_search_pat(tcx, e).0, Pat::Str("?")),
-        ExprKind::Match(e, _, MatchSource::AwaitDesugar) | ExprKind::Yield(e, YieldSource::Await { .. }) => {
-            (expr_search_pat(tcx, e).0, Pat::Str("await"))
-        },
-        ExprKind::Closure(&Closure { body, .. }) => (Pat::Str(""), expr_search_pat(tcx, tcx.hir().body(body).value).1),
-        ExprKind::Block(
-            Block {
-                rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided),
-                ..
+    fn expr_search_pat_inner(tcx: TyCtxt<'_>, e: &Expr<'_>, outer_span: Span) -> (Pat, Pat) {
+        // The expression can have subexpressions in different contexts, in which case
+        // building up a search pattern from the macro expansion would lead to false positives;
+        // e.g. `return format!(..)` would be considered to be from a proc macro
+        // if we build up a pattern for the macro expansion and compare it to the invocation `format!()`.
+        // So instead we return an empty pattern such that `span_matches_pat` always returns true.
+        if !e.span.eq_ctxt(outer_span) {
+            return (Pat::Str(""), Pat::Str(""));
+        }
+
+        match e.kind {
+            ExprKind::ConstBlock(_) => (Pat::Str("const"), Pat::Str("}")),
+            // Parenthesis are trimmed from the text before the search patterns are matched.
+            // See: `span_matches_pat`
+            ExprKind::Tup([]) => (Pat::Str(")"), Pat::Str("(")),
+            ExprKind::Unary(UnOp::Deref, e) => (Pat::Str("*"), expr_search_pat_inner(tcx, e, outer_span).1),
+            ExprKind::Unary(UnOp::Not, e) => (Pat::Str("!"), expr_search_pat_inner(tcx, e, outer_span).1),
+            ExprKind::Unary(UnOp::Neg, e) => (Pat::Str("-"), expr_search_pat_inner(tcx, e, outer_span).1),
+            ExprKind::Lit(lit) => lit_search_pat(&lit.node),
+            ExprKind::Array(_) | ExprKind::Repeat(..) => (Pat::Str("["), Pat::Str("]")),
+            ExprKind::Call(e, []) | ExprKind::MethodCall(_, e, [], _) => {
+                (expr_search_pat_inner(tcx, e, outer_span).0, Pat::Str("("))
             },
-            None,
-        ) => (Pat::Str("unsafe"), Pat::Str("}")),
-        ExprKind::Block(_, None) => (Pat::Str("{"), Pat::Str("}")),
-        ExprKind::Field(e, name) => (expr_search_pat(tcx, e).0, Pat::Sym(name.name)),
-        ExprKind::Index(e, _, _) => (expr_search_pat(tcx, e).0, Pat::Str("]")),
-        ExprKind::Path(ref path) => qpath_search_pat(path),
-        ExprKind::AddrOf(_, _, e) => (Pat::Str("&"), expr_search_pat(tcx, e).1),
-        ExprKind::Break(Destination { label: None, .. }, None) => (Pat::Str("break"), Pat::Str("break")),
-        ExprKind::Break(Destination { label: Some(name), .. }, None) => (Pat::Str("break"), Pat::Sym(name.ident.name)),
-        ExprKind::Break(_, Some(e)) => (Pat::Str("break"), expr_search_pat(tcx, e).1),
-        ExprKind::Continue(Destination { label: None, .. }) => (Pat::Str("continue"), Pat::Str("continue")),
-        ExprKind::Continue(Destination { label: Some(name), .. }) => (Pat::Str("continue"), Pat::Sym(name.ident.name)),
-        ExprKind::Ret(None) => (Pat::Str("return"), Pat::Str("return")),
-        ExprKind::Ret(Some(e)) => (Pat::Str("return"), expr_search_pat(tcx, e).1),
-        ExprKind::Struct(path, _, _) => (qpath_search_pat(path).0, Pat::Str("}")),
-        ExprKind::Yield(e, YieldSource::Yield) => (Pat::Str("yield"), expr_search_pat(tcx, e).1),
-        _ => (Pat::Str(""), Pat::Str("")),
+            ExprKind::Call(first, [.., last])
+            | ExprKind::MethodCall(_, first, [.., last], _)
+            | ExprKind::Binary(_, first, last)
+            | ExprKind::Tup([first, .., last])
+            | ExprKind::Assign(first, last, _)
+            | ExprKind::AssignOp(_, first, last) => (
+                expr_search_pat_inner(tcx, first, outer_span).0,
+                expr_search_pat_inner(tcx, last, outer_span).1,
+            ),
+            ExprKind::Tup([e]) | ExprKind::DropTemps(e) => expr_search_pat_inner(tcx, e, outer_span),
+            ExprKind::Cast(e, _) | ExprKind::Type(e, _) => (expr_search_pat_inner(tcx, e, outer_span).0, Pat::Str("")),
+            ExprKind::Let(let_expr) => (Pat::Str("let"), expr_search_pat_inner(tcx, let_expr.init, outer_span).1),
+            ExprKind::If(..) => (Pat::Str("if"), Pat::Str("}")),
+            ExprKind::Loop(_, Some(_), _, _) | ExprKind::Block(_, Some(_)) => (Pat::Str("'"), Pat::Str("}")),
+            ExprKind::Loop(_, None, LoopSource::Loop, _) => (Pat::Str("loop"), Pat::Str("}")),
+            ExprKind::Loop(_, None, LoopSource::While, _) => (Pat::Str("while"), Pat::Str("}")),
+            ExprKind::Loop(_, None, LoopSource::ForLoop, _) | ExprKind::Match(_, _, MatchSource::ForLoopDesugar) => {
+                (Pat::Str("for"), Pat::Str("}"))
+            },
+            ExprKind::Match(_, _, MatchSource::Normal) => (Pat::Str("match"), Pat::Str("}")),
+            ExprKind::Match(e, _, MatchSource::TryDesugar(_)) => {
+                (expr_search_pat_inner(tcx, e, outer_span).0, Pat::Str("?"))
+            },
+            ExprKind::Match(e, _, MatchSource::AwaitDesugar) | ExprKind::Yield(e, YieldSource::Await { .. }) => {
+                (expr_search_pat_inner(tcx, e, outer_span).0, Pat::Str("await"))
+            },
+            ExprKind::Closure(&Closure { body, .. }) => (
+                Pat::Str(""),
+                expr_search_pat_inner(tcx, tcx.hir().body(body).value, outer_span).1,
+            ),
+            ExprKind::Block(
+                Block {
+                    rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided),
+                    ..
+                },
+                None,
+            ) => (Pat::Str("unsafe"), Pat::Str("}")),
+            ExprKind::Block(_, None) => (Pat::Str("{"), Pat::Str("}")),
+            ExprKind::Field(e, name) => (expr_search_pat_inner(tcx, e, outer_span).0, Pat::Sym(name.name)),
+            ExprKind::Index(e, _, _) => (expr_search_pat_inner(tcx, e, outer_span).0, Pat::Str("]")),
+            ExprKind::Path(ref path) => qpath_search_pat(path),
+            ExprKind::AddrOf(_, _, e) => (Pat::Str("&"), expr_search_pat_inner(tcx, e, outer_span).1),
+            ExprKind::Break(Destination { label: None, .. }, None) => (Pat::Str("break"), Pat::Str("break")),
+            ExprKind::Break(Destination { label: Some(name), .. }, None) => {
+                (Pat::Str("break"), Pat::Sym(name.ident.name))
+            },
+            ExprKind::Break(_, Some(e)) => (Pat::Str("break"), expr_search_pat_inner(tcx, e, outer_span).1),
+            ExprKind::Continue(Destination { label: None, .. }) => (Pat::Str("continue"), Pat::Str("continue")),
+            ExprKind::Continue(Destination { label: Some(name), .. }) => {
+                (Pat::Str("continue"), Pat::Sym(name.ident.name))
+            },
+            ExprKind::Ret(None) => (Pat::Str("return"), Pat::Str("return")),
+            ExprKind::Ret(Some(e)) => (Pat::Str("return"), expr_search_pat_inner(tcx, e, outer_span).1),
+            ExprKind::Struct(path, _, _) => (qpath_search_pat(path).0, Pat::Str("}")),
+            ExprKind::Yield(e, YieldSource::Yield) => (Pat::Str("yield"), expr_search_pat_inner(tcx, e, outer_span).1),
+            _ => (Pat::Str(""), Pat::Str("")),
+        }
     }
+
+    expr_search_pat_inner(tcx, e, e.span)
 }
 
 fn fn_header_search_pat(header: FnHeader) -> Pat {

diff --git a/src/tools/clippy/tests/ui/dbg_macro/dbg_macro.fixed b/src/tools/clippy/tests/ui/dbg_macro/dbg_macro.fixed
@@ -1,5 +1,5 @@
 #![warn(clippy::dbg_macro)]
-#![allow(clippy::unnecessary_operation, clippy::no_effect)]
+#![allow(clippy::unnecessary_operation, clippy::no_effect, clippy::unit_arg)]
 
 fn foo(n: u32) -> u32 {
     if let Some(n) = n.checked_sub(4) { n } else { n }

diff --git a/src/tools/clippy/tests/ui/dbg_macro/dbg_macro.rs b/src/tools/clippy/tests/ui/dbg_macro/dbg_macro.rs
@@ -1,5 +1,5 @@
 #![warn(clippy::dbg_macro)]
-#![allow(clippy::unnecessary_operation, clippy::no_effect)]
+#![allow(clippy::unnecessary_operation, clippy::no_effect, clippy::unit_arg)]
 
 fn foo(n: u32) -> u32 {
     if let Some(n) = dbg!(n.checked_sub(4)) { n } else { n }

diff --git a/src/tools/clippy/tests/ui/needless_return.fixed b/src/tools/clippy/tests/ui/needless_return.fixed
@@ -1,3 +1,4 @@
+//@aux-build:proc_macros.rs
 #![feature(yeet_expr)]
 #![allow(unused)]
 #![allow(
@@ -9,6 +10,9 @@
 )]
 #![warn(clippy::needless_return)]
 
+extern crate proc_macros;
+use proc_macros::with_span;
+
 use std::cell::RefCell;
 
 macro_rules! the_answer {
@@ -359,6 +363,10 @@ fn issue12907() -> String {
     "".split("").next().unwrap().to_string()
 }
 
+fn issue13458() {
+    with_span!(span return);
+}
+
 fn main() {}
 
 fn a(x: Option<u8>) -> Option<u8> {

diff --git a/src/tools/clippy/tests/ui/needless_return.rs b/src/tools/clippy/tests/ui/needless_return.rs
@@ -1,3 +1,4 @@
+//@aux-build:proc_macros.rs
 #![feature(yeet_expr)]
 #![allow(unused)]
 #![allow(
@@ -9,6 +10,9 @@
 )]
 #![warn(clippy::needless_return)]
 
+extern crate proc_macros;
+use proc_macros::with_span;
+
 use std::cell::RefCell;
 
 macro_rules! the_answer {
@@ -369,6 +373,10 @@ fn issue12907() -> String {
     return "".split("").next().unwrap().to_string();
 }
 
+fn issue13458() {
+    with_span!(span return);
+}
+
 fn main() {}
 
 fn a(x: Option<u8>) -> Option<u8> {