Fix/12035 silence struct field names #12190
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
the
S-waiting-on-review
alexis-langlet
force-pushed
the
fix/12035-silence-struct-field-names
branch
from
57dbf57
to
9a3cdbe
Compare
| @@ -180,7 +181,8 @@ fn have_no_extra_prefix(prefixes: &[&str]) -> bool { | |||
| } | |||
|
|
|||
| fn check_fields(cx: &LateContext<'_>, threshold: u64, item: &Item<'_>, fields: &[FieldDef<'_>]) { | |||
| if (fields.len() as u64) < threshold { | |||
| if (fields.len() as u64) < threshold || any_impl_has_lint_allowed(cx, STRUCT_FIELD_NAMES, item.owner_id.to_def_id()) | |||
There was a problem hiding this comment.
thought: unsure if we should perform this check before or after the field name check
this is a bunch of queries, that is some string comparisons, so probably before makes sense.
|
@bors r+ |
…names, r=Manishearth Fix/12035 silence struct field names fixes #12035 Allows to silence lints `struct_field_names` and `enum_variant_names` through their impls. This is done in order for some crates such as `serde` to silence those lints changelog: [`struct_field_names`]: allow to silence lint through struct's impl changelog: [`enum_variant_names`]: allow to silence lint through enum's impl
|
💔 Test failed - checks-action_test |
| /// Returns `true` if any of the `impl`s for the given `item` has the lint allowed | ||
| pub fn any_impl_has_lint_allowed(cx: &LateContext<'_>, lint: &'static Lint, id: DefId) -> bool { | ||
| cx.tcx | ||
| .inherent_impls(id) |
There was a problem hiding this comment.
Sorry, a bit late, but does this also return trait impls? tcx.inherent_impls only contains those impls with no trait reference, right? If it does work correctly, I think it should at least have a test, since that's usually what derive macros expand to and AIUI what the issue asked for ?
Though reading #12035 (comment), it doesn't seem uncontroversial that serde should annotate its impls with this anyway and maybe deserves more discussion in general for the ser/de case?
There was a problem hiding this comment.
oh yeah good point.
perhaps we should wait on this PR for now
There was a problem hiding this comment.
also looking up all trait impls can be an expensive operation
…names, r=Manishearth Fix/12035 silence struct field names fixes #12035 Allows to silence lints `struct_field_names` and `enum_variant_names` through their impls. This is done in order for some crates such as `serde` to silence those lints changelog: [`struct_field_names`]: allow to silence lint through struct's impl changelog: [`enum_variant_names`]: allow to silence lint through enum's impl
|
💔 Test failed - checks-action_dev_test |
|
Hey @y21, this is a ping from triage. It looks like this PR was already approved. Is there anything that needs to be done, or did bors just fail for bors reasons? |
|
Hi @xFrednet, after discussing a bit about the related issue on Zulip, it looks like it won't really be useful. |
|
Hey @alexis-langlet, I've checked the tests and agree to close the issues. It seems a bit weird to me that impls should allow it, since the lints should trigger on the enum definition and not the impls. Thank you for working on this and also being part of the discussion. |
|
Okay, after reading skimming through the issue, I understand the motivation for allowing this lint to be allowed on an impl. My suggestion might be to allow traits to allow this lint instead, but it all seems a bit weird. I would like to get more input from the others. I'll nominate this for the next meeting with the question:
@rustbot label +I-nominate Personally, I think it's better to keep this lint simple. For a user it should be easy to add |
|
☔ The latest upstream changes (presumably #12635) made this pull request unmergeable. Please resolve the merge conflicts. |
fixes #12035
Allows to silence lints
struct_field_namesandenum_variant_namesthrough their impls.This is done in order for some crates such as
serdeto silence those lintschangelog: [
struct_field_names]: allow to silence lint through struct's implchangelog: [
enum_variant_names]: allow to silence lint through enum's impl