From 273ce602427b2c74f160d0d0749bcdeddd36e23c Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller Date: Mon, 7 Dec 2020 12:34:29 +0100 Subject: [PATCH] api-macro: forbid description on incompatible schemas References to external schemas (or types) already include the description in the external schema and therefore are illegal. The implementation consists of multiple parts: * Introduce a `Maybe` type which can be `Explicit`, `Derived` or `None`. * Forbid `Explicit` descriptions on references. * Instead of bailing out on such errors which causes all of the generated code to vanish and create heaps of additional nonsensical errors, add a way to *add* errors without bailing out immediately via the `error!()` macro. Signed-off-by: Wolfgang Bumiller --- proxmox-api-macro/src/api/enums.rs | 4 +-- proxmox-api-macro/src/api/method.rs | 4 +-- proxmox-api-macro/src/api/mod.rs | 50 +++++++++++++++----------- proxmox-api-macro/src/lib.rs | 56 +++++++++++++++++++++++++++-- proxmox-api-macro/src/util.rs | 53 +++++++++++++++++++++++++-- 5 files changed, 139 insertions(+), 28 deletions(-) diff --git a/proxmox-api-macro/src/api/enums.rs b/proxmox-api-macro/src/api/enums.rs index 9322f01c..df079d98 100644 --- a/proxmox-api-macro/src/api/enums.rs +++ b/proxmox-api-macro/src/api/enums.rs @@ -7,7 +7,7 @@ use quote::quote_spanned; use super::Schema; use crate::serde; -use crate::util::{self, FieldName, JSONObject, JSONValue}; +use crate::util::{self, FieldName, JSONObject, JSONValue, Maybe}; /// Enums, provided they're simple enums, simply get an enum string schema attached to them. pub fn handle_enum( @@ -30,7 +30,7 @@ pub fn handle_enum( if schema.description.is_none() { let (comment, span) = util::get_doc_comments(&enum_ty.attrs)?; - schema.description = Some(syn::LitStr::new(comment.trim(), span)); + schema.description = Maybe::Derived(syn::LitStr::new(comment.trim(), span)); } let mut ts = TokenStream::new(); diff --git a/proxmox-api-macro/src/api/method.rs b/proxmox-api-macro/src/api/method.rs index f3a108c6..5c0c170a 100644 --- a/proxmox-api-macro/src/api/method.rs +++ b/proxmox-api-macro/src/api/method.rs @@ -18,7 +18,7 @@ use syn::visit_mut::{self, VisitMut}; use syn::Ident; use super::{Schema, SchemaItem}; -use crate::util::{self, FieldName, JSONObject, JSONValue}; +use crate::util::{self, FieldName, JSONObject, JSONValue, Maybe}; /// Parse `input`, `returns` and `protected` attributes out of an function annotated /// with an `#[api]` attribute and produce a `const ApiMethod` named after the function. @@ -29,7 +29,7 @@ pub fn handle_method(mut attribs: JSONObject, mut func: syn::ItemFn) -> Result input.into_object("input schema definition")?.try_into()?, None => Schema { span: Span::call_site(), - description: None, + description: Maybe::None, item: SchemaItem::Object(Default::default()), properties: Vec::new(), }, diff --git a/proxmox-api-macro/src/api/mod.rs b/proxmox-api-macro/src/api/mod.rs index dc14ff8c..81d91776 100644 --- a/proxmox-api-macro/src/api/mod.rs +++ b/proxmox-api-macro/src/api/mod.rs @@ -17,7 +17,7 @@ use syn::parse::{Parse, ParseStream, Parser}; use syn::spanned::Spanned; use syn::{ExprPath, Ident}; -use crate::util::{FieldName, JSONObject, JSONValue}; +use crate::util::{FieldName, JSONObject, JSONValue, Maybe}; mod enums; mod method; @@ -70,7 +70,7 @@ pub struct Schema { span: Span, /// Common in all schema entry types: - pub description: Option, + pub description: Maybe, /// The specific schema type (Object, String, ...) pub item: SchemaItem, @@ -105,10 +105,11 @@ impl TryFrom for Schema { type Error = syn::Error; fn try_from(mut obj: JSONObject) -> Result { - let description = obj - .remove("description") - .map(|v| v.try_into()) - .transpose()?; + let description = Maybe::explicit( + obj.remove("description") + .map(|v| v.try_into()) + .transpose()?, + ); Ok(Self { span: obj.brace_token.span, @@ -126,7 +127,7 @@ impl Schema { fn blank(span: Span) -> Self { Self { span, - description: None, + description: Maybe::None, item: SchemaItem::Inferred(span), properties: Vec::new(), } @@ -135,7 +136,7 @@ impl Schema { fn empty_object(span: Span) -> Self { Self { span, - description: None, + description: Maybe::None, item: SchemaItem::Object(SchemaObject::new()), properties: Vec::new(), } @@ -279,35 +280,36 @@ impl SchemaItem { fn to_inner_schema( &self, ts: &mut TokenStream, - description: Option<&syn::LitStr>, + description: Maybe<&syn::LitStr>, span: Span, properties: &[(Ident, syn::Expr)], ) -> Result { - let description = description.ok_or_else(|| format_err!(span, "missing description")); + let check_description = + move || description.ok_or_else(|| format_err!(span, "missing description")); match self { SchemaItem::Null => { - let description = description?; + let description = check_description()?; ts.extend(quote! { ::proxmox::api::schema::NullSchema::new(#description) }); } SchemaItem::Boolean => { - let description = description?; + let description = check_description()?; ts.extend(quote! { ::proxmox::api::schema::BooleanSchema::new(#description) }); } SchemaItem::Integer => { - let description = description?; + let description = check_description()?; ts.extend(quote! { ::proxmox::api::schema::IntegerSchema::new(#description) }); } SchemaItem::Number => { - let description = description?; + let description = check_description()?; ts.extend(quote! { ::proxmox::api::schema::NumberSchema::new(#description) }); } SchemaItem::String => { - let description = description?; + let description = check_description()?; ts.extend(quote! { ::proxmox::api::schema::StringSchema::new(#description) }); } SchemaItem::Object(obj) => { - let description = description?; + let description = check_description()?; let mut elems = TokenStream::new(); obj.to_schema_inner(&mut elems)?; ts.extend( @@ -315,7 +317,7 @@ impl SchemaItem { ); } SchemaItem::Array(array) => { - let description = description?; + let description = check_description()?; let mut items = TokenStream::new(); array.to_schema(&mut items)?; ts.extend(quote! { @@ -324,15 +326,23 @@ impl SchemaItem { } SchemaItem::ExternType(path) => { if !properties.is_empty() { - bail!(&properties[0].0 => "additional properties not allowed on external type"); + error!(&properties[0].0 => "additional properties not allowed on external type"); } + if let Maybe::Explicit(description) = description { + error!(description => "description not allowed on external type"); + } + ts.extend(quote_spanned! { path.span() => #path::API_SCHEMA }); return Ok(true); } SchemaItem::ExternSchema(path) => { if !properties.is_empty() { - bail!(&properties[0].0 => "additional properties not allowed on schema ref"); + error!(&properties[0].0 => "additional properties not allowed on schema ref"); } + if let Maybe::Explicit(description) = description { + error!(description => "description not allowed on external type"); + } + ts.extend(quote_spanned! { path.span() => #path }); return Ok(true); } @@ -354,7 +364,7 @@ impl SchemaItem { fn to_schema( &self, ts: &mut TokenStream, - description: Option<&syn::LitStr>, + description: Maybe<&syn::LitStr>, span: Span, properties: &[(Ident, syn::Expr)], typed: bool, diff --git a/proxmox-api-macro/src/lib.rs b/proxmox-api-macro/src/lib.rs index eab8fcc6..2f543e76 100644 --- a/proxmox-api-macro/src/lib.rs +++ b/proxmox-api-macro/src/lib.rs @@ -3,6 +3,8 @@ extern crate proc_macro; extern crate proc_macro2; +use std::cell::RefCell; + use anyhow::Error; use proc_macro::TokenStream as TokenStream_1; @@ -16,6 +18,11 @@ macro_rules! format_err { ($span:expr, $($msg:tt)*) => { syn::Error::new($span, format!($($msg)*)) }; } +/// Produce a compile error which does not immediately abort. +macro_rules! error { + ($($msg:tt)*) => {{ crate::add_error(format_err!($($msg)*)); }} +} + /// Our `bail` macro replacement to enforce the inclusion of a `Span`. /// The arrow variant takes a spanned syntax element, the comma variant expects an actual `Span` as /// first parameter. @@ -30,7 +37,7 @@ mod util; /// Handle errors by appending a `compile_error!()` macro invocation to the original token stream. fn handle_error(mut item: TokenStream, data: Result) -> TokenStream { - match data { + let mut data = match data { Ok(output) => output, Err(err) => match err.downcast::() { Ok(err) => { @@ -39,12 +46,15 @@ fn handle_error(mut item: TokenStream, data: Result) -> Toke } Err(err) => panic!("error in api/router macro: {}", err), }, - } + }; + data.extend(take_non_fatal_errors()); + data } /// TODO! #[proc_macro] pub fn router(item: TokenStream_1) -> TokenStream_1 { + let _error_guard = init_local_error(); let item: TokenStream = item.into(); handle_error(item.clone(), router_do(item)).into() } @@ -221,6 +231,48 @@ fn router_do(item: TokenStream) -> Result { */ #[proc_macro_attribute] pub fn api(attr: TokenStream_1, item: TokenStream_1) -> TokenStream_1 { + let _error_guard = init_local_error(); let item: TokenStream = item.into(); handle_error(item.clone(), api::api(attr.into(), item)).into() } + +thread_local!(static NON_FATAL_ERRORS: RefCell> = RefCell::new(None)); + +/// The local error TLS must be freed at the end of a macro as any leftover `TokenStream` (even an +/// empty one) will just panic between different runs as the multiple source files are handled by +/// the same compiler thread. +struct LocalErrorGuard; + +impl Drop for LocalErrorGuard { + fn drop(&mut self) { + NON_FATAL_ERRORS.with(|errors| { + *errors.borrow_mut() = None; + }); + } +} + +fn init_local_error() -> LocalErrorGuard { + NON_FATAL_ERRORS.with(|errors| { + *errors.borrow_mut() = Some(TokenStream::new()); + }); + LocalErrorGuard +} + +pub(crate) fn add_error(err: syn::Error) { + NON_FATAL_ERRORS.with(|errors| { + errors + .borrow_mut() + .as_mut() + .expect("missing call to init_local_error") + .extend(err.to_compile_error()) + }); +} + +pub(crate) fn take_non_fatal_errors() -> TokenStream { + NON_FATAL_ERRORS.with(|errors| { + errors + .borrow_mut() + .take() + .expect("missing call to init_local_mut") + }) +} diff --git a/proxmox-api-macro/src/util.rs b/proxmox-api-macro/src/util.rs index d5488def..3cd29a02 100644 --- a/proxmox-api-macro/src/util.rs +++ b/proxmox-api-macro/src/util.rs @@ -442,14 +442,15 @@ pub fn derive_descriptions( if let Some(first) = parts.next() { if input_schema.description.is_none() { - input_schema.description = Some(syn::LitStr::new(first.trim(), doc_span)); + input_schema.description = Maybe::Derived(syn::LitStr::new(first.trim(), doc_span)); } } if let Some(second) = parts.next() { if let Some(ref mut returns_schema) = returns_schema { if returns_schema.description.is_none() { - returns_schema.description = Some(syn::LitStr::new(second.trim(), doc_span)); + returns_schema.description = + Maybe::Derived(syn::LitStr::new(second.trim(), doc_span)); } } @@ -574,3 +575,51 @@ where } text } + +/// Helper to distinguish between explicitly set or derived data. +#[derive(Clone, Copy, Eq, PartialEq)] +pub enum Maybe { + Explicit(T), + Derived(T), + None, +} + +impl Maybe { + pub fn as_ref(&self) -> Maybe<&T> { + match self { + Maybe::Explicit(t) => Maybe::Explicit(t), + Maybe::Derived(t) => Maybe::Derived(t), + Maybe::None => Maybe::None, + } + } + + pub fn explicit(t: Option) -> Self { + match t { + Some(t) => Maybe::Explicit(t), + None => Maybe::None, + } + } + + pub fn ok_or_else(self, other: F) -> Result + where + F: FnOnce() -> E, + { + match self { + Maybe::Explicit(t) | Maybe::Derived(t) => Ok(t), + Maybe::None => Err(other()), + } + } + + pub fn is_none(&self) -> bool { + matches!(self, Maybe::None) + } +} + +impl Into> for Maybe { + fn into(self) -> Option { + match self { + Maybe::Explicit(t) | Maybe::Derived(t) => Some(t), + Maybe::None => None, + } + } +}