From bbcc260b6feecdeadd2b366cc8f021cc3404c9ec Mon Sep 17 00:00:00 2001 From: dswij Date: Fri, 25 Nov 2022 18:04:17 +0800 Subject: [PATCH 1/3] `manual_let_else`: keep macro call on suggestion blocks --- clippy_lints/src/manual_let_else.rs | 30 ++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 20f06830952..91f0dc2a717 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher::IfLetOrMatch; use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::peel_blocks; -use clippy_utils::source::snippet_opt; +use clippy_utils::source::{snippet, snippet_with_macro_callsite}; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::visitors::{for_each_expr, Descend}; use if_chain::if_chain; @@ -143,18 +143,22 @@ fn emit_manual_let_else(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, pat: // for this to be machine applicable. let app = Applicability::HasPlaceholders; - if let Some(sn_pat) = snippet_opt(cx, pat.span) && - let Some(sn_expr) = snippet_opt(cx, expr.span) && - let Some(sn_else) = snippet_opt(cx, else_body.span) - { - let else_bl = if matches!(else_body.kind, ExprKind::Block(..)) { - sn_else - } else { - format!("{{ {sn_else} }}") - }; - let sugg = format!("let {sn_pat} = {sn_expr} else {else_bl};"); - diag.span_suggestion(span, "consider writing", sugg, app); - } + let snippet_fn = if span.from_expansion() { + snippet + } else { + snippet_with_macro_callsite + }; + let sn_pat = snippet_fn(cx, pat.span, ""); + let sn_expr = snippet_fn(cx, expr.span, ""); + let sn_else = snippet_fn(cx, else_body.span, ""); + + let else_bl = if matches!(else_body.kind, ExprKind::Block(..)) { + sn_else.into_owned() + } else { + format!("{{ {sn_else} }}") + }; + let sugg = format!("let {sn_pat} = {sn_expr} else {else_bl};"); + diag.span_suggestion(span, "consider writing", sugg, app); }, ); } From a4b53c9c14705475b6e9041b56c441c981e86423 Mon Sep 17 00:00:00 2001 From: dswij Date: Fri, 25 Nov 2022 18:04:31 +0800 Subject: [PATCH 2/3] `manual_let_else`: Add test with expanded macros --- tests/ui/manual_let_else.rs | 14 ++++++++++++++ tests/ui/manual_let_else.stderr | 11 ++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs index 2ef40e5911a..48a162c1360 100644 --- a/tests/ui/manual_let_else.rs +++ b/tests/ui/manual_let_else.rs @@ -234,4 +234,18 @@ fn not_fire() { // If a type annotation is present, don't lint as // expressing the type might be too hard let v: () = if let Some(v_some) = g() { v_some } else { panic!() }; + + // Issue 9940 + // Suggestion should not expand macros + macro_rules! macro_call { + () => { + return () + }; + } + + let ff = Some(1); + let _ = match ff { + Some(value) => value, + _ => macro_call!(), + }; } diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr index 453b68b8bd0..52aac6bc673 100644 --- a/tests/ui/manual_let_else.stderr +++ b/tests/ui/manual_let_else.stderr @@ -259,5 +259,14 @@ LL | create_binding_if_some!(w, g()); | = note: this error originates in the macro `create_binding_if_some` (in Nightly builds, run with -Z macro-backtrace for more info) -error: aborting due to 17 previous errors +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else.rs:247:5 + | +LL | / let _ = match ff { +LL | | Some(value) => value, +LL | | _ => macro_call!(), +LL | | }; + | |______^ help: consider writing: `let Some(value) = ff else { macro_call!() };` + +error: aborting due to 18 previous errors From c502dee41ec38e6e0096abaa50a48e792e4c9d40 Mon Sep 17 00:00:00 2001 From: dswij Date: Wed, 30 Nov 2022 14:50:13 +0800 Subject: [PATCH 3/3] Use `snippet_with_context` instead of `_with_macro_callsite` --- clippy_lints/src/manual_let_else.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 91f0dc2a717..874d36ca9f4 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher::IfLetOrMatch; use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::peel_blocks; -use clippy_utils::source::{snippet, snippet_with_macro_callsite}; +use clippy_utils::source::snippet_with_context; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::visitors::{for_each_expr, Descend}; use if_chain::if_chain; @@ -141,16 +141,10 @@ fn emit_manual_let_else(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, pat: // * unused binding collision detection with existing ones // * putting patterns with at the top level | inside () // for this to be machine applicable. - let app = Applicability::HasPlaceholders; - - let snippet_fn = if span.from_expansion() { - snippet - } else { - snippet_with_macro_callsite - }; - let sn_pat = snippet_fn(cx, pat.span, ""); - let sn_expr = snippet_fn(cx, expr.span, ""); - let sn_else = snippet_fn(cx, else_body.span, ""); + let mut app = Applicability::HasPlaceholders; + let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", &mut app); + let (sn_expr, _) = snippet_with_context(cx, expr.span, span.ctxt(), "", &mut app); + let (sn_else, _) = snippet_with_context(cx, else_body.span, span.ctxt(), "", &mut app); let else_bl = if matches!(else_body.kind, ExprKind::Block(..)) { sn_else.into_owned()