syntax: simplify hir::Repetition

This greatly simplifies how repetitions are represented in the HIR from
a sprawling set of variants down to just a simple `(u32, Option<u32>)`.
This is much simpler and still permits us to specialize all of the cases
we did before if necessary.

This also simplifies some of the HIR printer's output. e.g., 'a{1}' is
just 'a'.

Author: BurntSushi

regex-syntax/src/hir/literal/mod.rs

@@ -602,26 +602,19 @@ fn prefixes(expr: &Hir, lits: &mut Literals) {
         HirKind::Group(hir::Group { ref hir, .. }) => {
             prefixes(&**hir, lits);
         }
-        HirKind::Repetition(ref x) => match x.kind {
-            hir::RepetitionKind::ZeroOrOne => {
+        HirKind::Repetition(ref x) => match (x.min, x.max) {
+            (0, Some(1)) => {
                 repeat_zero_or_one_literals(&x.hir, lits, prefixes);
             }
-            hir::RepetitionKind::ZeroOrMore => {
+            (0, None) => {
                 repeat_zero_or_more_literals(&x.hir, lits, prefixes);
             }
-            hir::RepetitionKind::OneOrMore => {
+            (1, None) => {
                 repeat_one_or_more_literals(&x.hir, lits, prefixes);
             }
-            hir::RepetitionKind::Range(ref rng) => {
-                let (min, max) = match *rng {
-                    hir::RepetitionRange::Exactly(m) => (m, Some(m)),
-                    hir::RepetitionRange::AtLeast(m) => (m, None),
-                    hir::RepetitionRange::Bounded(m, n) => (m, Some(n)),
-                };
-                repeat_range_literals(
-                    &x.hir, min, max, x.greedy, lits, prefixes,
-                )
-            }
+            (min, max) => repeat_range_literals(
+                &x.hir, min, max, x.greedy, lits, prefixes,
+            ),
         },
         HirKind::Concat(ref es) if es.is_empty() => {}
         HirKind::Concat(ref es) if es.len() == 1 => prefixes(&es[0], lits),
@@ -678,26 +671,19 @@ fn suffixes(expr: &Hir, lits: &mut Literals) {
         HirKind::Group(hir::Group { ref hir, .. }) => {
             suffixes(&**hir, lits);
         }
-        HirKind::Repetition(ref x) => match x.kind {
-            hir::RepetitionKind::ZeroOrOne => {
+        HirKind::Repetition(ref x) => match (x.min, x.max) {
+            (0, Some(1)) => {
                 repeat_zero_or_one_literals(&x.hir, lits, suffixes);
             }
-            hir::RepetitionKind::ZeroOrMore => {
+            (0, None) => {
                 repeat_zero_or_more_literals(&x.hir, lits, suffixes);
             }
-            hir::RepetitionKind::OneOrMore => {
+            (1, None) => {
                 repeat_one_or_more_literals(&x.hir, lits, suffixes);
             }
-            hir::RepetitionKind::Range(ref rng) => {
-                let (min, max) = match *rng {
-                    hir::RepetitionRange::Exactly(m) => (m, Some(m)),
-                    hir::RepetitionRange::AtLeast(m) => (m, None),
-                    hir::RepetitionRange::Bounded(m, n) => (m, Some(n)),
-                };
-                repeat_range_literals(
-                    &x.hir, min, max, x.greedy, lits, suffixes,
-                )
-            }
+            (min, max) => repeat_range_literals(
+                &x.hir, min, max, x.greedy, lits, suffixes,
+            ),
         },
         HirKind::Concat(ref es) if es.is_empty() => {}
         HirKind::Concat(ref es) if es.len() == 1 => suffixes(&es[0], lits),
@@ -736,7 +722,8 @@ fn repeat_zero_or_one_literals<F: FnMut(&Hir, &mut Literals)>(
 ) {
     f(
         &Hir::repetition(hir::Repetition {
-            kind: hir::RepetitionKind::ZeroOrMore,
+            min: 0,
+            max: None,
             // FIXME: Our literal extraction doesn't care about greediness.
             // Which is partially why we're treating 'e?' as 'e*'. Namely,
             // 'ab??' yields [Complete(ab), Complete(a)], but it should yield
@@ -794,7 +781,8 @@ fn repeat_range_literals<F: FnMut(&Hir, &mut Literals)>(
         // just treat it as `e*`.
         f(
             &Hir::repetition(hir::Repetition {
-                kind: hir::RepetitionKind::ZeroOrMore,
+                min: 0,
+                max: None,
                 greedy,
                 hir: Box::new(e.clone()),
             }),

regex-syntax/src/hir/mod.rs

@@ -1362,8 +1362,18 @@ pub enum GroupKind {
 /// sub-expression.
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct Repetition {
-    /// The kind of this repetition operator.
-    pub kind: RepetitionKind,
+    /// The minimum range of the repetition.
+    ///
+    /// Note that special cases like `?`, `+` and `*` all get translated into
+    /// the ranges `{0,1}`, `{1,}` and `{0,}`, respectively.
+    pub min: u32,
+    /// The maximum range of the repetition.
+    ///
+    /// Note that when `max` is `None`, `min` acts as a lower bound but where
+    /// there is no upper bound. For something like `x{5}` where the min and
+    /// max are equivalent, `min` will be set to `5` and `max` will be set to
+    /// `Some(5)`.
+    pub max: Option<u32>,
     /// Whether this repetition operator is greedy or not. A greedy operator
     /// will match as much as it can. A non-greedy operator will match as
     /// little as it can.
@@ -1385,42 +1395,14 @@ impl Repetition {
     /// string and one or more occurrences of something that matches the
     /// empty string will always match the empty string. In order to get the
     /// inductive definition, see the corresponding method on [`Hir`].
+    ///
+    /// This returns true in precisely the cases that [`Repetition::min`]
+    /// is equal to `0`.
     pub fn is_match_empty(&self) -> bool {
-        match self.kind {
-            RepetitionKind::ZeroOrOne => true,
-            RepetitionKind::ZeroOrMore => true,
-            RepetitionKind::OneOrMore => false,
-            RepetitionKind::Range(RepetitionRange::Exactly(m)) => m == 0,
-            RepetitionKind::Range(RepetitionRange::AtLeast(m)) => m == 0,
-            RepetitionKind::Range(RepetitionRange::Bounded(m, _)) => m == 0,
-        }
+        self.min == 0
     }
 }
 
-/// The kind of a repetition operator.
-#[derive(Clone, Debug, Eq, PartialEq)]
-pub enum RepetitionKind {
-    /// Matches a sub-expression zero or one times.
-    ZeroOrOne,
-    /// Matches a sub-expression zero or more times.
-    ZeroOrMore,
-    /// Matches a sub-expression one or more times.
-    OneOrMore,
-    /// Matches a sub-expression within a bounded range of times.
-    Range(RepetitionRange),
-}
-
-/// The kind of a counted repetition operator.
-#[derive(Clone, Debug, Eq, PartialEq)]
-pub enum RepetitionRange {
-    /// Matches a sub-expression exactly this many times.
-    Exactly(u32),
-    /// Matches a sub-expression at least this many times.
-    AtLeast(u32),
-    /// Matches a sub-expression at least `m` times and at most `n` times.
-    Bounded(u32, u32),
-}
-
 /// A custom `Drop` impl is used for `HirKind` such that it uses constant stack
 /// space but heap space proportional to the depth of the total `Hir`.
 impl Drop for Hir {
@@ -2257,7 +2239,8 @@ mod tests {
                     hir: Box::new(expr),
                 });
                 expr = Hir::repetition(Repetition {
-                    kind: RepetitionKind::ZeroOrOne,
+                    min: 0,
+                    max: Some(1),
                     greedy: true,
                     hir: Box::new(expr),
                 });

regex-syntax/src/hir/print.rs

@@ -176,27 +176,31 @@ impl<W: fmt::Write> Visitor for Writer<W> {
             | HirKind::Concat(_)
             | HirKind::Alternation(_) => {}
             HirKind::Repetition(ref x) => {
-                match x.kind {
-                    hir::RepetitionKind::ZeroOrOne => {
+                match (x.min, x.max) {
+                    (0, Some(1)) => {
                         self.wtr.write_str("?")?;
                     }
-                    hir::RepetitionKind::ZeroOrMore => {
+                    (0, None) => {
                         self.wtr.write_str("*")?;
                     }
-                    hir::RepetitionKind::OneOrMore => {
+                    (1, None) => {
                         self.wtr.write_str("+")?;
                     }
-                    hir::RepetitionKind::Range(ref x) => match *x {
-                        hir::RepetitionRange::Exactly(m) => {
-                            write!(self.wtr, "{{{}}}", m)?;
-                        }
-                        hir::RepetitionRange::AtLeast(m) => {
-                            write!(self.wtr, "{{{},}}", m)?;
-                        }
-                        hir::RepetitionRange::Bounded(m, n) => {
-                            write!(self.wtr, "{{{},{}}}", m, n)?;
-                        }
-                    },
+                    (1, Some(1)) => {
+                        // 'a{1}' and 'a{1}?' are exactly equivalent to 'a'.
+                        return Ok(());
+                    }
+                    (m, None) => {
+                        write!(self.wtr, "{{{},}}", m)?;
+                    }
+                    (m, Some(n)) if m == n => {
+                        write!(self.wtr, "{{{}}}", m)?;
+                        // a{m} and a{m}? are always exactly equivalent.
+                        return Ok(());
+                    }
+                    (m, Some(n)) => {
+                        write!(self.wtr, "{{{},{}}}", m, n)?;
+                    }
                 }
                 if !x.greedy {
                     self.wtr.write_str("?")?;
@@ -241,7 +245,10 @@ impl<W: fmt::Write> Writer<W> {
 
 #[cfg(test)]
 mod tests {
-    use alloc::string::String;
+    use alloc::{
+        boxed::Box,
+        string::{String, ToString},
+    };
 
     use crate::ParserBuilder;
 
@@ -338,14 +345,17 @@ mod tests {
         roundtrip("a+?", "a+?");
         roundtrip("(?U)a+", "a+?");
 
-        roundtrip("a{1}", "a{1}");
-        roundtrip("a{1,}", "a{1,}");
+        roundtrip("a{1}", "a");
+        roundtrip("a{2}", "a{2}");
+        roundtrip("a{1,}", "a+");
         roundtrip("a{1,5}", "a{1,5}");
-        roundtrip("a{1}?", "a{1}?");
-        roundtrip("a{1,}?", "a{1,}?");
+        roundtrip("a{1}?", "a");
+        roundtrip("a{2}?", "a{2}");
+        roundtrip("a{1,}?", "a+?");
         roundtrip("a{1,5}?", "a{1,5}?");
-        roundtrip("(?U)a{1}", "a{1}?");
-        roundtrip("(?U)a{1,}", "a{1,}?");
+        roundtrip("(?U)a{1}", "a");
+        roundtrip("(?U)a{2}", "a{2}");
+        roundtrip("(?U)a{1,}", "a+?");
         roundtrip("(?U)a{1,5}", "a{1,5}?");
     }
 
@@ -371,4 +381,85 @@ mod tests {
         roundtrip("a|b|c", "a|b|c");
         roundtrip("foo|bar|quux", "foo|bar|quux");
     }
+
+    // This is a regression test that stresses a peculiarity of how the HIR
+    // is both constructed and printed. Namely, it is legal for a repetition
+    // to directly contain a concatenation. This particular construct isn't
+    // really possible to build from the concrete syntax directly, since you'd
+    // be forced to put the concatenation into (at least) a non-capturing
+    // group. Concurrently, the printer doesn't consider this case and just
+    // kind of naively prints the child expression and tacks on the repetition
+    // operator.
+    //
+    // As a result, if you attached '+' to a 'concat(a, b)', the printer gives
+    // you 'ab+', but clearly it really should be '(?:ab)+'.
+    //
+    // This bug isn't easy to surface because most ways of building an HIR
+    // come directly from the concrete syntax, and as mentioned above, it just
+    // isn't possible to build this kind of HIR from the concrete syntax.
+    // Nevertheless, this is definitely a bug.
+    //
+    // See: https://github.com/rust-lang/regex/issues/731
+    #[test]
+    fn regression_repetition_concat() {
+        let expr = Hir::concat(alloc::vec![
+            Hir::literal(hir::Literal::Unicode('x')),
+            Hir::repetition(hir::Repetition {
+                min: 1,
+                max: None,
+                greedy: true,
+                hir: Box::new(Hir::concat(alloc::vec![
+                    Hir::literal(hir::Literal::Unicode('a')),
+                    Hir::literal(hir::Literal::Unicode('b')),
+                ])),
+            }),
+            Hir::literal(hir::Literal::Unicode('y')),
+        ]);
+        assert_eq!(r"x(?:ab)+y", expr.to_string());
+    }
+
+    // Just like regression_repetition_concat, but with the repetition using
+    // an alternation as a child expression instead.
+    //
+    // See: https://github.com/rust-lang/regex/issues/731
+    #[test]
+    fn regression_repetition_alternation() {
+        let expr = Hir::concat(alloc::vec![
+            Hir::literal(hir::Literal::Unicode('x')),
+            Hir::repetition(hir::Repetition {
+                min: 1,
+                max: None,
+                greedy: true,
+                hir: Box::new(Hir::alternation(alloc::vec![
+                    Hir::literal(hir::Literal::Unicode('a')),
+                    Hir::literal(hir::Literal::Unicode('b')),
+                ])),
+            }),
+            Hir::literal(hir::Literal::Unicode('y')),
+        ]);
+        assert_eq!(r"x(?:a|b)+y", expr.to_string());
+    }
+
+    // This regression test is very similar in flavor to
+    // regression_repetition_concat in that the root of the issue lies in a
+    // peculiarity of how the HIR is represented and how the printer writes it
+    // out. Like the other regression, this one is also rooted in the fact that
+    // you can't produce the peculiar HIR from the concrete syntax. Namely, you
+    // just can't have a 'concat(a, alt(b, c))' because the 'alt' will normally
+    // be in (at least) a non-capturing group. Why? Because the '|' has very
+    // low precedence (lower that concatenation), and so something like 'ab|c'
+    // is actually 'alt(ab, c)'.
+    //
+    // See: https://github.com/rust-lang/regex/issues/516
+    #[test]
+    fn regression_alternation_concat() {
+        let expr = Hir::concat(alloc::vec![
+            Hir::literal(hir::Literal::Unicode('a')),
+            Hir::alternation(alloc::vec![
+                Hir::literal(hir::Literal::Unicode('b')),
+                Hir::literal(hir::Literal::Unicode('c')),
+            ]),
+        ]);
+        assert_eq!(r"a(?:b|c)", expr.to_string());
+    }
 }