Rust 2018

[jebrosen]

This includes two changes:

• Running cargo fix --edition --all-features and enabling edition = "2018" for every crate. Minor additional changes/cleanups were made, such as one instance of dyn used as a variable name.
• Setting #![deny(rust_2018_idioms)] and running cargo fix --all-features for all crates except for examples.
  • cargo fix seemed to mess up one or two extern crates, but overall worked quite well!
  • This is a fairly opinionated change and can certainly be done separately or not at all. However, it is the intention that these lints will become at least warn-by-default in the future: Tracking issue for warning for rust_2018_idioms by default rust-lang/rust#54910.

This does not address:

• There are a few extern crates that survived because we #[macro_use] them; these would also be possible to remove if desired e.g. #[macro_use] extern crate lazy_static; can be replaced by lazy_static::lazy_static!().
• The tests under core/lib/benches, which already weren't compiling anyway?
• It's no longer tested that 2015 edition crates that use rocket still work. We have at least two PRs that will very likely require application crates to be 2018 anyway (Remove use of decl_macro by using a different hack. #964 and Upgrade to hyper 0.12 (WIP) #1008).
• The hello_2018 example is mostly redundant now, but I think it does some tricks with macro scoping tests that might be worth keeping if the example is removed.

[SergioBenitez]

Aside from the failing --release test, on a very quick pass, I noticed that there are many places where a ::external_crate:: was converted to crate::external_crate:: where as Rust 2018 should allow us to use external_crate:: unambiguously. This would also mean that the PR churn would be reduced. Could we make this change?

[jebrosen]

Yeah, I'll take a closer look at those and favor external:: over crate::external::

[jebrosen]

... I noticed that there are many places where a ::external_crate:: was converted to crate::external_crate:: where as Rust 2018 should allow us to use external_crate:: unambiguously. This would also mean that the PR churn would be reduced. Could we make this change?

It looks like many or most of these are false positives; for example ::proc_macro2 is not the crate proc_macro2 (which is not in Cargo.toml). It actually maps to the pub use devise::proc_macro2;, which doesn't get the same treatment.

[SergioBenitez]

This looks phenomenal, and I'm excited to merge it soon! Apart from the changes I've made, I had the following comments:

1. Why are we denying rust_2018_idioms everywhere?
2. Many intra-crate links are now failing to resolve and should be updated accordingly.
3. Can we take advantage of lifetime elision in impl anywhere?
4. Are there any existing modules where we should remove foo/mod.rs in favor of foo.rs?
5. I found a few spurious ::std here and there, which I replaced with std::. Perhaps there are other latent ::external_crate uses as well?

[jebrosen]

1. Why are we denying rust_2018_idioms everywhere?

I think I did that primarily to make sure I caught everything / for demonstration purposes. It could just as easily be warn, or left at the default and they will become warn on their own eventually.

2. Many intra-crate links are now failing to resolve and should be updated accordingly.

Right, thanks for the reminder.

3. Can we take advantage of lifetime elision in impl anywhere?

Almost certainly; FromRequest comes to mind as a likely candidate for applying this. I'll take a look at that as well.

4. Are there any existing modules where we should remove foo/mod.rs in favor of foo.rs?

Nothing jumps out at me, and I think foo/mod.rs is actually clearer in rocket_contrib at least.

5. I found a few spurious ::std here and there, which I replaced with std::. Perhaps there are other latent ::external_crate uses as well?

Could be; I'll look for these at the same time as I look for instances of 3)

---

Do you have a preference for commit order (e.g. one commit per crate), or is this likely to be squashed in the end?

[SergioBenitez]

I think I did that primarily to make sure I caught everything / for demonstration purposes. It could just as easily be warn, or left at the default and they will become warn on their own eventually.

Got it. In that case, I think we should remove all of the denys. It's far too risky, and we should be checking for warnings anyway.

Do you have a preference for commit order (e.g. one commit per crate), or is this likely to be squashed in the end?

I don't mind having multiple commits - this is a significant endeavor after all - but we should make sure that all code/tests compile/pass after each commit is applied. In other words, each commit should be valid on its own.

[jebrosen]

I've redone this from the start with a more consistent and thorough methodology.

For each crate:

• Run cargo fix --edition --all-features
• Set edition = "2018" in Cargo.toml
• Run cargo fix --all-features
• Set #![warn(rust_2018_idioms)] and cargo fix --edition-idioms --all-features
• ack '[^A-Za-z0-9>"]::' to find problems with style and doc links:
  • ::<external-crate>:: --> <external-crate>:: unless it is inside a quote! invocation
  • ::Item -> crate::Item
• Make other manual fixes or adjustments (blank lines, use reordering, Sergio's fixes from the previous changeset, etc.)
• cargo test --all-features to verify and fix tests
• cargo doc --all-features to verify and fix doc links

A similar set of steps was done with everything examples/, with one exception: I fixed the edition idioms but did not add #![warn(rust_2018_idioms)].

I've pushed these changes to let the CI find anything I missed that might differ between features or debug/release.

---

Remaining steps that I want to take before a real review:

• Take advantage of lifetime elision in impl blocks where appropriate.
• There are a few extern crates that survived because of #[macro_use]; some of these can probably be replaced by use <crate>::macro; or <crate>::macro!()

I don't currently plan to handle:

• Testing that 2015 edition crates that use rocket still work. We have at least two ideas that will require application crates to be 2018 anyway (see Remove use of decl_macro by using a different hack. #964 and Upgrade to hyper 0.12 (WIP) #1008).
• Cross-crate doc links don't work, but they didn't work before either -- for example the link to Request::accept() at https://api.rocket.rs/v0.4/rocket/http/struct.Accept.html

[jebrosen]

@SergioBenitez I think this is almost ready for another review. If we like the changes in "elide lifetimes in 'http'" and "elide lifetimes in 'core'", I'll apply the same to other crates. But it is a fairly tedious manual replacement task because of rust-lang/rust#46944 and rust-lang/rust#53738, and I have almost certainly missed several elidable lifetimes.

[SergioBenitez]

This all looks phenomenal to me! One tiny note: pub extern crate foo has been replaced with pub use foo. While I'm okay with this, it means that the documentation no longer says extern crate but pub use. I think I prefer to see extern crate to make it clear that we're reexporting an entire crate as opposed to a regular item.

One more note: I believe the only doc links that don't work are ones that didn't work prior to this change as well. In particular, links in the http crate. Is that right? Are there any new broken doc links?

[jebrosen]

I think I prefer to see extern crate to make it clear that we're reexporting an entire crate as opposed to a regular item.

Agreed. That should be an easy change.

Are there any new broken doc links?

The last time I checked them, and unless something changed, I think the current broken links are 1:1 with cross-crate links in crateA that look like "[crateB::thing]" where crateA does not depend on crateB.

[jebrosen]

Agreed. That should be an easy change.

And done. I only found two or three.

I also just fixed a few warnings I caught at the last minute, and I feel pretty good about this aside from possible commit rewording/squashing.

[SergioBenitez]

Merged in the following series of commits: d9f989a, 2315171, be784a7, 34cb1c1, 90e37be. This was a stellar PR, @jebrosen! Thank you for the phenomenal work!