Utils: Add for_each_expr and contains_return utils

marker_api/src/prelude.rs

@@ -10,6 +10,7 @@ pub use crate::ast::ty::SynTyData;
 
 // Common types
 pub use crate::ast::expr::ExprKind;
+pub use crate::ast::item::Body;
 pub use crate::ast::item::ItemKind;
 pub use crate::ast::pat::PatKind;
 pub use crate::ast::stmt::StmtKind;

marker_uilints/Cargo.toml

@@ -15,6 +15,7 @@ crate-type = ["cdylib"]
 
 [dependencies]
 marker_api = { workspace = true }
+marker_utils = { workspace = true }
 
 [dev-dependencies]
 marker_uitest = { workspace = true }

marker_uilints/src/lib.rs

@@ -1,6 +1,8 @@
 #![doc = include_str!("../README.md")]
 #![warn(clippy::pedantic)]
 
+mod utils;
+
 use marker_api::{
     ast::{
         item::{EnumVariant, Field, StaticItem},
@@ -66,10 +68,18 @@ fn emit_item_with_test_name_lint<'ast>(
 
 impl LintPass for TestLintPass {
     fn info(&self) -> LintPassInfo {
-        LintPassInfoBuilder::new(Box::new([TEST_LINT, ITEM_WITH_TEST_NAME, PRINT_EVERY_EXPR])).build()
+        LintPassInfoBuilder::new(Box::new([
+            TEST_LINT,
+            ITEM_WITH_TEST_NAME,
+            PRINT_EVERY_EXPR,
+            utils::TEST_CONTAINS_RETURN,
+        ]))
+        .build()
     }
 
     fn check_item<'ast>(&mut self, cx: &'ast AstContext<'ast>, item: ItemKind<'ast>) {
+        utils::check_item(cx, item);
+
         if let ItemKind::Fn(item) = item {
             if let Some(ident) = item.ident() {
                 if ident.name() == "test_ty_id_resolution_trigger" {

marker_uilints/src/utils.rs

@@ -0,0 +1,26 @@
+use marker_api::prelude::*;
+use marker_utils::search::contains_return;
+
+marker_api::declare_lint! {
+    /// # What it does
+    /// Tests the [`marker_utils::search::contains_return`] function.
+    TEST_CONTAINS_RETURN,
+    Warn,
+}
+
+pub fn check_item<'ast>(cx: &'ast AstContext<'ast>, item: ItemKind<'ast>) {
+    let ItemKind::Fn(fn_item) = item else { return };
+    let Some(ident) = fn_item.ident() else { return };
+
+    if ident.name().starts_with("test_contains_return") {
+        let res = contains_return(cx, cx.body(fn_item.body_id().unwrap()).expr());
+
+        cx.emit_lint(
+            TEST_CONTAINS_RETURN,
+            item.id(),
+            format!("testing `contains_return` -> {res}"),
+            ident.span(),
+            |_| {},
+        )
+    }
+}

marker_uilints/tests/ui/utils/contains_return.rs

@@ -0,0 +1,32 @@
+#![allow(unused)]
+
+fn test_contains_return_1_false() {
+    // False, doesn't contain anything
+}
+
+fn test_contains_return_2_false() {
+    if (16 << 2) < 2 {
+        println!("IDK");
+    } else {
+        println!("idk");
+    }
+}
+
+fn test_contains_return_3_true() {
+    // True, is this code useful? Nope, but it contains a return
+    return;
+}
+
+fn test_contains_return_4_true() -> Option<u32> {
+    // True, is this code useful? Still nope, somehow it's worse
+    let x: u32 = Some(2)?;
+    Some(x)
+}
+
+fn test_contains_return_5_false() {
+    // False, the return is nested
+    fn nested_function() {
+        return;
+    }
+    nested_function();
+}

marker_uilints/tests/ui/utils/contains_return.stderr

@@ -0,0 +1,34 @@
+warning: testing `contains_return` -> false
+ --> $DIR/contains_return.rs:3:4
+  |
+3 | fn test_contains_return_1_false() {
+  |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+  |
+  = note: `#[warn(marker::test_contains_return)]` on by default
+
+warning: testing `contains_return` -> false
+ --> $DIR/contains_return.rs:7:4
+  |
+7 | fn test_contains_return_2_false() {
+  |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+warning: testing `contains_return` -> true
+  --> $DIR/contains_return.rs:15:4
+   |
+15 | fn test_contains_return_3_true() {
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+warning: testing `contains_return` -> true
+  --> $DIR/contains_return.rs:20:4
+   |
+20 | fn test_contains_return_4_true() -> Option<u32> {
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+warning: testing `contains_return` -> false
+  --> $DIR/contains_return.rs:26:4
+   |
+26 | fn test_contains_return_5_false() {
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+warning: 5 warnings emitted
+

marker_utils/src/lib.rs

@@ -5,4 +5,5 @@
 #![allow(clippy::unused_self)] // `self` is needed to potentualy change the behavior later
 #![allow(clippy::trivially_copy_pass_by_ref)] // Needed to potentualy change the behavior later
 
+pub mod search;
 pub mod visitor;

marker_utils/src/search.rs

@@ -0,0 +1,26 @@
+//! This module contains utilities to search a given part of the AST for a
+//! different things.
+
+use std::ops::ControlFlow;
+
+use crate::visitor::{for_each_expr, Traverseable};
+
+use marker_api::prelude::*;
+
+/// Checks if the given node contains an early return, in the form of an
+/// [`ReturnExpr`](marker_api::ast::expr::ReturnExpr) or
+/// [`QuestionMarkExpr`](marker_api::ast::expr::QuestionMarkExpr).
+///
+/// This function is useful, for lints which suggest moving code snippets into
+/// a closure or different function. Return statements might prevent the suggested
+/// refactoring.
+pub fn contains_return<'ast>(cx: &'ast AstContext<'ast>, node: impl Traverseable<'ast, bool>) -> bool {
+    for_each_expr(cx, node, |expr| {
+        if matches!(expr, ExprKind::Return(_) | ExprKind::QuestionMark(_)) {
+            ControlFlow::Break(true)
+        } else {
+            ControlFlow::Continue(())
+        }
+    })
+    .is_some()
+}

marker_utils/src/visitor.rs

@@ -183,10 +183,6 @@ pub fn traverse_body<'ast, B>(
     visitor: &mut dyn Visitor<B>,
     body: &'ast Body<'ast>,
 ) -> ControlFlow<B> {
-    if let VisitorScope::NoBodies = visitor.scope() {
-        return ControlFlow::Continue(());
-    }
-
     visitor.visit_body(cx, body)?;
 
     traverse_expr(cx, visitor, body.expr())?;
@@ -373,3 +369,72 @@ pub fn traverse_expr<'ast, B>(
 
     ControlFlow::Continue(())
 }
+
+/// This trait is implemented for nodes, that can be traversed by a [`Visitor`].
+pub trait Traverseable<'ast, B> {
+    /// This calls the `traverse_*` function for the implementing node.
+    fn traverse(self, cx: &'ast AstContext<'ast>, visitor: &mut dyn Visitor<B>) -> ControlFlow<B>;
+}
+
+/// This macro implements the [`Traverseable`] trait for a given node that implements `Copy`
+macro_rules! impl_traversable_for {
+    ($ty:ty, $func:ident) => {
+        impl<'ast, B> Traverseable<'ast, B> for $ty {
+            fn traverse(self, cx: &'ast AstContext<'ast>, visitor: &mut dyn Visitor<B>) -> ControlFlow<B> {
+                $func(cx, visitor, self)
+            }
+        }
+    };
+}
+
+impl_traversable_for!(ExprKind<'ast>, traverse_expr);
+impl_traversable_for!(StmtKind<'ast>, traverse_stmt);
+impl_traversable_for!(ItemKind<'ast>, traverse_item);
+impl_traversable_for!(&'ast Body<'ast>, traverse_body);
+
+/// This function calls the given closure for every expression in the node. This
+/// traversal will not enter any nested bodies.
+///
+/// This function is a simple wrapper around the [`Visitor`] trait, that is good
+/// for most use cases. For example, the following code counts the number of `if`s
+/// in a body:
+///
+/// ```
+/// # use marker_api::prelude::*;
+/// # use std::ops::ControlFlow;
+/// # use marker_utils::visitor::for_each_expr;
+/// fn count_ifs<'ast>(cx: &'ast AstContext<'ast>, body: &'ast Body<'ast>) -> u32 {
+///     let mut count = 0;
+///     let _: Option<()> = for_each_expr(
+///         cx,
+///         body,
+///         |expr| {
+///             if matches!(expr, ExprKind::If(_)) {
+///                 count += 1;
+///             }
+///             ControlFlow::Continue(())
+///         }
+///     );
+///     count
+/// }
+/// ```
+pub fn for_each_expr<'ast, B, F: for<'a> FnMut(ExprKind<'a>) -> ControlFlow<B>>(
+    cx: &'ast AstContext<'ast>,
+    node: impl Traverseable<'ast, B>,
+    f: F,
+) -> Option<B> {
+    struct ExprVisitor<F> {
+        f: F,
+    }
+    impl<B, F: for<'a> FnMut(ExprKind<'a>) -> ControlFlow<B>> Visitor<B> for ExprVisitor<F> {
+        fn visit_expr<'v_ast>(&mut self, _cx: &'v_ast AstContext<'v_ast>, expr: ExprKind<'v_ast>) -> ControlFlow<B> {
+            (self.f)(expr)
+        }
+    }
+    let mut visitor = ExprVisitor { f };
+
+    match node.traverse(cx, &mut visitor) {
+        ControlFlow::Continue(_) => None,
+        ControlFlow::Break(b) => Some(b),
+    }
+}