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.

Sign up for GitHub

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

Jump to bottom

Tracking issue for Rc::downcast (rc_downcast feature) #44608

Closed
bluss opened this issue Sep 15, 2017 · 16 comments
Closed

Tracking issue for Rc::downcast (rc_downcast feature) #44608

bluss opened this issue Sep 15, 2017 · 16 comments
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@bluss
Copy link
Member

bluss commented Sep 15, 2017

Tracking issue for the Rc::downcast method implemented in #44273
@bluss bluss added B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 15, 2017
@bluss bluss mentioned this issue Sep 15, 2017
Merged
@SimonSapin
Copy link
Contributor

Looks good to me to stabilize.

@rfcbot fcp merge

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 17, 2018
@rfcbot
Copy link

rfcbot commented Mar 17, 2018 •
edited
Loading

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

Concerns:

Once a majority of reviewers approve (and none object), 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.

@alexcrichton
Copy link
Member

@rfcbot concern arc

We should add to Arc as well for consistency, right?

@SimonSapin
Copy link
Contributor

On the implementation PR:

Arc is skipped because Any itself has no downcast for the case that makes most sense: Any + Send + Sync

Why we don't implement downcast for Any + Send + Sync? Other than it isn't terribly necessary.

We can still do that.

@alexcrichton
Copy link
Member

Indeed!

@bluss
Copy link
Member Author

bluss commented Mar 19, 2018

Hehe, I can't quite find the right way to pick the right implementations.

All four combinations are "Trait", "Trait + Send", "Trait + Sync", "Trait + Send + Sync".

The frugal approach is just the first one for Rc and just the last one for Arc. The perfect would be to write just one that covered them all, for each of Rc and Arc. I think the pragmatic would be all four for both Rc and Arc.

@alexcrichton
Copy link
Member

@bluss I'd be ok conservatively just doing Arc<Any + Send + Sync> as I doubt most other combinations don't actually show up that often in practice

@jsgf
Copy link
Contributor

jsgf commented May 17, 2018

Hi! I ran into this today. Any objections if I put together a PR for Arc<Any + Send + Sync>?

@sfackler
Copy link
Member

SGTM

@jsgf
Copy link
Contributor

jsgf commented May 17, 2018

Should I reuse this issue and rc_downcast, or create new ones?

@SimonSapin
Copy link
Contributor

Using the same tracking issue sounds fine: it’s a identical copy of a fairly small function. By the way, I think you can also remove an unsafe block by using the (relatively) new cast method of NonNull, instead of NonNull::new_unchecked.

@jsgf
Copy link
Contributor

jsgf commented May 17, 2018

I also need to add Any + Send + Sync implementations for is, downcast_ref and Debug to std::any::Any. Do they need feature guards, or can I just insta-stable them? If so, what version would I put in since = "..."?

@jsgf jsgf mentioned this issue May 17, 2018
Merged
@Centril Centril added the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label May 24, 2018
jsgf added a commit to jsgf/rust that referenced this issue May 31, 2018
@jsgf
Implement downcast for Arc<Any + Send + Sync>
37f5cf5 
We only need to implement it for `Any + Send + Sync` because in practice
that's the only useful combination for `Arc` and `Any`.

Implementation for rust-lang#44608 under the `rc_downcast` feature.
bors added a commit that referenced this issue Jun 1, 2018
@bors
Auto merge of #50836 - jsgf:arc-downcast, r=SimonSapin
b7a9d4e 
Arc downcast

Implement `downcast` for `Arc<Any + Send + Sync>` as part of #44608, and gated by the same `rc_downcast` feature.

This PR is mostly lightly-edited cut'n'paste.

This has two additional changes:
- The `downcast` implementation needs `Any + Send + Sync` implementations for `is` and `Debug`, and I added `downcast_ref` and `downcast_mut` for completeness/consistency. (Can these be insta-stabilized?)
- At @SimonSapin's suggestion, I converted `Arc` and `Rc` to use `NonNull::cast` to avoid an `unsafe` block in each which tidied things up nicely.
@SimonSapin
Copy link
Contributor

@rfcbot resolve arc

The requested feature was implemented in #50836.

@SimonSapin
Copy link
Contributor

SimonSapin commented Jun 6, 2018 •
edited
Loading

Hmm it looks like only @alexcrichton can tell rfcbot to resolve the concern here, but I think we can consider the spirit of the process to be satisfied (if not the letter) and I would accept a stabilization PR. (Alex is away for a month.)

@alexcrichton
Copy link
Member

@rfcbot resolved arc

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jun 27, 2018
@rfcbot rfcbot added the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label Jun 27, 2018
@rfcbot
Copy link

rfcbot commented Jun 27, 2018

🔔 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 Jun 27, 2018
tmccombs added a commit to tmccombs/rust that referenced this issue Jul 6, 2018
@tmccombs
Stabilize rc_downcast
7fbc389 
Fixes rust-lang#44608
@tmccombs tmccombs mentioned this issue Jul 6, 2018
Merged
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 7, 2018
@Mark-Simulacrum
Rollup merge of rust-lang#52103 - tmccombs:rc_downcast, r=Mark-Simula…
e382338 
…crum

Stabilize rc_downcast

Fixes rust-lang#44608
@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 Jul 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@alexcrichton @jsgf @SimonSapin @Centril @sfackler @bluss @rfcbot