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

Nested closures give suboptimal move suggestion #64008

Open
oli-obk opened this issue Aug 29, 2019 · 5 comments · Fixed by #106575
Open

Nested closures give suboptimal move suggestion #64008

oli-obk opened this issue Aug 29, 2019 · 5 comments · Fixed by #106575
Labels
A-closures Area: Closures (`|…| { … }`) A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Aug 29, 2019

The following code needs a move on the map closure, but the suggestion first tells us to add it to the flat_map closure. After following that suggestion, the next error will tell us to put it on the map closure. This terminates correctly, but should suggest the final thing immediately. Note that with many nested levels of closures, there will be one error-fix-recompile cycle per nested closure until we get it working.

pub struct X;

pub fn foo<'a>(
    bar: &'a X,
) -> impl Iterator<Item = ()> + 'a {
    Some(()).iter().flat_map(|()| {
        Some(()).iter().map(|()| { bar; })
    })
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error: captured variable cannot escape `FnMut` closure body
 --> src/lib.rs:7:9
  |
6 |     Some(()).iter().flat_map(move |()| {
  |                                      - inferred to be a `FnMut` closure
7 |         Some(()).iter().map(|()| { bar; })
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ returns a reference to a captured variable which escapes the closure body
  |
  = note: `FnMut` closures only have access to their captured variables while they are executing...
  = note: ...therefore, they cannot allow references to captured variables to escape

error: aborting due to previous error

error: Could not compile `playground`.

To learn more, run the command again with --verbose.

@Mark-Simulacrum Mark-Simulacrum added A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Aug 30, 2019
@estebank
Copy link
Contributor

Note that both moves are needed.

@estebank estebank added A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 30, 2019
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 30, 2019

Oh... hmm... I thought I had a situation where only the inner one is needed. I'll investigate more.

@estebank
Copy link
Contributor

Triage: little change.

Current output:

error[E0373]: closure may outlive the current function, but it borrows `bar`, which is owned by the current function
 --> src/lib.rs:6:30
  |
6 |     Some(()).iter().flat_map(|()| {
  |                              ^^^^ may outlive borrowed value `bar`
7 |         Some(()).iter().map(|()| { bar; })
  |                                    --- `bar` is borrowed here
  |
note: closure is returned here
 --> src/lib.rs:6:5
  |
6 | /     Some(()).iter().flat_map(|()| {
7 | |         Some(()).iter().map(|()| { bar; })
8 | |     })
  | |______^
help: to force the closure to take ownership of `bar` (and any other referenced variables), use the `move` keyword
  |
6 |     Some(()).iter().flat_map(move |()| {
  |                              ++++

and when applying the suggestion we fail to suggest the second necessary move:

error: captured variable cannot escape `FnMut` closure body
 --> src/lib.rs:7:9
  |
4 |     bar: &'a X,
  |     --- variable defined here
5 | ) -> impl Iterator<Item = ()> + 'a {
6 |     Some(()).iter().flat_map(move |()| {
  |                                      - inferred to be a `FnMut` closure
7 |         Some(()).iter().map(|()| { bar; })
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^---^^^^
  |         |                          |
  |         |                          variable captured here
  |         returns a reference to a captured variable which escapes the closure body
  |
  = note: `FnMut` closures only have access to their captured variables while they are executing...
  = note: ...therefore, they cannot allow references to captured variables to escape

@estebank estebank added A-closures Area: Closures (`|…| { … }`) D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. labels Nov 11, 2022
estebank added a commit to estebank/rust that referenced this issue Jan 11, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 2, 2023
Suggest `move` in nested closure when appropriate

Fix rust-lang#64008.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 3, 2023
Suggest `move` in nested closure when appropriate

Fix rust-lang#64008.
@bors bors closed this as completed in e6b84eb Feb 3, 2023
@safinaskar
Copy link
Contributor

@estebank , the compiler still gives bad advice. Please, reopen the bug. Consider this code:

fn main() {
    let get = |path: &str| || {
        assert!(path.starts_with('/'));
    };
}

Playground: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=7d311a40c9b0662af5b2894f7ce8e6eb
Playground currently uses nightly 75a0be9 (as reported by playground), which (as well as I understand) includes recent fix ( 6b94f4d ). But still compiler error is bad:

Compiling playground v0.0.1 (/playground)
warning: unused variable: `get`
 --> src/main.rs:2:9
  |
2 |     let get = |path: &str| || {
  |         ^^^ help: if this is intentional, prefix it with an underscore: `_get`
  |
  = note: `#[warn(unused_variables)]` on by default

error: lifetime may not live long enough
 --> src/main.rs:2:28
  |
2 |       let get = |path: &str| || {
  |  ______________________-___-_^
  | |                      |   |
  | |                      |   return type of closure `[closure@src/main.rs:2:28: 2:30]` contains a lifetime `'2`
  | |                      let's call the lifetime of this reference `'1`
3 | |         assert!(path.starts_with('/'));
4 | |     };
  | |_____^ returning this value requires that `'1` must outlive `'2`
  |
help: consider adding 'move' keyword before the nested closure
  |
2 |     let get = |path: &str| move || {
  |                            ++++

warning: `playground` (bin "playground") generated 1 warning
error: could not compile `playground` due to previous error; 1 warning emitted

The compiler suggests adding move before ||. But after this change the code still doesn't compile! So compiler's advice is unhelpful

@estebank
Copy link
Contributor

estebank commented Feb 7, 2023

I'm not entirely sure what we should suggest in that case, if anything.

@estebank estebank reopened this Feb 7, 2023
@estebank estebank added A-lifetimes Area: Lifetimes / regions and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. labels Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: Closures (`|…| { … }`) A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions P-low Low priority T-compiler Relevant to the compiler 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