From c48e17fe2699966a096a40853e5f21757b7283d2 Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller Date: Mon, 17 Jun 2019 10:45:24 +0200 Subject: [PATCH] macro: add `cli` property and remove specialization Drop #!feature(specialization) in favor of having a `cli` property for types to decide whether they are CLI compatible. The unconstrained_type! macro now has both ParseCli and ParseCliFromStr in view, and requires one of the two to be implemented for a type. This means that if a type implements FromStr, it should "just work". For types created without the help of the #[api] macro, there's a shortcut to exclude a type from the CLI via the no_cli_type!{typename} macro. Signed-off-by: Wolfgang Bumiller --- proxmox-api-macro/src/api_def.rs | 81 +++++++++++++++++++++++++++++- proxmox-api-macro/src/api_macro.rs | 27 +++++----- proxmox-api-macro/src/parsing.rs | 23 +++++++++ proxmox-api-macro/tests/basic.rs | 4 ++ proxmox-api/src/api_type.rs | 6 ++- proxmox-api/src/cli.rs | 40 ++++++++++++--- proxmox-api/src/lib.rs | 8 --- proxmox-api/tests/router.rs | 2 + 8 files changed, 160 insertions(+), 31 deletions(-) diff --git a/proxmox-api-macro/src/api_def.rs b/proxmox-api-macro/src/api_def.rs index 157813b1..78a258ed 100644 --- a/proxmox-api-macro/src/api_def.rs +++ b/proxmox-api-macro/src/api_def.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::convert::TryFrom; use proc_macro2::TokenStream; @@ -8,9 +9,85 @@ use quote::{quote, ToTokens}; use super::parsing::Expression; +#[derive(Clone)] +pub enum CliMode { + Disabled, + ParseCli, // By default we try proxmox::cli::ParseCli + FromStr, + Function(syn::Expr), +} + +impl Default for CliMode { + fn default() -> Self { + CliMode::ParseCli + } +} + +impl TryFrom for CliMode { + type Error = Error; + fn try_from(expr: Expression) -> Result { + if expr.is_ident("FromStr") { + return Ok(CliMode::FromStr); + } + + if let Ok(value) = expr.is_lit_bool() { + return Ok(if value.value { + CliMode::ParseCli + } else { + CliMode::Disabled + }); + } + + Ok(CliMode::Function(expr.expect_expr()?)) + } +} + +impl CliMode { + pub fn quote(&self, name: &proc_macro2::Ident) -> TokenStream { + match self { + CliMode::Disabled => quote! { None }, + CliMode::ParseCli => quote! { Some(<#name as ::proxmox::api::cli::ParseCli>::parse_cli) }, + CliMode::FromStr => quote! { + Some(<#name as ::proxmox::api::cli::ParseCliFromStr>::parse_cli) + }, + CliMode::Function(func) => quote! { Some(#func) }, + } + } +} + +#[derive(Builder)] +pub struct CommonTypeDefinition { + pub description: syn::LitStr, + #[builder(default)] + pub cli: CliMode, +} + +impl CommonTypeDefinition { + fn builder() -> CommonTypeDefinitionBuilder { + CommonTypeDefinitionBuilder::default() + } + + pub fn from_object(obj: &mut HashMap) -> Result { + let mut def = Self::builder(); + + if let Some(value) = obj.remove("description") { + def.description(value.expect_lit_str()?); + } + if let Some(value) = obj.remove("cli") { + def.cli(CliMode::try_from(value)?); + } + + match def.build() { + Ok(r) => Ok(r), + Err(err) => bail!("{}", err), + } + } +} + #[derive(Builder)] pub struct ParameterDefinition { - pub description: syn::LitStr, + #[builder(default)] + pub description: Option, #[builder(default)] pub validate: Option, #[builder(default)] @@ -30,7 +107,7 @@ impl ParameterDefinition { for (key, value) in obj { match key.as_str() { "description" => { - def.description(value.expect_lit_str()?); + def.description(Some(value.expect_lit_str()?)); } "validate" => { def.validate(Some(value.expect_expr()?)); diff --git a/proxmox-api-macro/src/api_macro.rs b/proxmox-api-macro/src/api_macro.rs index 5e70289f..9d0ae60b 100644 --- a/proxmox-api-macro/src/api_macro.rs +++ b/proxmox-api-macro/src/api_macro.rs @@ -6,7 +6,7 @@ use failure::{bail, format_err, Error}; use quote::{quote, ToTokens}; use syn::{Expr, Token}; -use super::api_def::ParameterDefinition; +use super::api_def::{CommonTypeDefinition, ParameterDefinition}; use super::parsing::*; pub fn api_macro(attr: TokenStream, item: TokenStream) -> Result { @@ -423,7 +423,7 @@ fn handle_struct( } fn handle_struct_unnamed( - definition: HashMap, + mut definition: HashMap, name: &Ident, item: &syn::FieldsUnnamed, ) -> Result { @@ -434,6 +434,7 @@ fn handle_struct_unnamed( //let field = fields.first().unwrap().value(); + let common = CommonTypeDefinition::from_object(&mut definition)?; let apidef = ParameterDefinition::from_object(definition)?; let validator = match apidef.validate { @@ -441,14 +442,18 @@ fn handle_struct_unnamed( None => quote! { ::proxmox::api::ApiType::verify(&self.0) }, }; + let description = common.description; + let parse_cli = common.cli.quote(&name); Ok(quote! { impl ::proxmox::api::ApiType for #name { fn type_info() -> &'static ::proxmox::api::TypeInfo { + use ::proxmox::api::cli::ParseCli; + use ::proxmox::api::cli::ParseCliFromStr; const INFO: ::proxmox::api::TypeInfo = ::proxmox::api::TypeInfo { name: stringify!(#name), - description: "FIXME", + description: #description, complete_fn: None, // FIXME! - parse_cli: Some(<#name as ::proxmox::api::cli::ParseCli>::parse_cli), + parse_cli: #parse_cli, }; &INFO } @@ -461,32 +466,28 @@ fn handle_struct_unnamed( } fn handle_struct_named( - definition: HashMap, + mut definition: HashMap, name: &Ident, item: &syn::FieldsNamed, ) -> Result { let mut verify_entries = None; - let mut description = None; + let common = CommonTypeDefinition::from_object(&mut definition)?; for (key, value) in definition { match key.as_str() { "fields" => { verify_entries = Some(handle_named_struct_fields(item, value.expect_object()?)?); } - "description" => { - description = Some(value.expect_lit_str()?); - } other => bail!("unknown api definition field: {}", other), } } - let description = description - .ok_or_else(|| format_err!("missing 'description' for type {}", name.to_string()))?; - use std::iter::FromIterator; let verifiers = TokenStream::from_iter( verify_entries.ok_or_else(|| format_err!("missing 'fields' definition for struct"))?, ); + let description = common.description; + let parse_cli = common.cli.quote(&name); Ok(quote! { impl ::proxmox::api::ApiType for #name { fn type_info() -> &'static ::proxmox::api::TypeInfo { @@ -494,7 +495,7 @@ fn handle_struct_named( name: stringify!(#name), description: #description, complete_fn: None, // FIXME! - parse_cli: Some(<#name as ::proxmox::api::cli::ParseCli>::parse_cli), + parse_cli: #parse_cli, }; &INFO } diff --git a/proxmox-api-macro/src/parsing.rs b/proxmox-api-macro/src/parsing.rs index 8accc824..9d021493 100644 --- a/proxmox-api-macro/src/parsing.rs +++ b/proxmox-api-macro/src/parsing.rs @@ -196,6 +196,19 @@ impl Expression { } } + pub fn is_lit_bool(&self) -> Result { + match self { + Expression::Expr(expr) => match expr { + Expr::Lit(lit) => match &lit.lit { + Lit::Bool(lit) => Ok(lit.clone()), + other => bail!("expected boolean literal, got: {:?}", other), + }, + other => bail!("expected boolean literal, got: {:?}", other), + }, + _ => bail!("expected boolean literal"), + } + } + pub fn expect_lit_bool(self) -> Result { match self { Expression::Expr(expr) => match expr { @@ -232,6 +245,16 @@ impl Expression { _ => bail!("expected a type name, got {:?}", self), } } + + pub fn is_ident(&self, ident: &str) -> bool { + match self { + Expression::Expr(expr) => match expr { + Expr::Path(path) => path.path.is_ident(Ident::new(ident, Span::call_site())), + _ => false, + }, + _ => false, + } + } } pub fn parse_object2(tokens: TokenStream) -> Result, Error> { diff --git a/proxmox-api-macro/tests/basic.rs b/proxmox-api-macro/tests/basic.rs index 0d578af4..29fa3f4f 100644 --- a/proxmox-api-macro/tests/basic.rs +++ b/proxmox-api-macro/tests/basic.rs @@ -15,6 +15,9 @@ use proxmox::api::{api, Router}; #[repr(transparent)] pub struct HostOrIp(String); +// We don't bother with the CLI interface in this test: +proxmox::api::no_cli_type! {HostOrIp} + // Simplified for example purposes fn validate_hostname(name: &str) -> Result<(), Error> { if name == "" { @@ -35,6 +38,7 @@ fn validate_hostname(name: &str) -> Result<(), Error> { maximum: 10000, }, }, + cli: false, })] #[derive(Deserialize, Serialize)] pub struct Person { diff --git a/proxmox-api/src/api_type.rs b/proxmox-api/src/api_type.rs index f5c238e5..f06c31d5 100644 --- a/proxmox-api/src/api_type.rs +++ b/proxmox-api/src/api_type.rs @@ -226,6 +226,9 @@ impl ApiType for Result { /// This is not supposed to be used, but can be if needed. This will provide an empty `ApiType` /// declaration with no description and no verifier. /// +/// This requires that the type already implements the `ParseCli` trait (or has a `parse_cli` type +/// of the same signature in view from any other trait). +/// /// This rarely makes sense, but sometimes a `string` is just a `string`. #[macro_export] macro_rules! unconstrained_api_type { @@ -236,11 +239,12 @@ macro_rules! unconstrained_api_type { } fn type_info() -> &'static $crate::TypeInfo { + use $crate::cli::ParseCli; const INFO: $crate::TypeInfo = $crate::TypeInfo { name: stringify!($type), description: stringify!($type), complete_fn: None, - parse_cli: Some(<$type as $crate::cli::ParseCli>::parse_cli), + parse_cli: Some(<$type>::parse_cli), }; &INFO } diff --git a/proxmox-api/src/cli.rs b/proxmox-api/src/cli.rs index 4ac4b6af..2bdaaed2 100644 --- a/proxmox-api/src/cli.rs +++ b/proxmox-api/src/cli.rs @@ -289,16 +289,42 @@ pub trait ParseCli { fn parse_cli(name: &str, value: Option<&str>) -> Result; } -// Saves us another mass impl macro such as the one below: -impl ParseCli for T { - default fn parse_cli(name: &str, _value: Option<&str>) -> Result { - bail!( - "invalid type for command line interface found for parameter '{}'", - name - ); +/// This is a version of ParseCli with a default implementation falling to FromStr. +pub trait ParseCliFromStr +where + Self: FromStr + Serialize, + ::Err: Into, +{ + fn parse_cli(name: &str, value: Option<&str>) -> Result { + parse_cli_from_str::(name, value) } } +impl ParseCliFromStr for T +where + T: FromStr + Serialize, + ::Err: Into, +{} + +#[macro_export] +macro_rules! no_cli_type { + ($type:ty $(, $more:ty)*) => { + impl $crate::cli::ParseCli for $type { + fn parse_cli(name: &str, _value: Option<&str>) -> Result { + bail!( + "invalid type for command line interface found for parameter '{}'", + name + ); + } + } + + $crate::impl_parse_cli_from_str!{$($more),*} + }; + () => {}; +} + +no_cli_type! {Vec} + #[macro_export] macro_rules! impl_parse_cli_from_str { ($type:ty $(, $more:ty)*) => { diff --git a/proxmox-api/src/lib.rs b/proxmox-api/src/lib.rs index e3b9dc45..21bcbba0 100644 --- a/proxmox-api/src/lib.rs +++ b/proxmox-api/src/lib.rs @@ -6,14 +6,6 @@ //! Note that you'll rarely need the [`Router`] type itself, as you'll most likely be creating them //! with the `router` macro provided by the `proxmox-api-macro` crate. -// This saves us some repetition (which we could do via macros), but this makes the code shorter -// and easier to review. -// FIXME: While the RFC has been approved a while ago and the implementation is there, there isn't -// much activity on the issue tracker for this, so should we remove this? -// Currently this is only used in cli.rs for a `default fn` which could instead made explicit for -// our types -#![feature(specialization)] - use std::future::Future; use std::pin::Pin; diff --git a/proxmox-api/tests/router.rs b/proxmox-api/tests/router.rs index 4934c936..555d01c7 100644 --- a/proxmox-api/tests/router.rs +++ b/proxmox-api/tests/router.rs @@ -151,6 +151,8 @@ mod methods { #[derive(Deserialize, Serialize)] pub struct CubicMeters(f64); + // We don't bother with the CLI interface in this test: + proxmox_api::no_cli_type! {CubicMeters} proxmox_api::unconstrained_api_type! {CubicMeters} #[derive(Deserialize, Serialize)]