feat: configuration support added for test without fail case

Author: kayagokalp

clippy_config/src/conf.rs

@@ -1,6 +1,9 @@
 use crate::ClippyConfiguration;
 use crate::msrvs::Msrv;
-use crate::types::{DisallowedPath, MacroMatcher, MatchLintBehaviour, PubUnderscoreFieldsBehaviour, Rename};
+use crate::types::{
+    DEFAULT_FALLIBLE_PATHS, DEFAULT_NONFALLIBLE_PATHS, DisallowedPath, MacroMatcher, MatchLintBehaviour,
+    PubUnderscoreFieldsBehaviour, Rename, TestWithoutFailCaseBehaviour,
+};
 use rustc_errors::Applicability;
 use rustc_session::Session;
 use rustc_span::edit_distance::edit_distance;
@@ -628,6 +631,9 @@ define_Conf! {
     /// if no suggestion can be made.
     #[lints(indexing_slicing)]
     suppress_restriction_lint_in_const: bool = false,
+    /// Lint tests to understand whether it can fail or not.
+    #[lints(test_without_fail_case)]
+    test_without_fail_case: TestWithoutFailCaseBehaviour = TestWithoutFailCaseBehaviour::default(),
     /// The maximum size of objects (in bytes) that will be linted. Larger objects are ok on the heap
     #[lints(boxed_local, useless_vec)]
     too_large_for_stack: u64 = 200,
@@ -724,6 +730,14 @@ pub fn lookup_conf_file() -> io::Result<(Option<PathBuf>, Vec<String>)> {
 fn deserialize(file: &SourceFile) -> TryConf {
     match toml::de::Deserializer::new(file.src.as_ref().unwrap()).deserialize_map(ConfVisitor(file)) {
         Ok(mut conf) => {
+            extend_vec_if_indicator_present(
+                &mut conf.conf.test_without_fail_case.fallible_paths,
+                DEFAULT_FALLIBLE_PATHS,
+            );
+            extend_vec_if_indicator_present(
+                &mut conf.conf.test_without_fail_case.non_fallible_paths,
+                DEFAULT_NONFALLIBLE_PATHS,
+            );
             extend_vec_if_indicator_present(&mut conf.conf.disallowed_names, DEFAULT_DISALLOWED_NAMES);
             extend_vec_if_indicator_present(&mut conf.conf.allowed_prefixes, DEFAULT_ALLOWED_PREFIXES);
             extend_vec_if_indicator_present(

clippy_config/src/types.rs

@@ -130,3 +130,55 @@ pub enum PubUnderscoreFieldsBehaviour {
     PubliclyExported,
     AllPubFields,
 }
+
+/// Default paths considered as fallible for `test_without_fail_case` lint.
+pub(crate) const DEFAULT_FALLIBLE_PATHS: &[&str] =
+    &["core::panic", "core::assert", "core::assert_eq", "core::assert_ne"];
+/// Default paths considered as non-fallible for `test_without_fail_case` lint.
+pub(crate) const DEFAULT_NONFALLIBLE_PATHS: &[&str] =
+    &["std::print", "std::println", "std::dbg", "std::eprint", "std::eprintln"];
+
+#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)]
+pub struct TestWithoutFailCaseBehaviour {
+    /// Indexing into a slice is fallible, and thus the a test that is doing that
+    /// can fail. If set true, the lint consider indexing into a slice a failable case
+    /// and won't lint tests that has some sort of indexing. This analysis still done
+    /// in a interfunctional manner. Meaning that any indexing opeartion done inside of
+    /// a function that the test calls will still mark the test fallible.
+    ///
+    /// By default this is set to `false`. That is becuase from a usability perspective,
+    /// indexing an array is not the intended way to fail a test. So setting this `true`
+    /// reduces false positives but makes the analysis more focused on possible byproducts
+    /// of a test. That is the set of operations to get the point we assert something rather
+    /// than the existance of asserting that thing.
+    pub include_slice_indexing: bool,
+    /// List of full paths of macros and functions, that can fail. If a test, or a function
+    /// that the test calls contains any one of these macros, we will mark the test fallible.
+    /// By default this macros are defined as `assert!`, `assert_eq!`, `panic!`.
+    pub fallible_paths: Vec<String>,
+    /// List of full paths of macros and functions, that we want to mark as not going to fail.
+    /// This allows us to make the lint more focused on actual short comings of our test suite
+    /// by marking common routines non-fallible, even though they are fallible.
+    /// By default this list contains: `println!`, `print!`, `dbg!`.
+    pub non_fallible_paths: Vec<String>,
+}
+
+impl TestWithoutFailCaseBehaviour {
+    pub fn new(include_slice_indexing: bool, fallible_paths: Vec<String>, non_fallible_paths: Vec<String>) -> Self {
+        Self {
+            include_slice_indexing,
+            fallible_paths,
+            non_fallible_paths,
+        }
+    }
+}
+
+impl Default for TestWithoutFailCaseBehaviour {
+    fn default() -> Self {
+        Self {
+            include_slice_indexing: false,
+            fallible_paths: DEFAULT_FALLIBLE_PATHS.iter().map(|s| s.to_string()).collect(),
+            non_fallible_paths: DEFAULT_NONFALLIBLE_PATHS.iter().map(|a| a.to_string()).collect(),
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -950,6 +950,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(move |_| Box::new(unused_trait_names::UnusedTraitNames::new(conf)));
     store.register_late_pass(|_| Box::new(manual_ignore_case_cmp::ManualIgnoreCaseCmp));
     store.register_late_pass(|_| Box::new(unnecessary_literal_bound::UnnecessaryLiteralBound));
-    store.register_late_pass(|_| Box::new(test_without_fail_case::TestWithoutFailCase));
+    store.register_late_pass(|_| Box::new(test_without_fail_case::TestWithoutFailCase::new(conf)));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }

clippy_lints/src/test_without_fail_case.rs

@@ -1,5 +1,6 @@
+use clippy_config::Conf;
 use clippy_utils::diagnostics::span_lint_and_note;
-use clippy_utils::macros::{is_panic, root_macro_call_first_node};
+use clippy_utils::macros::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};
@@ -11,8 +12,8 @@ use rustc_hir::{AnonConst, Expr, ExprKind, Item, ItemKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::nested_filter;
 use rustc_middle::ty;
-use rustc_session::declare_lint_pass;
-use rustc_span::{Span, sym};
+use rustc_session::impl_lint_pass;
+use rustc_span::sym;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -46,10 +47,26 @@ declare_clippy_lint! {
     #[clippy::version = "1.82.0"]
     pub TEST_WITHOUT_FAIL_CASE,
     restriction,
-    "test function cannot fail because it does not panic or assert"
+    "test function cannot fail because it does not anyway to panic or assert"
 }
 
-declare_lint_pass!(TestWithoutFailCase => [TEST_WITHOUT_FAIL_CASE]);
+pub struct TestWithoutFailCase {
+    config: SearchConfig,
+}
+
+impl TestWithoutFailCase {
+    pub fn new(conf: &Conf) -> Self {
+        Self {
+            config: SearchConfig {
+                indexing_into_slice_fallible: conf.test_without_fail_case.include_slice_indexing,
+                fallible_paths: conf.test_without_fail_case.fallible_paths.iter().cloned().collect(),
+                non_fallible_paths: conf.test_without_fail_case.non_fallible_paths.iter().cloned().collect(),
+            },
+        }
+    }
+}
+
+impl_lint_pass!(TestWithoutFailCase => [TEST_WITHOUT_FAIL_CASE]);
 
 impl<'tcx> LateLintPass<'tcx> for TestWithoutFailCase {
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
@@ -59,8 +76,8 @@ impl<'tcx> LateLintPass<'tcx> for TestWithoutFailCase {
         {
             let body = cx.tcx.hir().body(body_id);
             let typeck_results = cx.tcx.typeck(item.owner_id);
-            let panic_span = SearchPanicIntraFunction::find_span(cx, typeck_results, body);
-            if panic_span.is_none() {
+            let fail_found = SearchFailIntraFunction::find_fail(cx, typeck_results, &self.config, body);
+            if !fail_found {
                 // No way to panic for this test function
                 span_lint_and_note(
                     cx,
@@ -75,43 +92,61 @@ impl<'tcx> LateLintPass<'tcx> for TestWithoutFailCase {
     }
 }
 
+/// Set of options that user provivdes through configs, to modify the lint behaviour
+/// according to their repo.
+struct SearchConfig {
+    /// If search should consider indexing into slice as fallible.
+    indexing_into_slice_fallible: bool,
+    /// Set of paths that are marked as fallible.
+    fallible_paths: FxHashSet<String>,
+    /// Set of paths that are marked as non fallible.
+    non_fallible_paths: FxHashSet<String>,
+}
+
 /// 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> {
+struct SearchFailIntraFunction<'a, 'tcx> {
     /// The lint context
     cx: &'a LateContext<'tcx>,
-    /// The span where a panic was found
-    panic_span: Option<Span>,
+    /// Whether a way to fail is found or not.
+    fail_found: bool,
     /// 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>,
+    /// Search configs containing the set of user provided configurations.
+    search_config: &'a SearchConfig,
 }
 
-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 {
+impl<'a, 'tcx> SearchFailIntraFunction<'a, 'tcx> {
+    pub fn new(
+        cx: &'a LateContext<'tcx>,
+        typeck_results: &'tcx ty::TypeckResults<'tcx>,
+        search_config: &'a SearchConfig,
+    ) -> Self {
         Self {
             cx,
-            panic_span: None,
+            fail_found: false,
             typeck_results,
             visited_functions: FxHashSet::default(),
+            search_config,
         }
     }
 
     /// Searches for a way to panic in the given body and returns the span if found
-    pub fn find_span(
+    pub fn find_fail(
         cx: &'a LateContext<'tcx>,
         typeck_results: &'tcx ty::TypeckResults<'tcx>,
+        search_config: &'a SearchConfig,
         body: impl Visitable<'tcx>,
-    ) -> Option<Span> {
-        let mut visitor = Self::new(cx, typeck_results);
+    ) -> bool {
+        let mut visitor = Self::new(cx, typeck_results, search_config);
         body.visit(&mut visitor);
-        visitor.panic_span
+        visitor.fail_found
     }
 
     /// Checks the called function to see if it contains a panic
-    fn check_called_function(&mut self, def_id: DefId, span: Span) {
+    fn check_called_function(&mut self, def_id: DefId) {
         // Avoid infinite recursion by checking if we've already visited this function
         if !self.visited_functions.insert(def_id) {
             return;
@@ -122,30 +157,31 @@ impl<'a, 'tcx> SearchPanicIntraFunction<'a, 'tcx> {
             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 {
+                    let mut new_visitor = SearchFailIntraFunction {
                         cx: self.cx,
-                        panic_span: None,
+                        fail_found: false,
                         typeck_results,
                         visited_functions: self.visited_functions.clone(),
+                        search_config: &self.search_config,
                     };
                     body.visit(&mut new_visitor);
-                    if let Some(panic_span) = new_visitor.panic_span {
-                        self.panic_span = Some(panic_span);
+                    if new_visitor.fail_found {
+                        self.fail_found = true;
                     }
                 }
             }
         } else {
             // For external functions, assume they can panic
-            self.panic_span = Some(span);
+            self.fail_found = true;
         }
     }
 }
 
-impl<'tcx> Visitor<'tcx> for SearchPanicIntraFunction<'_, 'tcx> {
+impl<'tcx> Visitor<'tcx> for SearchFailIntraFunction<'_, 'tcx> {
     type NestedFilter = nested_filter::OnlyBodies;
 
     fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
-        if self.panic_span.is_some() {
+        if self.fail_found {
             // If we've already found a panic, no need to continue
             return;
         }
@@ -154,8 +190,8 @@ impl<'tcx> Visitor<'tcx> for SearchPanicIntraFunction<'_, 'tcx> {
             ExprKind::Call(callee, args) => {
                 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() {
+                        self.check_called_function(def_id);
+                        if self.fail_found {
                             return;
                         }
                     }
@@ -167,8 +203,8 @@ impl<'tcx> Visitor<'tcx> for SearchPanicIntraFunction<'_, 'tcx> {
             },
             ExprKind::MethodCall(_, receiver, args, _) => {
                 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() {
+                    self.check_called_function(def_id);
+                    if self.fail_found {
                         return;
                     }
                 }
@@ -179,33 +215,34 @@ impl<'tcx> Visitor<'tcx> for SearchPanicIntraFunction<'_, 'tcx> {
             },
             _ => {
                 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.
+                    let macro_with_path = self.cx.tcx.def_path_str(macro_call.def_id);
+                    // Skip macros that are defined as `non_fallible` in the clippy.toml file.
+                    // Some examples that would fit here can be `println!`, `print!`, `eprintln!`,
+                    // `eprint!`. This is a special case, these macros can panic, but it is very
+                    // unlikely that this is intended as the tests assertion. In the name of
+                    // reducing false negatives we are giving out soundness.
                     //
-                    // This decision can be justified as it is highly unlikely that the tool is sound
+                    // This decision can be justified as it is highly unlikely that this lint 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") {
+                    // negatives.
+                    if self.search_config.non_fallible_paths.contains(&macro_with_path) {
                         return;
                     }
-                    if is_panic(self.cx, macro_call.def_id)
-                        || matches!(macro_name.as_str(), "assert" | "assert_eq" | "assert_ne")
-                    {
-                        self.panic_span = Some(macro_call.span);
+
+                    if self.search_config.fallible_paths.contains(&macro_with_path) {
+                        self.fail_found = true;
                         return;
                     }
                 }
 
+                // TODO: also make these two configurable.
                 // 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);
+                        self.fail_found = true;
                         return;
                     }
                 }

tests/ui/test_without_fail_case.rs

@@ -73,6 +73,14 @@ fn test_implicit_assert() {
     implicit_assert();
 }
 
+// Should lint with default config.
+#[test]
+fn test_slice_index() {
+    let a = [1, 2, 3, 4, 5, 6];
+    // indexing into slice, this can fail but by default check for this is disabled.
+    let b = a[0];
+}
+
 fn implicit_panic() {
     panic!("this is an implicit panic");
 }

tests/ui/test_without_fail_case.stderr

@@ -12,5 +12,17 @@ LL | | }
    = note: `-D clippy::test-without-fail-case` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::test_without_fail_case)]`
 
-error: aborting due to 1 previous error
+error: this function marked with `#[test]` cannot fail and will always succeed
+  --> tests/ui/test_without_fail_case.rs:78:1
+   |
+LL | / fn test_slice_index() {
+LL | |     let a = [1, 2, 3, 4, 5, 6];
+LL | |     // indexing into slice, this can fail but by default check for this is disabled.
+LL | |     let b = a[0];
+LL | | }
+   | |_^
+   |
+   = note: consider adding assertions or panics to test failure cases
+
+error: aborting due to 2 previous errors