From 534b0c283b7adcd22886f59f4c2a63b1ea658ffd Mon Sep 17 00:00:00 2001
From: Cosimo Lupo <clupo@google.com>
Date: Thu, 22 Aug 2024 15:45:46 +0100
Subject: [PATCH] use a simpler approach to cycle detection to match
 googlefonts/fontc#909

https://github.com/googlefonts/fontc/pull/909
---
 .../transformations/propagate_anchors.py      | 78 +++++++++----------
 .../transformations/propagate_anchors_test.py |  4 +-
 2 files changed, 39 insertions(+), 43 deletions(-)

diff --git a/Lib/glyphsLib/builder/transformations/propagate_anchors.py b/Lib/glyphsLib/builder/transformations/propagate_anchors.py
index b4359a978..e4f2d492b 100644
--- a/Lib/glyphsLib/builder/transformations/propagate_anchors.py
+++ b/Lib/glyphsLib/builder/transformations/propagate_anchors.py
@@ -14,6 +14,7 @@
 from __future__ import annotations
 
 import logging
+from collections import deque
 from itertools import chain
 from math import atan2, degrees, isinf
 from typing import TYPE_CHECKING
@@ -404,6 +405,7 @@ def get_component_layer_anchors(
 
 
 def compute_max_component_depths(glyphs: dict[str, GSGlyph]) -> dict[str, float]:
+    queue = deque()
     # Returns a map of the maximum component depth of each glyph.
     # - a glyph with no components has depth 0,
     # - a glyph with a component has depth 1,
@@ -412,53 +414,47 @@ def compute_max_component_depths(glyphs: dict[str, GSGlyph]) -> dict[str, float]
     #   technically a source error
     depths = {}
 
-    def component_names(glyph):
-        return {
+    # for cycle detection; anytime a glyph is waiting for components (and so is
+    # pushed to the back of the queue) we record its name and the length of the queue.
+    # If we process the same glyph twice without the queue having gotten smaller
+    # (meaning we have gone through everything in the queue) that means we aren't
+    # making progress, and have a cycle.
+    waiting_for_components = {}
+
+    for name, glyph in glyphs.items():
+        if _has_components(glyph):
+            queue.append(glyph)
+        else:
+            depths[name] = 0
+
+    while queue:
+        next_glyph = queue.popleft()
+        comp_names = {
             comp.name
             for comp in chain.from_iterable(
-                l.components for l in _interesting_layers(glyph)
+                l.components for l in _interesting_layers(next_glyph)
             )
             if comp.name in glyphs  # ignore missing components
         }
-
-    # we depth-first traverse the component trees so we can detect cycles as they
-    # happen, but we do it iteratively with an explicit stack to avoid recursion
-    for name, glyph in glyphs.items():
-        if name in depths:
-            continue
-        stack = [(glyph, False)]
-        # set to track the currently visiting glyphs for cycle detection
-        visiting = set()
-        while stack:
-            glyph, is_backtracking = stack.pop()
-            if is_backtracking:
-                # All dependencies have been processed: calculate depth and remove
-                # from the visiting set
-                depths[glyph.name] = (
-                    max((depths[c] for c in component_names(glyph)), default=-1) + 1
-                )
-                visiting.remove(glyph.name)
+        if all(comp in depths for comp in comp_names):
+            depths[next_glyph.name] = (
+                max((depths[c] for c in comp_names), default=-1) + 1
+            )
+            waiting_for_components.pop(next_glyph.name, None)
+        else:
+            # else push to the back to try again after we've done the rest
+            # (including the currently missing components)
+            last_queue_len = waiting_for_components.get(next_glyph.name)
+            waiting_for_components[next_glyph.name] = len(queue)
+            if last_queue_len != len(queue):
+                logger.debug("glyph '%s' is waiting for components", next_glyph.name)
+                queue.append(next_glyph)
             else:
-                if glyph.name in depths:
-                    # Already visited and processed
-                    continue
-
-                if glyph.name in visiting:
-                    # Already visiting? It's a cycle!
-                    logger.warning("Cycle detected in composite glyph '%s'", glyph.name)
-                    depths[glyph.name] = float("inf")
-                    continue
-
-                # Neither visited nor visiting: mark as visiting and re-add to the
-                # stack so it will get processed _after_ its components
-                # (is_backtracking == True)
-                visiting.add(glyph.name)
-                stack.append((glyph, True))
-                # Add all its components (if any) to the stack
-                for comp_name in component_names(glyph):
-                    if comp_name not in depths:
-                        stack.append((glyphs[comp_name], False))
-        assert not visiting
+                depths[next_glyph.name] = float("inf")
+                waiting_for_components.pop(next_glyph.name, None)
+                logger.warning("glyph '%s' has cyclical components", next_glyph.name)
+
+    assert not waiting_for_components
     assert len(depths) == len(glyphs)
 
     return depths
diff --git a/tests/builder/transformations/propagate_anchors_test.py b/tests/builder/transformations/propagate_anchors_test.py
index 06486a5b8..20f70c45a 100644
--- a/tests/builder/transformations/propagate_anchors_test.py
+++ b/tests/builder/transformations/propagate_anchors_test.py
@@ -490,9 +490,9 @@ def test_propagate_across_layers_with_circular_reference(caplog):
 
     assert len(caplog.records) == 2
     assert (
-        caplog.records[0].message == "Cycle detected in composite glyph 'acutecomb.alt'"
+        caplog.records[0].message == "glyph 'acutecomb.alt' has cyclical components"
     )
-    assert caplog.records[1].message == "Cycle detected in composite glyph 'gravecomb'"
+    assert caplog.records[1].message == "glyph 'gravecomb' has cyclical components"
 
 
 def test_remove_exit_anchor_on_component():