[dbg_macro] tolerates use of dbg! in items which have #[cfg(test)] attribute
#8838
Conversation
|
r? @flip1995 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @Jarcho 🙃 |
clippy_utils/src/lib.rs
Outdated
| if attr.has_name(sym::cfg) | ||
| && let Some(items) = attr.meta_item_list() | ||
| && items.len() == 1 | ||
| && items[0].has_name(sym::test) |
There was a problem hiding this comment.
Small nit. This could be let [item] = &*items && item.has_name(sym::test).
clippy_utils/src/lib.rs
Outdated
| } | ||
| tcx.hir() | ||
| .parent_iter(id) | ||
| .any(|(parent_id, _)| tcx.hir().attrs(parent_id).iter().any(is_cfg_test)) |
There was a problem hiding this comment.
Small nit. This could be flat_map(|(parent_id, _)| tcx.hir().attrs(parent_id)).any(is_cfg_test).
|
@Jarcho |
|
@bors delegate=Jarcho (Bors doesn't work in review comments) |
|
✌️ @Jarcho can now approve this pull request |
|
Everything looks good. @bors r=Jarcho,xFrednet |
|
📌 Commit db41df1 has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
|
That is now a very confusing behaviour on the part of that lint. When I run it, I expect it to find all uses of dbg, regardless of test or no test. When I use dbg in tests, it's also for debugging purposes, and leaving it in will just cause tests to spam debug messages in the console. A change like that should have been a separate lint with a clear name, which could be separately allowed. |
|
I would dislike splitting the lint, but we could introduce a config value, which enables the user to specify the behavior in tests. Do you think that is a better idea? @afetisov 🙃 |
|
I don't have any objections to a config value, I would just want the lint to be enabled everywhere by default. |
|
I think a separate lint is a bit better for discoverability, since Clippy will print its name when it triggers. Config values are afaik mentioned only in the docs. But from a practical perspective, a config value indeed makes more sense. |
|
The question would be how the lints, should be split. It wouldn't make sense to have one lint that tests everything and one that is specific to anything but functions, as they would overlap. Therefore, we would need one lint for tests and one for other code. For me, that would be more confusing than a config value. But I'm also used to working with the lint list ^^ |
|
I think it is better to introduce a config value and to warn all dbg! by default too. Mentioning it in warning messages of [dbg_macro] might be useful. |
|
Would you mind adding it? We have some documentation how this can be done. If not, we can just create a new issue 🙃 |
|
Okey. I will take care of it |
This was done because Nightly Rust complained about these, despite them all being in tests. That is now fixed upstream: rust-lang/rust-clippy#8758 rust-lang/rust-clippy#8838 This reverts commit 9d26a91.
fix: #8758
changelog: [dbg_macro] tolerates use of
dbg!in items with#[cfg(test)]attribute