rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.25k stars 1.51k forks source link

Respect `#[allow(clippy::inconsistent_struct_constructor)]` on adt definition. #13203

Closed rzvxa closed 1 month ago

rzvxa commented 1 month ago

Description

Is it possible to add support to inconsistent_struct_constructors so it respects the allow attribute on the adt definition as well as on construction?

We are using a proc macro to reorder fields in repr C structs; Which means we have inconsistency between field definitions and their ABI; It would be really convenient for us to be able to mark the types with the allow attribute instead of having to use constructor methods for everything(or force this attribute to our downstream with isn't a possibility).

So are there any other rules that work in this way(respecting the attribute on both call site and definition)? I'm happily willing to create PR for this issue if I get the green light.

I'd appreciate any suggestions to resolve my issue, There might be some way of doing it which I'm missing here.

Version

No response

Additional Labels

No response

Jarcho commented 1 month ago

There are a few lints that do this (e.g. len_without_is_empty). Just needs a use of is_lint_allowed and fulfull_expectation on the right HIR node.

rzvxa commented 1 month ago

There are a few lints that do this (e.g. len_without_is_empty). Just needs a use of is_lint_allowed and fulfull_expectation on the right HIR node.

Thanks, I'd like to give it a try(It seems to be the fastest way to get it in). Is it OK if we assign this issue to me?

Jarcho commented 1 month ago

Just use @rustbot claim in a comment on the issue you wan to claim (without the backticks). You can read https://rustc-dev-guide.rust-lang.org/rustbot.html for more info.

rzvxa commented 1 month ago

@rustbot claim

rzvxa commented 1 month ago

@Jarcho Is it possible to use is_lint_allowed on an AdtDef where it is from a foreign crate? I can't find a way to get hir from non-local AdtDef of the struct type.

            && !expr.span.from_expansion()
            && let ty = cx.typeck_results().expr_ty(expr)
            && let Some(adt_def) = ty.ty_adt_def()
            && adt_def.is_struct()
            && !is_lint_allowed(cx, INCONSISTENT_STRUCT_CONSTRUCTOR, adt_def.to_hir_somehow?()) // what should go here?

If is_lint_allowed only operates on local types, How can we achieve this when the inconsistent type is marked with allow and exists outside of our crate?

Jarcho commented 1 month ago

You can't get lint attributes from other crates. I would probably just restrict the lint to only work with types from the current crate while you're at it. Upstream crates don't have a definition order from the point of view of the current crate (with the exception of #[repr(C)] structs).

You can use adt_def.did().as_local() to get an Option<LocalDefId> and cx.tcx.local_def_id_to_hir_id(_) get the HirId from there.

rzvxa commented 1 month ago

You can't get lint attributes from other crates. I would probably just restrict the lint to only work with types from the current crate while you're at it. Upstream crates don't have a definition order from the point of view of the current crate (with the exception of #[repr(C)] structs).

You can use adt_def.did().as_local() to get an Option<LocalDefId> and cx.tcx.local_def_id_to_hir_id(_) get the HirId from there.

Thanks for your quick response, I really appreciate it; Yep if we only lint where adt def is local it should be pretty trivial, It'd be almost identical to how len_zero handles it.