From c8d47d0d21208e4c551bf6827724064edacafc9f Mon Sep 17 00:00:00 2001 From: Matt Coffin Date: Thu, 28 Mar 2024 19:37:15 -0600 Subject: [PATCH 1/2] parse_interface_item: fix passing over unrecognized elements previously, if things that utilized this separated match pattern would fail if encountering any unexpected/unknown elements. EX. when parsing the OpenXR spec, `require` can contain `interaction_profile` elements, which causes `parse_interface_item` to return None (as an error case), and the rest of the block is never processed. I introduced an alternative pattern to `unwrap_attribute!` in `parse_attributes!` that allows to acheive a simliar pattern, without the early return being *in the macro*, allowing the invoker to regain control of the control flow. This allowed moving all invocations of `parse_interface_item` to more standard `parse_*` function patterns used elsewhere, allowing it to function simliarly and properly handle unexpected elements. --- vk-parse/src/parse.rs | 114 +++++++++++++++++++----------------------- 1 file changed, 51 insertions(+), 63 deletions(-) diff --git a/vk-parse/src/parse.rs b/vk-parse/src/parse.rs index a8c4b5c..0a5ae8c 100644 --- a/vk-parse/src/parse.rs +++ b/vk-parse/src/parse.rs @@ -58,6 +58,32 @@ macro_rules! unwrap_attribute ( }; ); +macro_rules! parse_attributes { + ($ctx:expr, $($attribute:ident),+ => $val:expr) => { + { + let parse_xpath = &$ctx.xpath; + let parse_attribs = move || -> Result<_, Error> { + $( + let $attribute = $attribute.ok_or_else(|| { + Error::MissingAttribute { + xpath: parse_xpath.clone(), + name: stringify!($attribute).to_string(), + } + })?; + )+ + Ok($val) + }; + match parse_attribs() { + Ok(v) => Some(v), + Err(e) => { + $ctx.errors.push(e); + None + } + } + } + }; +} + macro_rules! match_attributes { ($ctx:expr, $a:ident in $attributes:expr, $($p:pat => $e:expr),+ $(,)?) => { for $a in $attributes { @@ -76,7 +102,7 @@ macro_rules! match_attributes { } macro_rules! match_elements { - ($ctx:expr, $($p:pat => $e:expr),+) => { + ($ctx:expr, $($p:pat => $e:expr),+ $(,)?) => { while let Some(Ok(e)) = $ctx.events.next() { match e { XmlEvent::StartElement { name, .. } => { @@ -87,11 +113,11 @@ macro_rules! match_elements { $p => $e, )+ _ => { + consume_current_element($ctx); $ctx.errors.push(Error::UnexpectedElement { xpath: $ctx.xpath.clone(), name: String::from(name), }); - consume_current_element($ctx); } } } @@ -115,11 +141,11 @@ macro_rules! match_elements { $p => $e, )+ _ => { + consume_current_element($ctx); $ctx.errors.push(Error::UnexpectedElement { xpath: $ctx.xpath.clone(), name: String::from(name), }); - consume_current_element($ctx); } } } @@ -317,9 +343,7 @@ fn parse_registry(ctx: &mut ParseCtx) -> Result if let Some(v) = parse_feature(ctx, attributes) { - registry.0.push(v); - }, + "feature" => registry.0.extend(parse_feature(ctx, attributes)), "extensions" => registry.0.push(parse_extensions(ctx, attributes)), "formats" => registry.0.push(parse_formats(ctx)), "spirvextensions" => registry.0.push(parse_spirvextensions(ctx, attributes)), @@ -1100,7 +1124,6 @@ fn parse_extension_item_require( let mut feature = None; let mut comment = None; let mut depends = None; - let mut items = Vec::new(); match_attributes! {ctx, a in attributes, "api" => api = Some(a.value), @@ -1111,24 +1134,7 @@ fn parse_extension_item_require( "depends" => depends = Some(a.value), } - while let Some(Ok(e)) = ctx.events.next() { - match e { - XmlEvent::StartElement { - name, attributes, .. - } => { - let name = name.local_name.as_str(); - ctx.push_element(name); - if let Some(v) = parse_interface_item(ctx, name, attributes) { - items.push(v); - } - } - XmlEvent::EndElement { .. } => { - ctx.pop_element(); - break; - } - _ => {} - } - } + let items = parse_interface_items(ctx); ExtensionChild::Require { api, @@ -1148,7 +1154,6 @@ fn parse_extension_item_remove( let mut api = None; let mut profile = None; let mut comment = None; - let mut items = Vec::new(); match_attributes! {ctx, a in attributes, "api" => api = Some(a.value), @@ -1156,24 +1161,7 @@ fn parse_extension_item_remove( "comment" => comment = Some(a.value) } - while let Some(Ok(e)) = ctx.events.next() { - match e { - XmlEvent::StartElement { - name, attributes, .. - } => { - let name = name.local_name.as_str(); - ctx.push_element(name); - if let Some(v) = parse_interface_item(ctx, name, attributes) { - items.push(v); - } - } - XmlEvent::EndElement { .. } => { - ctx.pop_element(); - break; - } - _ => {} - } - } + let items = parse_interface_items(ctx); ExtensionChild::Remove { api, @@ -1183,13 +1171,12 @@ fn parse_extension_item_remove( } } -fn parse_interface_item( +fn parse_interface_items( ctx: &mut ParseCtx, - name: &str, - attributes: Vec, -) -> Option { - match name { - "comment" => Some(InterfaceItem::Comment(parse_text_element(ctx))), +) -> Vec { + let mut items = Vec::new(); + match_elements! {ctx, attributes, + "comment" => items.push(InterfaceItem::Comment(parse_text_element(ctx))), "type" => { let mut name = None; let mut comment = None; @@ -1197,11 +1184,16 @@ fn parse_interface_item( "name" => name = Some(a.value), "comment" => comment = Some(a.value) } - unwrap_attribute!(ctx, type, name); + let item = parse_attributes!(ctx, name => InterfaceItem::Type { + name, comment, + }); consume_current_element(ctx); - Some(InterfaceItem::Type { name, comment }) - } - "enum" => parse_enum(ctx, attributes).map(|v| InterfaceItem::Enum(v)), + items.extend(item); + }, + "enum" => { + let ret = parse_enum(ctx, attributes).map(|v| InterfaceItem::Enum(v)); + items.extend(ret); + }, "command" => { let mut name = None; let mut comment = None; @@ -1209,18 +1201,14 @@ fn parse_interface_item( "name" => name = Some(a.value), "comment" => comment = Some(a.value) } - unwrap_attribute!(ctx, type, name); - consume_current_element(ctx); - Some(InterfaceItem::Command { name, comment }) - } - _ => { - ctx.errors.push(Error::UnexpectedElement { - xpath: ctx.xpath.clone(), - name: String::from(name), + let item = parse_attributes!(ctx, name => InterfaceItem::Command { + name, comment, }); - return None; + consume_current_element(ctx); + items.extend(item); } } + items } fn parse_formats(ctx: &mut ParseCtx) -> RegistryChild { From 36d718e804fc94b8f3347a2858711b2a77e6b2c4 Mon Sep 17 00:00:00 2001 From: Matt Coffin Date: Thu, 28 Mar 2024 22:11:27 -0600 Subject: [PATCH 2/2] tests: Add `openxr` feature to `ci` crate to test parsing of OpenXR API spec This is off by default since use of this crate to parse non-vulkan APIs isn't widespread enough to justify it's automation in CI jobs --- ci/Cargo.toml | 3 +++ ci/src/openxr.rs | 36 +++++++++++++++++++++++++ ci/tests/test.rs | 55 +++++++++++++++++++++++++++----------- vk-parse/examples/parse.rs | 34 +++++++++++++++++++++++ 4 files changed, 113 insertions(+), 15 deletions(-) create mode 100644 ci/src/openxr.rs create mode 100644 vk-parse/examples/parse.rs diff --git a/ci/Cargo.toml b/ci/Cargo.toml index 249a9ac..c27d62d 100644 --- a/ci/Cargo.toml +++ b/ci/Cargo.toml @@ -13,3 +13,6 @@ serde_derive = "^1.0.77" vk-parse = { path = "../vk-parse", features = ["serialize", "vkxml-convert"] } vkxml = "^0.3" xml-rs = "^0.8" + +[features] +openxr = [] diff --git a/ci/src/openxr.rs b/ci/src/openxr.rs new file mode 100644 index 0000000..8b7fda4 --- /dev/null +++ b/ci/src/openxr.rs @@ -0,0 +1,36 @@ +#![cfg(feature = "openxr")] +use crate::{ + TestApi, + parsing_test, +}; + +#[derive(Debug, Clone, Copy)] +pub struct OpenXR; + +impl TestApi for OpenXR { + const MAIN_URL: Option<&'static str> = None; + const ALLOW_WARNINGS: bool = true; + const USE_VKXML: bool = false; + fn download_url(major: u32, minor: u32, patch: u32, _url_suffix: &str) -> String { + format!( + "https://github.com/KhronosGroup/OpenXR-SDK-Source/raw/release-{}.{}.{}/specification/registry/xr.xml", + major, minor, patch + ) + } +} + +macro_rules! test_xr_versions { + ($($test_name:ident($major:expr, $minor:expr, $patch:expr)),+ $(,)?) => { + $( + #[test] + fn $test_name() { + parsing_test::($major, $minor, $patch, ""); + } + )+ + }; +} + +test_xr_versions! { + xr_test_v1_0_33(1, 0, 33), + xr_test_v1_0_34(1, 0, 34), +} diff --git a/ci/tests/test.rs b/ci/tests/test.rs index 5365543..fd62081 100644 --- a/ci/tests/test.rs +++ b/ci/tests/test.rs @@ -7,8 +7,35 @@ extern crate vk_parse; extern crate vkxml; extern crate xml; -const URL_REPO: &str = "https://raw.githubusercontent.com/KhronosGroup/Vulkan-Docs"; -const URL_MAIN: &str = "https://raw.githubusercontent.com/KhronosGroup/Vulkan-Docs/main/xml/vk.xml"; +#[cfg(feature = "openxr")] +#[path = "../src/openxr.rs"] +mod openxr; + +/// Trait representing a given API used for validation tests +pub(crate) trait TestApi { + const MAIN_URL: Option<&'static str>; + /// Fail tests on non-fatal parse warnings? + const ALLOW_WARNINGS: bool; + /// Also try to use [`vk_parse::parse_file_as_vkxml`] compatibility layer + const USE_VKXML: bool; + fn download_url(major: u32, minor: u32, patch: u32, url_suffix: &str) -> String; +} + +#[derive(Debug, Clone, Copy)] +struct Vulkan; +impl TestApi for Vulkan { + const MAIN_URL: Option<&'static str> = Some("https://raw.githubusercontent.com/KhronosGroup/Vulkan-Docs/main/xml/vk.xml"); + const ALLOW_WARNINGS: bool = false; + const USE_VKXML: bool = true; + fn download_url(major: u32, minor: u32, patch: u32, url_suffix: &str) -> String { + format!( + "{}/v{}.{}.{}{}/vk.xml", + "https://raw.githubusercontent.com/KhronosGroup/Vulkan-Docs", + major, minor, patch, + url_suffix + ) + } +} fn download(dst: &mut T, url: &str) { let resp = minreq::get(url) @@ -120,29 +147,27 @@ fn write_code(path: &str, reg: &vk_parse::Registry) { } } -fn parsing_test(major: u32, minor: u32, patch: u32, url_suffix: &str) { - let src = format!( - "{}/v{}.{}.{}{}/vk.xml", - URL_REPO, major, minor, patch, url_suffix - ); +pub(crate) fn parsing_test(major: u32, minor: u32, patch: u32, url_suffix: &str) { + let src = A::download_url(major, minor, patch, url_suffix); use std::io::Cursor; let mut buf = Cursor::new(vec![0; 15]); download(&mut buf, &src); buf.set_position(0); match vk_parse::parse_stream(buf.clone()) { - Ok((_reg, errors)) => { - // write_code(&format!("v{}.{}.{}.c", major, minor, patch), &_reg); - if !errors.is_empty() { + Ok((_reg, errors)) if !errors.is_empty() => { + if !A::ALLOW_WARNINGS { panic!("{:?}", errors); } } Err(fatal_error) => panic!("{:?}", fatal_error), + Ok(..) => {}, } - match vk_parse::parse_stream_as_vkxml(buf) { - Ok(_) => (), - Err(fatal_error) => panic!("{:?}", fatal_error), + if A::USE_VKXML { + if let Err(e) = vk_parse::parse_stream_as_vkxml(buf) { + panic!("{:?}", e); + } } } @@ -150,7 +175,7 @@ macro_rules! test_version { ($test_name:ident, $major:expr, $minor:expr, $patch:expr, $url_suffix:expr) => { #[test] fn $test_name() { - parsing_test($major, $minor, $patch, $url_suffix); + parsing_test::($major, $minor, $patch, $url_suffix); } }; } @@ -159,7 +184,7 @@ macro_rules! test_version { fn test_main() { use std::io::Cursor; let mut buf = Cursor::new(vec![0; 15]); - download(&mut buf, URL_MAIN); + download(&mut buf, Vulkan::MAIN_URL.unwrap()); buf.set_position(0); match vk_parse::parse_stream(buf.clone()) { diff --git a/vk-parse/examples/parse.rs b/vk-parse/examples/parse.rs new file mode 100644 index 0000000..b3503ca --- /dev/null +++ b/vk-parse/examples/parse.rs @@ -0,0 +1,34 @@ +use std::{ + error::Error, + path::PathBuf, + fmt, +}; + +fn main() -> Result<(), Box> { + let path = std::env::args() + .skip(1) + .map(PathBuf::from) + .next() + .ok_or(MissingArgumentError("XML_PATH"))?; + let registry = vk_parse::parse_file(path.as_ref()) + .map(|(ret, errors)| { + errors.into_iter().for_each(|e| { + eprintln!("non-fatal error while parsing registry: {:?}", &e); + }); + ret + }); + println!("{:#?}", ®istry); + Ok(()) +} + +#[derive(Debug, Clone, Copy)] +#[repr(transparent)] +struct MissingArgumentError<'a>(&'a str); + +impl<'a> fmt::Display for MissingArgumentError<'a> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "missing required argument: {}", self.0) + } +} + +impl<'a> Error for MissingArgumentError<'a> {}