From 3eb873d89dc748fce867216da913378fb212d2e5 Mon Sep 17 00:00:00 2001 From: Sergio Benitez Date: Thu, 4 Oct 2018 02:00:04 -0700 Subject: [PATCH] Tidy 'uri!' proc-macro. Improve error reporting. --- .../tests/ui/typed-uris-bad-params.stderr | 4 +- .../tests/ui/typed-uris-invalid-syntax.stderr | 30 +-- core/codegen_next/src/bang/mod.rs | 11 +- core/codegen_next/src/bang/uri.rs | 87 +++---- core/codegen_next/src/bang/uri_parsing.rs | 231 ++++++++++-------- core/http/src/uri/uri_display.rs | 4 +- 6 files changed, 195 insertions(+), 172 deletions(-) diff --git a/core/codegen/tests/ui/typed-uris-bad-params.stderr b/core/codegen/tests/ui/typed-uris-bad-params.stderr index cf820d93..ac5d9369 100644 --- a/core/codegen/tests/ui/typed-uris-bad-params.stderr +++ b/core/codegen/tests/ui/typed-uris-bad-params.stderr @@ -18,7 +18,7 @@ error: `simple` route uri expects 1 parameter but 2 were supplied --> $DIR/typed-uris-bad-params.rs:20:18 | 20 | uri!(simple: "Hello", 23, ); - | ^^^^^^^^^^^ + | ^^^^^^^^^^^^ | = note: expected parameter: id: i32 @@ -73,7 +73,7 @@ error: invalid parameters for `simple` route uri --> $DIR/typed-uris-bad-params.rs:26:18 | 26 | uri!(simple: id = 100, id = 100, ); - | ^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^ | = note: uri parameters are: id: i32 help: duplicate parameter: `id` diff --git a/core/codegen/tests/ui/typed-uris-invalid-syntax.stderr b/core/codegen/tests/ui/typed-uris-invalid-syntax.stderr index c354a450..d6746881 100644 --- a/core/codegen/tests/ui/typed-uris-invalid-syntax.stderr +++ b/core/codegen/tests/ui/typed-uris-invalid-syntax.stderr @@ -10,11 +10,11 @@ error: named and unnamed parameters cannot be mixed 12 | uri!(simple: "Hello", id = 100); | ^^^^^^^^^^^^^^^^^ -error: expected one of `::`, `:`, or ``, found `,` +error: expected `:` --> $DIR/typed-uris-invalid-syntax.rs:13:16 | 13 | uri!(simple,); - | ^ expected one of `::`, `:`, or `` here + | ^ error: expected argument list after `:` --> $DIR/typed-uris-invalid-syntax.rs:14:16 @@ -22,45 +22,41 @@ error: expected argument list after `:` 14 | uri!(simple:); | ^ -error: expected `,`, found `` +error: unexpected end of input: expected ',' followed by route path --> $DIR/typed-uris-invalid-syntax.rs:15:10 | 15 | uri!("/mount"); - | ^^^^^^^^ expected `,` + | ^^^^^^^^ -error: expected identifier, found `` - --> $DIR/typed-uris-invalid-syntax.rs:16:18 +error: unexpected end of input, expected identifier + --> $DIR/typed-uris-invalid-syntax.rs:16:5 | 16 | uri!("/mount",); - | ^ expected identifier + | ^^^^^^^^^^^^^^^^ -error: invalid mount point +error: invalid mount point; mount points must be static, absolute URIs: `/example` --> $DIR/typed-uris-invalid-syntax.rs:17:10 | 17 | uri!("mount", simple); | ^^^^^^^ - | - = help: mount points must be static, absolute URIs: `/example` -error: invalid mount point +error: invalid mount point; mount points must be static, absolute URIs: `/example` --> $DIR/typed-uris-invalid-syntax.rs:18:10 | 18 | uri!("/mount/", simple); | ^^^^^^^^^^^^^ - | - = help: mount points must be static, absolute URIs: `/example` -error: call to `uri!` cannot be empty +error: unexpected end of input, call to `uri!` cannot be empty --> $DIR/typed-uris-invalid-syntax.rs:19:5 | 19 | uri!(); | ^^^^^^^ -error: expected expression, found `` - --> $DIR/typed-uris-invalid-syntax.rs:20:21 +error: unexpected end of input, expected expression + --> $DIR/typed-uris-invalid-syntax.rs:20:5 | 20 | uri!(simple: id = ); - | ^ expected expression + | ^^^^^^^^^^^^^^^^^^^^ error: aborting due to 10 previous errors diff --git a/core/codegen_next/src/bang/mod.rs b/core/codegen_next/src/bang/mod.rs index 856f56de..2ce634b4 100644 --- a/core/codegen_next/src/bang/mod.rs +++ b/core/codegen_next/src/bang/mod.rs @@ -8,6 +8,12 @@ use syn_ext::{IdentExt, syn_to_diag}; mod uri; mod uri_parsing; + +crate fn prefix_last_segment(path: &mut Path, prefix: &str) { + let mut last_seg = path.segments.last_mut().expect("syn::Path has segments"); + last_seg.value_mut().ident = last_seg.value().ident.prepend(prefix); +} + fn _prefixed_vec(prefix: &str, input: TokenStream, ty: &TokenStream2) -> Result { // Parse a comma-separated list of paths. let mut paths = >::parse_terminated @@ -15,10 +21,7 @@ fn _prefixed_vec(prefix: &str, input: TokenStream, ty: &TokenStream2) -> Result< .map_err(syn_to_diag)?; // Prefix the last segment in each path with `prefix`. - for path in paths.iter_mut() { - let mut last_seg = path.segments.last_mut().expect("last path segment"); - last_seg.value_mut().ident = last_seg.value().ident.prepend(prefix); - } + paths.iter_mut().for_each(|p| prefix_last_segment(p, prefix)); // Return a `vec!` of the prefixed, mapped paths. let prefixed_mapped_paths = paths.iter() diff --git a/core/codegen_next/src/bang/uri.rs b/core/codegen_next/src/bang/uri.rs index 9b87f550..ad6bc083 100644 --- a/core/codegen_next/src/bang/uri.rs +++ b/core/codegen_next/src/bang/uri.rs @@ -3,60 +3,43 @@ use proc_macro2::TokenStream as TokenStream2; use std::fmt::Display; use derive_utils::{syn, Result}; -use quote::ToTokens; use syn_ext::{IdentExt, syn_to_diag}; use self::syn::{Expr, Ident, Type}; use self::syn::spanned::Spanned as SynSpanned; -use super::uri_parsing::*; +use bang::{prefix_last_segment, uri_parsing::*}; -use rocket_http::uri::Origin; -use rocket_http::ext::IntoOwned; +use rocket_http::{uri::Origin, ext::IntoOwned}; const URI_INFO_MACRO_PREFIX: &str = "rocket_uri_for_"; -crate fn _uri_macro(input: TokenStream) -> Result { - let args: TokenStream2 = input.clone().into(); - - let params = match syn::parse::(input) { - Ok(p) => p, - Err(e) => return Err(syn_to_diag(e)), - }; - let mut path = params.route_path; - { - let mut last_seg = path.segments.last_mut().expect("last path segment"); - last_seg.value_mut().ident = last_seg.value().ident.prepend(URI_INFO_MACRO_PREFIX); - } - - // It's incredibly important we use this span as the Span for the generated - // code so that errors from the `internal` call show up on the user's code. - Ok(quote_spanned!(args.span().into() => { - #path!(#args) - }).into()) -} - macro_rules! p { - ("parameter", $num:expr) => ( - if $num == 1 { "parameter" } else { "parameters" } + (@go $num:expr, $singular:expr, $plural:expr) => ( + if $num == 1 { $singular.into() } else { $plural } ); - ($num:expr, "was") => ( - if $num == 1 { "1 was".into() } else { format!("{} were", $num) } - ); - - ($num:expr, "parameter") => ( - if $num == 1 { "1 parameter".into() } else { format!("{} parameters", $num) } - ) + ("parameter", $n:expr) => (p!(@go $n, "parameter", "parameters")); + ($n:expr, "was") => (p!(@go $n, "1 was", format!("{} were", $n))); + ($n:expr, "parameter") => (p!(@go $n, "1 parameter", format!("{} parameters", $n))); } -fn extract_exprs(internal: &InternalUriParams) -> Result> { +crate fn _uri_macro(input: TokenStream) -> Result { + let input2: TokenStream2 = input.clone().into(); + let mut params = syn::parse::(input).map_err(syn_to_diag)?; + prefix_last_segment(&mut params.route_path, URI_INFO_MACRO_PREFIX); + + let path = ¶ms.route_path; + Ok(quote!(#path!(#input2)).into()) +} + +fn extract_exprs(internal: &InternalUriParams) -> Result> { let route_name = &internal.uri_params.route_path; match internal.validate() { Validation::Ok(exprs) => Ok(exprs), Validation::Unnamed(expected, actual) => { let mut diag = internal.uri_params.args_span().error( - format!("`{}` route uri expects {} but {} supplied", - route_name.clone().into_token_stream(), p!(expected, "parameter"), p!(actual, "was"))); + format!("`{}` route uri expects {} but {} supplied", quote!(#route_name), + p!(expected, "parameter"), p!(actual, "was"))); if expected > 0 { let ps = p!("parameter", expected); @@ -66,9 +49,9 @@ fn extract_exprs(internal: &InternalUriParams) -> Result> { Err(diag) } Validation::Named(missing, extra, dup) => { - let e = format!("invalid parameters for `{}` route uri", route_name.clone().into_token_stream()); - let mut diag = internal.uri_params.args_span().error(e); - diag = diag.note(format!("uri parameters are: {}", internal.fn_args_str())); + let e = format!("invalid parameters for `{}` route uri", quote!(#route_name)); + let mut diag = internal.uri_params.args_span().error(e) + .note(format!("uri parameters are: {}", internal.fn_args_str())); fn join>(iter: T) -> (&'static str, String) { let items: Vec<_> = iter.map(|i| format!("`{}`", i)).collect(); @@ -114,8 +97,8 @@ fn extract_origin(internal: &InternalUriParams) -> Result> { .map_err(|_| internal.uri.span().unstable().error("invalid route URI")) } -fn explode(route_str: &str, items: I) -> TokenStream2 - where I: Iterator +fn explode<'a, I>(route_str: &str, items: I) -> TokenStream2 + where I: Iterator { // Generate the statements to typecheck each parameter. // Building <$T as ::rocket::http::uri::FromUriParam<_>>::from_uri_param($e). @@ -123,16 +106,18 @@ fn explode(route_str: &str, items: I) -> TokenStream2 let mut fmt_exprs = vec![]; for (mut ident, ty, expr) in items { - let (span, mut expr) = (expr.span(), expr.clone()); - ident.set_span(span); - let ident_tmp = ident.prepend("tmp"); + let (span, expr) = (expr.span(), expr); + let ident_tmp = ident.prepend("tmp_"); let_bindings.push(quote_spanned!(span => - let #ident_tmp = #expr; let #ident = <#ty as ::rocket::http::uri::FromUriParam<_>>::from_uri_param(#ident_tmp); + let #ident_tmp = #expr; + let #ident = <#ty as ::rocket::http::uri::FromUriParam<_>>::from_uri_param(#ident_tmp); )); // generating: arg tokens for format string - fmt_exprs.push(quote_spanned!(span => { &#ident as &::rocket::http::uri::UriDisplay })); + fmt_exprs.push(quote_spanned! { span => + &#ident as &::rocket::http::uri::UriDisplay + }); } // Convert all of the '<...>' into '{}'. @@ -164,13 +149,15 @@ crate fn _uri_internal_macro(input: TokenStream) -> Result { let path_param_count = origin.path().matches('<').count(); // Create an iterator over the `ident`, `ty`, and `expr` triple. - let mut arguments = internal.fn_args - .into_iter() - .zip(exprs.into_iter()) - .map(|(FnArg { ident, ty }, expr)| (ident, ty, expr)); + let mut arguments = internal.fn_args.iter() + .zip(exprs.iter()) + .map(|(FnArg { ident, ty }, &expr)| (ident, ty, expr)); // Generate an expression for both the path and query. let path = explode(origin.path(), arguments.by_ref().take(path_param_count)); + + // FIXME: Use Optional. + // let query = Optional(origin.query().map(|q| explode(q, arguments))); let query = if let Some(expr) = origin.query().map(|q| explode(q, arguments)) { quote!({ Some(#expr) }) } else { diff --git a/core/codegen_next/src/bang/uri_parsing.rs b/core/codegen_next/src/bang/uri_parsing.rs index b795b52f..a034a571 100644 --- a/core/codegen_next/src/bang/uri_parsing.rs +++ b/core/codegen_next/src/bang/uri_parsing.rs @@ -1,26 +1,26 @@ use proc_macro::Span; -use derive_utils::syn; +use derive_utils::{syn, Spanned}; +use derive_utils::proc_macro2::TokenStream as TokenStream2; use derive_utils::ext::TypeExt; use quote::ToTokens; use self::syn::{Expr, Ident, LitStr, Path, Token, Type}; -use self::syn::spanned::Spanned as SynSpanned; use self::syn::parse::{self, Parse, ParseStream}; use self::syn::punctuated::Punctuated; use indexmap::IndexMap; #[derive(Debug)] -enum Arg { +pub enum Arg { Unnamed(Expr), - Named(Ident, Expr), + Named(Ident, Token![=], Expr), } #[derive(Debug)] pub enum Args { - Unnamed(Vec), - Named(Vec<(Ident, Expr)>), + Unnamed(Punctuated), + Named(Punctuated), } // For an invocation that looks like: @@ -28,12 +28,11 @@ pub enum Args { // ^-------------| ^----------| ^---------| // uri_params.mount_point | uri_params.arguments // uri_params.route_path -// #[derive(Debug)] pub struct UriParams { pub mount_point: Option, pub route_path: Path, - pub arguments: Option, + pub arguments: Args, } #[derive(Debug)] @@ -42,18 +41,29 @@ pub struct FnArg { pub ty: Type, } -pub enum Validation { +pub enum Validation<'a> { // Number expected, what we actually got. Unnamed(usize, usize), // (Missing, Extra, Duplicate) - Named(Vec, Vec, Vec), - // Everything is okay. - Ok(Vec) + Named(Vec<&'a Ident>, Vec<&'a Ident>, Vec<&'a Ident>), + // Everything is okay; here are the expressions in the route decl order. + Ok(Vec<&'a Expr>) } +// This is invoked by Rocket itself. The `uri!` macro expands to a call to a +// route-specific macro which in-turn expands to a call to `internal_uri!`, +// passing along the user's parameters from the original `uri!` call. This is +// necessary so that we can converge the type information in the route (from the +// route-specific macro) with the user's parameters (by forwarding them to the +// internal_uri! call). +// // `fn_args` are the URI arguments (excluding guards) from the original route's // handler in the order they were declared in the URI (`/`). -// `uri` is the full URI used in the origin route's attribute +// `uri` is the full URI used in the origin route's attribute. +// +// internal_uri!("//", (first: ty, second: ty), $($tt)*); +// ^-----------------| ^-----------|---------| ^-----| +// uri fn_args uri_params #[derive(Debug)] pub struct InternalUriParams { pub uri: String, @@ -61,63 +71,14 @@ pub struct InternalUriParams { pub uri_params: UriParams, } -impl Arg { - fn is_named(&self) -> bool { - match *self { - Arg::Named(..) => true, - Arg::Unnamed(_) => false, - } - } - - fn unnamed(self) -> Expr { - match self { - Arg::Unnamed(expr) => expr, - _ => panic!("Called Arg::unnamed() on an Arg::named!"), - } - } - - fn named(self) -> (Ident, Expr) { - match self { - Arg::Named(ident, expr) => (ident, expr), - _ => panic!("Called Arg::named() on an Arg::Unnamed!"), - } - } -} - -impl UriParams { - /// The Span to use when referring to all of the arguments. - pub fn args_span(&self) -> Span { - match self.arguments { - Some(ref args) => { - let (first, last) = match args { - Args::Unnamed(ref exprs) => { - ( - exprs.first().unwrap().span().unstable(), - exprs.last().unwrap().span().unstable() - ) - }, - Args::Named(ref pairs) => { - ( - pairs.first().unwrap().0.span().unstable(), - pairs.last().unwrap().1.span().unstable() - ) - }, - }; - first.join(last).expect("join spans") - }, - None => self.route_path.span().unstable(), - } - } -} - impl Parse for Arg { fn parse(input: ParseStream) -> parse::Result { let has_key = input.peek2(Token![=]); if has_key { let ident = input.parse::()?; - input.parse::()?; + let eq_token = input.parse::()?; let expr = input.parse::()?; - Ok(Arg::Named(ident, expr)) + Ok(Arg::Named(ident, eq_token, expr)) } else { let expr = input.parse::()?; Ok(Arg::Unnamed(expr)) @@ -125,6 +86,10 @@ impl Parse for Arg { } } +fn err>(span: Span, s: S) -> parse::Result { + Err(parse::Error::new(span.into(), s.as_ref())) +} + impl Parse for UriParams { // Parses the mount point, if any, route identifier, and arguments. fn parse(input: ParseStream) -> parse::Result { @@ -137,8 +102,16 @@ impl Parse for UriParams { let string = input.parse::()?; let value = string.value(); if value.contains('<') || !value.starts_with('/') { - return Err(parse::Error::new(string.span(), "invalid mount point; mount points must be static, absolute URIs: `/example`")); + // TODO(proc_macro): add example as a help, not in error + return err(string.span().unstable(), "invalid mount point; \ + mount points must be static, absolute URIs: `/example`"); } + + if !input.peek(Token![,]) && input.cursor().eof() { + return err(string.span().unstable(), "unexpected end of input: \ + expected ',' followed by route path"); + } + input.parse::()?; Some(string) } else { @@ -149,20 +122,18 @@ impl Parse for UriParams { let route_path = input.parse::()?; // If there are no arguments, finish early. - if !input.peek(Token![:]) { - let arguments = None; + if !input.peek(Token![:]) && input.cursor().eof() { + let arguments = Args::Unnamed(Punctuated::new()); return Ok(Self { mount_point, route_path, arguments }); } - let colon = input.parse::()?; - // Parse arguments - let args_start = input.cursor(); + let colon = input.parse::()?; let arguments: Punctuated = input.parse_terminated(Arg::parse)?; // A 'colon' was used but there are no arguments. if arguments.is_empty() { - return Err(parse::Error::new(colon.span(), "expected argument list after `:`")); + return err(colon.span(), "expected argument list after `:`"); } // Ensure that both types of arguments were not used at once. @@ -175,18 +146,15 @@ impl Parse for UriParams { } if !homogeneous_args { - // TODO: This error isn't showing up with the right span. - return Err(parse::Error::new(args_start.token_stream().span(), "named and unnamed parameters cannot be mixed")); + return err(arguments.span(), "named and unnamed parameters cannot be mixed"); } - // Create the `Args` enum, which properly types one-kind-of-argument-ness. - let args = if prev_named.unwrap() { - Args::Named(arguments.into_iter().map(|arg| arg.named()).collect()) - } else { - Args::Unnamed(arguments.into_iter().map(|arg| arg.unnamed()).collect()) + // Create the `Args` enum, which properly record one-kind-of-argument-ness. + let arguments = match prev_named { + Some(true) => Args::Named(arguments), + _ => Args::Unnamed(arguments) }; - let arguments = Some(args); Ok(Self { mount_point, route_path, arguments }) } } @@ -204,7 +172,6 @@ impl Parse for FnArg { impl Parse for InternalUriParams { fn parse(input: ParseStream) -> parse::Result { let uri = input.parse::()?.value(); - //let uri = parser.prev_span.wrap(uri_str); input.parse::()?; let content; @@ -221,32 +188,30 @@ impl Parse for InternalUriParams { impl InternalUriParams { pub fn fn_args_str(&self) -> String { self.fn_args.iter() - .map(|&FnArg { ref ident, ref ty }| format!("{}: {}", ident, ty.clone().into_token_stream().to_string().trim())) + .map(|FnArg { ident, ty }| format!("{}: {}", ident, quote!(#ty).to_string().trim())) .collect::>() .join(", ") } pub fn validate(&self) -> Validation { - let unnamed = |args: &Vec| -> Validation { - let (expected, actual) = (self.fn_args.len(), args.len()); - if expected != actual { Validation::Unnamed(expected, actual) } - else { Validation::Ok(args.clone()) } - }; - - match self.uri_params.arguments { - None => unnamed(&vec![]), - Some(Args::Unnamed(ref args)) => unnamed(args), - Some(Args::Named(ref args)) => { - let mut params: IndexMap> = self.fn_args.iter() - .map(|&FnArg { ref ident, .. }| (ident.clone(), None)) + let args = &self.uri_params.arguments; + match args { + Args::Unnamed(inner) => { + let (expected, actual) = (self.fn_args.len(), inner.len()); + if expected != actual { Validation::Unnamed(expected, actual) } + else { Validation::Ok(args.unnamed().unwrap().collect()) } + }, + Args::Named(_) => { + let mut params: IndexMap<&Ident, Option<&Expr>> = self.fn_args.iter() + .map(|FnArg { ident, .. }| (ident, None)) .collect(); let (mut extra, mut dup) = (vec![], vec![]); - for &(ref ident, ref expr) in args { + for (ident, expr) in args.named().unwrap() { match params.get_mut(ident) { - Some(ref entry) if entry.is_some() => dup.push(ident.clone()), - Some(entry) => *entry = Some(expr.clone()), - None => extra.push(ident.clone()), + Some(ref entry) if entry.is_some() => dup.push(ident), + Some(entry) => *entry = Some(expr), + None => extra.push(ident), } } @@ -268,3 +233,75 @@ impl InternalUriParams { } } +impl UriParams { + /// The Span to use when referring to all of the arguments. + pub fn args_span(&self) -> Span { + match self.arguments.num() { + 0 => self.route_path.span(), + _ => self.arguments.span() + } + } +} + +impl Arg { + fn is_named(&self) -> bool { + match *self { + Arg::Named(..) => true, + Arg::Unnamed(_) => false, + } + } + + fn unnamed(&self) -> &Expr { + match self { + Arg::Unnamed(expr) => expr, + _ => panic!("Called Arg::unnamed() on an Arg::named!"), + } + } + + fn named(&self) -> (&Ident, &Expr) { + match self { + Arg::Named(ident, _, expr) => (ident, expr), + _ => panic!("Called Arg::named() on an Arg::Unnamed!"), + } + } +} + +impl Args { + fn num(&self) -> usize { + match self { + Args::Named(inner) | Args::Unnamed(inner) => inner.len(), + } + } + + fn named(&self) -> Option> { + match self { + Args::Named(args) => Some(args.iter().map(|arg| arg.named())), + _ => None + } + } + + fn unnamed(&self) -> Option> { + match self { + Args::Unnamed(args) => Some(args.iter().map(|arg| arg.unnamed())), + _ => None + } + } +} + + +impl ToTokens for Arg { + fn to_tokens(&self, tokens: &mut TokenStream2) { + match self { + Arg::Unnamed(e) => e.to_tokens(tokens), + Arg::Named(ident, eq, expr) => tokens.extend(quote!(#ident #eq #expr)) + } + } +} + +impl ToTokens for Args { + fn to_tokens(&self, tokens: &mut TokenStream2) { + match self { + Args::Unnamed(e) | Args::Named(e) => e.to_tokens(tokens) + } + } +} diff --git a/core/http/src/uri/uri_display.rs b/core/http/src/uri/uri_display.rs index a80f15a9..1b2ad186 100644 --- a/core/http/src/uri/uri_display.rs +++ b/core/http/src/uri/uri_display.rs @@ -105,9 +105,9 @@ use self::priv_encode_set::PATH_ENCODE_SET; /// dynamic parameter type. /// /// ```rust -/// # #![feature(plugin, decl_macro)] +/// # #![feature(plugin, proc_macro_non_items, proc_macro_gen, decl_macro)] /// # #![plugin(rocket_codegen)] -/// # extern crate rocket; +/// # #[macro_use] extern crate rocket; /// # fn main() { } /// use rocket::http::RawStr; /// use rocket::request::FromParam;