Macro call guard and lint suggestion refactor

Author: benacq

clippy_lints/src/manual_checked_sub.rs

@@ -1,18 +1,15 @@
 use clippy_config::Conf;
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::diagnostics::span_lint;
 use clippy_utils::msrvs::{self, Msrv};
-use clippy_utils::source::snippet_with_applicability;
-use clippy_utils::sugg::Sugg;
+
 use clippy_utils::{is_mutable, path_to_local};
 use rustc_ast::{BinOpKind, LitIntType, LitKind};
 use rustc_data_structures::packed::Pu128;
-use rustc_errors::Applicability;
-use rustc_hir::intravisit::{Visitor, walk_expr, walk_stmt};
-use rustc_hir::{Expr, ExprKind, PatKind, QPath};
+use rustc_hir::intravisit::{Visitor, walk_expr};
+use rustc_hir::{Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::{self};
 use rustc_session::impl_lint_pass;
-use std::fmt::Write;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -79,12 +76,16 @@ impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub {
                 return;
             }
 
+            // Skip if either lhs or rhs is a macro call
+            if lhs.span.from_expansion() || rhs.span.from_expansion() {
+                return;
+            }
+
             if let BinOpKind::Ge | BinOpKind::Gt | BinOpKind::Le | BinOpKind::Lt = op.node {
                 SubExprVisitor {
                     cx,
                     if_expr: expr,
-                    if_body_block: if_expr.then,
-                    else_block: if_expr.r#else,
+
                     condition_lhs: lhs,
                     condition_rhs: rhs,
                     condition_op: op.node,
@@ -98,8 +99,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub {
 struct SubExprVisitor<'cx, 'tcx> {
     cx: &'cx LateContext<'tcx>,
     if_expr: &'tcx Expr<'tcx>,
-    if_body_block: &'tcx Expr<'tcx>,
-    else_block: Option<&'tcx Expr<'tcx>>,
+
     condition_lhs: &'tcx Expr<'tcx>,
     condition_rhs: &'tcx Expr<'tcx>,
     condition_op: BinOpKind,
@@ -110,20 +110,25 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> {
         if let ExprKind::Binary(op, sub_lhs, sub_rhs) = expr.kind
             && let BinOpKind::Sub = op.node
         {
+            // // Skip if either sub_lhs or sub_rhs is a macro call
+            if sub_lhs.span.from_expansion() || sub_rhs.span.from_expansion() {
+                return;
+            }
+
             if let ExprKind::Lit(lit) = self.condition_lhs.kind
                 && self.condition_op == BinOpKind::Lt
                 && let LitKind::Int(Pu128(0), _) = lit.node
                 && (is_referencing_same_variable(sub_lhs, self.condition_rhs))
             {
-                self.emit_lint(expr, Some(sub_rhs));
+                self.emit_lint();
             }
 
             if let ExprKind::Lit(lit) = self.condition_rhs.kind
                 && self.condition_op == BinOpKind::Gt
                 && let LitKind::Int(Pu128(0), _) = lit.node
                 && (is_referencing_same_variable(sub_lhs, self.condition_lhs))
             {
-                self.emit_lint(expr, Some(sub_rhs));
+                self.emit_lint();
             }
 
             if self.condition_op == BinOpKind::Ge
@@ -132,7 +137,7 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> {
                 && (is_referencing_same_variable(sub_rhs, self.condition_rhs)
                     || are_literals_equal(sub_rhs, self.condition_rhs))
             {
-                self.emit_lint(expr, None);
+                self.emit_lint();
             }
 
             if self.condition_op == BinOpKind::Le
@@ -141,7 +146,7 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> {
                 && (is_referencing_same_variable(sub_rhs, self.condition_lhs)
                     || are_literals_equal(sub_rhs, self.condition_lhs))
             {
-                self.emit_lint(expr, None);
+                self.emit_lint();
             }
         }
 
@@ -150,62 +155,12 @@ impl<'tcx> Visitor<'tcx> for SubExprVisitor<'_, 'tcx> {
 }
 
 impl<'tcx> SubExprVisitor<'_, 'tcx> {
-    fn emit_lint(&mut self, expr: &Expr<'tcx>, sub_rhs_expr: Option<&'tcx Expr<'tcx>>) {
-        let mut applicability = Applicability::MachineApplicable;
-
-        let body_snippet = snippet_with_applicability(self.cx, self.if_body_block.span, "..", &mut applicability);
-
-        let checked_sub_result_var_name = match self.condition_op {
-            BinOpKind::Lt | BinOpKind::Gt => generate_unique_var_name("decremented", self.if_body_block),
-            _ => generate_unique_var_name("difference", self.if_body_block),
-        };
-
-        let binary_expr_snippet = snippet_with_applicability(self.cx, expr.span, "..", &mut applicability);
-        let updated_usage_context_snippet = body_snippet
-            .as_ref()
-            .replace(binary_expr_snippet.as_ref(), &checked_sub_result_var_name);
-
-        let lhs_snippet = Sugg::hir(self.cx, self.condition_lhs, "..").maybe_paren();
-
-        let rhs_snippet = match sub_rhs_expr {
-            Some(sub_rhs) => Sugg::hir(self.cx, sub_rhs, "..").maybe_paren(),
-            None => Sugg::hir(self.cx, self.condition_rhs, "..").maybe_paren(),
-        };
-
-        // TODO: Check result variable is not already in scope
-        let mut suggestion = match self.condition_op {
-            BinOpKind::Le => {
-                format!(
-                    "if let Some({checked_sub_result_var_name}) = {rhs_snippet}.checked_sub({lhs_snippet}) {updated_usage_context_snippet}"
-                )
-            },
-
-            BinOpKind::Lt => {
-                format!(
-                    "if let Some({checked_sub_result_var_name}) = {}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}",
-                    Sugg::hir(self.cx, self.condition_rhs, "..").maybe_paren()
-                )
-            },
-            _ => {
-                format!(
-                    "if let Some({checked_sub_result_var_name}) = {lhs_snippet}.checked_sub({rhs_snippet}) {updated_usage_context_snippet}"
-                )
-            },
-        };
-
-        if let Some(else_expr) = self.else_block {
-            let else_snippet = snippet_with_applicability(self.cx, else_expr.span, "..", &mut applicability);
-            write!(suggestion, " else {else_snippet}").unwrap();
-        }
-
-        span_lint_and_sugg(
+    fn emit_lint(&mut self) {
+        span_lint(
             self.cx,
             MANUAL_CHECKED_SUB,
             self.if_expr.span,
-            "manual re-implementation of checked subtraction",
-            "consider using `.checked_sub()`",
-            suggestion,
-            applicability,
+            "manual re-implementation of checked subtraction - consider using `.checked_sub()`",
         );
     }
 }
@@ -236,93 +191,3 @@ fn is_referencing_same_variable<'tcx>(expr1: &'tcx Expr<'_>, expr2: &'tcx Expr<'
 
     false
 }
-
-/// Generates a unique variable name based on the provided base name.
-/// If the base name already exists in the scope, appends a number to make it unique.
-fn generate_unique_var_name<'tcx>(base_name: &str, scope_expr: &'tcx Expr<'tcx>) -> String {
-    struct VarNameVisitor<'cx> {
-        base_name: &'cx str,
-        is_name_in_scope: bool,
-    }
-
-    impl<'tcx> Visitor<'tcx> for VarNameVisitor<'_> {
-        fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
-            if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind
-                && let Some(segment) = path.segments.last()
-            {
-                let name = segment.ident.name.to_string();
-                if name == self.base_name {
-                    self.is_name_in_scope = true;
-                }
-            }
-
-            walk_expr(self, expr);
-        }
-
-        fn visit_stmt(&mut self, stmt: &'tcx rustc_hir::Stmt<'_>) {
-            if let rustc_hir::StmtKind::Let(let_stmt) = &stmt.kind
-                && let PatKind::Binding(_, _, ident, _) = &let_stmt.pat.kind
-            {
-                let name = ident.name.to_string();
-                if name == self.base_name {
-                    self.is_name_in_scope = true;
-                }
-            }
-
-            walk_stmt(self, stmt);
-        }
-    }
-
-    let mut visitor = VarNameVisitor {
-        base_name,
-        is_name_in_scope: false,
-    };
-
-    if let ExprKind::Block(block, _) = &scope_expr.kind {
-        for stmt in block.stmts {
-            visitor.visit_stmt(stmt);
-        }
-
-        // Visit the optional expression at the end of the block
-        if let Some(expr) = block.expr {
-            visitor.visit_expr(expr);
-        }
-    } else {
-        visitor.visit_expr(scope_expr);
-    }
-
-    if !visitor.is_name_in_scope {
-        return base_name.to_string();
-    }
-
-    // If the base name is in scope, append a number
-    // Keep incrementing until we find a name that's not in scope
-    let mut counter = 1;
-    loop {
-        let candidate = format!("{base_name}_{counter}");
-
-        let mut candidate_visitor = VarNameVisitor {
-            base_name: &candidate,
-            is_name_in_scope: false,
-        };
-
-        // Check if the candidate name is in scope
-        if let ExprKind::Block(block, _) = &scope_expr.kind {
-            for stmt in block.stmts {
-                candidate_visitor.visit_stmt(stmt);
-            }
-
-            if let Some(expr) = block.expr {
-                candidate_visitor.visit_expr(expr);
-            }
-        } else {
-            candidate_visitor.visit_expr(scope_expr);
-        }
-
-        if !candidate_visitor.is_name_in_scope {
-            return candidate;
-        }
-
-        counter += 1;
-    }
-}