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

docs: Improve AsRef / AsMut docs on blanket impls #99460

Merged
merged 4 commits into from
Oct 3, 2022

Conversation

JanBeh
Copy link
Contributor

@JanBeh JanBeh commented Jul 19, 2022

There are several issues with the current state of AsRef and AsMut as discussed here on IRLO. See also #39397, #45742, #73390, #98905, and the FIXMEs here and here. These issues are difficult to fix. This PR aims to update the documentation to better reflect the status-quo and to give advice on how AsRef and AsMut should be used.

In particular:

  • Explicitly mention that AsRef and AsMut do not auto-dereference generally for all dereferencable types (but only if inner type is a shared and/or mutable reference)
  • Give advice to not use AsRef or AsMut for the sole purpose of dereferencing
  • Suggest providing a transitive AsRef or AsMut implementation for types which implement Deref
  • Add new section "Reflexivity" in documentation comments for AsRef and AsMut
  • Provide better example for AsMut
  • Added heading "Relation to Borrow" in AsRef's docs to improve structure

- Explicitly mention that `AsRef` and `AsMut` do not auto-dereference
  generally for all dereferencable types (but only if inner type is a
  shared and/or mutable reference)
- Give advice to not use `AsRef` or `AsMut` for the sole purpose of
  dereferencing
- Suggest providing a transitive `AsRef` or `AsMut` implementation for
  types which implement `Deref`
- Add new section "Reflexivity" in documentation comments for `AsRef`
  and `AsMut`
- Provide better example for `AsMut`
- Added heading "Relation to `Borrow`" in `AsRef`'s docs to improve
  structure

Issue rust-lang#45742 and a corresponding FIXME in the libcore suggest that
`AsRef` and `AsMut` should provide a blanket implementation over
`Deref`. As that is difficult to realize at the moment, this commit
updates the documentation to better describe the status-quo and to give
advice on how to use `AsRef` and `AsMut`.
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 19, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 19, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2022
library/core/src/convert/mod.rs Outdated Show resolved Hide resolved
library/core/src/convert/mod.rs Outdated Show resolved Hide resolved
Fixed examples in sections "Generic Implementations" of `AsRef`'s and
`AsMut`'s doc comments, which failed tests.
Better conform to Rust API Documentation Conventions
@JanBeh
Copy link
Contributor Author

JanBeh commented Jul 19, 2022

In addition to fixing compilation of the examples, I provided a fixup to improve some style issues (as in these recommendations). Note that the docs currently use AsRef in some parts where it should be AsRef<T> according to the style guide. I didn't change the existing parts. Also note that in some places I omitted the <T> where it seemed appropriate to avoid confusion. Please let me know if the style should be changed in any way.

Copy link

@danjl1100 danjl1100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello! Following along on the IRLO thread, I quickly realized that this AsRef vs Borrow distinction is something I never fully understood.

I like these changes, a big improvement to help clarify the distinction and mention that deref coercion is preferred for usage in generic contexts.

This is one of my first reviews as a common IRLO bystander.
Great work organizing your thoughts to improve the docs 👌

library/core/src/convert/mod.rs Outdated Show resolved Hide resolved
library/core/src/convert/mod.rs Outdated Show resolved Hide resolved
///
/// fn null_terminate<T: AsMut<Vec<u8>>>(data: &mut T) {
/// // Using a non-generic inner function, which contains most of the
/// // functionality, helps to minimize monomorphization overhead.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unclear to me whether this monomorphize optimization is helpful to portray the meaning of the example.
(e.g. remove the doit function, and mention the possible optimization in words below the example instead, with a relevant link. That is, if that "tip" merits staying on this AsMut doc page.)

I haven't read over very much of std doc, so it is possible things like this "tip not strictly related, but useful" are more common than I realize.

Just wanted to draw attention for consideration / discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Providing a generic API is the use case of AsRef. Unfortunately, the compiler isn't capable of doing these optimizations by itself. I think people who write generic APIs should see how it's done correctly (with current Rust).

I agree, however, that it makes the example even more complex. Maybe the example code could be non-optimized and the "better practice" added at the end:

Note, however, that APIs don’t need to be generic. In many cases taking a &mut [u8] or &mut Vec<u8>, for example, is the better choice (callers need to pass the correct type then).

When providing a generic API, it's good practice to use a non-generic inner function to minimize monomorphization overhead. Thus the function null_terminate from above would be better written as:

fn null_terminate<T: AsMut<Vec<u8>>>(data: &mut T) {
    fn inner(data: &mut Vec<u8>) {
        let len = data.len();
        if len == 0 || data[len-1] != 0 {
            data.push(0);
        }
    }
    inner(data.as_mut());
}

What do you think about it, and do you know how (and with what target?) I could (should?) link "monomorphization" in the docs properly?

Downside would be it makes the example section even longer, but maybe it's necessary to not confuse readers.

Copy link

@danjl1100 danjl1100 Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now see what you mean, generic usage is pretty central to these types.

The only reference I could find comes from a rust perf book, which is a separate project, not part of the official rust documentation. I don't think this should be linked in std docs.

I like having this optimization separated out. When the compiler is improved in the future to optimize this for users, then this addendum could be easily removed.

I'd like to hear what more seasoned rust members think, for the question of whether to include the optimization example now.
As that link mentions,

The effects of these sorts of changes on
compile times will usually be small,
though occasionally they can be large. Example

Copy link
Contributor Author

@JanBeh JanBeh Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally would feel okay with keeping it as it is, but I also would like to hear some other opinions, as I acknowledge this makes the whole example a bit long. I don't see a good reference to link to (and none that is part of std or rust itself) – at least I didn't find any.

Changed wording in sections on "Reflexivity":
replaced "that is there is" with "i.e. there would be" and removed comma
before "with"

Reason: "there is" somewhat contradicted the "would be" hypothetical.
A slightly redundant wording has now been chosen for better clarity.
The comma seemed to be superfluous.
@joshtriplett
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 3, 2022

📌 Commit e6b761b has been approved by joshtriplett

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 3, 2022
…riplett

docs: Improve AsRef / AsMut docs on blanket impls

There are several issues with the current state of `AsRef` and `AsMut` as [discussed here on IRLO](https://internals.rust-lang.org/t/semantics-of-asref/17016). See also rust-lang#39397, rust-lang#45742, rust-lang#73390, rust-lang#98905, and the FIXMEs [here](https://github.com/rust-lang/rust/blob/1.62.0/library/core/src/convert/mod.rs#L509-L515) and [here](https://github.com/rust-lang/rust/blob/1.62.0/library/core/src/convert/mod.rs#L530-L536). These issues are difficult to fix. This PR aims to update the documentation to better reflect the status-quo and to give advice on how `AsRef` and `AsMut` should be used.

In particular:

- Explicitly mention that `AsRef` and `AsMut` do not auto-dereference generally for all dereferencable types (but only if inner type is a shared and/or mutable reference)
- Give advice to not use `AsRef` or `AsMut` for the sole purpose of dereferencing
- Suggest providing a transitive `AsRef` or `AsMut` implementation for types which implement `Deref`
- Add new section "Reflexivity" in documentation comments for `AsRef` and `AsMut`
- Provide better example for `AsMut`
- Added heading "Relation to `Borrow`" in `AsRef`'s docs to improve structure
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 3, 2022
…riplett

docs: Improve AsRef / AsMut docs on blanket impls

There are several issues with the current state of `AsRef` and `AsMut` as [discussed here on IRLO](https://internals.rust-lang.org/t/semantics-of-asref/17016). See also rust-lang#39397, rust-lang#45742, rust-lang#73390, rust-lang#98905, and the FIXMEs [here](https://github.com/rust-lang/rust/blob/1.62.0/library/core/src/convert/mod.rs#L509-L515) and [here](https://github.com/rust-lang/rust/blob/1.62.0/library/core/src/convert/mod.rs#L530-L536). These issues are difficult to fix. This PR aims to update the documentation to better reflect the status-quo and to give advice on how `AsRef` and `AsMut` should be used.

In particular:

- Explicitly mention that `AsRef` and `AsMut` do not auto-dereference generally for all dereferencable types (but only if inner type is a shared and/or mutable reference)
- Give advice to not use `AsRef` or `AsMut` for the sole purpose of dereferencing
- Suggest providing a transitive `AsRef` or `AsMut` implementation for types which implement `Deref`
- Add new section "Reflexivity" in documentation comments for `AsRef` and `AsMut`
- Provide better example for `AsMut`
- Added heading "Relation to `Borrow`" in `AsRef`'s docs to improve structure
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 3, 2022
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#98218 (Document the conditional existence of `alloc::sync` and `alloc::task`.)
 - rust-lang#99216 (docs: be less harsh in wording for Vec::from_raw_parts)
 - rust-lang#99460 (docs: Improve AsRef / AsMut docs on blanket impls)
 - rust-lang#100470 (Tweak `FpCategory` example order.)
 - rust-lang#101040 (Fix `#[derive(Default)]` on a generic `#[default]` enum adding unnecessary `Default` bounds)
 - rust-lang#101308 (introduce `{char, u8}::is_ascii_octdigit`)
 - rust-lang#102486 (Add diagnostic struct for const eval error in `rustc_middle`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit eedb512 into rust-lang:master Oct 3, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 3, 2022
@JanBeh JanBeh deleted the PR_asref_asmut_docs branch October 4, 2022 07:37
@JanBeh JanBeh restored the PR_asref_asmut_docs branch October 4, 2022 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants