feat: implement function call follow logic

kayagokalp · kayagokalp · commit e0fc5b4251d2 · 2024-10-21T15:31:17.000-07:00
diff --git a/clippy_lints/src/test_without_fail_case.rs b/clippy_lints/src/test_without_fail_case.rs
@@ -3,8 +3,11 @@ use clippy_utils::macros::{is_panic, root_macro_call_first_node};
 use clippy_utils::ty::is_type_diagnostic_item;
 use clippy_utils::visitors::Visitable;
 use clippy_utils::{is_in_test_function, method_chain_args};
-use rustc_hir::intravisit::{self, FnKind, Visitor};
-use rustc_hir::{AnonConst, Body, Expr, FnDecl, Item, ItemKind};
+use rustc_data_structures::fx::FxHashSet;
+use rustc_hir::def::Res;
+use rustc_hir::def_id::DefId;
+use rustc_hir::intravisit::{self, Visitor};
+use rustc_hir::{AnonConst, Expr, ExprKind, Item, ItemKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::nested_filter;
 use rustc_middle::ty;
@@ -13,142 +16,216 @@ use rustc_span::{Span, sym};
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Triggers when a testing function (marked with the `#[test]` attribute) does not have a way to fail.
+    /// Checks for test functions that cannot fail.
     ///
-    /// ### Why restrict this?
-    /// If a test does not have a way to fail, the developer might be getting false positives from their test suites.
-    /// The idiomatic way of using test functions should be such that they actually can fail in an erroneous state.
+    /// ### Why is this bad?
+    /// A test function that cannot fail might indicate that it does not actually test anything.
+    /// It could lead to false positives in test suites, giving a false sense of security.
     ///
     /// ### Example
-    /// ```no_run
+    /// ```rust
     /// #[test]
-    /// fn my_cool_test() {
-    ///     // [...]
-    /// }
-    ///
-    /// #[cfg(test)]
-    /// mod tests {
-    ///     // [...]
+    /// fn my_test() {
+    ///     let x = 2;
+    ///     let y = 2;
+    ///     if x + y != 4 {
+    ///         eprintln!("this is not a correct test")
+    ///     }
     /// }
-    ///
     /// ```
+    ///
     /// Use instead:
-    /// ```no_run
-    /// #[cfg(test)]
-    /// mod tests {
-    ///     #[test]
-    ///     fn my_cool_test() {
-    ///         // [...]
-    ///     }
+    /// ```rust
+    /// #[test]
+    /// fn my_test() {
+    ///     let x = 2;
+    ///     let y = 2;
+    ///     assert_eq!(x + y, 4);
     /// }
     /// ```
-    #[clippy::version = "1.70.0"]
+    #[clippy::version = "1.82.0"]
     pub TEST_WITHOUT_FAIL_CASE,
     restriction,
-    "A test function is outside the testing module."
+    "test function cannot fail because it does not panic or assert"
 }
 
 declare_lint_pass!(TestWithoutFailCase => [TEST_WITHOUT_FAIL_CASE]);
 
-/*
-impl<'tcx> LateLintPass<'tcx> for TestWithoutFailCase {
-    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx rustc_hir::Item<'_>) {
-        if let rustc_hir::ItemKind::Fn(sig, _, body_id) = item.kind {
-
-        }
-        if is_in_test_function(cx.tcx, item.hir_id()) {
-            let typck = cx.tcx.typeck(item.id.owner_id.def_id);
-            let find_panic_visitor = FindPanicUnwrap::find_span(cx, typck, item.body)
-            span_lint(cx, TEST_WITHOUT_FAIL_CASE, item.span, "test function cannot panic");
-        }
-    }
-}
-    */
-
 impl<'tcx> LateLintPass<'tcx> for TestWithoutFailCase {
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
+        // Only interested in functions that are test functions
         if let ItemKind::Fn(_, _, body_id) = item.kind
             && is_in_test_function(cx.tcx, item.hir_id())
         {
             let body = cx.tcx.hir().body(body_id);
-            let typ = cx.tcx.typeck(item.owner_id);
-            let panic_span = FindPanicUnwrap::find_span(cx, typ, body);
+            let typeck_results = cx.tcx.typeck(item.owner_id);
+            let panic_span = SearchPanicIntraFunction::find_span(cx, typeck_results, body);
             if panic_span.is_none() {
-                // No way to panic for this test.
-                #[expect(clippy::collapsible_span_lint_calls, reason = "rust-clippy#7797")]
+                // No way to panic for this test function
                 span_lint_and_then(
                     cx,
                     TEST_WITHOUT_FAIL_CASE,
                     item.span,
-                    "this function marked with #[test] has no way to fail",
+                    "this function marked with `#[test]` cannot fail and will always succeed",
                     |diag| {
-                        diag.note("make sure that something is checked in this test");
+                        diag.note("consider adding assertions or panics to test failure cases");
                     },
                 );
             }
         }
     }
 }
 
-struct FindPanicUnwrap<'a, 'tcx> {
+/// Visitor that searches for expressions that could cause a panic, such as `panic!`,
+/// `assert!`, `unwrap()`, or calls to functions that can panic.
+struct SearchPanicIntraFunction<'a, 'tcx> {
+    /// The lint context
     cx: &'a LateContext<'tcx>,
+    /// Whether we are inside a constant context
     is_const: bool,
+    /// The span where a panic was found
     panic_span: Option<Span>,
+    /// Type checking results for the current body
     typeck_results: &'tcx ty::TypeckResults<'tcx>,
+    /// Set of function `DefId`s that have been visited to avoid infinite recursion
+    visited_functions: FxHashSet<DefId>,
 }
 
-impl<'a, 'tcx> FindPanicUnwrap<'a, 'tcx> {
+impl<'a, 'tcx> SearchPanicIntraFunction<'a, 'tcx> {
+    /// Creates a new `FindPanicUnwrap` visitor
+    pub fn new(cx: &'a LateContext<'tcx>, typeck_results: &'tcx ty::TypeckResults<'tcx>) -> Self {
+        Self {
+            cx,
+            is_const: false,
+            panic_span: None,
+            typeck_results,
+            visited_functions: FxHashSet::default(),
+        }
+    }
+
+    /// Searches for a way to panic in the given body and returns the span if found
     pub fn find_span(
         cx: &'a LateContext<'tcx>,
         typeck_results: &'tcx ty::TypeckResults<'tcx>,
         body: impl Visitable<'tcx>,
     ) -> Option<(Span, bool)> {
-        let mut vis = Self {
-            cx,
-            is_const: false,
-            panic_span: None,
-            typeck_results,
-        };
-        body.visit(&mut vis);
-        vis.panic_span.map(|el| (el, vis.is_const))
+        let mut visitor = Self::new(cx, typeck_results);
+        body.visit(&mut visitor);
+        visitor.panic_span.map(|span| (span, visitor.is_const))
+    }
+
+    /// Checks the called function to see if it contains a panic
+    fn check_called_function(&mut self, def_id: DefId, span: Span) {
+        // Avoid infinite recursion by checking if we've already visited this function
+        if !self.visited_functions.insert(def_id) {
+            return;
+        }
+
+        if def_id.is_local() {
+            let hir = self.cx.tcx.hir();
+            if let Some(local_def_id) = def_id.as_local() {
+                if let Some(body) = hir.maybe_body_owned_by(local_def_id) {
+                    let typeck_results = self.cx.tcx.typeck(local_def_id);
+                    let mut new_visitor = SearchPanicIntraFunction {
+                        cx: self.cx,
+                        is_const: false,
+                        panic_span: None,
+                        typeck_results,
+                        visited_functions: self.visited_functions.clone(),
+                    };
+                    body.visit(&mut new_visitor);
+                    if let Some(panic_span) = new_visitor.panic_span {
+                        self.panic_span = Some(panic_span);
+                    }
+                }
+            }
+        } else {
+            // For external functions, assume they can panic
+            self.panic_span = Some(span);
+        }
     }
 }
 
-impl<'a, 'tcx> Visitor<'tcx> for FindPanicUnwrap<'a, 'tcx> {
+impl<'a, 'tcx> Visitor<'tcx> for SearchPanicIntraFunction<'a, 'tcx> {
     type NestedFilter = nested_filter::OnlyBodies;
 
     fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
         if self.panic_span.is_some() {
+            // If we've already found a panic, no need to continue
             return;
         }
 
-        if let Some(macro_call) = root_macro_call_first_node(self.cx, expr) {
-            if is_panic(self.cx, macro_call.def_id)
-                || matches!(
-                    self.cx.tcx.item_name(macro_call.def_id).as_str(),
-                    "assert" | "assert_eq" | "assert_ne"
-                )
-            {
-                self.is_const = self.cx.tcx.hir().is_inside_const_context(expr.hir_id);
-                self.panic_span = Some(macro_call.span);
-            }
-        }
+        match expr.kind {
+            ExprKind::Call(callee, args) => {
+                // Function call
+                if let ExprKind::Path(ref qpath) = callee.kind {
+                    if let Res::Def(_, def_id) = self.cx.qpath_res(qpath, callee.hir_id) {
+                        self.check_called_function(def_id, expr.span);
+                        if self.panic_span.is_some() {
+                            return;
+                        }
+                    }
+                }
+                // Visit callee and arguments
+                self.visit_expr(callee);
+                for arg in args {
+                    self.visit_expr(arg);
+                }
+            },
+            ExprKind::MethodCall(_, receiver, args, _) => {
+                // Method call
+                if let Some(def_id) = self.typeck_results.type_dependent_def_id(expr.hir_id) {
+                    self.check_called_function(def_id, expr.span);
+                    if self.panic_span.is_some() {
+                        return;
+                    }
+                }
+                // Visit receiver and arguments
+                self.visit_expr(receiver);
+                for arg in args {
+                    self.visit_expr(arg);
+                }
+            },
+            _ => {
+                if let Some(macro_call) = root_macro_call_first_node(self.cx, expr) {
+                    let macro_name = self.cx.tcx.item_name(macro_call.def_id);
+                    // Skip macros like `println!`, `print!`, `eprintln!`, `eprint!`.
+                    // This is a special case, these macros can panic, but it is very unlikely
+                    // that this is intended. In the name of reducing false positiveness we are
+                    // giving out soundness.
+                    //
+                    // This decision can be justified as it is highly unlikely that the tool is sound
+                    // without this additional check, and with this we are reducing the number of false
+                    // positives.
+                    if matches!(macro_name.as_str(), "println" | "print" | "eprintln" | "eprint" | "dbg") {
+                        return;
+                    }
+                    if is_panic(self.cx, macro_call.def_id)
+                        || matches!(macro_name.as_str(), "assert" | "assert_eq" | "assert_ne")
+                    {
+                        self.is_const = self.cx.tcx.hir().is_inside_const_context(expr.hir_id);
+                        self.panic_span = Some(macro_call.span);
+                        return;
+                    }
+                }
 
-        // check for `unwrap` and `expect` for both `Option` and `Result`
-        if let Some(arglists) = method_chain_args(expr, &["unwrap"]).or(method_chain_args(expr, &["expect"])) {
-            let receiver_ty = self.typeck_results.expr_ty(arglists[0].0).peel_refs();
-            if is_type_diagnostic_item(self.cx, receiver_ty, sym::Option)
-                || is_type_diagnostic_item(self.cx, receiver_ty, sym::Result)
-            {
-                self.panic_span = Some(expr.span);
-            }
-        }
+                // Check for `unwrap` and `expect` method calls
+                if let Some(arglists) = method_chain_args(expr, &["unwrap"]).or(method_chain_args(expr, &["expect"])) {
+                    let receiver_ty = self.typeck_results.expr_ty(arglists[0].0).peel_refs();
+                    if is_type_diagnostic_item(self.cx, receiver_ty, sym::Option)
+                        || is_type_diagnostic_item(self.cx, receiver_ty, sym::Result)
+                    {
+                        self.panic_span = Some(expr.span);
+                        return;
+                    }
+                }
 
-        // and check sub-expressions
-        intravisit::walk_expr(self, expr);
+                intravisit::walk_expr(self, expr);
+            },
+        }
     }
 
-    // Panics in const blocks will cause compilation to fail.
+    // Do not visit anonymous constants, as panics in const contexts are compile-time errors
     fn visit_anon_const(&mut self, _: &'tcx AnonConst) {}
 
     fn nested_visit_map(&mut self) -> Self::Map {
diff --git a/tests/ui/test_without_fail_case.rs b/tests/ui/test_without_fail_case.rs
@@ -3,16 +3,50 @@
 #![allow(clippy::unnecessary_literal_unwrap)]
 #![warn(clippy::test_without_fail_case)]
 
-// Should lint
+struct DummyStruct;
+
+impl DummyStruct {
+    fn panic_in_impl(self) {
+        panic!("a")
+    }
+
+    fn assert_in_impl(self, a: bool) {
+        assert!(a)
+    }
+
+    fn unwrap_in_impl(self, a: Option<i32>) {
+        let _ = a.unwrap();
+    }
+}
+
 #[test]
 fn test_without_fail() {
-    // This test cannot fail.
     let x = 5;
     let y = x + 2;
     println!("y: {}", y);
 }
-//~^ ERROR: this function marked with #[test] does not have a way to fail.
-//~^ NOTE: Ensure that something is being tested and asserted by this test.
+
+// Should not lint
+#[test]
+fn impl_panic() {
+    let dummy_struct = DummyStruct;
+    dummy_struct.panic_in_impl();
+}
+
+// Should not lint
+#[test]
+fn impl_assert() {
+    let dummy_struct = DummyStruct;
+    dummy_struct.assert_in_impl(false);
+}
+
+// Should not lint
+#[test]
+fn impl_unwrap() {
+    let dummy_struct = DummyStruct;
+    let a = Some(10i32);
+    dummy_struct.unwrap_in_impl(a);
+}
 
 // Should not lint
 #[test]
@@ -27,6 +61,18 @@ fn test_implicit_panic() {
     implicit_panic()
 }
 
+// Should not lint
+#[test]
+fn test_implicit_unwrap() {
+    implicit_unwrap();
+}
+
+// Should not lint
+#[test]
+fn test_implicit_assert() {
+    implicit_assert();
+}
+
 fn implicit_panic() {
     panic!("this is an implicit panic");
 }
diff --git a/tests/ui/test_without_fail_case.stderr b/tests/ui/test_without_fail_case.stderr
