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 <w.bumiller@proxmox.com>

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();

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<T
         Some(input) => 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(),
         },

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<syn::LitStr>,
+    pub description: Maybe<syn::LitStr>,
 
     /// The specific schema type (Object, String, ...)
     pub item: SchemaItem,
@@ -105,10 +105,11 @@ impl TryFrom<JSONObject> for Schema {
     type Error = syn::Error;
 
     fn try_from(mut obj: JSONObject) -> Result<Self, syn::Error> {
-        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<bool, Error> {
-        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,

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, Error>) -> TokenStream {
-    match data {
+    let mut data = match data {
         Ok(output) => output,
         Err(err) => match err.downcast::<syn::Error>() {
             Ok(err) => {
@@ -39,12 +46,15 @@ fn handle_error(mut item: TokenStream, data: Result<TokenStream, Error>) -> 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<TokenStream, Error> {
 */
 #[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<Option<TokenStream>> = 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")
+    })
+}

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<T> {
+    Explicit(T),
+    Derived(T),
+    None,
+}
+
+impl<T> Maybe<T> {
+    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<T>) -> Self {
+        match t {
+            Some(t) => Maybe::Explicit(t),
+            None => Maybe::None,
+        }
+    }
+
+    pub fn ok_or_else<E, F>(self, other: F) -> Result<T, E>
+    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<T> Into<Option<T>> for Maybe<T> {
+    fn into(self) -> Option<T> {
+        match self {
+            Maybe::Explicit(t) | Maybe::Derived(t) => Some(t),
+            Maybe::None => None,
+        }
+    }
+}