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

Regression because of #26008 #26096

Closed
tomaka opened this issue Jun 8, 2015 · 4 comments
Closed

Regression because of #26008 #26096

tomaka opened this issue Jun 8, 2015 · 4 comments
Labels
P-high High priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@tomaka
Copy link
Contributor

tomaka commented Jun 8, 2015

#26008 caused a regression (see travis).

Here is the code:

use std::rc::Rc;

let a: Rc<Option<u32>> = Rc::new(Some(5));
let _b: Option<&u32> = a.as_ref();

It works with stable because as_ref() is called on the Option.
But in the nightlies, as_ref() is called on the Rc and I get a &Option<u32> instead of a Option<&u32>.
@Aatch
Copy link
Contributor

Aatch commented Jun 8, 2015

Related to #26080

@arielb1
Copy link
Contributor

arielb1 commented Jun 8, 2015

@Aatch

Not really. This case works in the same way as a.clone() (working on the Rc rather than dereferencing it).

@alexcrichton alexcrichton added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 8, 2015
@retep998
Copy link
Member

retep998 commented Jun 8, 2015

According to the semver RFC this is semver compatible because it is possible to rewrite the code in a way that will work with both versions: Option::as_ref(&*a)

@tomaka tomaka mentioned this issue Jun 8, 2015
Closed
@alexcrichton alexcrichton mentioned this issue Jun 9, 2015
Merged
@alexcrichton
Copy link
Member

triage: P-high

decided to revert the addition of the AsRef impl, and try again later perhaps but first run it through crater.

@rust-highfive rust-highfive added P-high High priority and removed I-nominated labels Jun 9, 2015
This was referenced Jun 9, 2015
Merged
Merged
bors added a commit that referenced this issue Jun 12, 2015
@bors
Auto merge of #26160 - alexcrichton:revert-rc-as-ref, r=aturon
c6b1483 
This is a revert of PR #26008 which caused the unintended breakage reported in #26096. We may want to add these implementations in the long run, but for now this revert allows us to take some more time to evaluate the impact of such a change (e.g. run it through crater).

Closes #26096
@alexcrichton alexcrichton mentioned this issue Sep 18, 2015
Merged
alexcrichton added a commit to alexcrichton/rust that referenced this issue Oct 2, 2015
@alexcrichton
std: Add AsRef/AsMut impls to Box/Rc/Arc
db76ac7 
These common traits were left off originally by accident from these smart
pointers, and a past attempt (rust-lang#26008) to add them was later reverted (rust-lang#26160)
due to unexpected breakge (rust-lang#26096) occurring. The specific breakage in worry is
the meaning of this return value changed:

    let a: Box<Option<T>> = ...;
    a.as_ref()

Currently this returns `Option<&T>` but after this change it will return
`&Option<T>` because the `AsRef::as_ref` method shares the same name as
`Option::as_ref`. A [crater report][crater] of this change, however, has shown
that the fallout of this change is quite minimal. These trait implementations
are "the right impls to add" to these smart pointers and would enable various
generalizations such as those in rust-lang#27197.

[crater]: https://gist.github.com/anonymous/0ba4c3512b07641c0f99

This commit is a breaking change for the above reasons mentioned, and the
mitigation strategies look like any of:

    Option::as_ref(&a)
    a.as_ref().as_ref()
    (*a).as_ref()
@alexcrichton alexcrichton mentioned this issue Oct 2, 2015
Merged
bors added a commit that referenced this issue Oct 8, 2015
@bors
Auto merge of #28811 - alexcrichton:as-ref-ptrs, r=aturon
e362679 
These common traits were left off originally by accident from these smart
pointers, and a past attempt (#26008) to add them was later reverted (#26160)
due to unexpected breakge (#26096) occurring. The specific breakage in worry is
the meaning of this return value changed:

    let a: Box<Option<T>> = ...;
    a.as_ref()

Currently this returns `Option<&T>` but after this change it will return
`&Option<T>` because the `AsRef::as_ref` method shares the same name as
`Option::as_ref`. A [crater report][crater] of this change, however, has shown
that the fallout of this change is quite minimal. These trait implementations
are "the right impls to add" to these smart pointers and would enable various
generalizations such as those in #27197.

[crater]: https://gist.github.com/anonymous/0ba4c3512b07641c0f99

This commit is a breaking change for the above reasons mentioned, and the
mitigation strategies look like any of:

    Option::as_ref(&a)
    a.as_ref().as_ref()
    (*a).as_ref()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
P-high High priority 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
6 participants
@alexcrichton @Aatch @retep998 @tomaka @arielb1 @rust-highfive