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

Implement Default for AssertUnwindSafe #95949

Merged
merged 1 commit into from
Apr 26, 2022
Merged

Conversation

SoniEx2
Copy link
Contributor

@SoniEx2 SoniEx2 commented Apr 11, 2022

Trait impls are still insta-stable yeah...?

Trait impls are still insta-stable yeah...?
@rust-highfive
Copy link
Collaborator

Thank you for submitting a new PR for the library teams! If this PR contains a stabilization of a library feature that has not already completed FCP in its tracking issue, introduces new or changes existing unstable library APIs, or changes our public documentation in ways that create new stability guarantees then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @scottmcm (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 11, 2022
@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Apr 11, 2022

r? rust-lang/libs-api @rustbot label +T-libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 11, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Apr 13, 2022

Thanks for your PR.

Do you have an example or use case for this? When is it useful for AssertUnwindSafe to implement Default?

@m-ou-se m-ou-se 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 Apr 13, 2022
@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Apr 13, 2022

We have this "beauty": https://docs.rs/hexchat-unsafe-plugin/1.0.0/src/hexchat_unsafe_plugin/lib.rs.html#1143 (it is a bit ugly, but anyway)

Altho it was pretty easy to work around it: https://docs.rs/hexchat-unsafe-plugin/1.0.0/src/hexchat_unsafe_plugin/lib.rs.html#1361

(We put the AssertUnwindSafe on the RefCell rather than Rc so we could easily Rc::clone(&it) and it's not disruptive to calling .borrow() or .borrow_mut() on it)

Edit: We do believe it would help with clarity to use Contexts::default(). And there's no benefit to making a full wrapper type just for a Default impl here (we would even argue it'd bring drawbacks).

@m-ou-se
Copy link
Member

m-ou-se commented Apr 13, 2022

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 13, 2022

Team member @m-ou-se 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. 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 13, 2022
@rfcbot
Copy link

rfcbot commented Apr 14, 2022

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

@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 24, 2022
@rfcbot
Copy link

rfcbot commented Apr 24, 2022

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.

This will be merged soon.

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

m-ou-se commented Apr 26, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 26, 2022

📌 Commit 8d5a496 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 26, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 26, 2022
Implement Default for AssertUnwindSafe

Trait impls are still insta-stable yeah...?
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 26, 2022
Implement Default for AssertUnwindSafe

Trait impls are still insta-stable yeah...?
This was referenced Apr 26, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 26, 2022
…laumeGomez

Rollup of 8 pull requests

Successful merges:

 - rust-lang#94022 (Clarify that `Cow::into_owned` returns owned data)
 - rust-lang#94703 (Fix codegen bug in "ptx-kernel" abi related to arg passing)
 - rust-lang#95949 (Implement Default for AssertUnwindSafe)
 - rust-lang#96361 (Switch JS code to ES6)
 - rust-lang#96372 (Suggest calling method on nested field when struct is missing method)
 - rust-lang#96386 (simplify `describe_field` func in borrowck's diagnostics part)
 - rust-lang#96400 (Correct documentation for `Rvalue::ShallowInitBox`)
 - rust-lang#96415 (Remove references to git.io)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2b8bf0d into rust-lang:master Apr 26, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 26, 2022
@SoniEx2 SoniEx2 deleted the patch-5 branch April 26, 2022 19:14
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 28, 2022
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented May 3, 2022

This was probably a perf regression (marking from the upstream PR which actually merged this).

Rollup of 8 pull requests #96428
(Comparison Link)

Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count 7 2 1 0 8
mean 1.3% 0.3% -0.4% N/A 1.1%
max 1.8% 0.3% -0.4% N/A 1.8%

Looks likely to be related to the new impl Default for AssertUnwindSafe, though
detailed query pages do not reflect the regressions. Marking the regression as
triaged, because it's unlikely we would want to revert the impl at this point.
However, it's probably the case that impls for public standard library types may
want to avoid being rolled up, as they can influence downstream performance
across both rustdoc and regular compilation.

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented May 3, 2022

Diesel does throw trait resolution into overdrive... but we're surprised impl Default for AssertUnwindSafe out of all things turns out to be relevant here o.o

wonder how that happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

9 participants