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

Minor: note how Any is an unsafe trait in SAFETY comments #67722

Merged
merged 1 commit into from
Jan 29, 2020

Conversation

petertodd
Copy link
Contributor

Motivation: helpful to people like myself reading the standard library source to better understand how to use Any, especially if we do go ahead with #67562 and make it an unsafe trait.

@rust-highfive
Copy link
Collaborator

r? @withoutboats

(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 30, 2019
@Dylan-DPC-zz
Copy link

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 30, 2019

📌 Commit 98b58940150f40563a4ee7945678ecf9ab10fd78 has been approved by Dylan-DPC

@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 30, 2019
@Mark-Simulacrum
Copy link
Member

@bors r- let's avoid documenting any as unsafe until we actually do that - currently, the FCP on the relevant PR has some dissent

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 30, 2019
@petertodd
Copy link
Contributor Author

petertodd commented Dec 30, 2019 via email

@joelpalmer
Copy link

Ping from Triage: any update @petertodd?

@petertodd
Copy link
Contributor Author

Ping from Triage: any update @petertodd?

This makes sense with the current wording if #67562 is merged, which I'm in favor of. But if not maybe reword to "because Any is unsafe to implement"?

@Dylan-DPC-zz
Copy link

Marking this as blocked on #67562

@Dylan-DPC-zz Dylan-DPC-zz added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 6, 2020
@petertodd
Copy link
Contributor Author

Marking this as blocked on #67562

Makes sense, thanks!

@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Jan 17, 2020

The PR that this was blocked on has been closed so i'm not sure what happens to this one

@Mark-Simulacrum
Copy link
Member

The comments here should be updated to indicate that the operations are safe because the blanket impl for Any is correct, and no other impls can exist (as they would conflict with the existing impl).

I believe that would satisfy the safety obligation for these methods. It may also be worth linking to #67562 in the SAFETY comment for additional context.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jan 17, 2020
@Mark-Simulacrum
Copy link
Member

I'm going to also steal the review here r? @Mark-Simulacrum, but if someone feels they would want to take a look too feel free to add yourself.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 25, 2020
@Mark-Simulacrum
Copy link
Member

AFAICT, this is still waiting on author -- see #67722 (comment) for what needs to be done.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 26, 2020
@petertodd
Copy link
Contributor Author

@Mark-Simulacrum Revised comments as per #67562

@Mark-Simulacrum
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Jan 29, 2020

📌 Commit f722964 has been approved by Mark-Simulacrum

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 29, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 29, 2020
… r=Mark-Simulacrum

Minor: note how Any is an unsafe trait in SAFETY comments

Motivation: helpful to people like myself reading the standard library source to better understand how to use Any, especially if we do go ahead with rust-lang#67562 and make it an unsafe trait.
bors added a commit that referenced this pull request Jan 29, 2020
Rollup of 7 pull requests

Successful merges:

 - #67722 (Minor: note how Any is an unsafe trait in SAFETY comments)
 - #68586 (Make conflicting_repr_hints a deny-by-default c-future-compat lint)
 - #68598 (Fix null synthetic_implementors error)
 - #68603 (Changelog: Demonstrate final build-override syntax)
 - #68609 (Set lld flavor for MSVC to link.exe)
 - #68611 (Correct ICE caused by macros generating invalid spans.)
 - #68627 (Document that write_all will not call write if given an empty buffer)

Failed merges:

r? @ghost
@bors bors merged commit f722964 into rust-lang:master Jan 29, 2020
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.

8 participants