Skip to content

Commit

Permalink
Auto merge of rust-lang#13464 - y21:issue13458, r=flip1995
Browse files Browse the repository at this point in the history
Don't warn on proc macro generated code in `needless_return`

Fixes rust-lang#13458
Fixes rust-lang#13457
Fixes rust-lang#13467
Fixes rust-lang#13479
Fixes rust-lang#13481
Fixes rust-lang#13526
Fixes rust-lang#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
  • Loading branch information
bors authored and flip1995 committed Oct 10, 2024
1 parent 65d941d commit 806a9df
Show file tree
Hide file tree
Showing 7 changed files with 154 additions and 111 deletions.
2 changes: 1 addition & 1 deletion clippy_lints/src/returns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
135 changes: 81 additions & 54 deletions clippy_utils/src/check_proc_macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/dbg_macro/dbg_macro.fixed
Original file line number Diff line number Diff line change
@@ -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 }
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/dbg_macro/dbg_macro.rs
Original file line number Diff line number Diff line change
@@ -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 }
Expand Down
8 changes: 8 additions & 0 deletions tests/ui/needless_return.fixed
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
//@aux-build:proc_macros.rs
#![feature(yeet_expr)]
#![allow(unused)]
#![allow(
Expand All @@ -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 {
Expand Down Expand Up @@ -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> {
Expand Down
8 changes: 8 additions & 0 deletions tests/ui/needless_return.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
//@aux-build:proc_macros.rs
#![feature(yeet_expr)]
#![allow(unused)]
#![allow(
Expand All @@ -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 {
Expand Down Expand Up @@ -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> {
Expand Down
Loading

0 comments on commit 806a9df

Please sign in to comment.