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

Bad example in FnOnce documentation #47091

Closed
alercah opened this issue Dec 31, 2017 · 5 comments · Fixed by #63845
Closed

Bad example in FnOnce documentation #47091

alercah opened this issue Dec 31, 2017 · 5 comments · Fixed by #63845
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-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. P-medium Medium priority

Comments

@alercah
Copy link
Contributor

alercah commented Dec 31, 2017

In the documentation for FnOnce, there are two examples. The first is

let x = 5;
let square_x = move || x * x;
assert_eq!(square_x(), 25);

This is not a FnOnce closure, as it never moves out of a variable in a way that would prevent it from calling again.

@sfackler sfackler added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Dec 31, 2017
@durka
Copy link
Contributor

durka commented Dec 31, 2017

As a technical point, all closures are FnOnce closures, because they may be called at least once. Fn implies FnMut which implies FnOnce. However, closures that modify their environment are only FnMut and FnOnce (not Fn) and closures that move out of their environment are only FnOnce.

@alercah
Copy link
Contributor Author

alercah commented Dec 31, 2017

Fair point. But as someone new to the documentation, I feel there's an implication in a few places (including the book, but unfortunately that section is frozen) that any move closure is only FnOnce and neither FnMut or Fn, which isn't true.

@durka
Copy link
Contributor

durka commented Dec 31, 2017 via email

@XAMPPRocky XAMPPRocky added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Apr 10, 2018
@steveklabnik steveklabnik added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label May 29, 2018
@steveklabnik steveklabnik added the P-medium Medium priority label Dec 27, 2018
@DevQps
Copy link
Contributor

DevQps commented Aug 9, 2019

I personally think that the first example is better to be removed from the FnOnce page, since (in my opinion) it causes more confusion then that it actually helps. I guess that most people looking up FnOnce already know a bit about closures and might already know about the move || x * x; syntax. The page also references the book on closures, so I don't think its necessary to provide an example for calling by-value. The second example also includes the calling by value part.

I can imagine that users think the first example can only be called once (because they are on the FnOnce page), but this code also works:

fn main() {
    let x = 5;
    let square_x = move || x * x;
    println!("{}", square_x());
    println!("{}", square_x());
}

@steveklabnik What do you think about this? I can pick this up if you agree.

@steveklabnik
Copy link
Member

Sounds good to me!

Centril added a commit to Centril/rust that referenced this issue Aug 26, 2019
…nikomatsakis

Removed a confusing FnOnce example

# Description
See rust-lang#47091 for a discussion.

## Changes
- Removed an example that might suggest readers that square_x is (only) FnOnce.

closes rust-lang#47091
@bors bors closed this as completed in a577316 Aug 27, 2019
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-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. P-medium Medium priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants