Skip to content

Remove T: Debug bound on UnsafeCell Debug impl #83707

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 13, 2021
Merged

Remove T: Debug bound on UnsafeCell Debug impl #83707

merged 1 commit into from
Apr 13, 2021

Conversation

exrook
Copy link
Contributor

@exrook exrook commented Mar 31, 2021 •
edited
Loading

Prior art: #65013

@bjorn3 bjorn3 requested a review from Mark-Simulacrum March 31, 2021 16:18
@bjorn3 bjorn3 removed the request for review from Mark-Simulacrum March 31, 2021 16:19
@bjorn3
Copy link
Member

bjorn3 commented Mar 31, 2021

Assigned to a random member of the libs team as highfive didn't respond.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 31, 2021
@JohnTitor JohnTitor added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 31, 2021
@Mark-Simulacrum
Copy link
Member

Not a member of the libs team :)

cc @rust-lang/libs, do we want to do a full FCP here? It seems clear we can't print the value in a safe way, so there's not much we can do with the Debug bound I think, even if we have it.

@Amanieu
Copy link
Member

Amanieu commented Apr 2, 2021

It doesn't hurt to be sure. I can't see any reason why this would cause any issues though.

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 2, 2021 •
edited by m-ou-se
Loading

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

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 2, 2021
dtolnay
dtolnay reviewed Apr 2, 2021
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I agree -- we can't possibly go from &UnsafeCell<T> to &T in order to invoke T's Debug impl, so the T: Debug bound is not useful.

Conceivably Debug could grow a defaulted method which takes an argument of type &UnsafeCell<Self>, which this PR rules out, but I am comfortable ruling that out because there isn't a type which could meaningfully format an arbitrary &UnsafeCell<Self>.

@joshtriplett
Copy link
Member

This seems reasonable to me as well. Because of the rules regarding trait impls, nobody could have an impl Debug for UnsafeCell<SomeType>, even for a type or trait defined in their crate, without having a wrapper type.

@rfcbot reviewed

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 2, 2021

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

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 2, 2021
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 12, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 12, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Apr 12, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Apr 12, 2021

@bors r+ rolllup

@bors
Copy link
Collaborator

bors commented Apr 12, 2021

📌 Commit 58218f6 has been approved by m-ou-se

@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 Apr 12, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Apr 12, 2021

@bors rollup

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 12, 2021
@Dylan-DPC
Rollup merge of rust-lang#83707 - exrook:unsafecell, r=m-ou-se
abb93d3 
Remove `T: Debug` bound on UnsafeCell Debug impl

Prior art: rust-lang#65013
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Apr 12, 2021
Closed
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Apr 13, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2021
@bors
Auto merge of rust-lang#84153 - Dylan-DPC:rollup-5jiqrwu, r=Dylan-DPC
5258a74 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#83438 (Update RELEASES.md)
 - rust-lang#83707 (Remove `T: Debug` bound on UnsafeCell Debug impl)
 - rust-lang#84084 (Stabilize duration_zero.)
 - rust-lang#84121 (Stabilize BTree{Map,Set}::retain)
 - rust-lang#84140 (Don't call bump in check_mistyped_turbofish_with_multiple_type_params)
 - rust-lang#84141 (Fix typo in error message)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1cc4b6d into rust-lang:master Apr 13, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 13, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 15, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@dtolnay dtolnay dtolnay left review comments

Assignees

@m-ou-se m-ou-se

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.53.0
Development

Successfully merging this pull request may close these issues.