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

array::from_fn behavior is unclear from documentation #102609

Closed
jgao222 opened this issue Oct 3, 2022 · 5 comments · Fixed by #104839
Closed

array::from_fn behavior is unclear from documentation #102609

jgao222 opened this issue Oct 3, 2022 · 5 comments · Fixed by #104839
Assignees
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@jgao222
Copy link

jgao222 commented Oct 3, 2022

Location

https://doc.rust-lang.org/std/array/fn.from_fn.html and https://doc.rust-lang.org/core/array/fn.from_fn.html

Summary

The documentation for from_fn states

Creates an array [T; N] where each array element T is returned by the cb call.

Arguments

  • cb: Callback where the passed argument is the current array index.

It is not clear what "current array index" refers to, since the function description doesn't necessarily indicate that there is iteration happening to create the resulting array.

Additionally, the first line summary doesn't actually reflect the behavior of the function at all. It mentions "the cb call" -- as if there is only one call to cb.

I think would be more clear if the documentation somehow mentioned that the i-th element in the output array is the result of calling cb(i).

The function try_from_fn is similarly unclear. Anyone have thoughts on this?

@jgao222 jgao222 added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Oct 3, 2022
@vacuus
Copy link
Contributor

vacuus commented Oct 3, 2022

I'm not sure if the Arguments section helps, since it separates the description of the callback somewhat. I think it should all be at the same visual level. Juxtaposing T next to "array element" also seems weird. How about:

Creates an array of type [T; N], where each element is the result of calling cb with that element's index.

(Maybe "returned value from" instead of "result of"?)

@scottmcm
Copy link
Member

scottmcm commented Oct 3, 2022

They're both the same function (std is a re-export of the core one), so https://doc.rust-lang.org/nightly/std/array/fn.from_fn.html is a sufficient link.

I agree the "Arguments" section is pretty unusual (I don't remember seeing it on anything else), and I'm not sure that "callback" is a usual word here. iter::from_fn looks like it just uses f for the generator, FWIW.

The "Examples" section does, I think, really help clarify what it's going to do. But both the prose and the examples could due with some expansion. For example, this should probably get a guarantee of the order in which the items are generated, since it takes impl FnMut, and a corresponding example, though additional guarantees like that take a libs-api sign-off.

Oh, and I might as well cc #100462, which is open with some other suggested improvements.

@scottmcm scottmcm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 3, 2022
@vacuus
Copy link
Contributor

vacuus commented Oct 4, 2022

I don't know about how best to document the type inference in the other issue (though, to throw my 2¢ in, I'm not sure it needs documentation :), but you raise a good point about impl FnMut and ordering. Assuming the guarantee of the order of arguments:

Creates an array of type [T; N], where each element is the result of calling cb with that element's index. It is guaranteed that the index ranges from 0 to the end of the array.

Is "the end of the array" interchangeable with N - 1? Is N - 1 clearer? I'm also unsure about explicitly mentioning the index 0 because of zero-length arrays. Instead of explicitly using numbers, maybe an analogue of iteration can be described. I'm not sure if accurately describing that can avoid expose implementation details and/or uninitialized memory, though.

@scottmcm
Copy link
Member

scottmcm commented Oct 4, 2022

Consider writing out pseudocode as well, as that can be more helpful than prose for some people.

For example, you could rename the function to gen, and describe it as

[gen(0), gen(1), gen(2),, gen(N-2), gen(N-1)]

That's pretty accurate, and the should have them understand that it's not literally 0 or N-1 or anything.

But your note about N == 0 is a good one. Edge cases are a good thing to mention explicitly even if not strictly necessary. For example, you could mention that "If N == 0, an empty array will be returned and the the closure will never be called".

You could also describe it by analogy to other things. For example, this is in a way the eager version of (0..N).map(gen).

@HintringerFabian
Copy link
Contributor

@rustbot claim

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 26, 2022
…n, r=scottmcm

improve array_from_fn documenation

Improves array::from_fn documentation
Fixes rust-lang#102609

There were also unresolved comments from [this PR rust-lang#100462](rust-lang#100462), which have been added to my PR
@bors bors closed this as completed in 1fc83ae Nov 26, 2022
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 T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants