Skip to content
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

Document why Any is not an unsafe trait #67519

Merged
merged 1 commit into from
Dec 22, 2019

Conversation

Mark-Simulacrum
Copy link
Member

The added documentation is not public (i.e., not in a doc comment) but that seems appropriate for this sort of low-level detail.

Fixes #62303

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 22, 2019
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with nits

src/libcore/any.rs Outdated Show resolved Hide resolved
src/libcore/any.rs Outdated Show resolved Hide resolved
src/libcore/any.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Dec 22, 2019

r? @Centril

@Mark-Simulacrum
Copy link
Member Author

@bors r=Centril

@bors
Copy link
Contributor

bors commented Dec 22, 2019

📌 Commit 6878913 has been approved by Centril

@bors
Copy link
Contributor

bors commented Dec 22, 2019

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 22, 2019
Centril added a commit to Centril/rust that referenced this pull request Dec 22, 2019
Document why Any is not an unsafe trait

The added documentation is not public (i.e., not in a doc comment) but that seems appropriate for this sort of low-level detail.

Fixes rust-lang#62303
Centril added a commit to Centril/rust that referenced this pull request Dec 22, 2019
Document why Any is not an unsafe trait

The added documentation is not public (i.e., not in a doc comment) but that seems appropriate for this sort of low-level detail.

Fixes rust-lang#62303
Centril added a commit to Centril/rust that referenced this pull request Dec 22, 2019
Document why Any is not an unsafe trait

The added documentation is not public (i.e., not in a doc comment) but that seems appropriate for this sort of low-level detail.

Fixes rust-lang#62303
bors added a commit that referenced this pull request Dec 22, 2019
Rollup of 8 pull requests

Successful merges:

 - #66877 (Add simpler entry points to const eval for common usages.)
 - #67299 (Add `ImmTy::try_from_(u)int` methods)
 - #67487 (Rustdoc mutability removal)
 - #67499 (Misc MIR building cleanups)
 - #67506 (Remove iter_private.rs)
 - #67508 (Fix typo in path parser name)
 - #67519 (Document why Any is not an unsafe trait)
 - #67525 (Utilize rust-lang/rust commit hashes in toolstate)

Failed merges:

r? @ghost
@bors bors merged commit 6878913 into rust-lang:master Dec 22, 2019
Comment on lines +82 to +86
// We could plausibly make this trait unsafe -- it would not cause breakage,
// since we control all the implementations -- but we choose not to as that's
// both not really necessary and may confuse users about the distinction of
// unsafe traits and unsafe methods (i.e., `type_id` would still be safe to call,
// but we would likely want to indicate as such in documentation).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To the contrary, I think making this trait unsafe will help explain the difference between unsafe traits and unsafe methods as it gives a fairly simple example for such a combination. Anyone writing unsafe code should be comfortable with that distinction.

Not a big deal, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, fwiw, I had the same thought while reading this, but ultimately kept quiet cause "not a big deal". ^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly just didn't want to wait for crater, but since people seem on board with unsafe here I might reconsider. I do think it's not a bad example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Any trait should be unsafe
6 participants