clone path hierarchy nodes when their parent has a language-specific counterpart #1203
Conversation
…counterpart rdar://144862231
|
@swift-ci Please test |
| // Make sure that the clone's children can all line up with symbols from this symbol graph. | ||
| for (childName, children) in sourceNode.children { | ||
| for child in children.storage { | ||
| guard let childSymbol = child.node.symbol else { | ||
| // We shouldn't come across any non-symbol nodes here, | ||
| // but assume they can work as child of both variants. | ||
| clonedSourceNode.add(child: child.node, kind: child.kind, hash: child.hash) | ||
| continue | ||
| } | ||
| if nodes[childSymbol.identifier.precise] === child.node { | ||
| clonedSourceNode.add(symbolChild: child.node) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This block was added in a fit of paranoia, since i already needed to update the symbol data. I'm not sure if this is correct, but if C unions are the only thing that trigger this scenario this code shouldn't be hit in the first place. (Nested types in C create their declarations at top-level, so even declaring a type inline with a name won't create grandchild nodes of the union itself.)
| @@ -558,6 +611,8 @@ extension PathHierarchy { | |||
| ) | |||
| return | |||
| } | |||
|
|
|||
| assert(child.parent == nil, "Nodes that already have a parent should not be added to a different parent.") | |||
There was a problem hiding this comment.
I added this assertion to facilitate testing, but i left it in because tests seemed to pass with it present. I could take it out if you think it's a problem.
| ) { | ||
| self.symbol = source.symbol | ||
| self.name = source.name | ||
| self.children = children ?? source.children |
There was a problem hiding this comment.
Would it be possible to perform the deep copy recursively in this initializer instead? I believe that would be easier to follow and it would support more than one level of copying.
There was a problem hiding this comment.
I would still need to filter out the nodes that don't match up with the current symbol graph, which isn't accessible in this initializer. I also can't remove children after the fact since children's setter is marked private. (Unless i add a remove method, i suppose.)
There was a problem hiding this comment.
I thought you'd still be able to filter the list before assigning it but I haven't tried. Anyway, this is all private code so we can revisit it later.
There was a problem hiding this comment.
It would be possible to add a filtering component to the init(cloning:) initializer, but then the filtering check would be moved up into the initializer call. I did wind up tweaking the code layout to perform some of the cleanup in that call, but i feel like splitting the loop and the graph inclusion check would be Too Weird for me. Not sure. Maybe there's a better way to do it than what i'm thinking about.
| @@ -148,7 +148,48 @@ struct PathHierarchy { | |||
| // would require that we redundantly create multiple nodes for the same symbol in many common cases and then merge them. To avoid doing that, we instead check | |||
| // the source symbol's path components to find the correct target symbol by matching its name. | |||
| if let targetNode = nodes[relationship.target], targetNode.name == expectedContainerName { | |||
| targetNode.add(symbolChild: sourceNode) | |||
| if sourceNode.parent == nil || sourceNode.parent === targetNode { | |||
There was a problem hiding this comment.
We could do nothing if the target already is the parent of the source, otherwise we might hit the added assertion.
|
@swift-ci Please test |
|
@swift-ci Please test |
…counterpart (swiftlang#1203) rdar://144862231
Bug/issue #, if applicable: rdar://144862231
Summary
When C unions are imported into Swift, they are converted into structs with computed properties to preserve union semantics. That means that the same symbol has different symbol kinds depending on the source language. This causes problems for the path hierarchy when associating those fields/properties with their parent, since there are now two separate hierarchy nodes that they could be associated with. This causes Swift-DocC to trigger a number of assertions due to the inconsistent state. (In release builds when debug assertions are compiled out, the issue is papered over with backup code, but the issue can still cause problems as evidenced with the need for workarounds like #1200.)
This PR updates the
PathHierarchyinitializer to clone children of these split parents to ensure that the hierarchy tree remains in a consistent state.Dependencies
None
Testing
This is a catalog that contains Clang and Swift symbol graphs for a simple union with one field: DemoFramework.docc.zip
Steps:
docc convert DemoFramework.doccChecklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded