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

Rust 2018 #161

Closed
DGolubets opened this issue Oct 23, 2018 · 13 comments
Closed

Rust 2018 #161

DGolubets opened this issue Oct 23, 2018 · 13 comments

Comments

@DGolubets
Copy link
Contributor

I was experimenting yesterday, upgrading my little project to the 2018 edition. One of the changes there is about crates and imports. To cut it short, following the guide, I should remove extern crate actix and the rest should just work. However there is a problem.

The problem is actix::prelude has a module named actix and it causes a conflict with the crate name itself. I was able to fix the error in 2 ways:

  1. Replace prelude with specific imports. This obviously works, but defeats the purpose of prelude.
  2. Add :: prefix: use ::actix::prelude::*;. I would make a peace importing it this way, however rustfmt rewrites it removing the prefix.

It's a minor issue, but it would be nice to have it fixed (if possible at all) for 2018 edition release, for smoother migration.

@DoumanAsh
Copy link
Contributor

DoumanAsh commented Oct 23, 2018

Oh this is good idea, we need to prepare to Rust 2018.
Though as far as I remember by default all code will be compiled in 2015 edition mode so it is no critical concern, but nothing wrong with modernizing code to latest edition. Need to check it

I'd say it would be better to do all imports change before next major version change to avoid problems, but I'd prefer to have prelude still so 2 seems better
Most likely it will just take a while for rustfmt get updated to handle 2018/2015 editions

@DGolubets
Copy link
Contributor Author

It seems they are aware in rustfmt. So :: will work fine. It's not perfect aesthetically though.

@subhojit777
Copy link

I made it work by using the actix sub-module using actix_web. Like this, actix_web::actix::*

@NPN
Copy link
Contributor

NPN commented Dec 9, 2018

I'm guessing the reason why the prelude module reexports the actix crate through its own module is so that users of actix-web can access the actix crate (use actix_web::actix::*;, as mentioned above) without having to put actix as a dependency in their Cargo.toml. This also prevents a crate which uses actix-web and actix from using incompatible versions of the two crates and running into errors.

But, since this causes problems with Rust 2018, why not move the actix module from actix::prelude to actix-web? That is, change actix-web's lib.rs to:

#[macro_use]
extern crate actix as actix_inner;

// ...

pub mod actix {
    // You could even delete this and just use `actix_inner::` everywhere
    extern crate actix;
    pub use self::actix::actors::resolver;
    pub use self::actix::actors::signal;
    pub use self::actix::fut;
    pub use self::actix::msgs;
    // prelude no longer has an actix module
    pub use self::actix::prelude::*;
    pub use self::actix::{run, spawn};

    // Instead, we create it here
    pub mod actix {
        pub use actix_inner::actors;
        pub use actix_inner::dev;
        pub use actix_inner::fut;
        pub use actix_inner::io;
        pub use actix_inner::msgs;
        pub use actix_inner::prelude::*;
        pub use actix_inner::registry::SystemService;
        pub use actix_inner::utils::{Condition, IntervalFunc, TimerFunc};
    }
}

(We need to use actix_inner:: to avoid another crate/module error.)

This would fix the Rust 2018 error while letting users of actix-web still access the actix crate. It also wouldn't break any code for actix users, since they can just import what they want from the actix crate itself.

@fafhrd91
Copy link
Member

fafhrd91 commented Dec 9, 2018

@DoumanAsh do you have time for this?

@DoumanAsh
Copy link
Contributor

@fafhrd91 To be honest I think it would be better if we re-export actix crate fully as public dependency.
It would simplify things for everyone, including us

I'll look into it, if you're still against re-exporting whole actix

@fafhrd91
Copy link
Member

fafhrd91 commented Dec 9, 2018

I don’t think we need reexport at all, 2018 edition simplify extern crates

@DoumanAsh
Copy link
Contributor

Act of re-exporting is only convenience for users not to add actix as dependency in Cargo.toml, we might as well not to re-export any of actix in actix-web then, considering that accessing from actix crate is easier with 2018.
Should we just remove it altogether?

Personally I only see it as either full re-export, or let user to manually add actix as dependency.
Current state is a bit ugly.

@fafhrd91
Copy link
Member

fafhrd91 commented Dec 9, 2018

We should remove actix reexport. Question is in what form to do this for current releases. For sure next release should switch to 2018 but not current one

@DoumanAsh
Copy link
Contributor

DoumanAsh commented Dec 9, 2018

We cannot really do anything with current state, removing actix re-export would be breaking change.
It might be inconvenience, but we can only leave it as it is,

For 0.8 in both actix and actix-web we'll need to do clean-up and upgrade it to 2018 edition

@fafhrd91
Copy link
Member

fafhrd91 commented Dec 9, 2018

I agree, but 0.8 will take some time.

@DoumanAsh
Copy link
Contributor

Removing actix module in prelude #185

@DoumanAsh
Copy link
Contributor

While it is technically breaking change, it should not significantly affect people as 2018 edition is going to be prevalent
New actix-web pins down to actix 0.7.9 which doesn't have actix re-export

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants