Fix character class and choice corner cases in address patterns

src/address.rs

@@ -1,5 +1,6 @@
 use crate::errors::OscError;
 
+use alloc::iter::Peekable;
 use alloc::string::{String, ToString};
 use alloc::vec::Vec;
 use core::fmt::{Display, Formatter};
@@ -108,11 +109,9 @@ impl Matcher {
             let result = match part {
                 AddressPatternComponent::Tag(s) => match_literally(remainder, s),
                 AddressPatternComponent::WildcardSingle => match_wildcard_single(remainder),
-                AddressPatternComponent::Wildcard(l) => {
-                    match_wildcard(remainder, *l, iter.peek().copied())
-                }
+                AddressPatternComponent::Wildcard(l) => match_wildcard(remainder, *l, iter.clone()),
                 AddressPatternComponent::CharacterClass(cc) => match_character_class(remainder, cc),
-                AddressPatternComponent::Choice(s) => match_choice(remainder, s),
+                AddressPatternComponent::Choice(s) => match_choice(remainder, s, iter.clone()),
             };
 
             remainder = match result {
@@ -165,7 +164,7 @@ fn pattern_character_class(input: &str) -> IResult<&str, &str> {
                 // separated pair parser above. Will always validate as true.
                 satisfy(is_address_character).map(|c| ('\0', c)),
             )),
-            |(o1, o2): &(char, char)| o1 < o2,
+            |(o1, o2): &(char, char)| o1 <= o2,
         ))),
     );
 
@@ -285,22 +284,76 @@ fn match_character_class<'a>(
     input: &'a str,
     character_class: &'a CharacterClass,
 ) -> IResult<&'a str, &'a str> {
+    // Character classes in OSC only match one character. nom does not include a method similar to
+    // `is_not` or `is_a` that only checks one character. So we explicitly take one character from the input
+    // using `take_while_m_n` and then use the `is_not` or `is_a` to check for matches.
     if character_class.negated {
-        is_not(character_class.characters.as_str())(input)
+        let (rest, single_char) = take_while_m_n(1, 1, |_| true)(input)?;
+        let (_, matched) = is_not(character_class.characters.as_str())(single_char)?;
+        Ok((rest, matched))
     } else {
-        is_a(character_class.characters.as_str())(input)
+        let (rest, single_char) = take_while_m_n(1, 1, |_| true)(input)?;
+        let (_, matched) = is_a(character_class.characters.as_str())(single_char)?;
+        Ok((rest, matched))
     }
 }
 
 /// Sequentially try all tags from choice element until one matches or return an error
 /// Example choice element: '{foo,bar}'
 /// It will get parsed into a vector containing the strings "foo" and "bar", which are then matched
-fn match_choice<'a>(input: &'a str, choices: &[String]) -> IResult<&'a str, &'a str> {
+///
+/// This needs lookahead, because we need to decide if we matched with the correct choice.
+/// Example:
+/// `/{1, 10}/mute`: When choices are substrings of one another, we need to know with which
+/// choice we are supposed to match the input with (because it affects the remainder as the choice
+/// is consumed)
+/// Imagine the following address: `/10/mute`. Both choices match, but the remainder (the part
+/// after the match) looks different.
+/// Match with `1`:  `0/mute` errors out in the next step
+/// Match with `10`: `/mute` successfully matches
+fn match_choice<'a>(
+    input: &'a str,
+    choices: &[String],
+    mut lookahead_iter: Peekable<std::slice::Iter<'_, AddressPatternComponent>>,
+) -> IResult<&'a str, &'a str> {
+    let next_component = lookahead_iter.next();
     for choice in choices {
-        if let Ok((i, o)) = tag::<_, _, nom::error::Error<&str>>(choice.as_str())(input) {
-            return Ok((i, o));
+        if let Ok((remainder, o)) = tag::<_, _, nom::error::Error<&str>>(choice.as_str())(input) {
+            // Lookahead to see if the rest of the string still parses
+            match next_component {
+                Some(component) => {
+                    let result: IResult<_, _, nom::error::Error<&str>> = match component {
+                        AddressPatternComponent::Tag(s) => match_literally(&remainder, s.as_str()),
+                        AddressPatternComponent::CharacterClass(cc) => {
+                            match_character_class(remainder, cc)
+                        }
+                        AddressPatternComponent::Choice(s) => {
+                            match_choice(remainder, s, lookahead_iter.clone())
+                        }
+                        AddressPatternComponent::WildcardSingle => {
+                            match_wildcard_single(remainder)
+                        }
+                        AddressPatternComponent::Wildcard(l) => {
+                            match_wildcard(remainder, *l, lookahead_iter.clone())
+                        }
+                    };
+                    // Is true when the rest of the address string parsed successfully
+                    if result.is_ok() {
+                        return Ok((remainder, o));
+                    }
+                }
+                None => {
+                    // If no further AddressPatternComponents are expected, check that the
+                    // remainder is empty. Otherwise the string has more characters which where not
+                    // expected in the address pattern and thus does not match the address pattern.
+                    if remainder.is_empty() {
+                        return Ok((remainder, o));
+                    }
+                }
+            }
         }
     }
+
     Err(nom::Err::Error(nom::error::Error::from_error_kind(
         input,
         ErrorKind::Tag,
@@ -312,9 +365,10 @@ fn match_choice<'a>(input: &'a str, choices: &[String]) -> IResult<&'a str, &'a
 fn match_wildcard<'a>(
     input: &'a str,
     minimum_length: usize,
-    next: Option<&AddressPatternComponent>,
+    mut iter: Peekable<std::slice::Iter<'_, AddressPatternComponent>>,
 ) -> IResult<&'a str, &'a str> {
     // If the next component is a '/', there are no more components in the current part and it can be wholly consumed
+    let next = iter.next();
     let next = next.filter(|&part| match part {
         AddressPatternComponent::Tag(s) => s != "/",
         _ => true,
@@ -342,7 +396,7 @@ fn match_wildcard<'a>(
                     AddressPatternComponent::CharacterClass(cc) => {
                         match_character_class(substring, cc)
                     }
-                    AddressPatternComponent::Choice(s) => match_choice(substring, s),
+                    AddressPatternComponent::Choice(s) => match_choice(substring, s, iter.clone()),
                     // These two cases are prevented from happening by map_address_pattern_component
                     AddressPatternComponent::WildcardSingle => {
                         panic!("Single wildcard ('?') must not follow wildcard ('*')")

tests/address_test.rs

@@ -38,6 +38,28 @@ fn test_matcher() {
     assert!(matcher
         .match_address(&OscAddress::new(String::from("/footron")).expect("Valid address pattern")));
 
+    matcher = Matcher::new("/oscillator/{1,10}/frequency").expect("Should be valid");
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/1/frequency")).expect("Valid address pattern")
+    ));
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/10/frequency")).expect("Valid address pattern")
+    ));
+
+    matcher = Matcher::new("/oscillator/{1,10}").expect("Should be valid");
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/1")).expect("Valid address pattern")
+    ));
+    assert!(!matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/12")).expect("Valid address pattern")
+    ));
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/10")).expect("Valid address pattern")
+    ));
+    assert!(!matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/100")).expect("Valid address pattern")
+    ));
+
     // Character class
     // Character classes are sets or ranges of characters to match.
     // e.g. [a-z] will match any lower case alphabetic character. [abcd] will match the characters abcd.
@@ -93,6 +115,44 @@ fn test_matcher() {
         &OscAddress::new(String::from("/oscillator/-")).expect("Valid address pattern")
     ));
 
+    matcher = Matcher::new("/oscillator/[1-128]").expect("Should be valid");
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/1")).expect("Valid address pattern")
+    ));
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/2")).expect("Valid address pattern")
+    ));
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/8")).expect("Valid address pattern")
+    ));
+    assert!(!matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/11")).expect("Valid address pattern")
+    ));
+    assert!(!matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/12")).expect("Valid address pattern")
+    ));
+    assert!(!matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/28")).expect("Valid address pattern")
+    ));
+    assert!(!matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/128")).expect("Valid address pattern")
+    ));
+
+    // Trailing dash has no special meaning
+    matcher = Matcher::new("/oscillator/[12345]?").expect("Should be valid");
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/12")).expect("Valid address pattern")
+    ));
+    assert!(!matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/112")).expect("Valid address pattern")
+    ));
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/50")).expect("Valid address pattern")
+    ));
+    assert!(!matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/60")).expect("Valid address pattern")
+    ));
+
     // Single wildcard
     // A single wildcard '?' matches exactly one alphanumeric character
     matcher = Matcher::new("/oscillator/?/frequency").expect("Should be valid");
@@ -201,6 +261,56 @@ fn test_matcher() {
             .expect("Valid address pattern")
     ));
 
+    matcher = Matcher::new("/oscillator/*{1,10}/frequency").expect("Should be valid");
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/foo1/frequency"))
+            .expect("Valid address pattern")
+    ));
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/foo10/frequency"))
+            .expect("Valid address pattern")
+    ));
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/1/frequency")).expect("Valid address pattern")
+    ));
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/10/frequency")).expect("Valid address pattern")
+    ));
+    assert!(!matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/something/frequency"))
+            .expect("Valid address pattern")
+    ));
+
+    matcher = Matcher::new("/oscillator/*{1,10}?/frequency").expect("Should be valid");
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/foo1a/frequency"))
+            .expect("Valid address pattern")
+    ));
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/foo10/frequency"))
+            .expect("Valid address pattern")
+    ));
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/10/frequency")).expect("Valid address pattern")
+    ));
+    assert!(!matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/something/frequency"))
+            .expect("Valid address pattern")
+    ));
+
+    matcher = Matcher::new("/oscillator/*{10,1}?/frequency").expect("Should be valid");
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/foo1a/frequency"))
+            .expect("Valid address pattern")
+    ));
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/foo10/frequency"))
+            .expect("Valid address pattern")
+    ));
+    assert!(matcher.match_address(
+        &OscAddress::new(String::from("/oscillator/10/frequency")).expect("Valid address pattern")
+    ));
+
     // Wildcard as last part
     matcher = Matcher::new("/oscillator/*").expect("Should be valid");
     assert!(matcher.match_address(
@@ -280,15 +390,16 @@ fn test_verify_address_pattern() {
     verify_address_pattern("/test[a-za-z]").expect("Should be valid");
     verify_address_pattern("/test[a-z]*??/{foo,bar,baz}[!a-z0-9]/*").expect("Should be valid");
     verify_address_pattern("/test{foo}").expect("Should be valid");
+    // Character range starting and ending at same character.
+    // Is equivalent to /[a], but no reason to forbid
+    verify_address_pattern("/[a-a]").expect("Should be valid");
 
     // Empty element in choice
     verify_address_pattern("/{asd,}/").expect_err("Should not be valid");
     // Illegal character in range
     verify_address_pattern("/[a-b*]/").expect_err("Should not be valid");
     // Character range is reversed
     verify_address_pattern("/[b-a]").expect_err("Should not be valid");
-    // Character range starting and ending at same character
-    verify_address_pattern("/[a-a]").expect_err("Should not be valid");
 
     // Empty
     verify_address_pattern("").expect_err("Should not be valid");