Detect phi cycles in PhiMatcher::Find and PhiMatcher::Final.

openfst/lib/matcher.h

@@ -613,8 +613,9 @@ enum MatcherRewriteMode {
 // underlying matching. By default, the underlying matcher is
 // constructed by PhiMatcher. The user can instead pass in this
 // object; in that case, PhiMatcher takes its ownership.
-// Phi non-determinism not supported. No non-consuming symbols other
-// than epsilon supported with the underlying template argument matcher.
+// Phi non-determinism and non-trivial phi cycles are not supported. No
+// non-consuming symbols other than epsilon supported with the underlying
+// template argument matcher.
 template <class M>
 class PhiMatcher : public MatcherBase<typename M::Arc> {
  public:
@@ -720,13 +721,31 @@ class PhiMatcher : public MatcherBase<typename M::Arc> {
     }
     weight = Weight::One();
     matcher_->SetState(s);
+    // For cycle detection uses Brent's O(1)-space algorithm. See:
+    //
+    // Brent, R. P. 1980. An improved Monte Carlo factorization algorithm.
+    // BIT Numerical Mathematics 20(2): 176-184.
+    StateId checkpoint = s;
+    size_t power = 1;
+    size_t length = 0;
     while (matcher_->Final(s) == Weight::Zero()) {
       if (!matcher_->Find(phi_label_ == 0 ? -1 : phi_label_)) break;
       weight = Times(weight, matcher_->Value().weight);
-      if (s == matcher_->Value().nextstate) {
-        return Weight::Zero();  // Does not follow phi self-loops.
+      const auto nextstate = matcher_->Value().nextstate;
+      // Does not follow phi self-loops.
+      if (s == nextstate) return Weight::Zero();
+      if (nextstate == checkpoint) {
+        FSTERROR() << "PhiMatcher: Phi cycle detected";
+        error_ = true;
+        return Weight::Zero();
+      }
+      ++length;
+      if (length == power) {
+        checkpoint = nextstate;
+        power *= 2;
+        length = 0;
       }
-      s = matcher_->Value().nextstate;
+      s = nextstate;
       matcher_->SetState(s);
     }
     weight = Times(weight, matcher_->Final(s));
@@ -768,7 +787,7 @@ class PhiMatcher : public MatcherBase<typename M::Arc> {
   Weight phi_weight_;     // Product of the weights of phi transitions taken.
   bool phi_loop_;         // When true, phi self-loop are allowed and treated
                           // as rho (required for Aho-Corasick).
-  bool error_;            // Error encountered?
+  mutable bool error_;    // Error encountered?
 
   PhiMatcher& operator=(const PhiMatcher&) = delete;
 };
@@ -801,21 +820,43 @@ inline bool PhiMatcher<M>::Find(Label label) {
     return matcher_->Find(label);
   }
   auto s = state_;
+  // For cycle detection uses Brent's O(1)-space algorithm. See:
+  //
+  // Brent, R. P. 1980. An improved Monte Carlo factorization algorithm.
+  // BIT Numerical Mathematics 20(2): 176-184.
+  StateId checkpoint = s;
+  size_t power = 1;
+  size_t length = 0;
   while (!matcher_->Find(label)) {
     // Look for phi transition (if phi_label_ == 0, we need to look
     // for -1 to avoid getting the virtual self-loop)
     if (!matcher_->Find(phi_label_ == 0 ? -1 : phi_label_)) return false;
-    if (phi_loop_ && matcher_->Value().nextstate == s) {
-      phi_match_ = label;
-      return true;
+    const auto nextstate = matcher_->Value().nextstate;
+    if (nextstate == s) {
+      if (phi_loop_) {
+        phi_match_ = label;
+        return true;
+      }
+      return false;  // Does not follow phi self-loops.
     }
     phi_weight_ = Times(phi_weight_, matcher_->Value().weight);
-    s = matcher_->Value().nextstate;
     matcher_->Next();
     if (!matcher_->Done()) {
       FSTERROR() << "PhiMatcher: Phi non-determinism not supported";
       error_ = true;
     }
+    if (nextstate == checkpoint) {
+      FSTERROR() << "PhiMatcher: Phi cycle detected";
+      error_ = true;
+      return false;
+    }
+    ++length;
+    if (length == power) {
+      checkpoint = nextstate;
+      power *= 2;
+      length = 0;
+    }
+    s = nextstate;
     matcher_->SetState(s);
   }
   return true;

openfst/test/BUILD.bazel

@@ -1485,6 +1485,9 @@ cc_test(
         ":test_main",
         "//openfst/compat:file_path",
         "//openfst/lib",
+        "//openfst/lib:util",
+        "@com_google_absl//absl/flags:flag",
+        "@com_google_absl//absl/flags:reflection",
         "@com_google_googletest//:gtest",
     ],
 )

openfst/test/matcher_test.cc

@@ -26,14 +26,18 @@
 
 #include "openfst/compat/file_path.h"
 #include "gtest/gtest.h"
+#include "absl/flags/flag.h"
+#include "absl/flags/reflection.h"
 #include "openfst/lib/arc.h"
 #include "openfst/lib/arcsort.h"
 #include "openfst/lib/cache.h"
 #include "openfst/lib/compose-filter.h"
 #include "openfst/lib/compose.h"
 #include "openfst/lib/equal.h"
 #include "openfst/lib/fst.h"
+#include "openfst/lib/properties.h"
 #include "openfst/lib/relabel.h"
+#include "openfst/lib/util.h"
 #include "openfst/lib/vector-fst.h"
 #include "openfst/lib/verify.h"
 
@@ -183,6 +187,9 @@ class MatcherTest : public testing::Test {
   std::unique_ptr<const VectorFst<Arc>> lm_a_fst_;
   std::unique_ptr<const VectorFst<Arc>> lm_bb_fst_;
   std::unique_ptr<const VectorFst<Arc>> lm_a_bb_fst_;
+
+ private:
+  const absl::FlagSaver fs_;
 };
 
 TEST_F(MatcherTest, ComposeWithRhoMatcher) {
@@ -409,6 +416,83 @@ TEST_F(MatcherTest, PhiMatcherFinal) {
   }
 }
 
+TEST_F(MatcherTest, PhiMatcherCycle) {
+  absl::SetFlag(&FLAGS_fst_error_fatal, false);
+
+  // States 0 and 1 form a 2-state phi cycle (0 -phi-> 1 -phi-> 0); state 1
+  // also has a matching transition on label 1 to final state 2.
+  StdVectorFst cyclic_fst;
+  const auto s0 = cyclic_fst.AddState();
+  const auto s1 = cyclic_fst.AddState();
+  const auto s2 = cyclic_fst.AddState();
+  cyclic_fst.SetStart(s0);
+  cyclic_fst.AddArc(s0, Arc(kSpecialLabel, kSpecialLabel, Weight(0.5), s1));
+  cyclic_fst.AddArc(s1, Arc(kSpecialLabel, kSpecialLabel, Weight(0.25), s0));
+  cyclic_fst.AddArc(s1, Arc(1, 1, Weight(1.0), s2));
+  cyclic_fst.SetFinal(s2, Weight::One());
+  ArcSort(&cyclic_fst, StdILabelCompare());
+
+  // Matching label 1 from state 0 succeeds after one phi step without hitting
+  // the cycle.
+  {
+    TestPhiMatcher matcher(cyclic_fst, MATCH_INPUT, kSpecialLabel);
+    matcher.SetState(s0);
+    ASSERT_TRUE(matcher.Find(1));
+    EXPECT_EQ(matcher.Value().nextstate, s2);
+    EXPECT_EQ(matcher.Value().weight, Weight(1.5));
+    EXPECT_EQ(matcher.Properties(0) & kError, 0);
+  }
+
+  // Searching for an unmatched label (2) detects the 0 -> 1 -> 0 phi cycle,
+  // returns false, and sets kError.
+  {
+    TestPhiMatcher matcher(cyclic_fst, MATCH_INPUT, kSpecialLabel);
+    matcher.SetState(s0);
+    EXPECT_FALSE(matcher.Find(2));
+    EXPECT_EQ(matcher.Properties(0) & kError, kError);
+  }
+
+  // Final(s0) when neither s0 nor s1 is final detects the 0 -> 1 -> 0 phi
+  // cycle, returns Zero(), and sets kError.
+  {
+    TestPhiMatcher matcher(cyclic_fst, MATCH_INPUT, kSpecialLabel);
+    EXPECT_EQ(matcher.Final(s0), Weight::Zero());
+    EXPECT_EQ(matcher.Properties(0) & kError, kError);
+  }
+
+  // Composing with an FST that triggers the phi cycle terminates and sets
+  // kError on the result.
+  {
+    StdVectorFst query;
+    const auto q0 = query.AddState();
+    const auto q1 = query.AddState();
+    query.SetStart(q0);
+    query.AddArc(q0, Arc(2, 2, Weight::One(), q1));
+    query.SetFinal(q1, Weight::One());
+
+    ComposeFst<Arc> cfst(
+        query, cyclic_fst,
+        ComposeFstOptions<Arc, TestPhiMatcher>(
+            CacheOptions(), new TestPhiMatcher(query, MATCH_NONE, kNoLabel),
+            new TestPhiMatcher(cyclic_fst, MATCH_INPUT, kSpecialLabel)));
+    StdVectorFst composed(cfst);
+    EXPECT_TRUE(composed.Properties(kError, false));
+  }
+
+  // A phi self-loop with phi_loop=false does not loop forever in Find().
+  {
+    StdVectorFst self_loop_fst;
+    const auto s = self_loop_fst.AddState();
+    self_loop_fst.SetStart(s);
+    self_loop_fst.AddArc(s, Arc(kSpecialLabel, kSpecialLabel, Weight(0.5), s));
+    TestPhiMatcher matcher(self_loop_fst, MATCH_INPUT, kSpecialLabel,
+                           /*phi_loop=*/false);
+    matcher.SetState(s);
+    EXPECT_FALSE(matcher.Find(2));
+    EXPECT_EQ(matcher.Properties(0) & kError, 0);
+  }
+}
+
 TEST_F(MatcherTest, SortedMatcher) {
   SortedMatcher<VectorFst<Arc>> sorted_matcher(*mfst2_, MATCH_INPUT);
   sorted_matcher.SetState(0);