Skip to content

Conversation

alexcrichton
Copy link
Member

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 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.

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()

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()
@rust-highfive
Copy link
Contributor

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton alexcrichton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. relnotes Marks issues that should be documented in the release notes of the next release. labels Oct 2, 2015
@alexcrichton
Copy link
Member Author

cc @tomaka, reporter of #26096

@alexcrichton
Copy link
Member Author

Of the breakage in the crater report, it was also decided that if we move forward with this that we'll send a PR to cairo to get ahead of the breakage there.

@aturon
Copy link
Member

aturon commented Oct 7, 2015

@bors: r+

@bors
Copy link
Collaborator

bors commented Oct 7, 2015

📌 Commit db76ac7 has been approved by aturon

bors added a commit that referenced this pull request Oct 8, 2015
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()
@bors
Copy link
Collaborator

bors commented Oct 8, 2015

⌛ Testing commit db76ac7 with merge e362679...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. 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.

4 participants