tests: add ui-toml test structure

Author: kayagokalp

CHANGELOG.md

@@ -6216,6 +6216,7 @@ Released 2018-09-13
 [`standard-macro-braces`]: https://doc.rust-lang.org/clippy/lint_configuration.html#standard-macro-braces
 [`struct-field-name-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#struct-field-name-threshold
 [`suppress-restriction-lint-in-const`]: https://doc.rust-lang.org/clippy/lint_configuration.html#suppress-restriction-lint-in-const
+[`test-without-fail-case`]: https://doc.rust-lang.org/clippy/lint_configuration.html#test-without-fail-case
 [`too-large-for-stack`]: https://doc.rust-lang.org/clippy/lint_configuration.html#too-large-for-stack
 [`too-many-arguments-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#too-many-arguments-threshold
 [`too-many-lines-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#too-many-lines-threshold

book/src/lint_configuration.md

@@ -831,6 +831,16 @@ if no suggestion can be made.
 * [`indexing_slicing`](https://rust-lang.github.io/rust-clippy/master/index.html#indexing_slicing)
 
 
+## `test-without-fail-case`
+Lint tests to understand whether it can fail or not.
+
+**Default Value:** `{ include_indexing_as_fallible = false, fallible_paths = ["core::panic", "core::assert", "core::assert_eq", "core::assert_ne"], non_fallible_paths = ["std::print", "std::println", "std::dbg", "std::eprint", "std::eprintln"] }`
+
+---
+**Affected lints:**
+* [`test_without_fail_case`](https://rust-lang.github.io/rust-clippy/master/index.html#test_without_fail_case)
+
+
 ## `too-large-for-stack`
 The maximum size of objects (in bytes) that will be linted. Larger objects are ok on the heap
 

clippy_config/src/conf.rs

@@ -1,9 +1,6 @@
 use crate::ClippyConfiguration;
 use crate::msrvs::Msrv;
-use crate::types::{
-    DEFAULT_FALLIBLE_PATHS, DEFAULT_NONFALLIBLE_PATHS, DisallowedPath, MacroMatcher, MatchLintBehaviour,
-    PubUnderscoreFieldsBehaviour, Rename, TestWithoutFailCaseBehaviour,
-};
+use crate::types::{DisallowedPath, MacroMatcher, MatchLintBehaviour, PubUnderscoreFieldsBehaviour, Rename};
 use rustc_errors::Applicability;
 use rustc_session::Session;
 use rustc_span::edit_distance::edit_distance;
@@ -49,6 +46,12 @@ const DEFAULT_ALLOWED_IDENTS_BELOW_MIN_CHARS: &[&str] = &["i", "j", "x", "y", "z
 const DEFAULT_ALLOWED_PREFIXES: &[&str] = &["to", "as", "into", "from", "try_into", "try_from"];
 const DEFAULT_ALLOWED_TRAITS_WITH_RENAMED_PARAMS: &[&str] =
     &["core::convert::From", "core::convert::TryFrom", "core::str::FromStr"];
+/// 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"];
 
 /// Conf with parse errors
 #[derive(Default)]
@@ -631,9 +634,35 @@ 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.
+    /// List of full paths of macros and functions, that can fail. If a test, or a function
+    /// that the test calls contains a call to any one of these, lint will mark the test fallible.
+    ///
+    /// By default this macros are defined as `assert!`, `assert_eq!`, `panic!`.
+    #[lints(test_without_fail_case)]
+    test_without_fail_case_fallible_paths: Vec<String> = Vec::new(),
+    /// 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!`.
+    #[lints(test_without_fail_case)]
+    test_without_fail_case_non_fallible_paths: Vec<String> = Vec::new(),
+    /// Whether to consider indexing as a fallible operation while assesing if a test can fail.
+    /// Indexing is fallible, and thus the a test that is doing that can fail but it is likely
+    /// that tests that fail this way were not intended.
+    ///
+    /// If set true, the lint will 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 interprocedural manner. Meaning that any indexing opeartion done inside of
+    /// a function that the test calls will still result the test getting marked fallible.
+    ///
+    /// By default this is set to `false`. That is because 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.
     #[lints(test_without_fail_case)]
-    test_without_fail_case: TestWithoutFailCaseBehaviour = TestWithoutFailCaseBehaviour::default(),
+    test_without_fail_case_include_indexing_as_fallible: bool = false,
     /// 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,
@@ -731,11 +760,11 @@ 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,
+                &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,
+                &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);

clippy_config/src/types.rs

@@ -130,46 +130,3 @@ 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 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 because 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_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 Default for TestWithoutFailCaseBehaviour {
-    fn default() -> Self {
-        Self {
-            include_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/test_without_fail_case.rs

@@ -58,9 +58,9 @@ impl TestWithoutFailCase {
     pub fn new(conf: &Conf) -> Self {
         Self {
             config: SearchConfig {
-                indexing_fallible: conf.test_without_fail_case.include_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(),
+                indexing_fallible: conf.test_without_fail_case_include_indexing_as_fallible,
+                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(),
             },
         }
     }

tests/ui-toml/test_without_fail_case/indexing_fallible/clippy.toml

@@ -0,0 +1 @@
+test-without-fail-case-include-indexing-as-fallible = false

tests/ui-toml/test_without_fail_case/macro_fallible/clippy.toml

@@ -0,0 +1 @@
+test-without-fail-case-fallible-paths = ["vec"]

tests/ui-toml/test_without_fail_case/test_without_fail_case.rs

@@ -0,0 +1,32 @@
+//@revisions: default fail_macro no_fail_macro index_fail
+//@[default] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/test_without_fail_case/default
+//@[fail_macro] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/test_without_fail_case/macro_fallible
+//@[no_fail_macro] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/test_without_fail_case/macro_non_fallible
+//@[index_fail] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/test_without_fail_case/indexing_fallible
+
+#![allow(unused)]
+#![allow(clippy::tests_outside_test_module)]
+#![allow(clippy::unnecessary_literal_unwrap)]
+#![warn(clippy::test_without_fail_case)]
+
+macro_rules! test {
+    ( $( $x:expr ),* ) => {{
+        let _ = $x;
+        panic!("a");
+    }};
+}
+
+// Should not lint because of the clippy.toml file that adds `test` as fallible.
+#[test]
+fn test_custom_macro_fallible() {
+    test![1];
+}
+
+// Should lint because of the clippy.toml file makes indexing fallible.
+#[test]
+fn test_indexing_fallible() {
+    let a = vec![1, 2, 3];
+    let _ = a[0];
+}
+
+fn main() {}

tests/ui/test_without_fail_case.stderr

@@ -1,17 +1,3 @@
-error: this function marked with `#[test]` cannot fail and will always succeed
-  --> tests/ui/test_without_fail_case.rs:23:1
-   |
-LL | / fn test_without_fail() {
-LL | |     let x = 5;
-LL | |     let y = x + 2;
-LL | |     println!("y: {}", y);
-LL | | }
-   | |_^
-   |
-   = note: consider adding assertions or panics to test failure cases
-   = note: `-D clippy::test-without-fail-case` implied by `-D warnings`
-   = help: to override `-D warnings` add `#[allow(clippy::test_without_fail_case)]`
-
 error: this function marked with `#[test]` cannot fail and will always succeed
   --> tests/ui/test_without_fail_case.rs:78:1
    |
@@ -23,6 +9,8 @@ LL | | }
    | |_^
    |
    = note: consider adding assertions or panics to test failure cases
+   = 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 2 previous errors
+error: aborting due to 1 previous error