From d06e4de7b035e708e9aac03008cbcc4dd1f51f84 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Fri, 6 Oct 2023 13:08:15 -0400 Subject: [PATCH 1/9] Starting a new macro for comparing fields. This will help us assert that all fields are tested and that only expected fields are present. --- src/compare/diff.rs | 5 ++++ src/compare/elisp_fact.rs | 6 ++++ src/compare/macros.rs | 59 +++++++++++++++++++++++++++++++++++++++ src/compare/mod.rs | 1 + 4 files changed, 71 insertions(+) create mode 100644 src/compare/macros.rs diff --git a/src/compare/diff.rs b/src/compare/diff.rs index d4af397..23d2575 100644 --- a/src/compare/diff.rs +++ b/src/compare/diff.rs @@ -13,6 +13,8 @@ use super::util::get_property_boolean; use super::util::get_property_numeric; use super::util::get_property_quoted_string; use super::util::get_property_unquoted_atom; +use crate::compare::elisp_fact::EmacsField; +use crate::compare::macros::compare_properties; use crate::types::AngleLink; use crate::types::AstNode; use crate::types::BabelCall; @@ -2688,6 +2690,9 @@ fn compare_code<'b, 's>( )); } + let foo: Result)>, Box> = + compare_properties!(emacs, EmacsField::Required(":value")); + Ok(DiffResult { status: this_status, name: rust.get_elisp_name(), diff --git a/src/compare/elisp_fact.rs b/src/compare/elisp_fact.rs index fd2810b..8f97ed3 100644 --- a/src/compare/elisp_fact.rs +++ b/src/compare/elisp_fact.rs @@ -550,3 +550,9 @@ impl<'s> ElispFact<'s> for PlainText<'s> { "plain-text".into() } } + +#[derive(Debug)] +pub(crate) enum EmacsField<'s> { + Required(&'s str), + Optional(&'s str), +} diff --git a/src/compare/macros.rs b/src/compare/macros.rs new file mode 100644 index 0000000..b71aa03 --- /dev/null +++ b/src/compare/macros.rs @@ -0,0 +1,59 @@ +/// Create iterators for ast nodes where it only has to iterate over children +macro_rules! compare_properties { + ($emacs:expr, $($emacs_field:expr),+) => { + { + let mut this_status = DiffStatus::Good; + let mut message: Option = None; + let children = $emacs.as_list()?; + let attributes_child = children + .iter() + .nth(1) + .ok_or("Should have an attributes child.")?; + let attributes_map = attributes_child.as_map()?; + let mut emacs_keys: BTreeSet<&str> = attributes_map.keys().map(|s| *s).collect(); + $( + match $emacs_field { + EmacsField::Required(name) if emacs_keys.contains(name) => { + emacs_keys.remove(name); + }, + EmacsField::Optional(name) if emacs_keys.contains(name) => { + emacs_keys.remove(name); + }, + EmacsField::Required(name) => { + this_status = DiffStatus::Bad; + message = Some(format!( + "Emacs token lacks required field: {}", + name + )); + }, + EmacsField::Optional(_name) => {}, + } + ),+ + + if !emacs_keys.is_empty() { + let unexpected_keys: Vec<&str> = emacs_keys.into_iter().collect(); + let unexpected_keys = unexpected_keys.join(", "); + this_status = DiffStatus::Bad; + message = Some(format!( + "Emacs token had extra field(s): {}", + unexpected_keys + )); + } + + $( + println!("{:?}", $emacs_field); + ),+ + + match this_status { + DiffStatus::Good => { + Ok(None) + }, + _ => { + Ok(Some((this_status, message))) + } + } + } + }; +} + +pub(crate) use compare_properties; diff --git a/src/compare/mod.rs b/src/compare/mod.rs index 1a75e2e..e7bcde8 100644 --- a/src/compare/mod.rs +++ b/src/compare/mod.rs @@ -1,6 +1,7 @@ mod compare; mod diff; mod elisp_fact; +mod macros; mod parse; mod sexp; mod util; From 1b603f3a0546095eba57bf4e2e3d3519a969c2ee Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Fri, 6 Oct 2023 13:29:46 -0400 Subject: [PATCH 2/9] Implement the comparison. --- src/compare/diff.rs | 29 +++++++++++++++++++++++++++-- src/compare/macros.rs | 23 ++++++++++++++++++++--- 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/compare/diff.rs b/src/compare/diff.rs index 23d2575..739aa9c 100644 --- a/src/compare/diff.rs +++ b/src/compare/diff.rs @@ -425,6 +425,24 @@ fn compare_ast_node<'b, 's>( Ok(compare_result.into()) } +fn compare_property_quoted_string_required_value<'b, 's, 'x>( + emacs: &'b Token<'s>, + emacs_field: &'x str, + rust_value: &'s str, +) -> Result)>, Box> { + let value = get_property_quoted_string(emacs, emacs_field)?; + if value.as_ref().map(String::as_str) != Some(rust_value) { + let this_status = DiffStatus::Bad; + let message = Some(format!( + "{} mismatch (emacs != rust) {:?} != {:?}", + emacs_field, value, rust_value + )); + Ok(Some((this_status, message))) + } else { + Ok(None) + } +} + pub fn compare_document<'b, 's>( emacs: &'b Token<'s>, rust: &'b Document<'s>, @@ -2690,8 +2708,15 @@ fn compare_code<'b, 's>( )); } - let foo: Result)>, Box> = - compare_properties!(emacs, EmacsField::Required(":value")); + if let Some((new_status, new_message)) = compare_properties!( + emacs, + EmacsField::Required(":value"), + rust.contents, + compare_property_quoted_string_required_value + )? { + this_status = new_status; + message = new_message; + } Ok(DiffResult { status: this_status, diff --git a/src/compare/macros.rs b/src/compare/macros.rs index b71aa03..463dd9b 100644 --- a/src/compare/macros.rs +++ b/src/compare/macros.rs @@ -1,6 +1,6 @@ /// Create iterators for ast nodes where it only has to iterate over children macro_rules! compare_properties { - ($emacs:expr, $($emacs_field:expr),+) => { + ($emacs:expr, $($emacs_field:expr, $rust_value:expr, $compare_fn: ident),+) => { { let mut this_status = DiffStatus::Good; let mut message: Option = None; @@ -41,12 +41,29 @@ macro_rules! compare_properties { } $( - println!("{:?}", $emacs_field); + let emacs_name = match $emacs_field { + EmacsField::Required(name) => { + name + }, + EmacsField::Optional(name) => { + name + }, + }; + let result = $compare_fn($emacs, emacs_name, $rust_value)?; + match result { + Some((DiffStatus::Good, _)) => unreachable!("No comparison functions should return Some() when DiffStatus is good."), + Some((status, msg)) => { + this_status = status; + message = msg; + }, + _ => {} + } ),+ match this_status { DiffStatus::Good => { - Ok(None) + let result: Result<_, Box> = Ok(None); + result }, _ => { Ok(Some((this_status, message))) From 7af18e2312fcb3f4f9a6fa225e548c67300bb783 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Fri, 6 Oct 2023 13:32:39 -0400 Subject: [PATCH 3/9] Move the EmacsField enum since it has nothing to do with ElispFact. --- src/compare/diff.rs | 2 +- src/compare/elisp_fact.rs | 6 ------ src/compare/macros.rs | 6 ++++++ 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/compare/diff.rs b/src/compare/diff.rs index 739aa9c..788a2d2 100644 --- a/src/compare/diff.rs +++ b/src/compare/diff.rs @@ -13,8 +13,8 @@ use super::util::get_property_boolean; use super::util::get_property_numeric; use super::util::get_property_quoted_string; use super::util::get_property_unquoted_atom; -use crate::compare::elisp_fact::EmacsField; use crate::compare::macros::compare_properties; +use crate::compare::macros::EmacsField; use crate::types::AngleLink; use crate::types::AstNode; use crate::types::BabelCall; diff --git a/src/compare/elisp_fact.rs b/src/compare/elisp_fact.rs index 8f97ed3..fd2810b 100644 --- a/src/compare/elisp_fact.rs +++ b/src/compare/elisp_fact.rs @@ -550,9 +550,3 @@ impl<'s> ElispFact<'s> for PlainText<'s> { "plain-text".into() } } - -#[derive(Debug)] -pub(crate) enum EmacsField<'s> { - Required(&'s str), - Optional(&'s str), -} diff --git a/src/compare/macros.rs b/src/compare/macros.rs index 463dd9b..4070e77 100644 --- a/src/compare/macros.rs +++ b/src/compare/macros.rs @@ -1,3 +1,9 @@ +#[derive(Debug)] +pub(crate) enum EmacsField<'s> { + Required(&'s str), + Optional(&'s str), +} + /// Create iterators for ast nodes where it only has to iterate over children macro_rules! compare_properties { ($emacs:expr, $($emacs_field:expr, $rust_value:expr, $compare_fn: ident),+) => { From 45dd38ac2d79e8d3125ded8d3c9da725dfca5f46 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Fri, 6 Oct 2023 13:40:11 -0400 Subject: [PATCH 4/9] Assume :standard-properties is an expected field. --- src/compare/compare_field.rs | 27 ++++++++++++++++++++ src/compare/diff.rs | 49 ++++++++---------------------------- src/compare/macros.rs | 8 +----- src/compare/mod.rs | 1 + 4 files changed, 40 insertions(+), 45 deletions(-) create mode 100644 src/compare/compare_field.rs diff --git a/src/compare/compare_field.rs b/src/compare/compare_field.rs new file mode 100644 index 0000000..2f96d63 --- /dev/null +++ b/src/compare/compare_field.rs @@ -0,0 +1,27 @@ +use super::diff::DiffStatus; +use super::sexp::Token; +use super::util::get_property_quoted_string; + +#[derive(Debug)] +pub(crate) enum EmacsField<'s> { + Required(&'s str), + Optional(&'s str), +} + +pub(crate) fn compare_property_quoted_string_required_value<'b, 's, 'x>( + emacs: &'b Token<'s>, + emacs_field: &'x str, + rust_value: &'s str, +) -> Result)>, Box> { + let value = get_property_quoted_string(emacs, emacs_field)?; + if value.as_ref().map(String::as_str) != Some(rust_value) { + let this_status = DiffStatus::Bad; + let message = Some(format!( + "{} mismatch (emacs != rust) {:?} != {:?}", + emacs_field, value, rust_value + )); + Ok(Some((this_status, message))) + } else { + Ok(None) + } +} diff --git a/src/compare/diff.rs b/src/compare/diff.rs index 788a2d2..c824519 100644 --- a/src/compare/diff.rs +++ b/src/compare/diff.rs @@ -3,6 +3,7 @@ use std::borrow::Cow; use std::collections::BTreeSet; use std::collections::HashSet; +use super::compare_field::compare_property_quoted_string_required_value; use super::elisp_fact::ElispFact; use super::elisp_fact::GetElispFact; use super::sexp::unquote; @@ -13,8 +14,8 @@ use super::util::get_property_boolean; use super::util::get_property_numeric; use super::util::get_property_quoted_string; use super::util::get_property_unquoted_atom; +use crate::compare::compare_field::EmacsField; use crate::compare::macros::compare_properties; -use crate::compare::macros::EmacsField; use crate::types::AngleLink; use crate::types::AstNode; use crate::types::BabelCall; @@ -126,7 +127,7 @@ pub struct DiffResult<'b, 's> { } #[derive(Debug, PartialEq)] -enum DiffStatus { +pub(crate) enum DiffStatus { Good, Bad, } @@ -425,24 +426,6 @@ fn compare_ast_node<'b, 's>( Ok(compare_result.into()) } -fn compare_property_quoted_string_required_value<'b, 's, 'x>( - emacs: &'b Token<'s>, - emacs_field: &'x str, - rust_value: &'s str, -) -> Result)>, Box> { - let value = get_property_quoted_string(emacs, emacs_field)?; - if value.as_ref().map(String::as_str) != Some(rust_value) { - let this_status = DiffStatus::Bad; - let message = Some(format!( - "{} mismatch (emacs != rust) {:?} != {:?}", - emacs_field, value, rust_value - )); - Ok(Some((this_status, message))) - } else { - Ok(None) - } -} - pub fn compare_document<'b, 's>( emacs: &'b Token<'s>, rust: &'b Document<'s>, @@ -2669,14 +2652,14 @@ fn compare_verbatim<'b, 's>( let mut this_status = DiffStatus::Good; let mut message = None; - // Compare value - let value = get_property_quoted_string(emacs, ":value")?; - if value.as_ref().map(String::as_str) != Some(rust.contents) { - this_status = DiffStatus::Bad; - message = Some(format!( - "Value mismatch (emacs != rust) {:?} != {:?}", - value, rust.contents - )); + if let Some((new_status, new_message)) = compare_properties!( + emacs, + EmacsField::Required(":value"), + rust.contents, + compare_property_quoted_string_required_value + )? { + this_status = new_status; + message = new_message; } Ok(DiffResult { @@ -2698,16 +2681,6 @@ fn compare_code<'b, 's>( let mut this_status = DiffStatus::Good; let mut message = None; - // Compare value - let value = get_property_quoted_string(emacs, ":value")?; - if value.as_ref().map(String::as_str) != Some(rust.contents) { - this_status = DiffStatus::Bad; - message = Some(format!( - "Value mismatch (emacs != rust) {:?} != {:?}", - value, rust.contents - )); - } - if let Some((new_status, new_message)) = compare_properties!( emacs, EmacsField::Required(":value"), diff --git a/src/compare/macros.rs b/src/compare/macros.rs index 4070e77..392ca07 100644 --- a/src/compare/macros.rs +++ b/src/compare/macros.rs @@ -1,9 +1,3 @@ -#[derive(Debug)] -pub(crate) enum EmacsField<'s> { - Required(&'s str), - Optional(&'s str), -} - /// Create iterators for ast nodes where it only has to iterate over children macro_rules! compare_properties { ($emacs:expr, $($emacs_field:expr, $rust_value:expr, $compare_fn: ident),+) => { @@ -16,7 +10,7 @@ macro_rules! compare_properties { .nth(1) .ok_or("Should have an attributes child.")?; let attributes_map = attributes_child.as_map()?; - let mut emacs_keys: BTreeSet<&str> = attributes_map.keys().map(|s| *s).collect(); + let mut emacs_keys: BTreeSet<&str> = attributes_map.keys().map(|s| *s).filter(|s| *s != ":standard-properties").collect(); $( match $emacs_field { EmacsField::Required(name) if emacs_keys.contains(name) => { diff --git a/src/compare/mod.rs b/src/compare/mod.rs index e7bcde8..278146b 100644 --- a/src/compare/mod.rs +++ b/src/compare/mod.rs @@ -1,4 +1,5 @@ mod compare; +mod compare_field; mod diff; mod elisp_fact; mod macros; From ae11e390d16faf989bbcff050d0a5e3d0ff65caa Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Fri, 6 Oct 2023 13:45:19 -0400 Subject: [PATCH 5/9] Add a default case for tokens which do not have any expected properties except for :standard-properties. --- src/compare/diff.rs | 9 ++++++-- src/compare/macros.rs | 51 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/src/compare/diff.rs b/src/compare/diff.rs index c824519..4b3a670 100644 --- a/src/compare/diff.rs +++ b/src/compare/diff.rs @@ -2575,9 +2575,14 @@ fn compare_bold<'b, 's>( rust: &'b Bold<'s>, ) -> Result, Box> { let children = emacs.as_list()?; - let this_status = DiffStatus::Good; + let mut this_status = DiffStatus::Good; let mut child_status = Vec::new(); - let message = None; + let mut message = None; + + if let Some((new_status, new_message)) = compare_properties!(emacs)? { + this_status = new_status; + message = new_message; + } for (emacs_child, rust_child) in children.iter().skip(2).zip(rust.children.iter()) { child_status.push(compare_ast_node(source, emacs_child, rust_child.into())?); diff --git a/src/compare/macros.rs b/src/compare/macros.rs index 392ca07..0e14012 100644 --- a/src/compare/macros.rs +++ b/src/compare/macros.rs @@ -10,7 +10,15 @@ macro_rules! compare_properties { .nth(1) .ok_or("Should have an attributes child.")?; let attributes_map = attributes_child.as_map()?; - let mut emacs_keys: BTreeSet<&str> = attributes_map.keys().map(|s| *s).filter(|s| *s != ":standard-properties").collect(); + let mut emacs_keys: BTreeSet<&str> = attributes_map.keys().map(|s| *s).collect(); + if emacs_keys.contains(":standard-properties") { + emacs_keys.remove(":standard-properties"); + } else { + this_status = DiffStatus::Bad; + message = Some(format!( + "Emacs token lacks :standard-properties field.", + )); + } $( match $emacs_field { EmacsField::Required(name) if emacs_keys.contains(name) => { @@ -71,6 +79,47 @@ macro_rules! compare_properties { } } }; + // Default case for when there are no expected properties except for :standard-properties + ($emacs:expr) => { + { + let mut this_status = DiffStatus::Good; + let mut message: Option = None; + let children = $emacs.as_list()?; + let attributes_child = children + .iter() + .nth(1) + .ok_or("Should have an attributes child.")?; + let attributes_map = attributes_child.as_map()?; + let mut emacs_keys: BTreeSet<&str> = attributes_map.keys().map(|s| *s).collect(); + if emacs_keys.contains(":standard-properties") { + emacs_keys.remove(":standard-properties"); + } else { + this_status = DiffStatus::Bad; + message = Some(format!( + "Emacs token lacks :standard-properties field.", + )); + } + + if !emacs_keys.is_empty() { + let unexpected_keys: Vec<&str> = emacs_keys.into_iter().collect(); + let unexpected_keys = unexpected_keys.join(", "); + this_status = DiffStatus::Bad; + message = Some(format!( + "Emacs token had extra field(s): {}", + unexpected_keys + )); + } + match this_status { + DiffStatus::Good => { + let result: Result<_, Box> = Ok(None); + result + }, + _ => { + Ok(Some((this_status, message))) + } + } + } + }; } pub(crate) use compare_properties; From 3da52a0826fe3e67df2112e739e01146a75197d1 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Fri, 6 Oct 2023 14:07:25 -0400 Subject: [PATCH 6/9] Make a more generic version of compare_property_quoted_string. This allows for the rust value to be determined by a function rather than hard-coded. --- src/compare/compare_field.rs | 42 ++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/compare/compare_field.rs b/src/compare/compare_field.rs index 2f96d63..e736170 100644 --- a/src/compare/compare_field.rs +++ b/src/compare/compare_field.rs @@ -1,3 +1,5 @@ +use std::fmt::Debug; + use super::diff::DiffStatus; use super::sexp::Token; use super::util::get_property_quoted_string; @@ -5,9 +7,22 @@ use super::util::get_property_quoted_string; #[derive(Debug)] pub(crate) enum EmacsField<'s> { Required(&'s str), + #[allow(dead_code)] Optional(&'s str), } +/// Do no comparison. +/// +/// This is for when you want to acknowledge that a field exists in the emacs token, but you do not have any validation for it when using the compare_properties!() macro. Ideally, this should be kept to a minimum since this represents untested values. +#[allow(dead_code)] +pub(crate) fn compare_noop<'b, 's, 'x>( + _emacs: &'b Token<'s>, + _emacs_field: &'x str, + _rust_value: (), +) -> Result)>, Box> { + Ok(None) +} + pub(crate) fn compare_property_quoted_string_required_value<'b, 's, 'x>( emacs: &'b Token<'s>, emacs_field: &'x str, @@ -25,3 +40,30 @@ pub(crate) fn compare_property_quoted_string_required_value<'b, 's, 'x>( Ok(None) } } + +pub(crate) fn compare_property_quoted_string< + 'b, + 's, + 'x, + R, + RV: Debug + for<'a> PartialEq>, + RG: Fn(R) -> RV, +>( + emacs: &'b Token<'s>, + emacs_field: &'x str, + rust_node: R, + rust_value_getter: RG, +) -> Result)>, Box> { + let value = get_property_quoted_string(emacs, emacs_field)?; + let rust_value = rust_value_getter(rust_node); + if !rust_value.eq(&value.as_ref().map(String::as_str)) { + let this_status = DiffStatus::Bad; + let message = Some(format!( + "{} mismatch (emacs != rust) {:?} != {:?}", + emacs_field, value, rust_value + )); + Ok(Some((this_status, message))) + } else { + Ok(None) + } +} From c7dbe596b37d28bb0ef2ed83c9edd65738f8cb7c Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Fri, 6 Oct 2023 16:03:41 -0400 Subject: [PATCH 7/9] Switch to more generic implementation. --- src/compare/compare_field.rs | 29 ++--------------------------- src/compare/diff.rs | 11 +++++++---- src/compare/macros.rs | 4 ++-- 3 files changed, 11 insertions(+), 33 deletions(-) diff --git a/src/compare/compare_field.rs b/src/compare/compare_field.rs index e736170..c922b63 100644 --- a/src/compare/compare_field.rs +++ b/src/compare/compare_field.rs @@ -23,35 +23,10 @@ pub(crate) fn compare_noop<'b, 's, 'x>( Ok(None) } -pub(crate) fn compare_property_quoted_string_required_value<'b, 's, 'x>( +pub(crate) fn compare_property_quoted_string<'b, 's, 'x, R, RG: Fn(R) -> Option<&'s str>>( emacs: &'b Token<'s>, - emacs_field: &'x str, - rust_value: &'s str, -) -> Result)>, Box> { - let value = get_property_quoted_string(emacs, emacs_field)?; - if value.as_ref().map(String::as_str) != Some(rust_value) { - let this_status = DiffStatus::Bad; - let message = Some(format!( - "{} mismatch (emacs != rust) {:?} != {:?}", - emacs_field, value, rust_value - )); - Ok(Some((this_status, message))) - } else { - Ok(None) - } -} - -pub(crate) fn compare_property_quoted_string< - 'b, - 's, - 'x, - R, - RV: Debug + for<'a> PartialEq>, - RG: Fn(R) -> RV, ->( - emacs: &'b Token<'s>, - emacs_field: &'x str, rust_node: R, + emacs_field: &'x str, rust_value_getter: RG, ) -> Result)>, Box> { let value = get_property_quoted_string(emacs, emacs_field)?; diff --git a/src/compare/diff.rs b/src/compare/diff.rs index 4b3a670..af12011 100644 --- a/src/compare/diff.rs +++ b/src/compare/diff.rs @@ -3,6 +3,7 @@ use std::borrow::Cow; use std::collections::BTreeSet; use std::collections::HashSet; +use super::compare_field::compare_property_quoted_string; use super::compare_field::compare_property_quoted_string_required_value; use super::elisp_fact::ElispFact; use super::elisp_fact::GetElispFact; @@ -2659,9 +2660,10 @@ fn compare_verbatim<'b, 's>( if let Some((new_status, new_message)) = compare_properties!( emacs, + rust, EmacsField::Required(":value"), - rust.contents, - compare_property_quoted_string_required_value + |r| Some(r.contents), + compare_property_quoted_string )? { this_status = new_status; message = new_message; @@ -2688,9 +2690,10 @@ fn compare_code<'b, 's>( if let Some((new_status, new_message)) = compare_properties!( emacs, + rust, EmacsField::Required(":value"), - rust.contents, - compare_property_quoted_string_required_value + |r| Some(r.contents), + compare_property_quoted_string )? { this_status = new_status; message = new_message; diff --git a/src/compare/macros.rs b/src/compare/macros.rs index 0e14012..c40c719 100644 --- a/src/compare/macros.rs +++ b/src/compare/macros.rs @@ -1,6 +1,6 @@ /// Create iterators for ast nodes where it only has to iterate over children macro_rules! compare_properties { - ($emacs:expr, $($emacs_field:expr, $rust_value:expr, $compare_fn: ident),+) => { + ($emacs:expr, $rust:expr, $($emacs_field:expr, $rust_value_getter:expr, $compare_fn: ident),+) => { { let mut this_status = DiffStatus::Good; let mut message: Option = None; @@ -57,7 +57,7 @@ macro_rules! compare_properties { name }, }; - let result = $compare_fn($emacs, emacs_name, $rust_value)?; + let result = $compare_fn($emacs, $rust, emacs_name, $rust_value_getter)?; match result { Some((DiffStatus::Good, _)) => unreachable!("No comparison functions should return Some() when DiffStatus is good."), Some((status, msg)) => { From 368c6a457edd703e4bb6a25fe3af491677f9452b Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Fri, 6 Oct 2023 16:19:43 -0400 Subject: [PATCH 8/9] Wrap the parameter sets in parenthesis. --- src/compare/compare_field.rs | 13 +++++++++++-- src/compare/diff.rs | 29 ++++++++++++++++++++++------- src/compare/macros.rs | 18 +++++++++++++++--- 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/src/compare/compare_field.rs b/src/compare/compare_field.rs index c922b63..4a860f7 100644 --- a/src/compare/compare_field.rs +++ b/src/compare/compare_field.rs @@ -15,14 +15,23 @@ pub(crate) enum EmacsField<'s> { /// /// This is for when you want to acknowledge that a field exists in the emacs token, but you do not have any validation for it when using the compare_properties!() macro. Ideally, this should be kept to a minimum since this represents untested values. #[allow(dead_code)] -pub(crate) fn compare_noop<'b, 's, 'x>( +pub(crate) fn impl_compare_noop<'b, 's, 'x, R, RG>( _emacs: &'b Token<'s>, + _rust_node: R, _emacs_field: &'x str, - _rust_value: (), + _rust_value_getter: RG, ) -> Result)>, Box> { Ok(None) } +/// Do no comparison. +/// +/// This is for when you want to acknowledge that a field exists in the emacs token, but you do not have any validation for it when using the compare_properties!() macro. Ideally, this should be kept to a minimum since this represents untested values. +#[allow(dead_code)] +pub(crate) fn compare_identity() -> () { + () +} + pub(crate) fn compare_property_quoted_string<'b, 's, 'x, R, RG: Fn(R) -> Option<&'s str>>( emacs: &'b Token<'s>, rust_node: R, diff --git a/src/compare/diff.rs b/src/compare/diff.rs index af12011..ac63862 100644 --- a/src/compare/diff.rs +++ b/src/compare/diff.rs @@ -3,8 +3,9 @@ use std::borrow::Cow; use std::collections::BTreeSet; use std::collections::HashSet; +use super::compare_field::compare_identity; use super::compare_field::compare_property_quoted_string; -use super::compare_field::compare_property_quoted_string_required_value; +use super::compare_field::impl_compare_noop; use super::elisp_fact::ElispFact; use super::elisp_fact::GetElispFact; use super::sexp::unquote; @@ -2661,9 +2662,21 @@ fn compare_verbatim<'b, 's>( if let Some((new_status, new_message)) = compare_properties!( emacs, rust, - EmacsField::Required(":value"), - |r| Some(r.contents), - compare_property_quoted_string + ( + EmacsField::Required(":value"), + |r| Some(r.contents), + compare_property_quoted_string + ), + ( + EmacsField::Required(":value"), + |r| Some(r.contents), + compare_property_quoted_string + ), + ( + EmacsField::Required(":foo"), + compare_identity, + impl_compare_noop + ) )? { this_status = new_status; message = new_message; @@ -2691,9 +2704,11 @@ fn compare_code<'b, 's>( if let Some((new_status, new_message)) = compare_properties!( emacs, rust, - EmacsField::Required(":value"), - |r| Some(r.contents), - compare_property_quoted_string + ( + EmacsField::Required(":value"), + |r| Some(r.contents), + compare_property_quoted_string + ) )? { this_status = new_status; message = new_message; diff --git a/src/compare/macros.rs b/src/compare/macros.rs index c40c719..2f2947f 100644 --- a/src/compare/macros.rs +++ b/src/compare/macros.rs @@ -1,6 +1,18 @@ +macro_rules! compare_noop { + ($($field:expr),+) => { + $( + EmacsField::Required(":foo"), + compare_identity, + impl_compare_noop +),+ + }; +} + +pub(crate) use compare_noop; + /// Create iterators for ast nodes where it only has to iterate over children macro_rules! compare_properties { - ($emacs:expr, $rust:expr, $($emacs_field:expr, $rust_value_getter:expr, $compare_fn: ident),+) => { + ($emacs:expr, $rust:expr, $(($emacs_field:expr, $rust_value_getter:expr, $compare_fn: expr)),+) => { { let mut this_status = DiffStatus::Good; let mut message: Option = None; @@ -36,7 +48,7 @@ macro_rules! compare_properties { }, EmacsField::Optional(_name) => {}, } - ),+ + )+ if !emacs_keys.is_empty() { let unexpected_keys: Vec<&str> = emacs_keys.into_iter().collect(); @@ -66,7 +78,7 @@ macro_rules! compare_properties { }, _ => {} } - ),+ + )+ match this_status { DiffStatus::Good => { From fbe3c76ab780dbf67ab672d340e006db8a300f52 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Fri, 6 Oct 2023 16:32:49 -0400 Subject: [PATCH 9/9] Cleanup. --- src/compare/compare_field.rs | 2 +- src/compare/diff.rs | 39 +++++++++++++++++--------------- src/compare/macros.rs | 44 +++++++++++++++++++++++++----------- 3 files changed, 53 insertions(+), 32 deletions(-) diff --git a/src/compare/compare_field.rs b/src/compare/compare_field.rs index 4a860f7..5a26323 100644 --- a/src/compare/compare_field.rs +++ b/src/compare/compare_field.rs @@ -15,7 +15,7 @@ pub(crate) enum EmacsField<'s> { /// /// This is for when you want to acknowledge that a field exists in the emacs token, but you do not have any validation for it when using the compare_properties!() macro. Ideally, this should be kept to a minimum since this represents untested values. #[allow(dead_code)] -pub(crate) fn impl_compare_noop<'b, 's, 'x, R, RG>( +pub(crate) fn compare_noop<'b, 's, 'x, R, RG>( _emacs: &'b Token<'s>, _rust_node: R, _emacs_field: &'x str, diff --git a/src/compare/diff.rs b/src/compare/diff.rs index ac63862..cf802f8 100644 --- a/src/compare/diff.rs +++ b/src/compare/diff.rs @@ -3,9 +3,7 @@ use std::borrow::Cow; use std::collections::BTreeSet; use std::collections::HashSet; -use super::compare_field::compare_identity; use super::compare_field::compare_property_quoted_string; -use super::compare_field::impl_compare_noop; use super::elisp_fact::ElispFact; use super::elisp_fact::GetElispFact; use super::sexp::unquote; @@ -2607,9 +2605,14 @@ fn compare_italic<'b, 's>( rust: &'b Italic<'s>, ) -> Result, Box> { let children = emacs.as_list()?; - let this_status = DiffStatus::Good; + let mut this_status = DiffStatus::Good; let mut child_status = Vec::new(); - let message = None; + let mut message = None; + + if let Some((new_status, new_message)) = compare_properties!(emacs)? { + this_status = new_status; + message = new_message; + } for (emacs_child, rust_child) in children.iter().skip(2).zip(rust.children.iter()) { child_status.push(compare_ast_node(source, emacs_child, rust_child.into())?); @@ -2632,9 +2635,14 @@ fn compare_underline<'b, 's>( rust: &'b Underline<'s>, ) -> Result, Box> { let children = emacs.as_list()?; - let this_status = DiffStatus::Good; + let mut this_status = DiffStatus::Good; let mut child_status = Vec::new(); - let message = None; + let mut message = None; + + if let Some((new_status, new_message)) = compare_properties!(emacs)? { + this_status = new_status; + message = new_message; + } for (emacs_child, rust_child) in children.iter().skip(2).zip(rust.children.iter()) { child_status.push(compare_ast_node(source, emacs_child, rust_child.into())?); @@ -2666,16 +2674,6 @@ fn compare_verbatim<'b, 's>( EmacsField::Required(":value"), |r| Some(r.contents), compare_property_quoted_string - ), - ( - EmacsField::Required(":value"), - |r| Some(r.contents), - compare_property_quoted_string - ), - ( - EmacsField::Required(":foo"), - compare_identity, - impl_compare_noop ) )? { this_status = new_status; @@ -2731,9 +2729,14 @@ fn compare_strike_through<'b, 's>( rust: &'b StrikeThrough<'s>, ) -> Result, Box> { let children = emacs.as_list()?; - let this_status = DiffStatus::Good; + let mut this_status = DiffStatus::Good; let mut child_status = Vec::new(); - let message = None; + let mut message = None; + + if let Some((new_status, new_message)) = compare_properties!(emacs)? { + this_status = new_status; + message = new_message; + } for (emacs_child, rust_child) in children.iter().skip(2).zip(rust.children.iter()) { child_status.push(compare_ast_node(source, emacs_child, rust_child.into())?); diff --git a/src/compare/macros.rs b/src/compare/macros.rs index 2f2947f..c71ce9d 100644 --- a/src/compare/macros.rs +++ b/src/compare/macros.rs @@ -1,16 +1,34 @@ -macro_rules! compare_noop { - ($($field:expr),+) => { - $( - EmacsField::Required(":foo"), - compare_identity, - impl_compare_noop -),+ - }; -} - -pub(crate) use compare_noop; - -/// Create iterators for ast nodes where it only has to iterate over children +/// Assert only the listed properties exist on the Emacs AST node and compare their values to the values on the rust AST node. +/// +/// Example invocation: +/// ``` +/// if let Some((new_status, new_message)) = compare_properties!( +/// emacs, +/// rust, +/// ( +/// EmacsField::Required(":key"), +/// |r| Some(r.key), +/// compare_property_quoted_string +/// ), +/// ( +/// EmacsField::Required(":value"), +/// |r| Some(r.contents), +/// compare_property_quoted_string +/// ), +/// (EmacsField::Required(":pre-blank"), compare_identity, compare_noop) +/// )? { +/// this_status = new_status; +/// message = new_message; +/// } +/// ``` +/// +/// Or if the node has no properties aside from :standard-properties, we can still assert that the node has no unexpected properties: +/// ``` +/// if let Some((new_status, new_message)) = compare_properties!(emacs)? { +/// this_status = new_status; +/// message = new_message; +/// } +/// ``` macro_rules! compare_properties { ($emacs:expr, $rust:expr, $(($emacs_field:expr, $rust_value_getter:expr, $compare_fn: expr)),+) => { {