adds support to correctly identify snippet symbolgraphs

[heckj]

• expanded documentatin on mergeSymbolGraph to describe the forceLoading
  parameter.
• updated the logic to use the lack of @ and the isVirtual metadata
  to determine if a symbolgraph is the primary graph for that module.

resolves #88

Snippet extension graphs being considered primary is the source of non-deterministic results in previewing documentation that include snippets (swiftlang/swift-docc#1084). For the logic within swift-dock's SymbolGraphLoader, they should be considered extension graphs. Since the snippet symbol graph extractor doesn't generate an @ in the file name of the graph, the logic is amended to look at the isVirtual metadata for the module, which is set to true for snippets.

I considered using the name generated by the snippet extractor, or doing some heuristic on the name the generator metadata, but after staring at the data for a while, the SymbolGraph.module.isVirtual seemed a more exact and lasting means of determining if a given symbol graph is "the main one" or not.

[d-ronnqvist]

LGTM

[d-ronnqvist]

@swift-ci please test

[QuietMisdreavus]

This behavior is somewhat surprising to me; if snippets graphs are meant to be handled as "auxiliary" symbol graphs to their main module, wouldn't it make more sense for them to report "A" as their module name? That way, Swift-DocC (and the GraphCollector) can correctly sort them into their main module's graph.

[heckj]

Honestly, I didn't pay a huge amount of attention to the name, as the name of the module returned didn't play into the issue I was debugging, the isMain Boolean value is what reflects the issue. Because of that, I didn't touch anything in the name logic (or see what it would impact), only tweaking the isMain logic.

The test, when I created it, was based on what snippet-extractor actually dumps (or a short-form version of it), and explicitly covered what the name returns, just to get that explicitly tested. I wasn't sure of the intention of that logic, and there weren't any tests working that internal bit of name tweaking to know how (or if) I should update it.

For the specific exposure through GraphCollector, the graphs aren't indexed by name - but the name is sourced from the graph structure itself, and not the name of the URL of the file that houses the symbolGraph.

[heckj]

(I'm more than happy to try and change that logic if you like, but I've zero sense of "what it should be", and don't have any reference to the radar thats mentioned in the code to reference the bits that do get manipulated)

[QuietMisdreavus]

The GraphCollector uses this method to determine which module to merge symbol graphs into:

swift-docc-symbolkit/Sources/SymbolKit/UnifiedSymbolGraph/GraphCollector.swift

Lines 61 to 64 in 96bce1c

	public func mergeSymbolGraph(_ inputGraph: SymbolGraph, at url: URL, forceLoading: Bool = false) {
	let (extendedModuleName, isMainSymbolGraph) = Self.moduleNameFor(inputGraph, at: url)
	let moduleName = extensionGraphAssociationStrategy == .extendedGraph ? extendedModuleName : inputGraph.module.name

This is what allows extensions to be sorted with their target type rather than being sorted in the module that declared them. Does Swift-DocC handle snippets graphs differently to look for a ModuleName-snippets symbol graph? If it doesn't, this PR as-is will cause snippets to be dropped entirely.

[heckj]

If that were the case, I think it would be dropping snippets entirely now - as the name logic that's being returned hasn't changed. The flow is a little woolly and tangled, but the way it seems to end up working is that the graph is identified as belonging to the correct module, as it's working today - and I didn't change anything in the name part of this flow - the test that's surprising is just working the way it always did and i'm showing it (that part wasn't explicitly tested by itself).

That said, I'll pull all the pieces together (make a quick check that uses the updated branch here with swift-docc) to verify that it works as expected, and I'll see if I can ferret out why the existing naming structure of the symbolgraph file that snippets generates today doesn't cause things to go awry.

[heckj]

I did just double check that this change won't break things (yet to explain why) - I did the export SWIFTCI_USE_LOCAL_DEPS=true and ran through my test scenario, which is previewing the docs consistently and including all the snippets in the flow, so at least I'm confident that the change does fix the issue and does not break the code entirely.

[heckj]

Digging back through all the Callers (in Docc using SymbolKit at least) of moduleNameFor(_:at:). The callers are

• GraphCollect.mergeSymbolGraph(_:at:forceLoading:)
• SymbolGraphLoader.loadAll()
• SymbolGraphLoader.loadSymbolGraph(at:)
• UnifiedSymbolGraph.init(fromSingleGraph:at:)
• UnifiedSymbolGraph.merge(graph:at:)

The two UnifiedSymbolGraph discard the returned name entirely, only working with “is this the main graph” information.

The GraphCollector.mergeSymbolGraph (GraphCollector.swift) captures the name as “extendedModuleName” and then only uses that as the “moduleName” based on the extensionGraphAssociationStrategy (only when it’s .extendedGraph). The alternative takes the name from the inputGraph internal data (inputGraph.module.name).

• Digging this back, the extensionGraphAssociation defaults to .extendedGraph

The SymbolGraph loader (SymbolGraphLoader.swift) loadAll() uses the name to invoke addDefaultAvailability, and if it’s not a “main” module, to invoke ExtendedTypeFormatTransformation.transformExtensionBlockFormatToExtendedTypeFormat(&symbolGraph, moduleName: moduleName), but otherwise doesn’t use the name - as it tends to reference the symbol graphs by URL.

The other place SymbolGraph loader uses is (loadSymbolGraph(at:)) captures the name, and changes the name IN the symbol graph only if the graph isn’t a main module and has no bystanders.

Based on all this, I think the name happens to be slipping through the cracks, but your sense of “hey, that’s likely wrong” is correct in that it could easily break some assumptions elsewhere in the code. So while it doesn’t break anything now, it’s not “correct” as it stands for snippets.

…

The logic in moduleNameFor uses returns the name for the module from the symbol graph itself (graph.module.name), and only futzes with the name of the URL in the case of an extension. What I’m uncertain of is what the correct logic is here.

My sample code has an extension onto some core Swift types, and as I looked through the symbol graph, it’s module.name was correct, so I’m unclear on when it isn’t correct and should be pulled form the name of the URL. The parsing code references cross-import overlays, with an example name that shows cascading frameworks, and also assumes that the name will always be in the form Framework1@Framework2. The snippets extractor writes out the name of the graph as “Framework1-snippets.symbols.json”.

Based on that, I could put in some additional logic to check the form and go from there.

What do you think of a special case looking for “-snippets” in that name, and in that case returning the value from the graph itself (graph.module.name?)

[heckj]

(added my guess-work "fix" as a commit) - let me know what you think, and if there's a different way you'd prefer to approach this

It seems like there's more discussion to be had about potentially better ways to solve this.