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.

Sign up for GitHub

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

Jump to bottom

catch assert! assert_eq! assert_ne! and variants in ResultFN #6082

Closed
thefallentree opened this issue Sep 24, 2020 · 9 comments · Fixed by #6280
Closed

catch assert! assert_eq! assert_ne! and variants in ResultFN #6082

thefallentree opened this issue Sep 24, 2020 · 9 comments · Fixed by #6280
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy

Comments

@thefallentree
Copy link

What it does

catch assert! assert_eq! assert_ne! and variants in ResultFN

in a function that returns result , it is very likely the use of these macros are plain wrong.

Categories (optional)

similar to https://rust-lang.github.io/rust-clippy/master/#panic_in_result_fn
@thefallentree thefallentree added the A-lint Area: New lints label Sep 24, 2020
@thefallentree thefallentree changed the title catch aseert! assert_eq! assert_ne! and variants in ResultFN catch assert! assert_eq! assert_ne! and variants in ResultFN Sep 24, 2020
@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy hacktoberfest labels Oct 5, 2020
@flip1995
Copy link
Member

flip1995 commented Oct 5, 2020

@nahuakang This seems like a good first issue to me.

A similar lint is already implemented and code can probably shared with that lint. So what you would have to do is to look at the panic_in_result_fn lint, understand how it works and implement the same (optimally sharing code) for assert macros. And obviously write tests for them.

@nahuakang
Copy link
Contributor

Thanks Phil. I'll pick up on your hints!

@dp304
Copy link
Contributor

dp304 commented Oct 31, 2020

@nahuakang are you still working on this issue?

@nahuakang
Copy link
Contributor

nahuakang commented Oct 31, 2020 •
edited
Loading

@dp304 Yes I intend to. Edit: I'm gonna get started on it soon :)

@dp304 dp304 mentioned this issue Oct 31, 2020
Merged
@dp304
Copy link
Contributor

dp304 commented Oct 31, 2020

@nahuakang I haven't noticed your comment edit, so I've made a PR. Sorry about that.

@nahuakang
Copy link
Contributor

Cool, then I'll read your PR and learn from you!

bors added a commit that referenced this issue Dec 7, 2020
@bors
Auto merge of #6280 - dp304:assert_in_result_fn, r=ebroto
c5096e6 
Add lint for assertions in functions returning Result

changelog: none
fixes #6082
@bors bors closed this as completed in 856f4f3 Dec 7, 2020
@daxpedda
Copy link
Contributor

Why was debug_assert & co included into this list?
I consider debug_assert just a helper to do sanity checks during debug builds that go away during production, this lint would discourage that kind of use and instead would recommend creating new error types to enable debug errors?

I would like to make a PR that removes debug_assert & co from the list of macros that triggers this lint.
@thefallentree & @flip1995 what you think?

@flip1995
Copy link
Member

flip1995 commented Apr 10, 2021 •
edited
Loading

@daxpedda You're right, this lint shouldn't trigger on debug macros. It would be great if you could open a PR fixing that. Please also note this in the lint documentation though.

@daxpedda
Copy link
Contributor

Will do!

@daxpedda daxpedda mentioned this issue Apr 10, 2021
Merged
bors added a commit that referenced this issue Apr 10, 2021
@bors
Auto merge of #7060 - daxpedda:debug-assert-panic-in-result-fn, r=fli…
a6c46e9 
…p1995

Remove `debug_assert` from `panic_in_result_fn`

I couldn't find any documentation on `debug_assert` that should be remove.
In my humble opinion, I would also like to argue that `todo` and `unreachable` shouldn't trigger this lint?

Related: #6082

r? `@flip1995`

changelog:
Change `panic_in_result_fn` to ignore `debug_assert` and co macros
bors added a commit that referenced this issue Apr 10, 2021
@bors
Auto merge of #7060 - daxpedda:debug-assert-panic-in-result-fn, r=fli…
479d4f8 
…p1995

Remove `debug_assert` from `panic_in_result_fn`

I couldn't find any documentation on `debug_assert` that should be remove.
In my humble opinion, I would also like to argue that `todo` and `unreachable` shouldn't trigger this lint?

Related: #6082

r? `@flip1995`

changelog:
Change `panic_in_result_fn` to ignore `debug_assert` and co macros
bors added a commit that referenced this issue Apr 10, 2021
@bors
Auto merge of #7060 - daxpedda:debug-assert-panic-in-result-fn, r=fli…
fd5cf4e 
…p1995

Remove `debug_assert` from `panic_in_result_fn`

I couldn't find any documentation on `debug_assert` that should be remove.
In my humble opinion, I would also like to argue that `todo` and `unreachable` shouldn't trigger this lint?

Related: #6082

r? `@flip1995`

changelog: Change `panic_in_result_fn` to ignore `debug_assert` and co macros
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add lint for assertions in functions returning Result dp304/rust-clippy
5 participants
@thefallentree @daxpedda @flip1995 @nahuakang @dp304