Skip to content
This repository has been archived by the owner on Mar 3, 2020. It is now read-only.

possibility of merging this back to upstream? #44

Closed
wants to merge 14 commits into from
Closed

possibility of merging this back to upstream? #44

wants to merge 14 commits into from

Conversation

kdy1
Copy link
Contributor

@kdy1 kdy1 commented Dec 16, 2017

It's almost rewrite. I failed to split commits, but if you want, I'll extract part of these pr as a separate pr.


pmutil::smart_quote instead of quote::quote!

As this fork generates lots of code, I needed some ide support and span-aware quasi quotter.

quote! {{
    let __stream = #fut_expr;
}}

// rustfmt, and syntax highlighters work.
Quote::new_call_site()
    .quote_with(smart_quote!(Vars{ fut_expr }, {
        {
             let __stream = fut_expr;
         }
    }))
    .parse()

But currently this crate works with git dependencies, so I should publish something like futures-await-pmutil.

Stream support

Syntax and return types differs from current upstream's.

await! works for future and stream. This uses an associated constant ::__rt::YieldType::NOT_READY.

Yield = Result<T, StreamError>

StreamError has a NotReady variant.

#[async_stream]
fn foo() -> impl Stream<Item = T, Error = E> {
    yield Ok(T);
    
    // Downside of this approach is that we can't yield Err(E)
    let err: E = ..;
    yield Err(err.into());

    // But it's not problem for custom error types.
    let o_err: OtherError = ..; // let's say E: From<OtherError>
    yield Err(o_err.into()); // works because StreamError<E>: From<OE> where OE: Into<E>

    // But I don't think it's huge downside, as when you want to yield *error item*,
    // you can do 
    yield do catch {
         // Assuming E: From<String>
         String::from("Request was invalid, msg...")?;
         // Valid case
         Ok(resp)
    };

     
}

Return = ()

Current return type is ().
If stream v0.2 supports terminating with value, it'll be changed to reflect that.

Currently ? operator is not usable from this scope, as ? in rust means 'error is returned' instead of 'error is yielded as an item'.

Better type inference

#[async]
fn foo() -> io::Result<()> {}

// expanded, omitted lifetime
fn foo() -> impl MyFuture<io::Result<()>> {
    ::__rt::async_future(move || {
        #[allow(unreachable_code)]
        {
             if false {
                 yield;
                 // this macro does not exist, but means let _v:  Type = unreachable!();

                 // When user specified non-result as return type, this improves error message.
                 return unreachable_typed_expr!(Result<_, _>);

                 return unreachable_typed_expr!(io::Result<()> as ::TooLongForHere);
             }
        }
    })
}

Lifetime support

Fixes #11, #17.

#[async]
fn foo<'b, T>(_: &str, _: StructLt<'b>, _: T) -> Result<(), ()> {}

expands to

fn foo<'ret, 'b: 'ret, T: 'ret>(_: &'ret str, _: StructLt<'b>, _ :T) -> impl MyFuture<Result<(), ()>> + 'ret {}

But this isn't complete, and should implement opt-out.

Heterogeneous await

await!({
    if a {
        load_cache()
    } else {
         call_http()
    }
})

is equivalent to

{
    if a {
        await!(load_cache())
    } else {
         await!(call_http())
    }
}

await! macro lives in futures-async-macro

I did this before extracting pmutil as proc_macro can't export utils, and I'm not sure if this was a good idea.

This is a huge change.

 - stream is supported

For `impl Stream<Item = T, Error = E>`,
 yield type is `Result<T, StreamError<E>>` and return type is `()`.
 This signature reflects the fact that stream from futures v0.1
 does not support terminating itself with error, and if it's
 supported in future, fn signature will be changed.
StreamError is a internal error type which has `NotReady` variant.

 - non-static lifetime is supported

This is implemented by prepending a special
 lifetime `'__returned_future` and bounding returned future like
 `impl Future + '__returned_future`.
But this should **not** be done if signature matters
 (e.g. inside trait impl).
Opt out is not implemented yet.

 - yield / return types are explicitly annotated

So rustc can use them for type inference and error reporting.

 - await! macro works for different type of futures

As user get `Result<T, E>` from `await!` regardless of future provided,
 `await!(if a { fut_a } else { fut_b })` expands to
 `if a { await!(fut_a) } else { await!(fut_b) }` and
 give `Result<T, E>` back to user.
@kdy1
Copy link
Contributor Author

kdy1 commented Dec 16, 2017

I think this can be merged when futures v0.2 is realeased and it supports terminating stream with value.

TODOs

  • allow lifetime opt-out

  • allow static lifetime

  • handle lifetime bounds in where clauses

  • handle custom lifetimes

  • determine syntax for stream

  • publish pmutil to crates.io

  • fetch ui tests from upstream

  • improve error handling for await! macro

  • provide a macro for yield futures_await::__rt::YieldType::NOT_READY;


Edit(2017/12/19): some more

  • (maybe) remove lifetime support
    I think fixing lifetime issues of generators itself and using lifetime elision would be better idea.
  • (maybe) await_item! macro to await one item.

@alexcrichton
Copy link
Owner

Thanks for the PR! Sorry I'm having a little difficulty following the changes though. Before I dive into the code though I was hoping I could get a bit more of a high-level overview about the intended changes?

  • First up, can you explain in a bit more detail what pmutil is? I'm not too familiar with it and would be curious to learn more!
  • For streams I'd personally like to avoid the usage of yield right now as I think it's exposing low-level implementation details of generators which may be tweaked over time (as this PR is showing). In other words I'd prefer to avoid yield being a stable part of the macro.
  • For better type inference, do you have an example of what this improves? Sounds like a great addition regardless!
  • I'll hold off on thinking about the lifetime support for now. That's a pretty tricky question I'm not sure we're ready to tackle yet, but once I understand moreso what's happening elsewhere I can try to dive in!
  • Finally could you explain in a bit more detail the rationale for the heterogeneous await?

@kdy1
Copy link
Contributor Author

kdy1 commented Dec 19, 2017

  • First up, can you explain in a bit more detail what pmutil is? I'm not too familiar with it and would be curious to learn more!

pmutil is a replacement for quote.

quote has two problems

  • precise respanning is almost impossible
    respan(span, quote!( { #tokens } ))
    will respan every token even if span of #tokens was correct.
    As spans of type annotation like
if false {
    return {
        let _v: io::Result<()> = unreachable!();
        _v
    });
}

are used for error reporting, they should be correct.

  • development tools can't work well with it
    Because #tokens isn't valid rust, they can't.
    But quoter from pmutil abuses error recovery, because almost all parsers for development tool implement fairly good error recovery.

I checked that rustfmt and syntax highlighter for rustdoc / vscode works.


  • For streams I'd personally like to avoid the usage of yield right now as I think it's exposing low-level implementation details of generators which may be tweaked over time (as this PR is showing). In other words I'd prefer to avoid yield being a stable part of the macro.

I think this is correct. I'll take some time to think more about this.

  • For better type inference, do you have an example of what this improves? Sounds like a great addition regardless!

await! works both for future and stream because it use type inference trick inspired by Default::default().
It does

yield ::futures::__rt::YieldType::not_ready();

,
but it can't work without type annotation as type inference does not work well for generator literal.

Even if futures::__rt::gen_future specifies Yield = (), it doen't work because type of generator literal determined before being passed to function.

And it also improves error reporting.

For example,
not-a-result.stderr
contains

error[E0308]: mismatched types
 --> $DIR/not-a-result.rs:8:13
  |
8 | fn foo() -> u32 {
  |             ^^^ expected enum `std::result::Result`, found u32
  |
  = note: expected type `std::result::Result<_, _>`
             found type `u32`

It's because type annotation has return Result<_, _> before return u32 which is return type.

UI test does not exists, but it improves error message for other user's mistake.

error[E0308]: mismatched types
  --> tests/stream-smoke.rs:33:18
   |
33 |         yield Ok(ch);
   |                  ^^ expected char, found u8

error: aborting due to previous error

error: Could not compile `futures-await`.

for

#[async_stream]
fn chars(src: &str) -> impl Stream<Item = char, Error = ()> {
    for ch in src.bytes() {
        yield Ok(ch);
    }
}

instead of

error[E0271]: type mismatch resolving `<impl futures::__rt::MyStream<u8, <[generator@tests/smoke.rs:122:32: 1:5 _] as futures::__rt::Generator>::Return> as futures::Stream>::Item == char`
   --> tests/smoke.rs:122:16
    |
122 | fn _chars() -> Result<(), i32> {
    |                ^^^^^^^^^^^^^^^ expected u8, found char
    |
    = note: required for the cast to the object type `futures::Stream<Error=i32, Item=char>`

error: aborting due to previous error

error: Could not compile `futures-await`.

for

#[async_stream(boxed, item = char)]
fn _chars() -> Result<(), i32> {
    for c in "abc".bytes(){
        stream_yield!(c);
    }
    Ok(())
}

  • I'll hold off on thinking about the lifetime support for now. That's a pretty tricky question I'm not sure we're ready to tackle yet, but once I understand moreso what's happening elsewhere I can try to dive in!

I think I should too. Current implementation works for free functions, but while writing a lexer with this I noticed that this sucks if it's used inside impl block because lifetime 'ret is declared on function. At now, I'm not sure about the correct way to support lifetime, and I'm considering reverting lifetime support.


  • Finally could you explain in a bit more detail the rationale for the heterogeneous await?

Primarily I did it to test what can proc macro do, and to see accuracy of respanning. I just didn't revert it because user gives impl Future to await!, and gets Result back, so type of future isn't important at all.

(I don't mind reverting it)

I mean,

// assuming error has timeout variant..

let req = if let Some(timeout) = timeout {
    req.with_timeout(timeout).map_err(From::from)
} else {
    req
};

await!(req);

doesn't compile, because type of req cannot be determined.

Alternatively, code below works.

let req = if let Some(timeout) = timeout {
    await!(req.with_timeout(timeout).map_err(From::from))
} else {
    await!(req)
};

But I think code below makes meaning of await more obvious.

let req = await!(if let Some(timeout) = timeout {
    req.with_timeout(timeout).map_err(From::from)
} else {
    req
});

Edit: formatting

@kdy1
Copy link
Contributor Author

kdy1 commented Dec 19, 2017

Lifetime issue seems like rust-lang/rust#45259 (or implicit self referential struct).

#![feature(generators)]

fn main() {
    move || {
        yield true;

        let mut i = "";
        let r = &i;
        yield panic!();
    };
}

fails to compile with

error[E0626]: borrow may still be in use when generator yields
 --> src/main.rs:8:22
  |
8 |         let r = &i;
  |                      ^
9 |         yield panic!();
  |         -------------- possible yield occurs here

Immovable generator will fix this lifetime issue.

@kdy1
Copy link
Contributor Author

kdy1 commented Dec 22, 2017

I discovered quote_spanned! macro and pr for Span::join api.

When the pr is merged, I'll remove pmutil and update this pr to use quote_spanned!.

Span::join is required to replace Quote::from_tokens with quote_spanned!.
Currently from_tokens is used to make error reporting correct.


Edit: added description

@alexcrichton
Copy link
Owner

Ok thanks for the explanation! (and sorry for the slow response)

I think nowadays quote should be up to the job for various bits and pieces? Would it also be possible to split this apart to consider each feature separately? Improvements to span information and/or type inference seem unambiguously good, but other tweaks to apis and interfaces may want to hold off for a bit longer

@kdy1
Copy link
Contributor Author

kdy1 commented Jan 2, 2018

Opened #49

@kdy1
Copy link
Contributor Author

kdy1 commented Mar 11, 2018

Closing as stream and lifetime issues are solved, and type inference is extracted to separated pull requests (#46 #49).

Lifetime: #53 (comment)

@kdy1 kdy1 closed this Mar 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants