Merge pull request #18 from LCSB-BioCore/mk-stricter-checking

enforce the grammar checking for ε-matches in invalid positions

Author: exaexa

Project.toml

@@ -1,7 +1,7 @@
 name = "PikaParser"
 uuid = "3bbf5609-3e7b-44cd-8549-7c69f321e792"
 authors = ["The developers of PikaParser.jl"]
-version = "0.5.2"
+version = "0.6.0"
 
 [deps]
 DocStringExtensions = "ffbed154-4ef7-542d-bbb7-c09d3a79fcae"

src/clauses.jl

@@ -102,12 +102,18 @@ can_match_epsilon(x::Union{Epsilon,EndOfInput}, ::Vector{Bool}) = true
 can_match_epsilon(x::Seq, ch::Vector{Bool}) = all(ch)
 can_match_epsilon(x::First, ch::Vector{Bool}) =
     isempty(ch) ? false :
-    any(ch[begin:end-1]) ? error("First with non-terminal epsilon match") : last(ch)
+    any(ch[begin:end-1]) ?
+    error(
+        "Subclauses of `First` must never allow an epsilon match other than the last one.",
+    ) : last(ch)
 can_match_epsilon(x::NotFollowedBy, ch::Vector{Bool}) =
-    ch[1] ? error("NotFollowedBy epsilon match") : true
+    ch[1] ? error("Contents of `NotFollowedBy` clause must never allow an epsilon match.") :
+    true
 can_match_epsilon(x::FollowedBy, ch::Vector{Bool}) = ch[1]
-can_match_epsilon(x::Some, ch::Vector{Bool}) = ch[1]
-can_match_epsilon(x::Many, ch::Vector{Bool}) = true
+can_match_epsilon(x::Some, ch::Vector{Bool}) =
+    ch[1] ? error("Contents of `Some` clause must never allow an epsilon match.") : false
+can_match_epsilon(x::Many, ch::Vector{Bool}) =
+    ch[1] ? error("Contents of `Many` clause must never allow an epsilon match.") : true
 can_match_epsilon(x::Tie, ch::Vector{Bool}) = ch[1]
 
 

src/grammar.jl

@@ -72,11 +72,14 @@ function make_grammar(
 
     while !isempty(q)
         cur = pop!(q)
-        emptiable[cur] && continue
-        emptiable[cur] =
+        # Here we could skip the check if emptiable[cur] is already True, but
+        # redoing it actually allows the `can_match_epsilon` implementations to
+        # fail in case the grammar is somehow invalid.
+        cur_is_emptiable =
             can_match_epsilon(clauses[cur], emptiable[child_clauses(clauses[cur])])
-        if emptiable[cur]
-            # there was a flip!
+        if !emptiable[cur] && cur_is_emptiable
+            # There was a flip, force rechecking.
+            emptiable[cur] = cur_is_emptiable
             for pid in parent_clauses[cur]
                 push!(q, pid)
             end

src/parse.jl

@@ -21,9 +21,15 @@ function lookup_best_match_id!(
             return match_epsilon!(cls, clause, pos, st)
         elseif cls isa Many{Int,T}
             return match_epsilon!(cls, clause, pos, st)
-        else
-            # This is reached rarely in corner cases
+        elseif cls isa Seq{Int,T}
+            return match_epsilon!(cls, clause, pos, st)
+        elseif cls isa First{Int,T}
+            return match_epsilon!(cls, clause, pos, st)
+        elseif cls isa Tie{Int,T}
             return match_epsilon!(cls, clause, pos, st)
+        else
+            # This should not be reached.
+            return match_epsilon!(cls, clause, pos, st) # COV_EXCL_LINE
         end
     end
 
@@ -48,6 +54,7 @@ function new_match!(match::Match, st::ParserState)
     end
 
     for seed in st.grammar.seed_clauses[match.clause]
+        # TODO: can we remove the extra epsilon condition in the next line?
         if updated || st.grammar.can_match_epsilon[seed]
             push!(st.q, seed)
         end
@@ -189,8 +196,10 @@ function parse(
             elseif cls isa Tie{Int,T}
                 match_clause!(cls, clause, i, st)
             else
-                # Fallback (execution shouldn't reach here)
-                match_clause!(cls, clause, i, st)
+                # Fallback (execution shouldn't reach here). You might want to
+                # add a warning statement here if you added new clauses and
+                # hunt for performance issues.
+                match_clause!(cls, clause, i, st) # COV_EXCL_LINE
             end
             # Shame ends here.
         end

test/clauses.jl

@@ -114,4 +114,20 @@ end
     @test P.find_match_at!(p, :x, firstindex(str)) == 0
     @test P.find_match_at!(p, :x, lastindex(str)) == 0
     @test P.find_match_at!(p, :x, nextind(str, lastindex(str))) != 0
+
+    rules = Dict(:x => P.seq(P.epsilon, :y => P.first(P.token('a'), P.epsilon)))
+    p = P.parse(P.make_grammar([:x, :y], P.flatten(rules, Char)), "b")
+
+    @test P.find_match_at!(p, :x, firstindex(p.input)) != 0
+    @test P.find_match_at!(p, :y, firstindex(p.input)) != 0
+end
+
+@testset "Invalid epsilon matches are avoided" begin
+    rules = Dict(:x => P.not_followed_by(P.epsilon))
+    @test_throws ErrorException P.make_grammar([:x], P.flatten(rules, Char))
+
+    # thanks go to @CptWesley for bringing this up in
+    # https://github.com/lukehutch/pikaparser/issues/35
+    rules = Dict(:x => P.first(P.token('a'), P.epsilon), :y => P.first(P.seq(:y, :x), :x))
+    @test_throws ErrorException P.make_grammar([:y], P.flatten(rules, Char))
 end