Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new unconditional_recursion lint #11938

Merged
merged 4 commits into from
Dec 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5581,6 +5581,7 @@ Released 2018-09-13
[`type_id_on_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_id_on_box
[`type_repetition_in_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_repetition_in_bounds
[`unchecked_duration_subtraction`]: https://rust-lang.github.io/rust-clippy/master/index.html#unchecked_duration_subtraction
[`unconditional_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#unconditional_recursion
[`undocumented_unsafe_blocks`]: https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks
[`undropped_manually_drops`]: https://rust-lang.github.io/rust-clippy/master/index.html#undropped_manually_drops
[`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::types::REDUNDANT_ALLOCATION_INFO,
crate::types::TYPE_COMPLEXITY_INFO,
crate::types::VEC_BOX_INFO,
crate::unconditional_recursion::UNCONDITIONAL_RECURSION_INFO,
crate::undocumented_unsafe_blocks::UNDOCUMENTED_UNSAFE_BLOCKS_INFO,
crate::undocumented_unsafe_blocks::UNNECESSARY_SAFETY_COMMENT_INFO,
crate::unicode::INVISIBLE_CHARACTERS_INFO,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ mod trait_bounds;
mod transmute;
mod tuple_array_conversions;
mod types;
mod unconditional_recursion;
mod undocumented_unsafe_blocks;
mod unicode;
mod uninhabited_references;
Expand Down Expand Up @@ -1075,6 +1076,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
store.register_late_pass(|_| Box::new(repeat_vec_with_capacity::RepeatVecWithCapacity));
store.register_late_pass(|_| Box::new(uninhabited_references::UninhabitedReferences));
store.register_late_pass(|_| Box::new(ineffective_open_options::IneffectiveOpenOptions));
store.register_late_pass(|_| Box::new(unconditional_recursion::UnconditionalRecursion));
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
134 changes: 134 additions & 0 deletions clippy_lints/src/unconditional_recursion.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::{get_trait_def_id, path_res};
use rustc_ast::BinOpKind;
use rustc_hir::def::Res;
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::intravisit::FnKind;
use rustc_hir::{Body, Expr, ExprKind, FnDecl, Item, ItemKind, Node};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::{self, Ty};
use rustc_session::declare_lint_pass;
use rustc_span::{sym, Span};

declare_clippy_lint! {
/// ### What it does
/// Checks that there isn't an infinite recursion in `PartialEq` trait
/// implementation.
///
/// ### Why is this bad?
/// This is a hard to find infinite recursion which will crashing any code
/// using it.
///
/// ### Example
/// ```no_run
/// enum Foo {
/// A,
/// B,
/// }
///
/// impl PartialEq for Foo {
/// fn eq(&self, other: &Self) -> bool {
/// self == other // bad!
/// }
/// }
/// ```
/// Use instead:
///
/// In such cases, either use `#[derive(PartialEq)]` or don't implement it.
#[clippy::version = "1.76.0"]
pub UNCONDITIONAL_RECURSION,
suspicious,
"detect unconditional recursion in some traits implementation"
}

declare_lint_pass!(UnconditionalRecursion => [UNCONDITIONAL_RECURSION]);

fn get_ty_def_id(ty: Ty<'_>) -> Option<DefId> {
match ty.peel_refs().kind() {
ty::Adt(adt, _) => Some(adt.did()),
ty::Foreign(def_id) => Some(*def_id),
_ => None,
}
}

fn is_local(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
matches!(path_res(cx, expr), Res::Local(_))
}

impl<'tcx> LateLintPass<'tcx> for UnconditionalRecursion {
#[allow(clippy::unnecessary_def_path)]
fn check_fn(
&mut self,
cx: &LateContext<'tcx>,
kind: FnKind<'tcx>,
_decl: &'tcx FnDecl<'tcx>,
body: &'tcx Body<'tcx>,
method_span: Span,
def_id: LocalDefId,
) {
// If the function is a method...
if let FnKind::Method(name, _) = kind
// That has two arguments.
&& let [self_arg, other_arg] = cx
.tcx
.instantiate_bound_regions_with_erased(cx.tcx.fn_sig(def_id).skip_binder())
.inputs()
&& let Some(self_arg) = get_ty_def_id(*self_arg)
&& let Some(other_arg) = get_ty_def_id(*other_arg)
// The two arguments are of the same type.
&& self_arg == other_arg
&& let hir_id = cx.tcx.local_def_id_to_hir_id(def_id)
&& let Some((
_,
Node::Item(Item {
kind: ItemKind::Impl(impl_),
owner_id,
..
}),
)) = cx.tcx.hir().parent_iter(hir_id).next()
// We exclude `impl` blocks generated from rustc's proc macros.
&& !cx.tcx.has_attr(*owner_id, sym::automatically_derived)
// It is a implementation of a trait.
&& let Some(trait_) = impl_.of_trait
&& let Some(trait_def_id) = trait_.trait_def_id()
// The trait is `PartialEq`.
&& Some(trait_def_id) == get_trait_def_id(cx, &["core", "cmp", "PartialEq"])
{
let to_check_op = if name.name == sym::eq {
BinOpKind::Eq
} else {
BinOpKind::Ne
};
let expr = body.value.peel_blocks();
let is_bad = match expr.kind {
ExprKind::Binary(op, left, right) if op.node == to_check_op => {
is_local(cx, left) && is_local(cx, right)
},
ExprKind::MethodCall(segment, receiver, &[arg], _) if segment.ident.name == name.name => {
if is_local(cx, receiver)
&& is_local(cx, &arg)
&& let Some(fn_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
&& let Some(trait_id) = cx.tcx.trait_of_item(fn_id)
&& trait_id == trait_def_id
{
true
} else {
false
}
},
_ => false,
};
if is_bad {
span_lint_and_then(
cx,
UNCONDITIONAL_RECURSION,
method_span,
"function cannot return without recursing",
|diag| {
diag.span_note(expr.span, "recursive call site");
},
);
}
}
}
}
163 changes: 163 additions & 0 deletions tests/ui/unconditional_recursion.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
//@no-rustfix

#![warn(clippy::unconditional_recursion)]
#![allow(clippy::partialeq_ne_impl)]

enum Foo {
A,
B,
}

impl PartialEq for Foo {
fn ne(&self, other: &Self) -> bool {
//~^ ERROR: function cannot return without recursing
self != other
}
fn eq(&self, other: &Self) -> bool {
//~^ ERROR: function cannot return without recursing
self == other
}
}

enum Foo2 {
A,
B,
}

impl PartialEq for Foo2 {
fn ne(&self, other: &Self) -> bool {
self != &Foo2::B // no error here
}
fn eq(&self, other: &Self) -> bool {
self == &Foo2::B // no error here
}
}

enum Foo3 {
A,
B,
}

impl PartialEq for Foo3 {
fn ne(&self, other: &Self) -> bool {
//~^ ERROR: function cannot return without recursing
self.ne(other)
}
fn eq(&self, other: &Self) -> bool {
//~^ ERROR: function cannot return without recursing
self.eq(other)
}
}

enum Foo4 {
A,
B,
}

impl PartialEq for Foo4 {
fn ne(&self, other: &Self) -> bool {
self.eq(other) // no error
}
fn eq(&self, other: &Self) -> bool {
self.ne(other) // no error
}
}

enum Foo5 {
A,
B,
}

impl Foo5 {
fn a(&self) -> bool {
true
}
}

impl PartialEq for Foo5 {
fn ne(&self, other: &Self) -> bool {
self.a() // no error
}
fn eq(&self, other: &Self) -> bool {
self.a() // no error
}
}

struct S;

// Check the order doesn't matter.
impl PartialEq for S {
fn ne(&self, other: &Self) -> bool {
//~^ ERROR: function cannot return without recursing
other != self
}
fn eq(&self, other: &Self) -> bool {
//~^ ERROR: function cannot return without recursing
other == self
}
}

struct S2;

// Check that if the same element is compared, it's also triggering the lint.
impl PartialEq for S2 {
fn ne(&self, other: &Self) -> bool {
//~^ ERROR: function cannot return without recursing
other != other
}
fn eq(&self, other: &Self) -> bool {
//~^ ERROR: function cannot return without recursing
other == other
}
}

struct S3;

impl PartialEq for S3 {
fn ne(&self, _other: &Self) -> bool {
//~^ ERROR: function cannot return without recursing
self != self
}
fn eq(&self, _other: &Self) -> bool {
//~^ ERROR: function cannot return without recursing
self == self
}
}

// There should be no warning here!
#[derive(PartialEq)]
enum E {
A,
B,
}

#[derive(PartialEq)]
struct Bar<T: PartialEq>(T);

struct S4;

impl PartialEq for S4 {
fn eq(&self, other: &Self) -> bool {
// No warning here.
Bar(self) == Bar(other)
}
}

macro_rules! impl_partial_eq {
($ty:ident) => {
impl PartialEq for $ty {
fn eq(&self, other: &Self) -> bool {
self == other
}
}
};
}

struct S5;

impl_partial_eq!(S5);
//~^ ERROR: function cannot return without recursing

fn main() {
// test code goes here
}
Loading