Unsafe derives and attributes

[joshtriplett]

Allow declaring proc macro attributes and derive macros as unsafe, and
requiring unsafe to invoke them.

Rendered

[clarfonthey]

Just my 2¢, but I think that the shorthand for derive(..., unsafe(Trait), ...) is a bit unprecedented.

You have to separate out derive traits any time there's some different requirement, e.g. cfg_attr(feature = "serde", derive(Serialize, Deserialize)), and by keeping unsafe at the top level you can easily grep for #!?\[unsafe\( whereas unsafe( will catch any call to method_unsafe(.

Sure, it's likely it won't make a difference, but I think that only having to check attributes at the top level for unsafe to verify safety is best.

[carbotaniuman]

The greppability is already broken by things like #[cfg_attr(all(), unsafe(no_mangle)], and can be restored by just grepping unsafe( I think putting the unsafe with the trait makes syntactic sense as it discharges the safety obligation of each trait derive.

[clarfonthey]

The greppability is already broken by things like #[cfg_attr(all(), unsafe(no_mangle)], and can be restored by just grepping unsafe( I think putting the unsafe with the trait makes syntactic sense as it discharges the safety obligation of each trait derive.

I feel kind of silly for literally alluding to this point in my post and missing it somehow. You're right and I retract my original claim.

[joshtriplett]

We had a @rust-lang/lang design meeting today on the set of macro RFCs. I've updated the RFC to incorporate the feedback from that design meeting.

Per the feedback in that meeting, I'm starting an FCP to start letting people register consensus for this RFC.

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

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.
See this document for info about what commands tagged team members can give me.

[joshtriplett]

@rust-lang/lang I've updated this to match the consensus syntax of derive(unsafe(UnsafeTrait)), and provided all the points of rationale that I remember from the discussion. Please feel free to post further suggestions if I missed any points of rationale.

[carbotaniuman]

One thing I thought of was that it might make sense to make a derive maybe-unsafe and branch between whether unsafe was provided, although it might be better to just spell that out as 2 separate derives... The use case for that would be something where the derive can check the preconditions using static_asserts or similar, and one where you can promise the preconditions hold if they aren't checkable.

[RalfJung]

One thing I thought of was that it might make sense to make a derive maybe-unsafe and branch between whether unsafe was provided

I think that would be a big mistake. The presence or absence of unsafe should never affect the resulting generated code, it should only affect whether the code is accepted or not.

[RalfJung]

@rust-lang/lang I've updated this to match the consensus syntax of derive(unsafe(UnsafeTrait)), and provided all the points of rationale that I remember from the discussion. Please feel free to post further suggestions if I missed any points of rationale.

In terms of the mental model I find derive(unsafe(Trait)) a bit odd -- this looks like the trait is called unsafe(Trait). In the grammar of attributes (well, in its simplified form in my head), unsafe($attr) is also an $attr, and derive($trait) is an $attr, but to fit the proposed scheme into that model we have to do an ad-hoc extension of the grammar to allow unsafe inside derive.

OTOH, the RFC does make some good points in favor of "inner unsafe", so I am a bit torn here.

[traviscross]

I don't understand that distinction...

That distinction is tied exactly to:

But good point that the name in the derive is a macro name, not a trait name.

That is, I agree that if derive were itself doing a code transformation producing obligations for the user to uphold, that it should be unsafe. But it's actually the derive macro that's doing that. So derive(..) is kind of just a context marker that allows us to invoke one or more derive macros.

Putting that all together, we could say that, within attributes, we have to be in a derive(..) context to invoke derive macros, some of which might be unsafe, and those have to be so marked.

[kennytm]

While to the proc_macro author in #[derive(Clone, Copy)] struct Foo { ... } it may be understood as invoking the "macros" Clone and Copy on struct Foo { ... }, to an average user it is highly unintuitive to think like that.

We would think

1. #[derive(Clone, Copy)] is just a short hand for #[derive(Clone)] #[derive(Copy)] and
2. the derive and the trait (ok, macro) name is a single unit to drive the code generation.

So #[unsafe(derive(UnsafeTrait))] makes more sense to me, which the unsafe is applied the entirety of derive(UnsafeTrait), not the word derive alone.

[traviscross]

Suggested change

	If writing code that enforces `SAFETY` comments for every use of `unsafe`, you
	can write the `SAFETY` comment immediately prior to the attribute:
	You can write the `SAFETY` comment immediately prior to the attribute:

"What, you mean there's code somewhere that doesn't require that?" I'd prefer to not hint that this expectation is somehow not the default. Probably we can just leave this predicate out entirely.

[joshtriplett]

@traviscross It is, in a very practical sense, not currently the default: there's no warn-by-default lint for it. I'm all for changing that, but meanwhile, it'd be inaccurate to imply that it is currently required everywhere.

[traviscross]

Josh and I talked, but for others here, by wanting to remove the predicate, I don't mean to suggest by this language that it is the default or is required.

If we say, "you can write X here," I don't think that implies that you must write "X" at all. I read it instead as, "if you're going to write an X, which is entirely your option as far as we're concerned, you can write it here."

What I don't want to suggest is that writing such safety comments is unusual, and the fussy language referencing extrinsic requirements for doing this makes it feel that way to me.

I proposed wording along the lines of "when adding a safety comment, you can write it here" that both Josh and I liked, and that I think he's planning to update the text to reflect.

[traviscross]

Let's cancel the stale FCP and restart...

@rfcbot fcp cancel

[rfcbot]

@traviscross proposal cancelled.

[traviscross]

This all looks correct and desirable to me with one amendment, which I'm including in this proposed FCP, which is that we change the def site for derive macros to match the use site, and write the unsafe(..) around the derive macro name in the proc_macro_derive attribute, e.g.:

#[proc_macro_derive(unsafe(DangerousDeriveMacro))]
pub fn derive_helper_attr(_item: TokenStream) -> TokenStream {
    TokenStream::new()
}

(For me, that the use site syntax we chose allows for this symmetry is yet another reason to prefer it.)

@rfcbot fcp merge

[rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• pick-def-site-name (Unsafe derives and attributes #3715 (comment))

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.
See this document for info about what commands tagged team members can give me.

[joshtriplett]

@traviscross As mentioned by @carbotaniuman and myself in response to that suggestion: that makes it look like something that's unsafe to define rather than unsafe to use. For that reason, I don't think that syntax is an improvement.

We've already set the precedent, with unsafe attributes, that unsafe(...) is what an unsafe block looks like in an attribute context.

I could imagine making the case that this should be written proc_macro_derive(unsafe DangerousDeriveMacro) (by analogy with unsafe fn), but not proc_macro_derive(unsafe(DangerousDeriveMacro)).

I've listed an alternative for this, along with rationale for why we shouldn't go with that alternative.

[traviscross]

Let's use a concern to mark the discussion we need to have on def-site syntax.

@rfcbot concern pick-def-site-name

(Edit: I meant to type pick-def-site-syntax, but it's OK.)

[traviscross]

We'll need to discuss it on a lang call regardless, and I'm rather curious to hear what others on the team think. While I'm thinking about it, though, here are my notes on the case for symmetry between the def site and the use site. (@joshtriplett and I talked these points over, and our interesting and fruitful discussion helped to sharpen them for this quick write-up. These are my views only though.)

First off, I agree that it's a bit of a pun, but I think it's a kind of attractive pun in this case. What pushes me over the line is that I just can't imagine that, having picked unsafe(Macro) at the use site, that we'd ever use unsafe(Macro) at the def site to mean something else. If I thought we were seriously considering some alternative future meaning for that, I'd raise a concern about using unsafe(Macro) at the use site.

Broadly, I think it's fine, given how things are today, if define(unsafe(Thing)) creates an obligation and use(unsafe(Thing)) discharges it. If we're unhappy with that, probably the problem is the reuse of the keyword, not the reuse of how that keyword is placed. One thing that makes me happy with it here is that I just don't see any other plausible meaning for define(unsafe(Thing)). The Thing doesn't exist yet -- we're defining it. There's no way we could be discharging an obligation for it yet. So we must be creating an obligation, just like we are in other places that we're defining something using unsafe, e.g. in unsafe fn.

I'm reminded here too about our recent syntax discussions for const trait impls. We may not end up marking them at all -- that's what the current RFC draft says -- but when we were thinking of doing that, we had to pick between const impl Trait for .. and impl const Trait for ... There are a lot of good reasons to like const impl Trait, e.g. in terms of syntactic and conceptual parity with const fn and other uses. But people really liked impl const Trait for .. because it matched the T: const Trait bound syntax. We could say here too that this is just a pun as these are different. One "requires a const impl" and the other "provides a const impl". And yet, it seemed attractive to lean into this pun. That's roughly similar to how I feel here, mapped to "discharging a proof obligation" and "creating a proof obligation".

(The parity we try to maintain between our type syntax and constructor expression syntax is maybe also a place where we could say that it's a bit of a pun between two distinct things, but that we like it.)

Another angle is to imagine what we'd do if we didn't have this feature. Probably we'd still mark these unsafe derive macros by naming them things like UnsafeThing. Then the def site would look like define(UnsafeThing) and the use site like use(UnsafeThing). So in that way, define(unsafe(Thing)) and use(unsafe(Thing)) feel equally fine to me. It's like we're just binding the unsafety to the identity of the derive macro.

(Here and throughout, I'm using define(..) and use(..) to abstract the question a bit, and to avoid having to repeat proc_macro_derive(..) everywhere.)

Regarding the fact that unsafe(..) in attributes today only discharges obligations, what I'd point out is that we only very recently shipped unsafe attributes at all, so I don't assign much in the way of precedent value. It doesn't surprise me one bit that we started by only using the syntax one way, and I don't think that precludes us from using it in another way, in a different context, if it's appropriate, well motivated, and sufficiently unambiguous.

One reason that I proposed this change is that putting the unsafe keyword somewhere arbitrary in an argument list -- as with define(Thing, attributes(..), unsafe) -- just doesn't speak to me strongly enough that we're creating an unsafe obligation. Everywhere else that we do this, it comes in front and is paired closely with the name of what we're defining that carries that obligation. So probably I'd like to see us address that somehow. I had thought too about define(unsafe Thing), and that'd be OK if others don't like the proposed pun, but the arguments above cause me to still lean more toward the symmetric answer.

[RalfJung]

We're already using unsafe both to mark the point of introducing unsafe obligations and of discharging them. This is a point that often trips people up (it is on the short list of things I would change about Rust 1.0 if I could). For the existing cases at least there's fairly short list of you have to learn to know which is which. The proposal to use unsafe(Trait) for both cases means that in the future one will have to know exactly how all attributes use unsafe to understand which of them introduces vs discharges obligations -- that does not seem great.

[programmerjake]

it just occurred to me that we may want to mark derive macro helper attributes unsafe instead of (or in addition to) the derive macro:

#[proc_macro_derive(MyDebug, attributes(unsafe(debug_deref)))]
fn my_debug ...

// no `unsafe(MyDebug)` here since it's only unsafe
// when you need to deref pointers, which not all structs have.
#[derive(MyDebug)]
pub struct FooPtr<T> {
    // SAFETY: ptr points to a valid memory chunk allocated with `malloc`
    #[unsafe(debug_deref)]
    ptr: NonNull<T>,
    bar: usize,
}

[traviscross]

Excellent point. We'll take that up with the nomination as well.

[kennytm]

Note that as of 1.87 as long as you insert a single comment between any item, rustfmt will automatically convert to the style to put everything in a single line.

#[derive(
    Copy, Clone, Eq, Hash, PartialEq, Ord, PartialOrd, Debug,
    // SAFETY: demo
    unsafe(FromRawFd),
)]
struct Foo(RawFd);

// ↓ after rustfmt ↓

#[derive(
    Copy,
    Clone,
    Eq,
    Hash,
    PartialEq,
    Ord,
    PartialOrd,
    Debug,
    // SAFETY: demo
    unsafe(FromRawFd),
)]
struct Foo(RawFd);

if you want to keep this within 3~5 lines and rustfmt-stable you need to split the safe and unsafe parts anyway.

#[derive(Copy, Clone, Eq, Hash, PartialEq, Ord, PartialOrd, Debug)]
// SAFETY: demo
#[derive(unsafe(FromRawFd))]
struct Foo(RawFd);

[tmandry]

The motivation section is really short. Even though this seems like an "obvious" extension, I would appreciate it if @joshtriplett and others could add concrete examples of derives that would use this. Until then further bikeshedding on the spelling seems premature.