Add safety section for DisjointBitor::disjoint_bitor

[yilin0518]

This PR adds a # Safety section to the documentation for the unsafe fallback::DisjointBitOr::disjoint_bitor method.

The method previously referred readers to core::intrinsics::disjoint_bitor, whose documentation already describes the required safety invariant. This change copies that invariant into the fallback trait method documentation directly, using self and other to match the method parameters.

No behavior changes.

[rustbot]

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

[rustbot]

Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @clarfonthey (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from 6 candidates

[RalfJung]

What is the point of duplicating this comment? It will just inevitably go out of sync with the intrinsic.

[yilin0518]

@RalfJung said:

What is the point of duplicating this comment?

We are committed to ensuring that every unsafe API has a Safety Doc. For the pattern of impl trait for struct, we extract the safety doc from the trait method, by this way types that implement DisjointBitor share the same safety comment. So we add a safety section to this API. We look forward to your advice about our idea and the way to do it.

@RalfJung said:

It will just inevitably go out of sync with the intrinsic.

We notice that the current way that adding safety section to DisjointBitor::disjoint_bitor is duplicating and rises the cost of maintaining safety comments. (The safety comments become longer). But the trait method has different input arguments compared with super::disjoint_bitor. It may have better way to add safety comment, or we can just add a safety section like this:

    /// # Safety
    /// See [`super::disjoint_bitor`]; we just need the trait indirection to handle
    /// different types since calling intrinsics with generics doesn't work.
    unsafe fn disjoint_bitor(self, other: Self) -> Self;

The same, we expect your advice.

[RalfJung]

We are committed to ensuring that every unsafe API has a Safety Doc.

I understand that you want to help. However, this is a cross-cutting goal -- did you coordinate with the libs / libs-api team that they agree with this goal even for internal perma-unstable APIs?

It may have better way to add safety comment, or we can just add a safety section like this:

🤷 that's fine for me

[yilin0518]

@RalfJung said:

I understand that you want to help. However, this is a cross-cutting goal -- did you coordinate with the libs / libs-api team that they agree with this goal even for internal perma-unstable APIs?

We haven't notified libs / libs-api team about our initiative. We created this PR to hope the libs team will review our initiative and receive feedback.

[clarfonthey]

Who exactly is we here?

I understand the desire to include this, but I'm very tempted to close this and #158382 under the same justification that Compiler uses, under not accepting small changes to private documentation on their own: https://rustc-dev-guide.rust-lang.org/contributing.html#writing-documentation

Even though this is technically larger than a simple typo/grammar fix, it feels similar to just trying to check a box when that box really doesn't need to be checked. These traits purely exist for bounds on intrinsics, and while there is potentially some argument to be made in favour of making the trait bounds more closely tied to the functions themselves, I don't think we need to really duplicate the documentation.

But I'll wait for some more justification before I close this, in case there is genuinely some initiative I didn't know about.

[RalfJung]

We haven't notified libs / libs-api team about our initiative. We created this PR to hope the libs team will review our initiative and receive feedback.

That's not how this works, though I understand our processes can be confusing. But also, your PR description does not explain the initiative, so it is unclear to me how you imagined that review to happen.

I don't think either libs or libs-api have a process for proposals that are not of the form "add this new API". What I would suggest you do is to produce a document (something between half a page to a page) that explains the motivation and goal, cost and benefits of your initiative (like a mini-RFC), and then bring it up on Zulip in the t-libs channel. A typical place for such documents is a markdown file on https://hackmd.io/. Note that such a document should be human-written; it makes no sense to have an AI produce a text that is primarily meant to coordinate groups of people working together.

[clarfonthey]

To be fair, even though they're not explicitly worded this way, I think that an initiative of this kind would be best worded in an ACP (API Change Proposal) since those are effectively the ways the libs team tracks initiatives. You could also just file an issue in the libs-team repo if you want to talk about something to discuss.

I think that trying to do large refactors or safety documentation improvements are good, but this is not anything we've had planned and not really anything we're prepared to accept, especially when it could result in redundant documentation like displayed here.

Again, I would be interested in what group you're representing here and what interests they have in case there is something you'd like to discuss, but otherwise I don't think we can merge this PR as-is.

[yilin0518]

Who exactly is we here?

I'm a member of safer-rust, which audits the safety documentation of Rust standary library for now. Several months ago we proposed a Pre-RFC: Rust Safety Standard. In this Pre-RFC we discussed the standard of unsafe code documentation in detail, and you can refer to section 5: Rules for Traits to understand our goal and initiative.

We haven't provided an ACP or open a talk on Zulip, sorry for directly opening this PR! In addition to this Pre-RFC, we have also created our own project goal: rust-lang/rust-project-goals#511. Before this PR, we have submitted another PR related to intrinsic :#138309 , and it was successfully merged, so I chose to open this PR directly.

To fix my errors in the process, I think the following steps I can do:

1. Bring the rust-project goal and Pre-RFC to the Zulip, and contact the t-libs team.
2. Provide an ACP, which contains our goals, initiatives, solution, cost and so on.
3. If Adding safety section to unsafe API is approved, we can change our PR to simply add a # Safety

What's more, thank you for @RalfJung and @clarfonthey. Your advice are valuable for us!

[clarfonthey]

This makes a lot more sense, and was completely unaware you had an outstanding project goal. Admittedly some of this stuff is harder to track since the libs team is a bit segmented right now (most of the reviewers are just libs-contributors, who aren't 100% in on all the larger libs team decisions).

For now, I think that linking the project goal in PRs might be a good way to help bring visibility to the effort, since it would provide a nice way to help reviewers like me who aren't immediately aware of the effort to be able to read up on it. I think also that at least outlining some of the future work in an ACP would be the best way to bring greater visibility to the team, and we can decide on a few tracking issues that will be better places to link for the effort and the specific initiatives you'd like to do.

Here, I think that the fact that the trait and the intrinsic aren't necessarily linked is a weird issue and I'm not 100% sure how to resolve it. Since intrinsic fallbacks are kind of a library thing, maybe it's something we should discuss more to help ensure that we still document things everywhere without having to explicitly duplicate strings. There could potentially be macros as well we make that could assist with this.

Also posting on Zulip is appreciated as well, since it's a great way to just quickly notify team members. If you made a thread in the t-libs channel linking back to this work, it would help bring more visibility, but isn't strictly required.

Thank you for your work!

On this change, I think the best act for now is to close this until we have a better idea on how to deal with duplicate documentation on intrinsic fallbacks. I agree that we'd ideally have safety documentation everywhere, but right now, just duplicating things isn't very helpful.

[yilin0518]

Thanks for your advice! We will push our goal forward, also we will try to create an ACP and open a talk on Zulip to contact the lib team. We are working hard to improve Rust.