diff options
| -rw-r--r-- | clippy_lints/src/misc.rs | 79 | ||||
| -rw-r--r-- | tests/ui/cmp_owned/asymmetric_partial_eq.fixed | 93 | ||||
| -rw-r--r-- | tests/ui/cmp_owned/asymmetric_partial_eq.rs | 93 | ||||
| -rw-r--r-- | tests/ui/cmp_owned/asymmetric_partial_eq.stderr | 46 | ||||
| -rw-r--r-- | tests/ui/cmp_owned/issue_4874.rs | 51 |
5 files changed, 282 insertions, 80 deletions
diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 1b65a01690d..76ffe6f6a1c 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -371,8 +371,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MiscLints { if op.is_comparison() { check_nan(cx, left, expr); check_nan(cx, right, expr); - check_to_owned(cx, left, right); - check_to_owned(cx, right, left); + check_to_owned(cx, left, right, true); + check_to_owned(cx, right, left, false); } if (op == BinOpKind::Eq || op == BinOpKind::Ne) && (is_float(cx, left) || is_float(cx, right)) { if is_allowed(cx, left) || is_allowed(cx, right) { @@ -570,20 +570,30 @@ fn is_array(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool { matches!(&walk_ptrs_ty(cx.tables.expr_ty(expr)).kind, ty::Array(_, _)) } -fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr<'_>, other: &Expr<'_>) { - fn symmetric_partial_eq<'tcx>(cx: &LateContext<'_, 'tcx>, lhs: Ty<'tcx>, rhs: Ty<'tcx>) -> bool { - if let Some(trait_def_id) = cx.tcx.lang_items().eq_trait() { - return implements_trait(cx, lhs, trait_def_id, &[rhs.into()]) - && implements_trait(cx, rhs, trait_def_id, &[lhs.into()]); +fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr<'_>, other: &Expr<'_>, left: bool) { + #[derive(Default)] + struct EqImpl { + ty_eq_other: bool, + other_eq_ty: bool, + } + + impl EqImpl { + fn is_implemented(&self) -> bool { + self.ty_eq_other || self.other_eq_ty } + } - false + fn symmetric_partial_eq<'tcx>(cx: &LateContext<'_, 'tcx>, ty: Ty<'tcx>, other: Ty<'tcx>) -> Option<EqImpl> { + cx.tcx.lang_items().eq_trait().map(|def_id| EqImpl { + ty_eq_other: implements_trait(cx, ty, def_id, &[other.into()]), + other_eq_ty: implements_trait(cx, other, def_id, &[ty.into()]), + }) } let (arg_ty, snip) = match expr.kind { ExprKind::MethodCall(.., ref args, _) if args.len() == 1 => { if match_trait_method(cx, expr, &paths::TO_STRING) || match_trait_method(cx, expr, &paths::TO_OWNED) { - (cx.tables.expr_ty_adjusted(&args[0]), snippet(cx, args[0].span, "..")) + (cx.tables.expr_ty(&args[0]), snippet(cx, args[0].span, "..")) } else { return; } @@ -591,7 +601,7 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr<'_>, other: &Expr<'_>) { ExprKind::Call(ref path, ref v) if v.len() == 1 => { if let ExprKind::Path(ref path) = path.kind { if match_qpath(path, &["String", "from_str"]) || match_qpath(path, &["String", "from"]) { - (cx.tables.expr_ty_adjusted(&v[0]), snippet(cx, v[0].span, "..")) + (cx.tables.expr_ty(&v[0]), snippet(cx, v[0].span, "..")) } else { return; } @@ -602,24 +612,19 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr<'_>, other: &Expr<'_>) { _ => return, }; - let other_ty = cx.tables.expr_ty_adjusted(other); + let other_ty = cx.tables.expr_ty(other); - let deref_arg_impl_partial_eq_other = arg_ty - .builtin_deref(true) - .map_or(false, |tam| symmetric_partial_eq(cx, tam.ty, other_ty)); - let arg_impl_partial_eq_deref_other = other_ty + let without_deref = symmetric_partial_eq(cx, arg_ty, other_ty).unwrap_or_default(); + let with_deref = arg_ty .builtin_deref(true) - .map_or(false, |tam| symmetric_partial_eq(cx, arg_ty, tam.ty)); - let arg_impl_partial_eq_other = symmetric_partial_eq(cx, arg_ty, other_ty); + .and_then(|tam| symmetric_partial_eq(cx, tam.ty, other_ty)) + .unwrap_or_default(); - if !deref_arg_impl_partial_eq_other && !arg_impl_partial_eq_deref_other && !arg_impl_partial_eq_other { + if !with_deref.is_implemented() && !without_deref.is_implemented() { return; } - let other_gets_derefed = match other.kind { - ExprKind::Unary(UnOp::UnDeref, _) => true, - _ => false, - }; + let other_gets_derefed = matches!(other.kind, ExprKind::Unary(UnOp::UnDeref, _)); let lint_span = if other_gets_derefed { expr.span.to(other.span) @@ -639,18 +644,34 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr<'_>, other: &Expr<'_>) { return; } - let try_hint = if deref_arg_impl_partial_eq_other { - // suggest deref on the left - format!("*{}", snip) + let expr_snip; + let eq_impl; + if with_deref.is_implemented() { + expr_snip = format!("*{}", snip); + eq_impl = with_deref; } else { - // suggest dropping the to_owned on the left - snip.to_string() + expr_snip = snip.to_string(); + eq_impl = without_deref; }; + let span; + let hint; + if (eq_impl.ty_eq_other && left) || (eq_impl.other_eq_ty && !left) { + span = expr.span; + hint = expr_snip; + } else { + span = expr.span.to(other.span); + if eq_impl.ty_eq_other { + hint = format!("{} == {}", expr_snip, snippet(cx, other.span, "..")); + } else { + hint = format!("{} == {}", snippet(cx, other.span, ".."), expr_snip); + } + } + diag.span_suggestion( - lint_span, + span, "try", - try_hint, + hint, Applicability::MachineApplicable, // snippet ); }, diff --git a/tests/ui/cmp_owned/asymmetric_partial_eq.fixed b/tests/ui/cmp_owned/asymmetric_partial_eq.fixed new file mode 100644 index 00000000000..3305ac9bf8b --- /dev/null +++ b/tests/ui/cmp_owned/asymmetric_partial_eq.fixed @@ -0,0 +1,93 @@ +// run-rustfix +#![allow(unused, clippy::redundant_clone)] // See #5700 + +// Define the types in each module to avoid trait impls leaking between modules. +macro_rules! impl_types { + () => { + #[derive(PartialEq)] + pub struct Owned; + + pub struct Borrowed; + + impl ToOwned for Borrowed { + type Owned = Owned; + fn to_owned(&self) -> Owned { + Owned {} + } + } + + impl std::borrow::Borrow<Borrowed> for Owned { + fn borrow(&self) -> &Borrowed { + static VALUE: Borrowed = Borrowed {}; + &VALUE + } + } + }; +} + +// Only Borrowed == Owned is implemented +mod borrowed_eq_owned { + impl_types!(); + + impl PartialEq<Owned> for Borrowed { + fn eq(&self, _: &Owned) -> bool { + true + } + } + + pub fn compare() { + let owned = Owned {}; + let borrowed = Borrowed {}; + + if borrowed == owned {} + if borrowed == owned {} + } +} + +// Only Owned == Borrowed is implemented +mod owned_eq_borrowed { + impl_types!(); + + impl PartialEq<Borrowed> for Owned { + fn eq(&self, _: &Borrowed) -> bool { + true + } + } + + fn compare() { + let owned = Owned {}; + let borrowed = Borrowed {}; + + if owned == borrowed {} + if owned == borrowed {} + } +} + +mod issue_4874 { + impl_types!(); + + // NOTE: PartialEq<Borrowed> for T can't be implemented due to the orphan rules + impl<T> PartialEq<T> for Borrowed + where + T: AsRef<str> + ?Sized, + { + fn eq(&self, _: &T) -> bool { + true + } + } + + impl std::fmt::Display for Borrowed { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "borrowed") + } + } + + fn compare() { + let borrowed = Borrowed {}; + + if borrowed == "Hi" {} + if borrowed == "Hi" {} + } +} + +fn main() {} diff --git a/tests/ui/cmp_owned/asymmetric_partial_eq.rs b/tests/ui/cmp_owned/asymmetric_partial_eq.rs new file mode 100644 index 00000000000..88bc2f51dd6 --- /dev/null +++ b/tests/ui/cmp_owned/asymmetric_partial_eq.rs @@ -0,0 +1,93 @@ +// run-rustfix +#![allow(unused, clippy::redundant_clone)] // See #5700 + +// Define the types in each module to avoid trait impls leaking between modules. +macro_rules! impl_types { + () => { + #[derive(PartialEq)] + pub struct Owned; + + pub struct Borrowed; + + impl ToOwned for Borrowed { + type Owned = Owned; + fn to_owned(&self) -> Owned { + Owned {} + } + } + + impl std::borrow::Borrow<Borrowed> for Owned { + fn borrow(&self) -> &Borrowed { + static VALUE: Borrowed = Borrowed {}; + &VALUE + } + } + }; +} + +// Only Borrowed == Owned is implemented +mod borrowed_eq_owned { + impl_types!(); + + impl PartialEq<Owned> for Borrowed { + fn eq(&self, _: &Owned) -> bool { + true + } + } + + pub fn compare() { + let owned = Owned {}; + let borrowed = Borrowed {}; + + if borrowed.to_owned() == owned {} + if owned == borrowed.to_owned() {} + } +} + +// Only Owned == Borrowed is implemented +mod owned_eq_borrowed { + impl_types!(); + + impl PartialEq<Borrowed> for Owned { + fn eq(&self, _: &Borrowed) -> bool { + true + } + } + + fn compare() { + let owned = Owned {}; + let borrowed = Borrowed {}; + + if owned == borrowed.to_owned() {} + if borrowed.to_owned() == owned {} + } +} + +mod issue_4874 { + impl_types!(); + + // NOTE: PartialEq<Borrowed> for T can't be implemented due to the orphan rules + impl<T> PartialEq<T> for Borrowed + where + T: AsRef<str> + ?Sized, + { + fn eq(&self, _: &T) -> bool { + true + } + } + + impl std::fmt::Display for Borrowed { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "borrowed") + } + } + + fn compare() { + let borrowed = Borrowed {}; + + if "Hi" == borrowed.to_string() {} + if borrowed.to_string() == "Hi" {} + } +} + +fn main() {} diff --git a/tests/ui/cmp_owned/asymmetric_partial_eq.stderr b/tests/ui/cmp_owned/asymmetric_partial_eq.stderr new file mode 100644 index 00000000000..43bf8851fc6 --- /dev/null +++ b/tests/ui/cmp_owned/asymmetric_partial_eq.stderr @@ -0,0 +1,46 @@ +error: this creates an owned instance just for comparison + --> $DIR/asymmetric_partial_eq.rs:42:12 + | +LL | if borrowed.to_owned() == owned {} + | ^^^^^^^^^^^^^^^^^^^ help: try: `borrowed` + | + = note: `-D clippy::cmp-owned` implied by `-D warnings` + +error: this creates an owned instance just for comparison + --> $DIR/asymmetric_partial_eq.rs:43:21 + | +LL | if owned == borrowed.to_owned() {} + | ---------^^^^^^^^^^^^^^^^^^^ + | | + | help: try: `borrowed == owned` + +error: this creates an owned instance just for comparison + --> $DIR/asymmetric_partial_eq.rs:61:21 + | +LL | if owned == borrowed.to_owned() {} + | ^^^^^^^^^^^^^^^^^^^ help: try: `borrowed` + +error: this creates an owned instance just for comparison + --> $DIR/asymmetric_partial_eq.rs:62:12 + | +LL | if borrowed.to_owned() == owned {} + | ^^^^^^^^^^^^^^^^^^^--------- + | | + | help: try: `owned == borrowed` + +error: this creates an owned instance just for comparison + --> $DIR/asymmetric_partial_eq.rs:88:20 + | +LL | if "Hi" == borrowed.to_string() {} + | --------^^^^^^^^^^^^^^^^^^^^ + | | + | help: try: `borrowed == "Hi"` + +error: this creates an owned instance just for comparison + --> $DIR/asymmetric_partial_eq.rs:89:12 + | +LL | if borrowed.to_string() == "Hi" {} + | ^^^^^^^^^^^^^^^^^^^^ help: try: `borrowed` + +error: aborting due to 6 previous errors + diff --git a/tests/ui/cmp_owned/issue_4874.rs b/tests/ui/cmp_owned/issue_4874.rs deleted file mode 100644 index b29c555eb1e..00000000000 --- a/tests/ui/cmp_owned/issue_4874.rs +++ /dev/null @@ -1,51 +0,0 @@ -#![allow(clippy::redundant_clone)] // See #5700 - -#[derive(PartialEq)] -struct Foo; - -struct Bar; - -impl std::fmt::Display for Bar { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "bar") - } -} - -// NOTE: PartialEq<Bar> for T can't be implemented due to the orphan rules -impl<T> PartialEq<T> for Bar -where - T: AsRef<str> + ?Sized, -{ - fn eq(&self, _: &T) -> bool { - true - } -} - -// NOTE: PartialEq<Bar> for Foo is not implemented -impl PartialEq<Foo> for Bar { - fn eq(&self, _: &Foo) -> bool { - true - } -} - -impl ToOwned for Bar { - type Owned = Foo; - fn to_owned(&self) -> Foo { - Foo - } -} - -impl std::borrow::Borrow<Bar> for Foo { - fn borrow(&self) -> &Bar { - static BAR: Bar = Bar; - &BAR - } -} - -fn main() { - let b = Bar {}; - if "Hi" == b.to_string() {} - - let f = Foo {}; - if f == b.to_owned() {} -} |