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

Identical arguments on assert macro family #6167

Merged
merged 6 commits into from
Oct 19, 2020

Conversation

ThibsG
Copy link
Contributor

@ThibsG ThibsG commented Oct 13, 2020

Lint when identical args are used on assert_eq!, debug_assert_eq!, assert_ne! and debug_assert_ne! macros.

Added to the lint eq_op.

Common functions added to utils/higher.rs

Fixes: #3574
Fixes: #4694

changelog: Lint on identical args when calling assert_eq!, debug_assert_eq!, assert_ne! and debug_assert_ne! macros

@rust-highfive
Copy link

r? @Manishearth

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 13, 2020
clippy_lints/src/lib.rs Outdated Show resolved Hide resolved
@ThibsG
Copy link
Contributor Author

ThibsG commented Oct 13, 2020

I've been looking at mutable_debug_assertion lint that links to #4694. I will probably give it a try in a future PR, it could be nice to have less duplicated code for these two lints.

@Manishearth
Copy link
Member

r? @ebroto since you've already started, feel free to bounce back the review if you're busy!

@rust-highfive rust-highfive assigned ebroto and unassigned Manishearth Oct 13, 2020
@flip1995
Copy link
Member

2 things:

  1. (you already noticed that) This would greatly profit from Add {debug_,}assert{,-eq,-ne} to utils/higher.rs #4694 being implemented first
  2. counter example: assert_ne!(my_special_iter.next(), my_special_iter.next()) could be a valid test for some iterator implementation on a type.

@ThibsG
Copy link
Contributor Author

ThibsG commented Oct 14, 2020

  1. (you already noticed that) This would greatly profit from Add {debug_,}assert{,-eq,-ne} to utils/higher.rs #4694 being implemented first

Ok I will implement #4694 in this PR 😉

  1. counter example: assert_ne!(my_special_iter.next(), my_special_iter.next()) could be a valid test for some iterator implementation on a type.

It doesn't lint, because eq_expr_value is disabling side effects. But I will add this as it is a good test case though.

@ThibsG ThibsG changed the title Identical arguments on assert macro family WIP: Identical arguments on assert macro family Oct 14, 2020
@ThibsG ThibsG force-pushed the IdenticalArgumentsAssertEq3574 branch from f95b2ee to 9009016 Compare October 16, 2020 16:01
clippy_lints/src/eq_op.rs Show resolved Hide resolved
clippy_lints/src/utils/higher.rs Outdated Show resolved Hide resolved
clippy_lints/src/utils/higher.rs Outdated Show resolved Hide resolved
@ebroto ebroto added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 16, 2020
@ThibsG ThibsG force-pushed the IdenticalArgumentsAssertEq3574 branch from 9009016 to 5a13217 Compare October 17, 2020 09:54
Copy link
Member

@ebroto ebroto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we have to split the eq_op tests in order to avoid the stderr file limit.

@ThibsG
Copy link
Contributor Author

ThibsG commented Oct 19, 2020

👀 oops will do.

By the way, in #4694 (comment), @flip1995 was wishing the following:

A great extension of this would be to also return the format_arg expressions. So for assert!(expr, "some {} things", fmt_expr) it would return [expr, fmt_expr], but we can do this in a second iteration.

Is it still wanted?

@ThibsG ThibsG force-pushed the IdenticalArgumentsAssertEq3574 branch from 3e2e1e8 to 16b5f37 Compare October 19, 2020 15:37
Copy link
Member

@ebroto ebroto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it still wanted?

I would say so, but I think it can be left for a follow-up PR. This is already an improvement on Clippy's toolset, and a cool enhancement of the lint. Thank you!

@ebroto
Copy link
Member

ebroto commented Oct 19, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 19, 2020

📋 Looks like this PR is still in progress, ignoring approval.

Hint: Remove WIP from this PR's title when it is ready for review.

@ebroto ebroto changed the title WIP: Identical arguments on assert macro family Identical arguments on assert macro family Oct 19, 2020
@ebroto
Copy link
Member

ebroto commented Oct 19, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 19, 2020

📌 Commit 16b5f37 has been approved by ebroto

@bors
Copy link
Contributor

bors commented Oct 19, 2020

⌛ Testing commit 16b5f37 with merge eaffd0e...

@bors
Copy link
Contributor

bors commented Oct 19, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing eaffd0e to master...

@bors bors merged commit eaffd0e into rust-lang:master Oct 19, 2020
@ThibsG ThibsG deleted the IdenticalArgumentsAssertEq3574 branch October 19, 2020 21:13
@flip1995
Copy link
Member

Awesome! Thanks for implementing this :)

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.

Add {debug_,}assert{,-eq,-ne} to utils/higher.rs warn on identical arguments of assert_eq!()
6 participants