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

Many From implementations are undocumented #51430

Open
skade opened this issue Jun 8, 2018 · 32 comments
Open

Many From implementations are undocumented #51430

skade opened this issue Jun 8, 2018 · 32 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. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@skade
Copy link
Contributor

skade commented Jun 8, 2018

While std::convert::From has very detailed high level documentation, the implementations often lack docs on how they work and use the default "performs the conversion" boilerplate.

This at least includes all String methods, also, all integer conversions. Both of these can have interesting behaviour, especially in relation to each other. For example, the different allocation behaviour between From<Box<str>> and From<&'a str> is notable and hard to figure out from the source, as it goes through three layers of indirection.

@frewsxcv frewsxcv added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Jun 8, 2018
bors added a commit that referenced this issue Jun 10, 2018
add some docs to `From` conversions

This adds a helpful document to the bool → int* conversions as well as to the lossless integer conversions.

One of #51430 down, some more to go.
@skade
Copy link
Contributor Author

skade commented Jun 18, 2018

@frewsxcv After speaking to @steveklabnik, I'd be willing to mentor this. Can you add the E-easy and E-mentor labels and assign me?

@oli-obk oli-obk added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Jun 18, 2018
@skade
Copy link
Contributor Author

skade commented Jun 19, 2018

Tutorial

To fix this issue, we have to go through the whole rustc codebase
and document all impl From implementations.

std::convert::From represents the
possible conversion from one type to another. This conversion is usually cheap,
but the exact nature of it is not specified. Currently, most implementations
in the std lib are using the standard documentation "performs the conversion".

The goal is to improve this situation.

Picking a task

This assumes some basic git knowledge. If you need help with git, this short tutorial
covers all you need.

  1. Pick a file from the rustc source that has undocumented From implementations.
  2. Write a comment here, mentioning @skade that you want to work on this file.
  • This is to ensure that you don't do double work
  1. Download rust-lang/rust and create a branch
  2. Document the from function like this:
impl From<Foo> for Bar {
    /// Converts a `Foo` into a `Bar`.
    /// This conversion does not allocate.
    fn from(f: Foo) -> Bar {
        //....
    }
}
  1. Post a pull request and add a link to it to this issue.

What to document

Have a close look at the implementation and try to find out what it does.
Write that down. The implementation might be spread and refer to other functions,
follow as deep as you need. Write down everything you learn in the documentation.

Keep an eye out for:

  • Heap allocations ("The result is allocated on the heap" [or not])
  • Give a rough estimate of the cost. ("The conversion copies the data" or "The conversion happens in place")
  • Is the conversion free? (yes, this may happen and should be documented)

Don't feel bad if the resulting text feels formulaic or repetetive, that's very
usual in documentation.

If you have some brain cycles free, try to find a clear, understandable wording.
But don't overthink it, we will improve during review together.

If you are confused or stuck, please get back here as well. This is what
the mentor can help with.

@skade
Copy link
Contributor Author

skade commented Jun 19, 2018

This is an incomplete list of From implementations, found with rg "impl From<". Others, especially in macros, may be found.

  • libproc_macro/lib.rs
    156:impl From for TokenStream {
    550:impl From for TokenTree {
    557:impl From for TokenTree {
    564:impl From for TokenTree {
    571:impl From for TokenTree {

  • libsyntax/tokenstream.rs
    194:impl From for TokenStream {
    200:impl From for TokenStream {
    565:impl From for ThinTokenStream {
    576:impl From for TokenStream {

  • libserialize/json.rs
    367:impl Fromfmt::Error for EncoderError {

  • liballoc/boxed.rs
    442:impl From<Box> for Box<[u8]> {

  • liballoc/string.rs
    2209:impl From<Box> for String {
    2218:impl From for Box {
    2277:impl From for Vec {

  • liballoc/arc.rs
    1422:impl From for Arc {

  • liballoc/rc.rs
    1101:impl From for Rc {

  • libstd/error.rs
    167:impl From for Box<Error + Send + Sync> {
    187:impl From for Box {

  • libsyntax_pos/lib.rs
    115:impl From for FileName {
    643:impl From for MultiSpan {
    649:impl From<Vec> for MultiSpan {

  • libstd/path.rs
    1402:impl From<Box> for PathBuf {
    1409:impl From for Box {
    1423:impl From for PathBuf {
    1430:impl From for OsString {
    1437:impl From for PathBuf {
    1524:impl From for Arc {
    1542:impl From for Rc {

  • libstd/process.rs
    975:impl From for Stdio {
    982:impl From for Stdio {
    989:impl From for Stdio {
    996:impl Fromfs::File for Stdio {

  • libcore/alloc.rs
    394:impl From for CollectionAllocErr {
    402:impl From for CollectionAllocErr {

  • libcore/task.rs
    173:impl From for Waker {

  • libsyntax/parse/parser.rs
    498:impl From<Option<ThinVec>> for LhsExpr {
    508:impl From<P> for LhsExpr {

  • libstd/sys_common/process.rs
    28:impl From for DefaultEnvKey {
    32:impl From for OsString {

  • libstd/ffi/os_str.rs
    350:impl From for OsString {
    618:impl From<Box> for OsString {
    625:impl From for Box {
    632:impl From for Arc {
    650:impl From for Rc {

  • libstd/ffi/c_str.rs
    644:impl From for Vec {
    702:impl From<Box> for CString {
    710:impl From for Box {
    742:impl From for Arc {
    760:impl From for Rc {
    833:impl From for io::Error {

  • libstd/io/error.rs
    223:impl From for Error {

  • libstd/net/addr.rs
    556:impl From for SocketAddr {
    563:impl From for SocketAddr {

  • libstd/net/ip.rs
    665:impl From for IpAddr {
    672:impl From for IpAddr {
    780:impl From for u32 {
    798:impl From for Ipv4Addr {
    815:impl From<[u8; 4]> for Ipv4Addr {
    830:impl From<[u8; 4]> for IpAddr {
    1395:impl From for u128 {
    1404:impl From for Ipv6Addr {
    1415:impl From<[u8; 16]> for Ipv6Addr {
    1424:impl From<[u16; 8]> for Ipv6Addr {
    1433:impl From<[u8; 16]> for IpAddr {
    1461:impl From<[u16; 8]> for IpAddr {

  • libcore/char/convert.rs
    117:impl From for u32 {
    143:impl From for char {

  • libcore/sync/atomic.rs
    933:impl From for AtomicBool {
    985: impl From<$int_type> for $atomic_type {

  • libcore/tests/pattern.rs
    36:impl From for Step {
    46:impl From<Option<(usize, usize)>> for Step {

  • libcore/num/mod.rs
    4410:impl From<!> for TryFromIntError {
    4714: impl From<$Small> for $Large {

  • libstd/sys/redox/time.rs
    188:impl Fromsyscall::TimeSpec for SystemTime {

  • libstd/sys/unix/time.rs
    209: impl Fromlibc::timeval for SystemTime {
    218: impl Fromlibc::timespec for SystemTime {
    333: impl Fromlibc::timespec for SystemTime {

  • libstd/sys/redox/process.rs
    401:impl From for Stdio {
    407:impl From for Stdio {
    467:impl From for ExitStatus {

  • libstd/sys/wasm/process.rs
    73:impl From for Stdio {
    79:impl From for Stdio {

  • libstd/sys/windows/time.rs
    171:impl From<c::FILETIME> for SystemTime {

  • libstd/sys/windows/process.rs
    44:impl From for WindowsEnvKey {
    52:impl From for OsString {
    315:impl From for Stdio {
    321:impl From for Stdio {
    398:impl From<c::DWORD> for ExitStatus {

  • libstd/sync/mpsc/mod.rs
    1751:impl From for TryRecvError {
    1792:impl From for RecvTimeoutError {

  • libstd/sys/cloudabi/shims/process.rs
    71:impl From for Stdio {
    77:impl From for Stdio {

  • libstd/sys/unix/process/process_common.rs
    317:impl From for Stdio {
    323:impl From for Stdio {
    383:impl From<c_int> for ExitStatus {

  • libstd/sys/redox/net/dns/mod.rs
    45:impl From for n16 {
    54:impl From for u16 {

@sunjay sunjay closed this as completed Jun 19, 2018
@sunjay sunjay reopened this Jun 19, 2018
@gruberb
Copy link
Contributor

gruberb commented Jun 19, 2018

Hey @skade, I would be up for the task. I skimmed through libstd/path.rs and would like to take it.

I have experienced with JavaScript (and some other languages), but just played around with Rust a bit. Therefore would need some help.

@cruessler
Copy link

I’d like to help. Can I work on libstd/net/ip.rs?

@skade
Copy link
Contributor Author

skade commented Jun 19, 2018

@cruessler Sure!

@skade
Copy link
Contributor Author

skade commented Jun 19, 2018

@gruberb Hi! I'd be happy to help! How about you open a pull request and we start discussing there?

Path is one of the things where "(heap) allocates" vs. "not" is of particular importance.

@sehinde-gb
Copy link

Good Morning @skade, I would like to work on liballoc/boxed.rs. I will create a pull request as per the documentation.

@sehinde-gb
Copy link

Hi @skade, I would like to work on
libstd/sys_common/process.rs
28:impl From for DefaultEnvKey {
32:impl From for OsString {

I will work on them and issue a pull request in due course

@cruessler
Copy link

@skade The From trait is already documented to various degrees for IP addresses. I could provide my findings in a comment, but I am not sure how to proceed (as the comment would be quite long and detailed).

@cypher
Copy link
Contributor

cypher commented Jun 25, 2018

@skade I'd like to take a stab at libstd/ffi/os_str.rs and libstd/ffi/c_str.rs

@skade
Copy link
Contributor Author

skade commented Jun 25, 2018

@cruessler If you feel like they can't be improved, it's fine to keep things the way they are. Findings in a comment would be great, than we can mark them as completed and know why. 👍

@skade
Copy link
Contributor Author

skade commented Jun 26, 2018

@cypher \o/ go ahead!

@cruessler
Copy link

Here’s what I found:

  • Constructing an IPv4 address via Ipv4Addr::new calls hton which calls i.to_be to convert its argument to big endian. That may by a noop if the host byte order equals the network byte order. i.to_be is explicitly inlined via #[inline] so I assume hton is inlined, too (I haven’t looked at the assembly, though).
  • Constructing an IPv6 address via IPv6Attr::new does not need to call hton.
  • Converting an IPv4 address to u32 calls ntoh.
  • Converting an IPv6 address to u128 does not call ntoh.

None of the implementations does heap allocation, all of them barely do anything besides bitshifting.

These implementations are currently undocumented:

  • impl From<Ipv4Addr> for IpAddr
  • impl From<Ipv6Addr> for IpAddr
  • impl From<Ipv6Addr> for u128
  • impl From<u128> for Ipv6Addr
  • impl From<[u8; 16]> for Ipv6Addr
  • impl From<[u16; 8]> for Ipv6Addr

These implementations are currently currently documented and have an example:

  • impl From<Ipv4Addr> for u32
  • impl From<u32> for Ipv4Addr
  • impl From<[u8; 4]> for IpAddr
  • impl From<[u8; 16]> for IpAddr
  • impl From<[u16; 8]> for IpAddr

Example for documentation:

Convert an Ipv4Addr into a host byte order u32.

Examples

use std::net::Ipv4Addr;

let addr = Ipv4Addr::new(13, 12, 11, 10);
assert_eq!(0x0d0c0b0au32, u32::from(addr));

This implementation only comes with an example:

  • impl From<[u8; 4]> for Ipv4Addr

Conclusion

If the documentation is supposed to be consistent, adding a short sentence (like the one cited above) and an example to every implementation seems like a good solution to me. Since that would be very repetitive, adding the comment in one, central location might be an alternative. What do you think?

@cypher
Copy link
Contributor

cypher commented Jul 11, 2018

@skade I've pushed a branch with my first stab at documenting the From conversion: https://github.com/cypher/rust/tree/document-from-trait-in-ffi

Not sure if I read the various methods right in regards to copying or allocating data, so someone more knowledgable should double-check those.

Should I just open a pull request, or do you want to take a look first?

@Ajacmac
Copy link
Contributor

Ajacmac commented Jul 30, 2018

@skade I'll see what I can do with libserialize/json.rs if that one isn't taken.

Edit: I'm probably on the lower end of the target demographic for this kind of E-mentoring. Assuming I get everything sorted with the first one, would it be ok if I did a few of these (or maybe more than a few) to make sure I really get what's going on?

Thanks for your time!

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 30, 2018
…r=steveklabnik

Document From trait implementations for OsStr, OsString, CString, and CStr

As part of issue rust-lang#51430 (cc @skade).

The allocation and copy claims should be double-checked.

r? @steveklabnik
JohnTitor added a commit to JohnTitor/rust that referenced this issue Aug 3, 2021
Write docs for SyncOnceCell From and Default impl

Part of rust-lang#51430
JohnTitor added a commit to JohnTitor/rust that referenced this issue Aug 3, 2021
Write docs for SyncOnceCell From and Default impl

Part of rust-lang#51430
JohnTitor added a commit to JohnTitor/rust that referenced this issue Aug 3, 2021
Write docs for SyncOnceCell From and Default impl

Part of rust-lang#51430
notriddle added a commit to notriddle/rust that referenced this issue Sep 15, 2021
Manishearth added a commit to Manishearth/rust that referenced this issue Sep 16, 2021
…ocs, r=Mark-Simulacrum

Clean up and add doc comments for CStr

CC rust-lang#51430
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 16, 2021
…ocs, r=Mark-Simulacrum

Clean up and add doc comments for CStr

CC rust-lang#51430
Manishearth added a commit to Manishearth/rust that referenced this issue Sep 16, 2021
…ocs, r=Mark-Simulacrum

Clean up and add doc comments for CStr

CC rust-lang#51430
timClicks added a commit to timClicks/rust that referenced this issue Sep 23, 2021
timClicks added a commit to timClicks/rust that referenced this issue Oct 3, 2021
Among other changes, documents whether allocations are necessary
to complete the type conversion.

Part of rust-lang#51430

Co-authored-by: Giacomo Stevanato <giaco.stevanato@gmail.com>

Co-authored-by: Joshua Nelson <github@jyn.dev>
timClicks added a commit to timClicks/rust that referenced this issue Oct 8, 2021
Among other changes, documents whether allocations are necessary
to complete the type conversion.

Part of rust-lang#51430

Co-authored-by: Giacomo Stevanato <giaco.stevanato@gmail.com>

Co-authored-by: Joshua Nelson <github@jyn.dev>
timClicks added a commit to timClicks/rust that referenced this issue Oct 8, 2021
timClicks added a commit to timClicks/rust that referenced this issue Oct 8, 2021
Among other changes, documents whether allocations are necessary
to complete the type conversion.

Part of rust-lang#51430

Co-authored-by: Giacomo Stevanato <giaco.stevanato@gmail.com>

Co-authored-by: Joshua Nelson <github@jyn.dev>
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Oct 9, 2021
…ersions, r=m-ou-se

Add documentation to boxed conversions

Among other changes, documents whether allocations are necessary
to complete the type conversion.

Part of rust-lang#51430, supersedes rust-lang#89199
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Oct 9, 2021
…ersions, r=m-ou-se

Add documentation to boxed conversions

Among other changes, documents whether allocations are necessary
to complete the type conversion.

Part of rust-lang#51430, supersedes rust-lang#89199
@halvko
Copy link

halvko commented Jun 16, 2022

Hi @skade, I'd like to do libproc_macro/lib.rs and libsyntax/tokenstream.rs

@TornaxO7
Copy link

TornaxO7 commented May 2, 2023

May I ask in which directory you guys are calling rg? I called

rg -trust '(?! \s+///.*\n)(\s+#\[inline\])?\s+fn from\(' -U --pcre2 src/

and

rg -trust '(?! \s+///.*\n)(\s+#\[inline\])?\s+fn from\(' -U --pcre2 src/ | rg '\blib'

but I don't get any outputs from libcore or libstd anymore.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 4, 2024
…-Simulacrum

Document `&CStr` to `CString` conversion

Related to rust-lang#51430
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 4, 2024
…-Simulacrum

Document `&CStr` to `CString` conversion

Related to rust-lang#51430
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 5, 2024
…-Simulacrum

Document `&CStr` to `CString` conversion

Related to rust-lang#51430
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 5, 2024
Rollup merge of rust-lang#120458 - rytheo:cstr-conversion-doc, r=Mark-Simulacrum

Document `&CStr` to `CString` conversion

Related to rust-lang#51430
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 11, 2024
…ark-Simulacrum

Document various I/O descriptor/handle conversions

Related to rust-lang#51430
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 11, 2024
Rollup merge of rust-lang#120459 - rytheo:handle-conversion-docs, r=Mark-Simulacrum

Document various I/O descriptor/handle conversions

Related to rust-lang#51430
@palozano
Copy link

palozano commented Sep 3, 2024

Is this something I could pick up? I want to contribute but don't know if this is a good issue.

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. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-medium Medium priority 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