Fix a few issues with infinite recursions if the content contains cyclic curation

[d-ronnqvist]

Bug/issue #, if applicable: rdar://126974763

Summary

This fixes a few different issues with infinite recursion, which ultimately surface as crashes, when the content contains cyclic curation.

There were two main issues:

• Multi source-language projects can have the same symbol in different places in the different language representation which are all mixed in the topic graph. These multiple paths to and from symbol in the hierarchy makes it possible to create cycles through a mix of automatic and manual curation.
• Even if the topic graph didn't contain curation, the navigator is based on the rendered topic sections which don't filter out any curation.

The first issue surfaced as an infinite recursion in DocumentationContext/pathsTo(...) which incorrectly assumed that the topic graph was acyclic. Ironically the infinite recursion occurred when DocC tried to identify and skip curation that would cause a cycle.

After fixing the pathsTo(...) implementation to make the distinction between finite and infinite parts in the graph, I noticed that a handful of the callers didn't need the the full paths and that there were faster/cheaper ways to compute the information that they needed.

The second issue surfaced as in infinite recursion in NavigatorTree/Node/copy() which had a faulty implementation that tried to identify and prevent infinite recursion. The issue with that implementation was that it checked NavigatorTree/Node/parent which didn't always match the node from the previous iteration of the recursion when the page that the node represents was curated in multiple places. This made the crash somewhat unreliable, but by adding multiple nodes in the path that are all curated in more than one place, the crash reproduces most of the time.

I tried making a more robust navigator builder implementation that worked in 2 steps:

• Identify and break all the cycles in the navigator
• Build up the navigator hierarchy bottom up (from the leaves to the root)

This does produce a nicer looking navigator hierarchy when there are cyclic curation (both implementations produce stable navigator hierarchies across different executions) but it takes ~2-3x times as long to create the navigator hierarchy this way which increases the total documentation build time by ~2-3%. Since DocC already raises warnings about the cyclic curation, it didn't seem worth it to produce an "optimal" minimal navigation hierarchy when the there's cyclic curation, so I didn't push this commit.

Dependencies

None

Testing

The best way to test both of these issues are described by the added unit tests.

To test the first issue:

• In a project that's configured for both Swift and Objective-C, define an error with NS_ERROR_ENUM and any class

  extern NSErrorDomain const SomeErrorDomain;
  typedef NS_ERROR_ENUM(SomeErrorDomain, SomeErrorCode) {
      SomeErrorSomething = 1
  };
  
  @interface SomeClass: NSObject
  
  @end

• Add two documentation extension files,

  • One for the class to curate the generated Swift only error structure:

    # ``SomeClass``
    
    Curate the error
    
    ## Topics
    
    - ``SomeError``

  • One for the error to curate the class:

    # ``SomeError/Code``
    
    Curate the class
    
    ## Topics
    
    - ``SomeClass``

• Building documentation for this project should no longer cause a crash due to infinite recursion determining the paths to `SomeError/Code``.

To test the second issue:

• In a Swift only project, create two public classes; one with two members and one empty.

• Curate most symbols in two places:

  /// ## Topics
  /// - ``SomeClass/second()``
  public class Empty {}
  
  /// ## Topics
  /// - ``first()``
  /// - ``Empty``
  public class SomeClass {
      /// ## Topics
      /// - ``second()``
      public find first() {}
      /// ## Topics
      /// - ``first()``
      public find second() {}
  }

• Add an API collection (an artile with curation) and curate both classes:

  # My API collection
  
  ## Topics
  - ``SomeClass``
  - ``Empty``

• Add an documentation extension for the module and curate the API collection:

  # ``ModuleName``
  
  ## Topics
  - <doc:my-api-collection>

• Building documentation for this project should no longer cause a crash due to infinite recursion copying the navigation tree nodes. Even with this many mulyi-curated symbols you may need to build documentation a few times to reproduce the crash.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

• Added tests
• Ran the ./bin/test script and it succeeded
• Updated documentation if necessary Not applicable

[d-ronnqvist]

@swift-ci please test

[patshaughnessy]

Amazing work 👏 👏

It was a bit difficult understanding all of the new algorithms that you introduced at first, but it made sense after a while and was a real pleasure reading the code. Very clean, well tested and well documented.

My only suggestions really are to add a few more comments, move a couple things around and rename a few things... just to make it a bit easier to understand what a directed graph is at the call sites where we will now use it.

One algorithm question: I don't see where you use the cycle information. The functions in DirectedGraph+Cycles.swift don't appear to be called outside of the unit tests. Maybe what you mean by the word "finite" (e.g. finitePaths(to:, options:)) are paths that don't contain cycles? Maybe a comment there would help also. If the DirectedGraph by its very nature will never return cycles then we should point that out. But maybe I'm missing something?

[patshaughnessy]

Thanks for naming this tuple to simplify all the type annotations below...

[patshaughnessy]

Should checks like this be an assertion instead? If we have a parent reference which does not exist in the topic graph, is that a programming error? Or a valid state somehow?

[d-ronnqvist]

Possibly... 🤔

I believe that we expect that every symbol reference known to the PathHierarchyBasedLinkResolver exist in the context and that every symbol reference in the context also exist in the topic graph.

[patshaughnessy]

Same here - why would we have a parent reference to a node that is not in the topic graph?

[patshaughnessy]

Why call this neighbors and not edges?

[d-ronnqvist]

You're right. The parameter name should be called "edges".

In earlier iterations of this implementation, I used a (Node) -> [Node] closure for _neighbors so that I could create a directed graph from classes that describe the graph using direct parent and children pointers (like NavigatorTree/Node). Later, I didn't need that anymore so I changed the implementation to a dictionary which changed the semantic of the parameter:

• a "neighbor" is a node that's one step away from a given node (also called an "adjacent node").
• an "edge" is the connection between the current node and one of its neighbors.

Since the parameter is a data representation of the connections between nodes, it is semantically the edges of the graph.

[patshaughnessy]

Since the graph is a struct, does this mean that this line will copy the entire graph? Do we need to worry about performance here?

[d-ronnqvist]

Yes and no.

The graph is a struct so each new value is a copy. The graph only has one stored property which it stores inline. That property is a dictionary, which is also a struct in Swift.

However, dictionaries in Swift have copy-on-write semantics which means that two separate "copies" of the same dictionary can share the underlying data until either one of them modifies it. The graph doesn't expose any API (even internal) to modify the dictionary, so the only way that we'd make a copy of the dictionary data is if the caller modified the original dictionary that the graph was created from while the graph struct is still in scope. Even then, the iterator would be able to share its copy with the graph, so we'd only have two copies---the callers modified value and the value that the graph was created with---the iterator would be able to share its data with the graph.

Because of this, creating a new graph or making copies of it is basically a reference counting operation of the underlying dictionary storage (O(1)). Currently there aren't any callers that modify the original edges while operating on the directed graph created from those edges, so there shouldn't be any places that makes real copies of the "edges" data.

[patshaughnessy]

Suggested change

	DirectedGraph(neighbors: context.topicGraph.reverseEdges)
	context.topicGraph.reverseEdgesGraph

[d-ronnqvist]

Nice. I think that reads much better. I'll update all the call sites to access a computed property on the topic graph instead.

[patshaughnessy]

I see this pattern in a few places... map, drop first, reversed. Should we consider changing the way paths are returned by DirectedGraph? Or maybe create a utility function inside of DirectedGraph+Paths.swift that does this?

Understanding why we need this here without reading the entire implementation of DirectedGraph is a bit difficult.

Or maybe you just need one of your ascii art examples here in a commit to make it clear what this code expects from DirectedGraph.

[patshaughnessy]

Tangential: Is it really safe here to use try! ? I know this was existing code you didn't change here. But are we sure that the first element in a (or in a path that was reversed above) will always be a technology node? Might be worth an assertion in case we have a bug in DirectedGraph somewhere.

[d-ronnqvist]

But are we sure that the first element in a (or in a path that was reversed above) will always be a technology node?

The first element doesn't have to be a "technology" (tutorials table of contents page), it just needs to exist in the context. The only portion of this line that raises an error is entity(with: first). There's nothing the graph algorithm can do wrong to make it return a node that wasn't in the edges it was created with because the DirectedGraph/Node is just any Hashable, so the graph doesn't know how to create a new value.

This means that every element in every path is guaranteed to appear in topicGraph.reverseEdges.

Unless the topic graph node is virtual and doesn't represent a page, there should always exist a documentation node for that reference in the context.

In theory it's possible that there's either a bug in the graph traversal or in the topic graph reverse edges data so that the start of the path is a virtual node but that would be a really bad bug where it's probably better to crash.

[patshaughnessy]

Isn't there a Swift function enumerated() you could use here, maybe off by 1? Why the explicit call to zip with the range?

[d-ronnqvist]

The enumerated() function works very similar to this but it starts at 0. I could have used enumerated and then done let indexAfter = index + 1 in the loop body but that would introduce a slight risk of using index instead of indexAfter which is avoided by only having one index variable in the loop body.

[patshaughnessy]

Do we use firstCycle or cycles anywhere? I only see references to them in tests.

[d-ronnqvist]

Not anymore. It was the first code that I added and I used it a lot in early development of these fixes to find the cycles and to understand the bugs and creates small examples with the same cycle structure that reproduce these bugs.

I was also using it in the implementation of a way to create the smallest non-cyclic tree for the navigator but that was slower than the current implementation—and only created different navigator hierarchies when there were cycles—so I didn't commit it.

I view this API similar to some of the dump() functions that we have; a valuable tool during development and investigation of bugs, even if it's not using during normal program execution.

[d-ronnqvist]

Fun coincidence; when I was looking at other similar debugging API we have I noticed that TopicGraph.dump() doesn't handle cycles so I added a call to firstCycle(from:) there to exit with a more descriptive error message instead of recursing infinitely. That will hopefully help someone who uses it for debugging and encounters a cycle.

[mayaepps]

I had a few questions, but otherwise very clean and easy-to-read code, particularly the new algorithms you added.

[mayaepps]

Would 1,2,3 and 3,1 be considered the same cycle?

[d-ronnqvist]

No, those are different cycles because they require removing different edges to break each cycle.

I'll add some more documentation to describe when and why two cycles are considered the "same", both here and in the code.

---

It's only possible to enter cycles from different directions like that when the graph has more than one entry point to the cycle. For example:

                        0──────────┐
                        │          ▼
    ┏━━━━━━▶2           │  ┏━━━━━━▶2            ┏━━━━━━▶2◀──┐
    ┃       ┃           │  ┃       ┃            ┃       ┃   │
┌──▶1━━━┓   ┃           └─▶1━━━┓   ┃            1━━━┓   ┃   │
│   ▲   ▼   ┃              ▲   ▼   ┃            ▲   ▼   ┃   │
│   ┗━━━3◀━━┛              ┗━━━3◀━━┛            ┗━━━3◀━━┛   │
│       ▲                                           ▲       │
0───────┘                                           └───────0

In each of these cases:

• "1,3" and "3,1,2" are different cycles because we need to remove both the 3─▶1 edge and the 2─▶3 edge to break both cycles
• "1,3" and "2,3,1" are different cycles because we need to remove both the 3─▶1 edge and the 1─▶2 edge to break both cycles
• "3,1" and "2,3,1" are different cycles because we need to remove both the 1─▶3 edge and the 1─▶2 edge to break both cycles

                        0──────────┐
                        │          ▼
    ┏━━━━━━▶2           │  ┌ ─ ─ ─ 2            ┌ ─ ─ ─ 2◀──┐
    ┃                   │          ┃                    ┃   │
┌──▶1━━━┓   │           └─▶1━━━┓   ┃            1 ─ ┐   ┃   │
│       ▼                      ▼   ┃            ▲       ┃   │
│   └ ─ 3 ─ ┘              └ ─ 3◀━━┛            ┗━━━3◀━━┛   │
│       ▲                                           ▲       │
0───────┘                                           └───────0

When there's a single entry point to this cycle, we mostly get different edges like above:

┌ ─ ─ ─ 2◀──0       ┏━━━━━━▶2
        ┃           ┃        
1 ─ ┐   ┃           1 ─ ┐   │
▲       ┃           ▲        
┗━━━3◀━━┛           ┗━━━3 ─ ┘
                        ▲    
                        │    
                        0    

except when entering from the 1 node, where both the "1,3" cycle and the "1,2,3" cycle can be broken by removing the 3─▶1 edge:

    ┏━━━━━━▶2
    ┃       ┃
0──▶1━━━┓   ┃
        ▼   ┃
    └ ─ 3◀━━┛

[mayaepps]

Just a thought--would using a breadth-first GraphNodeIterator<(Node, Path)> make sense here?

[d-ronnqvist]

It wouldn't. One crucial implementation difference between this iterator and GraphNodeIterator is that this iterator will repeat visits to the same node but GraphNodeIterator will only visit each node the first time that it's encountered.

For example, consider this small graph

    ┌──▶2──┐
    │      ▼
0──▶1─────▶3

Using GraphNodeIterator the implementation would only visit 3 once, so it couldn't compute both paths: 0,1,3 and 0,1,2,3.

[mayaepps]

If we're following the ascii art diagram:

Suggested change

	4: [6,7],
	4: [5,6,7],

[mayaepps]

I think this should add the paths [0,2,4,5,8], [0,2,4,5,9,11], and [0,2,4,5,9,7,10]

[mayaepps]

Small nit: I noticed this shows up in QH as though it's one long list of nodes, maybe there's a way to differentiate them in some way?

[d-ronnqvist]

I see. I can write "and" between each of them instead.

[mayaepps]

As another data point, the name confused me in the same way at first.

[d-ronnqvist]

Maybe what you mean by the word "finite" (e.g. finitePaths(to:, options:)) are paths that don't contain cycles? Maybe a comment there would help also. If the DirectedGraph by its very nature will never return cycles then we should point that out. But maybe I'm missing something?

A path through a graph is either finite or infinite (meaning that it loops back on itself at some point). DirectedGraph.accumulatingPaths(from:) is implemented so that it detects cycles and stops before looping back on itself. I'll find some place to add a comment about this.

[d-ronnqvist]

@swift-ci please test