enforce unsafe attributes in pre-2024 editions by default #139718
Conversation
|
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
rustbot
added
S-waiting-on-review
| const SAFE_BEFORE_RUST_2024: &[Symbol] = | ||
| &[sym::export_name, sym::link_section, sym::no_mangle]; |
There was a problem hiding this comment.
based on https://github.com/rust-lang/rust/blob/master/compiler/rustc_feature/src/builtin_attrs.rs, I believe these are the only 3 attributes that changed their unsafety in the 2024 edition and were stable at that point.
This comment has been minimized.
This comment has been minimized.
folkertdev
force-pushed
the
unsafe-attributes-earlier-editions
branch
from
df25f1b to
a1fb6f3
Compare
This comment has been minimized.
This comment has been minimized.
folkertdev
force-pushed
the
unsafe-attributes-earlier-editions
branch
from
a1fb6f3 to
731a653
Compare
| const SAFE_BEFORE_RUST_2024: &[Symbol] = | ||
| &[sym::export_name, sym::link_section, sym::no_mangle]; |
There was a problem hiding this comment.
Instead of hard-coding this, you could instead modify AttributeSafety. For example, making its Unsafe variant hold { since: Option<Edition> }. However, I don't know how large the ripple effect would be in terms of LOCs.
There was a problem hiding this comment.
On a second look, it should be quite easy to do and not affect much. You'd just need to modify the "unsafe" macro matchers of macro gated in mod builtin_attrs to optionally take an edition, maybe via unsafe $(($edition:ident..))?, so we can mark these attributes as unsafe(Rust2024..) (or whatever other syntax seems more legible to you (unsafe(since Rust2024)), unsafe(>= Rust2024), …).
There was a problem hiding this comment.
allright, gave that a go. I don't think using $(....)? is really doable here, because you can't handle the case where the value is unspecified well (there are workarounds, but I think here just writing out an extra case is simpler).
There was a problem hiding this comment.
Fair. I mean you'd just need to create two helper macro rules but it's okay to delay that until we can no longer deal with the combinatorial explosion of (non-helper) macro matcher arms.
the `no_mangle`, `link_section` and `export_name` attributes are exceptions, and can still be used without an unsafe in earlier editions
folkertdev
force-pushed
the
unsafe-attributes-earlier-editions
branch
from
731a653 to
f472cc8
Compare
| // Attributes can be safe in earlier editions, and become unsafe in later ones. | ||
| let emit_error = match unsafe_since { | ||
| None => true, | ||
| Some(unsafe_since) => attr.span.edition() >= unsafe_since, | ||
| }; |
There was a problem hiding this comment.
This is equivalent to Some(attr.span.edition) >= unsafe_since but that is very subtle so I wrote out the logic here. In general I don't love using Option here, but a custom enum would add a bunch of extra code and comes with its own naming problems.
| const SAFE_BEFORE_RUST_2024: &[Symbol] = | ||
| &[sym::export_name, sym::link_section, sym::no_mangle]; |
There was a problem hiding this comment.
allright, gave that a go. I don't think using $(....)? is really doable here, because you can't handle the case where the value is unspecified well (there are workarounds, but I think here just writing out an extra case is simpler).
fmease
changed the title
There was a problem hiding this comment.
Thanks for having applied my feedback! Making rustc more flexible in this regard (& more strict in what it accepts) definitely makes a lot of sense.
That said, having gone over the list of attributes, I'm confused as to why #[ffi_{pure,const}] need to be marked unsafe in the first place (which is preexisting). I know of this table: #124214 (comment). Since both attributes can only be applied to foreign functions, the surrounding extern { … } (unsafe extern { … } in Rust >=2024) "should be enough" according to the very same table (see reasoning for link, link_ordinal, link_name staying "safe").
I guess I have to create Zulip discussion and ask people who are more in the loop here.
|
There is some info on that starting here #58329 (comment) |
|
Okay right, I see. I won't block this then. @bors r+ rollup |
bors
added
S-waiting-on-bors
Rollup of 9 pull requests Successful merges: - rust-lang#138336 (Improve `-Z crate-attr` diagnostics) - rust-lang#139636 (Encode dep node edge count as u32 instead of usize) - rust-lang#139666 (cleanup `mir_borrowck`) - rust-lang#139695 (compiletest: consistently use `camino::{Utf8Path,Utf8PathBuf}` throughout) - rust-lang#139699 (Proactively update coroutine drop shim's phase to account for later passes applied during shim query) - rust-lang#139718 (enforce unsafe attributes in pre-2024 editions by default) - rust-lang#139722 (Move some things to rustc_type_ir) - rust-lang#139760 (UI tests: migrate remaining compile time `error-pattern`s to line annotations when possible) - rust-lang#139776 (Switch attrs to `diagnostic::on_unimplemented`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#139718 - folkertdev:unsafe-attributes-earlier-editions, r=fmease enforce unsafe attributes in pre-2024 editions by default New unsafe attributes should emit an error when used without the `unsafe(...)` in all editions. The `no_mangle`, `link_section` and `export_name` attributes are exceptions, and can still be used without an unsafe in earlier editions. The only attributes for which this change is relevant right now are `#[ffi_const]` and `#[ffi_pure]`. This change is required for making `#[unsafe(naked)]` sound in pre-2024 editions.
New unsafe attributes should emit an error when used without the
unsafe(...)in all editions.The
no_mangle,link_sectionandexport_nameattributes are exceptions, and can still be used without an unsafe in earlier editions. The only attributes for which this change is relevant right now are#[ffi_const]and#[ffi_pure].This change is required for making
#[unsafe(naked)]sound in pre-2024 editions.