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

impl Generator complains about missing named lifetime if yielded expression contains a borrow #44197

Closed
Arnavion opened this issue Aug 30, 2017 · 11 comments · Fixed by #45337
Closed
Labels
A-coroutines Area: Coroutines C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Arnavion
Copy link

Arnavion commented Aug 30, 2017

... even though the value of the expression does not borrow from anything.

rustc 1.21.0-nightly (c11f689d2 2017-08-29)
binary: rustc
commit-hash: c11f689d2475dd9ab956e881238d5d7b6b485efb
commit-date: 2017-08-29
host: x86_64-pc-windows-msvc
release: 1.21.0-nightly
LLVM version: 4.0

Case 1:

#![feature(conservative_impl_trait, generators, generator_trait)]

use std::ops::{ Generator, GeneratorState };

fn foo(_: &str) -> String {
    String::new()
}

fn bar(baz: String) -> impl Generator<Yield = String, Return = ()> {
    move || {
        yield foo(&baz);
    }
}

fn main() {
    assert_eq!(bar(String::new()).resume(), GeneratorState::Yielded(String::new()));
}

This gives:

error[E0564]: only named lifetimes are allowed in `impl Trait`, but `` was found in the type `[generator@src\main.rs:10:2: 12:3 baz:std::string::String ((), std::string::String, &str, fn(&str) -> std::string::String {foo})]`
 --> src\main.rs:9:24
  |
9 | fn bar(baz: String) -> impl Generator<Yield = String, Return = ()> {
  |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Workaround:

-        yield foo(&baz);
+        let quux = foo(&baz);
+        yield quux;

Case 2:

#![feature(conservative_impl_trait, generators, generator_trait)]

use std::ops::{ Generator, GeneratorState };

fn foo(_: &str) -> Result<String, ()> {
    Err(())
}

fn bar(baz: String) -> impl Generator<Yield = String, Return = ()> {
    move || {
        if let Ok(quux) = foo(&baz) {
            yield quux;
        }
    }
}

fn main() {
    assert_eq!(bar(String::new()).resume(), GeneratorState::Complete(()));
}

This gives

error[E0564]: only named lifetimes are allowed in `impl Trait`, but `` was found in the type `[generator@src\main.rs:10:2: 14:3 baz:std::string::String ((), std::result::Result<std::string::String, ()>, &str, std::string::String, fn(&str) -> std::result::Result<std::string::String, ()> {foo})]`
 --> src\main.rs:9:24
  |
9 | fn bar(baz: String) -> impl Generator<Yield = String, Return = ()> {
  |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Workaround:

-        if let Ok(quux) = foo(&baz) {
+        let quux = foo(&baz);
+        if let Ok(quux) = quux {

I assume the borrows in the expressions are the problem because in both cases, adding a + 'static bound to the impl Generator returns the error that baz does not live long enough, even though it does not need to be borrowed past the call to foo(&baz).

@alexcrichton
Copy link
Member

cc @Zoxc

@alexcrichton alexcrichton added the A-coroutines Area: Coroutines label Aug 31, 2017
@alexcrichton
Copy link
Member

This may or may not be the same as #44200

@Zoxc
Copy link
Contributor

Zoxc commented Aug 31, 2017

Like #44200 this is due to how interior types are being calculated.
In both your examples &baz is considered a temporary and lives to the enclosing destruction scope, which are the yield statement and the if statement. Both of these contains a yield expression so &str is being considered part of the generator interior. Now impl Trait only allows explicitly named lifetimes in the return type so it gives an error about that &str, although poorly. You need to pass -Z verbose to the compiler for that error to be usable.

@Arnavion
Copy link
Author

In both your examples &baz is considered a temporary and lives to the enclosing destruction scope

Just to be clear, you're only talking about the situation due to the bug with generators, right? Because in regular Rust the borrow of &baz is limited to the foo(&baz) expression and not any outer match / if let expression that it's part of. (Otherwise something like if let Some(first) = iter.next() { if let Some(second) = iter.next() { } } would not compile and complain about multiple mutable borrows of iter.)

@Zoxc
Copy link
Contributor

Zoxc commented Aug 31, 2017

@Arnavion It's not really a bug (just undesirable semantics), and you typically don't notice these rules unless the types involved have destructors (which references do not) and isn't immediately moved.

@Arnavion
Copy link
Author

Okay, regardless of what we call it, it's something that happens only with generators and not with regular Rust code, so it would be nice to fix it.

@pftbest
Copy link
Contributor

pftbest commented Sep 5, 2017

This code produces the same error:

fn multiply<G>(g: G, x: i32) -> impl Generator<Return = (), Yield = i32>
where
    G: Generator<Return = (), Yield = i32>,
{
    || while let GeneratorState::Yielded(v) = g.resume() {
        yield v * x;
    }
}

Is there some workaround?

@Arnavion
Copy link
Author

Arnavion commented Sep 5, 2017

@pftbest

fn multiply<G>(mut g: G, x: i32) -> impl Generator<Return = (), Yield = i32>
where
    G: Generator<Return = (), Yield = i32>
{
    move || loop {
        let state = g.resume();
        if let GeneratorState::Yielded(v) = state {
            yield v * x;
        }
        else {
            return;
        }
    }
}

If you still want to keep the while let you can do something like:

    move || while let GeneratorState::Yielded(v) = { let state = g.resume(); state } {
        yield v * x;
    }

@pftbest
Copy link
Contributor

pftbest commented Sep 5, 2017

@Arnavion , thanks! Looks like I also hit #38615 when I forgot to add move.

@arielb1
Copy link
Contributor

arielb1 commented Oct 15, 2017

@Zoxc

In both your examples &baz is considered a temporary and lives to the enclosing destruction scope,

That's not actually correct. &baz is a vexpr (rvalue in C++-speak) which is never converted to a lexpr, so the destruction scope is not relevant. vexprs are destroyed at the end of their parent expression (in your case, at the end of foo(&bar)), so they wouldn't be alive at the yield.

@arielb1
Copy link
Contributor

arielb1 commented Oct 15, 2017

If you'll use expr_use_visitor, you should be able to identify all lexpr spots by looking for when the ExprUseVisitor::borrow function is called. Or you could just wait for MIR-based inference.

Zoxc added a commit to Zoxc/rust that referenced this issue Jan 23, 2018
bors added a commit that referenced this issue Jan 23, 2018
Immovable generators

This adds support for immovable generators which allow you to borrow local values inside generator across suspension points. These are declared using a `static` keyword:
```rust
let mut generator = static || {
    let local = &Vec::new();
    yield;
    local.push(0i8);
};
generator.resume();

// ERROR moving the generator after it has resumed would invalidate the interior reference
// drop(generator);
```

Region inference is no longer affected by the types stored in generators so the regions inside should be similar to other code (and unaffected by the presence of `yield` expressions). The borrow checker is extended to pick up the slack so interior references still result in errors for movable generators. This fixes #44197, #45259 and #45093.

This PR depends on [PR #44917 (immovable types)](#44917), I suggest potential reviewers ignore the first commit as it adds immovable types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-coroutines Area: Coroutines 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

Successfully merging a pull request may close this issue.

6 participants