-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
unsafe attributes #3325
unsafe attributes #3325
Conversation
afb2e6c
to
263a510
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please! :)
text/0000-unsafe-attributes.md
Outdated
- **Unsafe attributes on statements.** For now, the only unsafe attributes we | ||
have don't make sense on the statement level. Once we do have unsafe statement | ||
attributes, we need to figure out whether inside `unsafe {}` blocks one still | ||
needs to also write `unsafe(...)`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have to settle this. You can put declarations inside a block within a function, including AFAIK an unsafe block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A fn
inside of an unsafe
block inside of another fn
doesn't "inherit" the unsafe block over its body, so it would be most logical for it to also not inherit the unsafe block over its attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, will add a note on that.
text/0000-unsafe-attributes.md
Outdated
`unsafe { ... }`.) | ||
- **Unsafe derive.** We could use `#[unsafe(derive(Trait))]` to derive an | ||
`unsafe impl` where the deriving macro itself cannot check all required safety | ||
conditions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to bikeshed something that doesn't need to be in this RFC. But I do think this is the wrong syntax. unsafe used like that should be "unsafe to derive", not "derives an unsafe trait".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, the intention is that this is an example for "unsafe to derive". Note that the text says "the deriving macro itself cannot check all required safety conditions".
I don't think that this RFC offers much for library developers. Normally, when you write a library and use an However, say that a library needs to export a function under a particular name. The library author needs to use either The same goes for If you write an The RFC makes unsafe attribute usage more "grep-able", but not actually less unsafe or less error prone. This doesn't give a library author the ability to communicate to a user, "I'm doing this unusual thing, here's what you need to do on your end to make sure things work out". The RFC doesn't prevent UB at all. It just follows the absolute letter of the rust law that says "if there's UB then someone must have used unsafe", and now we can point to the literal ascii string having appeared in some file somewhere. |
"More greppable" is largely the point. While the specific attributes ( With regards to usage by library authors, you should think about it like But I think @Lokathor 's objection can be rephrased in the following way: with |
I absolutely would not say that about |
It sounds like the missing step there is the ability to mark your crate as unsafe /// SAFETY: We require users of this crate to promise there is no other global `write` symbol
#[unsafe(no_mangle)]
pub fn write() {} This provides something similar to item-level |
Both of these examples are the latter. You are discharging the unsafety of the attribute or the derive. There is no way existing or with this RFC to do the former. |
The other thing is that if the compiler handled it then we could make no_mangle a safe attribute, but we're just not doing that. |
That’s quite impossible if you’re statically linking any C code or other precompiled objects into the executable, since the compiler doesn’t know what names that code might define or depend on. And on some platforms (Linux), symbol definitions can leak even across dynamically linked libraries. |
Well the compiler could scan at least all objects it knows go into the linking, even if they're written in non-rust. But if symbols can leak in even after the initial linking then not only is there no hope to make no_mangle fully safe, but there's clearly also no hope you could ever discharge your safety obligation within a library. So I think that supports my point that just putting the string "unsafe" next to no_mangle is not itself improving things by much. Idea for a separate RFC: Perhaps cargo could generate a list of all the unmangled symbols all your deps are bringing in so that you can check the list? |
That is just fundamental in the nature of these attributes -- they can only be checked on the whole-program "post-linking" level. There is no way to soundly use any of them in a library. The RFC doesn't change that, it just makes this existing fact more clear. I have amended the text to clarify that. |
It would not cover all cases, so you would still need to mark them as unsafe. I think such a scan would be orthogonal to the RFC: it could be performed (or not) regardless of whether we get unsafe attributes.
Yeah, but since there is no way to make it happen in the general case, at least this one should be marked as such. Of course, that doesn't mean you cannot add new, safe attributes in the future for similar use cases.
The generated docs of |
@rfcbot merge |
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
@rfcbot reviewed
Not a blocking concern since this is forward-compatible, but I don't see how supporting safe attributes makes it difficult to support non-builtin unsafe attributes in the future. I suppose nesting could be problematic if we support nesting under arbitrary unknown attributes. |
I really don't know what is hard or easy with proc macro attributes and things like that, so after the first version of this PR (which was very liberal with where |
text/0000-unsafe-attributes.md
Outdated
- **Other syntax.** Obviously we could pick a different syntax for the same | ||
concept, but this seems like the most natural marriage of the idea of unsafe | ||
blocks from regular code, and the existing attributes syntax. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pondering: this leans more into the use of unsafe
as the keyword to discharge the safety obligation.
Would this be a place to add the hold_my_beer
(not under that name) term that is for discarding specifically, to leave unsafe
as the marker for introducing obligations?
Or would it be too weird to have the different word here but not in the other places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or would it be too weird to have the different word here but not in the other places?
Yeah that feels pretty weird to me. If we want to use another keyword for this use of unsafe we should do it consistently.
🔔 This is now entering its final comment period, as per the review above. 🔔 |
@rfcbot concern maybe-make-this-part-of-next-edition During T-lang triage, I posed the question of whether we should not push to land this change across all Rust editions, and instead have it only become a change on e.g. Rust 2024. If we do want this to be coupled to the edition boundary, then it would be good for the RFC to spell out how it is coupled to the edition. (E.g. maybe it is always warn-by-default for Rust editions < 2024, but is a hard error to leave out the |
Since this RFC has reached the "syntax" stage of discussion, maybe someone else should take over -- I don't have the patience required for these discussions. Once there is a consensus, I'm happy to edit the RFC. I don't care what the syntax is. I'm just convinced we need this concept of unsafe attributes. |
For lack of any better idea, I created a discussion with poll to try to estimate the general feel. https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Unsafe.20attribute.20syntax/near/424767855 (personally I still feel there is little convincing reason to differ from the parentheses syntax currently in this RFC - it is consistent with |
I am confused now, you think it should use |
Whoops, I missed a very important "feel there is little convincing reason" keyword 🙂 |
Here is a technical question that might give a reason beyond subjective readability to pick a syntax: Is
|
Well, currently parentheses has a pretty big lead 23-3-3. I don't think anyone likes making decisions by poll, but that does seem to be a reasonable enough level of consensus to pass this on to the lang team, barring any technical/grammar issues that have been brought up.
Great concern - but to create an attribute macro you would need to create a function of the same name, which you cannot do. I guess that is just keyword vs. more or less a builtin function with name scope. extern crate proc_macro;
use proc_macro::TokenStream;
#[proc_macro]
pub fn unsafe(_: TokenStream) -> TokenStream { todo!() }
mod unsafe {
macro_rules! unsafe { ()=>() }
}
|
Technically, you can use the |
Given all the support the parenthesis option has, and the opposition to some of the other options, and the impending edition cutoff, I'm going to resolve this concern. I no longer think we should block on this. If another team member would like to raise a blocking concern, please go ahead. @rfcbot resolve syntax-not-ideal |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
I have updated the filename to match the RFC number. |
The FCP has completed for RFC 3325. Let's prepare it to be merged.
The lang team has accepted this RFC, and we've now merged it. Thanks to @RalfJung for pushing this work forward, and thanks to all those who reviewed this RFC and contributed helpful feedback. For further updates on this work, follow the tracking issue: |
…r, r=nnethercote Stabilize `unsafe_attributes` # Stabilization report ## Summary This is a tracking issue for the RFC 3325: unsafe attributes We are stabilizing `#![feature(unsafe_attributes)]`, which makes certain attributes considered 'unsafe', meaning that they must be surrounded by an `unsafe(...)`, as in `#[unsafe(no_mangle)]`. RFC: rust-lang/rfcs#3325 Tracking issue: rust-lang#123757 ## What is stabilized ### Summary of stabilization Certain attributes will now be designated as unsafe attributes, namely, `no_mangle`, `export_name`, and `link_section` (stable only), and these attributes will need to be called by surrounding them in `unsafe(...)` syntax. On editions prior to 2024, this is simply an edition lint, but it will become a hard error in 2024. This also works in `cfg_attr`, but `unsafe` is not allowed for any other attributes, including proc-macros ones. ```rust #[unsafe(no_mangle)] fn a() {} #[cfg_attr(any(), unsafe(export_name = "c"))] fn b() {} ``` For a table showing the attributes that were considered to be included in the list to require unsafe, and subsequent reasoning about why each such attribute was or was not included, see [this comment here](rust-lang#124214 (comment)) ## Tests The relevant tests are in `tests/ui/rust-2024/unsafe-attributes` and `tests/ui/attributes/unsafe`.
… r=nnethercote Stabilize `unsafe_attributes` # Stabilization report ## Summary This is a tracking issue for the RFC 3325: unsafe attributes We are stabilizing `#![feature(unsafe_attributes)]`, which makes certain attributes considered 'unsafe', meaning that they must be surrounded by an `unsafe(...)`, as in `#[unsafe(no_mangle)]`. RFC: rust-lang/rfcs#3325 Tracking issue: rust-lang#123757 ## What is stabilized ### Summary of stabilization Certain attributes will now be designated as unsafe attributes, namely, `no_mangle`, `export_name`, and `link_section` (stable only), and these attributes will need to be called by surrounding them in `unsafe(...)` syntax. On editions prior to 2024, this is simply an edition lint, but it will become a hard error in 2024. This also works in `cfg_attr`, but `unsafe` is not allowed for any other attributes, including proc-macros ones. ```rust #[unsafe(no_mangle)] fn a() {} #[cfg_attr(any(), unsafe(export_name = "c"))] fn b() {} ``` For a table showing the attributes that were considered to be included in the list to require unsafe, and subsequent reasoning about why each such attribute was or was not included, see [this comment here](rust-lang#124214 (comment)) ## Tests The relevant tests are in `tests/ui/rust-2024/unsafe-attributes` and `tests/ui/attributes/unsafe`.
Consider some attributes 'unsafe', so that they must only be used like this:
#[unsafe(no_mangle)]
Rendered
Prior discussion on IRLO