diff --git a/xee-xpath-macros/src/convert.rs b/xee-xpath-macros/src/convert.rs index 69e221519..509bf5809 100644 --- a/xee-xpath-macros/src/convert.rs +++ b/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 { - 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::(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 { +fn convert_kind_test( + kind_test: &ast::KindTest, + fn_arg: &syn::FnArg, + arg: TokenStream, +) -> syn::Result { 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)")); + } } diff --git a/xee-xpath-macros/src/parse.rs b/xee-xpath-macros/src/parse.rs index 294386d58..f163c3b2a 100644 --- a/xee-xpath-macros/src/parse.rs +++ b/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::(r#"context_first"#)); + } } diff --git a/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_attribute_kind_test_errors.snap b/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_attribute_kind_test_errors.snap new file mode 100644 index 000000000..b646a2911 --- /dev/null +++ b/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" diff --git a/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_named_element_test_errors.snap b/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_named_element_test_errors.snap new file mode 100644 index 000000000..32968c46a --- /dev/null +++ b/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()`" diff --git a/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_nonempty_occurrence_errors.snap b/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_nonempty_occurrence_errors.snap new file mode 100644 index 000000000..9ec06db9d --- /dev/null +++ b/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" diff --git a/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_text_kind_test_errors.snap b/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_text_kind_test_errors.snap new file mode 100644 index 000000000..56631bd66 --- /dev/null +++ b/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" diff --git a/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_typed_array_errors.snap b/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_typed_array_errors.snap new file mode 100644 index 000000000..27db6560d --- /dev/null +++ b/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(*)`" diff --git a/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_typed_map_errors.snap b/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_typed_map_errors.snap new file mode 100644 index 000000000..42da1b35f --- /dev/null +++ b/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(*)`" diff --git a/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_typed_wildcard_element_test_errors.snap b/xee-xpath-macros/src/snapshots/xee_xpath_macros__convert__tests__convert_typed_wildcard_element_test_errors.snap new file mode 100644 index 000000000..eb90a503b --- /dev/null +++ b/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()`" diff --git a/xee-xpath-macros/src/snapshots/xee_xpath_macros__parse__tests__parse_keyword_only_errors.snap b/xee-xpath-macros/src/snapshots/xee_xpath_macros__parse__tests__parse_keyword_only_errors.snap new file mode 100644 index 000000000..9640523d2 --- /dev/null +++ b/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::(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", + ), +) diff --git a/xee-xpath-macros/src/snapshots/xee_xpath_macros__wrapper__tests__wrapper_arity_mismatch_with_injected_context.snap b/xee-xpath-macros/src/snapshots/xee_xpath_macros__wrapper__tests__wrapper_arity_mismatch_with_injected_context.snap new file mode 100644 index 000000000..687834bd7 --- /dev/null +++ b/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))" diff --git a/xee-xpath-macros/src/snapshots/xee_xpath_macros__wrapper__tests__wrapper_too_few_rust_args_errors.snap b/xee-xpath-macros/src/snapshots/xee_xpath_macros__wrapper__tests__wrapper_too_few_rust_args_errors.snap new file mode 100644 index 000000000..4937ff663 --- /dev/null +++ b/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" diff --git a/xee-xpath-macros/src/snapshots/xee_xpath_macros__wrapper__tests__wrapper_too_many_rust_args_errors.snap b/xee-xpath-macros/src/snapshots/xee_xpath_macros__wrapper__tests__wrapper_too_many_rust_args_errors.snap new file mode 100644 index 000000000..2136b3e62 --- /dev/null +++ b/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" diff --git a/xee-xpath-macros/src/wrapper.rs b/xee-xpath-macros/src/wrapper.rs index f8a9eee08..038e7dfa4 100644 --- a/xee-xpath-macros/src/wrapper.rs +++ b/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::(r#""fn:foo($x as xs:int, $y as xs:int) as xs:string""#) + .unwrap(); + let ast = parse_str::(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::(r#""fn:foo($x as xs:int) as xs:string""#).unwrap(); + let ast = parse_str::(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::(r#""fn:foo() as xs:string""#).unwrap(); + let ast = parse_str::( + r#"fn foo(context: &DynamicContext, extra: &i64) -> String { String::new() }"#, + ) + .unwrap(); + assert_debug_snapshot!(xpath_fn_wrapper(&ast, &options).unwrap_err().to_string()); + } }