uri! scope resolution issues

[jebrosen]

Now that uri! internally uses macro_rules! instead of macro (#964), path resolution works differently. As far as I can tell this is a direct consequence of the fact that macro was hygienic with respect to paths, resolving them at the macro definition site i.e. the "target" route. In contrast macro_rules! is not hygienic for item paths (only locals), so paths are resolved in the invocation scope of the uri macro.

As a consequence, code such as this now fails to compile because PathBuf is not in scope at the uri! call:

#![feature(proc_macro_hygiene)]
#[macro_use] extern crate rocket;

#[get("/")]
fn hello() -> String {
    format!("Try going to {}", uri!(submodule::echo_path: "example/path"))
}

mod submodule {
    use std::path::PathBuf;

    #[get("/<path..>")]
    pub fn echo_path(path: PathBuf) -> String {
        path.display().to_string()
    }
}

fn main() {
    rocket::ignite().mount("/", routes![hello, submodule::echo_path]).launch();
}

The best possible solution for this issue is to use macro once it stabilizes, but that is pretty far off. If it works, we could try fudging some Spans in the generated macro_rules! macro. Another solution would be to use structs instead of a macro to encode the route information at compile time, but that is a much more significant rewrite of uri! that probably can't be done with the same feature set that is implemented now.

[SergioBenitez]

Note: This only applies to unreleased versions of Rocket, notably, the master and async branches.

[jebrosen]

This seems like it should be doable with type aliases:

use std::path::PathBuf;
#[get("/hello/<s1>/<s2>/<rest..>")]
fn hello(s1: String, s2: &str, rest: PathBuf) { }

// generated:
type rocket__type_for_uri_param_hello_0 = String;
type rocket__type_for_uri_param_hello_1<'a> = &'a str;
type rocket__type_for_uri_param_hello_2 = PathBuf;

Generated code would refer to the rocket__type_for_uri_param_hello_ names instead of naming the type directly. The biggest issues I can see with this are lifetime parameters, for two reasons. Frist, some type names like Segments have a hidden lifetime parameter, and the user would have to use Segments<'_> in their route definition in order for this approach to work since we have no way to "guess" how many lifetime parameters a type has. Second, a type with two lifetime parameters that are equal to each other (i.e. &'a DataType<'a>) could be a huge problem, but I am having a hard time imagining that being useful or usable as a route parameter anyway.

That requirement to spell out anonymous lifetimes seems not too burdensome - it's already a hard requirement in async fn definitions, and it's also part of the rust_2018_idioms lint group.

[SergioBenitez]

I've opened rust-lang/rfcs#2968, which would allow us to resolve this issue on stable.

Edit: For posterity, besides the issue of identifying lifetimes, the type alias proposal and any like it do not handle ignorables, _ in uri!() invocations.

[SergioBenitez]

Unfortunately, I don't see this as resolvable without support from rustc or an entirely new approach to uri!. Given that the error message emitted points to a resolution (i.e, import the missing type), I'm going to move this to 0.6. Any solution here, however, is backwards compatible, so ideally we can fix this as soon as a fix is known or possible.

[SergioBenitez]

Alright, I think I have a solution.

First, we add the following impls to http:

Ignored, which can be converted into any Ignorable URI parameter type:

pub struct Ignored;

impl UriDisplay<Query> for Ignored {
    fn fmt(&self, _: &mut Formatter<'_, Query>) -> fmt::Result {
        Ok(())
    }
}

impl<T: Ignorable<Query>> FromUriParam<Query, Ignored> for T {
    type Target = Ignored;

    fn from_uri_param(_: Ignored) -> Self::Target { Ignored }
}

IntoUriParam, the dual of FromUriParam:

pub trait IntoUriParam<P: Part, T> {
    type Target: UriDisplay<P>;

    fn into_uri_param(self) -> Self::Target;
}

impl<P: Part, T, U: FromUriParam<P, T>> IntoUriParam<P, U> for T {
    type Target = U::Target;

    fn into_uri_param(self) -> Self::Target {
        U::from_uri_param(self)
    }
}

Then, we generate the following code for the example route foo:

#[get("/<a>/b/<_>?<c>&<d>&e&<f..>")]
fn foo(a: &str, c: usize, d: Option<Date>, f: MyForm) {}

struct foo {}

impl foo {
   fn uri<'a>(
        self,
        a: impl IntoUriParam<Path, &'a str>,
        _0: impl UriDisplay<Path>,
        c: impl IntoUriParam<Query, usize>,
        d: impl IntoUriParam<Query, Option<Date>>,
        f: impl IntoUriParam<Query, MyForm>,
    ) -> RouteUriBuilder {
       let _a = a.into_uri_param();
       let _c = c.into_uri_param();
       let _d = d.into_uri_param();
       let _f = f.into_uri_param();

       RouteUriBuilder::new(
           UriArgumentsKind::Dynamic(
               &[&_a, &"b", &_0],
           ),
           UriArgumentsKind::Dynamic(
               &[
                   UriQueryArgument::NameValue("c", &_c),
                   UriQueryArgument::NameValue("d", &_d),
                   UriQueryArgument::Raw("e"),
                   UriQueryArgument::Value(&_f),
               ]
           )
       )
    }
}

Finally, a call to uri!(foo(a, _0, c, d, f)) expands to:

(foo {}).uri(a, _0, c, d, f);

While a call to uri!(foo(a, _0, c, _, f)) expands to:

(foo {}).uri(a, _0, c, Ignored, f);

If we want to keep named arguments, we need to keep the two-layers of expansion we have now. However, if we're okay with dropping named arguments, we can remove the entire first layer of expansion. This would also likely sacrifice error messages, where today we show errors of the form:

error: invalid parameters for `has_one` route uri
  --> $DIR/typed-uris-bad-params.rs:37:18
   |
37 |     uri!(has_one(name = 100, age = 50, id = 100, id = 50));
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: uri parameters are: id: i32
help: unknown parameters: `name`, `age`
  --> $DIR/typed-uris-bad-params.rs:37:18
   |
37 |     uri!(has_one(name = 100, age = 50, id = 100, id = 50));
   |                  ^^^^        ^^^
help: duplicate parameter: `id`
  --> $DIR/typed-uris-bad-params.rs:37:50
   |
37 |     uri!(has_one(name = 100, age = 50, id = 100, id = 50));
   |                                                  ^^

And...

error: route expects 2 parameters but 1 was supplied
  --> $DIR/typed-uris-bad-params.rs:29:18
   |
29 |     uri!(has_two(10));
   |                  ^^
   |
   = note: route `has_two` has uri "/<id>?<name>"

...a single level of expansion wouldn't allow us the same diagnostics.

[sedrik]

I ran into a related issue in regards to `uri!`` today and leaving this as a note here after discussion in the rocket chat.

Doing let url = uri!(verify_email(user_id, code)); in a different file from where the end-point is defined (in have not investigated how it behaves if it is defined and used in the same file) gave me a error stating that I should try to import a generated macro rocket_uri_macro_verify_email.

This issue was resolved/side-stepped by importing the parent file of the end-point (in my case its users::verify_email) and calling uri with that.

Works: use users; let url = uri!(users::verify_email(user_id, code));

Does not work: let url = uri!(verify_email(user_id, code));

[sedrik]

Just tried and the calls works if it is done in the same file as the endpoint.

#[get("/health")]
pub fn health() -> &'static str {
    println!("{}", uri!(health()));
    "ok"
}

Works just fine.

[jebrosen]

@sedrik In Rocket 0.4 (and I think Rocket 0.3), this same issue affected routes!, and was documented: https://rocket.rs/v0.4/guide/overview/#namespacing . It doesn't look like it was documented as such, but uri! had the same limitation. This limitation was removed for routes (092e03f), but not for URIs!

As far as I can see, the expansion strategy proposed above in #1120 (comment) -- that is a uri() function on the generated struct -- would also solve this problem.

[sedrik]

I see, if there is no good path to lift the restriction for now maybe the documentation for the old route! could be applied to url! then? If you want I could look at creating a PR tonight.