-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Conversation
- 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`.
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
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. |
Fixed examples in sections "Generic Implementations" of `AsRef`'s and `AsMut`'s doc comments, which failed tests.
Better conform to Rust API Documentation Conventions
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 |
There was a problem hiding this 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 👌
/// | ||
/// 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@bors r+ rollup |
…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
…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
…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
There are several issues with the current state of
AsRef
andAsMut
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 howAsRef
andAsMut
should be used.In particular:
AsRef
andAsMut
do not auto-dereference generally for all dereferencable types (but only if inner type is a shared and/or mutable reference)AsRef
orAsMut
for the sole purpose of dereferencingAsRef
orAsMut
implementation for types which implementDeref
AsRef
andAsMut
AsMut
Borrow
" inAsRef
's docs to improve structure