Skip to content

Don't warn on proc macro generated code in needless_return #13464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 10, 2024
Merged

Don't warn on proc macro generated code in needless_return #13464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8725f7
add proc macro check to needless_return
y21 Sep 26, 2024
55834a3
deal with differing syntax contexts for subexpressions in check_proc_…
y21 Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clippy_lints/src/returns.rs
View file
Open in desktop
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) {
Copy link
Member

@jieyouxu jieyouxu Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: if this is the Span::from_expansion that is the rustc one, then AFAIK in general it is the most robust check for spans from macro expansions that is possible, and I would not have expected is_from_proc_macro detection to not be already covered by from_expansion if the proc macro implements span hygiene correctly.

I suspect that some crates have proc-macros which generate spans that have call-site hygiene instead of mixed-site hygiene, causing from_expansion to return false.

But I suppose it's possible that clippy wants different trade-offs here and try to provide warnings even if the proc-macros do not emit spans with proper hygiene?

See: similar issue I ran into in implementing unit_bindings lint in rustc rust-lang/rust#112380 (comment).

Also see a Rocket PR where I used Span::mixed_site().located_at(span) so the proc-macro produces a Span which will correctly return true for Span::from_expansion because it has distinguishing SyntaxContext.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the issue that is_from_proc_macro solves is crates using quote_spanned! with an input span like tokio does in the linked issues which makes it (to my knowledge) completely impossible to detect whether a span is from a proc macro, so this is a big hack that approximates it even further by checking if the code pointed at the span lines up with the AST.

@Alexendoo wrote a good comment about the proc macro situation in a zulip thread: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/report_in_external_macro.20no.20longer.20configurable.3F/near/448135182

To be clear, I'd also love to get rid of this, but afaik we're kind of stuck with this hack for now and add such checks whenever needed because proc macro tooling like quote::quote_spanned! makes it incredibly easy to fool clippy and we get a lot of bug reports for it because a lot of proc macros do this

Copy link
Member

@jieyouxu jieyouxu Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that's entirely reasonable for clippy. I opened a zulip topic for T-compiler (and T-lang) to discuss the proc-macro span hygiene syntax context situation and if we can somehow make more people aware of the proc-macro span hygiene syntax context issues (because rustc lints/diagnostics also gets bitten by this quite often): https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/proc-macro.20span.20hygiene.

It's really not ideal when many, many crate authors get proc-macro hygiene syntax context wrong because there's not much awareness or guidance for it. This in turn causes diagnostic and lint issues in rustc and clippy.

EDIT: and an issue rust-lang/rust#130995
EDIT 2: hygiene -> syntax context, I am one of the people confused myself

return;
}

Expand Down
135 changes: 81 additions & 54 deletions clippy_utils/src/check_proc_macro.rs
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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)]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It now emits a warning on this line

rust-clippy/tests/ui/dbg_macro/dbg_macro.rs

Line 53 in c771204

bar(dbg!());

which looks like a true positive to me


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
View file
Open in desktop
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,4 +363,8 @@ fn issue12907() -> String {
"".split("").next().unwrap().to_string()
}

fn issue13458() {
with_span!(span return);
}

fn main() {}
8 changes: 8 additions & 0 deletions tests/ui/needless_return.rs
View file
Open in desktop
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,4 +373,8 @@ fn issue12907() -> String {
return "".split("").next().unwrap().to_string();
}

fn issue13458() {
with_span!(span return);
}

fn main() {}
Loading