Skip to content

Manual assert_eq! (enhance if_then_panic) #7716

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

Open
camsteffen opened this issue Sep 24, 2021 · 5 comments
Open

Manual assert_eq! (enhance if_then_panic) #7716

camsteffen opened this issue Sep 24, 2021 · 5 comments
Assignees
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good first issue These issues are a good way to get started with Clippy

Comments

@camsteffen
Copy link
Contributor

camsteffen commented Sep 24, 2021

We just added if_then_panic, but then I realized there's a possible enhancement for assert_eq!/assert_ne!:

// before
if a != b {
    panic!("hi");
}
// after
assert_eq!(a, b, "hi");

I think a rename to manual_assert would be best. But we could also keep the current name (it still kinda makes sense) or create a new lint for this case.

@camsteffen camsteffen added C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good first issue These issues are a good way to get started with Clippy labels Sep 24, 2021
@xFrednet
Copy link
Member

Nice suggestion, and renaming the lint to manual_assert sounds like a good idea. We should try to do this, before the if_then_panic lint hit's stable, IMO 🙃

@Labelray
Copy link
Contributor

I am interested in implementing the enhancement. And I think it is better to use just one lint manual_assert for current if_then_panic and this new case, not new lint for this new case.

@rustbot claim

@xFrednet
Copy link
Member

Hey @Labelray, if you're working on this issue, you might also want to take a look at #7718. @mikerite suggested in the issue to move this lint to pedantic which seems reasonable to me. That might be an easy change to do along the way 🙃.

@camsteffen
Copy link
Contributor Author

Another thing I thought of - lint if-else-panic and don't lint if-panic-else-panic.

@jplatte
Copy link
Contributor

jplatte commented Sep 30, 2021

Relatedly, if !something { panic!(); } should suggest assert!(something); instead of assert!(!!something);. (saw this in https://github.com/ruma/ruma/runs/3759040805?check_suite_focus=true#step:5:399, I'm guessing that the double negation would be happening without the binop which is also not being treated correctly (#7731).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good first issue These issues are a good way to get started with Clippy
Projects
None yet
Development

No branches or pull requests

4 participants