Skip to content

Commit

Permalink
Auto merge of #5701 - ebroto:4874_cmp_owned_fp, r=flip1995
Browse files Browse the repository at this point in the history
cmp_owned: handle when PartialEq is not implemented symmetrically

changelog: Handle asymmetrical implementations of PartialEq in [`cmp_owned`].

Fixes #4874
  • Loading branch information
bors committed Jun 24, 2020
2 parents a14eab3 + b498e1d commit 46d3304
Show file tree
Hide file tree
Showing 4 changed files with 291 additions and 33 deletions.
92 changes: 59 additions & 33 deletions clippy_lints/src/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ use rustc_ast::ast::LitKind;
use rustc_errors::Applicability;
use rustc_hir::intravisit::FnKind;
use rustc_hir::{
def, BinOpKind, BindingAnnotation, Body, Expr, ExprKind, FnDecl, HirId, Mutability, PatKind, Stmt, StmtKind, Ty,
TyKind, UnOp,
self as hir, def, BinOpKind, BindingAnnotation, Body, Expr, ExprKind, FnDecl, HirId, Mutability, PatKind, Stmt,
StmtKind, TyKind, UnOp,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty;
use rustc_middle::ty::{self, Ty};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::hygiene::DesugaringKind;
use rustc_span::source_map::{ExpnKind, Span};
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -570,19 +570,38 @@ 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 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
}
}

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;
}
},
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;
}
Expand All @@ -593,28 +612,19 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr<'_>, other: &Expr<'_>) {
_ => return,
};

let other_ty = cx.tables.expr_ty_adjusted(other);
let partial_eq_trait_id = match cx.tcx.lang_items().eq_trait() {
Some(id) => id,
None => return,
};
let other_ty = cx.tables.expr_ty(other);

let deref_arg_impl_partial_eq_other = arg_ty.builtin_deref(true).map_or(false, |tam| {
implements_trait(cx, tam.ty, partial_eq_trait_id, &[other_ty.into()])
});
let arg_impl_partial_eq_deref_other = other_ty.builtin_deref(true).map_or(false, |tam| {
implements_trait(cx, arg_ty, partial_eq_trait_id, &[tam.ty.into()])
});
let arg_impl_partial_eq_other = implements_trait(cx, arg_ty, partial_eq_trait_id, &[other_ty.into()]);
let without_deref = symmetric_partial_eq(cx, arg_ty, other_ty).unwrap_or_default();
let with_deref = arg_ty
.builtin_deref(true)
.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)
Expand All @@ -634,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
);
},
Expand Down Expand Up @@ -694,7 +720,7 @@ fn non_macro_local(cx: &LateContext<'_, '_>, res: def::Res) -> bool {
}
}

fn check_cast(cx: &LateContext<'_, '_>, span: Span, e: &Expr<'_>, ty: &Ty<'_>) {
fn check_cast(cx: &LateContext<'_, '_>, span: Span, e: &Expr<'_>, ty: &hir::Ty<'_>) {
if_chain! {
if let TyKind::Ptr(ref mut_ty) = ty.kind;
if let ExprKind::Lit(ref lit) = e.kind;
Expand Down
93 changes: 93 additions & 0 deletions tests/ui/cmp_owned/asymmetric_partial_eq.fixed
Original file line number Diff line number Diff line change
@@ -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() {}
93 changes: 93 additions & 0 deletions tests/ui/cmp_owned/asymmetric_partial_eq.rs
Original file line number Diff line number Diff line change
@@ -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() {}
46 changes: 46 additions & 0 deletions tests/ui/cmp_owned/asymmetric_partial_eq.stderr
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 46d3304

Please sign in to comment.