Make Any an unsafe trait

[Mark-Simulacrum]

Follow-up to #67519 per popular request that making it an unsafe trait is actually desirable.

[rust-highfive]

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

cc @Centril

I think we might want to at least FCP this with the libs team (since it does look different in public docs) but I don't personally think that a crater run here is necessary, we don't expect any regressions and if they do happen then we can catch them in beta.

[Centril]

Imo this is just a bug-fix so I wouldn't bother with FCP but that's just me.

[Mark-Simulacrum]

It is at a high level just a bug fix but it would also be the first major unsafe trait that we expose (joining primarily TrustedLen). I think as such we should be a little careful here, but if e.g. @dtolnay as the auto-assigned reviewer deems this good to go then we can proceed without FCP of course.

[dtolnay]

I can't say that I am excited about this. I follow the justification, but I disagree with it on the basis that the boundary at which Rust is designed to contain unsafe code is at the module level. The std::any module uses careful library design to perfectly contain its unsafe internals in a way that can't be misused outside the module. From the point of view of library design at the module level, Any as a safe trait is correct and I am happy to encourage libraries that follow a similar design to also document such traits as safe traits.

I think whatever teaching benefits there are to publicly associating the Any trait with unsafety are far less than the downsides of publicly associating the Any trait with unsafety.

Thanks anyway!

@rfcbot fcp close

[rfcbot]

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

• @Amanieu
• @KodrAus
• @SimonSapin
• @alexcrichton
• @dtolnay
• @sfackler
• @withoutboats

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!

See this document for info about what commands tagged team members can give me.

[dtolnay]

To give an example on why the library design perspective on this differs from the comments on #67519 (review) which are from the language perspective: suppose the trait in question were:

pub trait Example {}
impl<T: ?Sized> Example for T {}

and we were adding a private implementation detail which is unsafe to implement:

pub trait Example {
    // unsafe to implement
    #[doc(hidden)]
    fn f();
}

Why should this affect anything in the public facing documentation of the trait? The function is an implementation detail while the the docs are for documenting the public API, which has not changed. The logic is the same for Any.

[Mark-Simulacrum]

I am fairly ambivalent, I can definitely see both downsides and benefits to either approach. The "private" hidden method is convincing, though. It might be worth asking folks to take a look at the (just added) reasoning comment this PR removes as well, to see if anyone has changes to suggest there.

[RalfJung]

This makes it sound like there'd be something odd here, but this is entirely normal for unsafe traits: they are unsafe to implement but safe to use.

[RalfJung]

I can't say that I am excited about this. I follow the justification, but I disagree with it on the basis that the boundary at which Rust is designed to contain unsafe code is at the module level. The std::any module uses careful library design to perfectly contain its unsafe internals in a way that can't be misused outside the module.

I would argue it is bad style to have a private safe function that ought to be unsafe because it is not actually safe to call under all situations. I would not accept such code in my own projects.

"safety works at the module level" means that when verifying that something is safe, we have to consider all code inside the module. It does not mean that we should stop caring about properly marking things as unsafe within a module.

The same reasoning applies here.

[petertodd]

The unstable TrustedLen trait puts the notes re: trait guarantees under a "Safety" header; I noticed there doesn't seem to be much guidance on how to document unsafe traits.

How about we mark these notes as "Unsafe Guarantees" or "Safety Guarantees" and word it something along the lines of:

Unsafe Guarantees

Unsafe code can rely Any implementations returning a stable TypeId that is unique for the type. This guarantee is why the Any trait is marked unsafe; normally unsafe code may not rely on traits being implementing correctly for memory safety. Having said that because Any is implemented for all types, it currently isn't actually possible for Any to be implemented outside of the standard library.

Similarly, we could word TrustedLen along the lines of:

Unsafe Guarantees

Unsafe code may rely on the .size_hint's upper bound being correctly implemented for memory safety.

I looked around and it doesn't look like there's an accepted standard for how to document unsafe traits. Clippy for instance doesn't have any lints related to unsafe traits for instance.

[petertodd]

It is at a high level just a bug fix but it would also be the first major unsafe trait that we expose (joining primarily TrustedLen).

Actually, there's two very important unsafe traits that we expose: Send and Sync.

[withoutboats]

I don't really care what we do here, I agree in principle that Any should be marked unsafe, but that has no impact on users since users cannot implement it, and its more likely to just confuse people than anything so it doesn't seem great.

This is a good example of how the trait system is leaky around implementation details.

[withoutboats]

what I want to see is:

1. Internally, Any is marked as unsafe, so the code is pedantically correct.
2. In rustdoc, Any is shown as impossible for users to implement, rather than unsafe to implement. This would depend on having sealed traits as a feature or rustdoc being able to figure out that the impls of Any make it impossible to add another impl.

[petertodd]

On Wed, Jan 08, 2020 at 06:45:22AM -0800, Saoirse Shipwreckt wrote: I don't really care what we do here, I agree in principle that Any should be marked unsafe, but that has no impact on users since users cannot implement it, and its more likely to just confuse people than anything so it doesn't seem great. This is a good example of how the trait system is leaky around implementation details.

Re: "no impact on users", I actually disagree there: it has an impact on downstream code that wants to rely on `type_id` being implemented correctly in otherwise unsafe ways. It's not a common case - usually you'd just use the provided `downcast` methods - but it does happen and I've written such code myself.

[withoutboats]

you can rely on type_id being implemented correctly for the same reason the other code in the Any module can rely on that

[petertodd]

On Wed, Jan 08, 2020 at 06:55:15AM -0800, Saoirse Shipwreckt wrote: you can rely on type_id being implemented correctly for the same reason the other code in the Any module can rely on that

You mean, I can rely on it because it has a blanket implementation that we assume to be correct? My thinking on that is basically, I don't know the rules of specialization and other things enough to be 100% sure that's actually always true and will be in the future (note that unfortunate `Error::type_id` issue). OTOH, it being an unsafe trait like `Send` and `Sync` makes the logic pretty easy: I can trust `Any` to be implemented correctly. FWIW, by default Rustdoc hides the actual declaration, so user's aren't even going to see the "unsafe" keyword by default.

[withoutboats]

You mean, I can rely on it because it has a blanket implementation that we assume to be correct?

yes. if that were not true the entire API would be unsound.

[petertodd]

On Wed, Jan 08, 2020 at 07:08:56AM -0800, Saoirse Shipwreckt wrote: > You mean, I can rely on it because it has a blanket implementation that we assume to be correct? yes. if that were not true the entire API would be unsound.

Yes, without being marked as unsafe. But if marked as unsafe, it'd actually be fine if users implemented `Any` themselves (eg maybe if a future iteration of specialization ever allowed it). In one of my projects it'd be much easier if there was some way to get a `TypeId` for a !static, even if that `TypeId` had no lifetime information attached. What can I say, both API directions are valid ones. But to me an unsafe trait feels less magical than being a pseudo-compiler-builtin.

[dtolnay]

+1 for something along the lines of #67562 (comment). I am on board with Any requiring unsafe impl inside of libcore as long as rustdoc shows it as safe but unimplementable rather than an unsafe trait. It would be great to get an issue filed to track this.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[Mark-Simulacrum]

I will look into filing an issue to that effect, though I believe it (as @withoutboats notes) would require some notion of sealed traits. I don't think it needs it as a end-user exposed feature, though, possibly just a #[rustdoc_sealed] annotation or something like that, so I imagine we might be able to do that without RFC and so forth.

[Mark-Simulacrum]

I have filed #68235 and am closing this out (I guess in theory we could wait for FCP, but I don't think there's much point. If folks feel otherwise, feel free to reopen).