sammhicks / picoserve

An async no_std HTTP server suitable for bare-metal environments, heavily inspired by axum
MIT License
203 stars 25 forks source link

Type error when using `picoserve::routing::Router::nest` #49

Open Sycrosity opened 2 months ago

Sycrosity commented 2 months ago

While attempting to nest a router like so: (simplified example)

pub(crate) fn app_router() -> picoserve::Router<AppRouter, GlobalState> {
    picoserve::Router::new()
        .nest("/api", api_router())
        .route(
            "/",
            get_service(picoserve::response::File::html(include_str!(
                "../dist/index.html"
            ))),
        )
}

pub(crate) fn api_router() -> picoserve::Router<Apiouter, GlobalState> {
    picoserve::Router::new().route(
        ("/", parse_path_segment()),
        picoserve::routing::get(
            |on_off| async move {
                info!("Setting to {}", if on_off { "ON" } else { "OFF" });

                DebugValue(on_off)
            },
        ),
    )
}

I encountered the following error:

error[E0308]: mismatched types
    --> src/net.rs:192:23
     |
156  | type AppRouter = impl picoserve::routing::PathRouter<GlobalState>;
     |                  ------------------------------------------------ the found opaque type
...
192  |         .nest("/api", api_router())
     |          ----         ^^^^^^^^^^^^ expected `Router<_>`, found `Router<AppRouter, GlobalState>`
     |          |
     |          arguments to this method are incorrect
     |
     = note: expected struct `picoserve::Router<_, ()>`
                found struct `picoserve::Router<net::AppRouter, net::GlobalState>`
note: method defined here
    --> /Users/louis/.cargo/registry/src/index.crates.io-6f17d22bba15001f/picoserve-0.12.2/src/routing.rs:1185:12
     |
1185 |     pub fn nest<PD: PathDescription<CurrentPathParameters>>(
     |            ^^^^

which happens regardless of if the impl picoserve::routing::PathRouter<GlobalState> is different for each router or not.

Is this a bug, and if not what have I done wrong here? an example of how to use nesting could be useful to work out what should be done instead.

Amazing project btw!

sammhicks commented 2 months ago

There is indeed a bug! Good find!

I unfortunately don't have time to do a proper patch and release, but I've committed the fix to nest_fix.

I also suggest you change your app_router and api_router functions to have a return type of

picoserve::Router<impl picoserve::routing::PathRouter<GlobalState>, GlobalState>

Sycrosity commented 2 months ago

Thank you for the fix! It's working perfectly now - don't worry about the patch/release, take your time :)

I tried to change the return types, however since I'm using embassy, it needs a concrete type signature, so I kept the

pub type AppRouter = impl picoserve::routing::PathRouter<GlobalState>

annotation for the app_router, but changed it for the api_router.

Should I close the issue, or should that be done when the release is out?

sammhicks commented 2 months ago

I'll close the issue when the release is out.

JuliDi commented 2 months ago

@Sycrosity which version of the Rust compiler are you using and how do you pass the pub type AppRouter into embassy task arguments? I can't get rid of errors like

error: item does not constrain `AppRouter::{opaque#0}`, but has it in its signature
  --> src/main.rs:54:1
   |
54 | #[embassy_executor::task(pool_size = WEB_TASK_POOL_SIZE)]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: consider moving the opaque type's declaration and defining uses into a separate module

Any ideas? I'm using the latest versions of rustc nightly, embassy and the nest_fix branch of this crate.

Sycrosity commented 2 months ago

Any ideas?

I'm using version 1.80.0-nightly (the espup toolchain for esp32 devices)

I passed it like this:

pub type AppRouter = impl picoserve::routing::PathRouter<GlobalState>;

#[task(pool_size = WEB_TASK_POOL_SIZE)]
pub async fn site_task(
    id: usize,
    // uuid: Uuid,
    stack: &'static Stack<WifiDevice<'static, WifiApDevice>>,
    app: &'static picoserve::Router<AppRouter, GlobalState>,
    config: &'static picoserve::Config<Duration>,
    state: GlobalState,
) -> ! {
    .....

I'd likely need to see an example of your code or a repo link to see what you may have done differently.

JuliDi commented 2 months ago

Thanks! I will give it a try later, maybe I can get it to work with what you shared (first of all, I should use the same toolchain to avoid other possibly breaking changes on nightly). I've also uploaded the current main.rs here https://gist.github.com/JuliDi/284280e4b702cafda70e76f9c8ee406a if you want to have a look.

Sycrosity commented 1 month ago

@JuliDi I'm a bit confused as to what MyRouter is on lines 150-153. In my equivalent of the make_app function, I have this:

pub fn app_router() -> picoserve::Router<AppRouter, GlobalState> {

    picoserve::Router::from_service(NotFound)
        .route(
            "/",
            get_service(picoserve::response::File::html(include_str!(
                "../dist/index.html"
            ))),
        )
        .route(
            "/favicon.svg",
            get_service(picoserve::response::File::with_content_type(
                "text/plain; charset=utf-8",
                include_str!("../dist/favicon.svg").as_bytes(),
            )),
        )
        .nest("/api", api_router())
}
JuliDi commented 1 month ago

No special reason, I may have forgotten to revert some changes as I kept editing due to the overwhelming number of errors with opaque type stuff...

Btw, what is your GlobalState?

Thanks for the example, I'll give it a try. But I am unsure whether it solves the opaque type errors. Maybe this is also caused by newer versions of embassy? Which one are you using?

Also, is there a way to use picoserve without nightly features? I feel like these things might break more often in the future.

Sycrosity commented 1 month ago

GlobalState is

pub struct GlobalState {
    pub wifi_creds: WifiCredentialsState,
}
...
pub struct WifiCredentialsState(pub &'static Mutex<CriticalSectionRawMutex, WifiCredentials>);
...
pub struct WifiCredentials {
    ssid: heapless::String<32>,
    password: heapless::String<64>,
}

I am also on the latest version of embassy so I don't believe that is it either

JuliDi commented 1 month ago

Interesting, thanks! I will give it a try next week. Probably some rather trivial mistake after all...

Sycrosity commented 1 month ago

@Sycrosity which version of the Rust compiler are you using and how do you pass the pub type AppRouter into embassy task arguments? I can't get rid of errors like

error: item does not constrain `AppRouter::{opaque#0}`, but has it in its signature
  --> src/main.rs:54:1
   |
54 | #[embassy_executor::task(pool_size = WEB_TASK_POOL_SIZE)]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: consider moving the opaque type's declaration and defining uses into a separate module

On rust version 1.81.0, I now have this error too - seems to be due to something in rust 1.81.0

Sycrosity commented 1 month ago

I feel rather stupid - sammhicks' initial suggestion was all that was needed, with no type State = ... needed at all.

JuliDi commented 1 month ago

What do you mean? Apparently this is a problem with the new TAIT requirements

jcorrie commented 1 month ago

@Sycrosity which version of the Rust compiler are you using and how do you pass the pub type AppRouter into embassy task arguments? I can't get rid of errors like

error: item does not constrain `AppRouter::{opaque#0}`, but has it in its signature
  --> src/main.rs:54:1
   |
54 | #[embassy_executor::task(pool_size = WEB_TASK_POOL_SIZE)]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: consider moving the opaque type's declaration and defining uses into a separate module

On rust version 1.81.0, I now have this error too - seems to be due to something in rust 1.81.0

I've been battling this for a while - using 1.80.0-nightly has resolved the issue, but with anything later I'm encountering this error.

JuliDi commented 1 month ago

As suggested on the Embassy Matrix chat, you need to wrap the type into in its own module like so

mod approuter {
    use picoserve::routing::get;

    pub struct GlobalState {
        pub led_on: bool,
    }

    pub type AppRouter = impl picoserve::routing::PathRouter<GlobalState>;
    pub fn make_app() -> picoserve::Router<AppRouter, GlobalState> {
        picoserve::Router::new().route("/", get(|| async move { "Hello World" }))
    }
}

// and then use it like this e.g.

#[embassy_executor::task(pool_size = WEB_TASK_POOL_SIZE)]
async fn web_task(
    id: usize,
    stack: &'static embassy_net::Stack<Device<'static>>,
    app: &'static picoserve::Router<approuter::AppRouter, approuter::GlobalState>,
    config: &'static picoserve::Config<Duration>,
    state: approuter::GlobalState,
) -> ! {
//...