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

Fire clippy::op_ref on PartialOrd but !Ord types #4876

Closed
wants to merge 1 commit into from
Closed

Fire clippy::op_ref on PartialOrd but !Ord types #4876

wants to merge 1 commit into from

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Dec 3, 2019

Some context: this appears to be the only usage of .ord_trait() in the compiler. I've a PR that tries to remove it. It's definitely not correct here, though! Example:

#[derive(PartialOrd, PartialEq, Eq, /*Ord*/)]
struct S(u32);

fn main() {
    let a = S(0);
    let b = S(1);
    dbg!(&a < &b);
}

On the above snippet, clippy::op_ref does not fire unless we also derive Ord due to the use of .ord_trait() here meaning we gate the lint on implementing Ord, rather than just PartialOrd which is the trait actually required to use comparison operators.

changelog: Fix: trigger clippy::op_ref (needlessly taken reference of both operands) when a comparison operator's operands implement PartialOrd but not Ord. Previously, the lint only fired if and only if Ord was implemented.

@CAD97
Copy link
Contributor Author

CAD97 commented Dec 3, 2019

CI failure is an extra lint trigger:

error: taken reference of right operand
  --> $DIR/op_ref.rs:20:8
   |
LL |     if b < &a {
   |        ^^^^--
   |            |
   |            help: use the right value directly: `a`

Context:

let a = "a".to_string();
let b = "a";
if b < &a {
println!("OK");
}

Relevant arm:

// foo == &bar
(_, &ExprKind::AddrOf(BorrowKind::Ref, _, ref r)) => {
let rty = cx.tables.expr_ty(r);
let rcpy = is_copy(cx, rty);
if (requires_ref || rcpy)
&& implements_trait(cx, cx.tables.expr_ty(left), trait_id, &[rty.into()])
{
span_lint_and_then(cx, OP_REF, e.span, "taken reference of right operand", |db| {
let rsnip = snippet(cx, r.span, "...").to_string();
db.span_suggestion(
right.span,
"use the right value directly",
rsnip,
Applicability::MachineApplicable, // snippet
);
})
}
},

Removing the ref here is wrong due to not having str: PartialEq<String>. I'm not exactly sure why it's firing, though.

@flip1995
Copy link
Member

flip1995 commented Dec 3, 2019

The "FP" is the same as #2597, it is not really a FP tough. In such a case the correct suggestion would be to write *b < *a.

@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Dec 3, 2019
@CAD97
Copy link
Contributor Author

CAD97 commented Dec 3, 2019

rust-lang/rust#66941 has been merged and this patch is now required to get clippy building with latest rust again.

Should I just change the test expected output and mark it as a case of #2597? I'm honestly not certain how best to go about fixing that issue (and fixing it in this PR is probably outside of scope).

@flip1995
Copy link
Member

flip1995 commented Dec 3, 2019

I'm on this myself (there was also another PR, that broke Clippy). It stopped linting for this case, which is good (?). But it's weird, that it returned true for &str: Ord<String>, but not for &str: PartialOrd<String>...

@CAD97
Copy link
Contributor Author

CAD97 commented Dec 3, 2019

Note that Ord doesn't take parameters. It's just trait Ord {, not trait Ord<Rhs> { like trait PartialOrd<Rhs>. I suspect implements_trait is just ignoring excess generic arguments.

@flip1995 flip1995 mentioned this pull request Dec 3, 2019
@flip1995 flip1995 closed this Dec 3, 2019
@CAD97 CAD97 deleted the patch-1 branch December 3, 2019 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants