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

Tracking issue for RFC 1651: Extend Cell to non-Copy types #39264

Closed
aturon opened this issue Jan 23, 2017 · 10 comments

Comments

Projects
None yet
4 participants
@aturon
Copy link
Member

commented Jan 23, 2017

RFC

wesleywiser added a commit to wesleywiser/rust that referenced this issue Jan 25, 2017

king6cong added a commit to king6cong/rust that referenced this issue Feb 3, 2017

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2017

#39287 broke a library of mine where I had code like this:

trait Take<T> {
    fn take(&self) -> Option<T>;
}

impl<T: Copy> Take<T> for Cell<Option<T>> {
    fn take(&self) -> Option<T> {
        let value = self.get();
        self.set(None);
        value
    }
}

foo.take() calls that used to call this trait method now call the new inherent method, but since that’s unstable the crate didn’t build until I added #![feature(move_cell)]. After that I could remove the trait since the new method has the same behavior.

Anyway, this is just FYI. If we don’t want this kind of breakage we can’t add any new method to existing std types ever, so that doesn’t sound viable. If the new method had had different behavior or if I wanted to keep supporting older compilers, it wouldn’t be too hard to rename my trait’s method or use UFCS.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Feb 6, 2017

Yeah in general we reserve the right to break downstream code with differences in method resolution because the downstream code can always be written in an "elaborated" form which works on all compilers (e.g. using ufcs).

If a change to the standard library breaks an excessive amount of code in the ecosystem, though, then we'd need to take the steps much more purposefully.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2017

If a change to the standard library breaks an excessive amount of code in the ecosystem

Sounds good.

Again, I don’t expect anything to change here and only made the previous comment to let people know this happened. I made this particular crate as an experiment, I don’t know of anyone actually using it.

frewsxcv added a commit to frewsxcv/rust that referenced this issue Feb 13, 2017

bors added a commit that referenced this issue Feb 13, 2017

Auto merge of #39716 - F001:swapCell, r=alexcrichton
Add `swap` method for `Cell`

Addition to #39264

r? @alexcrichton

frewsxcv added a commit to frewsxcv/rust that referenced this issue Feb 13, 2017

RalfJung added a commit to RalfJung/rust that referenced this issue Feb 13, 2017

RalfJung added a commit to RalfJung/rust that referenced this issue Feb 14, 2017

frewsxcv added a commit to frewsxcv/rust that referenced this issue Feb 15, 2017

Rollup merge of rust-lang#39793 - RalfJung:cell, r=alexcrichton
Allow more Cell methods for non-Copy types

Clearly, `get_mut` is safe for any `T`. The other two only provide unsafe pointers anyway.

The only remaining inherent method with `Copy` bound is `get`, which sounds about right to me.

I found the order if `impl` blocks in the file a little weird (first inherent impl, then some trait impls, then another inherent impl), but didn't change it to keep the diff small.

Contributes to rust-lang#39264

frewsxcv added a commit to frewsxcv/rust that referenced this issue Feb 15, 2017

Rollup merge of rust-lang#39793 - RalfJung:cell, r=alexcrichton
Allow more Cell methods for non-Copy types

Clearly, `get_mut` is safe for any `T`. The other two only provide unsafe pointers anyway.

The only remaining inherent method with `Copy` bound is `get`, which sounds about right to me.

I found the order if `impl` blocks in the file a little weird (first inherent impl, then some trait impls, then another inherent impl), but didn't change it to keep the diff small.

Contributes to rust-lang#39264

frewsxcv added a commit to frewsxcv/rust that referenced this issue Feb 16, 2017

Rollup merge of rust-lang#39793 - RalfJung:cell, r=alexcrichton
Allow more Cell methods for non-Copy types

Clearly, `get_mut` is safe for any `T`. The other two only provide unsafe pointers anyway.

The only remaining inherent method with `Copy` bound is `get`, which sounds about right to me.

I found the order if `impl` blocks in the file a little weird (first inherent impl, then some trait impls, then another inherent impl), but didn't change it to keep the diff small.

Contributes to rust-lang#39264

frewsxcv added a commit to frewsxcv/rust that referenced this issue Feb 16, 2017

Rollup merge of rust-lang#39793 - RalfJung:cell, r=alexcrichton
Allow more Cell methods for non-Copy types

Clearly, `get_mut` is safe for any `T`. The other two only provide unsafe pointers anyway.

The only remaining inherent method with `Copy` bound is `get`, which sounds about right to me.

I found the order if `impl` blocks in the file a little weird (first inherent impl, then some trait impls, then another inherent impl), but didn't change it to keep the diff small.

Contributes to rust-lang#39264
@aturon

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2017

Given that the core changes to the bounds on existing methods were instantly stable, it doesn't make a lot of sense to keep take and replace as unstable. I propose we immediately stabilize these APIs.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented Feb 23, 2017

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

No concerns currently listed.

Once these reviewers reach consensus, 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

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

ping @BurntSushi (checkbox)

@rfcbot

This comment has been minimized.

Copy link

commented Feb 28, 2017

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

@rfcbot

This comment has been minimized.

Copy link

commented Mar 10, 2017

The final comment period is now complete.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2017

What’s the next step here? Batch stabilization PR near the end of the release cycle?

@aturon

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2017

@SimonSapin Yep, I'm preparing one right now.

frewsxcv pushed a commit to frewsxcv/rust that referenced this issue Mar 17, 2017

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 17, 2017

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 17, 2017

@bors bors closed this in 33a5665 Mar 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.