From 798cc2fcfc769fca6958416ce9a5853aa23d5aa9 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Sat, 2 Sep 2023 12:59:10 +0200 Subject: [PATCH] Chore: Address PR review <3 Co-authored-by: Veetaha (cx: &'ast AstContext<'ast>, item: ItemKind<'ast>) { 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()); + let body = cx.body(fn_item.body_id().unwrap()); + let res = body.contains_return(cx); cx.emit_lint( TEST_CONTAINS_RETURN, diff --git a/marker_utils/src/lib.rs b/marker_utils/src/lib.rs index 6f6eb7d2..41c1df9c 100644 --- a/marker_utils/src/lib.rs +++ b/marker_utils/src/lib.rs @@ -5,5 +5,4 @@ #![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; diff --git a/marker_utils/src/search.rs b/marker_utils/src/search.rs deleted file mode 100644 index ed5205e7..00000000 --- a/marker_utils/src/search.rs +++ /dev/null @@ -1,26 +0,0 @@ -//! 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() -} diff --git a/marker_utils/src/visitor.rs b/marker_utils/src/visitor.rs index 7a465cad..112e10d8 100644 --- a/marker_utils/src/visitor.rs +++ b/marker_utils/src/visitor.rs @@ -26,7 +26,7 @@ use marker_api::{ /// // The printed `x` is different from the `x` of `foo()` /// // since it comes from the function parameter of `bar()` /// println!("The magic number is {x}"); -/// // The `return` statement only effects the `bar` function. +/// // The `return` statement only affects the `bar` function. /// // `foo()` will just continue executing, when this is called /// return; /// } @@ -35,7 +35,7 @@ use marker_api::{ /// } /// ``` /// -/// The target scope, is checked when the respective `traverse_*` function is called +/// The target scope is checked when the respective `traverse_*` function is called /// For example, [`traverse_body`] will visit a given body [`Body`], but will not enter /// nested bodies, unless [`AllBodies`](VisitorScope::AllBodies) is defined. #[non_exhaustive] @@ -369,15 +369,68 @@ pub fn traverse_expr<'ast, B>( } /// This trait is implemented for nodes, that can be traversed by a [`Visitor`]. -pub trait Traverseable<'ast, B> { +pub trait Traversable<'ast, B> +where + Self: Sized + Copy, +{ /// This calls the `traverse_*` function for the implementing node. fn traverse(self, cx: &'ast AstContext<'ast>, visitor: &mut dyn Visitor) -> ControlFlow; + + /// 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::Traversable; + /// fn count_ifs<'ast>(cx: &'ast AstContext<'ast>, body: &'ast Body<'ast>) -> u32 { + /// let mut count = 0; + /// let _: Option<()> = body.for_each_expr( + /// cx, + /// |expr| { + /// if matches!(expr, ExprKind::If(_)) { + /// count += 1; + /// } + /// ControlFlow::Continue(()) + /// } + /// ); + /// count + /// } + /// ``` + fn for_each_expr FnMut(ExprKind<'a>) -> ControlFlow>( + self, + cx: &'ast AstContext<'ast>, + f: F, + ) -> Option { + struct ExprVisitor { + f: F, + } + impl FnMut(ExprKind<'a>) -> ControlFlow> Visitor for ExprVisitor { + fn visit_expr<'v_ast>( + &mut self, + _cx: &'v_ast AstContext<'v_ast>, + expr: ExprKind<'v_ast>, + ) -> ControlFlow { + (self.f)(expr) + } + } + let mut visitor = ExprVisitor { f }; + + match self.traverse(cx, &mut visitor) { + ControlFlow::Continue(()) => None, + ControlFlow::Break(b) => Some(b), + } + } } -/// This macro implements the [`Traverseable`] trait for a given node that implements `Copy` +/// This macro implements the [`Traversable`] 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 { + impl<'ast, B> Traversable<'ast, B> for $ty { fn traverse(self, cx: &'ast AstContext<'ast>, visitor: &mut dyn Visitor) -> ControlFlow { $func(cx, visitor, self) } @@ -390,49 +443,26 @@ 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>( - cx: &'ast AstContext<'ast>, - node: impl Traverseable<'ast, B>, - f: F, -) -> Option { - struct ExprVisitor { - f: F, - } - impl FnMut(ExprKind<'a>) -> ControlFlow> Visitor for ExprVisitor { - fn visit_expr<'v_ast>(&mut self, _cx: &'v_ast AstContext<'v_ast>, expr: ExprKind<'v_ast>) -> ControlFlow { - (self.f)(expr) - } - } - let mut visitor = ExprVisitor { f }; - - match node.traverse(cx, &mut visitor) { - ControlFlow::Continue(()) => None, - ControlFlow::Break(b) => Some(b), +/// This trait extends the [`Traversable`] trait with more functions, specific to +/// the `bool` return type. +pub trait BoolTraversable<'ast>: Traversable<'ast, bool> { + /// 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. + fn contains_return(&self, cx: &'ast AstContext<'ast>) -> bool { + self.for_each_expr(cx, |expr| { + if matches!(expr, ExprKind::Return(_) | ExprKind::QuestionMark(_)) { + ControlFlow::Break(true) + } else { + ControlFlow::Continue(()) + } + }) + .is_some() } } + +impl<'ast, T: Traversable<'ast, bool>> BoolTraversable<'ast> for T {}