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

improve documentation for AsRef/AsMut #62586

Open
BurntSushi opened this issue Jul 11, 2019 · 3 comments
Open

improve documentation for AsRef/AsMut #62586

BurntSushi opened this issue Jul 11, 2019 · 3 comments
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@BurntSushi
Copy link
Member

AsRef/AsMut is fairly common source of changes that result in breaking others' code. Most such changes occur by adding additional AsRef/AsMut impls, which in turn break type inference that users are unwittingly relying on. According to Rust's API evolution, this is, strictly speaking, allowable breakage.

Almost all such instances of code breakage correspond to a misuse of AsRef/AsMut, by using it situations where there are no constraints to help type inference. Idiomatic use of these traits is typically by specifying them in a context where the target type is known. For example:

fn foo<T: AsMut<[u8]>>(something: T) {
    // this will never fail, because the target type is unambiguous
    let my_mutable_slice = something.as_mut();
    // ...
}

Code like this is robust with respect to additional implementations of AsMut.

This issue is about improving the documentation of AsRef/AsMut to cover these important caveats. Currently, the docs include no mention of what incorrect usage actually is, or the fact that incorrectly using this trait can result in a new Rust compiler failing to compiler your code. (There is an example of correct usage, but no broader discussion of the intricacies of using this trait.) Additionally, the docs, IMO, don't do a good enough job differentiating AsRef from Borrow. The docs mention some specific differences, but don't cover high level concerns. It's very difficult, even for me, to take the noted differences in the docs and extrapolate that to a heuristic for when I should prefer one over the other.

This issue is motivated by the fallout from #60958, and my own recent experience in dealing with this in BurntSushi/rust-csv#160 (which is particularly subtle).

@BurntSushi BurntSushi added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools labels Jul 11, 2019
@jonas-schievink jonas-schievink added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 11, 2019
@lnicola
Copy link
Member

lnicola commented Jul 12, 2019

Why doesn't the compiler tell why the call is ambiguous? It could enumerate the implementations that as_ref() could come from.

@BurntSushi
Copy link
Member Author

Another example of misuse that lead to a new impl breaking (significant) downstream consumers: BurntSushi/bstr#149

Basically, I added a impl AsRef<BStr> for BStr, but folks were using bstr.as_ref() in a non-generic context that relied on inferring the target type to be &[u8]. But with the new impl, the target type became ambiguous and inference failed.

@Stargateur
Copy link
Contributor

What are we waiting for?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants