Check for repeat expressions that infinite-loop without consuming input

Fixes #210

peg-macros/analysis.rs

@@ -6,6 +6,7 @@ use crate::ast::*;
 pub struct GrammarAnalysis<'a> {
     pub rules: HashMap<String, &'a Rule>,
     pub left_recursion: Vec<LeftRecursionError>,
+    pub loop_nullability: Vec<LoopNullabilityError>,
 }
 
 pub fn check<'a>(grammar: &'a Grammar) -> GrammarAnalysis<'a> {
@@ -16,11 +17,13 @@ pub fn check<'a>(grammar: &'a Grammar) -> GrammarAnalysis<'a> {
         rules.entry(rule.name.to_string()).or_insert(rule);
     }
 
-    let (_, left_recursion) = LeftRecursionVisitor::check(grammar, &rules);
+    let (rule_nullability, left_recursion) = LeftRecursionVisitor::check(grammar, &rules);
+    let loop_nullability = LoopNullabilityVisitor::check(grammar, &rule_nullability);
 
     GrammarAnalysis {
         rules,
         left_recursion,
+        loop_nullability,
     }
 }
 
@@ -74,11 +77,12 @@ impl<'a> LeftRecursionVisitor<'a> {
         res
     }
 
-    /// Walk the prefix of an expression that can be reached without consuming input.
-    /// 
-    /// Returns true if the rule is known to match completely without consuming any input.
-    /// This is a conservative heuristic, if unknown, we return false to avoid reporting false-positives
-    /// for left recursion.
+    /// Walk the prefix of an expression that can be reached without consuming
+    /// input.
+    ///
+    /// Returns true if the rule is known to match completely without consuming
+    /// any input. This is a conservative heuristic, if unknown, we return false
+    /// to avoid reporting false-positives for left recursion.
     fn walk_expr(&mut self, this_expr: &SpannedExpr) -> bool {
         use self::Expr::*;
         match this_expr.expr {
@@ -162,3 +166,110 @@ impl<'a> LeftRecursionVisitor<'a> {
         }
     }
 }
+
+/// Check for loops whose body can succeed without consuming any input, which
+/// will loop infinitely.
+struct LoopNullabilityVisitor<'a> {
+    rule_nullability: &'a HashMap<String, bool>,
+    errors: Vec<LoopNullabilityError>,
+}
+
+pub struct LoopNullabilityError {
+    pub span: Span,
+}
+
+impl LoopNullabilityError {
+    pub fn msg(&self) -> String {
+        format!("loops infinitely because loop body can match without consuming input")
+    }
+}
+
+
+impl<'a> LoopNullabilityVisitor<'a> {
+    fn check(grammar: &'a Grammar, rule_nullability: &HashMap<String, bool>) -> Vec<LoopNullabilityError> {
+        let mut visitor = LoopNullabilityVisitor {
+            rule_nullability,
+            errors: Vec::new(),
+        };
+
+        for rule in grammar.iter_rules() {
+            visitor.walk_expr(&rule.expr);
+        }
+
+        visitor.errors
+    }
+
+
+    /// Walk an expr and its children analyzing the nullability of loop bodies.
+    ///
+    /// Returns true if the rule is known to match completely without consuming
+    /// any input. This is a conservative heuristic; if unknown, we return false
+    /// to avoid reporting false-positives.
+    ///
+    /// This is very similar to LeftRecursionVisitor::walk_expr, but walks the
+    /// entire expression tree rather than just the nullable prefix, and doesn't
+    /// recurse into calls.
+    fn walk_expr(&mut self, this_expr: &SpannedExpr) -> bool {
+        use self::Expr::*;
+        match this_expr.expr {
+            RuleExpr(ref rule_ident, _) => {
+                let name = rule_ident.to_string();
+                *self.rule_nullability.get(&name).unwrap_or(&false)
+            }
+
+            ActionExpr(ref elems, ..) => {
+                let mut nullable = true;
+                for elem in elems {
+                    nullable &= self.walk_expr(&elem.expr);
+                }
+                nullable
+            }
+
+            ChoiceExpr(ref choices) => {
+                let mut nullable = false;
+                for expr in choices {
+                    nullable |= self.walk_expr(expr);
+                }
+                nullable
+            }
+
+            OptionalExpr(ref expr) | PosAssertExpr(ref expr) | NegAssertExpr(ref expr) => {
+                self.walk_expr(expr);
+                true
+            }
+
+            Repeat { ref inner, ref bound, ref sep } => {
+                let inner_nullable = self.walk_expr(inner);
+                let sep_nullable = sep.as_ref().map_or(true, |sep| self.walk_expr(sep));
+
+                // The entire purpose of this analysis: report errors if the loop body is nullable
+                if inner_nullable && sep_nullable && !bound.has_upper_bound() {
+                    self.errors.push(LoopNullabilityError { span: this_expr.span });
+                }
+
+                inner_nullable | !bound.has_lower_bound()
+            }
+
+            MatchStrExpr(ref expr) | QuietExpr(ref expr) => self.walk_expr(expr),
+
+            PrecedenceExpr { ref levels } => {
+                let mut nullable = false;
+
+                for level in levels {
+                    for operator in &level.operators {
+                        let mut operator_nullable = true;
+                        for element in &operator.elements {
+                            operator_nullable &= self.walk_expr(&element.expr);
+                        }
+                        nullable |= operator_nullable;
+                    }
+                }
+
+                nullable 
+            }
+
+            LiteralExpr(_) | PatternExpr(_) | MethodExpr(_, _) | FailExpr(_) | MarkerExpr(_) => false,
+            PositionExpr => true,
+        }
+    }
+}

peg-macros/ast.rs

@@ -122,4 +122,11 @@ impl BoundedRepeat {
             BoundedRepeat::Plus | BoundedRepeat::Exact(_) | BoundedRepeat::Both(Some(_), _) => true
         }
     }
+
+    pub fn has_upper_bound(&self) -> bool {
+        match self {
+            BoundedRepeat::None |  BoundedRepeat::Plus | BoundedRepeat::Both(_, None) => false,
+            BoundedRepeat::Exact(_) | BoundedRepeat::Both(_, Some(_)) => true
+        }
+    }
 }

peg-macros/translate.rs

@@ -115,6 +115,10 @@ pub(crate) fn compile_grammar(grammar: &Grammar) -> TokenStream {
         errors.push(report_error(rec.span, rec.msg()));
     }
 
+    for rec in &analysis.loop_nullability {
+        errors.push(report_error(rec.span, rec.msg()));
+    }
+
     quote_spanned! { Span::mixed_site() =>
         #doc
         #visibility mod #name {

tests/compile-fail/nullable_loop.rs

@@ -0,0 +1,13 @@
+peg::parser!(grammar e() for str {
+    rule nested() = ("a"*)* //~ ERROR
+
+    rule nested_ok() = ("a"+)*
+
+    rule nullable() = "x"?
+
+    rule call() = "foo" nullable()* //~ ERROR
+
+    rule more_complex() = ("x" / "a"? "b"?)*<2,> //~ ERROR
+});
+
+fn main() {}

tests/compile-fail/nullable_loop.stderr

@@ -0,0 +1,17 @@
+error: loops infinitely because loop body can match without consuming input
+ --> $DIR/nullable_loop.rs:2:27
+  |
+2 |     rule nested() = ("a"*)* //~ ERROR
+  |                           ^
+
+error: loops infinitely because loop body can match without consuming input
+ --> $DIR/nullable_loop.rs:8:35
+  |
+8 |     rule call() = "foo" nullable()* //~ ERROR
+  |                                   ^
+
+error: loops infinitely because loop body can match without consuming input
+  --> $DIR/nullable_loop.rs:10:44
+   |
+10 |     rule more_complex() = ("x" / "a"? "b"?)*<2,> //~ ERROR
+   |                                            ^