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

missing rust_2021_incompatible_closure_captures with async block boxed future thingy #88444

Open
ehuss opened this issue Aug 28, 2021 · 3 comments
Assignees
Labels
A-closures Area: Closures (`|…| { … }`) A-diagnostics Area: Messages for errors, warnings, and lints A-edition-2021 Area: The 2021 edition A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ehuss
Copy link
Contributor

ehuss commented Aug 28, 2021

Given the following code:

#![allow(unused)]
// #![warn(rust_2021_incompatible_closure_captures)]
#![feature(capture_disjoint_fields)]
use futures::future::BoxFuture;
use futures::FutureExt;

struct Connect {
    host: String,
    insecure: bool,
}

pub struct GremlinContext;

pub enum Command {
    Print(Option<String>),
    Exec(Box<dyn FnOnce(&GremlinContext) -> BoxFuture<'static, Vec<Command>> + Send>),
}

struct ConnectionOptions<'a> {
    host: &'a str,
    insecure: bool,
}

fn handle(args: Vec<String>) -> Vec<Command> {
    let connect = Connect {
        host: "".to_string(),
        insecure: false,
    };
    let task = |_ctx: &GremlinContext| {
        let future = async move {
            let options = ConnectionOptions {
                host: connect.host.as_str(),
                insecure: connect.insecure,
            };
            vec![Command::Print(Some("Connected!".into()))]
        };
        future.boxed()
    };

    vec![Command::Exec(Box::new(task))]
}
fn main() {}

With feature disabled, and the warning enabled, there is no warning issued. With the feature enabled, this fails with the error:

error[E0597]: `connect.insecure` does not live long enough
  --> src/main.rs:33:27
   |
29 |     let task = |_ctx: &GremlinContext| {
   |                ----------------------- value captured here
...
33 |                 insecure: connect.insecure,
   |                           ^^^^^^^^^^^^^^^^ borrowed value does not live long enough
...
40 |     vec![Command::Exec(Box::new(task))]
   |                        -------------- cast requires that `connect.insecure` is borrowed for `'static`
41 | }
   | - `connect.insecure` dropped here while still borrowed

I think it is missing a suggestion for let _ = &connect in the closure.

Found in the 2021 crater run for https://crater-reports.s3.amazonaws.com/pr-87190-3/try%23a7a572ce3edd6d476191fbfe92c9c1986e009b34/reg/gremlin-cli-0.1.0/log.txt

rustc 1.56.0-nightly (ac50a5335 2021-08-27)
binary: rustc
commit-hash: ac50a53359328a5d7f2f558833e63d59d372e4f7
commit-date: 2021-08-27
host: x86_64-apple-darwin
release: 1.56.0-nightly
LLVM version: 13.0.0

cc @rust-lang/wg-rfc-2229

@ehuss ehuss added A-diagnostics Area: Messages for errors, warnings, and lints A-closures Area: Closures (`|…| { … }`) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` A-edition-2021 Area: The 2021 edition labels Aug 28, 2021
@Mark-Simulacrum Mark-Simulacrum added C-bug Category: This is a bug. A-edition-2021 Area: The 2021 edition and removed A-edition-2021 Area: The 2021 edition labels Aug 28, 2021
@arora-aman
Copy link
Member

@rustbot claim

So this is a bug in the capture analysis. The inner closure captures the field insecure by move, but where we propogate captures from inner to outer closers, we treat move of copy types as an ImmBorrow

@arora-aman
Copy link
Member

So upon further thought the capture analysis doing the correct thing. We'd have seen similar behavior on Rust 2018.

I tried the following code that only uses a boolean within the closure:

#![allow(unused)]

use futures::future::BoxFuture;
use futures::FutureExt;

pub struct GremlinContext;

pub enum Command {
    Print(Option<String>),
    Exec(Box<dyn FnOnce(&GremlinContext) -> BoxFuture<'static, Vec<Command>> + Send>),
}

fn handle(args: Vec<String>) -> Vec<Command> {
    let f = false;

    let task = |_ctx: &GremlinContext| {
        let future = async move {
            let options = f;
            vec![Command::Print(Some("Connected!".into()))]
        };
        future.boxed()
    };

    vec![Command::Exec(Box::new(task))]
}
fn main() {}

I tried a rust build from early 2020 and it failed

❯ cargo bisect-rustc --test-dir=foo --start=2020-01-01

installing nightly-2020-01-01
rustc for x86_64-unknown-linux-gnu: 102.98 KB / 57.99 MB  0.17 % 1019.28 KB/s 5cargo for x86_64-unknown-linux-gnu: 4.80 MB / 4.80 MB [===] 100.00 % 4.47 MB/s testing...
RESULT: nightly-2020-01-01, ===> Yes
uninstalling nightly-2020-01-01

ERROR: the start of the range (nightly-2020-01-01) must not reproduce the regression

This is another variant of #88114. It would've been nice if this suggested that the outer clouser should be a move a closure.

@nikomatsakis
Copy link
Contributor

I agree this is a duplicate of #88114

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-edition-2021 Area: The 2021 edition A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants