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

traverse_inorder example in ControlFlow compiles, but can't be called #90063

Closed
scottmcm opened this issue Oct 19, 2021 · 3 comments · Fixed by #90196
Closed

traverse_inorder example in ControlFlow compiles, but can't be called #90063

scottmcm opened this issue Oct 19, 2021 · 3 comments · Fixed by #90196
Assignees
Labels
C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@scottmcm
Copy link
Member

scottmcm commented Oct 19, 2021

Found by Inky-developer in https://users.rust-lang.org/t/how-to-use-documentation-example-for-std-controlflow/66115?u=scottmcm

The example compiles, but can't actually be monomorphized as it adds another &mut at each level of the recursion.

It needs a fix like https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=0b534dc88883f0ebd67774f56a857da2

(And it'd probably be good to show a call in the example anyway.)

@scottmcm scottmcm added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-bug Category: This is a bug. labels Oct 19, 2021
@yanok
Copy link
Contributor

yanok commented Oct 19, 2021

@rustbot claim

@yanok
Copy link
Contributor

yanok commented Oct 23, 2021

I made a PR but I have some questions.

First of all, how reasonable is to use FnMut for this example? I think it's a bit counter intuitive that one needs to pass a closure that just prints some elements by mutable reference. Of course, on a second thought, it's a closure that can potentially mutate captured variables, so it must be mutable reference. But then we should have an example with a closure that actually uses this possibility, otherwise it's a bit confusing. But on the other hand, our main goal here is to show the ControlFlow usage, so we probably don't want to over-complicate the rest. So I decided to use Fn trait in my PR.

Then, I'm just curious why the original code compiles. I understand Rust requires monomorphic usage, so it does this check at the call site. But this specific example looks like it can never be monomorphised, so probably it makes sense for the compiler to reject it, even if it is never used. Or am I missing something?

@scottmcm
Copy link
Member Author

(I left some comments on the PR.)

But this specific example looks like it can never be monomorphised, so probably it makes sense for the compiler to reject it, even if it is never used. Or am I missing something?

I agree that it couldn't ever work, because of the heterogeneous recursion. But the pre-monomorphization check is just that all the values meet the trait bounds, which they do in this case as &mut impl FnMut : impl FnMut.

It's an interesting question whether it's something that could feasibly be checked pre-monomorphization. For direct recursion maybe it could, though for indirect recursion I suspect it'd be pretty hard.

yanok added a commit to yanok/rust that referenced this issue Oct 24, 2021
1. The existing example compiles on its own, but any usage fails
   to be monomorphised and so doesn't compile. Fix that by using
   a mutable reference as an input argument.
2. Added an example usage of `traverse_inorder` showing how we
   can terminate the traversal early.

Fixes rust-lang#90063
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 25, 2021
Fix and extent ControlFlow `traverse_inorder` example

Fix and extent ControlFlow `traverse_inorder` example

1. The existing example compiles on its own, but any usage fails to be monomorphised and so doesn't compile. Fix that by using Fn trait instead of FnMut.
2. Added an example usage of `traverse_inorder` showing how we can terminate the traversal early.

Fixes rust-lang#90063
@bors bors closed this as completed in 508fada Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants