Port tower-web to Edition 2018

[elpiel]

Since I needed it to work with 2018 edition I've spend 10-15 minutes just porting it and at the end the async/await example also works!

I've just ran the cargo fix --edition and fixed one place where ::Error was left. The rest was just changing the example and the Cargo.toml of the project and the example.

Plus the features futures_api is not necessary any more.

Fixes https://github.com/carllerche/tower-web/issues/192#issuecomment-482449190

[lnicola]

Looks good to me. Do you think you can port tower-web-macros too? It's fine if you'd rather leave it for another PR, though.

[lnicola]

It looks like tower-web-macros is just a cargo fix --edition away, at least aside from the await support.

Other than that it might also be worth including the edition idiom fixes? It builds fine after cargo fix --edition-idioms, has some extra uses that need to be removed.

[lnicola]

CI failed because the minimum compiler version needs to be bumped.

[shepmaster]

Since I needed it to work with 2018 edition

Can you expand a bit more what you mean? One big point of the edition system is that libraries written in either edition can work with users in any edition. Even if the library was written using the 2015 edition, it can still be used by 2018 (and vice versa).

Port tower

This doesn't seem correct, as this library is tower -web; tower is a different project. (addressed)

[shepmaster]

including the edition idiom fixes

I also recommend adding #[deny(rust_2018_idioms)] to all crate roots to opt in to extra warnings.

[shepmaster]

remove feature future_api as it's in stable now

This isn't correct as it's not in stable, but it has been stabilized. Rust 1.34.1 is the current stable version and does not have this functionality.

[lnicola]

tower-web works fine from 2018 edition code, but we should be thankful for @elpiel's PR :-). It does (or should) fix the await support, which has been broken in Nightly for a while now.

[shepmaster]

we should be thankful for @elpiel's PR

Sure, I don't intend to be dismissive. My point is that the premise expressed in the comment is incorrect. If the crate wants to be updated (and require the requisite compiler upgrades), then this is a good step towards that.

[elpiel]

@shepmaster

• it's true tower-web has to be mentioned, not tower, my bad
• When I tried to compile the async/await example I got this error:

  
  error[E0670]: `async fn` is not permitted in the 2015 edition
  --> /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/tower-web-0.3.7/src/codegen/async_await.rs:20:1
  |
  20 | async fn map_ok<T, E>(future: T) -> Result<T::Output, E>
  | ^^^^^

error: aborting due to previous error

For more information about this error, try rustc --explain E0670.

Edition related issue. I am working in 2018 with nightly with the `async/await` preview, so that's why I needed it.

- True again about `stabilized` not `in stable`

I would suggest to run with this PR and I can update the `tower-web-macros` to 2018 + the `--edition-idioms` on both once this is done. I rather do it in small steps + sometimes not everything can be fixed automatically so fixing the idioms might cause more time to fix

[shepmaster]

• When I tried to compile the async/await example I got this error

Yes, using async fn requires the 2018 edition; but theoretically only that example needs edition 2018. You can see this enabled for that example:

https://github.com/carllerche/tower-web/blob/c2fbd0b7dc04a02fe98410536033e4d90dbe97d6/examples/async-await/Cargo.toml#L3

I'd actually expect that the error you report is because the crate where you are using tower-web doesn't have edition 2018 enabled in the Cargo.toml.

[lnicola]

@elpiel

Edition related issue. I am working in 2018 with nightly with the async/await preview, so that's why I needed it.

Yeah, await is broken without this PR (regardless of the edition of the user code).

I would suggest to run with this PR and I can update the tower-web-macros to 2018 + the --edition-idioms on both once this is done.

I think tower-web-macros can be ported in this PR, since it just works (cargo test --all --features tokio-async-await works, at least, though with some warnings that don't seem to be edition-related).

@shepmaster

The tower-web code (not the examples or the macro-generated code) has a help that doesn't work on 2015:

use futures::Future;

use std::future::Future as StdFuture;

async fn map_ok<T, E>(future: T) -> Result<T::Output, E>
where
    T: StdFuture,
{
    Ok(await!(future))
}

[elpiel]

@shepmaster Since it's trying to compile tower-web-0.3.7 and the file /src/codegen/async_await.rs has an async fn it's using the 2015 edition (the tower-web edition). My Cargo.toml has the edition = 2018 and this results in the same issue.

Edit: @lnicola Let me then port the macros and run idioms

[shepmaster]

The tower-web code has some helpers that don't work on 2015:

Right, which are only enabled if the user opts-in to them:

https://github.com/carllerche/tower-web/blob/c2fbd0b7dc04a02fe98410536033e4d90dbe97d6/src/codegen/mod.rs#L33-L34

What I'm not understanding is how did this ever work? Did a previous version of the compiler allow async fn in 2015? It also appears that the async-await example isn't compiled in CI, which probably needs to be updated.

[lnicola]

What I'm not understanding is how did this ever work? Did a previous version of the compiler allow async fn in 2015? It also appears that the async-await example isn't compiled in CI, which probably needs to be updated.

Basically yes, when feature-gated off. Now it's a parsing error, apparently. See #193 and #192.

[elpiel]

@lnicola and @shepmaster Thanks a lot for the quick feedback. Whenever I can I help, I've just started with Rust, but I am really happy that I can contribute even with small piece.

I hope these are all required changes from my side in order not to fail the build. Who is going to bump up the required version of Rust in Travis?

[lnicola]

@carllerche Do you think it's worth putting out a breaking release for this and #201 and #182?

I'd be inclined to say yes, since people will be wanting to play with await, and versions are cheap before 1.0.

@elpiel

I updated .travis.yml on your branch. Thanks for the PR and the extra effort on the idiom warnings.

[elpiel]

Thanks @lnicola ! I didn't know if it's ok just to bump it or try to run it nightly on 1.29 :D not sure even if it would work.

[elpiel]

Btw @lnicola what do you think of the #194 for breaking change as well? Not sure how much work and what should be done, but it sounds like the right time to do?

PS: And you're welcome, thanks for the guidance!

[lnicola]

Sure, #194 would be nice. But I don't know how to implement it :smile:, and @carllerche probably doesn't have time for it right now.

[elpiel]

Whoops, there is an issue now running the example that I've just found. I have no idea what's going wrong and how to find and fix this.

error: macro expansion ignores token `move` and any following
  --> <::tower_web::derive_resource macros>:2:14
   |
2  |   # [ derive ( derive_resource_impl , ) ] # [ allow ( unused ) ] enum
   |                ^^^^^^^^^^^^^^^^^^^^
   | 
  ::: src/hyper.rs:51:1
   |
51 | / impl_web! {
52 | |     impl HelloWorld {
53 | |         #[get("/")]
54 | |         async fn hello_world(&self) -> String {
...  |
81 | |     }
82 | | }
   | | -
   | | |
   | |_caused by the macro expansion here
   |   help: you might be missing a semicolon here: `;`
   |
   = note: the usage of `async_move_hax!` is likely invalid in expression context
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0425]: cannot find value `async` in this scope
  --> src/hyper.rs:51:1
   |
51 | / impl_web! {
52 | |     impl HelloWorld {
53 | |         #[get("/")]
54 | |         async fn hello_world(&self) -> String {
...  |
81 | |     }
82 | | }
   | |_^ not found in this scope
   |
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0425`.
error: Could not compile `examples-async-await`.

[lnicola]

@elpie It should be working now.

[elpiel]

Now it works! Thanks @lnicola ! :tada:

[carllerche]

Thanks for the work :+1:. I think this should count as a breaking change. I believe this requires the user of tower-web to also be on 2018?

[carllerche]

also, sorry for the delay. I have a rewrite in progress and I thought it would be ready now (but I am very bad at estimates). I would be fine w/ releasing 0.4 that requires Rust 2018.

[lnicola]

I believe this requires the user of tower-web to also be on 2018?

I don't think so? It does for await, though.

What about #182 (another breaking change)?

[carllerche]

@lnicola ah, i think you might be right? This doesn't do exactly what I thought it would... Though, it would be nice to verify that it still works without the user bumping to 2018... or we can jump straight to a breaking change. I'm fine with whatever route :+1:

[carllerche]

@lnicola bumping the min version is fine.

[lnicola]

The README example builds on both 2015 and 2018 with @elpiel's branch.

[carllerche]

:+1:

[carllerche]

Ok, lets ship this.

[carllerche]

@lnicola @shepmaster assuming both of you are good w/ this PR, we can move forward with it (plz add +1 reviews).

[elpiel]

@carllerche one question though. I couldn't fix the imports required for the impl_web, but maybe you can provide more information on the underlying issue for removing the impl_web macro?

I can merge the change for the impl_web imports as mentioned here: https://github.com/carllerche/tower-web/issues/77#issuecomment-490529266

[carllerche]

@elpiel which imports do you mean?

[elpiel]

@lnicola I can look at that tomorrow. @carllerche importing the impl_web macro in 2018 edition without:

#[macro_use]
extern crate tower_web;

See my comment at https://github.com/carllerche/tower-web/issues/77#issuecomment-490529266 . If we can remove the second import somehow it will be very nice addition as well.

[lnicola]

@lnicola I can look at that tomorrow.

You can simply replace await!(e) with e.await.

[carllerche]

@elpiel The macro uses a ton of hacks to work around lack of proc attr macros (when it was first written).

The best strategy there would be to switch to proc macros.

[elpiel]

@lnicola will do that now. @carllerche do you think I can merge also these changes: https://github.com/elpiel/tower-web/compare/edition-2018...elpiel:macro-fixes-for-2018-edition?expand=1 To be able to do this:

use tower_web::{derive_resource_impl, impl_web};

Instead of this:

use tower_web::{
    derive_resource, derive_resource_impl, impl_web, impl_web_clean_nested,
    impl_web_clean_top_level,
};

before it is rewritten with proc macros?

[carllerche]

@elpiel yes, that is fine w/ me.

[elpiel]

@lnicola because tokio, latest nightly fails (the example):

error[E0432]: unresolved import `std::await`
  --> /home/elpiel/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-async-await-0.1.7/src/lib.rs:35:9
   |
35 | pub use std::await as std_await;
   |         ^^^^^^^^^^^^^^^^^^^^^^^ no `await` in the root

I think we should wait (pun intended) for this to be resolved?! Otherwise I don't know if I want to go into pulling the tokio branch to check if this works. Currently it works with nightly-2019-05-08.

I can add the rust-toolchain for the example and mention it in the README?

[carllerche]

Yes, unfortunately Tokio will probably not track further nightly changes on 0.1.

[lnicola]

Yes, unfortunately Tokio will probably not track further nightly changes on 0.1.

Is there a non-0.1 tokio?

[carllerche]

@lnicola work is starting.

[carllerche]

https://github.com/tokio-rs/tokio/pull/1082

[lnicola]

Without this PR, await is broken. With this PR and without the tokio fixes, await is still broken. I say we merge it?

[elpiel]

I still can add the rust-toolchain file and mention it in README for now? At least it will be working.

[lnicola]

Please don't force a toolchain version. That's the application's decision.

[shepmaster]

force a toolchain version

I don't think that a rust-toolchain in the repo will affect any end users.

[elpiel]

It's also only for the example. As it's a separate project by it's own it shouldn't affect it right? I haven't tried it thought.

[lnicola]

If it's only for the example, I don't have anything against it.

[elpiel]

@lnicola can you check the latest commit? I was wondering if the examples should also be kept in 2015 edition style, but I think this can go to another PR :stuck_out_tongue: