Make core::ops::Place an unsafe trait

[cramertj]

Consumers of Place would reasonably expect that the pointer function returns a valid pointer to memory that can actually be written to.

[rust-highfive]

r? @KodrAus

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

[KodrAus]

I think that's a totally reasonable constraint 👍

If we only require that all returned pointers are non null, what do you think about using the NonNull (formally Shared) type from #46952 and leaving the function signature as safe? That might capture the contract a bit more clearly.

[bluss]

@KodrAus That does not in guarantee that the pointer can be dereferenced, so it's not enough.

[bluss]

To stave off misunderstanding, maybe this should say "must return a valid pointer". Since non-null is not the end of the validity requirements.

[shepmaster]

the end of the validity requirements

Are those requirements listed somewhere where we can refer to?

[cramertj]

The wording is actually kind of redundant as-is. The real requirement is "This function must return a pointer that a value of type Data can be written through." The specific requirements regarding aliasing and soundness are sort of floaty without unsafe code guidelines, but there's probably some kind of aliasing requirement in there, too 😄

[bluss]

I agree, it's redundant.

[KodrAus]

So I think we're all in agreement that there's more to the contract of Place than can be guaranteed by safe code alone, so marking the trait as unsafe seems like a good move to me.

For the specific guarantees it sounds like we might not be able to get too specific about what a value of T can be written through actually means but maybe we can iterate on the text a bit to see if we can make it any clearer, and let the work on unsafe guidelines clarify all such cases in the future.

What do you all think?

[cramertj]

I've pushed an update to the wording of the docs. LMK if there are more changes you'd like to see.

[sfackler]

I'm jumping in a bit late here, but it seems like the second paragraph is basically just restating the first paragraph.

[cramertj]

@sfackler IMO the first paragraph sounds like it's directed at users of the trait-- it describes suggested usage patterns, while the second paragraph focuses on the invariants that must be upheld by the implementer in order for the unsafe trait requirements to be met.

[KodrAus]

I'm not sure if there's precedent in std, but some crates capture the distinction @cramertj is making between user docs and implementor docs more concretely. bytes for instance has implementor notes on its trait methods.

[KodrAus]

I think rephrasing the trait's contract from the implementor's point of view is good, even though it sounds similar. This looks good to me, thanks @cramertj!

@rfcbot fcp merge

[kennytm]

@rust-lang/libs Hi could any of you start the FCP process? rfcbot has not caught up with the new members yet (cc rust-lang/rfcbot-rs#176)

[alexcrichton]

@rfcbot fcp merge

[rfcbot]

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

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @dtolnay
• @sfackler

No concerns currently listed.

Once these reviewers reach consensus, 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.

[rfcbot]

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

[alexcrichton]

@bors: r+

[bors]

📌 Commit f25f468 has been approved by alexcrichton

[bors]

⌛ Testing commit f25f468 with merge 9758ff9...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 9758ff9 to master...