mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #12459 - y21:unconditional_recursion_from_into, r=Jarcho

Browse Source 
lint when calling the blanket `Into` impl from a `From` impl

Closes #11150
```
warning: function cannot return without recursing
  --> x.rs:9:9
   |
9  | /         fn from(value: f32) -> Self {
10 | |             value.into()
11 | |         }
   | |_________^
   |
note: recursive call site
  --> x.rs:10:13
   |
10 |             value.into()
   |             ^^^^^^^^^^^^
```

I'm also thinking that we can probably generalize this lint to #11032 at some point (instead of hardcoding a bunch of impls), like how rustc's `unconditional_recursion` works, at least up to one indirect call, but this still seems useful for now :)

I've also noticed that we use `fn_def_id` in a bunch of lints and then try to get the node args of the call in a separate step, so I made a helper function that does both in one. I intend to refactor a bunch of uses of `fn_def_id` to use this later

I can add more test cases, but this is already using much of the same logic that exists for the other impls that this lint looks for (e.g. making sure that there are no conditional returns).

changelog: [`unconditional_recursion`]: emit a warning inside of `From::from` when unconditionally calling the blanket `.into()` impl
This commit is contained in:
bors 2024-03-13 16:32:25 +00:00
commit 73be4863f0
4 changed files with 174 additions and 85 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

54
clippy_lints/src/unconditional_recursion.rs
View File

@ -1,5 +1,5 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::{expr_or_init, get_trait_def_id, path_def_id};
use clippy_utils::{expr_or_init, fn_def_id_with_node_args, path_def_id};
use rustc_ast::BinOpKind;
use rustc_data_structures::fx::FxHashMap;
use rustc_hir as hir;
@ -19,11 +19,11 @@
declare_clippy_lint! {
/// ### What it does
/// Checks that there isn't an infinite recursion in `PartialEq` trait
/// implementation.
/// Checks that there isn't an infinite recursion in trait
/// implementations.
///
/// ### Why is this bad?
/// This is a hard to find infinite recursion which will crashing any code
/// This is a hard to find infinite recursion that will crash any code.
/// using it.
///
/// ### Example
@ -201,7 +201,6 @@ fn check_partial_eq(cx: &LateContext<'_>, method_span: Span, method_def_id: Loca
}
}
#[allow(clippy::unnecessary_def_path)]
fn check_to_string(cx: &LateContext<'_>, method_span: Span, method_def_id: LocalDefId, name: Ident, expr: &Expr<'_>) {
let args = cx
.tcx
@ -224,7 +223,7 @@ fn check_to_string(cx: &LateContext<'_>, method_span: Span, method_def_id: Local
&& let Some(trait_) = impl_.of_trait
&& let Some(trait_def_id) = trait_.trait_def_id()
// The trait is `ToString`.
&& Some(trait_def_id) == get_trait_def_id(cx, &["alloc", "string", "ToString"])
&& cx.tcx.is_diagnostic_item(sym::ToString, trait_def_id)
{
let is_bad = match expr.kind {
ExprKind::MethodCall(segment, _receiver, &[_arg], _) if segment.ident.name == name.name => {
@ -291,7 +290,6 @@ fn nested_visit_map(&mut self) -> Self::Map {
self.map
}
#[allow(clippy::unnecessary_def_path)]
fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
if self.found_default_call {
return;
@ -303,7 +301,7 @@ fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
&& is_default_method_on_current_ty(self.cx.tcx, qpath, self.implemented_ty_id)
&& let Some(method_def_id) = path_def_id(self.cx, f)
&& let Some(trait_def_id) = self.cx.tcx.trait_of_item(method_def_id)
&& Some(trait_def_id) == get_trait_def_id(self.cx, &["core", "default", "Default"])
&& self.cx.tcx.is_diagnostic_item(sym::Default, trait_def_id)
{
self.found_default_call = true;
span_error(self.cx, self.method_span, expr);
@ -312,10 +310,9 @@ fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
}
impl UnconditionalRecursion {
#[allow(clippy::unnecessary_def_path)]
fn init_default_impl_for_type_if_needed(&mut self, cx: &LateContext<'_>) {
if self.default_impl_for_type.is_empty()
&& let Some(default_trait_id) = get_trait_def_id(cx, &["core", "default", "Default"])
&& let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
{
let impls = cx.tcx.trait_impls_of(default_trait_id);
for (ty, impl_def_ids) in impls.non_blanket_impls() {
@ -394,6 +391,34 @@ fn check_default_new<'tcx>(
}
}
fn check_from(cx: &LateContext<'_>, method_span: Span, method_def_id: LocalDefId, expr: &Expr<'_>) {
let Some(sig) = cx
.typeck_results()
.liberated_fn_sigs()
.get(cx.tcx.local_def_id_to_hir_id(method_def_id))
else {
return;
};
// Check if we are calling `Into::into` where the node args match with our `From::from` signature:
// From::from signature: fn(S1) -> S2
// <S1 as Into<S2>>::into(s1), node_args=[S1, S2]
// If they do match, then it must mean that it is the blanket impl,
// which calls back into our `From::from` again (`Into` is not specializable).
// rustc's unconditional_recursion already catches calling `From::from` directly
if let Some((fn_def_id, node_args)) = fn_def_id_with_node_args(cx, expr)
&& let [s1, s2] = **node_args
&& let (Some(s1), Some(s2)) = (s1.as_type(), s2.as_type())
&& let Some(trait_def_id) = cx.tcx.trait_of_item(fn_def_id)
&& cx.tcx.is_diagnostic_item(sym::Into, trait_def_id)
&& get_impl_trait_def_id(cx, method_def_id) == cx.tcx.get_diagnostic_item(sym::From)
&& s1 == sig.inputs()[0]
&& s2 == sig.output()
{
span_error(cx, method_span, expr);
}
}
impl<'tcx> LateLintPass<'tcx> for UnconditionalRecursion {
fn check_fn(
&mut self,
@ -410,10 +435,11 @@ fn check_fn(
// Doesn't have a conditional return.
&& !has_conditional_return(body, expr)
{
if name.name == sym::eq || name.name == sym::ne {
check_partial_eq(cx, method_span, method_def_id, name, expr);
} else if name.name == sym::to_string {
check_to_string(cx, method_span, method_def_id, name, expr);
match name.name {
sym::eq | sym::ne => check_partial_eq(cx, method_span, method_def_id, name, expr),
sym::to_string => check_to_string(cx, method_span, method_def_id, name, expr),
sym::from => check_from(cx, method_span, method_def_id, expr),
_ => {},
}
self.check_default_new(cx, decl, body, method_span, method_def_id);
}

19
clippy_utils/src/lib.rs
View File

@ -2249,8 +2249,21 @@ pub fn fn_has_unsatisfiable_preds(cx: &LateContext<'_>, did: DefId) -> bool {
/// Returns the `DefId` of the callee if the given expression is a function or method call.
pub fn fn_def_id(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<DefId> {
fn_def_id_with_node_args(cx, expr).map(|(did, _)| did)
}
/// Returns the `DefId` of the callee if the given expression is a function or method call,
/// as well as its node args.
pub fn fn_def_id_with_node_args<'tcx>(
cx: &LateContext<'tcx>,
expr: &Expr<'_>,
) -> Option<(DefId, rustc_ty::GenericArgsRef<'tcx>)> {
let typeck = cx.typeck_results();
match &expr.kind {
ExprKind::MethodCall(..) => cx.typeck_results().type_dependent_def_id(expr.hir_id),
ExprKind::MethodCall(..) => Some((
typeck.type_dependent_def_id(expr.hir_id)?,
typeck.node_args(expr.hir_id),
)),
ExprKind::Call(
Expr {
kind: ExprKind::Path(qpath),
@ -2262,9 +2275,9 @@ pub fn fn_def_id(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<DefId> {
// Only return Fn-like DefIds, not the DefIds of statics/consts/etc that contain or
// deref to fn pointers, dyn Fn, impl Fn - #8850
if let Res::Def(DefKind::Fn | DefKind::Ctor(..) | DefKind::AssocFn, id) =
cx.typeck_results().qpath_res(qpath, *path_hir_id)
typeck.qpath_res(qpath, *path_hir_id)
{
Some(id)
Some((id, typeck.node_args(*path_hir_id)))
} else {
None
}

50
tests/ui/unconditional_recursion.rs
View File

@ -1,7 +1,11 @@
//@no-rustfix
#![warn(clippy::unconditional_recursion)]
#![allow(clippy::partialeq_ne_impl, clippy::default_constructed_unit_structs)]
#![allow(
clippy::partialeq_ne_impl,
clippy::default_constructed_unit_structs,
clippy::only_used_in_recursion
)]
enum Foo {
A,
@ -350,4 +354,48 @@ fn eq(&self, other: &T) -> bool {
}
}
// From::from -> Into::into -> From::from
struct BadFromTy1<'a>(&'a ());
struct BadIntoTy1<'b>(&'b ());
impl<'a> From<BadFromTy1<'a>> for BadIntoTy1<'static> {
fn from(f: BadFromTy1<'a>) -> Self {
f.into()
}
}
// Using UFCS syntax
struct BadFromTy2<'a>(&'a ());
struct BadIntoTy2<'b>(&'b ());
impl<'a> From<BadFromTy2<'a>> for BadIntoTy2<'static> {
fn from(f: BadFromTy2<'a>) -> Self {
Into::into(f)
}
}
// Different Into impl (<i16 as Into<i32>>), so no infinite recursion
struct BadFromTy3;
impl From<BadFromTy3> for i32 {
fn from(f: BadFromTy3) -> Self {
Into::into(1i16)
}
}
// A conditional return that ends the recursion
struct BadFromTy4;
impl From<BadFromTy4> for i32 {
fn from(f: BadFromTy4) -> Self {
if true {
return 42;
}
f.into()
}
}
// Types differ in refs, don't lint
impl From<&BadFromTy4> for i32 {
fn from(f: &BadFromTy4) -> Self {
BadFromTy4.into()
}
}
fn main() {}

136
tests/ui/unconditional_recursion.stderr
View File

@ -1,5 +1,5 @@
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:42:5
--> tests/ui/unconditional_recursion.rs:46:5
|
LL | fn ne(&self, other: &Self) -> bool {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
@ -12,7 +12,7 @@ LL | self.ne(other)
= help: to override `-D warnings` add `#[allow(unconditional_recursion)]`
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:46:5
--> tests/ui/unconditional_recursion.rs:50:5
|
LL | fn eq(&self, other: &Self) -> bool {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
@ -23,7 +23,7 @@ LL | self.eq(other)
= help: a `loop` may express intention better if this is on purpose
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:211:5
--> tests/ui/unconditional_recursion.rs:215:5
|
LL | fn to_string(&self) -> String {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
@ -34,7 +34,7 @@ LL | self.to_string()
= help: a `loop` may express intention better if this is on purpose
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:221:5
--> tests/ui/unconditional_recursion.rs:225:5
|
LL | fn to_string(&self) -> String {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
@ -45,7 +45,7 @@ LL | x.to_string()
= help: a `loop` may express intention better if this is on purpose
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:232:5
--> tests/ui/unconditional_recursion.rs:236:5
|
LL | fn to_string(&self) -> String {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
@ -56,7 +56,7 @@ LL | (self as &Self).to_string()
= help: a `loop` may express intention better if this is on purpose
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:12:5
--> tests/ui/unconditional_recursion.rs:16:5
|
LL | / fn ne(&self, other: &Self) -> bool {
LL | |
@ -65,7 +65,7 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:14:9
--> tests/ui/unconditional_recursion.rs:18:9
|
LL | self != other
| ^^^^^^^^^^^^^
@ -73,7 +73,7 @@ LL | self != other
= help: to override `-D warnings` add `#[allow(clippy::unconditional_recursion)]`
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:16:5
--> tests/ui/unconditional_recursion.rs:20:5
|
LL | / fn eq(&self, other: &Self) -> bool {
LL | |
@ -82,13 +82,13 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:18:9
--> tests/ui/unconditional_recursion.rs:22:9
|
LL | self == other
| ^^^^^^^^^^^^^
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:28:5
--> tests/ui/unconditional_recursion.rs:32:5
|
LL | / fn ne(&self, other: &Self) -> bool {
LL | | self != &Foo2::B // no error here
@ -96,13 +96,13 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:29:9
--> tests/ui/unconditional_recursion.rs:33:9
|
LL | self != &Foo2::B // no error here
| ^^^^^^^^^^^^^^^^
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:31:5
--> tests/ui/unconditional_recursion.rs:35:5
|
LL | / fn eq(&self, other: &Self) -> bool {
LL | | self == &Foo2::B // no error here
@ -110,13 +110,13 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:32:9
--> tests/ui/unconditional_recursion.rs:36:9
|
LL | self == &Foo2::B // no error here
| ^^^^^^^^^^^^^^^^
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:42:5
--> tests/ui/unconditional_recursion.rs:46:5
|
LL | / fn ne(&self, other: &Self) -> bool {
LL | |
@ -125,27 +125,13 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:44:9
--> tests/ui/unconditional_recursion.rs:48:9
|
LL | self.ne(other)
| ^^^^^^^^^^^^^^
error: parameter is only used in recursion
--> tests/ui/unconditional_recursion.rs:42:18
|
LL | fn ne(&self, other: &Self) -> bool {
| ^^^^^ help: if this is intentional, prefix it with an underscore: `_other`
|
note: parameter used here
--> tests/ui/unconditional_recursion.rs:44:17
|
LL | self.ne(other)
| ^^^^^
= note: `-D clippy::only-used-in-recursion` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::only_used_in_recursion)]`
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:46:5
--> tests/ui/unconditional_recursion.rs:50:5
|
LL | / fn eq(&self, other: &Self) -> bool {
LL | |
@ -154,25 +140,13 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:48:9
--> tests/ui/unconditional_recursion.rs:52:9
|
LL | self.eq(other)
| ^^^^^^^^^^^^^^
error: parameter is only used in recursion
--> tests/ui/unconditional_recursion.rs:46:18
|
LL | fn eq(&self, other: &Self) -> bool {
| ^^^^^ help: if this is intentional, prefix it with an underscore: `_other`
|
note: parameter used here
--> tests/ui/unconditional_recursion.rs:48:17
|
LL | self.eq(other)
| ^^^^^
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:90:5
--> tests/ui/unconditional_recursion.rs:94:5
|
LL | / fn ne(&self, other: &Self) -> bool {
LL | |
@ -181,13 +155,13 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:92:9
--> tests/ui/unconditional_recursion.rs:96:9
|
LL | other != self
| ^^^^^^^^^^^^^
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:94:5
--> tests/ui/unconditional_recursion.rs:98:5
|
LL | / fn eq(&self, other: &Self) -> bool {
LL | |
@ -196,13 +170,13 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:96:9
--> tests/ui/unconditional_recursion.rs:100:9
|
LL | other == self
| ^^^^^^^^^^^^^
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:104:5
--> tests/ui/unconditional_recursion.rs:108:5
|
LL | / fn ne(&self, other: &Self) -> bool {
LL | |
@ -211,13 +185,13 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:106:9
--> tests/ui/unconditional_recursion.rs:110:9
|
LL | other != other
| ^^^^^^^^^^^^^^
error: equal expressions as operands to `!=`
--> tests/ui/unconditional_recursion.rs:106:9
--> tests/ui/unconditional_recursion.rs:110:9
|
LL | other != other
| ^^^^^^^^^^^^^^
@ -225,7 +199,7 @@ LL | other != other
= note: `#[deny(clippy::eq_op)]` on by default
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:108:5
--> tests/ui/unconditional_recursion.rs:112:5
|
LL | / fn eq(&self, other: &Self) -> bool {
LL | |
@ -234,19 +208,19 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:110:9
--> tests/ui/unconditional_recursion.rs:114:9
|
LL | other == other
| ^^^^^^^^^^^^^^
error: equal expressions as operands to `==`
--> tests/ui/unconditional_recursion.rs:110:9
--> tests/ui/unconditional_recursion.rs:114:9
|
LL | other == other
| ^^^^^^^^^^^^^^
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:117:5
--> tests/ui/unconditional_recursion.rs:121:5
|
LL | / fn ne(&self, _other: &Self) -> bool {
LL | |
@ -255,19 +229,19 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:119:9
--> tests/ui/unconditional_recursion.rs:123:9
|
LL | self != self
| ^^^^^^^^^^^^
error: equal expressions as operands to `!=`
--> tests/ui/unconditional_recursion.rs:119:9
--> tests/ui/unconditional_recursion.rs:123:9
|
LL | self != self
| ^^^^^^^^^^^^
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:121:5
--> tests/ui/unconditional_recursion.rs:125:5
|
LL | / fn eq(&self, _other: &Self) -> bool {
LL | |
@ -276,19 +250,19 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:123:9
--> tests/ui/unconditional_recursion.rs:127:9
|
LL | self == self
| ^^^^^^^^^^^^
error: equal expressions as operands to `==`
--> tests/ui/unconditional_recursion.rs:123:9
--> tests/ui/unconditional_recursion.rs:127:9
|
LL | self == self
| ^^^^^^^^^^^^
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:149:13
--> tests/ui/unconditional_recursion.rs:153:13
|
LL | / fn eq(&self, other: &Self) -> bool {
LL | |
@ -300,7 +274,7 @@ LL | impl_partial_eq!(S5);
| -------------------- in this macro invocation
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:151:17
--> tests/ui/unconditional_recursion.rs:155:17
|
LL | self == other
| ^^^^^^^^^^^^^
@ -310,7 +284,7 @@ LL | impl_partial_eq!(S5);
= note: this error originates in the macro `impl_partial_eq` (in Nightly builds, run with -Z macro-backtrace for more info)
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:178:5
--> tests/ui/unconditional_recursion.rs:182:5
|
LL | / fn eq(&self, other: &Self) -> bool {
LL | |
@ -321,13 +295,13 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:182:9
--> tests/ui/unconditional_recursion.rs:186:9
|
LL | mine == theirs
| ^^^^^^^^^^^^^^
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:247:5
--> tests/ui/unconditional_recursion.rs:251:5
|
LL | / fn new() -> Self {
LL | |
@ -336,13 +310,13 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:249:9
--> tests/ui/unconditional_recursion.rs:253:9
|
LL | Self::default()
| ^^^^^^^^^^^^^^^
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:286:5
--> tests/ui/unconditional_recursion.rs:290:5
|
LL | / fn eq(&self, other: &Self) -> bool {
LL | |
@ -353,10 +327,38 @@ LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:290:9
--> tests/ui/unconditional_recursion.rs:294:9
|
LL | mine.eq(theirs)
| ^^^^^^^^^^^^^^^
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:361:5
|
LL | / fn from(f: BadFromTy1<'a>) -> Self {
LL | | f.into()
LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:362:9
|
LL | f.into()
| ^^^^^^^^
error: function cannot return without recursing
--> tests/ui/unconditional_recursion.rs:370:5
|
LL | / fn from(f: BadFromTy2<'a>) -> Self {
LL | | Into::into(f)
LL | | }
| |_____^
|
note: recursive call site
--> tests/ui/unconditional_recursion.rs:371:9
|
LL | Into::into(f)
| ^^^^^^^^^^^^^
error: aborting due to 27 previous errors