Warn on structs with a trailing zero-sized array but no repr attribute
#7838
Conversation
why was `rustc_lint_defs` not already externed in `lib.rs`? and how was r-a able to find it but cargo wasn't? 🤔
This reverts commit d85f903.
|
r? @camsteffen (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| } | ||
|
|
||
| fn has_repr_attr(cx: &LateContext<'tcx>, hir_id: HirId) -> bool { | ||
| cx.tcx.hir().attrs(hir_id).iter().any(|attr| attr.has_name(sym::repr)) |
There was a problem hiding this comment.
There's at least four other ways to do this but I liked this one the best. (All five agreed on all testcases.) Happy to use another; they're in the commit history if you want to look (or I can go find them).
|
Also want to note that a previous method I use using to detect if the array is zero-sized led to an ICE. I tried for a bit to figure it out but tbh it was above my paygrade, but I figured I should let y'all know bc there's nothing stopping that code from being written. I made a minimal-ish repro and put it on a separate branch in my fork here: https://github.com/nhamovitz/rust-clippy/tree/demonstrates_ice. (Specifically these lines: https://github.com/nhamovitz/rust-clippy/blob/d85f903c91d909534003ee2ff0e16316b20687dc/clippy_lints/src/trailing_zero_sized_array_without_repr.rs#L61-L69). Happy to open a new issue for this (might be a rustc issue, actually); just lmk. |
|
Also I guess r? @Manishearth because they've been helping me with this so far :) |
CHANGELOG.md
Outdated
| @@ -3021,6 +3021,7 @@ Released 2018-09-13 | |||
| [`too_many_arguments`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments | |||
| [`too_many_lines`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines | |||
| [`toplevel_ref_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#toplevel_ref_arg | |||
| [`trailing_zero_sized_array_without_repr`]: https://rust-lang.github.io/rust-clippy/master/index.html#trailing_zero_sized_array_without_repr | |||
There was a problem hiding this comment.
hmm, might need a better name (@camsteffen @xFrednet thoughts?)
maybe repr_rust_trailing_empty_array? still kinda long
There was a problem hiding this comment.
Currently, I can't think of a better name, we might be able to just call it trailing_empty_array but that would leave out the condition of having no repr attribute 🤔
Btw. nice commit messages @nhamovitz it's funny to see the full range of emotions that you went, though xD. Welcome to the project ^^
There was a problem hiding this comment.
trailing_empty_array seems fine imo actually
There was a problem hiding this comment.
👍 Changed to trailing_empty_array! (and updated the changelog in the initial PR message)
There was a problem hiding this comment.
trailing_empty_array_no_repr or trailing_zst_no_repr
|
@bors r+ thanks! |
|
📌 Commit 0f9f591 has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Using a zero-sized Typically I've seen this as the first field, so I don't know how much this lint will FP on this convention, but it's a definite possibility. (Personally I always tend to put "phantom fields" at the end, so I'd hit an FP here.) Personally, I'd say this definitely shouldn't lint if the ZST is the only field, and probably should only lint with a literal zero. |
Closes #2868
changelog: Implement
[`trailing_empty_array`], which warns if a struct is defined where the last field is a zero-sized array but there are noreprattributes. Zero-sized arrays aren't very useful in Rust itself, so such a struct is likely being created to pass to C code or in some other situation where control over memory layout matters. Either way, areprattribute is needed.