add a lint against references to packed fields #72270
Conversation
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
RalfJung
force-pushed
the
lint-ref-to-packed
branch
2 times, most recently
from
8f4bed8
to
1c0be80
Compare
|
Another question is whether this should lint against any reference to a field of a struct that has any level of packing, or if it should only lint when a reference is created to a field where the field's alignment is strictly greater than the struct's alignment? Although this question also applies to the |
|
It already does the smart thing here, as the |
|
Well, so far the "smartness" is that it never complains about references to 1-aligned types. There might be more we can do, but that seems rather orthogonal to the linting part. |
| @@ -229,6 +231,8 @@ fn mir_const(tcx: TyCtxt<'_>, def_id: DefId) -> Steal<Body<'_>> { | |||
| None, | |||
| MirPhase::Const, | |||
| &[&[ | |||
| // MIR-level lints. | |||
| &check_packed_ref::CheckPackedRef, | |||
| // What we need to do constant evaluation. | |||
There was a problem hiding this comment.
I don't even know what this means... I think this is a leftover from the pre-miri MIR based const evaluator. I think we could just merge the entire mir_const query into mir_validated
There was a problem hiding this comment.
Hm, maybe we can, I'll leave that to someone else.^^
If you change this, please remember to update the rustc-dev-guide, which is where I learned that this is not just for consts. :)
| @@ -229,6 +231,8 @@ fn mir_const(tcx: TyCtxt<'_>, def_id: DefId) -> Steal<Body<'_>> { | |||
| None, | |||
| MirPhase::Const, | |||
| &[&[ | |||
| // MIR-level lints. | |||
| &check_packed_ref::CheckPackedRef, | |||
There was a problem hiding this comment.
Looks like an alternative to this would be to add the lint here:
rust/src/librustc_mir_build/build/mod.rs
Line 184 in 31add7e
|
Our lint naming rules (https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints) say that we should have the "bad thing" in the name and use a plural, so |
|
Well this does not detect all unaligned references... |
RalfJung
force-pushed
the
lint-ref-to-packed
branch
from
1c0be80
to
c79535e
Compare
|
(I rebased so I wouldn't have to rebuild llvm to work on this again.) |
|
@oli-obk I ended up renaming the lint as you suggested. False negatives are okay for lints after all (and this lint will hopefully eventually become a hard error anyway -- to fix a soundness issue -- at which point it stops having a name). |
|
@bors r+ |
|
📌 Commit d959a8f has been approved by |
bors
added
S-waiting-on-bors
Rollup of 6 pull requests Successful merges: - rust-lang#72270 (add a lint against references to packed fields) - rust-lang#72294 (JS cleanup) - rust-lang#72342 (Warn about unused crate deps) - rust-lang#72401 (Use correct function for detecting `const fn` in unsafety checking) - rust-lang#72581 (Allow unlabeled breaks from desugared `?` in labeled blocks) - rust-lang#72592 (Update books) Failed merges: r? @ghost
Creating a reference to an insufficiently aligned packed field is UB and should be disallowed, both inside and outside of
unsafeblocks. However, currently there is no stable alternative (#64490) so all we do right now is have a future incompatibility warning when doing this outsideunsafe(#46043).This adds an allow-by-default lint. @retep998 suggested this can help early adopters avoid issues. It also means we can then do a crater run where this is deny-by-default as suggested by @joshtriplett.
I guess the main thing to bikeshed is the lint name. I am not particularly happy with "packed_references" as it sounds like the packed field has reference type. I chose this because it is similar to "safe_packed_borrows". What about "reference_to_packed" or "unaligned_reference" or so?