Macro hardening

xee-xpath-macros/src/convert.rs

@@ -3,6 +3,7 @@
 
 use proc_macro2::TokenStream;
 use quote::quote;
+use syn::spanned::Spanned;
 
 use xee_schema_type::Xs;
 use xee_xpath_ast::ast;
@@ -28,7 +29,7 @@ fn convert_item(
     name: TokenStream,
     arg: TokenStream,
 ) -> syn::Result<TokenStream> {
-    let (iterator, borrow) = convert_item_type(&item.item_type, arg.clone())?;
+    let (iterator, borrow) = convert_item_type(&item.item_type, fn_arg, arg.clone())?;
 
     Ok(match &item.occurrence {
         ast::Occurrence::One => {
@@ -93,42 +94,65 @@ fn convert_item(
             //     let #name = #name_temp.as_slice();
             // )
         }
-        ast::Occurrence::NonEmpty => todo!("NonEmpty not yet supported"),
+        ast::Occurrence::NonEmpty => bail_spanned!(
+            fn_arg.span() =>
+            "one-or-more occurrence (`+`) is not yet supported in #[xpath_fn] signatures"
+        ),
     })
 }
 
-fn convert_item_type(item: &ast::ItemType, arg: TokenStream) -> syn::Result<(TokenStream, bool)> {
+fn convert_item_type(
+    item: &ast::ItemType,
+    fn_arg: &syn::FnArg,
+    arg: TokenStream,
+) -> syn::Result<(TokenStream, bool)> {
     match item {
         ast::ItemType::Item => Ok((quote!(#arg.iter().map(Ok)), false)),
         ast::ItemType::AtomicOrUnionType(xs) => {
-            let (token_stream, borrow) = convert_atomic_or_union_type(*xs, arg)?;
+            let (token_stream, borrow) = convert_atomic_or_union_type(*xs, fn_arg, arg)?;
             Ok((token_stream, borrow))
         }
-        ast::ItemType::KindTest(kind_test) => Ok((convert_kind_test(kind_test, arg)?, false)),
+        ast::ItemType::KindTest(kind_test) => {
+            Ok((convert_kind_test(kind_test, fn_arg, arg)?, false))
+        }
         // we don't do anything special for higher order functions at this point;
         // the implementation is supposed to manually unpack the items
         ast::ItemType::FunctionTest(_) => Ok((quote!(#arg.iter().map(Ok)), false)),
         ast::ItemType::ArrayTest(array_test) => match array_test {
             ast::ArrayTest::AnyArrayTest => Ok((quote!(#arg.array_iter()), false)),
-            _ => todo!("Unsupported item type: typed array test"),
+            _ => bail_spanned!(
+                fn_arg.span() =>
+                "typed array tests (e.g. `array(xs:integer)`) are not yet supported in #[xpath_fn] signatures — use `array(*)`"
+            ),
         },
         ast::ItemType::MapTest(map_test) => match map_test {
             ast::MapTest::AnyMapTest => Ok((quote!(#arg.map_iter()), false)),
-            _ => todo!("Unsupported item type: typed map test"),
+            _ => bail_spanned!(
+                fn_arg.span() =>
+                "typed map tests (e.g. `map(xs:string, xs:integer)`) are not yet supported in #[xpath_fn] signatures — use `map(*)`"
+            ),
         },
     }
 }
 
-fn convert_atomic_or_union_type(xs: Xs, arg: TokenStream) -> syn::Result<(TokenStream, bool)> {
+fn convert_atomic_or_union_type(
+    xs: Xs,
+    fn_arg: &syn::FnArg,
+    arg: TokenStream,
+) -> syn::Result<(TokenStream, bool)> {
     if xs == Xs::AnyAtomicType || xs == Xs::Numeric {
         return Ok((quote!(#arg.atomized(interpreter.xot())), false));
     }
 
-    // TODO: another unwrap that should really be "we cannot create a rust wrapper
-    // for this type" error
-    let rust_info = xs
-        .rust_info()
-        .expect("Rust wrapper for this type not found");
+    let Some(rust_info) = xs.rust_info() else {
+        bail_spanned!(
+            fn_arg.span() =>
+            format!(
+                "XPath atomic type `{xs:?}` has no Rust wrapper — cannot bridge it through #[xpath_fn]. \
+                 If this type should be supported, register it in xee_schema_type::Xs::rust_info"
+            )
+        );
+    };
     let type_name = rust_info.rust_name();
     let type_name = syn::parse_str::<syn::Type>(type_name)?;
     let convert = quote!(std::convert::TryInto::<#type_name>::try_into(atomic));
@@ -140,18 +164,27 @@ fn convert_atomic_or_union_type(xs: Xs, arg: TokenStream) -> syn::Result<(TokenS
     ))
 }
 
-fn convert_kind_test(kind_test: &ast::KindTest, arg: TokenStream) -> syn::Result<TokenStream> {
+fn convert_kind_test(
+    kind_test: &ast::KindTest,
+    fn_arg: &syn::FnArg,
+    arg: TokenStream,
+) -> syn::Result<TokenStream> {
     match kind_test {
         ast::KindTest::Any => Ok(quote!(#arg.nodes())),
         ast::KindTest::Element(element_test) => {
             if element_test.is_some() {
-                unreachable!("Unsupported element test")
+                bail_spanned!(
+                    fn_arg.span() =>
+                    "constrained element tests (e.g. `element(foo)`, `element(*, xs:string)`) \
+                     are not yet supported in #[xpath_fn] signatures — use `element()`"
+                );
             }
             Ok(quote!(#arg.elements(interpreter.xot())?))
         }
-        _ => {
-            todo!("Unsupported kind test")
-        }
+        _ => bail_spanned!(
+            fn_arg.span() =>
+            "this kind test is not yet supported in #[xpath_fn] signatures — only `node()`, `element()`, and `item()` work today"
+        ),
     }
 }
 
@@ -262,4 +295,56 @@ mod tests {
     fn test_convert_array() {
         assert_debug_snapshot!(convert("array(*)"));
     }
+
+    fn convert_err(s: &str) -> String {
+        let fn_arg: syn::FnArg = syn::parse_str("a: &str").unwrap();
+        let namespaces = Namespaces::default();
+        let sequence_type = parse_sequence_type(s, &namespaces).unwrap();
+        let name = quote!(a);
+        let arg = quote!(arguments[0]);
+
+        convert_sequence_type(&sequence_type, &fn_arg, name, arg)
+            .expect_err("expected a syn::Error from convert_sequence_type")
+            .to_string()
+    }
+
+    #[test]
+    fn test_convert_nonempty_occurrence_errors() {
+        assert_debug_snapshot!(convert_err("xs:integer+"));
+    }
+
+    #[test]
+    fn test_convert_named_element_test_errors() {
+        // element(foo) — name-constrained element tests aren't supported yet
+        assert_debug_snapshot!(convert_err("element(foo)"));
+    }
+
+    #[test]
+    fn test_convert_typed_wildcard_element_test_errors() {
+        // element(*, xs:string) — wildcard name + type constraint,
+        // which is still Element(Some(_)) in the AST. The error
+        // message must not mislead the user into thinking only
+        // *named* element tests are rejected.
+        assert_debug_snapshot!(convert_err("element(*, xs:string)"));
+    }
+
+    #[test]
+    fn test_convert_attribute_kind_test_errors() {
+        assert_debug_snapshot!(convert_err("attribute()"));
+    }
+
+    #[test]
+    fn test_convert_text_kind_test_errors() {
+        assert_debug_snapshot!(convert_err("text()"));
+    }
+
+    #[test]
+    fn test_convert_typed_array_errors() {
+        assert_debug_snapshot!(convert_err("array(xs:integer)"));
+    }
+
+    #[test]
+    fn test_convert_typed_map_errors() {
+        assert_debug_snapshot!(convert_err("map(xs:string, xs:integer)"));
+    }
 }

xee-xpath-macros/src/parse.rs

@@ -46,7 +46,12 @@ impl Parse for XPathFnOptions {
                 }
             }
         }
-        let signature_string = signature.expect("Signature not found");
+        let Some(signature_string) = signature else {
+            return Err(input.error(
+                "missing XPath signature string — #[xpath_fn(...)] requires a signature literal \
+                 like \"fn:foo($x as xs:integer) as xs:integer\" as one of its arguments",
+            ));
+        };
         let namespaces = Namespaces::default();
         let signature = Signature::parse(&signature_string, &namespaces)
             .map_err(|e| input.error(format!("{:?}", e)))?;
@@ -134,4 +139,11 @@ mod tests {
             r#""fn:foo() as xs:string",blah"#
         ));
     }
+
+    #[test]
+    fn test_parse_keyword_only_errors() {
+        // #[xpath_fn(context_first)] with no signature string should
+        // surface a clean error, not a panic.
+        assert_debug_snapshot!(syn::parse_str::<XPathFnOptions>(r#"context_first"#));
+    }
 }

xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_attribute_kind_test_errors.snap

@@ -0,0 +1,5 @@
+---
+source: xee-xpath-macros/src/convert.rs
+expression: "convert_err(\"attribute()\")"
+---
+"this kind test is not yet supported in #[xpath_fn] signatures — only `node()`, `element()`, and `item()` work today"

xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_named_element_test_errors.snap

@@ -0,0 +1,5 @@
+---
+source: xee-xpath-macros/src/convert.rs
+expression: "convert_err(\"element(foo)\")"
+---
+"constrained element tests (e.g. `element(foo)`, `element(*, xs:string)`) are not yet supported in #[xpath_fn] signatures — use `element()`"

xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_nonempty_occurrence_errors.snap

@@ -0,0 +1,5 @@
+---
+source: xee-xpath-macros/src/convert.rs
+expression: "convert_err(\"xs:integer+\")"
+---
+"one-or-more occurrence (`+`) is not yet supported in #[xpath_fn] signatures"

xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_text_kind_test_errors.snap

@@ -0,0 +1,5 @@
+---
+source: xee-xpath-macros/src/convert.rs
+expression: "convert_err(\"text()\")"
+---
+"this kind test is not yet supported in #[xpath_fn] signatures — only `node()`, `element()`, and `item()` work today"

xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_typed_array_errors.snap

@@ -0,0 +1,5 @@
+---
+source: xee-xpath-macros/src/convert.rs
+expression: "convert_err(\"array(xs:integer)\")"
+---
+"typed array tests (e.g. `array(xs:integer)`) are not yet supported in #[xpath_fn] signatures — use `array(*)`"

xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_typed_map_errors.snap

@@ -0,0 +1,5 @@
+---
+source: xee-xpath-macros/src/convert.rs
+expression: "convert_err(\"map(xs:string, xs:integer)\")"
+---
+"typed map tests (e.g. `map(xs:string, xs:integer)`) are not yet supported in #[xpath_fn] signatures — use `map(*)`"

xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_typed_wildcard_element_test_errors.snap

@@ -0,0 +1,5 @@
+---
+source: xee-xpath-macros/src/convert.rs
+expression: "convert_err(\"element(*, xs:string)\")"
+---
+"constrained element tests (e.g. `element(foo)`, `element(*, xs:string)`) are not yet supported in #[xpath_fn] signatures — use `element()`"

xee-xpath-macros/src/snapshots/xee_xpath_macros__parse__tests__parse_keyword_only_errors.snap

@@ -0,0 +1,9 @@
+---
+source: xee-xpath-macros/src/parse.rs
+expression: "syn::parse_str::<XPathFnOptions>(r#\"context_first\"#)"
+---
+Err(
+    Error(
+        "unexpected end of input, missing XPath signature string — #[xpath_fn(...)] requires a signature literal like \"fn:foo($x as xs:integer) as xs:integer\" as one of its arguments",
+    ),
+)

xee-xpath-macros/src/snapshots/xee_xpath_macros__wrapper__tests__wrapper_arity_mismatch_with_injected_context.snap

@@ -0,0 +1,5 @@
+---
+source: xee-xpath-macros/src/wrapper.rs
+expression: "xpath_fn_wrapper(&ast, &options).unwrap_err().to_string()"
+---
+"#[xpath_fn] arity mismatch: signature declares 0 XPath parameter(s), but the Rust function takes 1 (after subtracting 1 injected `context`/`interpreter` argument(s))"

xee-xpath-macros/src/snapshots/xee_xpath_macros__wrapper__tests__wrapper_too_few_rust_args_errors.snap

@@ -0,0 +1,5 @@
+---
+source: xee-xpath-macros/src/wrapper.rs
+expression: "xpath_fn_wrapper(&ast, &options).unwrap_err().to_string()"
+---
+"#[xpath_fn] arity mismatch: signature declares 2 XPath parameter(s), but the Rust function takes 1"

xee-xpath-macros/src/snapshots/xee_xpath_macros__wrapper__tests__wrapper_too_many_rust_args_errors.snap

@@ -0,0 +1,5 @@
+---
+source: xee-xpath-macros/src/wrapper.rs
+expression: "xpath_fn_wrapper(&ast, &options).unwrap_err().to_string()"
+---
+"#[xpath_fn] arity mismatch: signature declares 1 XPath parameter(s), but the Rust function takes 2"

xee-xpath-macros/src/wrapper.rs

@@ -75,6 +75,24 @@ fn make_wrapper(
         conversion_names.push(interpreter_ident);
         adjust += 1;
     }
+
+    let expected_inputs = signature.params.len() + adjust;
+    if ast.sig.inputs.len() != expected_inputs {
+        let sig_count = signature.params.len();
+        let rust_user_count = ast.sig.inputs.len().saturating_sub(adjust);
+        let injected_note = match adjust {
+            0 => String::new(),
+            n => format!(" (after subtracting {n} injected `context`/`interpreter` argument(s))"),
+        };
+        bail_spanned!(
+            ast.sig.ident.span() =>
+            format!(
+                "#[xpath_fn] arity mismatch: signature declares {sig_count} XPath parameter(s), \
+                 but the Rust function takes {rust_user_count}{injected_note}"
+            )
+        );
+    }
+
     for (i, param) in signature.params.iter().enumerate() {
         let name = Ident::new(param.name.local_name(), Span::call_site());
         conversion_names.push(name.clone());
@@ -187,4 +205,40 @@ mod tests {
         .unwrap();
         assert_debug_snapshot!(xpath_fn_wrapper(&ast, &options).unwrap().to_string());
     }
+
+    #[test]
+    fn test_wrapper_too_few_rust_args_errors() {
+        // signature declares two XPath parameters but the Rust fn only
+        // takes one — used to panic with index-out-of-bounds; now should
+        // surface as a clean syn::Error pointing at the fn name.
+        let options =
+            parse_str::<XPathFnOptions>(r#""fn:foo($x as xs:int, $y as xs:int) as xs:string""#)
+                .unwrap();
+        let ast = parse_str::<ItemFn>(r#"fn foo(x: &i64) -> String { format!("{}", x) }"#).unwrap();
+        assert_debug_snapshot!(xpath_fn_wrapper(&ast, &options).unwrap_err().to_string());
+    }
+
+    #[test]
+    fn test_wrapper_too_many_rust_args_errors() {
+        // Rust fn has one more arg than the signature declares.
+        // The old code silently dropped the extra arg; we now reject.
+        let options =
+            parse_str::<XPathFnOptions>(r#""fn:foo($x as xs:int) as xs:string""#).unwrap();
+        let ast = parse_str::<ItemFn>(r#"fn foo(x: &i64, y: &i64) -> String { format!("{}", x) }"#)
+            .unwrap();
+        assert_debug_snapshot!(xpath_fn_wrapper(&ast, &options).unwrap_err().to_string());
+    }
+
+    #[test]
+    fn test_wrapper_arity_mismatch_with_injected_context() {
+        // Even with context injection, arity is checked correctly:
+        // signature 0 params + context injected = 1 Rust arg expected,
+        // but Rust fn has 2.
+        let options = parse_str::<XPathFnOptions>(r#""fn:foo() as xs:string""#).unwrap();
+        let ast = parse_str::<ItemFn>(
+            r#"fn foo(context: &DynamicContext, extra: &i64) -> String { String::new() }"#,
+        )
+        .unwrap();
+        assert_debug_snapshot!(xpath_fn_wrapper(&ast, &options).unwrap_err().to_string());
+    }
 }