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

Remove debug_assert from panic_in_result_fn #7060

Merged
merged 3 commits into from
Apr 10, 2021

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Apr 10, 2021

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 10, 2021
@flip1995
Copy link
Member

debug_assert with a message still seems to get linted. I don't really know why off the top of my head.

@flip1995 flip1995 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 Apr 10, 2021
@daxpedda
Copy link
Contributor Author

daxpedda commented Apr 10, 2021

That's fine, I just realized I ran the wrong tests locally, I'm currently attending the rust gamedev meeting but will fix it when I'm done.

@daxpedda
Copy link
Contributor Author

I don't know if this is an appropriate solution.

In my humble opinion, I would also like to argue that todo and unreachable shouldn't trigger this lint?

@flip1995 what do you think about that? If this is alright I could just remove find_macro_calls and only check with is_expn_of.

@flip1995
Copy link
Member

Since this is a restriction lint, I would leave unreachable and todo in it. For unreachable I'd argue, that you could also return Err("unreachable code reached") and todo! shouldn't be in the final code anyway. I see that this is opinionated, but restriction lints are really specific anyway, so I'm fine with leaving them in the lint and if people disagree they can allow this lint on functions that contains one of those macros. (allowing restriction lints is even more encouraged than allowing normal Clippy lints, if you say that it is okay that a Clippy lint triggers in a specific case).

@flip1995

This comment has been minimized.

@bors

This comment has been minimized.

@flip1995

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

bors added a commit that referenced this pull request Apr 10, 2021
…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

This comment has been minimized.

bors added a commit that referenced this pull request Apr 10, 2021
…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

This comment has been minimized.

@bors

This comment has been minimized.

@flip1995
Copy link
Member

@bors retry

@bors
Copy link
Collaborator

bors commented Apr 10, 2021

⌛ Testing commit 271c163 with merge fd5cf4e...

@bors
Copy link
Collaborator

bors commented Apr 10, 2021

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

@bors bors merged commit fd5cf4e into rust-lang:master Apr 10, 2021
@daxpedda daxpedda deleted the debug-assert-panic-in-result-fn branch April 10, 2021 19:44
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.

4 participants