Auto merge of #9987 - Jarcho:issue_9957, r=flip1995

Don't cross contexts while building the suggestion for `redundant_closure_call` fixes #9957 changelog: `redundant_closure_call`: Don't cross macro contexts while building the suggestion
rust-lang · Nov 30, 2022 · 87963f0 · 87963f0
2 parents 78589ff + e0eba9c
commit 87963f0
Show file tree

Hide file tree

Showing 7 changed files with 122 additions and 43 deletions.
diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs
@@ -160,11 +160,13 @@ fn check_collapsible_no_if_let(cx: &EarlyContext<'_>, expr: &ast::Expr, check: &
         if let ast::ExprKind::If(ref check_inner, ref content, None) = inner.kind;
         // Prevent triggering on `if c { if let a = b { .. } }`.
         if !matches!(check_inner.kind, ast::ExprKind::Let(..));
-        if expr.span.ctxt() == inner.span.ctxt();
+        let ctxt = expr.span.ctxt();
+        if inner.span.ctxt() == ctxt;
         then {
             span_lint_and_then(cx, COLLAPSIBLE_IF, expr.span, "this `if` statement can be collapsed", |diag| {
-                let lhs = Sugg::ast(cx, check, "..");
-                let rhs = Sugg::ast(cx, check_inner, "..");
+                let mut app = Applicability::MachineApplicable;
+                let lhs = Sugg::ast(cx, check, "..", ctxt, &mut app);
+                let rhs = Sugg::ast(cx, check_inner, "..", ctxt, &mut app);
                 diag.span_suggestion(
                     expr.span,
                     "collapse nested if block",
@@ -173,7 +175,7 @@ fn check_collapsible_no_if_let(cx: &EarlyContext<'_>, expr: &ast::Expr, check: &
                         lhs.and(&rhs),
                         snippet_block(cx, content.span, "..", Some(expr.span)),
                     ),
-                    Applicability::MachineApplicable, // snippet
+                    app, // snippet
                 );
             });
         }

diff --git a/clippy_lints/src/redundant_closure_call.rs b/clippy_lints/src/redundant_closure_call.rs
@@ -81,8 +81,8 @@ impl EarlyLintPass for RedundantClosureCall {
                         "try not to call a closure in the expression where it is declared",
                         |diag| {
                             if fn_decl.inputs.is_empty() {
-                                let app = Applicability::MachineApplicable;
-                                let mut hint = Sugg::ast(cx, body, "..");
+                                let mut app = Applicability::MachineApplicable;
+                                let mut hint = Sugg::ast(cx, body, "..", closure.span.ctxt(), &mut app);
 
                                 if asyncness.is_async() {
                                     // `async x` is a syntax error, so it becomes `async { x }`

diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs
@@ -5,6 +5,7 @@
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind};
 use rustc_lint::{LateContext, LintContext};
+use rustc_session::Session;
 use rustc_span::hygiene;
 use rustc_span::source_map::{original_sp, SourceMap};
 use rustc_span::{BytePos, Pos, Span, SpanData, SyntaxContext, DUMMY_SP};
@@ -204,11 +205,20 @@ pub fn snippet_with_applicability<'a, T: LintContext>(
     span: Span,
     default: &'a str,
     applicability: &mut Applicability,
+) -> Cow<'a, str> {
+    snippet_with_applicability_sess(cx.sess(), span, default, applicability)
+}
+
+fn snippet_with_applicability_sess<'a>(
+    sess: &Session,
+    span: Span,
+    default: &'a str,
+    applicability: &mut Applicability,
 ) -> Cow<'a, str> {
     if *applicability != Applicability::Unspecified && span.from_expansion() {
         *applicability = Applicability::MaybeIncorrect;
     }
-    snippet_opt(cx, span).map_or_else(
+    snippet_opt_sess(sess, span).map_or_else(
         || {
             if *applicability == Applicability::MachineApplicable {
                 *applicability = Applicability::HasPlaceholders;
@@ -226,8 +236,12 @@ pub fn snippet_with_macro_callsite<'a, T: LintContext>(cx: &T, span: Span, defau
 }
 
 /// Converts a span to a code snippet. Returns `None` if not available.
-pub fn snippet_opt<T: LintContext>(cx: &T, span: Span) -> Option<String> {
-    cx.sess().source_map().span_to_snippet(span).ok()
+pub fn snippet_opt(cx: &impl LintContext, span: Span) -> Option<String> {
+    snippet_opt_sess(cx.sess(), span)
+}
+
+fn snippet_opt_sess(sess: &Session, span: Span) -> Option<String> {
+    sess.source_map().span_to_snippet(span).ok()
 }
 
 /// Converts a span (from a block) to a code snippet if available, otherwise use default.
@@ -277,8 +291,8 @@ pub fn snippet_block<'a, T: LintContext>(
 
 /// Same as `snippet_block`, but adapts the applicability level by the rules of
 /// `snippet_with_applicability`.
-pub fn snippet_block_with_applicability<'a, T: LintContext>(
-    cx: &T,
+pub fn snippet_block_with_applicability<'a>(
+    cx: &impl LintContext,
     span: Span,
     default: &'a str,
     indent_relative_to: Option<Span>,
@@ -299,7 +313,17 @@ pub fn snippet_block_with_applicability<'a, T: LintContext>(
 ///
 /// This will also return whether or not the snippet is a macro call.
 pub fn snippet_with_context<'a>(
-    cx: &LateContext<'_>,
+    cx: &impl LintContext,
+    span: Span,
+    outer: SyntaxContext,
+    default: &'a str,
+    applicability: &mut Applicability,
+) -> (Cow<'a, str>, bool) {
+    snippet_with_context_sess(cx.sess(), span, outer, default, applicability)
+}
+
+fn snippet_with_context_sess<'a>(
+    sess: &Session,
     span: Span,
     outer: SyntaxContext,
     default: &'a str,
@@ -318,7 +342,7 @@ pub fn snippet_with_context<'a>(
     );
 
     (
-        snippet_with_applicability(cx, span, default, applicability),
+        snippet_with_applicability_sess(sess, span, default, applicability),
         is_macro_call,
     )
 }

diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs
@@ -176,25 +176,28 @@ impl<'a> Sugg<'a> {
     }
 
     /// Prepare a suggestion from an expression.
-    pub fn ast(cx: &EarlyContext<'_>, expr: &ast::Expr, default: &'a str) -> Self {
+    pub fn ast(
+        cx: &EarlyContext<'_>,
+        expr: &ast::Expr,
+        default: &'a str,
+        ctxt: SyntaxContext,
+        app: &mut Applicability,
+    ) -> Self {
         use rustc_ast::ast::RangeLimits;
 
-        let snippet_without_expansion = |cx, span: Span, default| {
-            if span.from_expansion() {
-                snippet_with_macro_callsite(cx, span, default)
-            } else {
-                snippet(cx, span, default)
-            }
-        };
-
+        #[expect(clippy::match_wildcard_for_single_variants)]
         match expr.kind {
+            _ if expr.span.ctxt() != ctxt => Sugg::NonParen(snippet_with_context(cx, expr.span, ctxt, default, app).0),
             ast::ExprKind::AddrOf(..)
             | ast::ExprKind::Box(..)
             | ast::ExprKind::Closure { .. }
             | ast::ExprKind::If(..)
             | ast::ExprKind::Let(..)
             | ast::ExprKind::Unary(..)
-            | ast::ExprKind::Match(..) => Sugg::MaybeParen(snippet_without_expansion(cx, expr.span, default)),
+            | ast::ExprKind::Match(..) => match snippet_with_context(cx, expr.span, ctxt, default, app) {
+                (snip, false) => Sugg::MaybeParen(snip),
+                (snip, true) => Sugg::NonParen(snip),
+            },
             ast::ExprKind::Async(..)
             | ast::ExprKind::Block(..)
             | ast::ExprKind::Break(..)
@@ -224,45 +227,49 @@ impl<'a> Sugg<'a> {
             | ast::ExprKind::Array(..)
             | ast::ExprKind::While(..)
             | ast::ExprKind::Await(..)
-            | ast::ExprKind::Err => Sugg::NonParen(snippet_without_expansion(cx, expr.span, default)),
+            | ast::ExprKind::Err => Sugg::NonParen(snippet_with_context(cx, expr.span, ctxt, default, app).0),
             ast::ExprKind::Range(ref lhs, ref rhs, RangeLimits::HalfOpen) => Sugg::BinOp(
                 AssocOp::DotDot,
-                lhs.as_ref()
-                    .map_or("".into(), |lhs| snippet_without_expansion(cx, lhs.span, default)),
-                rhs.as_ref()
-                    .map_or("".into(), |rhs| snippet_without_expansion(cx, rhs.span, default)),
+                lhs.as_ref().map_or("".into(), |lhs| {
+                    snippet_with_context(cx, lhs.span, ctxt, default, app).0
+                }),
+                rhs.as_ref().map_or("".into(), |rhs| {
+                    snippet_with_context(cx, rhs.span, ctxt, default, app).0
+                }),
             ),
             ast::ExprKind::Range(ref lhs, ref rhs, RangeLimits::Closed) => Sugg::BinOp(
                 AssocOp::DotDotEq,
-                lhs.as_ref()
-                    .map_or("".into(), |lhs| snippet_without_expansion(cx, lhs.span, default)),
-                rhs.as_ref()
-                    .map_or("".into(), |rhs| snippet_without_expansion(cx, rhs.span, default)),
+                lhs.as_ref().map_or("".into(), |lhs| {
+                    snippet_with_context(cx, lhs.span, ctxt, default, app).0
+                }),
+                rhs.as_ref().map_or("".into(), |rhs| {
+                    snippet_with_context(cx, rhs.span, ctxt, default, app).0
+                }),
             ),
             ast::ExprKind::Assign(ref lhs, ref rhs, _) => Sugg::BinOp(
                 AssocOp::Assign,
-                snippet_without_expansion(cx, lhs.span, default),
-                snippet_without_expansion(cx, rhs.span, default),
+                snippet_with_context(cx, lhs.span, ctxt, default, app).0,
+                snippet_with_context(cx, rhs.span, ctxt, default, app).0,
             ),
             ast::ExprKind::AssignOp(op, ref lhs, ref rhs) => Sugg::BinOp(
                 astbinop2assignop(op),
-                snippet_without_expansion(cx, lhs.span, default),
-                snippet_without_expansion(cx, rhs.span, default),
+                snippet_with_context(cx, lhs.span, ctxt, default, app).0,
+                snippet_with_context(cx, rhs.span, ctxt, default, app).0,
             ),
             ast::ExprKind::Binary(op, ref lhs, ref rhs) => Sugg::BinOp(
                 AssocOp::from_ast_binop(op.node),
-                snippet_without_expansion(cx, lhs.span, default),
-                snippet_without_expansion(cx, rhs.span, default),
+                snippet_with_context(cx, lhs.span, ctxt, default, app).0,
+                snippet_with_context(cx, rhs.span, ctxt, default, app).0,
             ),
             ast::ExprKind::Cast(ref lhs, ref ty) => Sugg::BinOp(
                 AssocOp::As,
-                snippet_without_expansion(cx, lhs.span, default),
-                snippet_without_expansion(cx, ty.span, default),
+                snippet_with_context(cx, lhs.span, ctxt, default, app).0,
+                snippet_with_context(cx, ty.span, ctxt, default, app).0,
             ),
             ast::ExprKind::Type(ref lhs, ref ty) => Sugg::BinOp(
                 AssocOp::Colon,
-                snippet_without_expansion(cx, lhs.span, default),
-                snippet_without_expansion(cx, ty.span, default),
+                snippet_with_context(cx, lhs.span, ctxt, default, app).0,
+                snippet_with_context(cx, ty.span, ctxt, default, app).0,
             ),
         }
     }

diff --git a/tests/ui/redundant_closure_call_fixable.fixed b/tests/ui/redundant_closure_call_fixable.fixed
@@ -25,4 +25,16 @@ fn main() {
         x * y
     };
     let d = async { something().await };
+
+    macro_rules! m {
+        () => {
+            0
+        };
+    }
+    macro_rules! m2 {
+        () => {
+            m!()
+        };
+    }
+    m2!();
 }
diff --git a/tests/ui/redundant_closure_call_fixable.rs b/tests/ui/redundant_closure_call_fixable.rs
@@ -25,4 +25,16 @@ fn main() {
         x * y
     })();
     let d = (async || something().await)();
+
+    macro_rules! m {
+        () => {
+            (|| 0)()
+        };
+    }
+    macro_rules! m2 {
+        () => {
+            (|| m!())()
+        };
+    }
+    m2!();
 }
diff --git a/tests/ui/redundant_closure_call_fixable.stderr b/tests/ui/redundant_closure_call_fixable.stderr
@@ -52,5 +52,27 @@ error: try not to call a closure in the expression where it is declared
 LL |     let d = (async || something().await)();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try doing something like: `async { something().await }`
 
-error: aborting due to 4 previous errors
+error: try not to call a closure in the expression where it is declared
+  --> $DIR/redundant_closure_call_fixable.rs:36:13
+   |
+LL |             (|| m!())()
+   |             ^^^^^^^^^^^ help: try doing something like: `m!()`
+...
+LL |     m2!();
+   |     ----- in this macro invocation
+   |
+   = note: this error originates in the macro `m2` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: try not to call a closure in the expression where it is declared
+  --> $DIR/redundant_closure_call_fixable.rs:31:13
+   |
+LL |             (|| 0)()
+   |             ^^^^^^^^ help: try doing something like: `0`
+...
+LL |     m2!();
+   |     ----- in this macro invocation
+   |
+   = note: this error originates in the macro `m` which comes from the expansion of the macro `m2` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: aborting due to 6 previous errors