propagate handler visibility through route macros

[Chaostheorie]

PR Type

Fix

PR Checklist

• Tests for the changes have been added / updated.
• Documentation comments have been added / updated.
• A changelog entry has been made for the appropriate packages.
• Format code with the latest stable rustfmt.
• (Team) Label with affected crates and semver status.

Overview

The visibility of the instrumented function is inherited to the generated struct by employing syn's ItemFn.vis attribute.

Closes #2696

[robjtede]

I suspect this could be tested in a trybuild test and asserting the compiler output contains the desired warning.

[Chaostheorie]

It doesn't seem like trybuild supports checking of compiler warnings. It would be possible to instead create a test that tests the visibility with modules instead, is this sufficient @robjtede ?

For example, the following should fail …

mod test {
#[get("/test")]
async fn test() ->  … { … }
}

fn main() ->  … {
    …
    App::new()
            .service(test::test)
    …    
}

… while on the other hand, this should work …

mod test {
#[get("/test")]
pub async fn test() ->  … { … }
}

fn main() ->  … {
    …
    App::new()
            .service(test::test)
    …    
}

[robjtede]

Good idea. That would work too, yeah.

[Chaostheorie]

The idea with trybuild tests worked out. Though, this most likely is a breaking change, right?

[robjtede]

Only the question about breakage is keeping this from being merged. I've asked the other contributors for input.

My view is this could reasonably be called a fix and resulting compile errors "relying on a bug".

Also toying with the idea of issuing a warning at compile time or otherwise trying to figure out how to release this in a non-technically-breaking way but nothing promising has come up yet.

[Chaostheorie]

Well, the docs AFAIK never actually mention the 'public' visibility of the routing macros, but since they've been public for at least v3 and now v4.0.1. Further, all examples of the routing macros in the actix-codegen docs are 'private' so they most likely need to be adjusted.

A warning for private visibility is also a problem since, e.g., the following pattern would be valid even after the change:

ServiceConfig Pattern

mod example {
    #[route("/test", method = "GET", method = "HEAD")]
    async fn example() -> HttpResponse {
        HttpResponse::Ok().finish()
    }

    pub fn configure_routes(cfg: &mut ServiceConfig) {
        cfg.service(example);
    }
}

#[actix_web::main]
pub fn main() -> … {
    HttpServer::new(move || {
        App::new()
            .service(web::scope("/example").configure(example::configure_routes))
            .build()
    });
}

The first thought coming to my mind is making the 'default' public visibility, a crate feature that is part of the default feature set. This would limit the problem to projects employing default-features = false, but the 'migration' would at least be easy/ cheap for those affected. In the long term, the feature could be made optional and/ or be removed.

[robjtede]

Makes me sad but we can't really justify this before the next breaking change and I don't see a way to do it without breaks.

[Chaostheorie]

Well, that's to be expected for such a change. Please ping me before the next breaking change, and I'll rebase and, if required, update the patch.

[porkbrain]

Looking forward to this change. It will help us keep our app with 100s of endpoints cleaner.